diff --git a/.coderabbit.yaml b/.coderabbit.yaml index cd08a04a1..128771fd7 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 + 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" 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..a89d697eb 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)] @@ -328,7 +332,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 +427,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 +439,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 +451,77 @@ pub fn stop_runtime(_plan: &RunPlan, runtime: &RuntimeHandle) -> Result Result<()> { + expect_runtime_field( + plan, + runtime, + "agent_name", + &plan.agent_name, + &runtime.agent_name, + )?; + expect_runtime_field( + plan, + runtime, + "harness_type", + &harness_type(plan), + &runtime.harness_type, + )?; + expect_runtime_field( + plan, + runtime, + "runtime.mode", + &runtime_mode_name(plan.config.runtime.mode), + &runtime_mode_name(runtime.mode), + )?; + expect_runtime_field( + plan, + runtime, + "adapter_kind", + &adapter_kind_name(adapter_kind(plan)), + &adapter_kind_name(runtime.adapter_kind), + )?; + expect_runtime_field( + plan, + runtime, + "adapter_id", + &optional_runtime_value(adapter_id(plan).as_deref()), + &optional_runtime_value(runtime.adapter_id.as_deref()), + )?; + expect_runtime_field( + plan, + runtime, + "environment.provider", + &expected_environment_provider(plan), + &runtime.environment.provider, + )?; + expect_runtime_field( + plan, + runtime, + "environment.control_location", + &control_location_name(expected_control_location(plan)), + &control_location_name(runtime.environment.control_location), + )?; + Ok(()) +} + +fn expect_runtime_field( + _plan: &RunPlan, + 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(Debug, Clone, PartialEq, Serialize, Deserialize)] struct ProcessAdapterSettings { command: String, @@ -506,6 +589,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), @@ -546,6 +634,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 +1109,51 @@ 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 control_location_name(control_location: ControlLocation) -> String { + match control_location { + ControlLocation::ExternalControl => "external_control", + ControlLocation::InEnvControl => "in_env_control", + } + .to_string() +} + +fn expected_environment_provider(plan: &RunPlan) -> String { + plan.environment_plan + .as_ref() + .map(|environment| environment.provider.clone()) + .unwrap_or_else(|| "local".to_string()) +} + +fn expected_control_location(plan: &RunPlan) -> ControlLocation { + plan.environment_plan + .as_ref() + .map(|environment| environment.control_location) + .unwrap_or(ControlLocation::ExternalControl) +} + +fn optional_runtime_value(value: Option<&str>) -> String { + value.unwrap_or("").to_string() +} + fn adapter_exit_error( code: &str, default_message: &str, @@ -1684,10 +1822,7 @@ 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")); 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 +1863,10 @@ runtime: }"# } + fn stopped_agents() -> Vec { + TEST_STOPPED_AGENTS.lock().expect("stop tracker").clone() + } + #[test] fn prepare_environment_absolutizes_workspace() { let root = @@ -1822,6 +1961,74 @@ 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 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 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." },