From 809039b66c104f971f6b452c8533a5317af5aa6a Mon Sep 17 00:00:00 2001 From: Ajay Thorve Date: Sat, 27 Jun 2026 18:47:52 -0700 Subject: [PATCH 1/2] chore: configure CodeRabbit reviews Signed-off-by: Ajay Thorve --- .coderabbit.yaml | 101 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 93 insertions(+), 8 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index cd08a04a1..d4d2f752b 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -2,24 +2,109 @@ # SPDX-License-Identifier: Apache-2.0 # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json -# CodeRabbit configuration: https://docs.coderabbit.ai/reference/configuration -# -# Intentionally minimal: NeMo Fabric is an early POC and the codebase is still -# changing, so this avoids per-path rules and strict merge gates. Tighten as the -# contract stabilizes. +# Docs: https://docs.coderabbit.ai/reference/configuration language: "en-US" -tone_instructions: "Be concise and technical. Prioritize correctness and maintainability over style nits." +tone_instructions: "Be concise, technical, and specific. Prioritize correctness, safety, and maintainability over style nits." reviews: - profile: chill + profile: assertive + review_status: true + review_details: true + collapse_walkthrough: false request_changes_workflow: false + pre_merge_checks: + title: + mode: error + requirements: >- + Title must follow Conventional Commits format: + type(optional-scope)[!]: concise imperative summary + + Allowed types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert. + Use lowercase type and scope. If the change is a breaking change, add a ! suffix before the colon. + Keep it under 72 characters and do not use a trailing period. + description: + mode: warning + issue_assessment: + mode: warning + auto_title_placeholder: "@coderabbitai" + auto_title_instructions: | + Generate the PR title using Conventional Commits format: + type(optional-scope)[!]: concise imperative summary + + Allowed types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert. + Prefer scopes from this repository when clear: core, cli, python, schemas, adapters, docs, ci, deps + Use lowercase type and scope. If the change is a breaking change, add a ! suffix before the colon. + Keep it under 72 characters and do not use a trailing period. auto_review: + base_branches: ["main", "release/.*"] enabled: true - drafts: false + drafts: true + auto_incremental_review: true path_filters: + - "!target/**" - "!**/target/**" + - "!.venv/**" - "!**/.venv/**" - "!**/node_modules/**" + - "!**/.pytest_cache/**" + - "!**/.ruff_cache/**" - "!.tmp/**" - "!docs/python-sdk/**" + path_instructions: + - path: "crates/fabric-core/src/**/*.rs" + instructions: | + 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. + - path: "crates/fabric-python/**/*" + instructions: | + Treat native binding changes as public API changes. Check JSON/type conversion, error propagation, GIL/thread behavior, and parity with the Python SDK. + - path: "python/src/nemo_fabric/**/*" + instructions: | + Review Python SDK changes for typed API consistency, import-time dependency neutrality, async/session behavior, and parity with the native extension. + Stubs and runtime implementations should stay aligned. + - path: "schemas/**/*" + instructions: | + Schemas are generated public contract snapshots. Check that schema diffs correspond to intentional Rust type changes and are covered by core tests. + - path: "{tests/**,python/tests/**}" + instructions: | + Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant. + - path: "{adapters/**,examples/**}" + instructions: | + Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts. + - path: "{docs/**,README.md,AGENTS.md}" + instructions: | + Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas. + - path: "**/SKILL.md" + instructions: | + Do not flag SKILL.md files for missing SPDX headers. Skill entrypoints intentionally start with YAML frontmatter instead. + Verify that every SKILL.md keeps valid YAML frontmatter with at least name and description fields before the Markdown body. poem: false + sequence_diagrams: true + suggested_reviewers: false + + tools: + clippy: + enabled: true + ruff: + enabled: true + shellcheck: + enabled: true + yamllint: + enabled: true + markdownlint: + enabled: true + gitleaks: + enabled: true + osvScanner: + enabled: true + semgrep: + enabled: true + +knowledge_base: + code_guidelines: + enabled: true + filePatterns: + - "AGENTS.md" + - "CLAUDE.md" + - "CONTRIBUTING.md" + - ".agents/skills/**/*.md" From a573d8df8beaae3698de663734a9c52eef3b1921 Mon Sep 17 00:00:00 2001 From: Ajay Thorve Date: Sat, 27 Jun 2026 18:47:59 -0700 Subject: [PATCH 2/2] fix(core): harden runtime contract invariants Signed-off-by: Ajay Thorve --- crates/fabric-core/src/config.rs | 123 ++++++-- crates/fabric-core/src/doctor.rs | 124 +++++++- crates/fabric-core/src/error.rs | 14 + crates/fabric-core/src/runtime.rs | 387 ++++++++++++++++++++++++- schemas/adapter-invocation.schema.json | 17 +- schemas/run-plan.schema.json | 17 +- schemas/runtime-handle.schema.json | 5 + 7 files changed, 654 insertions(+), 33 deletions(-) diff --git a/crates/fabric-core/src/config.rs b/crates/fabric-core/src/config.rs index 04416192c..85821f6a1 100644 --- a/crates/fabric-core/src/config.rs +++ b/crates/fabric-core/src/config.rs @@ -1070,7 +1070,8 @@ fn resolve_capability_plan( .unwrap_or_default(); let tools_are_native = config.tools.is_some() && accepts("tools"); let mut native = CapabilityTargetPlan::default(); - let mut managed = CapabilityTargetPlan::default(); + let managed = CapabilityTargetPlan::default(); + let mut unsupported = CapabilityTargetPlan::default(); let mut routes = Vec::new(); if config.tools.is_some() { @@ -1083,12 +1084,12 @@ fn resolve_capability_plan( reason: "selected adapter accepts Fabric tools config".to_string(), }); } else { - managed.tools_configured = true; + unsupported.tools_configured = true; routes.push(CapabilityRoute { kind: CapabilityKind::Tools, name: "tools".to_string(), - target: CapabilityTarget::FabricManaged, - reason: "selected adapter does not declare native tools support".to_string(), + target: CapabilityTarget::Unsupported, + reason: "selected adapter does not declare native tools support and Fabric-managed tools are not implemented".to_string(), }); } } @@ -1103,12 +1104,12 @@ fn resolve_capability_plan( reason: "selected adapter accepts Fabric skills config".to_string(), }); } else { - managed.skill_paths = skill_paths.clone(); + unsupported.skill_paths = skill_paths.clone(); routes.push(CapabilityRoute { kind: CapabilityKind::Skills, name: "skills".to_string(), - target: CapabilityTarget::FabricManaged, - reason: "selected adapter does not declare native skills support".to_string(), + target: CapabilityTarget::Unsupported, + reason: "selected adapter does not declare native skills support and Fabric-managed skills are not implemented".to_string(), }); } } @@ -1128,16 +1129,16 @@ fn resolve_capability_plan( ), }); } else { - managed.mcp_servers.insert(name.clone(), server.clone()); + unsupported.mcp_servers.insert(name.clone(), server.clone()); routes.push(CapabilityRoute { kind: CapabilityKind::Mcp, name: name.clone(), - target: CapabilityTarget::FabricManaged, + target: CapabilityTarget::Unsupported, reason: match server.exposure { McpExposure::FabricManaged => { - "MCP server explicitly requests Fabric-managed exposure".to_string() + "MCP server explicitly requests Fabric-managed exposure but Fabric-managed MCP is not implemented".to_string() } - _ => "selected adapter does not declare native MCP support".to_string(), + _ => "selected adapter does not declare native MCP support and Fabric-managed MCP is not implemented".to_string(), }, }); } @@ -1149,6 +1150,7 @@ fn resolve_capability_plan( mcp_servers, native, managed, + unsupported, routes, } } @@ -1295,6 +1297,9 @@ pub struct CapabilityPlan { /// Capabilities that Fabric must expose or manage outside the native harness config. #[serde(default)] pub managed: CapabilityTargetPlan, + /// Capabilities that are configured but not executable by this Fabric build. + #[serde(default)] + pub unsupported: CapabilityTargetPlan, /// Routing decisions made while resolving the effective config. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub routes: Vec, @@ -1347,6 +1352,8 @@ pub enum CapabilityTarget { HarnessNative, /// Fabric exposes or manages the capability around the harness. FabricManaged, + /// Capability is configured but no executable surface exists. + Unsupported, } /// Resolved MCP server exposure. @@ -1559,18 +1566,17 @@ environment: Some(1) ); assert!(plan.capability_plan.native.mcp_servers.is_empty()); + assert!(plan.capability_plan.managed.mcp_servers.is_empty()); assert!( - plan.capability_plan - .managed - .mcp_servers - .contains_key("github") + plan.capability_plan.routes.iter().any( + |route| route.name == "github" && route.target == CapabilityTarget::Unsupported + ) ); assert!( plan.capability_plan - .routes - .iter() - .any(|route| route.name == "github" - && route.target == CapabilityTarget::FabricManaged) + .unsupported + .mcp_servers + .contains_key("github") ); } @@ -1594,9 +1600,10 @@ environment: .map(|telemetry| telemetry.relay_enabled), Some(true) ); + assert!(plan.capability_plan.managed.mcp_servers.is_empty()); assert!( plan.capability_plan - .managed + .unsupported .mcp_servers .contains_key("github") ); @@ -1620,15 +1627,89 @@ environment: assert_eq!(plan.profiles, vec!["mcp_github"]); assert!(plan.config_path.ends_with("agent.yaml")); assert!(plan.config.profiles.directories.is_empty()); + assert!(plan.capability_plan.managed.mcp_servers.is_empty()); assert!( plan.capability_plan - .managed + .unsupported .mcp_servers .contains_key("github") ); assert_eq!(plan.config_root, root); } + #[test] + fn unsupported_capabilities_do_not_claim_fabric_managed_execution() { + let root = std::env::temp_dir().join(format!( + "fabric-unsupported-capability-test-{}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join("adapters/minimal")).expect("create adapters"); + std::fs::create_dir_all(root.join("skills/review")).expect("create skills"); + std::fs::write( + root.join("agent.yaml"), + r#"schema_version: fabric.agent/v1alpha1 +metadata: + name: unsupported-capability-agent +harness: + adapter_id: acme.fabric.minimal +models: + default: + provider: test + model: test-model +runtime: + mode: oneshot + transport: cli + input_schema: text + output_schema: text +tools: + - name: shell +skills: + paths: + - ./skills/review +mcp: + servers: + github: + transport: streamable-http + url: http://example.invalid/mcp + exposure: fabric_managed +"#, + ) + .expect("write agent config"); + std::fs::write( + root.join("adapters/minimal/fabric-adapter.json"), + r#"{ + "adapter_id": "acme.fabric.minimal", + "adapter_kind": "process" +}"#, + ) + .expect("write adapter descriptor"); + + let plan = resolve_run_plan(&root, None).expect("run plan"); + + assert!(!plan.capability_plan.managed.tools_configured); + assert!(plan.capability_plan.managed.skill_paths.is_empty()); + assert!(plan.capability_plan.managed.mcp_servers.is_empty()); + assert!(plan.capability_plan.unsupported.tools_configured); + assert_eq!(plan.capability_plan.unsupported.skill_paths.len(), 1); + assert!( + plan.capability_plan + .unsupported + .mcp_servers + .contains_key("github") + ); + assert!( + plan.capability_plan + .routes + .iter() + .all(|route| route.target == CapabilityTarget::Unsupported), + "{:?}", + plan.capability_plan.routes + ); + + let _ = std::fs::remove_dir_all(root); + } + #[test] fn later_profiles_override_earlier_profiles() { let profiles = vec!["env_opensandbox".to_string(), "env_local".to_string()]; diff --git a/crates/fabric-core/src/doctor.rs b/crates/fabric-core/src/doctor.rs index cbb997abe..016fd150c 100644 --- a/crates/fabric-core/src/doctor.rs +++ b/crates/fabric-core/src/doctor.rs @@ -10,7 +10,10 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use serde_json::Value; -use crate::config::{ControlLocation, EnvironmentOwnership, ResolutionStrategy, RunPlan}; +use crate::config::{ + AdapterKind, CapabilityTarget, ControlLocation, EnvironmentOwnership, ResolutionStrategy, + RunPlan, RuntimeMode, Transport, +}; /// Diagnostic status. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] @@ -57,7 +60,9 @@ pub fn doctor_plan(plan: &RunPlan) -> DoctorReport { let mut checks = Vec::new(); checks.push(check_adapter_descriptor(plan)); checks.push(check_resolution(plan)); + checks.extend(check_runtime_execution_surface(plan)); checks.push(check_environment_context(plan)); + checks.extend(check_capability_routes(plan)); checks.extend(check_requirements(plan)); let status = checks.iter().fold(DoctorStatus::Pass, |status, check| { worst(status, check.status) @@ -113,6 +118,9 @@ fn check_resolution(plan: &RunPlan) -> DoctorCheck { }; let message = match status { DoctorStatus::Pass => format!("selected resolution strategy `{resolution:?}`"), + DoctorStatus::Warn if matches!(resolution, ResolutionStrategy::Service) => { + "selected resolution strategy `service` is modeled but not implemented by Fabric runtime execution".to_string() + } DoctorStatus::Warn => format!( "selected resolution strategy `{resolution:?}` is declared but not executed by this POC" ), @@ -121,6 +129,46 @@ fn check_resolution(plan: &RunPlan) -> DoctorCheck { check("resolution", status, message) } +fn check_runtime_execution_surface(plan: &RunPlan) -> Vec { + let mut checks = Vec::new(); + match plan.config.runtime.mode { + RuntimeMode::Service => checks.push(check( + "runtime.mode", + DoctorStatus::Warn, + "runtime mode `service` is modeled but not implemented by Fabric runtime dispatch", + )), + RuntimeMode::Oneshot | RuntimeMode::Session => {} + } + match plan.config.runtime.transport { + Transport::Http => checks.push(check( + "runtime.transport", + DoctorStatus::Warn, + "runtime transport `http` is modeled but not implemented by Fabric runtime dispatch", + )), + Transport::NativePlugin => checks.push(check( + "runtime.transport", + DoctorStatus::Warn, + "runtime transport `native_plugin` is modeled but not implemented by Fabric runtime dispatch", + )), + Transport::Library | Transport::Cli => {} + } + let Some(adapter) = &plan.adapter_descriptor else { + return checks; + }; + match adapter.descriptor.adapter_kind { + AdapterKind::Http | AdapterKind::NativePlugin => checks.push(check( + "runtime.adapter", + DoctorStatus::Warn, + format!( + "`{}` adapter runtime dispatch is not implemented", + adapter_kind_name(adapter.descriptor.adapter_kind) + ), + )), + AdapterKind::Process | AdapterKind::Python => {} + } + checks +} + fn check_environment_context(plan: &RunPlan) -> DoctorCheck { let Some(environment) = &plan.environment_plan else { return check( @@ -154,6 +202,24 @@ fn check_environment_context(plan: &RunPlan) -> DoctorCheck { ) } +fn check_capability_routes(plan: &RunPlan) -> Vec { + plan.capability_plan + .routes + .iter() + .filter(|route| route.target == CapabilityTarget::Unsupported) + .map(|route| { + check( + "capability.unsupported", + DoctorStatus::Warn, + format!( + "{:?} capability `{}` is configured but not executable: {}", + route.kind, route.name, route.reason + ), + ) + }) + .collect() +} + fn check_requirements(plan: &RunPlan) -> Vec { match plan.resolution { Some(ResolutionStrategy::ImageProvided) => return check_image_provided_requirements(plan), @@ -310,6 +376,15 @@ fn resolution_name(resolution: Option) -> &'static str { } } +fn adapter_kind_name(adapter_kind: AdapterKind) -> &'static str { + match adapter_kind { + AdapterKind::Process => "process", + AdapterKind::Http => "http", + AdapterKind::Python => "python", + AdapterKind::NativePlugin => "native_plugin", + } +} + fn command_available(binary: &str) -> bool { let path = Path::new(binary); if path.components().count() > 1 { @@ -416,7 +491,9 @@ mod tests { use serde_json::Value; use super::*; - use crate::config::{ResolutionStrategy, resolve_run_plan}; + use crate::config::{ + AdapterKind, ResolutionStrategy, RuntimeMode, Transport, resolve_run_plan, + }; fn example_agent_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../examples/code-review-agent") @@ -484,4 +561,47 @@ mod tests { && check.message.contains("hermes_command") })); } + + #[test] + fn doctor_reports_service_and_http_execution_as_modeled_not_implemented() { + let mut plan = resolve_run_plan(example_agent_dir(), None).expect("run plan"); + plan.config.runtime.mode = RuntimeMode::Service; + plan.config.runtime.transport = Transport::Http; + plan.resolution = Some(ResolutionStrategy::Service); + plan.adapter_descriptor + .as_mut() + .expect("adapter descriptor") + .descriptor + .adapter_kind = AdapterKind::Http; + + let report = doctor_plan(&plan); + + assert_eq!(report.status, DoctorStatus::Warn); + assert!(report.checks.iter().any(|check| { + check.name == "runtime.mode" + && check.status == DoctorStatus::Warn + && check.message.contains("modeled but not implemented") + && check.message.contains("service") + })); + assert!(report.checks.iter().any(|check| { + check.name == "runtime.transport" + && check.status == DoctorStatus::Warn + && check.message.contains("modeled but not implemented") + && check.message.contains("http") + })); + assert!(report.checks.iter().any(|check| { + check.name == "runtime.adapter" + && check.status == DoctorStatus::Warn + && check + .message + .contains("runtime dispatch is not implemented") + && check.message.contains("http") + })); + assert!(report.checks.iter().any(|check| { + check.name == "resolution" + && check.status == DoctorStatus::Warn + && check.message.contains("modeled but not implemented") + && check.message.contains("service") + })); + } } diff --git a/crates/fabric-core/src/error.rs b/crates/fabric-core/src/error.rs index db894daf7..d8fa6ab05 100644 --- a/crates/fabric-core/src/error.rs +++ b/crates/fabric-core/src/error.rs @@ -109,6 +109,20 @@ pub enum FabricError { /// Adapter kind. adapter_kind: AdapterKind, }, + /// A runtime handle was used with a different run plan than the one that created it. + #[error( + "runtime handle does not match run plan for `{field}`: expected `{expected}` but found `{actual}` (runtime `{runtime_id}`)" + )] + RuntimeHandleMismatch { + /// Mismatched runtime handle field. + field: &'static str, + /// Expected value from the run plan. + expected: String, + /// Actual value from the runtime handle. + actual: String, + /// Runtime handle id. + runtime_id: String, + }, /// An environment provider is not runnable for the selected adapter in this POC. #[error("environment provider `{provider}` is not implemented for adapter `{adapter_kind:?}`")] UnsupportedEnvironmentProvider { diff --git a/crates/fabric-core/src/runtime.rs b/crates/fabric-core/src/runtime.rs index 16a038c71..03dfda4aa 100644 --- a/crates/fabric-core/src/runtime.rs +++ b/crates/fabric-core/src/runtime.rs @@ -7,6 +7,8 @@ use std::collections::BTreeMap; use std::io::{ErrorKind, Write}; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; +#[cfg(test)] +use std::sync::Mutex; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{SystemTime, UNIX_EPOCH}; @@ -21,6 +23,8 @@ use crate::config::{ use crate::error::{FabricError, Result}; static NEXT_ID: AtomicU64 = AtomicU64::new(1); +#[cfg(test)] +static TEST_STOPPED_AGENTS: Mutex> = Mutex::new(Vec::new()); /// A request passed to a Fabric-managed harness runtime. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema, Default)] @@ -225,6 +229,8 @@ pub struct EnvironmentHandle { pub struct RuntimeHandle { /// Runtime handle id. pub runtime_id: String, + /// Fabric-owned opaque binding for this runtime handle. + pub runtime_binding: String, /// Agent name. pub agent_name: String, /// Harness type. @@ -328,7 +334,13 @@ struct RelayRuntimeConfig { /// Invoke a Fabric run plan. pub fn run_plan(plan: &RunPlan, request: RunRequest) -> Result { let runtime = start_runtime(plan)?; - let mut result = invoke_runtime(plan, &runtime, request)?; + let mut result = match invoke_runtime(plan, &runtime, request) { + Ok(result) => result, + Err(error) => { + let _ = stop_runtime(plan, &runtime); + return Err(error); + } + }; result.events.extend(stop_runtime(plan, &runtime)?); Ok(result) } @@ -417,6 +429,7 @@ pub fn invoke_runtime( runtime: &RuntimeHandle, request: RunRequest, ) -> Result { + validate_runtime_handle(plan, runtime)?; match adapter_kind(plan) { AdapterKind::Process => ProcessAdapter.invoke(plan, runtime, request), AdapterKind::Python => PythonAdapter.invoke(plan, runtime, request), @@ -428,7 +441,8 @@ pub fn invoke_runtime( } /// Stop or detach from a harness runtime. -pub fn stop_runtime(_plan: &RunPlan, runtime: &RuntimeHandle) -> Result> { +pub fn stop_runtime(plan: &RunPlan, runtime: &RuntimeHandle) -> Result> { + validate_runtime_handle(plan, runtime)?; match runtime.adapter_kind { AdapterKind::Process => ProcessAdapter.stop(runtime), AdapterKind::Python => PythonAdapter.stop(runtime), @@ -439,6 +453,116 @@ pub fn stop_runtime(_plan: &RunPlan, runtime: &RuntimeHandle) -> Result Result<()> { + let expected_binding = runtime_binding(&runtime.runtime_id, plan, &runtime.environment)?; + expect_runtime_field( + runtime, + "runtime_binding", + &expected_binding, + &runtime.runtime_binding, + )?; + expect_runtime_field(runtime, "agent_name", &plan.agent_name, &runtime.agent_name)?; + expect_runtime_field( + runtime, + "harness_type", + &harness_type(plan), + &runtime.harness_type, + )?; + expect_runtime_field( + runtime, + "runtime.mode", + &runtime_mode_name(plan.config.runtime.mode), + &runtime_mode_name(runtime.mode), + )?; + expect_runtime_field( + runtime, + "adapter_kind", + &adapter_kind_name(adapter_kind(plan)), + &adapter_kind_name(runtime.adapter_kind), + )?; + expect_runtime_field( + runtime, + "adapter_id", + &optional_runtime_value(adapter_id(plan).as_deref()), + &optional_runtime_value(runtime.adapter_id.as_deref()), + )?; + Ok(()) +} + +fn expect_runtime_field( + runtime: &RuntimeHandle, + field: &'static str, + expected: &str, + actual: &str, +) -> Result<()> { + if expected == actual { + return Ok(()); + } + Err(FabricError::RuntimeHandleMismatch { + field, + expected: expected.to_string(), + actual: actual.to_string(), + runtime_id: runtime.runtime_id.clone(), + }) +} + +#[derive(Serialize)] +struct RuntimeBindingMaterial<'a> { + runtime_id: &'a str, + environment_id: &'a str, + plan: &'a RunPlan, + environment: RuntimeEnvironmentBinding<'a>, +} + +#[derive(Serialize)] +struct RuntimeEnvironmentBinding<'a> { + provider: &'a str, + control_location: ControlLocation, + workspace: &'a Option, + artifacts: &'a Option, + ownership: EnvironmentOwnership, + connection: &'a BTreeMap, + metadata: &'a BTreeMap, +} + +fn runtime_environment_binding(environment: &EnvironmentHandle) -> RuntimeEnvironmentBinding<'_> { + RuntimeEnvironmentBinding { + provider: &environment.provider, + control_location: environment.control_location, + workspace: &environment.workspace, + artifacts: &environment.artifacts, + ownership: environment.ownership, + connection: &environment.connection, + metadata: &environment.metadata, + } +} + +fn runtime_binding( + runtime_id: &str, + plan: &RunPlan, + environment: &EnvironmentHandle, +) -> Result { + stable_hash( + "fabric-runtime-binding", + &RuntimeBindingMaterial { + runtime_id, + environment_id: &environment.environment_id, + plan, + environment: runtime_environment_binding(environment), + }, + ) +} + +fn stable_hash(prefix: &str, value: &T) -> Result { + let bytes = serde_json::to_vec(value).map_err(FabricError::SerializeJson)?; + let mut hash = 0xcbf29ce484222325_u64; + for byte in bytes { + hash ^= u64::from(byte); + hash = hash.wrapping_mul(0x100000001b3); + } + Ok(format!("{prefix}-{hash:016x}")) +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] struct ProcessAdapterSettings { command: String, @@ -485,8 +609,11 @@ impl RuntimeAdapter for ProcessAdapter { adapter_kind: AdapterKind::Process, }); } + let runtime_id = new_id("runtime"); + let runtime_binding = runtime_binding(&runtime_id, plan, &environment)?; Ok(RuntimeHandle { - runtime_id: new_id("runtime"), + runtime_id, + runtime_binding, agent_name: plan.agent_name.clone(), harness_type: harness_type(plan), mode: plan.config.runtime.mode, @@ -506,6 +633,11 @@ impl RuntimeAdapter for ProcessAdapter { } fn stop(&self, runtime: &RuntimeHandle) -> Result> { + #[cfg(test)] + TEST_STOPPED_AGENTS + .lock() + .expect("stop tracker") + .push(runtime.agent_name.clone()); Ok(vec![event_with_metadata( "runtime_stop", format!("stopped runtime {}", runtime.runtime_id), @@ -525,8 +657,11 @@ impl RuntimeAdapter for PythonAdapter { adapter_kind: AdapterKind::Python, }); } + let runtime_id = new_id("runtime"); + let runtime_binding = runtime_binding(&runtime_id, plan, &environment)?; Ok(RuntimeHandle { - runtime_id: new_id("runtime"), + runtime_id, + runtime_binding, agent_name: plan.agent_name.clone(), harness_type: harness_type(plan), mode: plan.config.runtime.mode, @@ -546,6 +681,11 @@ impl RuntimeAdapter for PythonAdapter { } fn stop(&self, runtime: &RuntimeHandle) -> Result> { + #[cfg(test)] + TEST_STOPPED_AGENTS + .lock() + .expect("stop tracker") + .push(runtime.agent_name.clone()); Ok(vec![event_with_metadata( "runtime_stop", format!("stopped runtime {}", runtime.runtime_id), @@ -1016,6 +1156,29 @@ fn adapter_kind(plan: &RunPlan) -> AdapterKind { .unwrap_or(AdapterKind::Process) } +fn adapter_kind_name(adapter_kind: AdapterKind) -> String { + match adapter_kind { + AdapterKind::Process => "process", + AdapterKind::Http => "http", + AdapterKind::Python => "python", + AdapterKind::NativePlugin => "native_plugin", + } + .to_string() +} + +fn runtime_mode_name(mode: RuntimeMode) -> String { + match mode { + RuntimeMode::Oneshot => "oneshot", + RuntimeMode::Service => "service", + RuntimeMode::Session => "session", + } + .to_string() +} + +fn optional_runtime_value(value: Option<&str>) -> String { + value.unwrap_or("").to_string() +} + fn adapter_exit_error( code: &str, default_message: &str, @@ -1684,10 +1847,11 @@ mod tests { } fn temp_process_agent_dir() -> PathBuf { - let root = std::env::temp_dir().join(format!( - "fabric-process-adapter-test-{}", - std::process::id() - )); + let root = std::env::temp_dir().join(new_id("fabric-process-adapter-test")); + process_agent_dir(root) + } + + fn process_agent_dir(root: PathBuf) -> PathBuf { let _ = fs::remove_dir_all(&root); fs::create_dir_all(&root).expect("create agent dir"); fs::create_dir_all(root.join("adapters/process")).expect("create adapters dir"); @@ -1728,6 +1892,10 @@ runtime: }"# } + fn stopped_agents() -> Vec { + TEST_STOPPED_AGENTS.lock().expect("stop tracker").clone() + } + #[test] fn prepare_environment_absolutizes_workspace() { let root = @@ -1822,6 +1990,209 @@ environment: let _ = fs::remove_dir_all(root); } + #[test] + fn run_plan_stops_runtime_after_invoke_error() { + let root = temp_process_agent_dir(); + let mut plan = resolve_run_plan(&root, None).expect("run plan"); + plan.agent_name = new_id("invoke-error-agent"); + plan.effective_config.agent_name = plan.agent_name.clone(); + plan.config.harness.settings.remove("command"); + let agent_name = plan.agent_name.clone(); + + let error = run_plan(&plan, RunRequest::text("hello fabric")).expect_err("invoke error"); + + assert!( + error + .to_string() + .contains("invalid process adapter settings"), + "{error}" + ); + assert!( + stopped_agents().contains(&agent_name), + "run_plan must stop the started runtime for {agent_name}" + ); + + let _ = fs::remove_dir_all(root); + } + + #[test] + fn runtime_handle_exposes_single_opaque_binding() { + let root = temp_process_agent_dir(); + let plan = resolve_run_plan(&root, None).expect("run plan"); + let runtime = start_runtime(&plan).expect("runtime"); + + let value = serde_json::to_value(&runtime).expect("runtime json"); + assert!(value.get("runtime_binding").is_some()); + assert!(value.get("plan_fingerprint").is_none()); + assert!(value.get("environment_fingerprint").is_none()); + + let _ = fs::remove_dir_all(root); + } + + #[test] + fn runtime_handle_without_binding_is_rejected_during_deserialization() { + let root = temp_process_agent_dir(); + let plan = resolve_run_plan(&root, None).expect("run plan"); + let runtime = start_runtime(&plan).expect("runtime"); + let mut value = serde_json::to_value(&runtime).expect("runtime json"); + value + .as_object_mut() + .expect("runtime object") + .remove("runtime_binding"); + let error = serde_json::from_value::(value) + .expect_err("runtime binding must be required"); + + assert!( + error + .to_string() + .contains("missing field `runtime_binding`"), + "{error}" + ); + + let _ = fs::remove_dir_all(root); + } + + #[test] + fn runtime_handle_validation_is_independent_of_current_directory() { + const CHILD_ENV: &str = "FABRIC_TEST_RUNTIME_HANDLE_CWD_CHILD"; + if std::env::var_os(CHILD_ENV).is_none() { + let output = Command::new(std::env::current_exe().expect("current test executable")) + .arg("runtime_handle_validation_is_independent_of_current_directory") + .arg("--nocapture") + .env(CHILD_ENV, "1") + .output() + .expect("run isolated cwd test"); + assert!( + output.status.success(), + "isolated cwd test failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let parent = std::env::temp_dir().join(new_id("fabric-runtime-cwd-test")); + let root = process_agent_dir(parent.join("agent")); + fs::create_dir_all(parent.join("elsewhere")).expect("create alternate cwd"); + std::env::set_current_dir(&parent).expect("enter fixture parent"); + let plan = resolve_run_plan(Path::new("agent"), None).expect("relative run plan"); + let runtime = start_runtime(&plan).expect("runtime"); + + std::env::set_current_dir(parent.join("elsewhere")).expect("change cwd"); + validate_runtime_handle(&plan, &runtime).expect("valid runtime handle"); + + std::env::set_current_dir(std::env::temp_dir()).expect("leave fixture"); + let _ = fs::remove_dir_all(root.parent().expect("fixture parent")); + } + + #[test] + fn invoke_runtime_rejects_runtime_handle_from_different_plan() { + let root = temp_process_agent_dir(); + let plan = resolve_run_plan(&root, None).expect("run plan"); + let runtime = start_runtime(&plan).expect("runtime"); + let mut other_plan = plan.clone(); + other_plan.agent_name = "other-agent".to_string(); + other_plan.effective_config.agent_name = "other-agent".to_string(); + + let error = invoke_runtime(&other_plan, &runtime, RunRequest::text("hello fabric")) + .expect_err("runtime mismatch"); + + assert!( + error + .to_string() + .contains("runtime handle does not match run plan"), + "{error}" + ); + + let _ = fs::remove_dir_all(root); + } + + #[test] + fn invoke_runtime_rejects_runtime_handle_from_mutated_adapter_settings() { + let root = temp_process_agent_dir(); + let plan = resolve_run_plan(&root, None).expect("run plan"); + let runtime = start_runtime(&plan).expect("runtime"); + let mut other_plan = plan.clone(); + other_plan + .config + .harness + .settings + .insert("command".to_string(), Value::String("printf".to_string())); + + let error = invoke_runtime(&other_plan, &runtime, RunRequest::text("hello fabric")) + .expect_err("runtime mismatch"); + + assert!( + error + .to_string() + .contains("runtime handle does not match run plan"), + "{error}" + ); + + let _ = fs::remove_dir_all(root); + } + + #[test] + fn invoke_runtime_rejects_mutated_runtime_environment() { + let root = temp_process_agent_dir(); + let plan = resolve_run_plan(&root, None).expect("run plan"); + let mut runtime = start_runtime(&plan).expect("runtime"); + runtime.environment.workspace = Some(root.join("other-workspace")); + + let error = invoke_runtime(&plan, &runtime, RunRequest::text("hello fabric")) + .expect_err("runtime mismatch"); + + assert!( + error + .to_string() + .contains("runtime handle does not match run plan"), + "{error}" + ); + + let _ = fs::remove_dir_all(root); + } + + #[test] + fn invoke_runtime_rejects_mutated_runtime_identity() { + let root = temp_process_agent_dir(); + let plan = resolve_run_plan(&root, None).expect("run plan"); + let mut runtime = start_runtime(&plan).expect("runtime"); + runtime.harness_type = "other-harness".to_string(); + + let error = invoke_runtime(&plan, &runtime, RunRequest::text("hello fabric")) + .expect_err("runtime mismatch"); + + assert!( + error + .to_string() + .contains("runtime handle does not match run plan"), + "{error}" + ); + + let _ = fs::remove_dir_all(root); + } + + #[test] + fn stop_runtime_rejects_runtime_handle_from_different_plan() { + let root = temp_process_agent_dir(); + let plan = resolve_run_plan(&root, None).expect("run plan"); + let runtime = start_runtime(&plan).expect("runtime"); + let mut other_plan = plan.clone(); + other_plan.agent_name = "other-agent".to_string(); + other_plan.effective_config.agent_name = "other-agent".to_string(); + + let error = stop_runtime(&other_plan, &runtime).expect_err("runtime mismatch"); + + assert!( + error + .to_string() + .contains("runtime handle does not match run plan"), + "{error}" + ); + + let _ = fs::remove_dir_all(root); + } + #[test] fn run_promotes_relay_artifacts_into_artifact_manifest() { let root = std::env::temp_dir().join(format!( diff --git a/schemas/adapter-invocation.schema.json b/schemas/adapter-invocation.schema.json index 1014ae34d..8aa3f1d8a 100644 --- a/schemas/adapter-invocation.schema.json +++ b/schemas/adapter-invocation.schema.json @@ -112,6 +112,13 @@ "default": false, "description": "Whether tool configuration was provided.", "type": "boolean" + }, + "unsupported": { + "$ref": "#/$defs/CapabilityTargetPlan", + "default": { + "tools_configured": false + }, + "description": "Capabilities that are configured but not executable by this Fabric build." } }, "type": "object" @@ -156,6 +163,11 @@ "const": "fabric_managed", "description": "Fabric exposes or manages the capability around the harness.", "type": "string" + }, + { + "const": "unsupported", + "description": "Capability is configured but no executable surface exists.", + "type": "string" } ] }, @@ -958,7 +970,10 @@ "native": { "tools_configured": false }, - "tools_configured": false + "tools_configured": false, + "unsupported": { + "tools_configured": false + } }, "description": "Derived capability routing plan for the selected adapter." }, diff --git a/schemas/run-plan.schema.json b/schemas/run-plan.schema.json index 4fc021caa..fd2289970 100644 --- a/schemas/run-plan.schema.json +++ b/schemas/run-plan.schema.json @@ -214,6 +214,13 @@ "default": false, "description": "Whether tool configuration was provided.", "type": "boolean" + }, + "unsupported": { + "$ref": "#/$defs/CapabilityTargetPlan", + "default": { + "tools_configured": false + }, + "description": "Capabilities that are configured but not executable by this Fabric build." } }, "type": "object" @@ -258,6 +265,11 @@ "const": "fabric_managed", "description": "Fabric exposes or manages the capability around the harness.", "type": "string" + }, + { + "const": "unsupported", + "description": "Capability is configured but no executable surface exists.", + "type": "string" } ] }, @@ -999,7 +1011,10 @@ "native": { "tools_configured": false }, - "tools_configured": false + "tools_configured": false, + "unsupported": { + "tools_configured": false + } }, "description": "Resolved capability configuration." }, diff --git a/schemas/runtime-handle.schema.json b/schemas/runtime-handle.schema.json index 647e787dc..f1f405c20 100644 --- a/schemas/runtime-handle.schema.json +++ b/schemas/runtime-handle.schema.json @@ -158,6 +158,10 @@ "$ref": "#/$defs/RuntimeMode", "description": "Runtime mode." }, + "runtime_binding": { + "description": "Fabric-owned opaque binding for this runtime handle.", + "type": "string" + }, "runtime_id": { "description": "Runtime handle id.", "type": "string" @@ -165,6 +169,7 @@ }, "required": [ "runtime_id", + "runtime_binding", "agent_name", "harness_type", "mode",