feat: add adapter-selectable runtime strategies - #83
Closed
AjayThorve wants to merge 1 commit into
Closed
Conversation
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
WalkthroughFabric adds selectable adapter execution strategies, including persistent local hosts. It introduces a versioned lifecycle protocol, propagates execution strategy through plans and runtime handles, adds lifecycle schemas and validation, integrates Claude and Codex adapters, and documents the updated contracts. ChangesExecution strategy and lifecycle hosting
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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-83.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/src/nemo_fabric/types.py (1)
992-1015: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject unknown execution strategy values in Python models.
_required_textaccepts any nonempty string, while Rust and JSON Schema permit onlyprocess_per_invocation,persistent_local_host, andremote_service. Use aLiteralor enum annotation and validate the exact set in both normalizers; add rejection tests.As per coding guidelines, “Determine and update every affected public surface ... so they remain in parity.” As per path instructions, “Stubs and runtime implementations should stay aligned.”
Also applies to: 1226-1252
🤖 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 `@python/src/nemo_fabric/types.py` around lines 992 - 1015, Restrict execution_strategy to the supported values process_per_invocation, persistent_local_host, and remote_service instead of accepting any nonempty string. Update the execution_strategy annotation and validation in both normalizers, including the corresponding implementation at the additionally referenced section, keeping runtime and stub/public model surfaces aligned. Add rejection tests for unknown and invalid values while preserving acceptance of all three supported strategies.Sources: Coding guidelines, 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/codex/src/nemo_fabric_adapters/codex/adapter.py`:
- Line 32: Update the lifecycle import in the adapter module to use Ruff’s
lint-compliant import form instead of aliasing the module to the same name.
Preserve all existing references to lifecycle and avoid unrelated changes.
In `@adapters/common/src/nemo_fabric_adapters/common/lifecycle.py`:
- Line 236: Update the response output path around json.dumps in the lifecycle
handler to catch serialization failures, prevent the persistent host from
terminating, and emit a safe normalized lifecycle error response instead.
Preserve the existing successful JSON output behavior and flushing for
serializable adapter results.
In `@crates/fabric-core/src/runtime.rs`:
- Around line 1086-1088: Update the lifecycle invocation flow around
exchange_lifecycle_message and the corresponding persistent-invoke path to use
the configured lifecycle timeout or a cancellable receive instead of waiting
indefinitely while holding the host mutex. On timeout, terminate and unregister
the host, clean up any stale response state, and return the existing
timeout/error result while preserving normal successful responses and allowing
stop_runtime() to proceed.
- Around line 1263-1284: Bound persistent-host protocol output and diagnostics
across the runtime lifecycle, including the command setup and the associated
stdout/stderr consumption paths. Replace unlimited read_line-based message
handling with the existing PERSISTENT_HOST_DIAGNOSTIC_LIMIT (or equivalent
bounded framing), and route stderr through a bounded ring buffer or rotating
file instead of an unbounded host.stderr.log. Apply the same limits at the
additional persistent-host lifecycle sites noted in the review while preserving
supervision and cleanup behavior.
- Around line 974-983: Resolve adapter_effective_config(plan) before calling
spawn_persistent_host in both affected runtime lifecycle paths. Store the result
and reuse it when constructing AdapterLifecycleStart, so configuration errors
return before the child host and runtime directory are created. Preserve
existing error propagation and lifecycle behavior after successful resolution.
In `@docs/sdk/python.mdx`:
- Around line 156-163: Add a complete introductory sentence immediately before
the JSON example, explaining that the block demonstrates an adapter descriptor
declaring versioned lifecycle support; leave the example content unchanged.
In `@schemas/adapter-descriptor.schema.json`:
- Around line 24-44: Update the AdapterExecutionSupport schema to require a
nonempty strategies array, and add a conditional constraint requiring a non-null
lifecycle_contract_version whenever strategies contains persistent_local_host.
Keep the constraint scoped to that strategy while preserving support for other
execution strategies, and synchronize any checked-in JSON Schema snapshots with
the updated contract.
In `@tests/adapters/test_adapters_common_lifecycle.py`:
- Around line 120-149: Update
test_lifecycle_host_scopes_invocation_telemetry_environment to assign the host
value through os.environ[variable] instead of monkeypatch.setenv; retain the
existing lifecycle setup, assertions, and repository fixture-based environment
restoration.
In `@tests/adapters/test_claude_adapter.py`:
- Around line 972-982: Update test_main_serves_lifecycle_protocol_when_requested
to set adapter.lifecycle.CONTRACT_ENV through os.environ with the
CONTRACT_VERSION value, while retaining monkeypatch for replacing
adapter.lifecycle.serve.
In `@tests/adapters/test_codex_adapter.py`:
- Around line 584-594: Update test_main_serves_lifecycle_protocol_when_requested
to set adapter.lifecycle.CONTRACT_ENV to adapter.lifecycle.CONTRACT_VERSION via
os.environ instead of monkeypatch.setenv; retain monkeypatch for replacing
adapter.lifecycle.serve.
---
Outside diff comments:
In `@python/src/nemo_fabric/types.py`:
- Around line 992-1015: Restrict execution_strategy to the supported values
process_per_invocation, persistent_local_host, and remote_service instead of
accepting any nonempty string. Update the execution_strategy annotation and
validation in both normalizers, including the corresponding implementation at
the additionally referenced section, keeping runtime and stub/public model
surfaces aligned. Add rejection tests for unknown and invalid values while
preserving acceptance of all three supported strategies.
🪄 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: 1aff2465-033a-4034-83fd-9881850725e3
📒 Files selected for processing (144)
README.mdadapters/claude/README.mdadapters/claude/fabric-adapter.jsonadapters/claude/src/nemo_fabric_adapters/claude/adapter.pyadapters/codex/README.mdadapters/codex/fabric-adapter.jsonadapters/codex/src/nemo_fabric_adapters/codex/adapter.pyadapters/common/src/nemo_fabric_adapters/common/lifecycle.pyadapters/deepagents/fabric-adapter.jsonadapters/hermes/fabric-adapter.jsoncrates/fabric-core/src/config.rscrates/fabric-core/src/doctor.rscrates/fabric-core/src/error.rscrates/fabric-core/src/lib.rscrates/fabric-core/src/runtime.rscrates/fabric-core/src/schema.rsdocs/reference/api/python-library-reference/nemo_fabric.types.mddocs/reference/api/rust-library-reference/nemo-fabric-core/config/constant-adapter-lifecycle-contract-version.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-adapterdescriptorsource.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-adapterkind.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-capabilitykind.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-capabilitytarget.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-controllocation.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-environmentownership.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-executionstrategy.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-fabricdocument.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-mcpexposure.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatifstorageconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatofendpointfieldnamepolicy.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatofendpointtransport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayatofmode.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayotlptransport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-relayunsupportedbehavior.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-resolutionstrategy.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/enum-telemetryprovider.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-load-adapter-descriptor.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-load-fabric-document.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-resolve-effective-config-from-config.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-resolve-effective-config-with-profiles.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-resolve-effective-config.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-resolve-run-plan-from-config.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-resolve-run-plan-from-effective-config.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-resolve-run-plan-with-profiles.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-resolve-run-plan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/fn-validate-agent-directory.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-adapterconfigsupport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-adapterdescriptor.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-adapterexecutionsupport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-adapterrequirements.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-adaptertelemetryprovidersupport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-adaptertelemetrysupport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-capabilityplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-capabilityroute.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-capabilitytargetplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-effectiveconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-environmentconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-environmentplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-fabricconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-harnessconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-mcpserverplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-metadataconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-modelconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-profileconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-profileregistryconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-relayatifconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-relayatofconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-relayatofendpointconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-relaycomponentconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-relayconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-relayconfigpolicy.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-relayobservabilityconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-relayotlpconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-resolvecontext.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-resolvedadapterdescriptor.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-runplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-runtimecapabilities.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-runtimeconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-skillconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-telemetryconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-telemetryplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-telemetryproviderconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-toolsconfig.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/config/struct-toolsplan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/enum-doctorstatus.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/fn-doctor-plan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/struct-doctorcheck.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/doctor/struct-doctorreport.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/error/enum-fabricerror.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/error/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/error/type-result.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/fn-version.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/enum-adapterlifecycleoperation.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/enum-adapterlifecycleoutcome.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/enum-adapterlifecyclerequestkind.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/enum-errorstage.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/enum-runstatus.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/fn-invoke-runtime.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/fn-prepare-environment.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/fn-run-plan.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/fn-start-runtime.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/fn-stop-runtime.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/index.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-adapterinvocation.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-adapterlifecyclerequest.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-adapterlifecycleresponse.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-adapterlifecyclestart.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-adapterlifecyclestop.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-artifactmanifest.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-artifactref.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-environmenthandle.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-errorinfo.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-fabricevent.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-invocationhandle.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-runrequest.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-runresult.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-runtimecontext.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-runtimehandle.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-runtimetelemetrycontext.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/runtime/struct-telemetryref.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/schema/enum-schemaname.mdxdocs/reference/api/rust-library-reference/nemo-fabric-core/schema/index.mdxdocs/sdk/python.mdxexamples/harbor/swebench/adapters/claude/fabric-adapter.jsonexamples/harbor/swebench/adapters/hermes/fabric-adapter.jsonpython/src/nemo_fabric/types.pyschemas/SCHEMA.mdschemas/adapter-descriptor.schema.jsonschemas/adapter-invocation.schema.jsonschemas/adapter-lifecycle-request.schema.jsonschemas/adapter-lifecycle-response.schema.jsonschemas/run-plan.schema.jsonschemas/runtime-handle.schema.jsontests/adapters/test_adapters_common_lifecycle.pytests/adapters/test_claude_adapter.pytests/adapters/test_codex_adapter.pytests/python/test_code_review_example.pytests/python/test_runtime.pytests/python/test_sdk_contract.pytests/python/test_sdk_runtimes.py
|
|
||
| import nemo_fabric_adapters.common.relay_gateway as relay_gateway | ||
| import nemo_fabric_adapters.common.relay_hooks as relay_hooks | ||
| import nemo_fabric_adapters.common.lifecycle as lifecycle |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the lint-compliant lifecycle import.
Ruff PLR0402 flags this alias form.
Proposed fix
-import nemo_fabric_adapters.common.lifecycle as lifecycle
+from nemo_fabric_adapters.common import lifecycle📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import nemo_fabric_adapters.common.lifecycle as lifecycle | |
| from nemo_fabric_adapters.common import lifecycle |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 32-32: Use from nemo_fabric_adapters.common import lifecycle in lieu of alias
Replace with from nemo_fabric_adapters.common import lifecycle
(PLR0402)
🤖 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/codex/src/nemo_fabric_adapters/codex/adapter.py` at line 32, Update
the lifecycle import in the adapter module to use Ruff’s lint-compliant import
form instead of aliasing the module to the same name. Preserve all existing
references to lifecycle and avoid unrelated changes.
Source: Linters/SAST tools
| ), | ||
| ) | ||
| should_stop = False | ||
| print(json.dumps(response, sort_keys=True), file=output_stream, flush=True) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep response serialization inside the protocol boundary.
A non-JSON-serializable adapter output raises here and terminates the persistent host without a normalized response. Catch serialization failures and emit a safe lifecycle error.
Proposed fix
- print(json.dumps(response, sort_keys=True), file=output_stream, flush=True)
+ try:
+ encoded = json.dumps(response, sort_keys=True)
+ except (TypeError, ValueError) as error:
+ operation = response.get("operation")
+ if not isinstance(operation, str):
+ operation = "start"
+ print(
+ f"Invalid lifecycle response: {error}",
+ file=sys.stderr,
+ flush=True,
+ )
+ encoded = json.dumps(
+ _response(
+ operation,
+ error=_error(
+ operation,
+ "lifecycle_invalid_response",
+ "Adapter returned an invalid lifecycle response",
+ ),
+ ),
+ sort_keys=True,
+ )
+ print(encoded, file=output_stream, flush=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| print(json.dumps(response, sort_keys=True), file=output_stream, flush=True) | |
| try: | |
| encoded = json.dumps(response, sort_keys=True) | |
| except (TypeError, ValueError) as error: | |
| operation = response.get("operation") | |
| if not isinstance(operation, str): | |
| operation = "start" | |
| print( | |
| f"Invalid lifecycle response: {error}", | |
| file=sys.stderr, | |
| flush=True, | |
| ) | |
| encoded = json.dumps( | |
| _response( | |
| operation, | |
| error=_error( | |
| operation, | |
| "lifecycle_invalid_response", | |
| "Adapter returned an invalid lifecycle response", | |
| ), | |
| ), | |
| sort_keys=True, | |
| ) | |
| print(encoded, file=output_stream, flush=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapters/common/src/nemo_fabric_adapters/common/lifecycle.py` at line 236,
Update the response output path around json.dumps in the lifecycle handler to
catch serialization failures, prevent the persistent host from terminating, and
emit a safe normalized lifecycle error response instead. Preserve the existing
successful JSON output behavior and flushing for serializable adapter results.
Comment on lines
+974
to
+983
| let mut host = spawn_persistent_host(plan, &runtime)?; | ||
| let request = AdapterLifecycleRequest::new(AdapterLifecycleRequestKind::Start( | ||
| AdapterLifecycleStart { | ||
| runtime: runtime.clone(), | ||
| effective_config: adapter_effective_config(plan)?, | ||
| capability_plan: plan.capability_plan.clone(), | ||
| capabilities: plan.capabilities.clone(), | ||
| telemetry_plan: plan.telemetry_plan.clone(), | ||
| }, | ||
| )); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Resolve the effective configuration before spawning the host.
Line 978 can fail after Line 974 starts the child. That returns without terminating the unregistered process or removing its runtime directory.
Proposed fix
- let mut host = spawn_persistent_host(plan, &runtime)?;
+ let effective_config = adapter_effective_config(plan)?;
+ let mut host = spawn_persistent_host(plan, &runtime)?;
let request = AdapterLifecycleRequest::new(AdapterLifecycleRequestKind::Start(
AdapterLifecycleStart {
runtime: runtime.clone(),
- effective_config: adapter_effective_config(plan)?,
+ effective_config,As per path instructions, review the Rust core for runtime lifecycle correctness and error semantics.
Also applies to: 2196-2201
🤖 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/runtime.rs` around lines 974 - 983, Resolve
adapter_effective_config(plan) before calling spawn_persistent_host in both
affected runtime lifecycle paths. Store the result and reuse it when
constructing AdapterLifecycleStart, so configuration errors return before the
child host and runtime directory are created. Preserve existing error
propagation and lifecycle behavior after successful resolution.
Source: Path instructions
Comment on lines
+1086
to
+1088
| let mut host = host.lock().unwrap_or_else(|error| error.into_inner()); | ||
| let output = | ||
| exchange_lifecycle_message(&mut host, &runtime.runtime_id, &lifecycle_request, None)?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound or cancel persistent invoke waits.
The host mutex is held while recv() waits without a timeout. A live host that never returns a newline blocks the invocation forever, and concurrent stop_runtime() also blocks on that mutex. Use a configurable deadline or cancellable receive, then terminate and unregister the host on timeout to avoid stale responses.
The PR objective explicitly promises lifecycle timeouts and cleanup.
Also applies to: 1456-1497
🤖 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/runtime.rs` around lines 1086 - 1088, Update the
lifecycle invocation flow around exchange_lifecycle_message and the
corresponding persistent-invoke path to use the configured lifecycle timeout or
a cancellable receive instead of waiting indefinitely while holding the host
mutex. On timeout, terminate and unregister the host, clean up any stale
response state, and return the existing timeout/error result while preserving
normal successful responses and allowing stop_runtime() to proceed.
Comment on lines
+1263
to
+1284
| let stderr_path = runtime_dir.join("host.stderr.log"); | ||
| let stderr = File::create(&stderr_path).map_err(|source| FabricError::Write { | ||
| path: stderr_path.clone(), | ||
| source, | ||
| })?; | ||
| let (mut command, command_display) = match persistent_host_command(plan, runtime) { | ||
| Ok(command) => command, | ||
| Err(error) => { | ||
| let _ = std::fs::remove_dir_all(&runtime_dir); | ||
| return Err(error); | ||
| } | ||
| }; | ||
| command | ||
| .env( | ||
| "FABRIC_ADAPTER_LIFECYCLE_CONTRACT", | ||
| ADAPTER_LIFECYCLE_CONTRACT_VERSION, | ||
| ) | ||
| .env("FABRIC_RUNTIME_ID", &runtime.runtime_id) | ||
| .env("FABRIC_HOME", &runtime_dir) | ||
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::from(stderr)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound persistent-host stdout and stderr consumption.
read_line() permits an unlimited response line, while stderr writes to an unlimited file for the host’s lifetime. A malformed or noisy adapter can exhaust memory or disk; PERSISTENT_HOST_DIAGNOSTIC_LIMIT only limits later reads. Cap protocol messages and drain stderr into a bounded ring buffer or rotating file.
As per path instructions, review the Rust core for runtime lifecycle correctness and supervision.
Also applies to: 1319-1340, 1594-1600
🤖 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/runtime.rs` around lines 1263 - 1284, Bound
persistent-host protocol output and diagnostics across the runtime lifecycle,
including the command setup and the associated stdout/stderr consumption paths.
Replace unlimited read_line-based message handling with the existing
PERSISTENT_HOST_DIAGNOSTIC_LIMIT (or equivalent bounded framing), and route
stderr through a bounded ring buffer or rotating file instead of an unbounded
host.stderr.log. Apply the same limits at the additional persistent-host
lifecycle sites noted in the review while preserving supervision and cleanup
behavior.
Source: Path instructions
Comment on lines
+156
to
+163
| ```json | ||
| { | ||
| "execution": { | ||
| "lifecycle_contract_version": "fabric.adapter.lifecycle/v1alpha1", | ||
| "strategies": ["process_per_invocation", "persistent_local_host"] | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Nice to have — Introduce the JSON example.
Add a complete lead-in before this block, such as: “An adapter descriptor declares versioned lifecycle support as follows:”
As per coding guidelines, introduce every code block with a complete sentence.
🤖 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 `@docs/sdk/python.mdx` around lines 156 - 163, Add a complete introductory
sentence immediately before the JSON example, explaining that the block
demonstrates an adapter descriptor declaring versioned lifecycle support; leave
the example content unchanged.
Source: Coding guidelines
Comment on lines
+24
to
+44
| "AdapterExecutionSupport": { | ||
| "additionalProperties": true, | ||
| "description": "Execution strategies implemented by an adapter.", | ||
| "properties": { | ||
| "lifecycle_contract_version": { | ||
| "description": "Version of the external start/invoke/stop contract used by persistent strategies.", | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ] | ||
| }, | ||
| "strategies": { | ||
| "description": "Execution strategies implemented by this adapter.", | ||
| "items": { | ||
| "$ref": "#/$defs/ExecutionStrategy" | ||
| }, | ||
| "type": "array", | ||
| "uniqueItems": true | ||
| } | ||
| }, | ||
| "type": "object" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Must fix — Require a lifecycle version for persistent hosts.
persistent_local_host can validate without lifecycle_contract_version, and execution can contain no strategies. Require a nonempty strategies array and conditionally require a non-null version when it contains persistent_local_host; otherwise schema-valid external descriptors can fail only later during planning.
Proposed schema constraint
"AdapterExecutionSupport": {
"additionalProperties": true,
+ "required": ["strategies"],
+ "allOf": [
+ {
+ "if": {
+ "properties": {
+ "strategies": {
+ "contains": { "const": "persistent_local_host" }
+ }
+ }
+ },
+ "then": {
+ "properties": {
+ "lifecycle_contract_version": {
+ "type": "string",
+ "minLength": 1
+ }
+ },
+ "required": ["lifecycle_contract_version"]
+ }
+ }
+ ],
"properties": {
...
"strategies": {
+ "minItems": 1,As per coding guidelines, public contract changes must keep checked-in JSON Schema snapshots synchronized; this snapshot should encode the versioned lifecycle constraint.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "AdapterExecutionSupport": { | |
| "additionalProperties": true, | |
| "description": "Execution strategies implemented by an adapter.", | |
| "properties": { | |
| "lifecycle_contract_version": { | |
| "description": "Version of the external start/invoke/stop contract used by persistent strategies.", | |
| "type": [ | |
| "string", | |
| "null" | |
| ] | |
| }, | |
| "strategies": { | |
| "description": "Execution strategies implemented by this adapter.", | |
| "items": { | |
| "$ref": "#/$defs/ExecutionStrategy" | |
| }, | |
| "type": "array", | |
| "uniqueItems": true | |
| } | |
| }, | |
| "type": "object" | |
| "AdapterExecutionSupport": { | |
| "additionalProperties": true, | |
| "allOf": [ | |
| { | |
| "if": { | |
| "properties": { | |
| "strategies": { | |
| "contains": { "const": "persistent_local_host" } | |
| } | |
| } | |
| }, | |
| "then": { | |
| "properties": { | |
| "lifecycle_contract_version": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "required": ["lifecycle_contract_version"] | |
| } | |
| } | |
| ], | |
| "description": "Execution strategies implemented by an adapter.", | |
| "properties": { | |
| "lifecycle_contract_version": { | |
| "description": "Version of the external start/invoke/stop contract used by persistent strategies.", | |
| "type": [ | |
| "string", | |
| "null" | |
| ] | |
| }, | |
| "strategies": { | |
| "description": "Execution strategies implemented by this adapter.", | |
| "items": { | |
| "$ref": "`#/`$defs/ExecutionStrategy" | |
| }, | |
| "minItems": 1, | |
| "type": "array", | |
| "uniqueItems": true | |
| } | |
| }, | |
| "required": ["strategies"], | |
| "type": "object" |
🤖 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 `@schemas/adapter-descriptor.schema.json` around lines 24 - 44, Update the
AdapterExecutionSupport schema to require a nonempty strategies array, and add a
conditional constraint requiring a non-null lifecycle_contract_version whenever
strategies contains persistent_local_host. Keep the constraint scoped to that
strategy while preserving support for other execution strategies, and
synchronize any checked-in JSON Schema snapshots with the updated contract.
Source: Coding guidelines
Comment on lines
+120
to
+149
| def test_lifecycle_host_scopes_invocation_telemetry_environment(monkeypatch): | ||
| runtime_id = "runtime-1" | ||
| variable = "FABRIC_TEST_LIFECYCLE_ENV" | ||
| monkeypatch.setenv(variable, "host-value") | ||
| requests = [ | ||
| _request("start", {"runtime": {"runtime_id": runtime_id}}), | ||
| _request( | ||
| "invoke", | ||
| { | ||
| "runtime_context": { | ||
| "runtime_id": runtime_id, | ||
| "telemetry": {"env": {variable: "invocation-value"}}, | ||
| }, | ||
| "request": {"input": "hello"}, | ||
| }, | ||
| ), | ||
| _request("stop", {"runtime_id": runtime_id}), | ||
| ] | ||
| input_stream = io.StringIO("".join(f"{json.dumps(item)}\n" for item in requests)) | ||
| output_stream = io.StringIO() | ||
|
|
||
| lifecycle.serve( | ||
| lambda _payload: {"value": os.environ[variable]}, | ||
| input_stream=input_stream, | ||
| output_stream=output_stream, | ||
| ) | ||
|
|
||
| responses = [json.loads(line) for line in output_stream.getvalue().splitlines()] | ||
| assert responses[1]["outcome"]["output"] == {"value": "invocation-value"} | ||
| assert os.environ[variable] == "host-value" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use os.environ for the test environment override.
Replace monkeypatch.setenv(variable, "host-value") with os.environ[variable] = "host-value"; the repository’s autouse fixture restores the environment.
As per coding guidelines, “Use os.environ to modify environment variables in tests; do not use monkeypatch.setenv.”
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 137-137: use jsonify instead of json.dumps for JSON output
Context: json.dumps(item)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/adapters/test_adapters_common_lifecycle.py` around lines 120 - 149,
Update test_lifecycle_host_scopes_invocation_telemetry_environment to assign the
host value through os.environ[variable] instead of monkeypatch.setenv; retain
the existing lifecycle setup, assertions, and repository fixture-based
environment restoration.
Source: Coding guidelines
Comment on lines
+972
to
+982
| def test_main_serves_lifecycle_protocol_when_requested(monkeypatch): | ||
| serve = MagicMock() | ||
| monkeypatch.setenv( | ||
| adapter.lifecycle.CONTRACT_ENV, | ||
| adapter.lifecycle.CONTRACT_VERSION, | ||
| ) | ||
| monkeypatch.setattr(adapter.lifecycle, "serve", serve) | ||
|
|
||
| adapter.main() | ||
|
|
||
| serve.assert_called_once_with(adapter.run) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Set the lifecycle contract through os.environ.
Keep monkeypatch for setattr, but assign the environment variable through os.environ.
As per coding guidelines, “Use os.environ to modify environment variables in tests; do not use monkeypatch.setenv.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/adapters/test_claude_adapter.py` around lines 972 - 982, Update
test_main_serves_lifecycle_protocol_when_requested to set
adapter.lifecycle.CONTRACT_ENV through os.environ with the CONTRACT_VERSION
value, while retaining monkeypatch for replacing adapter.lifecycle.serve.
Source: Coding guidelines
Comment on lines
+584
to
+594
| def test_main_serves_lifecycle_protocol_when_requested(monkeypatch): | ||
| serve = MagicMock() | ||
| monkeypatch.setenv( | ||
| adapter.lifecycle.CONTRACT_ENV, | ||
| adapter.lifecycle.CONTRACT_VERSION, | ||
| ) | ||
| monkeypatch.setattr(adapter.lifecycle, "serve", serve) | ||
|
|
||
| adapter.main() | ||
|
|
||
| serve.assert_called_once_with(adapter.run) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Set the lifecycle contract through os.environ.
Keep monkeypatch for replacing serve, but assign the environment variable through os.environ.
As per coding guidelines, “Use os.environ to modify environment variables in tests; do not use monkeypatch.setenv.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/adapters/test_codex_adapter.py` around lines 584 - 594, Update
test_main_serves_lifecycle_protocol_when_requested to set
adapter.lifecycle.CONTRACT_ENV to adapter.lifecycle.CONTRACT_VERSION via
os.environ instead of monkeypatch.setenv; retain monkeypatch for replacing
adapter.lifecycle.serve.
Source: Coding guidelines
Collaborator
Author
|
Closing as superseded by a narrower FABRIC-59 recut. This prototype exposed adapter execution mechanics as a northbound strategy, and its persistence tests prove host-process reuse rather than native SDK-client reuse. The replacement will define a private, versioned adapter runtime lifecycle, retain the per-invocation path as a compatibility driver, and prove stateful execution with a real Claude SDK client. The branch is intentionally retained for design reference. |
This was referenced Jul 17, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds the normalized, adapter-selectable runtime execution strategy contract for FABRIC-59 while preserving the existing
Fabric.start_runtime()andRuntime.invoke()APIs and the backward-compatibleprocess_per_invocationdefault.The shared vocabulary contains
process_per_invocation,persistent_local_host, andremote_service. Claude and Codex currently declare the first two.remote_serviceis validated and fails closed when an adapter does not declare a real remote runtime transport; it does not silently fall back to per-invocation execution.Details
harness.settings.runtime_strategyand rejects unknown or adapter-unsupported values during planning.fabric.adapter.lifecycle/v1alpha1start/invoke/stop contract.process_per_invocation; no first-party adapter currently advertisesremote_service.Example usage
Select and verify persistent local hosting without starting either adapter:
Reuse one persistent adapter host for ordered invocations:
Omit
runtime_strategyto retain the compatibility default; the resolved plan and runtime handle reportprocess_per_invocation.Validation
cargo fmt --all -- --checkjust test-rust— 73 passedenv PATH="$HOME/.local/bin:$PATH" just test-python— 382 passed, 11 skippedgit diff --check origin/main...HEADjust docsrecipe was not used becausenpmis not on this shell's defaultPATH; its generators and validator were run separately.Where should the reviewer start?
Start with
ExecutionStrategyandAdapterExecutionSupportincrates/fabric-core/src/config.rs, then review the persistent-host lifecycle and failure semantics incrates/fabric-core/src/runtime.rs. The adapter-side protocol endpoint is isolated inadapters/common/src/nemo_fabric_adapters/common/lifecycle.py.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-59
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.