fix(core): harden runtime contract invariants - #25
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:
WalkthroughThe PR expands CodeRabbit review configuration, adds unsupported capability routing and schema support, extends doctor diagnostics, and adds runtime handle validation plus shutdown handling. ChangesCodeRabbit review configuration
Core runtime checks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fabric-core/src/config.rs (1)
1087-1142: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThis silently changes the public capability-routing contract.
CapabilityPlan/CapabilityTargetare serialized into the public schemas, and these branches now emitunsupportedwhere existing consumers previously sawfabric_managedplus populatedmanaged.*. Any downstream code branching onCapabilityTarget::FabricManagedor readingcapability_plan.managedwill change behavior immediately. If this rename is intentional, it needs a versioned contract or compatibility window rather than an in-place target swap.As per path instructions, "Public API changes should preserve existing behavior unless tests and docs show the intended migration path."
Also applies to: 1153-1153, 1300-1302, 1355-1356
🤖 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/config.rs` around lines 1087 - 1142, The capability-routing logic in CapabilityPlan is changing a public serialized contract by emitting CapabilityTarget::Unsupported and filling unsupported.* where consumers previously relied on CapabilityTarget::FabricManaged and managed.*. Update the mapping in the capability route construction paths (the Skills/MCP branches and any similar ones) to preserve existing behavior or add an explicit versioned migration/compatibility layer so downstream code and schemas do not break unexpectedly.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 `@crates/fabric-core/src/runtime.rs`:
- Around line 454-524: The runtime handle validation in validate_runtime_handle
still allows a cloned RunPlan with different invocation-affecting settings to
pass because it only checks a few human-readable fields. Add a stable plan
identity/fingerprint to RuntimeHandle when start_runtime() creates it, and have
validate_runtime_handle/expect_runtime_field verify that single identity instead
of comparing only agent_name, adapter_kind, runtime.mode, adapter_id, and
environment fields. Update invoke_runtime() callers accordingly so the handle is
bound to the exact plan that created it, and add a regression test that changes
harness.settings or another adapter input while keeping the existing handle.
---
Outside diff comments:
In `@crates/fabric-core/src/config.rs`:
- Around line 1087-1142: The capability-routing logic in CapabilityPlan is
changing a public serialized contract by emitting CapabilityTarget::Unsupported
and filling unsupported.* where consumers previously relied on
CapabilityTarget::FabricManaged and managed.*. Update the mapping in the
capability route construction paths (the Skills/MCP branches and any similar
ones) to preserve existing behavior or add an explicit versioned
migration/compatibility layer so downstream code and schemas do not break
unexpectedly.
🪄 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: 93ddc952-3b6a-4910-bc7b-59be8c884ccd
📒 Files selected for processing (7)
.coderabbit.yamlcrates/fabric-core/src/config.rscrates/fabric-core/src/doctor.rscrates/fabric-core/src/error.rscrates/fabric-core/src/runtime.rsschemas/adapter-invocation.schema.jsonschemas/run-plan.schema.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / Test: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 7.60s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
running 31 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 7.60s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
running 31 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (2)
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 preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/fabric-core/src/error.rscrates/fabric-core/src/doctor.rscrates/fabric-core/src/config.rscrates/fabric-core/src/runtime.rs
schemas/**/*
⚙️ CodeRabbit configuration file
schemas/**/*: Schemas are generated public contract snapshots. Check that schema diffs correspond to intentional Rust type changes and are covered by core tests.
Files:
schemas/adapter-invocation.schema.jsonschemas/run-plan.schema.json
🧬 Code graph analysis (3)
crates/fabric-core/src/error.rs (1)
crates/fabric-core/src/config.rs (1)
FabricError(15-15)
crates/fabric-core/src/doctor.rs (1)
crates/fabric-core/src/config.rs (5)
ResolutionStrategy(257-272)AdapterKind(407-416)RuntimeMode(455-462)Transport(467-476)RunPlan(1221-1255)
crates/fabric-core/src/runtime.rs (1)
crates/fabric-core/src/config.rs (3)
ControlLocation(277-282)AdapterKind(407-416)RuntimeMode(455-462)
🔇 Additional comments (3)
.coderabbit.yaml (3)
39-39: 📐 Maintainability & Code QualityConfirm these matchers use supported syntax.
Line 39 relies on
release/.*, and Lines 68-79 rely on brace alternation. If these fields accept only literal branch names or plain globs, release auto-reviews and several path instructions will silently never match.Also applies to: 68-79
52-52: 📐 Maintainability & Code Quality
docs/python-sdk/**is excluded before the docs instruction can run.Line 52 removes that subtree from review entirely, so the documentation guidance on Lines 74-76 can never apply there. If this directory is generated output, fine; otherwise this drops review coverage on a public SDK surface.
Also applies to: 74-76
85-102: 📐 Maintainability & Code QualityVerify
toolsis nested at the correct schema level.This block currently sits under
reviews:. If the current CodeRabbit schema expectstoolsat the top level, every scanner toggle here will be ignored.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fabric-core/src/runtime.rs (1)
489-500: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftValidate the full prepared environment, not just provider/control.
plan_fingerprintonly binds the handle to theRunPlan; it does not protectruntime.environmentfrom being swapped or mutated. A handle with the sameplan_fingerprintbut a differentworkspace/artifacts path would pass these checks and dispatch the adapter against the wrong prepared environment. Compare all execution-affectingEnvironmentHandlefields against the environment prepared forplan, or store/validate an environment fingerprint created atstart_runtime().🤖 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 489 - 500, The current validation in runtime.rs only checks environment.provider and environment.control_location, so a RuntimeHandle can still point at a swapped or mutated runtime.environment. Update the runtime validation around the existing expect_runtime_field checks to compare all execution-affecting EnvironmentHandle fields for the prepared environment associated with the RunPlan, or alternatively persist and verify an environment fingerprint created in start_runtime(). Use the existing runtime, plan, and expected_environment_provider/expected_control_location flow as the anchor for the fix.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.
Outside diff comments:
In `@crates/fabric-core/src/runtime.rs`:
- Around line 489-500: The current validation in runtime.rs only checks
environment.provider and environment.control_location, so a RuntimeHandle can
still point at a swapped or mutated runtime.environment. Update the runtime
validation around the existing expect_runtime_field checks to compare all
execution-affecting EnvironmentHandle fields for the prepared environment
associated with the RunPlan, or alternatively persist and verify an environment
fingerprint created in start_runtime(). Use the existing runtime, plan, and
expected_environment_provider/expected_control_location flow as the anchor for
the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 62d5fbab-6e43-46fc-8ba8-7b647f5176d8
📒 Files selected for processing (2)
crates/fabric-core/src/runtime.rsschemas/runtime-handle.schema.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / 0_Test.txt: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 7.82s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 32 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / Test: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 7.82s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 32 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (2)
schemas/**/*
⚙️ CodeRabbit configuration file
schemas/**/*: Schemas are generated public contract snapshots. Check that schema diffs correspond to intentional Rust type changes and are covered by core tests.
Files:
schemas/runtime-handle.schema.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 preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/fabric-core/src/runtime.rs
🧬 Code graph analysis (1)
crates/fabric-core/src/runtime.rs (1)
crates/fabric-core/src/config.rs (1)
RunPlan(1221-1255)
🔇 Additional comments (1)
schemas/runtime-handle.schema.json (1)
161-178: LGTM!
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 `@crates/fabric-core/src/runtime.rs`:
- Around line 466-472: The runtime validation in `runtime.rs` is still deriving
`expected_environment_fingerprint` from `runtime.environment`, which lets a
handle change invocation-affecting fields and recompute the fingerprint to
bypass checks. Update `expect_runtime_field` usage in `runtime` validation to
bind `environment_fingerprint` to the plan-derived environment (or explicitly
validate all invocation-affecting environment fields against the plan
environment), using the existing `environment_fingerprint` and
`runtime.environment`/plan-environment comparison logic. Add a regression test
that mutates both the environment fields and the fingerprint to confirm
validation fails.
🪄 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: 6a0d1e6f-5884-42af-bf67-46713cbbbf58
📒 Files selected for processing (2)
crates/fabric-core/src/runtime.rsschemas/runtime-handle.schema.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / 0_Test.txt: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 7.17s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
running 0 tests
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 33 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / Test: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 7.17s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
running 0 tests
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 33 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (2)
schemas/**/*
⚙️ CodeRabbit configuration file
schemas/**/*: Schemas are generated public contract snapshots. Check that schema diffs correspond to intentional Rust type changes and are covered by core tests.
Files:
schemas/runtime-handle.schema.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 preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/fabric-core/src/runtime.rs
🧬 Code graph analysis (1)
crates/fabric-core/src/runtime.rs (1)
crates/fabric-core/src/config.rs (1)
resolve_run_plan(673-676)
🔇 Additional comments (2)
crates/fabric-core/src/runtime.rs (1)
10-11: LGTM!Also applies to: 26-27, 232-235, 339-345, 434-434, 446-447, 458-465, 473-545, 596-597, 617-621, 643-644, 664-668, 1139-1183, 1852-1852, 1893-1896, 1991-2062, 2083-2103
schemas/runtime-handle.schema.json (1)
153-177: LGTM!
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 `@crates/fabric-core/src/runtime.rs`:
- Around line 232-233: Preserve `RuntimeHandle` JSON compatibility by updating
the `RuntimeHandle` struct so `runtime_binding` can be omitted during
deserialization, or explicitly document a breaking migration if that field must
remain required. Apply the fix on the `runtime_binding` field in `RuntimeHandle`
with serde handling that allows older persisted handles to deserialize before
`RuntimeHandleMismatch` is checked.
🪄 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: e9d72394-065a-486c-855c-767ecba52392
📒 Files selected for processing (2)
crates/fabric-core/src/runtime.rsschemas/runtime-handle.schema.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / Test: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 12.22s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
running 34 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 12.22s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
running 34 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (2)
schemas/**/*
⚙️ CodeRabbit configuration file
schemas/**/*: Schemas are generated public contract snapshots. Check that schema diffs correspond to intentional Rust type changes and are covered by core tests.
Files:
schemas/runtime-handle.schema.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 preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/fabric-core/src/runtime.rs
🧬 Code graph analysis (1)
crates/fabric-core/src/runtime.rs (1)
crates/fabric-core/src/config.rs (2)
resolve_run_plan(673-676)RunPlan(1221-1255)
🔇 Additional comments (3)
crates/fabric-core/src/runtime.rs (2)
456-462: Bind validation to the plan-derived environment.This still computes the expected binding from
runtime.environment; a caller that mutates environment fields such as workspace/artifacts and supplies a matching binding can bypass the intended plan/environment check. The regression only mutates the environment without updating the binding, so it does not cover that bypass. This is the same unresolved handle-validation concern from the previous review.As per path instructions, "Review the Rust core for runtime lifecycle correctness, handle validation, capability routing accuracy, schema stability, and error semantics."
Also applies to: 521-531, 2071-2089
Source: Path instructions
592-592: LGTM!Also applies to: 612-616, 638-638, 658-662, 1846-1848, 1887-1889, 1985-2023, 2024-2110
schemas/runtime-handle.schema.json (1)
161-172: Same compatibility concern as the Rust handle field.Making
runtime_bindingrequired in the public schema means old serialized runtime handles are rejected at schema/deserialization time instead of receiving the new mismatch error. Keep this aligned with theRuntimeHandlemigration decision.Source: Path instructions
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fabric-core/src/runtime.rs (1)
445-460: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid re-resolving the environment during stop/validation
prepare_environmentis read-only, butvalidate_runtime_handlestill depends on a freshcurrent_dir/path normalization pass. That makes teardown fail if cwd resolution breaks, even thoughruntime.environmentis already available. Reuse the stored environment for stop-path validation and avoid repeating the same normalization on every invoke.🤖 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 445 - 460, `validate_runtime_handle` is re-building environment state via `prepare_environment(plan)` during stop, which re-runs cwd/path normalization unnecessarily. Update the stop-path validation to reuse the already stored `runtime.environment` (and the existing `runtime_binding`/`validate_runtime_handle` flow) instead of resolving a fresh environment so teardown does not depend on current working directory state.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.
Outside diff comments:
In `@crates/fabric-core/src/runtime.rs`:
- Around line 445-460: `validate_runtime_handle` is re-building environment
state via `prepare_environment(plan)` during stop, which re-runs cwd/path
normalization unnecessarily. Update the stop-path validation to reuse the
already stored `runtime.environment` (and the existing
`runtime_binding`/`validate_runtime_handle` flow) instead of resolving a fresh
environment so teardown does not depend on current working directory state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d5c8d3da-ae5d-4486-8813-8fe21c1e66f4
📒 Files selected for processing (2)
crates/fabric-core/src/runtime.rsschemas/runtime-handle.schema.json
💤 Files with no reviewable changes (1)
- schemas/runtime-handle.schema.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
⚠️ CI failures not shown inline (2)
GitHub Actions: Rust / Test: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 8.42s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
running 36 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: fix(core): harden runtime contract invariants
Conclusion: failure
##[group]Run cargo test --workspace --locked
�[36;1mcargo test --workspace --locked�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 8.42s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-d6b0cd643d758f74)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-039dd9c04deba413)
running 36 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
🧰 Additional context used
📓 Path-based instructions (1)
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 preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/fabric-core/src/runtime.rs
🧬 Code graph analysis (1)
crates/fabric-core/src/runtime.rs (2)
crates/fabric-core/src/config.rs (2)
ControlLocation(277-282)EnvironmentOwnership(287-292)crates/fabric-core/src/error.rs (1)
FabricError(16-194)
🔇 Additional comments (6)
crates/fabric-core/src/runtime.rs (6)
232-234: LGTM!
554-572: LGTM!
677-689: LGTM!Also applies to: 725-737
2062-2125: LGTM!
2194-2214: LGTM!
605-619: 🗄️ Data Integrity & IntegrationDrop this concern.
runtime_bindingis intentionally an exact fingerprint of the resolvedRunPlanplus environment, and validation is meant to reject stale handles when any of those inputs drift. The config maps here useserde_json::Map/BTreeMap, notHashMap, so this is not a nondeterministic-serialization bug.> Likely an incorrect or invalid review comment.
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
5fffb2d to
a573d8d
Compare
2 participants
Summary
run_plancleans up started runtimes on invoke errors and validates runtime handles before invoke/stop dispatch.Test Plan
git diff --check HEAD~1..HEADcargo fmt --checkcargo test --workspacecargo check -p fabric-pythonpython3 tests/smoke_cli.pypython3 python/tests/smoke_native_sdk.pypython3 python/tests/smoke_typed_config.pypython3 python/tests/smoke_sdk.pyCloses FABRIC-38
Summary by CodeRabbit