feat(python): stabilize SDK lifecycle contract - #26
Conversation
WalkthroughReplaces singular profile handling with ordered profiles, renames harness_type to harness, adds runtime_binding to runtime handles, introduces runtime capabilities and unsupported routing, and ships a native-first Python SDK with typed models, sessions, errors, docs, and updated tests. ChangesSDK MVP: profiles, harness, capabilities, runtime binding, and Python SDK
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FabricClient
participant _native
participant Session
Caller->>FabricClient: start_session(agent, profiles, session_id)
FabricClient->>_native: resolve / plan
_native-->>FabricClient: RunPlan JSON
FabricClient->>_native: start_runtime(...)
_native-->>FabricClient: RuntimeHandle JSON
FabricClient-->>Caller: Session
Caller->>Session: invoke(input, overrides)
Session->>_native: invoke_runtime(...)
_native-->>Session: RunResult JSON
Session-->>Caller: RunResult
Caller->>Session: stop()
Session->>_native: stop_runtime(...)
_native-->>Session: ok
Session-->>Caller: stopped
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
43626e4 to
bb8a3b2
Compare
|
@coderabbitai can you review |
|
✅ Action performedReview finished.
|
AjayThorve
changed the title
abee70e to
f48af92
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@crates/fabric-core/src/config.rs`:
- Around line 1071-1089: The session capability is being enabled for all
Process/Python adapters in RuntimeCapabilities::resolve_runtime_capabilities,
but it must also be gated by the runtime transport. Update
resolve_runtime_capabilities so capabilities.session is only true when
runtime.mode is Session, the adapter kind is Process or Python, and the
transport is actually supported for sessions; keep unsupported transports like
http and native_plugin from reporting session support. Use the existing
FabricConfig.runtime and AdapterDescriptor checks in
resolve_runtime_capabilities to enforce this capability gate.
In `@python/src/nemo_fabric/client.py`:
- Around line 312-315: The exception handling in the client startup path drops
the structured stage when wrapping native session startup failures. Update the
relevant wrapper in the Fabric client startup flow so that the `start_runtime`
failure is re-raised as a `FabricRuntimeError` while preserving the normalized
structured failure metadata, especially the `stage`, consistent with the other
SDK error paths. Use the `FabricError`/`FabricRuntimeError` handling in
`client.py` to locate the fix.
In `@python/src/nemo_fabric/session.py`:
- Around line 66-67: Session is always serializing invocations by storing a
single _current_task, which breaks RunPlan.capabilities.concurrent_invocations.
Update Session.invoke and stop handling to allow multiple overlapping turns when
the capability is enabled, while still rejecting overlaps only when it is
disabled; replace the single-task bookkeeping in Session with tracking for all
active tasks so stop() can affect every in-flight invocation. Keep the
async/session behavior aligned with the native extension and review the related
Session methods around invoke and stop for the required parity.
- Around line 273-281: The _json_mapping helper is allowing non-string mapping
keys to be silently coerced by json.dumps, which mutates session override shape.
Update _json_mapping in session.py to validate that all keys are strings before
the JSON round-trip, and raise FabricConfigError for any non-string key while
keeping the existing JSON-compatible value check and error handling. Use the
existing _json_mapping symbol and FabricConfigError path so the public JSON
contract stays consistent with the native extension.
In `@python/src/nemo_fabric/types.py`:
- Around line 443-454: The _freeze helper currently returns _ConfigMapping
objects unchanged, which lets mutable config state leak into FabricMapping
snapshots and makes EffectiveConfig.config and to_mapping() non-defensive.
Update _freeze so _ConfigMapping is copied into an immutable or detached
structure just like other mutable mappings, and make sure the read-only snapshot
behavior is preserved consistently anywhere FabricMapping is built or exposed,
including the related logic around EffectiveConfig and to_mapping().
- Around line 650-657: The request normalization in the `data` builder is
treating any falsy `context` value as missing by using `context or {}`, which
silently turns invalid inputs like empty lists into empty mappings. Update the
`types.py` request assembly so only `None` falls back to `{}` and all other
values are passed through to `_mapping(context, "request context")` for
validation, matching the behavior already used for `overrides` and
`extra_fields`.
In `@python/tests/smoke_sdk_sessions.py`:
- Around line 190-195: The stopped-session assertion in the smoke test is
catching the wrong exception type, which is broader than the API contract.
Update the `session.invoke()` error check in `smoke_sdk_sessions.py` to catch
`FabricStateError` instead of `RuntimeError`, matching the behavior already
verified in `test_session.py` and ensuring `Session.invoke()` surfaces the typed
SDK error.
- Around line 211-220: The smoke test in failed_result_exposes_structured_error
only asserts the structured failed RunResult and never exercises cleanup. After
the existing failure assertions, explicitly stop the session via the session
object returned by _session/native flow so this test also covers the error-path
lifecycle cleanup and can catch regressions in stop() after a failed invoke.
In `@python/tests/smoke_typed_config.py`:
- Around line 163-175: The CLI-vs-SDK smoke parity check in the typed config
test only validates a subset of the plan shape, so it can miss drift in the
newly stabilized fields. Update the parity assertions in the helper around
client.plan and _cli_plan to compare the new public fields as well, especially
adapter_descriptor, harness_type, and top-level capabilities, in addition to the
existing config and plan sections. Keep the assertions aligned with the SDK
plan’s to_mapping output so this smoke test covers the full changed API surface.
In `@README.md`:
- Around line 200-208: The README example currently catches FabricError in the
FabricClient usage example and immediately wraps it in RuntimeError, which drops
the SDK’s structured contract. Update the example around FabricClient.run to
handle FabricError directly: inspect or log its structured fields like stage,
code, retryable, and details, then re-raise the same FabricError unchanged or
otherwise handle it without converting it. Keep the guidance aligned with the
stable Python SDK contract so readers see FabricError as the normalized failure
type.
In `@tests/test_session.py`:
- Around line 282-293: Extend the one-shot lifecycle test in
test_run_stops_runtime_after_success_and_failure to cover the stop-cleanup
failure path as well as the invoke failure path. Use the existing native_client
and mock_native setup around FabricClient.run and _run_native_lifecycle, then
make stop_runtime raise an error after a successful invoke_runtime and assert
that run still raises FabricRuntimeError with the cleanup failure message. Keep
the existing assertions that stop_runtime is called after both success and
failure.
🪄 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: 2c1abed4-cb0a-4bc3-a195-93fcc67e2778
📒 Files selected for processing (44)
.coderabbit.yamlPOC-TO-MVP-PLAN.mdREADME.mdadapters/hermes-cli/fabric-adapter.jsonadapters/hermes-sdk/fabric-adapter.jsoncrates/fabric-cli/src/main.rscrates/fabric-core/src/config.rscrates/fabric-core/src/doctor.rscrates/fabric-core/src/error.rscrates/fabric-core/src/lib.rscrates/fabric-core/src/runtime.rscrates/fabric-python/src/lib.rsdocs/python-sdk-contract.mdpython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/_native.pyipython/src/nemo_fabric/client.pypython/src/nemo_fabric/errors.pypython/src/nemo_fabric/integrations/harbor.pypython/src/nemo_fabric/session.pypython/src/nemo_fabric/types.pypython/tests/smoke_environment_handle.pypython/tests/smoke_harbor_integration.pypython/tests/smoke_native_sdk.pypython/tests/smoke_readme_examples.pypython/tests/smoke_sdk.pypython/tests/smoke_sdk_concurrency.pypython/tests/smoke_sdk_sessions.pypython/tests/smoke_typed_config.pyschemas/adapter-descriptor.schema.jsonschemas/adapter-invocation.schema.jsonschemas/agent.schema.jsonschemas/effective-config.schema.jsonschemas/profile.schema.jsonschemas/run-plan.schema.jsonschemas/run-result.schema.jsonschemas/runtime-handle.schema.jsontests/fixtures/hermes-shim-agent/adapters/hermes-shim/fabric-adapter.jsontests/smoke_cli.pytests/smoke_hermes_session.pytests/test_hermes_cli.pytests/test_hermes_cli_preflight.pytests/test_sdk_contract.pytests/test_session.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
{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_preflight.pypython/tests/smoke_environment_handle.pytests/fixtures/hermes-shim-agent/adapters/hermes-shim/fabric-adapter.jsontests/smoke_cli.pypython/tests/smoke_harbor_integration.pypython/tests/smoke_sdk_concurrency.pytests/smoke_hermes_session.pypython/tests/smoke_sdk.pytests/test_hermes_cli.pypython/tests/smoke_readme_examples.pytests/test_sdk_contract.pypython/tests/smoke_native_sdk.pypython/tests/smoke_sdk_sessions.pytests/test_session.pypython/tests/smoke_typed_config.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/_native.pyipython/src/nemo_fabric/integrations/harbor.pypython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/errors.pypython/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/session.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/types.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-cli/fabric-adapter.jsonadapters/hermes-sdk/fabric-adapter.json
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/error.rscrates/fabric-core/src/doctor.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/runtime-handle.schema.jsonschemas/adapter-descriptor.schema.jsonschemas/run-result.schema.jsonschemas/profile.schema.jsonschemas/effective-config.schema.jsonschemas/agent.schema.jsonschemas/adapter-invocation.schema.jsonschemas/run-plan.schema.json
crates/fabric-python/**/*
⚙️ CodeRabbit configuration file
crates/fabric-python/**/*: Treat native binding changes as public API changes. Check JSON/type conversion, error propagation, GIL/thread behavior, and parity with the Python SDK.
Files:
crates/fabric-python/src/lib.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/python-sdk-contract.md
🧬 Code graph analysis (13)
tests/test_hermes_cli_preflight.py (1)
python/src/nemo_fabric/_native.pyi (1)
run(25-32)
python/tests/smoke_sdk_concurrency.py (2)
python/src/nemo_fabric/_native.pyi (1)
run(25-32)python/src/nemo_fabric/integrations/harbor.py (1)
run(70-99)
crates/fabric-core/src/error.rs (1)
crates/fabric-core/src/config.rs (1)
FabricError(16-16)
tests/test_hermes_cli.py (1)
python/src/nemo_fabric/_native.pyi (1)
run(25-32)
python/tests/smoke_readme_examples.py (1)
python/src/nemo_fabric/_native.pyi (2)
plan(13-13)doctor(19-19)
python/src/nemo_fabric/session.py (3)
python/src/nemo_fabric/client.py (1)
Session(317-323)python/src/nemo_fabric/_native.pyi (4)
plan(13-13)start_runtime(42-42)invoke_runtime(43-47)stop_runtime(48-48)python/tests/smoke_sdk_sessions.py (1)
stop_runtime(132-134)
python/tests/smoke_native_sdk.py (1)
python/src/nemo_fabric/_native.pyi (2)
plan(13-13)run(25-32)
crates/fabric-core/src/doctor.rs (1)
crates/fabric-core/src/config.rs (5)
ResolutionStrategy(275-290)AdapterKind(453-462)RuntimeMode(522-529)Transport(534-543)resolve_run_plan(755-758)
tests/test_session.py (1)
python/src/nemo_fabric/_native.pyi (5)
plan(13-13)plan_config(14-18)run(25-32)invoke_runtime(43-47)stop_runtime(48-48)
python/tests/smoke_typed_config.py (2)
tests/smoke_cli.py (1)
run(181-193)python/src/nemo_fabric/_native.pyi (3)
plan(13-13)doctor(19-19)run(25-32)
python/src/nemo_fabric/client.py (5)
python/src/nemo_fabric/_native.pyi (7)
inspect(7-7)plan_config(14-18)doctor_config(20-24)start_runtime(42-42)plan(13-13)doctor(19-19)run(25-25)python/tests/smoke_native_sdk.py (2)
resolve(17-17)FabricClient(23-23)python/tests/smoke_sdk_sessions.py (2)
resolve(15-15)run(233-233)tests/smoke_cli.py (5)
resolve(17-17)plan(36-36)doctor(98-98)run(172-172)run(181-181)tests/test_hermes_cli.py (1)
FabricClient(13-13)
python/src/nemo_fabric/types.py (1)
crates/fabric-core/src/doctor.rs (1)
check(456-463)
crates/fabric-core/src/config.rs (1)
crates/fabric-cli/src/main.rs (4)
serde_json(167-167)RunPlan(20-20)resolve_effective_config_with_profiles(22-22)resolve_effective_config_with_profiles(135-135)
🪛 ast-grep (0.44.0)
python/src/nemo_fabric/_config_sources.py
[info] 75-75: 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)
[info] 81-81: use jsonify instead of json.dumps for JSON output
Context: json.dumps([profile.to_mapping() for profile in profiles])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_sdk_contract.py
[info] 473-473: Do not hardcode temporary file or directory names
Context: "/tmp/relay"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 482-482: Do not hardcode temporary file or directory names
Context: "/tmp/relay"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 287-287: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 292-292: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan()["effective_config"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 301-301: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan()["effective_config"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 310-310: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 314-314: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_runtime())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 323-353: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"agent_name": "demo",
"profiles": ["typed"],
"harness_type": "hermes",
"adapter_kind": "python",
"adapter_id": "test.fabric.shim",
"runtime_id": json.loads(runtime_json)["runtime_id"],
"invocation_id": "invocation-1",
"request_id": request["request_id"],
"status": "failed" if request["input"] == "fail" else "succeeded",
"output": {"received": request["input"]},
"error": {
"stage": "invoke",
"code": "adapter_failed",
"message": "adapter failed",
"retryable": False,
}
if request["input"] == "fail"
else None,
"artifacts": {"artifacts": []},
"events": [
{
"event_id": "event-1",
"timestamp_millis": 1,
"kind": "invocation_end",
"message": "completed",
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 357-357: use jsonify instead of json.dumps for JSON output
Context: json.dumps([])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
python/src/nemo_fabric/session.py
[info] 149-149: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._plan.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 150-150: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._runtime.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 151-151: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 236-236: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._plan.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 237-237: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._runtime.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 278-278: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dict(value))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 361-361: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dict(plan))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 363-363: use jsonify instead of json.dumps for JSON output
Context: json.dumps(runtime)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 369-369: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dict(request))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_session.py
[info] 99-99: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 101-101: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 103-103: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_runtime())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 109-129: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"agent_name": "demo",
"profiles": ["typed"],
"harness_type": "hermes",
"adapter_kind": "python",
"adapter_id": "test.fabric.shim",
"runtime_id": "runtime-1",
"invocation_id": f"invocation-{turn}",
"request_id": request["request_id"],
"status": "succeeded",
"output": {
"messages": [
{"role": "user", "content": request["input"]},
{"role": "assistant", "content": f"reply-{turn}"},
]
},
"artifacts": {"artifacts": []},
"events": [],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 132-132: use jsonify instead of json.dumps for JSON output
Context: json.dumps([])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 178-178: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan("oneshot"))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
python/tests/smoke_typed_config.py
[error] 194-200: Use of unsanitized data to create processes
Context: subprocess.run(
args,
cwd=ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 194-200: Command coming from incoming request
Context: subprocess.run(
args,
cwd=ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
python/src/nemo_fabric/client.py
[info] 309-309: use jsonify instead of json.dumps for JSON output
Context: json.dumps(plan.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
docs/python-sdk-contract.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...ces, resolution, planning, diagnostics, oneshot runs, sessions, typed results and error...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.15.18)
python/tests/smoke_sdk.py
[warning] 53-53: Avoid specifying long messages outside the exception class
(TRY003)
python/src/nemo_fabric/_config_sources.py
[warning] 21-21: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 25-25: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 29-32: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 33-33: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 40-40: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 43-43: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 53-55: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 58-61: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 68-68: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 75-75: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_sdk_contract.py
[warning] 285-285: Dynamically typed expressions (typing.Any) are disallowed in profile
(ANN401)
[warning] 290-290: Dynamically typed expressions (typing.Any) are disallowed in profile
(ANN401)
[warning] 298-298: Unused method argument: profiles_json
(ARG002)
[warning] 299-299: Unused method argument: base_dir
(ARG002)
[warning] 307-307: Unused method argument: profiles_json
(ARG002)
[warning] 308-308: Unused method argument: base_dir
(ARG002)
[warning] 318-318: Unused method argument: plan_json
(ARG002)
[warning] 321-321: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 356-356: Unused method argument: plan_json
(ARG002)
[warning] 356-356: Unused method argument: runtime_json
(ARG002)
[warning] 369-369: Unused method argument: method
(ARG002)
[error] 474-474: Probable insecure usage of temporary file or directory: "/tmp/relay"
(S108)
[error] 483-483: Probable insecure usage of temporary file or directory: "/tmp/relay"
(S108)
[warning] 671-671: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 679-679: Unused method argument: mode
(ARG002)
[warning] 679-679: Unused method argument: exclude_none
(ARG002)
[warning] 684-684: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 694-694: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
python/src/nemo_fabric/session.py
[warning] 49-49: Dynamically typed expressions (typing.Any) are disallowed in client
(ANN401)
[error] 112-112: Function argument input is shadowing a Python builtin
(A002)
[warning] 112-112: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 121-121: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 123-123: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 125-125: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 158-158: Consider moving this statement to an else block
(TRY300)
[error] 169-169: Function argument input is shadowing a Python builtin
(A002)
[warning] 169-169: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 192-192: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 194-198: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 199-203: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 209-213: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 214-218: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 226-226: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 228-228: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 230-230: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 266-266: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 277-277: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 281-281: Avoid specifying long messages outside the exception class
(TRY003)
[error] 300-300: Function argument input is shadowing a Python builtin
(A002)
[warning] 300-300: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 315-317: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 320-322: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 327-327: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 339-339: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 357-357: Dynamically typed expressions (typing.Any) are disallowed in native
(ANN401)
[warning] 394-394: Dynamically typed expressions (typing.Any) are disallowed in func
(ANN401)
[warning] 394-394: Dynamically typed expressions (typing.Any) are disallowed in _call_blocking
(ANN401)
[warning] 399-402: Use contextlib.suppress(Exception) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(Exception): ...
(SIM105)
[error] 401-402: try-except-pass detected, consider logging the exception
(S110)
[warning] 401-401: Do not catch blind exception: Exception
(BLE001)
[warning] 409-413: Avoid specifying long messages outside the exception class
(TRY003)
python/tests/smoke_sdk_sessions.py
[warning] 141-141: Unused method argument: method
(ARG002)
[warning] 184-184: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 195-195: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 206-206: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_session.py
[warning] 100-100: Unused lambda argument: path
(ARG005)
[warning] 100-100: Unused lambda argument: profiles
(ARG005)
[warning] 102-102: Unused lambda argument: config_json
(ARG005)
[warning] 102-102: Unused lambda argument: profiles_json
(ARG005)
[warning] 102-102: Unused lambda argument: base_dir
(ARG005)
[warning] 106-106: Unused function argument: plan_json
(ARG001)
[warning] 106-106: Unused function argument: runtime_json
(ARG001)
[warning] 179-179: Unused lambda argument: path
(ARG005)
[warning] 179-179: Unused lambda argument: profiles
(ARG005)
[warning] 241-241: Missing return type annotation for private function blocking
(ANN202)
[warning] 265-265: Missing return type annotation for private function blocking
(ANN202)
python/tests/smoke_typed_config.py
[error] 195-195: subprocess call: check for execution of untrusted input
(S603)
python/src/nemo_fabric/client.py
[warning] 58-58: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 216-216: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 232-232: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[error] 247-247: Function argument input is shadowing a Python builtin
(A002)
[warning] 333-333: Dynamically typed expressions (typing.Any) are disallowed in start_service
(ANN401)
[warning] 344-344: Dynamically typed expressions (typing.Any) are disallowed in start_service
(ANN401)
[warning] 353-353: Unused method argument: overrides
(ARG002)
[warning] 358-363: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 365-365: Dynamically typed expressions (typing.Any) are disallowed in _native_module
(ANN401)
[warning] 368-368: Dynamically typed expressions (typing.Any) are disallowed in _require_native_module
(ANN401)
python/src/nemo_fabric/types.py
[warning] 25-25: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 25-25: Dynamically typed expressions (typing.Any) are disallowed in _plain
(ANN401)
[warning] 36-36: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 42-42: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 45-45: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 48-48: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 50-50: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 54-54: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 56-56: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 60-60: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 62-62: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 71-71: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 89-91: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 98-98: Dynamically typed expressions (typing.Any) are disallowed in __getattr__
(ANN401)
[warning] 106-106: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 148-148: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 180-180: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 208-208: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 217-217: Use a dictionary comprehension instead of a for-loop
(PERF403)
[warning] 221-221: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 263-263: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 308-308: Dynamically typed expressions (typing.Any) are disallowed in tools
(ANN401)
[warning] 335-335: Use a dictionary comprehension instead of a for-loop
(PERF403)
[warning] 339-339: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 342-342: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 344-344: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 393-393: Dynamically typed expressions (typing.Any) are disallowed in tools
(ANN401)
[warning] 421-421: Use a dictionary comprehension instead of a for-loop
(PERF403)
[warning] 425-425: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 443-443: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 443-443: Dynamically typed expressions (typing.Any) are disallowed in _freeze
(ANN401)
[warning] 457-457: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 457-457: Dynamically typed expressions (typing.Any) are disallowed in _thaw
(ANN401)
[warning] 481-481: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 488-488: Dynamically typed expressions (typing.Any) are disallowed in __getitem__
(ANN401)
[warning] 497-497: Dynamically typed expressions (typing.Any) are disallowed in __getattr__
(ANN401)
[error] 644-644: Function argument input is shadowing a Python builtin
(A002)
[warning] 644-644: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 660-662: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 667-667: Remove quotes from type annotation
Remove quotes
(UP037)
🔇 Additional comments (19)
.coderabbit.yaml (1)
85-101: 📐 Maintainability & Code QualityMove
toolsto the top level if CodeRabbit expects top-level analyzer config.The
toolsblock is still indented underreviews, unlikeknowledge_baseat the top level. Iftoolsis only recognized there, these analyzer settings will be ignored.crates/fabric-python/src/lib.rs (1)
10-11: LGTM!Also applies to: 40-54, 221-221
python/src/nemo_fabric/_native.pyi (1)
7-11: LGTM!python/src/nemo_fabric/errors.py (1)
13-49: LGTM!python/src/nemo_fabric/_config_sources.py (1)
21-82: LGTM!python/src/nemo_fabric/__init__.py (1)
6-75: LGTM!python/src/nemo_fabric/client.py (1)
55-270: LGTM!Also applies to: 294-311, 324-376
crates/fabric-core/src/error.rs (1)
112-125: LGTM!crates/fabric-core/src/runtime.rs (2)
10-11: LGTM!Also applies to: 26-27, 59-97, 228-246, 334-345, 426-453, 555-563, 603-697, 1147-1152, 1161-1183, 1721-1721, 1985-2205
508-553: 🩺 Stability & AvailabilityRemove this concern
RunPlanhas noHashMap/HashSetfields in the serialized graph, and this workspace’sserde_jsonbuild doesn’t enablepreserve_order, so the binding hash remains deterministic.> Likely an incorrect or invalid review comment.crates/fabric-core/src/doctor.rs (1)
13-16: LGTM!Also applies to: 46-54, 58-75, 117-129, 131-170, 204-221, 378-386, 493-495, 529-541, 569-611
python/tests/smoke_native_sdk.py (1)
112-120: 📐 Maintainability & Code QualityNo action needed here; the typed
FabricConfig/FabricProfileConfigcalls already matchFabricClient.resolve()andplan()’s public overloads.> Likely an incorrect or invalid review comment.schemas/adapter-invocation.schema.json (1)
115-121: LGTM!Also applies to: 166-170, 234-247, 256-256, 376-376, 463-463, 493-493, 522-522, 569-569, 590-590, 629-629, 708-739, 847-847, 861-861, 978-981
schemas/agent.schema.json (1)
19-19: LGTM!Also applies to: 87-87, 117-117, 146-146, 170-170, 191-191, 230-230, 284-315, 340-340, 354-354, 416-416
schemas/effective-config.schema.json (1)
19-204: LGTM!Also applies to: 233-317, 371-402, 427-441, 524-535
schemas/profile.schema.json (1)
3-3: LGTM!Also applies to: 13-42, 52-57, 67-83
schemas/run-plan.schema.json (1)
4-4: LGTM!Also applies to: 25-63, 109-109, 151-151, 226-232, 277-281, 348-358, 367-367, 487-487, 574-574, 604-604, 633-633, 680-680, 701-701, 740-740, 821-896, 921-921, 935-935, 1062-1065, 1075-1078, 1108-1111, 1142-1143
adapters/hermes-sdk/fabric-adapter.json (1)
3-3: LGTM!tests/fixtures/hermes-shim-agent/adapters/hermes-shim/fabric-adapter.json (1)
3-3: LGTM!
799d964 to
a573d8d
Compare
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
7a96c68 to
0b812fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fabric-cli/src/main.rs (1)
448-452: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't synthesize a
defaultprofile name from an empty stack.
RunPlan.profilesis now the canonical identity surface. Returning"default"here invents a profile that is not present in the plan, so the CLI can drift from SDK-visible state and mask bugs where the ordered stack is dropped upstream.Suggested fix
fn profile_label(plan: &RunPlan) -> String { - if !plan.profiles.is_empty() { - return plan.profiles.join(", "); - } - "default".to_string() + if plan.profiles.is_empty() { + return "[]".to_string(); + } + plan.profiles.join(", ") }🤖 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-cli/src/main.rs` around lines 448 - 452, The profile_label helper is inventing a fallback profile name when RunPlan.profiles is empty, which can hide upstream state issues. Update profile_label to derive the label only from the actual plan.profiles stack and avoid returning a synthetic "default" value; if the stack is empty, propagate that emptiness consistently in the CLI output. Use the profile_label function and RunPlan.profiles as the key symbols when making the change.
🤖 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 `@crates/fabric-core/src/config.rs`:
- Around line 376-403: The generated profile schema is missing explicit overlay
fields for metadata and profiles, so they currently fall into extensions as
arbitrary JSON instead of first-class object-or-null sections. Update
ProfileConfigSchema in config.rs to add optional metadata and profiles members
alongside harness, models, runtime, environment, tools, skills, mcp, and
telemetry, and ensure the schema generation reflects those as documented
overlays rather than flattened extras. Then verify any schema snapshots, tests,
and docs that rely on the public profile.schema.json contract are updated to
include the new explicit sections.
In `@docs/python-sdk-contract.md`:
- Around line 158-160: Clarify the defaulting behavior in the contract text so
it matches FabricConfig.from_mapping and FabricConfig: when environment is
omitted, the typed config remains environment=None and the local environment is
applied later during resolution. Update the sentence around the defaults in this
section to distinguish mutable config construction from resolution-time
defaults, and keep the wording aligned with the current SDK behavior and
generated schemas.
- Around line 451-452: The stream contract wording is backwards: in the
documented `stream()` behavior, clarify that adapters may buffer and callers
cannot assume immediate or unbuffered event emission before the terminal
`RunResult`. Update the description in the `stream()` section so it accurately
states that event delivery timing is not guaranteed, while keeping the notes
about additive event kinds and metadata consistent with the current API and
generated schemas.
In `@python/src/nemo_fabric/client.py`:
- Line 258: The async lifecycle methods are calling the synchronous native
planning path directly on the event loop, unlike doctor(). Update Client.run(),
Client.start_session(), and Client.start_service() to invoke self.plan(...) via
_call_blocking so planning is offloaded consistently. Keep the existing
plan(agent, profiles=profiles, base_dir=base_dir) call shape, but wrap the
native planning work in the same blocking helper pattern used by doctor() to
preserve parity with the native extension and avoid blocking async execution.
In `@python/src/nemo_fabric/session.py`:
- Around line 261-264: The message handling in session.py is too strict: in the
block that assigns self._messages from result.output["messages"], dict(message)
will crash if messages contains non-mapping JSON values like strings. Update the
logic in this section of the session/session result processing to only convert
mapping entries to dicts and safely ignore or preserve non-mapping items instead
of assuming every element is a mapping.
- Around line 146-164: The invoke path in Session.invoke currently re-raises
native/runtime parsing failures while leaving the session state unchanged, which
can leave a dead handle marked ACTIVE. Update the exception handling around
native.invoke_runtime and the json.loads/RunResult.from_mapping flow so
request-validation FabricError cases remain state-preserving, but native invoke
or parsing failures transition the Session to FAILED before raising
FabricRuntimeError. Use the Session.invoke method and Session.status/_absorb
behavior to locate the fix.
In `@python/src/nemo_fabric/types.py`:
- Around line 611-620: The normalizers are still masking missing or malformed
profiles by defaulting to an empty tuple, which breaks the required
ordered-profiles contract. Update the affected normalization paths in
`FabricConfig._normalize` and the related helpers at the other referenced
locations so they validate that `profiles` is present and already ordered, and
raise on missing/invalid input instead of substituting `[]`. Keep the existing
`Path` and `FabricConfig.from_mapping` handling, but remove the permissive
fallback and apply the same fail-fast behavior consistently across effective
config, run result, and session identity normalization.
- Around line 814-833: RuntimeHandle is still using the default FabricMapping
parsing path, so invalid or empty mappings can slip through and fail later; add
explicit validation in RuntimeHandle.from_mapping (or an equivalent constructor
override) to require runtime_id, runtime_binding, agent_name, harness, mode,
adapter_kind, and adapter_id shape before creating the object. Make this
rejection raise FabricConfigError, and keep the runtime_binding/native-boundary
checks aligned with the existing RuntimeHandle symbol so malformed inputs are
rejected at this boundary rather than accepted as arbitrary mappings.
- Around line 200-233: RuntimeConfig currently does not materialize the
documented stable defaults, leaving transport, input_schema, and output_schema
unset in RuntimeConfig.__init__ and RuntimeConfig.from_mapping. Update the
RuntimeConfig constructor so these fields default to "library", "text", and
"text" when not provided, while still allowing explicit overrides and preserving
extra_fields handling; use the existing RuntimeConfig symbols to keep the public
Python surface aligned with the runtime schema defaults.
In `@python/tests/smoke_native_sdk.py`:
- Around line 140-160: The smoke test in `test_smoke_native_sdk` only validates
`run()` output and session behavior, so it can miss regressions in the
stabilized `RunResult` API. Add assertions on the returned `RunResult` from
`client.run` to verify the new `profiles` and `harness` fields (and that legacy
`profile`/`harness_type` are not the expected surface), using the existing
`result` variable in this test to anchor the checks.
In `@python/tests/smoke_sdk_concurrency.py`:
- Around line 23-29: Promote the concurrency smoke scenario in
smoke_sdk_concurrency.py from the standalone main() coroutine into a
pytest-discovered async test. Replace main() with an async test function (using
the existing FabricClient and run_agent helpers) so pytest can collect and
report failures normally, and keep the async test unannotated since the async
runner detects it automatically.
In `@python/tests/smoke_sdk_sessions.py`:
- Around line 227-231: The smoke test file currently uses a single `main()`
aggregator to run `stable_runtime_across_turns`, `stream_and_lifecycle`,
`unsupported_cancel_leaves_session_active`, and
`failed_result_exposes_structured_error`, which hides independent failures from
pytest. Split each scenario into its own pytest test function (or parameterized
pytest case) so pytest can report lifecycle, cancellation, and error-path
regressions separately. Keep the existing scenario helpers if needed, but remove
the `main()` entrypoint and let pytest discover and run the tests directly.
In `@python/tests/smoke_sdk.py`:
- Around line 26-57: Move the negative-path smoke out of the standalone main()
entrypoint and into a pytest test so it runs under test discovery alongside the
other SDK smokes. Use a pytest fixture or context-managed setup in this test to
scope client_mod._native = None only within the test, then restore it afterward
so FabricClient.plan and FabricNativeUnavailableError are exercised without
leaking state into later tests. Keep the existing assertions on FabricConfig,
RunRequest, and the native-missing failure path, but make the test pytest-native
instead of relying on __main__ execution.
In `@python/tests/smoke_typed_config.py`:
- Around line 180-185: The smoke checks are currently bundled behind the main()
script runner, which makes each contract fail as one combined case. Refactor
smoke_typed_config.py to expose sdk_and_cli_profile_stacks_match,
resolves_and_diagnoses_without_a_directory, and runs_without_an_agent_package as
individual pytest tests so CI reports each parity regression separately. Keep
the existing async client usage with FabricClient, but let pytest discover and
run the async tests directly instead of calling main().
In `@schemas/adapter-descriptor.schema.json`:
- Around line 123-126: The adapter descriptor schema still allows an empty
harness string even though validate_adapter_descriptor_shape() in config.rs
rejects blank values. Update the harness property in
adapter-descriptor.schema.json to require a non-empty string by adding a
minLength constraint so the schema matches the runtime validation and the public
contract stays consistent.
In `@tests/test_sdk_contract.py`:
- Around line 305-397: Replace the handwritten native doubles in the test
contract setup with shared pytest fixtures, since NativeRecorder and
NativeClient duplicate the native harness pattern already used in
tests/test_session.py. Move the common native setup into conftest.py as reusable
fixtures, and swap the custom classes for unittest.mock.MagicMock or
AsyncMock-based mocks. Keep the FabricClient integration points (_native_module
and _require_native_module) wired through those fixtures so the contract tests
still exercise the same native paths without duplicating helper classes.
- Around line 44-53: The public contract test for FabricClient.start_service
only checks overloads, but it should verify the actual capability-gated failure
behavior. Add an async test around start_service that calls it on a client
without service support and asserts it raises FabricCapabilityError with the
documented stage, code, and details. Use the existing FabricClient and
FabricCapabilityError symbols so the test covers the promised API behavior, not
just the signature.
---
Outside diff comments:
In `@crates/fabric-cli/src/main.rs`:
- Around line 448-452: The profile_label helper is inventing a fallback profile
name when RunPlan.profiles is empty, which can hide upstream state issues.
Update profile_label to derive the label only from the actual plan.profiles
stack and avoid returning a synthetic "default" value; if the stack is empty,
propagate that emptiness consistently in the CLI output. Use the profile_label
function and RunPlan.profiles as the key symbols when making the change.
🪄 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: 45e5f85b-7912-400f-b284-31b68d2fbebe
📒 Files selected for processing (43)
.coderabbit.yamlPOC-TO-MVP-PLAN.mdREADME.mdadapters/hermes-cli/fabric-adapter.jsonadapters/hermes-sdk/fabric-adapter.jsoncrates/fabric-cli/src/main.rscrates/fabric-core/src/config.rscrates/fabric-core/src/doctor.rscrates/fabric-core/src/lib.rscrates/fabric-core/src/runtime.rscrates/fabric-python/src/lib.rsdocs/python-sdk-contract.mdpython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/_native.pyipython/src/nemo_fabric/client.pypython/src/nemo_fabric/errors.pypython/src/nemo_fabric/integrations/harbor.pypython/src/nemo_fabric/session.pypython/src/nemo_fabric/types.pypython/tests/smoke_environment_handle.pypython/tests/smoke_harbor_integration.pypython/tests/smoke_native_sdk.pypython/tests/smoke_readme_examples.pypython/tests/smoke_sdk.pypython/tests/smoke_sdk_concurrency.pypython/tests/smoke_sdk_sessions.pypython/tests/smoke_typed_config.pyschemas/adapter-descriptor.schema.jsonschemas/adapter-invocation.schema.jsonschemas/agent.schema.jsonschemas/effective-config.schema.jsonschemas/profile.schema.jsonschemas/run-plan.schema.jsonschemas/run-result.schema.jsonschemas/runtime-handle.schema.jsontests/fixtures/hermes-shim-agent/adapters/hermes-shim/fabric-adapter.jsontests/smoke_cli.pytests/smoke_hermes_session.pytests/test_hermes_cli.pytests/test_hermes_cli_preflight.pytests/test_sdk_contract.pytests/test_session.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / 0_Test.txt: feat(python): stabilize SDK lifecycle contract
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 8.28s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
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-039dd9c04deba413)
running 40 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / Test: feat(python): stabilize SDK lifecycle contract
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 8.28s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
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-039dd9c04deba413)
running 40 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (8)
{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/fixtures/hermes-shim-agent/adapters/hermes-shim/fabric-adapter.jsonpython/tests/smoke_environment_handle.pypython/tests/smoke_harbor_integration.pytests/test_hermes_cli_preflight.pytests/test_hermes_cli.pytests/smoke_cli.pytests/smoke_hermes_session.pypython/tests/smoke_sdk_concurrency.pypython/tests/smoke_sdk.pypython/tests/smoke_readme_examples.pypython/tests/smoke_native_sdk.pypython/tests/smoke_typed_config.pypython/tests/smoke_sdk_sessions.pytests/test_sdk_contract.pytests/test_session.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_hermes_cli_preflight.pytests/test_hermes_cli.pytests/smoke_cli.pytests/smoke_hermes_session.pytests/test_sdk_contract.pytests/test_session.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-cli/fabric-adapter.jsonadapters/hermes-sdk/fabric-adapter.json
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/integrations/harbor.pypython/src/nemo_fabric/errors.pypython/src/nemo_fabric/_native.pyipython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/session.pypython/src/nemo_fabric/types.py
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/adapter-descriptor.schema.jsonschemas/runtime-handle.schema.jsonschemas/run-result.schema.jsonschemas/profile.schema.jsonschemas/agent.schema.jsonschemas/effective-config.schema.jsonschemas/adapter-invocation.schema.jsonschemas/run-plan.schema.json
crates/fabric-python/**/*
⚙️ CodeRabbit configuration file
crates/fabric-python/**/*: Treat native binding changes as public API changes. Check JSON/type conversion, error propagation, GIL/thread behavior, and parity with the Python SDK.
Files:
crates/fabric-python/src/lib.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/lib.rscrates/fabric-core/src/doctor.rscrates/fabric-core/src/runtime.rscrates/fabric-core/src/config.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/python-sdk-contract.md
🧠 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/tests/smoke_environment_handle.pypython/tests/smoke_harbor_integration.pypython/src/nemo_fabric/integrations/harbor.pypython/src/nemo_fabric/errors.pypython/tests/smoke_sdk_concurrency.pypython/src/nemo_fabric/__init__.pypython/tests/smoke_sdk.pypython/tests/smoke_readme_examples.pypython/tests/smoke_native_sdk.pypython/tests/smoke_typed_config.pypython/tests/smoke_sdk_sessions.pypython/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/session.pypython/src/nemo_fabric/types.py
🧬 Code graph analysis (2)
crates/fabric-core/src/doctor.rs (1)
crates/fabric-core/src/config.rs (4)
ResolutionStrategy(275-290)AdapterKind(453-462)RuntimeMode(522-529)Transport(534-543)
python/src/nemo_fabric/client.py (1)
crates/fabric-core/src/config.rs (1)
EffectiveConfig(1300-1313)
🪛 ast-grep (0.44.0)
python/tests/smoke_typed_config.py
[error] 196-202: Command coming from incoming request
Context: subprocess.run(
args,
cwd=ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 196-202: Use of unsanitized data to create processes
Context: subprocess.run(
args,
cwd=ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
tests/test_sdk_contract.py
[info] 534-534: Do not hardcode temporary file or directory names
Context: "/tmp/relay"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 543-543: Do not hardcode temporary file or directory names
Context: "/tmp/relay"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 313-313: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 318-318: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan()["effective_config"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 327-327: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan()["effective_config"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 336-336: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 340-340: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_runtime())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 349-379: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"agent_name": "demo",
"profiles": ["typed"],
"harness": "hermes",
"adapter_kind": "python",
"adapter_id": "test.fabric.shim",
"runtime_id": json.loads(runtime_json)["runtime_id"],
"invocation_id": "invocation-1",
"request_id": request["request_id"],
"status": "failed" if request["input"] == "fail" else "succeeded",
"output": {"received": request["input"]},
"error": {
"stage": "invoke",
"code": "adapter_failed",
"message": "adapter failed",
"retryable": False,
}
if request["input"] == "fail"
else None,
"artifacts": {"artifacts": []},
"events": [
{
"event_id": "event-1",
"timestamp_millis": 1,
"kind": "invocation_end",
"message": "completed",
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 383-383: use jsonify instead of json.dumps for JSON output
Context: json.dumps([])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
python/src/nemo_fabric/_config_sources.py
[info] 75-75: 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)
[info] 81-81: use jsonify instead of json.dumps for JSON output
Context: json.dumps([profile.to_mapping() for profile in profiles])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
python/src/nemo_fabric/client.py
[info] 311-311: use jsonify instead of json.dumps for JSON output
Context: json.dumps(plan.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_session.py
[info] 100-100: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 102-102: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 104-104: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_runtime())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 110-130: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"agent_name": "demo",
"profiles": ["typed"],
"harness": "hermes",
"adapter_kind": "python",
"adapter_id": "test.fabric.shim",
"runtime_id": "runtime-1",
"invocation_id": f"invocation-{turn}",
"request_id": request["request_id"],
"status": "succeeded",
"output": {
"messages": [
{"role": "user", "content": request["input"]},
{"role": "assistant", "content": f"reply-{turn}"},
]
},
"artifacts": {"artifacts": []},
"events": [],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 133-133: use jsonify instead of json.dumps for JSON output
Context: json.dumps([])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 179-179: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan("oneshot"))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
python/src/nemo_fabric/session.py
[info] 149-149: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._plan.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 150-150: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._runtime.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 151-151: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 236-236: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._plan.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 237-237: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._runtime.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 293-293: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dict(value), allow_nan=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 376-376: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dict(plan))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 378-378: use jsonify instead of json.dumps for JSON output
Context: json.dumps(runtime)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 384-384: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dict(request))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
docs/python-sdk-contract.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...ces, resolution, planning, diagnostics, oneshot runs, sessions, typed results and error...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.15.20)
python/tests/smoke_sdk.py
[warning] 53-53: Avoid specifying long messages outside the exception class
(TRY003)
python/tests/smoke_typed_config.py
[error] 197-197: subprocess call: check for execution of untrusted input
(S603)
python/tests/smoke_sdk_sessions.py
[warning] 142-142: Unused method argument: method
(ARG002)
[warning] 185-185: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 196-196: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 207-207: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_sdk_contract.py
[warning] 311-311: Dynamically typed expressions (typing.Any) are disallowed in profile
(ANN401)
[warning] 316-316: Dynamically typed expressions (typing.Any) are disallowed in profile
(ANN401)
[warning] 324-324: Unused method argument: profiles_json
(ARG002)
[warning] 325-325: Unused method argument: base_dir
(ARG002)
[warning] 333-333: Unused method argument: profiles_json
(ARG002)
[warning] 334-334: Unused method argument: base_dir
(ARG002)
[warning] 344-344: Unused method argument: plan_json
(ARG002)
[warning] 347-347: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 382-382: Unused method argument: plan_json
(ARG002)
[warning] 382-382: Unused method argument: runtime_json
(ARG002)
[warning] 395-395: Unused method argument: method
(ARG002)
[error] 535-535: Probable insecure usage of temporary file or directory: "/tmp/relay"
(S108)
[error] 544-544: Probable insecure usage of temporary file or directory: "/tmp/relay"
(S108)
[warning] 732-732: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 740-740: Unused method argument: mode
(ARG002)
[warning] 740-740: Unused method argument: exclude_none
(ARG002)
[warning] 745-745: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 755-755: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
python/src/nemo_fabric/_config_sources.py
[warning] 21-21: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 25-25: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 29-32: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 33-33: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 40-40: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 43-43: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 53-55: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 58-61: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 68-68: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 75-75: Avoid specifying long messages outside the exception class
(TRY003)
python/src/nemo_fabric/client.py
[warning] 59-59: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 217-217: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 233-233: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[error] 248-248: Function argument input is shadowing a Python builtin
(A002)
[warning] 335-335: Dynamically typed expressions (typing.Any) are disallowed in start_service
(ANN401)
[warning] 346-346: Dynamically typed expressions (typing.Any) are disallowed in start_service
(ANN401)
[warning] 355-355: Unused method argument: overrides
(ARG002)
[warning] 360-365: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 367-367: Dynamically typed expressions (typing.Any) are disallowed in _native_module
(ANN401)
[warning] 370-370: Dynamically typed expressions (typing.Any) are disallowed in _require_native_module
(ANN401)
tests/test_session.py
[warning] 101-101: Unused lambda argument: path
(ARG005)
[warning] 101-101: Unused lambda argument: profiles
(ARG005)
[warning] 103-103: Unused lambda argument: config_json
(ARG005)
[warning] 103-103: Unused lambda argument: profiles_json
(ARG005)
[warning] 103-103: Unused lambda argument: base_dir
(ARG005)
[warning] 107-107: Unused function argument: plan_json
(ARG001)
[warning] 107-107: Unused function argument: runtime_json
(ARG001)
[warning] 180-180: Unused lambda argument: path
(ARG005)
[warning] 180-180: Unused lambda argument: profiles
(ARG005)
[warning] 280-280: Missing return type annotation for private function blocking
(ANN202)
[warning] 304-304: Missing return type annotation for private function blocking
(ANN202)
python/src/nemo_fabric/session.py
[warning] 49-49: Dynamically typed expressions (typing.Any) are disallowed in client
(ANN401)
[error] 112-112: Function argument input is shadowing a Python builtin
(A002)
[warning] 112-112: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 121-121: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 123-123: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 125-125: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 158-158: Consider moving this statement to an else block
(TRY300)
[error] 169-169: Function argument input is shadowing a Python builtin
(A002)
[warning] 169-169: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 192-192: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 194-198: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 199-203: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 209-213: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 214-218: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 226-226: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 228-228: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 230-230: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 266-266: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 277-277: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 289-289: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 296-296: Avoid specifying long messages outside the exception class
(TRY003)
[error] 315-315: Function argument input is shadowing a Python builtin
(A002)
[warning] 315-315: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 330-332: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 335-337: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 342-342: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 354-354: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 372-372: Dynamically typed expressions (typing.Any) are disallowed in native
(ANN401)
[warning] 409-409: Dynamically typed expressions (typing.Any) are disallowed in func
(ANN401)
[warning] 409-409: Dynamically typed expressions (typing.Any) are disallowed in _call_blocking
(ANN401)
[warning] 414-417: Use contextlib.suppress(Exception) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(Exception): ...
(SIM105)
[error] 416-417: try-except-pass detected, consider logging the exception
(S110)
[warning] 416-416: Do not catch blind exception: Exception
(BLE001)
[warning] 424-428: Avoid specifying long messages outside the exception class
(TRY003)
python/src/nemo_fabric/types.py
[warning] 25-25: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 25-25: Dynamically typed expressions (typing.Any) are disallowed in _plain
(ANN401)
[warning] 36-36: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 42-42: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 45-45: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 48-48: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 50-50: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 54-54: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 56-56: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 60-60: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 62-62: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 71-71: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 89-91: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 98-98: Dynamically typed expressions (typing.Any) are disallowed in __getattr__
(ANN401)
[warning] 106-106: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 148-148: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 183-183: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 211-211: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 220-220: Use a dictionary comprehension instead of a for-loop
(PERF403)
[warning] 224-224: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 272-272: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 317-317: Dynamically typed expressions (typing.Any) are disallowed in tools
(ANN401)
[warning] 348-348: Use a dictionary comprehension instead of a for-loop
(PERF403)
[warning] 352-352: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 355-355: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 357-357: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 406-406: Dynamically typed expressions (typing.Any) are disallowed in tools
(ANN401)
[warning] 434-434: Use a dictionary comprehension instead of a for-loop
(PERF403)
[warning] 438-438: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 456-456: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 456-456: Dynamically typed expressions (typing.Any) are disallowed in _freeze
(ANN401)
[warning] 470-470: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 470-470: Dynamically typed expressions (typing.Any) are disallowed in _thaw
(ANN401)
[warning] 484-484: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 484-484: Dynamically typed expressions (typing.Any) are disallowed in _snapshot_value
(ANN401)
[warning] 504-504: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 511-511: Dynamically typed expressions (typing.Any) are disallowed in __getitem__
(ANN401)
[warning] 523-523: Dynamically typed expressions (typing.Any) are disallowed in __getattr__
(ANN401)
[error] 686-686: Function argument input is shadowing a Python builtin
(A002)
[warning] 686-686: Dynamically typed expressions (typing.Any) are disallowed in input
(ANN401)
[warning] 708-710: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 715-715: Remove quotes from type annotation
Remove quotes
(UP037)
🔇 Additional comments (23)
python/src/nemo_fabric/client.py (1)
4-209: LGTM!Also applies to: 259-272, 306-306, 308-378
python/src/nemo_fabric/session.py (1)
1-145: LGTM!Also applies to: 166-260, 266-429
python/src/nemo_fabric/integrations/harbor.py (1)
171-172: LGTM!adapters/hermes-cli/fabric-adapter.json (1)
3-3: LGTM!adapters/hermes-sdk/fabric-adapter.json (1)
3-3: LGTM!tests/fixtures/hermes-shim-agent/adapters/hermes-shim/fabric-adapter.json (1)
3-3: LGTM!crates/fabric-python/src/lib.rs (1)
40-54: 🎯 Functional CorrectnessNo SDK leak here
FabricClient.resolve()wraps_native.resolve_config()failures intoFabricConfigError, so the public Python API does not expose the binding’s genericPyRuntimeError.> Likely an incorrect or invalid review comment.python/tests/smoke_readme_examples.py (1)
23-41: LGTM!Also applies to: 76-87
python/tests/smoke_harbor_integration.py (1)
96-97: LGTM!Also applies to: 151-151
tests/smoke_cli.py (1)
60-60: LGTM!Also applies to: 139-139
tests/smoke_hermes_session.py (1)
78-89: LGTM!Also applies to: 105-116
tests/test_hermes_cli.py (1)
18-22: LGTM!Also applies to: 53-59, 95-96, 108-111, 157-157
tests/test_hermes_cli_preflight.py (1)
31-35: LGTM!schemas/adapter-invocation.schema.json (1)
115-121: LGTM!Also applies to: 166-170, 237-247, 256-256, 376-376, 463-463, 493-493, 522-522, 569-569, 590-590, 629-629, 708-739, 847-847, 861-861, 978-981
crates/fabric-core/src/runtime.rs (1)
59-65: LGTM!Also applies to: 228-236, 334-344, 413-558, 599-665, 766-770, 893-900, 971-975, 1104-1110, 1142-1177, 1708-1720, 1884-1891, 1978-2199
crates/fabric-core/src/lib.rs (1)
18-19: LGTM!crates/fabric-core/src/doctor.rs (1)
46-73: LGTM!Also applies to: 102-169, 204-220, 378-385, 493-611
schemas/agent.schema.json (1)
19-19: LGTM!Also applies to: 87-87, 117-117, 146-146, 170-170, 191-191, 230-230, 284-315, 340-340, 354-354, 416-416
schemas/effective-config.schema.json (1)
19-204: LGTM!Also applies to: 233-317, 371-402, 427-441, 524-535
schemas/profile.schema.json (1)
2-3: LGTM!Also applies to: 13-42, 52-57, 67-83
schemas/run-plan.schema.json (1)
4-4: LGTM!Also applies to: 25-25, 41-44, 63-63, 109-109, 151-151, 226-232, 277-281, 348-358, 367-367, 487-487, 574-574, 604-604, 633-633, 680-680, 701-701, 740-740, 821-865, 876-876, 885-896, 921-921, 935-935, 1062-1065, 1108-1111, 1142-1143
schemas/run-result.schema.json (1)
271-272: LGTM!Also applies to: 288-293, 321-322
schemas/runtime-handle.schema.json (1)
153-156: LGTM!Also applies to: 171-177
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_sdk_contract.py (1)
520-535: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse contract-valid
RunResultfixtures in these tests.Both cases build
RunResult.from_mapping(...)payloads without required identity fields such asharness,adapter_kind,runtime_id, andinvocation_id. That locks in acceptance of result snapshots that the published contract does not allow. Either populate the missing fields here or flip these to rejection tests so the Python model stays aligned with the documented/native schema. As per path instructions,schemas/run-result.schema.jsonrequires orderedprofilesand a requiredharness, and test coverage should reflect the changed API contract.Also applies to: 547-557
🤖 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_sdk_contract.py` around lines 520 - 535, Use contract-valid RunResult fixtures in the affected RunResult.from_mapping tests: the current payloads are missing required identity fields like harness, adapter_kind, runtime_id, and invocation_id, so they should either be updated to include those fields or rewritten as rejection cases. Update the fixtures around test_run_result_wraps_nested_error_and_keeps_mapping_access and the other matching test so they align with RunResult and the published run-result schema contract.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/src/nemo_fabric/_config_sources.py`:
- Around line 37-45: Reject raw mapping inputs in path_profiles() by adding an
explicit mapping-type check before the final list(profiles) fallback, so
dict-like profile values raise FabricConfigError instead of being treated as
keys. Update the path_profiles function in _config_sources to handle the
existing None, str, and bytes cases, then reject mappings from
client.plan/profile parsing paths so mixed bare-string/raw-mapping profile
inputs do not reach native code.
In `@python/src/nemo_fabric/client.py`:
- Around line 361-364: The start_service method in client.py is receiving
overrides but never uses or validates it before the capability error path, so
wire the parameter into the service startup flow or explicitly validate/reject
unsupported overrides there; make sure the logic in start_service either applies
overrides to the request/config construction or checks them and raises a clear
error before continuing, using the existing start_service signature and related
service/client setup symbols to place the fix.
---
Outside diff comments:
In `@tests/test_sdk_contract.py`:
- Around line 520-535: Use contract-valid RunResult fixtures in the affected
RunResult.from_mapping tests: the current payloads are missing required identity
fields like harness, adapter_kind, runtime_id, and invocation_id, so they should
either be updated to include those fields or rewritten as rejection cases.
Update the fixtures around
test_run_result_wraps_nested_error_and_keeps_mapping_access and the other
matching test so they align with RunResult and the published run-result schema
contract.
🪄 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: 27fada6b-e923-48fe-8e00-15b288c73c2c
📒 Files selected for processing (12)
crates/fabric-core/src/config.rscrates/fabric-core/src/schema.rsdocs/python-sdk-contract.mdpython/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/session.pypython/src/nemo_fabric/types.pypython/tests/smoke_native_sdk.pyschemas/adapter-descriptor.schema.jsonschemas/run-plan.schema.jsontests/test_sdk_contract.pytests/test_session.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / Test: feat(python): stabilize SDK lifecycle contract
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 8.72s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
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-039dd9c04deba413)
running 41 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: feat(python): stabilize SDK lifecycle contract
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 8.72s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
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-039dd9c04deba413)
running 41 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (6)
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/schema.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/adapter-descriptor.schema.jsonschemas/run-plan.schema.json
{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:
python/tests/smoke_native_sdk.pytests/test_sdk_contract.pytests/test_session.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/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/types.pypython/src/nemo_fabric/session.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_sdk_contract.pytests/test_session.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:
docs/python-sdk-contract.md
🧠 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/tests/smoke_native_sdk.pypython/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/types.pypython/src/nemo_fabric/session.py
🧬 Code graph analysis (5)
tests/test_sdk_contract.py (2)
python/src/nemo_fabric/types.py (2)
from_mapping(234-243)to_mapping(552-557)python/src/nemo_fabric/errors.py (2)
FabricCapabilityError(45-46)FabricConfigError(33-34)
tests/test_session.py (2)
python/src/nemo_fabric/session.py (1)
SessionStatus(36-41)python/src/nemo_fabric/errors.py (3)
FabricCapabilityError(45-46)FabricStateError(41-42)FabricRuntimeError(37-38)
python/src/nemo_fabric/types.py (2)
crates/fabric-core/src/config.rs (2)
EffectiveConfig(690-690)RunPlan(757-757)crates/fabric-core/src/schema.rs (2)
RuntimeHandle(141-141)RunResult(144-144)
python/src/nemo_fabric/session.py (1)
python/src/nemo_fabric/errors.py (2)
FabricError(14-30)FabricRuntimeError(37-38)
crates/fabric-core/src/config.rs (1)
crates/fabric-core/src/schema.rs (2)
AdapterDescriptor(60-60)AdapterDescriptor(13-13)
🪛 ast-grep (0.44.0)
tests/test_sdk_contract.py
[info] 364-364: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 369-369: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan()["effective_config"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_session.py
[info] 265-265: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"input": "hello", "request_id": "request-1"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 270-270: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
python/src/nemo_fabric/session.py
[info] 150-150: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._plan.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 151-151: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._runtime.to_mapping())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 152-152: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.20)
python/src/nemo_fabric/_config_sources.py
[warning] 43-43: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_sdk_contract.py
[warning] 213-221: Wrong values type in pytest.mark.parametrize expected list
Use list for parameter values
(PT007)
[warning] 233-238: Wrong values type in pytest.mark.parametrize expected list of tuple
Use list of tuple for parameter values
(PT007)
[warning] 362-362: Dynamically typed expressions (typing.Any) are disallowed in profile
(ANN401)
[warning] 367-367: Dynamically typed expressions (typing.Any) are disallowed in profile
(ANN401)
[warning] 822-822: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
tests/test_session.py
[warning] 385-385: Missing return type annotation for private function record_plan
(ANN202)
[warning] 385-385: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
[warning] 385-385: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
python/src/nemo_fabric/client.py
[warning] 364-364: Unused method argument: overrides
(ARG002)
python/src/nemo_fabric/types.py
[warning] 62-62: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 65-65: Avoid specifying long messages outside the exception class
(TRY003)
python/src/nemo_fabric/session.py
[warning] 273-273: Remove quotes from type annotation
Remove quotes
(UP037)
🔇 Additional comments (8)
crates/fabric-core/src/schema.rs (1)
227-233: LGTM!python/tests/smoke_native_sdk.py (1)
153-163: LGTM!python/src/nemo_fabric/client.py (1)
71-88: LGTM!Also applies to: 118-135, 165-182, 216-247, 259-263, 283-326, 340-378, 383-391
python/src/nemo_fabric/types.py (1)
60-66: LGTM!Also applies to: 622-648, 677-677, 831-860, 902-951
python/src/nemo_fabric/session.py (1)
146-163: LGTM!Also applies to: 271-278
schemas/run-plan.schema.json (1)
4-4: LGTM!Also applies to: 25-65, 111-153, 350-360, 369-489, 576-742, 823-898, 923-937
crates/fabric-core/src/config.rs (1)
121-125: LGTM!tests/test_sdk_contract.py (1)
355-435: Reuse the sharedMagicMockfixtures instead of custom native doubles.
NativeRecorderandNativeClientstill duplicate the native test setup already covered elsewhere. As per coding guidelines, "When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock," "Prefer pytest fixtures over helper methods," and "Do not repeat fixtures across test files; if a fixture is needed in multiple test files, place it inconftest.py."Source: Coding guidelines
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
|
Addressed the latest outside-diff RunResult fixture finding in 92307df. The affected fixtures now include every schema-required identity field, and RunResult.from_mapping() has regression coverage that rejects missing required identities at the Python boundary. Local validation: 115 passed, 4 skipped; 41 Rust tests passed; all 14 dependency-free smoke scripts passed. |
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_sdk_contract.py`:
- Around line 347-363: The shared _run_result() helper should be converted to a
pytest fixture-backed builder to match the repo’s test conventions. Replace the
local helper with a fixture that returns a callable for constructing the run
result, and update any tests that use _run_result() to request and call that
fixture instead. Keep the existing default fields and update-overrides behavior
intact while moving the setup into pytest fixture style.
🪄 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: b7a419b8-17ed-4aa6-8d22-7ee548ec8d87
📒 Files selected for processing (4)
python/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/types.pytests/test_sdk_contract.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
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/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/types.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_sdk_contract.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_sdk_contract.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/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/types.py
🧬 Code graph analysis (4)
python/src/nemo_fabric/_config_sources.py (1)
python/src/nemo_fabric/client.py (2)
path_profiles(106-106)path_profiles(23-23)
tests/test_sdk_contract.py (3)
python/src/nemo_fabric/errors.py (1)
FabricConfigError(33-34)python/src/nemo_fabric/client.py (1)
start_service(337-345)crates/fabric-core/src/runtime.rs (1)
RunResult(60-98)
python/src/nemo_fabric/client.py (1)
python/src/nemo_fabric/session.py (1)
_json_mapping(282-305)
python/src/nemo_fabric/types.py (2)
python/src/nemo_fabric/client.py (1)
RunResult(49-49)tests/test_sdk_contract.py (1)
RunResult(35-35)
🪛 ast-grep (0.44.0)
tests/test_sdk_contract.py
[info] 594-594: Do not hardcode temporary file or directory names
Context: "/tmp/relay"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 Ruff (0.15.20)
python/src/nemo_fabric/_config_sources.py
[warning] 45-45: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_sdk_contract.py
[warning] 347-347: Dynamically typed expressions (typing.Any) are disallowed in **updates
(ANN401)
[error] 595-595: Probable insecure usage of temporary file or directory: "/tmp/relay"
(S108)
[warning] 610-618: Wrong values type in pytest.mark.parametrize expected list
Use list for parameter values
(PT007)
🔇 Additional comments (4)
python/src/nemo_fabric/_config_sources.py (1)
37-50: LGTM!tests/test_sdk_contract.py (1)
539-550: LGTM!Also applies to: 565-569, 591-599, 608-625, 719-726, 874-883
python/src/nemo_fabric/client.py (1)
357-379: LGTM!python/src/nemo_fabric/types.py (1)
900-932: LGTM!
| def _run_result(**updates: Any) -> dict[str, Any]: | ||
| result = { | ||
| "agent_name": "demo", | ||
| "profiles": [], | ||
| "harness": "hermes", | ||
| "adapter_kind": "python", | ||
| "adapter_id": "test.fabric.shim", | ||
| "runtime_id": "runtime-1", | ||
| "invocation_id": "invocation-1", | ||
| "request_id": "request-1", | ||
| "status": "succeeded", | ||
| "output": None, | ||
| "artifacts": {"artifacts": []}, | ||
| "events": [], | ||
| } | ||
| result.update(updates) | ||
| return result |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer a fixture-backed result factory here.
_run_result() is shared test setup. Converting it to a pytest fixture that returns a builder keeps this file aligned with the repo’s test conventions. As per coding guidelines, "Prefer pytest fixtures over helper methods."
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 347-347: Dynamically typed expressions (typing.Any) are disallowed in **updates
(ANN401)
🤖 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_sdk_contract.py` around lines 347 - 363, The shared _run_result()
helper should be converted to a pytest fixture-backed builder to match the
repo’s test conventions. Replace the local helper with a fixture that returns a
callable for constructing the run result, and update any tests that use
_run_result() to request and call that fixture instead. Keep the existing
default fields and update-overrides behavior intact while moving the setup into
pytest fixture style.
Source: Coding guidelines
2 participants
Summary
FabricConfiguses orderedFabricProfileConfigoverlays.profile,*_config,start,from_text, and CLI-backedFabricClientconstruction) from the public SDK.stream()contract.Notes
mainafter fix(core): harden runtime contract invariants #25 lands.Test Plan
cargo fmt --all -- --checkcargo test --workspace --lockedcargo check -p fabric-pythonuv run pytest -q(83 passed, 4 skipped)uv run python python/tests/smoke_native_sdk.pyuv run python python/tests/smoke_typed_config.pyuv run python python/tests/smoke_readme_examples.pyuv run python python/tests/smoke_harbor_integration.pypython3 tests/smoke_cli.pySummary by CodeRabbit
profilesend-to-end (config/planning/results) and introduced capability-gated session/runtime update flows viastart_session,RuntimeCapabilities, and structuredRunRequest/RunResult.extensionsand profileoverlaymerging.resolve_configand expanded public types/errors.harness) across outputs and schemas.