feat: add Claude adapter - #43
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Claude Agent SDK Fabric adapter with normalized configuration mapping, MCP and skill support, runtime-keyed session resume, structured results and failures, package/CI integration, comprehensive tests, and installation and usage documentation. ChangesClaude Agent SDK Adapter
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FabricRuntime
participant ClaudeAdapter
participant RuntimeArtifacts
participant ClaudeAgentSDK
FabricRuntime->>ClaudeAdapter: invoke Fabric payload
ClaudeAdapter->>RuntimeArtifacts: load session by runtime_id
ClaudeAdapter->>ClaudeAgentSDK: query with mapped options and optional resume
ClaudeAgentSDK-->>ClaudeAdapter: messages and terminal result
ClaudeAdapter->>RuntimeArtifacts: persist Claude session_id
ClaudeAdapter-->>FabricRuntime: normalized JSON result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
📖 Fern docs preview: https://nvidia-preview-pull-request-43.docs.buildwithfern.com/nemo/fabric |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py`:
- Around line 332-363: The terminal-state mapping in normalize_result currently
relies only on result.is_error, but some Claude SDK termination cases should be
classified from result.subtype as well. Update normalize_result to cross-check
subtype values such as error_max_turns and error_max_budget_usd when setting
completed, failed, and error, while keeping the existing ResultMessage fields
and _json_safe usage intact.
- Around line 153-160: The cwd setting is passed into resolve_cwd and
_resolve_path without the same type check used for cli_path, system_prompt, and
tools, so invalid values can raise an unstructured TypeError. Add an isinstance
guard for settings["cwd"] in resolve_cwd (or inside _resolve_path if that is the
shared validation point) and raise AdapterConfigError with the same structured
error path used by the other validated settings. Make sure any code paths in
resolve_cwd and related path resolution helpers handle non-string/non-Path
values consistently.
- Around line 460-467: `run()` and `main()` currently only convert
`ClaudeAdapterError` into the structured failure response, so unexpected
exceptions from `asyncio.run(run_claude_sdk(...))` or
`common_utils.load_payload()` can escape and crash the process. Update `run()`
to catch a broad exception path and route it through `adapter_failure(...)` with
a clear unexpected-error message, and make `main()` guard both payload loading
and the `run()` call so every failure still emits the normalized JSON contract
instead of a raw traceback.
- Around line 277-293: The runtime state loader in load_claude_session_id
assumes json.loads() returns a dict, so a non-object JSON value can trigger an
uncaught AttributeError on state.get(). Add an explicit type check after parsing
the file and before accessing runtime_id or claude_session_id, and treat
non-dict state the same as other invalid-state cases by raising
AdapterStateError from the caught error. Keep the normalization behavior inside
load_claude_session_id so callers like run() continue to receive the same
invalid-runtime-state handling.
- Around line 36-64: The child environment setup in claude_sdk/adapter.py is
dropping ANTHROPIC_API_KEY unless models.default.api_key_env explicitly matches
it, which breaks the default API-key flow. Update the inherited environment
handling used by child_environment() so ANTHROPIC_API_KEY is preserved by
default, and make sure scoped_environment() still passes it through before
query() is invoked.
In `@crates/fabric-core/src/doctor.rs`:
- Around line 317-343: The any_of file check in doctor.rs is using stricter
semantics than the existing requirements.files validation. Update the any_of
handling in the requirement loop over descriptor.requirements.any_of to match
the plain file requirement behavior by treating any existing path as valid
instead of only regular files. Keep the env_available logic unchanged, and
adjust the file_available branch so descriptor authors get consistent results
regardless of whether the path is a file or directory.
- Around line 519-526: resolve_requirement_path only expands "~" via HOME, so
Windows paths like "~/.claude/.credentials.json" can fail when HOME is unset.
Update resolve_requirement_path to also check USERPROFILE as a fallback when
expanding a leading "~", keeping the existing behavior in the same function and
using the same path.strip_prefix("~") logic.
- Around line 347-378: The `check_python_module` helper in `doctor.rs` currently
conflates a missing/broken Python interpreter with an unavailable module because
`Command::new(&python).status()` is reduced to a boolean. Update this function
to inspect the spawn/status result separately: if the `python` command cannot be
executed, report a distinct `DoctorStatus::Fail` message indicating the
interpreter path from `plan.config.harness.settings.get("python")`,
`FABRIC_PYTHON`, or `python3` could not be started; only report “module not
importable” when the interpreter runs successfully but the import check exits
nonzero.
In `@tests/adapters/test_claude_sdk_adapter.py`:
- Around line 14-21: The test imports are reaching into the private
claude_agent_sdk._errors module instead of using the public package API. Update
the imports in test_claude_sdk_adapter to pull ClaudeSDKError, CLINotFoundError,
CLIConnectionError, ProcessError, CLIJSONDecodeError, and MessageParseError from
claude_agent_sdk directly so the tests match the documented top-level interface
and do not depend on internal module layout.
- Around line 208-239: run_claude_sdk is mutating the process-global os.environ
around query(), which can leak or drop variables across overlapping awaits.
Update the Claude SDK call path in run_claude_sdk and the
query/ClaudeAgentOptions wiring to pass a per-invocation env copy via env=
instead of swapping globals, and keep the resume/session logic intact in
test_run_claude_sdk_resumes_and_persists_session.
🪄 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: 7a52ce1f-29c8-4232-bbdc-bc3424e6932b
⛔ Files ignored due to path filters (2)
adapters/claude-sdk/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/ci_python.ymlREADME.mdadapters/claude-sdk/README.mdadapters/claude-sdk/fabric-adapter.jsonadapters/claude-sdk/pyproject.tomladapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.pycrates/fabric-core/src/config.rscrates/fabric-core/src/doctor.rsdocs/design/adapter-runtime-execution-strategies.mddocs/superpowers/plans/2026-07-08-claude-agent-sdk-adapter.mddocs/superpowers/specs/2026-07-08-claude-agent-sdk-adapter-design.mdjustfilepyproject.tomlschemas/adapter-descriptor.schema.jsonschemas/run-plan.schema.jsontests/adapters/test_claude_sdk_adapter.pytests/e2e/test_claude_sdk.pytests/fixtures/claude-sdk/fake-claude.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
🧰 Additional context used
📓 Path-based instructions (6)
{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/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/README.mdadapters/claude-sdk/pyproject.tomladapters/claude-sdk/fabric-adapter.jsonadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
docs/superpowers/specs/2026-07-08-claude-agent-sdk-adapter-design.mddocs/design/adapter-runtime-execution-strategies.mddocs/superpowers/plans/2026-07-08-claude-agent-sdk-adapter.mdREADME.md
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run and write tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected automatically by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecargument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, place it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
If a fixture is needed for a test but does not return a value or its value is unused, use@pytest.mark.usefixtures.
If you need to modify environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/fixtures/claude-sdk/fake-claude.pytests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/fixtures/claude-sdk/fake-claude.pytests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.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/run-plan.schema.jsonschemas/adapter-descriptor.schema.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/config.rscrates/fabric-core/src/doctor.rs
🧠 Learnings (1)
📚 Learning: 2026-06-29T22:34:52.407Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 27
File: adapters/codex-cli/fabric-adapter.json:13-15
Timestamp: 2026-06-29T22:34:52.407Z
Learning: In NeMo-Fabric adapter manifest files (e.g., `*/fabric-adapter.json`), keep `config.accepts` limited to the top-level Fabric capability sections that `resolve_capability_plan` consumes (such as `models`, `tools`, `mcp`, `skills`, `telemetry`). Do not add adapter-owned `harness.settings` keys to `config.accepts`; `harness.settings` should remain adapter-owned and be passed through unchanged.
Applied to files:
adapters/claude-sdk/fabric-adapter.json
🧬 Code graph analysis (3)
tests/e2e/test_claude_sdk.py (1)
crates/fabric-core/src/doctor.rs (1)
check(529-536)
crates/fabric-core/src/doctor.rs (1)
crates/fabric-core/src/config.rs (1)
RunPlan(1363-1395)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (6)
load_payload(42-48)settings_payload(72-74)environment_payload(68-69)models_payload(77-78)config_root(34-35)runtime_context(51-52)
🪛 ast-grep (0.44.1)
tests/fixtures/claude-sdk/fake-claude.py
[warning] 13-13: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.environ["FAKE_CLAUDE_LOG"], "a", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[info] 14-14: use jsonify instead of json.dumps for JSON output
Context: json.dumps(sys.argv[1:])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 20-29: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "control_response",
"response": {
"subtype": "success",
"request_id": message["request_id"],
"response": {"commands": [], "output_style": "default"},
},
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 36-47: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "assistant",
"message": {
"role": "assistant",
"content": [{"type": "text", "text": "fake Claude response"}],
"model": "claude-test-model",
"usage": {"input_tokens": 1, "output_tokens": 2},
},
"session_id": SESSION_ID,
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 51-64: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "result",
"subtype": "success",
"duration_ms": 10,
"duration_api_ms": 8,
"is_error": False,
"num_turns": 1,
"session_id": SESSION_ID,
"total_cost_usd": 0.001,
"usage": {"input_tokens": 1, "output_tokens": 2},
"result": "fake Claude response",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[info] 304-307: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{"runtime_id": fabric_runtime_id, "claude_session_id": claude_session_id},
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 470-470: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/adapters/test_claude_sdk_adapter.py
[info] 254-254: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
docs/superpowers/plans/2026-07-08-claude-agent-sdk-adapter.md
[uncategorized] ~302-~302: The official name of this software platform is spelled with a capital “H”.
Context: ...endency-sync commands in justfile and .github/workflows/ci_python.yml. - [ ] **Step...
(GITHUB)
[style] ~342-~342: The double modal “required input” is nonstandard (only accepted in certain dialects). Consider “to be input”.
Context: ...Step 1: Add failing tests for required input and option mapping* Add a `claude_pay...
(NEEDS_FIXED)
🪛 Ruff (0.15.20)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[warning] 17-17: Import from collections.abc instead: Iterator, Mapping
Import from collections.abc
(UP035)
[warning] 88-88: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 96-96: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 106-106: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 284-284: Abstract raise to an inner function
(TRY301)
[warning] 284-284: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 287-287: Abstract raise to an inner function
(TRY301)
[warning] 287-287: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 288-288: Consider moving this statement to an else block
(TRY300)
[warning] 314-314: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 314-314: Dynamically typed expressions (typing.Any) are disallowed in _json_safe
(ANN401)
[warning] 365-365: Dynamically typed expressions (typing.Any) are disallowed in **metadata
(ANN401)
[warning] 385-385: Too many return statements (7 > 6)
(PLR0911)
tests/adapters/test_claude_sdk_adapter.py
[warning] 37-37: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 211-211: Missing return type annotation for private function query_result
(ANN202)
[error] 239-239: Possible hardcoded password assigned to: "FABRIC_UNRELATED_SECRET"
(S105)
🔇 Additional comments (25)
docs/design/adapter-runtime-execution-strategies.md (1)
1-217: LGTM!docs/superpowers/plans/2026-07-08-claude-agent-sdk-adapter.md (2)
9-9: Pin version should be double-checked against upstream releases.The plan pins
claude-agent-sdk==0.2.114. Given the project's stated intent to track a specific upstream release closely, verify this exact patch version exists on PyPI/GitHub before treating it as final in the implementation task list.
[dependency_check]
1-1202: LGTM!docs/superpowers/specs/2026-07-08-claude-agent-sdk-adapter-design.md (2)
27-29: Verify the cited upstream tag/date.The spec states this design was checked against
claude-agent-sdkv0.2.114 released July 8, 2026. Since the adapter's dependency pin and behavior contract hinge on this exact version, confirm the tag and release date against the upstream repository before relying on it as the behavior baseline.
[dependency_check]
1-429: LGTM!crates/fabric-core/src/config.rs (1)
315-353: LGTM!New
python_modules/any_offields and theAdapterRequirementAlternativesstruct follow existing serdedefault/skip_serializing_ifconventions in this struct, and are mirrored consistently inschemas/adapter-descriptor.schema.jsonandschemas/run-plan.schema.json.schemas/adapter-descriptor.schema.json (1)
49-131: LGTM!Schema additions (
AdapterRequirementAlternatives,any_of,python_modules) match the correspondingAdapterRequirements/AdapterRequirementAlternativesRust types incrates/fabric-core/src/config.rs, and are covered by the new doctor test.schemas/run-plan.schema.json (1)
110-182: LGTM!Consistent with
adapter-descriptor.schema.jsonandconfig.rs— same field names, types, andrequired: ["name"]on the newAdapterRequirementAlternativesdef.crates/fabric-core/src/doctor.rs (1)
1-8: LGTM!The rest of the
check_requirementsextension and the new test (doctor_checks_python_modules_and_alternative_requirements) are otherwise correct and matchAdapterRequirements/AdapterRequirementAlternativesfromconfig.rs.Also applies to: 223-345, 560-672
adapters/claude-sdk/fabric-adapter.json (2)
13-17: 🗄️ Data Integrity & IntegrationConfirm
~expansion for the credentials-file auth alternative.
files: ["~/.claude/.credentials.json"]relies on doctor-side path expansion. Per referenced schema docs, doctor requirement checks should include "path expansion." Please confirmdoctor.rsexpands~forany_of.filesentries (this file is outside the current review cohort, so it can't be verified here).Source: Path instructions
1-23: LGTM!adapters/claude-sdk/pyproject.toml (1)
1-38: LGTM!adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.py (1)
1-5: LGTM!adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (2)
171-196: 🗄️ Data Integrity & IntegrationConfirm
capability_plan.native.mcp_serversis the correct nesting, and whethermanagedMCP servers should also be mapped.Per the referenced adapter-invocation schema, MCP routing is expressed via
capability_planwithmanaged/native/unsupportedcategories plusmcp_servers/routes. This function only readscapability_plan.native.mcp_servers, ignoring anymanagedsection. Please confirm this is the intended split (adapter owns "native" mapping only) and not a gap versus whatresolve_capability_planinfabric-coreactually emits.
199-201: LGTM!Also applies to: 254-256
tests/adapters/test_claude_sdk_adapter.py (1)
1-282: LGTM!tests/fixtures/claude-sdk/fake-claude.py (1)
1-69: LGTM!tests/e2e/test_claude_sdk.py (2)
65-97: LGTM!
22-38: 🩺 Stability & AvailabilityNo change needed for
tests/fixtures/claude-sdk/fake-claude.py— it is already tracked in git with mode100755, so the SDK can exec it from a fresh checkout.> Likely an incorrect or invalid review comment..github/workflows/ci_python.yml (1)
58-58: LGTM!README.md (1)
108-137: LGTM!adapters/claude-sdk/README.md (2)
38-47: 📐 Maintainability & Code Quality | ⚡ Quick winVerify
model_nameis actually a supportedharness.settingskey.The documented settings list includes
model_name, but the unit-test fixture and assertions (test_build_options_maps_ticket_scopeintests/adapters/test_claude_sdk_adapter.py) derive the model exclusively fromeffective_config.config.models.default.model, with nomodel_nameoverride present anywhere in the payload. If the adapter doesn't actually read aharness.settings.model_namekey, this line is documenting a non-existent config knob.Based on path instructions for
{adapters/**,examples/**}to review adapter documentation for "config/schema consistency."Source: Path instructions
1-119: LGTM!justfile (1)
11-11: LGTM!Also applies to: 47-47, 83-83
pyproject.toml (1)
34-37: LGTM!Also applies to: 50-53, 77-77, 108-108
932f924 to
53eb092
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (5)
269-284: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-dict persisted state raises an uncaught
AttributeError, bypassingAdapterStateErrornormalization.If the state file contains valid JSON that isn't an object (list/scalar),
state.get(...)raisesAttributeError, which isn't in the caught exception tuple — this escapes the structured-failure contract this adapter is meant to guarantee.🛡️ Proposed fix
try: state = json.loads(path.read_text(encoding="utf-8")) + if not isinstance(state, dict): + raise ValueError("state is not an object") if state.get("runtime_id") != fabric_runtime_id:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 269 - 284, The load_claude_session_id function currently assumes json.loads returns a dict, so non-dict JSON can trigger an uncaught AttributeError on state.get and bypass AdapterStateError normalization. Update this function to validate the parsed state before accessing keys in the runtime_state_path/load_claude_session_id flow, and treat any non-mapping JSON as invalid runtime state by raising the same AdapterStateError path used for OSError, ValueError, and JSON decode failures.
35-63: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ANTHROPIC_API_KEYis dropped unlessmodels.default.api_key_envmatches it exactly.
child_environment()only preserves an API key env var when it equals the configuredapi_key_env; a plainANTHROPIC_API_KEY-based setup with nomodelsconfig (or a differentapi_key_env) loses credentials beforequery()runs sincescoped_environment()replacesos.environentirely.🔑 Proposed fix
def child_environment(payload: dict[str, Any]) -> dict[str, str]: values = {name: value for name in INHERITED_ENV_NAMES if (value := os.environ.get(name))} model = _selected_model_config(payload) api_key_env = model.get("api_key_env") if isinstance(api_key_env, str) and api_key_env in os.environ: values[api_key_env] = os.environ[api_key_env] + if "ANTHROPIC_API_KEY" in os.environ: + values.setdefault("ANTHROPIC_API_KEY", os.environ["ANTHROPIC_API_KEY"])Also applies to: 395-407
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 35 - 63, The child environment building in `child_environment()` is dropping `ANTHROPIC_API_KEY` unless it exactly matches `models.default.api_key_env`, which breaks the default Claude SDK credential flow. Update the environment allowlist logic in `INHERITED_ENV_NAMES` and the API-key handling in `scoped_environment()`/`query()` so a standard `ANTHROPIC_API_KEY` is preserved even when `models` config is absent or uses a different `api_key_env`, while still supporting custom configured key names.
450-467: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnhandled exceptions still escape the structured-failure contract.
run()only catchesClaudeAdapterError; any other exception (bugs, theAttributeErrorabove, a malformedshlex.splitin MCP parsing, etc.) propagates out ofasyncio.run(...)uncaught.main()doesn't guardcommon_utils.load_payload()orrun()either, so an unexpected error crashes the process with a raw traceback instead of the normalized JSON output Fabric expects on stdout.🛡️ Proposed fix
def run(payload: dict[str, Any]) -> dict[str, Any]: """Run one Fabric invocation.""" try: return asyncio.run(run_claude_sdk(payload)) except ClaudeAdapterError as error: return adapter_failure(error) + except Exception as error: # noqa: BLE001 - convert unexpected errors to a structured failure + return _failure("claude_adapter_internal_error", "Unexpected adapter error", exception=type(error).__name__)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 450 - 467, The `run()`/`main()` path only normalizes `ClaudeAdapterError`, so unexpected exceptions still escape and bypass the structured-failure JSON contract. Update `run()` in `adapter.py` to catch a broader exception set around `asyncio.run(run_claude_sdk(payload))` and convert any non-`ClaudeAdapterError` into `adapter_failure(...)`, and also guard `common_utils.load_payload()`/`run()` in `main()` so the process always prints normalized failure output before exiting. Use the existing `run`, `main`, `run_claude_sdk`, and `adapter_failure` symbols to keep the fix localized.
152-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
cwdstill lacks the type guard applied to other settings.A non-string/Path
cwd(e.g. int or list) raises an unguardedTypeErrorinstead of the structuredAdapterConfigErrorused elsewhere.🧹 Proposed fix
def resolve_cwd(payload: dict[str, Any]) -> Path: settings = _settings(payload) - if settings.get("cwd") is not None: - return _resolve_path(payload, settings["cwd"]) + cwd = settings.get("cwd") + if cwd is not None: + if not isinstance(cwd, (str, Path)): + raise AdapterConfigError("claude_invalid_configuration", "cwd must be a path") + return _resolve_path(payload, cwd)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 152 - 159, The resolve_cwd helper currently returns _resolve_path(payload, settings["cwd"]) without validating the cwd type, so non-string/Path values can raise an unstructured TypeError. Add the same type guard used by the other settings in this adapter before calling _resolve_path, and raise AdapterConfigError for invalid cwd values so resolve_cwd handles bad input consistently.
324-353: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider cross-checking
result.subtypealongsideis_errorfor defense-in-depth.
subtypeis already captured in the output but not used to drivecompleted/failed. Some SDK versions have had inconsistentis_errorsemantics forerror_max_turns/error_max_budget_usdterminations; branching onsubtypetoo guards against version drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 324 - 353, In normalize_result, the completed/failed flags currently rely only on result.is_error, which can be inconsistent for certain termination subtypes. Update the logic in normalize_result to also treat known error subtypes such as error_max_turns and error_max_budget_usd as failures, using result.subtype alongside result.is_error. Keep the existing error object shape, but derive completed and failed defensively from both result.is_error and result.subtype.crates/fabric-core/src/doctor.rs (1)
320-351: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInterpreter-not-found and module-not-importable still report the same Fail message.
Command::new(&python)....status().is_ok_and(...)collapses a failed-to-spawn interpreter (missingpython3/misconfiguredFABRIC_PYTHON) into the same "module not importable" message as an actual missing module, misleading the diagnosis this check is meant to give.🩺 Proposed fix to distinguish interpreter-missing from module-missing
- let available = Command::new(&python) - .args([ - "-c", - "import importlib.util,sys; sys.exit(0 if importlib.util.find_spec(sys.argv[1]) else 1)", - module, - ]) - .status() - .is_ok_and(|status| status.success()); - if available { - check( - "requirement.python_module", - DoctorStatus::Pass, - format!("Python module `{module}` is importable"), - ) - } else { - check( - "requirement.python_module", - DoctorStatus::Fail, - format!("Python module `{module}` is not importable"), - ) - } + match Command::new(&python) + .args([ + "-c", + "import importlib.util,sys; sys.exit(0 if importlib.util.find_spec(sys.argv[1]) else 1)", + module, + ]) + .status() + { + Ok(status) if status.success() => check( + "requirement.python_module", + DoctorStatus::Pass, + format!("Python module `{module}` is importable"), + ), + Ok(_) => check( + "requirement.python_module", + DoctorStatus::Fail, + format!("Python module `{module}` is not importable"), + ), + Err(_) => check( + "requirement.python_module", + DoctorStatus::Fail, + format!("Python interpreter `{}` could not be executed", python.display()), + ), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fabric-core/src/doctor.rs` around lines 320 - 351, The `check_python_module` function currently treats a failed `Command::new(&python).status()` the same as a missing Python module, so interpreter-not-found and module-not-importable both return the same Fail result. Update the logic around the `python` command execution to detect spawn/lookup failures separately from a nonzero exit status, and make the failure message in `check("requirement.python_module", ...)` distinguish a missing/misconfigured interpreter from an actually absent module.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 `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py`:
- Around line 145-149: `_resolve_path` currently joins any non-absolute path
under `common_utils.config_root(payload)`, so values like
`~/.claude/local/claude` are resolved incorrectly. Update `_resolve_path` (and
any other path resolution sites in the same adapter, including
`ClaudeSdkAdapter` path handling for `cwd`/`cli_path`) to expand user home
references before the absolute-path check, then apply the config-root fallback
only for truly relative paths.
- Around line 19-28: Import the SDK error types from the public claude_agent_sdk
package instead of claude_agent_sdk._errors in the claude_sdk adapter, and
update the import block in adapter.py accordingly using the existing symbols
like ClaudeSDKError, CLINotFoundError, CLIConnectionError, ProcessError, and
CLIJSONDecodeError; before changing MessageParseError, confirm that it is
re-exported at the top level for the pinned claude-agent-sdk version, and if
not, handle it without relying on the private submodule.
In `@tests/adapters/test_claude_sdk_adapter.py`:
- Around line 35-43: Replace the module-level load_claude_adapter() helper and
global adapter initialization with a pytest fixture, preferably session-scoped,
so the adapter module is loaded through pytest rather than at import time.
Update the test setup in test_claude_sdk_adapter.py to use the fixture in the
tests that need the adapter, keeping the importlib-based loading logic inside
the fixture and removing the module-level side effect.
- Around line 66-69: Add the missing return type annotation to the
claude_payload_fixture fixture so it follows the project fixture convention and
clearly indicates it returns a dict payload. Update the fixture definition for
claude_payload_fixture to include the appropriate return type and keep the
existing fixture behavior unchanged.
In `@tests/e2e/test_claude_sdk.py`:
- Around line 1-115: Add e2e coverage in test_claude_sdk for adapter failure
paths and session cleanup, not just the successful resume and doctor checks.
Extend the existing helpers like fabric_config and
test_fabric_session_launches_fresh_processes_and_resumes to exercise a fake
Claude CLI error case (for example, an invocation that returns is_error=True)
and assert the surfaced failure status/artifacts. Also add a session lifecycle
test around FabricClient.start_session/session.invoke that verifies teardown or
cleanup behavior after the session exits.
- Around line 22-61: The plain helper fabric_config should be converted into a
pytest fixture-based factory instead of a module-level helper. Create a fixture
that returns a config-builder callable, and move the current
FabricConfig.from_mapping assembly into that callable so tests can still pass
mode and cli_path per case. Keep the existing behavior in the builder and update
the test_claude_sdk usages to consume the fixture factory rather than calling
the bare helper directly.
- Around line 74-77: The resume-flag check in the Claude SDK e2e test uses an
unguarded `.index("--resume")`, which can fail with an unclear exception; update
the assertion around `arguments[1]` to first verify the `"--resume"` flag is
present and then check the following value equals `SESSION_ID`. Keep the fix in
the `test_claude_sdk.py` test case that reads `claude-args.jsonl` and asserts on
the `arguments` list so failures produce a direct assertion message instead of a
`ValueError`.
- Around line 81-91: The doctor test is asserting the wrong check identifier for
the SDK package. Update `test_doctor_checks_sdk_package` to match the actual
name emitted by `FabricClient.doctor`/`fabric-core` in `doctor.rs`, and keep the
assertion aligned with one of the real requirement check names instead of
`requirement.python_module`.
---
Duplicate comments:
In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py`:
- Around line 269-284: The load_claude_session_id function currently assumes
json.loads returns a dict, so non-dict JSON can trigger an uncaught
AttributeError on state.get and bypass AdapterStateError normalization. Update
this function to validate the parsed state before accessing keys in the
runtime_state_path/load_claude_session_id flow, and treat any non-mapping JSON
as invalid runtime state by raising the same AdapterStateError path used for
OSError, ValueError, and JSON decode failures.
- Around line 35-63: The child environment building in `child_environment()` is
dropping `ANTHROPIC_API_KEY` unless it exactly matches
`models.default.api_key_env`, which breaks the default Claude SDK credential
flow. Update the environment allowlist logic in `INHERITED_ENV_NAMES` and the
API-key handling in `scoped_environment()`/`query()` so a standard
`ANTHROPIC_API_KEY` is preserved even when `models` config is absent or uses a
different `api_key_env`, while still supporting custom configured key names.
- Around line 450-467: The `run()`/`main()` path only normalizes
`ClaudeAdapterError`, so unexpected exceptions still escape and bypass the
structured-failure JSON contract. Update `run()` in `adapter.py` to catch a
broader exception set around `asyncio.run(run_claude_sdk(payload))` and convert
any non-`ClaudeAdapterError` into `adapter_failure(...)`, and also guard
`common_utils.load_payload()`/`run()` in `main()` so the process always prints
normalized failure output before exiting. Use the existing `run`, `main`,
`run_claude_sdk`, and `adapter_failure` symbols to keep the fix localized.
- Around line 152-159: The resolve_cwd helper currently returns
_resolve_path(payload, settings["cwd"]) without validating the cwd type, so
non-string/Path values can raise an unstructured TypeError. Add the same type
guard used by the other settings in this adapter before calling _resolve_path,
and raise AdapterConfigError for invalid cwd values so resolve_cwd handles bad
input consistently.
- Around line 324-353: In normalize_result, the completed/failed flags currently
rely only on result.is_error, which can be inconsistent for certain termination
subtypes. Update the logic in normalize_result to also treat known error
subtypes such as error_max_turns and error_max_budget_usd as failures, using
result.subtype alongside result.is_error. Keep the existing error object shape,
but derive completed and failed defensively from both result.is_error and
result.subtype.
In `@crates/fabric-core/src/doctor.rs`:
- Around line 320-351: The `check_python_module` function currently treats a
failed `Command::new(&python).status()` the same as a missing Python module, so
interpreter-not-found and module-not-importable both return the same Fail
result. Update the logic around the `python` command execution to detect
spawn/lookup failures separately from a nonzero exit status, and make the
failure message in `check("requirement.python_module", ...)` distinguish a
missing/misconfigured interpreter from an actually absent module.
🪄 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: 63851ff7-d374-4055-9646-53599b60a732
⛔ Files ignored due to path filters (2)
adapters/claude-sdk/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/ci_python.ymlREADME.mdadapters/claude-sdk/README.mdadapters/claude-sdk/fabric-adapter.jsonadapters/claude-sdk/pyproject.tomladapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.pycrates/fabric-core/src/config.rscrates/fabric-core/src/doctor.rsjustfilepyproject.tomlschemas/adapter-descriptor.schema.jsonschemas/run-plan.schema.jsontests/adapters/test_claude_sdk_adapter.pytests/e2e/test_claude_sdk.pytests/fixtures/claude-sdk/fake-claude.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
{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/claude-sdk/fabric-adapter.jsonadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/README.mdadapters/claude-sdk/pyproject.tomladapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run and write tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected automatically by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecargument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, place it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
If a fixture is needed for a test but does not return a value or its value is unused, use@pytest.mark.usefixtures.
If you need to modify environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/fixtures/claude-sdk/fake-claude.pytests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/fixtures/claude-sdk/fake-claude.pytests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.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/run-plan.schema.jsonschemas/adapter-descriptor.schema.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/config.rscrates/fabric-core/src/doctor.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.md
🧠 Learnings (1)
📚 Learning: 2026-06-29T22:34:52.407Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 27
File: adapters/codex-cli/fabric-adapter.json:13-15
Timestamp: 2026-06-29T22:34:52.407Z
Learning: In NeMo-Fabric adapter manifest files (e.g., `*/fabric-adapter.json`), keep `config.accepts` limited to the top-level Fabric capability sections that `resolve_capability_plan` consumes (such as `models`, `tools`, `mcp`, `skills`, `telemetry`). Do not add adapter-owned `harness.settings` keys to `config.accepts`; `harness.settings` should remain adapter-owned and be passed through unchanged.
Applied to files:
adapters/claude-sdk/fabric-adapter.json
🧬 Code graph analysis (3)
tests/e2e/test_claude_sdk.py (1)
crates/fabric-core/src/doctor.rs (1)
check(493-500)
crates/fabric-core/src/doctor.rs (1)
crates/fabric-core/src/config.rs (2)
RunPlan(1347-1379)resolve_run_plan(784-787)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (1)
adapters/common/src/nemo_fabric_adapters/common/utils.py (6)
load_payload(42-48)settings_payload(72-74)environment_payload(68-69)models_payload(77-78)config_root(34-35)runtime_context(51-52)
🪛 ast-grep (0.44.1)
tests/fixtures/claude-sdk/fake-claude.py
[warning] 13-13: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.environ["FAKE_CLAUDE_LOG"], "a", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[info] 14-14: use jsonify instead of json.dumps for JSON output
Context: json.dumps(sys.argv[1:])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 20-29: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "control_response",
"response": {
"subtype": "success",
"request_id": message["request_id"],
"response": {"commands": [], "output_style": "default"},
},
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 36-47: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "assistant",
"message": {
"role": "assistant",
"content": [{"type": "text", "text": "fake Claude response"}],
"model": "claude-test-model",
"usage": {"input_tokens": 1, "output_tokens": 2},
},
"session_id": SESSION_ID,
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 51-64: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "result",
"subtype": "success",
"duration_ms": 10,
"duration_api_ms": 8,
"is_error": False,
"num_turns": 1,
"session_id": SESSION_ID,
"total_cost_usd": 0.001,
"usage": {"input_tokens": 1, "output_tokens": 2},
"result": "fake Claude response",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/adapters/test_claude_sdk_adapter.py
[info] 237-237: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[info] 296-299: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{"runtime_id": fabric_runtime_id, "claude_session_id": claude_session_id},
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 460-460: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.20)
tests/adapters/test_claude_sdk_adapter.py
[warning] 37-37: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 194-194: Missing return type annotation for private function query_result
(ANN202)
[error] 222-222: Possible hardcoded password assigned to: "FABRIC_UNRELATED_SECRET"
(S105)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[warning] 17-17: Import from collections.abc instead: Iterator, Mapping
Import from collections.abc
(UP035)
[warning] 87-87: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 95-95: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 105-105: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 276-276: Abstract raise to an inner function
(TRY301)
[warning] 276-276: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 279-279: Abstract raise to an inner function
(TRY301)
[warning] 279-279: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 280-280: Consider moving this statement to an else block
(TRY300)
[warning] 306-306: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 306-306: Dynamically typed expressions (typing.Any) are disallowed in _json_safe
(ANN401)
[warning] 356-356: Dynamically typed expressions (typing.Any) are disallowed in **metadata
(ANN401)
[warning] 375-375: Too many return statements (7 > 6)
(PLR0911)
🔇 Additional comments (19)
.github/workflows/ci_python.yml (1)
58-58: LGTM!justfile (1)
11-11: LGTM!Also applies to: 47-47, 83-83
pyproject.toml (1)
34-37: LGTM!Also applies to: 50-53, 77-77, 108-108
README.md (2)
108-118: LGTM!Also applies to: 134-135
423-426: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the closing
pytestfence.The fenced block is unterminated, so Markdown will treat the rest of the README as code.
Fix
pytest +```As per path instructions, "Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas."
> Likely an incorrect or invalid review comment.Source: Path instructions
adapters/claude-sdk/README.md (1)
1-105: LGTM!tests/adapters/test_claude_sdk_adapter.py (2)
14-21: 📐 Maintainability & Code QualityPrivate
_errorssubmodule import — already flagged.Same concern raised previously: prefer importing exception types from the public
claude_agent_sdktop-level package instead of the private_errorssubmodule. Public docs/README consistently showClaudeSDKError,CLINotFoundError,CLIConnectionError,ProcessError,CLIJSONDecodeErrorimported from the top-level package;MessageParseErrordoesn't appear in those top-level examples, so it's worth confirming it's actually re-exported before switching.
191-222: 🩺 Stability & AvailabilityGlobal
os.environmutation surfaced by this test — already flagged.This test captures
os.environsnapshots insidequery_resultto assert scoping, confirmingrun_claude_sdkswaps the process-global environment aroundquery(). This was already flagged as a major issue in a prior review pass (leaking/losing env vars across overlapping/concurrent invocations); the underlying adapter should pass a per-call env viaClaudeAgentOptions(env=...)rather than mutatingos.environglobally.tests/fixtures/claude-sdk/fake-claude.py (1)
1-69: LGTM!crates/fabric-core/src/config.rs (1)
331-333: LGTM!schemas/adapter-descriptor.schema.json (1)
81-87: LGTM!schemas/run-plan.schema.json (1)
142-148: LGTM!crates/fabric-core/src/doctor.rs (2)
8-8: LGTM!Also applies to: 314-318
531-532: LGTM!Also applies to: 608-628
adapters/claude-sdk/fabric-adapter.json (1)
10-15: LGTM!config.acceptsstays scoped to top-level capability sections andpython_modulesmatches the actual import name.Based on learnings, this complies with the guidance that
config.acceptsshould be limited to top-level Fabric capability sections consumed byresolve_capability_plan, not adapter-ownedharness.settingskeys.adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.py (1)
1-5: LGTM!adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (2)
87-127: LGTM!Also applies to: 170-196, 287-317, 375-393
232-248: 🩺 Stability & AvailabilityKeep
scoped_environment()here.ClaudeAgentOptions.envis merged into the subprocess environment; it does not replace the parentos.environ, so it cannot provide the same isolation as the current process-level swap.> Likely an incorrect or invalid review comment.adapters/claude-sdk/pyproject.toml (1)
26-30: 📐 Maintainability & Code Quality
claude-agent-sdk==0.2.114is a published release
No change needed here; the pin matches an available upstream version.> Likely an incorrect or invalid review comment.
53eb092 to
f2139d7
Compare
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 `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py`:
- Around line 198-200: The discard_stderr helper currently drops all Claude CLI
stderr, so failed runs lose useful diagnostics. Update discard_stderr in the
claude_sdk adapter to append stderr lines into a bounded buffer instead of
no-op, and thread that buffer into the failure payload metadata for
claude_process_failed and claude_sdk_failed outcomes. Keep the buffer truncated
and locate the change around discard_stderr and the normalized
response/failure-building path in the adapter.
🪄 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: 69ad1a32-07c0-480c-9558-d6f2200eeac6
⛔ Files ignored due to path filters (2)
adapters/claude-sdk/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci_python.ymlREADME.mdadapters/claude-sdk/README.mdadapters/claude-sdk/fabric-adapter.jsonadapters/claude-sdk/pyproject.tomladapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.pyjustfilepyproject.tomltests/adapters/test_claude_sdk_adapter.pytests/e2e/test_claude_sdk.pytests/fixtures/claude-sdk/mock-claude-cli.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
{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/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/pyproject.tomladapters/claude-sdk/README.mdadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.pyadapters/claude-sdk/fabric-adapter.json
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run and write tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected automatically by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecargument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, place it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual tests for different input types.
If a fixture is needed for a test but does not return a value or its value is unused, use@pytest.mark.usefixtures.
If you need to modify environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/fixtures/claude-sdk/mock-claude-cli.pytests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/fixtures/claude-sdk/mock-claude-cli.pytests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.py
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
README.md
🧠 Learnings (1)
📚 Learning: 2026-06-29T22:34:52.407Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 27
File: adapters/codex-cli/fabric-adapter.json:13-15
Timestamp: 2026-06-29T22:34:52.407Z
Learning: In NeMo-Fabric adapter manifest files (e.g., `*/fabric-adapter.json`), keep `config.accepts` limited to the top-level Fabric capability sections that `resolve_capability_plan` consumes (such as `models`, `tools`, `mcp`, `skills`, `telemetry`). Do not add adapter-owned `harness.settings` keys to `config.accepts`; `harness.settings` should remain adapter-owned and be passed through unchanged.
Applied to files:
adapters/claude-sdk/fabric-adapter.json
🧬 Code graph analysis (1)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (2)
adapters/common/src/nemo_fabric_adapters/common/utils.py (6)
load_payload(42-48)settings_payload(72-74)environment_payload(68-69)models_payload(77-78)config_root(34-35)runtime_context(51-52)tests/e2e/test_hermes_config_mapping.py (1)
payload(46-77)
🪛 ast-grep (0.44.1)
tests/fixtures/claude-sdk/mock-claude-cli.py
[warning] 13-13: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.environ["MOCK_CLAUDE_CLI_LOG"], "a", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[info] 14-14: use jsonify instead of json.dumps for JSON output
Context: json.dumps(sys.argv[1:])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 20-29: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "control_response",
"response": {
"subtype": "success",
"request_id": message["request_id"],
"response": {"commands": [], "output_style": "default"},
},
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 36-47: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "assistant",
"message": {
"role": "assistant",
"content": [{"type": "text", "text": "mock Claude response"}],
"model": "claude-test-model",
"usage": {"input_tokens": 1, "output_tokens": 2},
},
"session_id": SESSION_ID,
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 51-64: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "result",
"subtype": "success",
"duration_ms": 10,
"duration_api_ms": 8,
"is_error": False,
"num_turns": 1,
"session_id": SESSION_ID,
"total_cost_usd": 0.001,
"usage": {"input_tokens": 1, "output_tokens": 2},
"result": "mock Claude response",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/adapters/test_claude_sdk_adapter.py
[info] 234-234: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[info] 296-299: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{"runtime_id": fabric_runtime_id, "claude_session_id": claude_session_id},
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 460-460: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.20)
tests/adapters/test_claude_sdk_adapter.py
[warning] 37-37: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 191-191: Missing return type annotation for private function query_result
(ANN202)
[error] 219-219: Possible hardcoded password assigned to: "FABRIC_UNRELATED_SECRET"
(S105)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[warning] 17-17: Import from collections.abc instead: Iterator, Mapping
Import from collections.abc
(UP035)
[warning] 87-87: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 95-95: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 105-105: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 276-276: Abstract raise to an inner function
(TRY301)
[warning] 276-276: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 279-279: Abstract raise to an inner function
(TRY301)
[warning] 279-279: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 280-280: Consider moving this statement to an else block
(TRY300)
[warning] 306-306: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 306-306: Dynamically typed expressions (typing.Any) are disallowed in _json_safe
(ANN401)
[warning] 356-356: Dynamically typed expressions (typing.Any) are disallowed in **metadata
(ANN401)
[warning] 375-375: Too many return statements (7 > 6)
(PLR0911)
🔇 Additional comments (29)
tests/adapters/test_claude_sdk_adapter.py (6)
14-21: 📐 Maintainability & Code QualityImporting from private
claude_agent_sdk._errorssubmodule.Couples tests to SDK internals that can move without a semver-visible break; prefer top-level
claude_agent_sdkimports.
35-43: 📐 Maintainability & Code QualityModule-level adapter loading instead of a fixture.
As per coding guidelines, "Prefer pytest fixtures over helper methods."
load_claude_adapter()/globaladapterexecutes at import time rather than through a fixture.Source: Coding guidelines
63-69: 📐 Maintainability & Code QualityFixture missing return type annotation.
As per path instructions, fixtures should be defined as
<fixture_name>_fixture() -> <return_type>.Source: Path instructions
188-219: 🩺 Stability & AvailabilityEnv mutation around
query()(production code concern) still applies.Same underlying issue as flagged previously:
run_claude_sdkswaps process-globalos.environaroundquery(), risking leaks/loss across overlapping invocations. This test only proves it works serially, not under concurrency.
208-208: 📐 Maintainability & Code Quality | ⚡ Quick winUse
os.environinstead ofmonkeypatch.setenv.As per coding guidelines, "If you need to modify environment variables in a test, use
os.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary."♻️ Suggested fix
- monkeypatch.setenv("FABRIC_UNRELATED_SECRET", "do-not-forward") + os.environ["FABRIC_UNRELATED_SECRET"] = "do-not-forward"Source: Coding guidelines
46-61: LGTM!Also applies to: 122-186, 222-261
tests/fixtures/claude-sdk/mock-claude-cli.py (2)
14-15: Static "path traversal" hint is a false positive here.
MOCK_CLAUDE_CLI_LOGis set by the test harness itself (not external/attacker-controlled input), so this is not exploitable in this fixture context.
1-68: LGTM!tests/e2e/test_claude_sdk.py (4)
1-103: 🩺 Stability & AvailabilityNo e2e coverage for adapter failure/error paths or session lifecycle cleanup.
Per path instructions, e2e tests under
tests/**should cover error paths, lifecycle cleanup, and SDK/native parity — this file only covers the happy-path resume flow and the opt-in live test.Source: Path instructions
22-61: 📐 Maintainability & Code Quality
fabric_configis a plain helper, not a fixture.As per coding guidelines, "Prefer pytest fixtures over helper methods." Given it's parameterized (
mode,cli_path), a fixture-factory pattern would fit better.Source: Coding guidelines
76-77: 🎯 Functional CorrectnessUnguarded
.index("--resume")gives an opaque failure if the flag is missing.
64-73: LGTM!Also applies to: 78-78, 81-102
.github/workflows/ci_python.yml (1)
58-58: LGTM!justfile (1)
11-11: LGTM!Also applies to: 47-47, 83-83
pyproject.toml (1)
34-37: LGTM!Also applies to: 50-53, 77-77, 108-108
README.md (2)
108-118: LGTM!Also applies to: 134-135
423-426: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the closing code fence.
The
pytestblock is left open, so the rest of the README renders as code.Fix
pytest +```> Likely an incorrect or invalid review comment.adapters/claude-sdk/README.md (1)
1-106: LGTM!adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (8)
19-27: 📐 Maintainability & Code QualityImport SDK error classes from the public package, not the private
_errorssubmodule.Still importing
CLIConnectionError,CLIJSONDecodeError,CLINotFoundError,ClaudeSDKError,MessageParseError,ProcessErrorfromclaude_agent_sdk._errors— flagged previously and unresolved.
145-149: 🎯 Functional Correctness
_resolve_pathnever expands~.Still unresolved from prior review: home-relative
cli_path/cwdvalues are joined underconfig_rootinstead of expanding to the home directory.
152-159: 🎯 Functional Correctness
cwdis still not type-validated likecli_path/system_prompt/tools.A non-string/Path
cwdvalue still raises an unguardedTypeErrorinstead ofAdapterConfigError. Unresolved from prior review.
269-285: 🩺 Stability & AvailabilityNon-dict persisted state still raises uncaught
AttributeError.
state.get(...)at line 275/277 is not guarded by anisinstance(state, dict)check, so malformed (non-object) state JSON bypassesAdapterStateErrornormalization. Unresolved from prior review.
324-354: 🎯 Functional Correctness
normalize_resultstill only branches onis_error, notsubtype.
result.subtypeis captured but unused forcompleted/failedclassification (e.g.error_max_turns,error_max_budget_usd). Trivial, unresolved from prior review.
450-464: 🩺 Stability & Availability
run()/main()still don't catch unexpected exceptions.Only
ClaudeAdapterErroris caught inrun();main()guards neithercommon_utils.load_payload()norrun(). Any bug or unmapped exception still crashes with a raw traceback instead of the structured-failure contract. Unresolved from prior review.
395-435: 🩺 Stability & Availability | ⚡ Quick winPrefer
ClaudeAgentOptions(env=...)over globally mutatingos.environ.
scoped_environment()clears and repopulates the entire processos.environfor the duration of thequery()call (409-419), then restores it. This is process-wide global mutable state: any other concurrently scheduled coroutine, thread, or signal handler in the same process that readsos.environduring theasync forwindow sees the scoped/restricted environment instead of the real one.claude-agent-sdk'sClaudeAgentOptionsaccepts anenvdict directly (ClaudeAgentOptions(env={"ANTHROPIC_API_KEY": ..., "DEBUG": ...})) that is passed straight to the child CLI process without touching the parent'sos.environ. Passingchild_environment(payload)intobuild_options()/ClaudeAgentOptionsavoids the mutation entirely and is a strict improvement — it also fixes the separately-flaggedANTHROPIC_API_KEYdefault-forwarding gap for free, sinceenvmerges with (rather than fully replaces) the SDK's own inherited defaults.🛡️ Proposed fix
def build_options(payload: dict[str, Any], *, resume: str | None) -> ClaudeAgentOptions: ... return ClaudeAgentOptions( resume=resume, cwd=resolve_cwd(payload), model=selected_model(payload), system_prompt=system_prompt, tools=tools, allowed_tools=_string_list(settings.get("allowed_tools"), name="allowed_tools"), disallowed_tools=_string_list(settings.get("disallowed_tools"), name="disallowed_tools"), permission_mode=permission_mode, max_turns=max_turns, max_budget_usd=max_budget, setting_sources=sources, cli_path=_resolve_path(payload, cli_path) if cli_path is not None else None, mcp_servers=_mcp_servers(payload), strict_mcp_config=True, stderr=discard_stderr, + env=child_environment(payload), ) ... async def run_claude_sdk(payload: dict[str, Any]) -> dict[str, Any]: fabric_runtime_id = runtime_id(payload) prior_session_id = load_claude_session_id(payload, fabric_runtime_id) options = build_options(payload, resume=prior_session_id) messages: list[Message] = [] result: ResultMessage | None = None try: async with asyncio.timeout(timeout_seconds(payload)): - with scoped_environment(child_environment(payload)): - async for message in query(prompt=request_prompt(payload), options=options): + async for message in query(prompt=request_prompt(payload), options=options): if isinstance(message, ResultMessage): result = message else: messages.append(message)
1-467: LGTM on the remaining segments (error hierarchy, validators, MCP server parsing, timeout/state helpers not covered above,main()entrypoint wiring).adapters/claude-sdk/fabric-adapter.json (1)
1-13: LGTM!adapters/claude-sdk/pyproject.toml (1)
1-38: LGTM!adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.py (1)
1-5: LGTM!
f2139d7 to
330643a
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py (2)
127-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
session_idno longer surfaced in adapter output.The native Hermes session id (printed to stderr per the updated comment on Line 105) is no longer captured in the returned output, unlike before. Given the design intentionally decouples Fabric from the native session id (resolution is by title), this is fine functionally, but it does remove a useful debugging signal if the title/lineage lookup behaves unexpectedly in production.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py` around lines 127 - 128, The adapter output no longer includes the native Hermes session identifier, which removes a useful debugging signal in `adapter.py`. Update the response assembly in the Hermes CLI adapter (the block that returns `response` and `stdout`) to also surface `session_id` again by extracting it from the native stderr/session handling path used earlier in the flow. Keep the Fabric title-based resolution unchanged, but ensure the returned payload still exposes `session_id` for observability and troubleshooting.
69-88: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard native-session continuation when prep is skipped in
adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py:69-88:ensure_hermes_runtime_session()only runs whenprepare_runtime_stateis enabled, butbuild_command()always emits--continue <fabric_runtime_id>. If the caller disables prep before Hermes has created that title, this falls through to a raw CLI failure. Add an existence check or fail fast with a Fabric-side error on the opt-out path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py` around lines 69 - 88, The native-session continuation path in adapter.py’s build_command flow always passes --continue with fabric_runtime_id even when prepare_runtime_state is disabled, which can cause a raw Hermes CLI failure if the session was never created. Update the adapter logic around ensure_hermes_runtime_session and build_command to either verify the Hermes runtime session/title exists before constructing the command or raise a Fabric-side error immediately when prep is skipped. Keep the fix localized to the adapter’s request handling so the opt-out path is safely guarded before invoking build_command.
♻️ Duplicate comments (7)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (7)
169-172: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
workspacestill isn't type-validated before_resolve_path.
resolve_cwdpassesworkspacestraight into_resolve_path/Path(value)without theisinstanceguard used forcli_path/system_prompt/tools. A non-string value raises an unguardedTypeErrorinstead ofAdapterConfigError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 169 - 172, resolve_cwd currently forwards environment["workspace"] directly into _resolve_path, so non-string values can raise a raw TypeError instead of AdapterConfigError. Update resolve_cwd in claude_sdk/adapter.py to apply the same isinstance(str) validation pattern used by the other payload path helpers before calling _resolve_path, and ensure the fallback to common_utils.config_root(payload) still works when workspace is missing or invalid.
43-71: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ANTHROPIC_API_KEYstill isn't forwarded by default.
INHERITED_ENV_NAMESdoesn't includeANTHROPIC_API_KEY, andchild_environmentonly forwards it whenmodels.default.api_key_envexplicitly equals that name. Combined withscoped_environmentreplacingos.environwholesale, a default API-key setup still loses credentials beforequery()runs.Also applies to: 481-493
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 43 - 71, ANTHROPIC_API_KEY is still being dropped from the child process environment unless explicitly whitelisted, which breaks the default API-key flow. Update `INHERITED_ENV_NAMES` and the `child_environment` logic in `adapter.py` so `ANTHROPIC_API_KEY` is forwarded by default even when `scoped_environment` swaps out `os.environ`, while preserving the existing `models.default.api_key_env` override behavior.
162-166: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
~is still not expanded in_resolve_path.
Path(value).is_absolute()is false for a leading~, so home-relativecli_path/cwdsettings still resolve incorrectly underconfig_rootinstead of the home directory.🩹 Proposed fix
def _resolve_path(payload: dict[str, Any], value: str | Path) -> Path: - path = Path(value) + path = Path(value).expanduser() if not path.is_absolute(): path = Path(common_utils.config_root(payload)) / path return path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 162 - 166, The _resolve_path helper in claude_sdk/adapter.py still treats leading "~" as a relative path because it only checks Path(value).is_absolute(). Update _resolve_path to expand user home references first (before the absolute-path check), then keep the existing config_root-relative fallback for truly relative paths. Use the existing _resolve_path function and common_utils.config_root as the key locations to modify.
536-549: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnexpected exceptions still escape the structured-failure contract.
run()only catchesClaudeAdapterError;main()doesn't guardcommon_utils.load_payload()orrun()either. Any other exception still crashes the process with a raw traceback instead of the normalized JSON output Fabric expects to parse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 536 - 549, The structured-failure path in run() and main() is too narrow because only ClaudeAdapterError is handled, so unexpected exceptions still produce raw tracebacks instead of normalized JSON. Update run() in adapter.py to catch any exception from asyncio.run(run_claude_sdk(payload)) and convert it through adapter_failure, and add equivalent guarding in main() around common_utils.load_payload() and run() so the process always prints a JSON failure payload before exiting. Use the existing run(), main(), and adapter_failure() symbols to keep the behavior consistent with the current contract.
285-287: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
discard_stderris still a pure no-op, losing failure diagnostics.Stderr from the Claude CLI still isn't captured anywhere, making
claude_process_failed/claude_sdk_failedoutcomes hard to troubleshoot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 285 - 287, The discard_stderr helper is currently dropping all Claude CLI stderr, so failures from the Claude process are not diagnosable. Update discard_stderr in the Claude SDK adapter to capture or forward stderr into the existing failure reporting path used by claude_process_failed and claude_sdk_failed, instead of ignoring it, while still avoiding exposure in Fabric artifacts.
355-370: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-dict state JSON still raises an uncaught
AttributeError.
state.get(...)is called without checkingisinstance(state, dict)first; corrupted/legacy state that parses as a JSON list or scalar bypasses theexcept (OSError, ValueError, json.JSONDecodeError)handler and crashes the process instead of raisingAdapterStateError.🛡️ Proposed fix
try: state = json.loads(path.read_text(encoding="utf-8")) + if not isinstance(state, dict): + raise ValueError("state is not an object") if state.get("runtime_id") != fabric_runtime_id:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 355 - 370, The runtime state loader can crash on non-dict JSON because load_claude_session_id() calls state.get() before verifying the parsed value is a dict. Update load_claude_session_id() in the claude_sdk adapter to validate that json.loads(...) returned a dict before accessing runtime_id or claude_session_id, and raise the same AdapterStateError for any non-dict or malformed state so corrupted/legacy state is handled consistently.
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill importing SDK error classes from the private
_errorssubmodule.Same concern raised previously: relying on
claude_agent_sdk._errorsties the adapter to an internal layout that can change without notice. Import the error classes from the publicclaude_agent_sdkpackage instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 20 - 29, The adapter is still pulling error types from the private claude_agent_sdk._errors module, which makes it depend on internals. Update the imports in the claude_sdk adapter to use the public claude_agent_sdk package for CLIConnectionError, CLIJSONDecodeError, CLINotFoundError, ClaudeSDKError, MessageParseError, and ProcessError, while keeping query and ClaudeAgentOptions usage aligned with the existing adapter code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapters/claude-sdk/README.md`:
- Around line 97-103: The MCP example is missing the required exposure field on
the McpServerConfig used inside McpConfig.servers, so it no longer matches the
current mcp.servers[name] contract. Update the example to include exposure
alongside transport and url in the repo McpServerConfig block, keeping the same
McpConfig and McpServerConfig symbols so the snippet validates as-is.
In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py`:
- Around line 212-223: `_normalized_tools` is sourcing `tools` from raw
`fabric_config` instead of the resolved `capability_plan`, which makes it
inconsistent with `_mcp_servers` and `_native_skill_paths`. Update
`_normalized_tools` in `adapter.py` to read tools from the plan data already
present on the payload, and preserve any unsupported/routing signals carried by
`capability_plan` rather than re-deriving from `common_utils.fabric_config`.
Keep the existing validation logic in `_normalized_tools` and `_string_list`,
but apply it to the plan-derived tools value.
- Around line 212-223: The _normalized_tools helper currently returns an
explicit tools list unchanged, which means staged Fabric skills can be excluded
when a custom tool list is provided. Update _normalized_tools in the Claude SDK
adapter so that when FabricConfig.skills is staged and tools is a list, the
returned list includes "Skill" alongside the existing entries. Keep the existing
validation and preset-dict handling intact, and make the change in the
_normalized_tools path that build_options() relies on.
- Around line 240-283: The shared plugin staging in _stage_skill_plugin
currently writes directly into a runtime_id-derived plugin_root, so concurrent
calls can delete or partially rebuild each other’s output. Change the staging
flow to build the plugin under a unique temporary directory first, then
atomically move/swap it into plugin_root, or add equivalent serialization around
the plugin_root cleanup and copytree steps. Keep the fix localized to
_stage_skill_plugin and its plugin_root creation logic.
In `@adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py`:
- Around line 112-124: The `load_thread_id()` call in `run_codex` can raise on
corrupt or mismatched runtime state, bypassing the normal failed-result path.
Wrap the `load_thread_id(payload, runtime_id)` lookup in `run_codex` with the
existing structured error handling and convert any `RuntimeError` from
`load_thread_id` into the same `failed` output shape used for other adapter
failures. Keep the fix localized around `run_codex` and reference
`load_thread_id`/`run_codex` so the adapter still returns a normal failure
result instead of aborting.
In `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py`:
- Around line 155-156: The function signature that takes use_native_session and
fabric_runtime_id should be tightened to avoid boolean positional args by making
those parameters keyword-only, since current call sites already pass keywords.
Also address the TRY003 warning by removing the long inline RuntimeError text
from the exception site in the adapter code and replacing it with a shorter
message or a shared constant/message built outside the raise.
In `@docs/sdk/python.mdx`:
- Around line 425-440: The Models reference link in the Schemas And Python
Models section points to the wrong generated page. Update the markdown link in
this docs block so the SDK Pydantic models reference targets the generated
`nemo_fabric.models` page, and keep the surrounding text aligned with the
`FabricConfig` and `RunRequest` guidance.
In `@examples/harbor/README.md`:
- Around line 49-54: The install steps in the README assume Harbor is checked
out as a sibling repository, which is not true for a plain source checkout.
Update the documentation around the install commands to either explicitly state
the required workspace layout for the `pip install -e ../harbor` step or replace
it with a self-contained Harbor installation approach. Refer to the README
install block so the source-checkout instructions are accurate regardless of
where the repository is cloned.
---
Outside diff comments:
In `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py`:
- Around line 127-128: The adapter output no longer includes the native Hermes
session identifier, which removes a useful debugging signal in `adapter.py`.
Update the response assembly in the Hermes CLI adapter (the block that returns
`response` and `stdout`) to also surface `session_id` again by extracting it
from the native stderr/session handling path used earlier in the flow. Keep the
Fabric title-based resolution unchanged, but ensure the returned payload still
exposes `session_id` for observability and troubleshooting.
- Around line 69-88: The native-session continuation path in adapter.py’s
build_command flow always passes --continue with fabric_runtime_id even when
prepare_runtime_state is disabled, which can cause a raw Hermes CLI failure if
the session was never created. Update the adapter logic around
ensure_hermes_runtime_session and build_command to either verify the Hermes
runtime session/title exists before constructing the command or raise a
Fabric-side error immediately when prep is skipped. Keep the fix localized to
the adapter’s request handling so the opt-out path is safely guarded before
invoking build_command.
---
Duplicate comments:
In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py`:
- Around line 169-172: resolve_cwd currently forwards environment["workspace"]
directly into _resolve_path, so non-string values can raise a raw TypeError
instead of AdapterConfigError. Update resolve_cwd in claude_sdk/adapter.py to
apply the same isinstance(str) validation pattern used by the other payload path
helpers before calling _resolve_path, and ensure the fallback to
common_utils.config_root(payload) still works when workspace is missing or
invalid.
- Around line 43-71: ANTHROPIC_API_KEY is still being dropped from the child
process environment unless explicitly whitelisted, which breaks the default
API-key flow. Update `INHERITED_ENV_NAMES` and the `child_environment` logic in
`adapter.py` so `ANTHROPIC_API_KEY` is forwarded by default even when
`scoped_environment` swaps out `os.environ`, while preserving the existing
`models.default.api_key_env` override behavior.
- Around line 162-166: The _resolve_path helper in claude_sdk/adapter.py still
treats leading "~" as a relative path because it only checks
Path(value).is_absolute(). Update _resolve_path to expand user home references
first (before the absolute-path check), then keep the existing
config_root-relative fallback for truly relative paths. Use the existing
_resolve_path function and common_utils.config_root as the key locations to
modify.
- Around line 536-549: The structured-failure path in run() and main() is too
narrow because only ClaudeAdapterError is handled, so unexpected exceptions
still produce raw tracebacks instead of normalized JSON. Update run() in
adapter.py to catch any exception from asyncio.run(run_claude_sdk(payload)) and
convert it through adapter_failure, and add equivalent guarding in main() around
common_utils.load_payload() and run() so the process always prints a JSON
failure payload before exiting. Use the existing run(), main(), and
adapter_failure() symbols to keep the behavior consistent with the current
contract.
- Around line 285-287: The discard_stderr helper is currently dropping all
Claude CLI stderr, so failures from the Claude process are not diagnosable.
Update discard_stderr in the Claude SDK adapter to capture or forward stderr
into the existing failure reporting path used by claude_process_failed and
claude_sdk_failed, instead of ignoring it, while still avoiding exposure in
Fabric artifacts.
- Around line 355-370: The runtime state loader can crash on non-dict JSON
because load_claude_session_id() calls state.get() before verifying the parsed
value is a dict. Update load_claude_session_id() in the claude_sdk adapter to
validate that json.loads(...) returned a dict before accessing runtime_id or
claude_session_id, and raise the same AdapterStateError for any non-dict or
malformed state so corrupted/legacy state is handled consistently.
- Around line 20-29: The adapter is still pulling error types from the private
claude_agent_sdk._errors module, which makes it depend on internals. Update the
imports in the claude_sdk adapter to use the public claude_agent_sdk package for
CLIConnectionError, CLIJSONDecodeError, CLINotFoundError, ClaudeSDKError,
MessageParseError, and ProcessError, while keeping query and ClaudeAgentOptions
usage aligned with the existing adapter code.
🪄 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: f730beec-c9ba-4a1d-8d4a-6a35db75921a
⛔ Files ignored due to path filters (3)
adapters/claude-sdk/uv.lockis excluded by!**/*.lockpython/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (217)
.github/workflows/ci_python.ymlPOC-TO-MVP-PLAN.mdREADME.mdadapters/claude-sdk/README.mdadapters/claude-sdk/fabric-adapter.jsonadapters/claude-sdk/pyproject.tomladapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.pyadapters/codex-cli/README.mdadapters/codex-cli/fabric-adapter.jsonadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes-cli/README.mdadapters/hermes-cli/fabric-adapter.jsonadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/hermes-sdk/README.mdadapters/hermes-sdk/fabric-adapter.jsonadapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pycrates/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-core/src/schema.rsdocs/getting-started/overview.mdxdocs/guides/harbor-evaluation.mdxdocs/index.ymldocs/integrations/harbor.mdxdocs/python-sdk-contract.mddocs/reference/api/python-library-reference/index.mddocs/reference/api/python-library-reference/nemo_fabric.client.mddocs/reference/api/python-library-reference/nemo_fabric.errors.mddocs/reference/api/python-library-reference/nemo_fabric.models.mddocs/reference/api/python-library-reference/nemo_fabric.runtime.mddocs/reference/api/python-library-reference/nemo_fabric.session.mddocs/reference/api/python-library-reference/nemo_fabric.types.mddocs/reference/api/rust-library-reference/fabric-core/config/constant-adapter-contract-version.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-adapterdescriptorsource.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-adapterkind.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-capabilitykind.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-capabilitytarget.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-controllocation.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-environmentownership.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-fabricdocument.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-mcpexposure.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-resolutionstrategy.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-runtimemode.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-telemetryprovider.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-load-adapter-descriptor.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-load-fabric-document.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-resolve-effective-config-with-profiles.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-resolve-effective-config.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-resolve-run-plan-with-profiles.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-resolve-run-plan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-validate-agent-directory.mdxdocs/reference/api/rust-library-reference/fabric-core/config/index.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-adapterconfigsupport.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-adapterdescriptor.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-adapterrequirements.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-adaptertelemetrysupport.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-capabilityplan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-capabilityroute.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-capabilitytargetplan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-effectiveconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-environmentconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-environmentplan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-fabricconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-harnessconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-mcpconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-mcpserverconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-metadataconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-modelconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-profileconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-profileregistryconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-resolvecontext.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-resolvedadapterdescriptor.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-runplan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-runtimecapabilities.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-runtimeconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-skillconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-telemetryconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-telemetryplan.mdxdocs/reference/api/rust-library-reference/fabric-core/doctor/enum-doctorstatus.mdxdocs/reference/api/rust-library-reference/fabric-core/doctor/struct-doctorcheck.mdxdocs/reference/api/rust-library-reference/fabric-core/doctor/struct-doctorreport.mdxdocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdxdocs/reference/api/rust-library-reference/fabric-core/error/type-result.mdxdocs/reference/api/rust-library-reference/fabric-core/fn-version.mdxdocs/reference/api/rust-library-reference/fabric-core/index.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/enum-errorstage.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/enum-runstatus.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/fn-stop-runtime.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-adapterinvocation.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-artifactmanifest.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-artifactref.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-environmenthandle.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-errorinfo.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-fabricevent.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-invocationhandle.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runrequest.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runresult.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runtimecontext.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runtimehandle.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runtimetelemetrycontext.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-telemetryref.mdxdocs/reference/api/rust-library-reference/fabric-core/schema/enum-schemaname.mdxdocs/reference/api/rust-library-reference/fabric-core/schema/fn-generate-all-schemas.mdxdocs/reference/api/rust-library-reference/fabric-core/schema/fn-generate-schema-json.mdxdocs/reference/api/rust-library-reference/fabric-core/schema/fn-write-schema-snapshots.mdxdocs/sdk/python.mdxexamples/README.mdexamples/__init__.pyexamples/code-review-agent/profiles/hermes-cli-session.yamlexamples/code-review-agent/profiles/hermes-session.yamlexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/code_review_agent/repos/my-service/README.mdexamples/code_review_agent/repos/my-service/calculator.pyexamples/code_review_agent/skills/code-review/README.mdexamples/harbor/README.mdexamples/harbor/demo/README.mdexamples/harbor/demo/host-gateway.compose.yamlexamples/harbor/demo/task/environment/Dockerfileexamples/harbor/demo/task/environment/calculator.pyexamples/harbor/demo/task/environment/fabric/configs/adapters/scripted/fabric-adapter.jsonexamples/harbor/demo/task/environment/fabric/configs/adapters/scripted/run.pyexamples/harbor/demo/task/environment/fabric/configs/codex.yamlexamples/harbor/demo/task/environment/fabric/configs/hermes-relay.yamlexamples/harbor/demo/task/environment/fabric/configs/hermes.yamlexamples/harbor/demo/task/environment/fabric/configs/smoke.yamlexamples/harbor/demo/task/instruction.mdexamples/harbor/demo/task/solution/solve.shexamples/harbor/demo/task/task.tomlexamples/harbor/demo/task/tests/test.shexamples/harbor/demo/task/tests/verify.pyintegrations/harbor/README.mdintegrations/harbor/demo/README.mdintegrations/harbor/demo/host-gateway.compose.yamlintegrations/harbor/demo/task/environment/fabric/profiles/codex.yamlintegrations/harbor/demo/task/environment/fabric/profiles/smoke.yamlintegrations/harbor/demo/task/tests/test.shjustfilepyproject.tomlpython/pyproject.tomlpython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/_config_sources.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/errors.pypython/src/nemo_fabric/integrations/harbor/README.mdpython/src/nemo_fabric/integrations/harbor/__init__.pypython/src/nemo_fabric/integrations/harbor/fabric_agent.pypython/src/nemo_fabric/integrations/harbor/models.pypython/src/nemo_fabric/integrations/harbor/runner.pypython/src/nemo_fabric/models.pypython/src/nemo_fabric/runtime.pypython/src/nemo_fabric/session.pypython/src/nemo_fabric/types.pyschemas/SCHEMA.mdschemas/adapter-descriptor.schema.jsonschemas/adapter-invocation.schema.jsonschemas/agent.schema.jsonschemas/effective-config.schema.jsonschemas/run-plan.schema.jsonschemas/run-request.schema.jsonschemas/runtime-context.schema.jsonschemas/runtime-handle.schema.jsonscripts/generate_api_docs.shtests/adapters/test_adapaters_common_utils.pytests/adapters/test_claude_sdk_adapter.pytests/adapters/test_codex_cli.pytests/adapters/test_hermes_cli.pytests/adapters/test_hermes_cli_preflight.pytests/adapters/test_hermes_sdk_adapter.pytests/conftest.pytests/docs/test_python_api_docs.pytests/e2e/test_claude_sdk.pytests/e2e/test_cli.pytests/e2e/test_codex_cli.pytests/e2e/test_hermes_cli.pytests/e2e/test_hermes_e2e.pytests/e2e/test_hermes_runtime.pytests/fixtures/claude-sdk/mock-claude-cli.pytests/fixtures/file-config-agent/agent.yamltests/fixtures/file-config-agent/profiles/codex-cli.yamltests/fixtures/file-config-agent/profiles/env-local.yamltests/fixtures/file-config-agent/profiles/env-opensandbox.yamltests/fixtures/file-config-agent/profiles/hermes-cli.yamltests/fixtures/file-config-agent/profiles/hermes-sdk.yamltests/fixtures/file-config-agent/profiles/mcp-github.yamltests/fixtures/file-config-agent/profiles/native-otel.yamltests/fixtures/file-config-agent/profiles/relay-openinference.yamltests/fixtures/file-config-agent/profiles/relay-otel.yamltests/fixtures/file-config-agent/profiles/relay.yamltests/fixtures/file-config-agent/repos/my-service/calculator.pytests/fixtures/file-config-agent/skills/code-review/README.mdtests/fixtures/hermes-cli-agent/agent.yamltests/fixtures/hermes-shim-agent/adapters/hermes-shim/fabric-adapter.jsontests/fixtures/hermes-shim-agent/adapters/hermes-shim/src/nemo_fabric_test_adapters/hermes_shim/adapter.pytests/fixtures/hermes-shim-agent/agent.yamltests/fixtures/hermes-shim-agent/profiles/harbor-swebench-django-13741.yamltests/fixtures/hermes-shim-agent/profiles/swebench-shim.yamltests/integrations/test_harbor_runner.pytests/python/test_code_review_example.pytests/python/test_consumer_neutral.pytests/python/test_environment_handle.pytests/python/test_harbor_integration.pytests/python/test_native_sdk.pytests/python/test_readme_examples.pytests/python/test_runtime.pytests/python/test_sdk_concurrency.pytests/python/test_sdk_contract.pytests/python/test_sdk_runtimes.pytests/python/test_typed_config.py
💤 Files with no reviewable changes (7)
- docs/reference/api/rust-library-reference/fabric-core/config/enum-runtimemode.mdx
- docs/python-sdk-contract.md
- docs/guides/harbor-evaluation.mdx
- POC-TO-MVP-PLAN.md
- examples/code-review-agent/profiles/hermes-cli-session.yaml
- examples/code-review-agent/profiles/hermes-session.yaml
- docs/reference/api/python-library-reference/nemo_fabric.session.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
🧰 Additional context used
📓 Path-based instructions (3)
{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/reference/api/rust-library-reference/fabric-core/config/fn-load-fabric-document.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-resolve-run-plan-with-profiles.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-load-adapter-descriptor.mdxdocs/reference/api/rust-library-reference/fabric-core/fn-version.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-resolve-effective-config.mdxdocs/reference/api/rust-library-reference/fabric-core/schema/fn-write-schema-snapshots.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-validate-agent-directory.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-resolve-run-plan.mdxdocs/index.ymldocs/reference/api/rust-library-reference/fabric-core/schema/fn-generate-schema-json.mdxdocs/reference/api/python-library-reference/nemo_fabric.errors.mddocs/reference/api/rust-library-reference/fabric-core/config/constant-adapter-contract-version.mdxdocs/reference/api/rust-library-reference/fabric-core/config/fn-resolve-effective-config-with-profiles.mdxdocs/reference/api/rust-library-reference/fabric-core/schema/fn-generate-all-schemas.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/enum-runstatus.mdxdocs/reference/api/rust-library-reference/fabric-core/error/type-result.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runresult.mdxdocs/integrations/harbor.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-resolutionstrategy.mdxdocs/reference/api/rust-library-reference/fabric-core/index.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-adapterkind.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-adapterinvocation.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-telemetryconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/enum-errorstage.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-errorinfo.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-adapterdescriptor.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-resolvedadapterdescriptor.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-artifactref.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-telemetryprovider.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-harnessconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-fabricevent.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-profileregistryconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-runplan.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runtimetelemetrycontext.mdxdocs/reference/api/python-library-reference/nemo_fabric.runtime.mddocs/reference/api/rust-library-reference/fabric-core/config/struct-adapterconfigsupport.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-capabilityplan.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-invocationhandle.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-capabilitytargetplan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-capabilityroute.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-artifactmanifest.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/fn-stop-runtime.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-adapterdescriptorsource.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-telemetryref.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-adapterrequirements.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-adaptertelemetrysupport.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-environmentconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-skillconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-capabilitytarget.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-telemetryplan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-fabricconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-mcpconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/doctor/struct-doctorreport.mdxdocs/reference/api/rust-library-reference/fabric-core/doctor/struct-doctorcheck.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-capabilitykind.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-environmenthandle.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-environmentownership.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-modelconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-mcpserverconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-profileconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-environmentplan.mdxdocs/reference/api/rust-library-reference/fabric-core/config/index.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-controllocation.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-resolvecontext.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-runtimeconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/doctor/enum-doctorstatus.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runrequest.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-mcpexposure.mdxdocs/reference/api/python-library-reference/nemo_fabric.models.mddocs/reference/api/python-library-reference/index.mddocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runtimecontext.mdxdocs/reference/api/rust-library-reference/fabric-core/config/enum-fabricdocument.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-effectiveconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/error/enum-fabricerror.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-metadataconfig.mdxdocs/reference/api/rust-library-reference/fabric-core/runtime/struct-runtimehandle.mdxdocs/reference/api/rust-library-reference/fabric-core/schema/enum-schemaname.mdxdocs/reference/api/rust-library-reference/fabric-core/config/struct-runtimecapabilities.mdxdocs/sdk/python.mdxdocs/reference/api/python-library-reference/nemo_fabric.client.mddocs/getting-started/overview.mdxREADME.mddocs/reference/api/python-library-reference/nemo_fabric.types.md
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/code_review_agent/repos/my-service/README.mdexamples/code_review_agent/repos/my-service/calculator.pyadapters/hermes-sdk/README.mdexamples/code_review_agent/skills/code-review/README.mdadapters/hermes-cli/README.mdexamples/code_review_agent/README.mdexamples/__init__.pyexamples/README.mdadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyexamples/code_review_agent/__init__.pyadapters/codex-cli/fabric-adapter.jsonexamples/code_review_agent/__main__.pyadapters/claude-sdk/pyproject.tomladapters/codex-cli/README.mdadapters/hermes-sdk/fabric-adapter.jsonexamples/harbor/README.mdadapters/common/src/nemo_fabric_adapters/common/utils.pyadapters/hermes-cli/fabric-adapter.jsonadapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pyadapters/claude-sdk/fabric-adapter.jsonadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyexamples/code_review_agent/config.pyadapters/claude-sdk/README.mdadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
crates/fabric-core/src/**/*.rs
⚙️ CodeRabbit configuration file
crates/fabric-core/src/**/*.rs: Review the Rust core for runtime lifecycle correctness, handle validation, capability routing accuracy, schema stability, and error semantics.
Public API changes should match committed schemas, tests, and documentation.
Files:
crates/fabric-core/src/schema.rscrates/fabric-core/src/lib.rscrates/fabric-core/src/doctor.rscrates/fabric-core/src/config.rscrates/fabric-core/src/runtime.rs
🧠 Learnings (1)
📚 Learning: 2026-06-29T22:34:52.407Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 27
File: adapters/codex-cli/fabric-adapter.json:13-15
Timestamp: 2026-06-29T22:34:52.407Z
Learning: In NeMo-Fabric adapter manifest files (e.g., `*/fabric-adapter.json`), keep `config.accepts` limited to the top-level Fabric capability sections that `resolve_capability_plan` consumes (such as `models`, `tools`, `mcp`, `skills`, `telemetry`). Do not add adapter-owned `harness.settings` keys to `config.accepts`; `harness.settings` should remain adapter-owned and be passed through unchanged.
Applied to files:
adapters/codex-cli/fabric-adapter.jsonadapters/hermes-sdk/fabric-adapter.jsonadapters/hermes-cli/fabric-adapter.jsonadapters/claude-sdk/fabric-adapter.json
🧬 Code graph analysis (1)
crates/fabric-core/src/runtime.rs (1)
crates/fabric-core/src/config.rs (1)
resolve_run_plan(751-754)
🪛 ast-grep (0.44.1)
examples/code_review_agent/__main__.py
[info] 50-50: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output.to_mapping(), indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
[info] 132-132: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"runtime_id": runtime_id, "thread_id": thread_id}, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[info] 267-275: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"name": "nemo-fabric-skills",
"description": "Skills provided by NeMo Fabric",
"version": "1.0.0",
},
indent=2,
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 382-385: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{"runtime_id": fabric_runtime_id, "claude_session_id": claude_session_id},
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 546-546: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
docs/sdk/python.mdx
[style] ~230-~230: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Useinput=...for the common case. Userequest=RunRequest(...)` for structure...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~361-~361: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...apter selection and capability routing. Use the doctor report to catch missing depe...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.1)
docs/reference/api/python-library-reference/nemo_fabric.runtime.md
[warning] 6-6: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 7-7: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 29-29: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/reference/api/python-library-reference/nemo_fabric.models.md
[warning] 6-6: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 7-7: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 79-79: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 139-139: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 199-199: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 259-259: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 321-321: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 381-381: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 463-463: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 523-523: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 611-611: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 708-708: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 768-768: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 893-893: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 953-953: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/reference/api/python-library-reference/nemo_fabric.client.md
[warning] 17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 Ruff (0.15.20)
adapters/common/src/nemo_fabric_adapters/common/utils.py
[warning] 59-59: Avoid specifying long messages outside the exception class
(TRY003)
adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py
[warning] 155-155: Boolean-typed positional argument in function definition
(FBT001)
[warning] 155-155: Boolean default positional argument in function definition
(FBT002)
[warning] 168-168: Avoid specifying long messages outside the exception class
(TRY003)
examples/code_review_agent/config.py
[warning] 263-263: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py
[warning] 119-119: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 123-123: Avoid specifying long messages outside the exception class
(TRY003)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[warning] 18-18: Import from collections.abc instead: Iterator, Mapping
Import from collections.abc
(UP035)
[warning] 95-95: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 103-103: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 113-113: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 362-362: Abstract raise to an inner function
(TRY301)
[warning] 362-362: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 365-365: Abstract raise to an inner function
(TRY301)
[warning] 365-365: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 366-366: Consider moving this statement to an else block
(TRY300)
[warning] 392-392: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 392-392: Dynamically typed expressions (typing.Any) are disallowed in _json_safe
(ANN401)
[warning] 442-442: Dynamically typed expressions (typing.Any) are disallowed in **metadata
(ANN401)
[warning] 461-461: Too many return statements (7 > 6)
(PLR0911)
330643a to
ce1105c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (1)
162-172: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
~still not expanded, andresolve_cwd's newworkspacesource has no type check.
_resolve_pathstill lacks.expanduser()(previously flagged forcli_path/oldcwd). Additionally, sincecwdnow resolves fromFabricConfig.environment.workspace(line 171) rather thanharness.settings.cwd, the missing type guard has effectively moved here: a non-string/Pathworkspacevalue hitsPath(value)in_resolve_pathand raises an unguardedTypeErrorinstead of a structuredAdapterConfigError, which (combined with the missing generic exception handler inrun()) crashes the process.🐛 Proposed fix
def _resolve_path(payload: dict[str, Any], value: str | Path) -> Path: - path = Path(value) + path = Path(value).expanduser() if not path.is_absolute(): path = Path(common_utils.config_root(payload)) / path return path def resolve_cwd(payload: dict[str, Any]) -> Path: environment = common_utils.environment_payload(payload) workspace = environment.get("workspace") + if workspace is not None and not isinstance(workspace, (str, Path)): + raise AdapterConfigError( + "claude_invalid_configuration", "environment.workspace must be a path" + ) return _resolve_path(payload, workspace or common_utils.config_root(payload))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py` around lines 162 - 172, Update _resolve_path to call expanduser() on the Path constructed from value before resolving relative paths, and validate that resolve_cwd’s workspace is a str or Path before passing it to _resolve_path. For invalid workspace values, raise the established structured AdapterConfigError rather than allowing Path(value) to raise TypeError; preserve the existing config-root fallback for missing workspace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapters/claude-sdk/README.md`:
- Line 1: Add the repository-standard SPDX copyright and license header at the
beginning of the Markdown file, before the “Claude Agent SDK Adapter” heading.
- Around line 8-10: The README installation snippet should use the repository’s
local wheel-based flow instead of standard pip from PyPI. Replace the command in
the Claude SDK installation instructions with `uv pip install --find-links
<path_to_dist_directory> nemo-fabric[claude]`.
In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py`:
- Line 18: Replace the deprecated typing aliases in the imports of the Claude
SDK adapter: import Iterator and Mapping from collections.abc, while retaining
Any from typing. Update the import statement without changing usage elsewhere.
In `@README.md`:
- Around line 117-121: Update the Claude adapter installation instructions in
README.md to use the repository’s wheel-based flow: run `just wheels`, then
install with python -m pip install --find-links dist "nemo-fabric[claude]"
instead of fetching directly from PyPI.
In `@tests/adapters/test_claude_sdk_adapter.py`:
- Around line 241-273: Extend test_run_claude_sdk_resumes_and_persists_session
with a separate scenario where ANTHROPIC_API_KEY is provided only through
monkeypatch.setenv, removing api_key_env and settings.env configuration; assert
the captured query environment preserves that key while still excluding
unrelated secrets, covering the default child_environment inheritance path.
---
Duplicate comments:
In `@adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py`:
- Around line 162-172: Update _resolve_path to call expanduser() on the Path
constructed from value before resolving relative paths, and validate that
resolve_cwd’s workspace is a str or Path before passing it to _resolve_path. For
invalid workspace values, raise the established structured AdapterConfigError
rather than allowing Path(value) to raise TypeError; preserve the existing
config-root fallback for missing workspace.
🪄 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: cb6c919c-9d77-4eb1-8a93-5b0f9b91785d
⛔ Files ignored due to path filters (2)
adapters/claude-sdk/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci_python.ymlREADME.mdadapters/claude-sdk/README.mdadapters/claude-sdk/fabric-adapter.jsonadapters/claude-sdk/pyproject.tomladapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.pyjustfilepyproject.tomltests/adapters/test_claude_sdk_adapter.pytests/e2e/test_claude_sdk.pytests/fixtures/claude-sdk/mock-claude-cli.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**:Contributing to NeMo Fabric
Thank you for your interest in contributing to NeMo Fabric. This guide covers
the development workflow, coding standards, and pull request process.Development Setup
This section collects the setup steps needed before building, testing, or
contributing changes.Package Installation
NeMo Fabric is not currently available on PyPI. To consume the Python packages,
build wheels from a source checkout:just wheels uv pip install --find-links dist "nemo-fabric[runtime]"Adapters are distributed as optional extras. For example, install the Hermes
SDK adapter with:uv pip install --find-links dist "nemo-fabric[adapters-hermes-sdk]"Refer to the installation guide for the
complete list of adapters and installation options.Source Development
Install these tools before you start:
- Rust (stable toolchain) -- install with rustup
- Python >= 3.11
- uv -- follow the uv installation guide
- just >= 1.50.0 --
cargo install just --lockedClone the repository, create a virtual environment, and build the Rust and
Python packages:git clone https://github.com/NVIDIA/NeMo-Fabric.git cd NeMo-Fabric uv venv --seed .venv --python 3.13 source .venv/bin/activate uv sync --all-groups --all-extras just no_uv=true build-allVerify the checkout by running the test suites described in
Testing Requirements.Release Tagging
Versioned release tags must use raw Rust-compatible SemVer without a leading
v.
- Use
0.1.0for stable releases.- Use
0.1.0-rc.1for prereleases.- Do not create tags such as
v0.1.0orv0.1.0-rc.1.This keeps release tags aligned with Cargo package versions and lets...
Files:
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pypyproject.tomladapters/claude-sdk/pyproject.tomladapters/claude-sdk/fabric-adapter.jsontests/fixtures/claude-sdk/mock-claude-cli.pyjustfileadapters/claude-sdk/README.mdREADME.mdtests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.pyadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.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/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.pyadapters/claude-sdk/pyproject.tomladapters/claude-sdk/fabric-adapter.jsonadapters/claude-sdk/README.mdadapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, define it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only passscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/fixtures/claude-sdk/mock-claude-cli.pytests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/fixtures/claude-sdk/mock-claude-cli.pytests/e2e/test_claude_sdk.pytests/adapters/test_claude_sdk_adapter.py
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
README.md
🧠 Learnings (1)
📚 Learning: 2026-06-29T22:34:52.407Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 27
File: adapters/codex-cli/fabric-adapter.json:13-15
Timestamp: 2026-06-29T22:34:52.407Z
Learning: In NeMo-Fabric adapter manifest files (e.g., `*/fabric-adapter.json`), keep `config.accepts` limited to the top-level Fabric capability sections that `resolve_capability_plan` consumes (such as `models`, `tools`, `mcp`, `skills`, `telemetry`). Do not add adapter-owned `harness.settings` keys to `config.accepts`; `harness.settings` should remain adapter-owned and be passed through unchanged.
Applied to files:
adapters/claude-sdk/fabric-adapter.json
🧬 Code graph analysis (2)
tests/e2e/test_claude_sdk.py (3)
crates/fabric-core/src/config.rs (6)
FabricConfig(39-72)EnvironmentConfig(522-549)ModelConfig(476-493)HarnessConfig(106-118)RuntimeConfig(497-510)MetadataConfig(93-102)python/src/nemo_fabric/types.py (2)
add_mcp_server(691-709)add_skill_path(711-715)crates/fabric-core/src/runtime.rs (1)
start_runtime(411-421)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (2)
adapters/common/src/nemo_fabric_adapters/common/utils.py (7)
load_payload(72-78)settings_payload(104-106)environment_payload(100-101)models_payload(109-110)capability_plan(122-123)config_root(64-65)runtime_context(81-82)tests/e2e/test_hermes_config_mapping.py (1)
payload(46-77)
🪛 ast-grep (0.44.1)
tests/fixtures/claude-sdk/mock-claude-cli.py
[info] 14-14: use jsonify instead of json.dumps for JSON output
Context: json.dumps(sys.argv[1:])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 20-29: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "control_response",
"response": {
"subtype": "success",
"request_id": message["request_id"],
"response": {"commands": [], "output_style": "default"},
},
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 36-47: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "assistant",
"message": {
"role": "assistant",
"content": [{"type": "text", "text": "mock Claude response"}],
"model": "claude-test-model",
"usage": {"input_tokens": 1, "output_tokens": 2},
},
"session_id": SESSION_ID,
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 51-64: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "result",
"subtype": "success",
"duration_ms": 10,
"duration_api_ms": 8,
"is_error": False,
"num_turns": 1,
"session_id": SESSION_ID,
"total_cost_usd": 0.001,
"usage": {"input_tokens": 1, "output_tokens": 2},
"result": "mock Claude response",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 13-13: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.environ["MOCK_CLAUDE_CLI_LOG"], "a", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
tests/adapters/test_claude_sdk_adapter.py
[info] 288-288: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[info] 267-275: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"name": "nemo-fabric-skills",
"description": "Skills provided by NeMo Fabric",
"version": "1.0.0",
},
indent=2,
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 382-385: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{"runtime_id": fabric_runtime_id, "claude_session_id": claude_session_id},
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 546-546: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.20)
tests/adapters/test_claude_sdk_adapter.py
[warning] 37-37: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 182-182: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 244-244: Missing return type annotation for private function query_result
(ANN202)
[error] 273-273: Possible hardcoded password assigned to: "FABRIC_UNRELATED_SECRET"
(S105)
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py
[warning] 18-18: Import from collections.abc instead: Iterator, Mapping
Import from collections.abc
(UP035)
[warning] 95-95: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 103-103: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 113-113: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 362-362: Abstract raise to an inner function
(TRY301)
[warning] 362-362: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 365-365: Abstract raise to an inner function
(TRY301)
[warning] 365-365: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 366-366: Consider moving this statement to an else block
(TRY300)
[warning] 392-392: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 392-392: Dynamically typed expressions (typing.Any) are disallowed in _json_safe
(ANN401)
[warning] 442-442: Dynamically typed expressions (typing.Any) are disallowed in **metadata
(ANN401)
[warning] 461-461: Too many return statements (7 > 6)
(PLR0911)
🔇 Additional comments (26)
adapters/claude-sdk/README.md (2)
97-103: Duplicate of the existing MCP example finding.
McpServerConfigstill omits the requiredexposurefield, so this copied example remains invalid.
12-15: LGTM!Also applies to: 17-52, 54-96, 104-146
README.md (2)
97-108: LGTM!
123-125: LGTM!Also applies to: 141-142
adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py (8)
20-28: Still importing SDK error classes from the private_errorssubmodule.
Same concern as prior review — publicclaude_agent_sdkpackage re-exports these; importing from_errorscouples this file to internal layout.
212-223:toolsstill bypasses the resolvedcapability_plan, and staged skills still can't injectSkillinto an explicit tools list.Two still-open concerns from prior review, both here:
_normalized_toolsreadscommon_utils.fabric_config(payload).get("tools")directly instead ofcapability_plan, unlike_mcp_servers/_native_skill_pathswhich correctly read from the resolved plan.- When Fabric skills are staged (
pluginsnon-empty) andtoolsis an explicit list,"Skill"is never injected, so Claude can't invoke those skills per SDK skills docs.The adapter test at
tests/adapters/test_claude_sdk_adapter.py(test_build_options_maps_normalized_capabilities_and_claude_settings) currently locks in this exact scenario (explicit tools + staged skill) and assertsoptions.tools == ["Read", "Glob", "Grep"]without"Skill", so fixing this also requires updating that assertion.
240-282: Shared plugin directory is still rewritten in place with no isolation for overlapping invocations.
plugin_rootremains keyed only byruntime_id; concurrent invocations for the same runtime can still race onrmtree/mkdir/copytree.
285-287:discard_stderris still a pure no-op, discarding diagnostics useful for troubleshooting failed invocations.
355-371: Non-dict persisted state still raises an uncaughtAttributeError.
state.get(...)at line 361 is not guarded by anisinstance(state, dict)check, andAttributeErrorisn't in the caught(OSError, ValueError, json.JSONDecodeError)tuple, so corrupted/legacy state still crashes instead of raisingAdapterStateError.
410-439: Terminal-state mapping still relies solely onresult.is_error, ignoringresult.subtype.
Defense-in-depth suggestion from prior review remains unapplied.
481-493:ANTHROPIC_API_KEYstill isn't forwarded by default.
INHERITED_ENV_NAMESdoesn't include it, andchild_environmentonly preserves it whenmodels.default.api_key_envexplicitly equals that name — a normal API-key setup without an explicitapi_key_env/settings.enventry still loses credentials beforequery()runs. Note the adapter test suite only exercises the case whereapi_key_envandsettings.envare both explicitly configured, so this regression isn't caught by current tests.
536-549: Unexpected exceptions still escape the structured-failure contract.
run()only catchesClaudeAdapterError; any other exception (including theAttributeErrorabove, orcommon_utils.load_payload()failures inmain()) still propagates as a raw traceback with no JSON on stdout, breaking the adapter's own normalized-failure contract.tests/adapters/test_claude_sdk_adapter.py (3)
14-21: Still importing from the privateclaude_agent_sdk._errorsmodule.
35-43: Adapter is still loaded via a module-level helper/global instead of a fixture.
As per coding guidelines, "Prefer pytest fixtures over helper methods."Source: Coding guidelines
63-69:claude_payload_fixturestill has no return type annotation.
As per path instructions/coding guidelines fixture naming convention.Source: Path instructions
tests/e2e/test_claude_sdk.py (3)
1-129: Still no e2e coverage for adapter failure paths or session lifecycle cleanup.
Per path instructions,tests/**should cover "error paths, lifecycle cleanup, and SDK/native parity." Only the happy-path resume flow and skip-gated live test are present.Source: Path instructions
30-75:fabric_configis still a plain helper, not a fixture-factory.
As per coding guidelines, "Prefer pytest fixtures over helper methods."Source: Coding guidelines
97-104: Unguarded.index(...)lookups for--resume,--tools, and--allowedToolsstill give opaqueValueErrorfailures if a flag is missing.
Previously flagged for--resume(line 98); the same pattern now also appears at lines 99 and 104.♻️ Suggested fix
-assert arguments[1][arguments[1].index("--resume") + 1] == SESSION_ID -assert all(args[args.index("--tools") + 1] == "Read,Glob,Grep" for args in arguments) +assert "--resume" in arguments[1] +assert arguments[1][arguments[1].index("--resume") + 1] == SESSION_ID +assert all("--tools" in args and args[args.index("--tools") + 1] == "Read,Glob,Grep" for args in arguments)adapters/claude-sdk/fabric-adapter.json (1)
1-13: LGTM!tests/fixtures/claude-sdk/mock-claude-cli.py (1)
1-69: LGTM!adapters/claude-sdk/pyproject.toml (1)
1-30: LGTM!adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/__init__.py (1)
1-5: LGTM!pyproject.toml (1)
29-32: LGTM!Also applies to: 45-48, 72-72, 103-103
justfile (2)
13-13: LGTM!Also applies to: 334-334
282-282: 🎯 Functional CorrectnessConfirm why
build-pythononly adds--extra claude, not other adapter extras.
test-pythonand CI sync the full extras set (claude, codex, harbor, hermes, relay, runtime), butbuild-pythononly ever synced--extra runtimeand now adds--extra claudespecifically, still omittingcodex,harbor,hermes, andrelay. If this is intentional (e.g., claude-sdk needs to be present for the native-extension build step in a way other adapters don't), a brief comment would help; otherwise this looks like an inconsistency that should either add all adapter extras here or drop--extra claudeto keep parity with the other adapters..github/workflows/ci_python.yml (1)
58-58: LGTM!
8307835 to
1c51300
Compare
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
1c51300 to
64bab7a
Compare
AjayThorve
changed the title
|
/merge |
2 participants
Overview
Add the independently packaged Claude adapter described by FABRIC-53. The adapter uses the official Claude Agent SDK as an implementation detail behind the stable
nvidia.fabric.claudeidentity and builds on the typedFabricConfig,Fabric, andRuntimeSDK surface merged in #39. One logical Fabric runtime maps to one Claude session while each invocation runs in a fresh adapter process.Details
ClaudeAgentOptions.FabricConfig.skillsthrough a runtime-scoped local Claude plugin; reject model, workspace, tools, MCP, or skills duplication underharness.settings.runtime_idand resume it across fresh adapter processes.RunResultoutput.nemo-fabric-adapters-claude, with thenemo_fabric_adapters.claudemodule andadapters/claude/descriptor path. SDK-versus-CLI transport details are not part of the consumer-facing adapter identity.Validation
uv run --no-sync pytest tests/adapters/test_claude_adapter.py tests/e2e/test_claude.py -q- 27 passed, 1 skippedjust test-python- 247 passed, 4 skippedjust wheelsnemo_fabric_adapters_claude-0.1.0-py3-none-any.whlfor the renamed module and descriptor data path.git diff --checkjust no_uv=true test-rust- 46 passed before the naming-only review update; this PR has no Rust diff.cargo check -p fabric-python --lockedandcargo fmt --all -- --checkpassed before the naming-only review update.RUN_FABRIC_CLAUDE_INTEGRATION=1and Claude authentication.Follow-up Tasks
Where should the reviewer start?
Start with
adapters/claude/src/nemo_fabric_adapters/claude/adapter.py, thentests/e2e/test_claude.py. The end-to-end test proves that the typed runtime launches a fresh Claude process for each invocation while preserving one Claude session and carrying normalized tools, MCP, skills, response, usage, cost, and events throughRunResult.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to FABRIC-53
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.