diff --git a/lib/apps/fabro-cli/src/command_context.rs b/lib/apps/fabro-cli/src/command_context.rs index bb07748e8..bbd0b6d59 100644 --- a/lib/apps/fabro-cli/src/command_context.rs +++ b/lib/apps/fabro-cli/src/command_context.rs @@ -48,13 +48,6 @@ pub(crate) struct CommandContext { catalog: OnceLock>, } -struct ResolvedCommandSettings { - storage_dir: PathBuf, - run_settings: std::result::Result, - user_settings: UserSettings, - run_settings_key_presence: RunSettingsKeyPresence, -} - impl CommandContext { pub(crate) fn from_disk(cli_layer: &CliLayer, process_local_json: bool) -> Result { let resolved_settings = load_merged_settings(cli_layer, &ServerMode::None)?; @@ -235,13 +228,10 @@ impl CommandContext { } } -fn load_merged_settings( - cli_layer: &CliLayer, - server_mode: &ServerMode, -) -> Result { - let loaded_settings = match server_mode { +fn load_merged_settings(cli_layer: &CliLayer, server_mode: &ServerMode) -> Result { + match server_mode { ServerMode::None | ServerMode::ByTarget { .. } => { - user_config::load_resolved_settings(None, None, Some(cli_layer))? + user_config::load_resolved_settings(None, None, Some(cli_layer)) } ServerMode::ByStorageDir { storage_dir_override, @@ -250,17 +240,7 @@ fn load_merged_settings( None, storage_dir_override.as_deref(), Some(cli_layer), - )?, - }; - Ok(resolve_command_settings(loaded_settings)) -} - -fn resolve_command_settings(loaded_settings: LoadedSettings) -> ResolvedCommandSettings { - ResolvedCommandSettings { - storage_dir: loaded_settings.storage_dir, - run_settings: loaded_settings.run_settings, - user_settings: loaded_settings.user_settings, - run_settings_key_presence: loaded_settings.run_settings_key_presence, + ), } } @@ -274,7 +254,7 @@ mod tests { use fabro_util::printer::Printer; use tokio::sync::OnceCell; - use super::{CommandContext, ServerMode, resolve_command_settings}; + use super::{CommandContext, ServerMode}; use crate::user_config; fn cli_layer_with_json_and_verbose() -> CliLayer { @@ -289,10 +269,9 @@ mod tests { fn synthetic_context(process_local_json: bool, printer: Printer) -> CommandContext { let cli_layer = cli_layer_with_json_and_verbose(); - let resolved_settings = resolve_command_settings( + let resolved_settings = user_config::load_resolved_settings_from_toml("_version = 1\n", None, Some(&cli_layer)) - .expect("settings should resolve"), - ); + .expect("settings should resolve"); CommandContext { printer, process_local_json, @@ -326,32 +305,28 @@ mod tests { #[test] fn storage_dir_override_only_changes_storage_root_in_merged_settings() { let cli_layer = cli_layer_with_json_and_verbose(); - let base_settings = resolve_command_settings( - user_config::load_resolved_settings_from_toml( - r#" + let base_settings = user_config::load_resolved_settings_from_toml( + r#" _version = 1 [server.storage] root = "/srv/fabro/default" "#, - None, - Some(&cli_layer), - ) - .expect("base settings should resolve"), - ); - let connection_settings = resolve_command_settings( - user_config::load_resolved_settings_from_toml( - r#" + None, + Some(&cli_layer), + ) + .expect("base settings should resolve"); + let connection_settings = user_config::load_resolved_settings_from_toml( + r#" _version = 1 [server.storage] root = "/srv/fabro/default" "#, - Some(std::path::Path::new("/srv/fabro/override")), - Some(&cli_layer), - ) - .expect("connection settings should resolve"), - ); + Some(std::path::Path::new("/srv/fabro/override")), + Some(&cli_layer), + ) + .expect("connection settings should resolve"); assert_eq!( base_settings.user_settings, @@ -395,27 +370,24 @@ root = "/srv/fabro" "fixture intentionally omits [server.auth] so server_settings should fail to resolve" ); - let resolved = resolve_command_settings(loaded); - assert_eq!(resolved.storage_dir, PathBuf::from("/srv/fabro")); - assert!(resolved.run_settings.is_ok()); + assert_eq!(loaded.storage_dir, PathBuf::from("/srv/fabro")); + assert!(loaded.run_settings.is_ok()); } #[test] fn run_settings_include_run_agent_mcps() { - let resolved = resolve_command_settings( - user_config::load_resolved_settings_from_toml( - r#" + let resolved = user_config::load_resolved_settings_from_toml( + r#" _version = 1 [run.agent.mcps.demo] type = "stdio" command = ["demo-mcp"] "#, - None, - Some(&CliLayer::default()), - ) - .expect("settings should resolve"), - ); + None, + Some(&CliLayer::default()), + ) + .expect("settings should resolve"); let run_settings = resolved.run_settings.expect("run settings should resolve"); assert!(run_settings.agent.mcps.contains_key("demo")); diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index ecd49ffe0..4688064ca 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -2,6 +2,7 @@ use std::path::Path; use anyhow::{Context as _, anyhow, bail}; use fabro_config::project; +use fabro_environment::DEFAULT_ENVIRONMENT_ID; use fabro_server::manifest_validation; use fabro_types::settings::run::EnvironmentProvider; use fabro_types::{DirtyStatus, RunId, RunIntent, RunTarget}; @@ -18,10 +19,10 @@ pub(crate) struct CreatedRun { pub(crate) run_id: RunId, } -/// Create a workflow run: allocate run directory, persist RunSpec, return -/// (run_id, run_dir). +/// Register the local workflow version closure with the server and create a +/// run from an immutable workflow intent, leaving it in the submitted state. /// -/// This does NOT execute the workflow — it only prepares the run directory. +/// This does NOT start the workflow — starting is a separate request. pub(crate) async fn create_run( ctx: &CommandContext, args: &RunArgs, @@ -43,8 +44,7 @@ pub(crate) async fn create_run( workflow_path, &canonical_cwd, Some(&user_workflows_root), - ) - .map_err(anyhow::Error::new)?; + )?; let prepared = prepare_intent_overrides(args, &canonical_cwd)?; let validation = manifest_validation::validate_collected_workflow( package.closure(), @@ -80,17 +80,35 @@ pub(crate) async fn create_run( } let client = ctx.server().await?; - let parent_id = match args.parent.as_deref() { - Some(parent_selector) => Some(resolve_run_id(client.as_ref(), parent_selector).await?), - None => None, - }; - let environment_id = args.environment.as_deref().unwrap_or("default"); - let environment = client - .retrieve_environment(environment_id) - .await - .with_context(|| format!("could not retrieve environment `{environment_id}`"))?; - let target = - run_target_for_environment(environment.settings.provider, &canonical_cwd, ctx, styles)?; + let environment_id = args + .environment + .as_deref() + .unwrap_or(DEFAULT_ENVIRONMENT_ID); + let (parent_id, environment) = tokio::try_join!( + async { + match args.parent.as_deref() { + Some(parent_selector) => Ok(Some( + resolve_run_id(client.as_ref(), parent_selector).await?, + )), + None => Ok(None), + } + }, + async { + client + .retrieve_environment(environment_id) + .await + .with_context(|| format!("could not retrieve environment `{environment_id}`")) + }, + )?; + let (target, dirty_worktree) = + run_target_for_environment(environment.settings.provider, &canonical_cwd)?; + if dirty_worktree { + fabro_util::printerr!( + ctx.printer(), + "{} the caller Git working tree is dirty; uncommitted changes are not included in the run target.", + styles.yellow.apply_to("Warning:"), + ); + } let workflow_version_id = package.closure().root_id(); client .register_workflow_versions( @@ -138,42 +156,33 @@ fn warn_untransmitted_settings( ); } +/// Derives the run target from the caller directory for the environment's +/// provider. Returns the target plus whether a clone-based observation found a +/// dirty Git worktree, so the caller can warn about it. fn run_target_for_environment( provider: EnvironmentProvider, canonical_cwd: &Path, - ctx: &CommandContext, - styles: &Styles, -) -> anyhow::Result { - match provider { - EnvironmentProvider::Local => { - let path = canonical_cwd.to_str().ok_or_else(|| { - anyhow!( - "caller working directory is not valid UTF-8: {}", - canonical_cwd.display() - ) - })?; - Ok(RunTarget::Folder { +) -> anyhow::Result<(RunTarget, bool)> { + if !provider.is_clone_based() { + let path = canonical_cwd.to_str().ok_or_else(|| { + anyhow!( + "caller working directory is not valid UTF-8: {}", + canonical_cwd.display() + ) + })?; + return Ok(( + RunTarget::Folder { path: path.to_string(), - }) - } - EnvironmentProvider::Docker | EnvironmentProvider::Daytona => { - let Some(observation) = fabro_manifest::observe_git_run_target(canonical_cwd, None) - else { - return Ok(RunTarget::None {}); - }; - if observation.legacy_git_context.dirty == DirtyStatus::Dirty { - fabro_util::printerr!( - ctx.printer(), - "{} the caller Git working tree is dirty; uncommitted changes are not included in the run target.", - styles.yellow.apply_to("Warning:"), - ); - } - let target = observation.run_target.ok_or_else(|| { - anyhow!( - "the caller Git checkout cannot be represented as a canonical GitHub run target" - ) - })?; - Ok(RunTarget::Git(target)) - } + }, + false, + )); } + let Some(observation) = fabro_manifest::observe_git_run_target(canonical_cwd, None) else { + return Ok((RunTarget::None {}, false)); + }; + let dirty = observation.legacy_git_context.dirty == DirtyStatus::Dirty; + let target = observation.run_target.ok_or_else(|| { + anyhow!("the caller Git checkout cannot be represented as a canonical GitHub run target") + })?; + Ok((RunTarget::Git(target), dirty)) } diff --git a/lib/apps/fabro-cli/src/commands/run/overrides.rs b/lib/apps/fabro-cli/src/commands/run/overrides.rs index e337d396d..d655c8698 100644 --- a/lib/apps/fabro-cli/src/commands/run/overrides.rs +++ b/lib/apps/fabro-cli/src/commands/run/overrides.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use anyhow::{Context as _, Result, anyhow, bail}; +use anyhow::{Context as _, Result, anyhow}; use fabro_config::{ CliLayer, CliOutputLayer, RunGoalLayer, RunLayer, parse_input_overrides, parse_labels, }; @@ -84,39 +84,23 @@ pub(super) fn prepare_intent_overrides( args: &RunArgs, cwd: &Path, ) -> Result { - let goal = match (args.goal.as_deref(), args.goal_file.as_deref()) { - (Some(_), Some(_)) => { - bail!("--goal and --goal-file are mutually exclusive; use exactly one") - } - (Some(goal), None) => Some(goal.to_string()), - (None, Some(path)) => { - let absolute = if path.is_absolute() { - path.to_path_buf() - } else { - cwd.join(path) - }; + let goal = match goal_layer_from_args(args.goal.as_deref(), args.goal_file.as_deref(), cwd)? { + None => None, + Some(RunGoalLayer::Inline(goal)) => Some(goal.as_source()), + Some(RunGoalLayer::File { file }) => { + let path = PathBuf::from(file.as_source()); Some( - std::fs::read_to_string(&absolute) - .with_context(|| format!("failed to read goal file {}", absolute.display()))?, + std::fs::read_to_string(&path) + .with_context(|| format!("failed to read goal file {}", path.display()))?, ) } - (None, None) => None, }; let input_overrides = parse_input_overrides(&args.inputs.values)?; let inputs = input_overrides .iter() .map(|(key, value)| { - let value = match value { - toml::Value::String(value) => serde_json::Value::String(value.clone()), - toml::Value::Integer(value) => serde_json::Value::Number((*value).into()), - toml::Value::Float(value) => serde_json::Number::from_f64(*value) - .map(serde_json::Value::Number) - .ok_or_else(|| anyhow!("input override `{key}` must be a finite float"))?, - toml::Value::Boolean(value) => serde_json::Value::Bool(*value), - toml::Value::Datetime(_) | toml::Value::Array(_) | toml::Value::Table(_) => { - bail!("input override `{key}` must be a scalar value") - } - }; + let value = fabro_types::toml_scalar_to_json_value(value) + .map_err(|error| anyhow!("input override `{key}` {error}"))?; Ok((key.clone(), value)) }) .collect::>>()?; diff --git a/lib/apps/fabro-cli/src/main.rs b/lib/apps/fabro-cli/src/main.rs index 6cb9f9788..9ab51b332 100644 --- a/lib/apps/fabro-cli/src/main.rs +++ b/lib/apps/fabro-cli/src/main.rs @@ -1312,20 +1312,6 @@ destination = "{destination}" } } - #[test] - fn run_manifest_args_preserves_input_only_manifest_args() { - let cli = Cli::try_parse_from(["fabro", "run", "workflow.toml", "-I", "foo=bar"]) - .expect("should parse"); - match *cli.command.unwrap() { - Commands::RunCmd(RunCommands::Run(args)) => { - let manifest_args = manifest_args::run_manifest_args(&args) - .expect("input-only args should be retained"); - assert_eq!(manifest_args.input, vec!["foo=bar"]); - } - _ => panic!("unexpected command variant"), - } - } - #[test] fn parse_create_input_long_flag() { let cli = Cli::try_parse_from(["fabro", "create", "workflow.toml", "--input", "foo=bar"]) diff --git a/lib/apps/fabro-cli/src/manifest_args.rs b/lib/apps/fabro-cli/src/manifest_args.rs index 73610ef16..e3857ceb3 100644 --- a/lib/apps/fabro-cli/src/manifest_args.rs +++ b/lib/apps/fabro-cli/src/manifest_args.rs @@ -1,24 +1,6 @@ use fabro_api::types; use crate::args::PreflightArgs; -#[cfg(test)] -use crate::args::RunArgs; - -#[cfg(test)] -pub(crate) fn run_manifest_args(args: &RunArgs) -> Option { - let payload = types::ManifestArgs { - auto_approve: args.auto_approve.then_some(true), - dry_run: args.dry_run.then_some(true), - label: args.label.clone(), - model: args.model.clone(), - preserve_sandbox: args.preserve_sandbox.then_some(true), - provider: args.provider.clone(), - environment: args.environment.clone(), - input: args.inputs.values.clone(), - verbose: args.verbose.then_some(true), - }; - (!fabro_manifest::manifest_args_is_empty(&payload)).then_some(payload) -} pub(crate) fn preflight_manifest_args(args: &PreflightArgs) -> Option { let payload = types::ManifestArgs { diff --git a/lib/apps/fabro-cli/src/user_config.rs b/lib/apps/fabro-cli/src/user_config.rs index 0ded61e9c..d61f73bbe 100644 --- a/lib/apps/fabro-cli/src/user_config.rs +++ b/lib/apps/fabro-cli/src/user_config.rs @@ -91,21 +91,14 @@ pub(crate) fn load_resolved_settings( pub(crate) fn read_project_run_settings_key_presence( path: &Path, ) -> anyhow::Result { + let parse_error = + |source| fabro_config::Error::parse_file("Failed to parse settings file", path, source); let source = std::fs::read_to_string(path) .map_err(|source| fabro_config::Error::read_file(path, source))?; - let document: toml::Value = toml::from_str(&source).map_err(|source| { - fabro_config::Error::parse_file( - "Failed to parse settings file", - path, - ParseError::Toml(source.to_string()), - ) - })?; - let layer = source.parse::().map_err(|source| { - fabro_config::Error::parse_file("Failed to parse settings file", path, source) - })?; - validate_settings_source(&layer, SettingsSource::Project).map_err(|source| { - fabro_config::Error::parse_file("Failed to parse settings file", path, source) - })?; + let document: toml::Value = toml::from_str(&source) + .map_err(|source| parse_error(ParseError::Toml(source.to_string())))?; + let layer = source.parse::().map_err(parse_error)?; + validate_settings_source(&layer, SettingsSource::Project).map_err(parse_error)?; Ok(RunSettingsKeyPresence::from_document(&document)) } diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index e4d1c2029..778f1814b 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -12,8 +12,10 @@ use insta::assert_snapshot; use serde_json::json; use super::support::{ - created_run_id, fixture, output_stderr, output_stdout, remote_run_summary_json, resolve_run, - run_count_for_test_case, run_state, + created_run_id, environment_json, fixture, mock_environment, + mock_workflow_version_registrations, mock_workflow_version_registrations_recording, + output_stderr, output_stdout, remote_run_summary_json, resolve_run, run_count_for_test_case, + run_git, run_state, }; use crate::support::unique_run_id; @@ -36,56 +38,6 @@ fn run_status_response(run_id: &str, status: &str) -> serde_json::Value { ) } -fn environment_json(id: &str, provider: &str) -> serde_json::Value { - json!({ - "id": id, - "revision": "0".repeat(64), - "provider": provider, - "image": { "docker": null, "dockerfile": null }, - "resources": { "cpu": null, "memory": null, "disk": null }, - "network": { "mode": "allow_all", "allow": [] }, - "lifecycle": { - "preserve": false, - "stop_on_terminal": true, - "auto_stop": null - }, - "labels": {}, - "env": {} - }) -} - -fn mock_environment<'a>(server: &'a MockServer, id: &str, provider: &str) -> Mock<'a> { - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/environments/{id}")); - then.status(200) - .header("content-type", "application/json") - .json_body(environment_json(id, provider)); - }) -} - -fn mock_workflow_version_registrations(server: &MockServer) -> Mock<'_> { - server.mock(|when, then| { - when.method("POST").path("/api/v1/workflow-versions"); - then.respond_with(|request| { - let version: fabro_types::WorkflowVersion = serde_json::from_slice(request.body_ref()) - .expect("workflow-version request body should be valid JSON"); - HttpMockResponse::builder() - .status(201) - .header("content-type", "application/json") - .body( - json!({ - "workflow_version_id": version - .id() - .expect("mocked workflow version should have a valid ID") - }) - .to_string(), - ) - .build() - }); - }) -} - fn mock_intent_create<'a>( server: &'a MockServer, run_id: &str, @@ -126,31 +78,6 @@ fn write_workflow(root: &std::path::Path, directory: &str, graph_name: &str) -> directory.join("workflow.toml") } -#[expect( - clippy::disallowed_methods, - reason = "the integration fixture needs a real local Git checkout" -)] -fn run_git(path: &std::path::Path, args: &[&str]) -> String { - let output = std::process::Command::new("git") - .args(args) - .current_dir(path) - .output() - .expect("Git fixture command should execute"); - assert!( - output.status.success(), - "git {args:?} failed: {}", - String::from_utf8_lossy(&output.stderr) - ); - String::from_utf8(output.stdout) - .expect("Git fixture output should be UTF-8") - .trim() - .to_string() -} - -fn init_git_repository(path: &std::path::Path) { - run_git(path, &["init", "--quiet"]); -} - #[test] fn help() { let context = test_context!(); @@ -235,23 +162,8 @@ fn create_defers_provider_validation_to_the_server() { let run_id = unique_run_id(); let environment_mock = mock_environment(&server, "default", "docker"); let registered_versions = Arc::new(Mutex::new(Vec::new())); - let registered_versions_for_mock = Arc::clone(®istered_versions); - let version_mock = server.mock(|when, then| { - when.method("POST").path("/api/v1/workflow-versions"); - then.respond_with(move |request| { - let version: fabro_types::WorkflowVersion = - serde_json::from_slice(request.body_ref()).unwrap(); - registered_versions_for_mock - .lock() - .unwrap() - .push(serde_json::from_slice::(request.body_ref()).unwrap()); - HttpMockResponse::builder() - .status(201) - .header("content-type", "application/json") - .body(json!({ "workflow_version_id": version.id().unwrap() }).to_string()) - .build() - }); - }); + let version_mock = + mock_workflow_version_registrations_recording(&server, Arc::clone(®istered_versions)); let mock = server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); then.status(201) @@ -617,7 +529,7 @@ fn create_preserves_named_user_other_checkout_and_loose_file_selection() { let project = tempfile::tempdir().unwrap(); let outside = tempfile::tempdir().unwrap(); let other_checkout = tempfile::tempdir().unwrap(); - init_git_repository(project.path()); + run_git(project.path(), &["init", "--quiet"]); let project_workflow = write_workflow(project.path(), ".fabro/workflows/hello", "ProjectHello"); let user_root = context.home_dir.join(".fabro/workflows"); let user_workflow = write_workflow(&user_root, "hello", "UserHello"); @@ -969,23 +881,8 @@ fn create_registers_dependencies_before_the_root_and_then_creates_once() { let run_id = unique_run_id(); let environment_mock = mock_environment(&server, "local", "local"); let registrations = Arc::new(Mutex::new(Vec::new())); - let registrations_for_mock = Arc::clone(®istrations); - let version_mock = server.mock(|when, then| { - when.method("POST").path("/api/v1/workflow-versions"); - then.respond_with(move |request| { - let version: fabro_types::WorkflowVersion = - serde_json::from_slice(request.body_ref()).unwrap(); - registrations_for_mock - .lock() - .unwrap() - .push(version.entrypoint().as_str().to_string()); - HttpMockResponse::builder() - .status(201) - .header("content-type", "application/json") - .body(json!({ "workflow_version_id": version.id().unwrap() }).to_string()) - .build() - }); - }); + let version_mock = + mock_workflow_version_registrations_recording(&server, Arc::clone(®istrations)); let requests = Arc::new(Mutex::new(Vec::::new())); let requests_for_mock = Arc::clone(&requests); let registrations_for_create = Arc::clone(®istrations); @@ -1010,7 +907,7 @@ fn create_registers_dependencies_before_the_root_and_then_creates_once() { }); }); let project = tempfile::tempdir().unwrap(); - init_git_repository(project.path()); + run_git(project.path(), &["init", "--quiet"]); write_workflow(project.path(), ".fabro/workflows/root", "Root"); write_workflow(project.path(), ".fabro/workflows/child", "Child"); std::fs::write( @@ -1047,7 +944,13 @@ fn create_registers_dependencies_before_the_root_and_then_creates_once() { environment_mock.assert(); version_mock.assert_calls(2); create_mock.assert_calls(1); - assert_eq!(registrations.lock().unwrap().as_slice(), [ + let registered_entrypoints: Vec = registrations + .lock() + .unwrap() + .iter() + .map(|version| version["entrypoint"].as_str().unwrap().to_string()) + .collect(); + assert_eq!(registered_entrypoints, [ ".fabro/workflows/child/workflow.fabro", ".fabro/workflows/root/workflow.fabro", ]); diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index 67d347c00..3dcd06ecb 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -25,7 +25,7 @@ use fabro_types::{Graph, RunId, WorkflowSettings, test_support}; use httpmock::Method::{GET, POST}; use httpmock::MockServer; -use super::support::{mock_resolved_run, remote_run_summary_json}; +use super::support::{mock_resolved_run, remote_run_summary_json, run_git}; use crate::support::{ RealAuthHarness, TEST_DEV_TOKEN, run_projection_json, seed_dev_token_auth, unique_run_id, }; diff --git a/lib/apps/fabro-cli/tests/it/cmd/run.rs b/lib/apps/fabro-cli/tests/it/cmd/run.rs index 7e10bd517..fa615a42e 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/run.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/run.rs @@ -6,11 +6,12 @@ use fabro_config::Storage; use fabro_test::{fabro_json_snapshot, fabro_snapshot, test_context}; use fabro_vault::{SecretType, Vault}; -use httpmock::{HttpMockResponse, Mock, MockServer}; +use httpmock::MockServer; use serde_json::Value; use super::support::{ - created_run_id, output_stderr, remote_run_summary_json, run_state, wait_for_event_names, + created_run_id, mock_environment, mock_workflow_version_registrations, output_stderr, + remote_run_summary_json, run_state, wait_for_event_names, }; use crate::support::{LightweightCli, run_output_filters, run_projection_json, unique_run_id}; @@ -31,52 +32,6 @@ fn run_status_response(run_id: &str, status: &str) -> serde_json::Value { ) } -fn mock_environment<'a>(server: &'a MockServer, id: &str, provider: &str) -> Mock<'a> { - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/environments/{id}")); - then.status(200) - .header("content-type", "application/json") - .json_body(serde_json::json!({ - "id": id, - "revision": "0".repeat(64), - "provider": provider, - "image": { "docker": null, "dockerfile": null }, - "resources": { "cpu": null, "memory": null, "disk": null }, - "network": { "mode": "allow_all", "allow": [] }, - "lifecycle": { - "preserve": false, - "stop_on_terminal": true, - "auto_stop": null - }, - "labels": {}, - "env": {} - })); - }) -} - -fn mock_workflow_version_registrations(server: &MockServer) -> Mock<'_> { - server.mock(|when, then| { - when.method("POST").path("/api/v1/workflow-versions"); - then.respond_with(|request| { - let version: fabro_types::WorkflowVersion = serde_json::from_slice(request.body_ref()) - .expect("workflow-version request body should be valid JSON"); - HttpMockResponse::builder() - .status(201) - .header("content-type", "application/json") - .body( - serde_json::json!({ - "workflow_version_id": version - .id() - .expect("mocked workflow version should have a valid ID") - }) - .to_string(), - ) - .build() - }); - }) -} - fn remote_run_state_response(run_id: &str) -> serde_json::Value { let mut state = run_projection_json( run_id, diff --git a/lib/apps/fabro-cli/tests/it/cmd/support.rs b/lib/apps/fabro-cli/tests/it/cmd/support.rs index 5609a105e..51e179298 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/support.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/support.rs @@ -12,6 +12,7 @@ use std::path::{Path, PathBuf}; use std::process::Output; use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; use base64::Engine as _; @@ -23,7 +24,7 @@ use fabro_store::EventEnvelope; use fabro_test::{TestContext, expect_reqwest_status}; use fabro_types::test_support::test_principal; use fabro_types::{RunId, StageId}; -use httpmock::{Mock, MockServer}; +use httpmock::{HttpMockResponse, Mock, MockServer}; use serde_json::Value; use shlex::try_quote; @@ -141,6 +142,90 @@ pub(crate) fn mock_resolved_run<'a>( }) } +/// Canonical environment response body for mock servers, matching the +/// `GET /api/v1/environments/{id}` shape the run-intent create path reads. +pub(crate) fn environment_json(id: &str, provider: &str) -> Value { + serde_json::json!({ + "id": id, + "revision": "0".repeat(64), + "provider": provider, + "image": { "docker": null, "dockerfile": null }, + "resources": { "cpu": null, "memory": null, "disk": null }, + "network": { "mode": "allow_all", "allow": [] }, + "lifecycle": { + "preserve": false, + "stop_on_terminal": true, + "auto_stop": null + }, + "labels": {}, + "env": {} + }) +} + +pub(crate) fn mock_environment<'a>(server: &'a MockServer, id: &str, provider: &str) -> Mock<'a> { + server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/environments/{id}")); + then.status(200) + .header("content-type", "application/json") + .json_body(environment_json(id, provider)); + }) +} + +pub(crate) fn mock_workflow_version_registrations(server: &MockServer) -> Mock<'_> { + mock_workflow_version_registrations_recording(server, Arc::new(Mutex::new(Vec::new()))) +} + +/// Accepts `POST /api/v1/workflow-versions`, echoing each version's +/// content-derived ID back, and records every request body into +/// `registrations` for later assertions. +pub(crate) fn mock_workflow_version_registrations_recording( + server: &MockServer, + registrations: Arc>>, +) -> Mock<'_> { + server.mock(|when, then| { + when.method("POST").path("/api/v1/workflow-versions"); + then.respond_with(move |request| { + let body: Value = serde_json::from_slice(request.body_ref()) + .expect("workflow-version request body should be valid JSON"); + let version: fabro_types::WorkflowVersion = serde_json::from_value(body.clone()) + .expect("workflow-version request body should be a workflow version"); + registrations.lock().unwrap().push(body); + HttpMockResponse::builder() + .status(201) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "workflow_version_id": version + .id() + .expect("mocked workflow version should have a valid ID") + }) + .to_string(), + ) + .build() + }); + }) +} + +/// Runs a `git` command in `path` for fixture setup, panicking on failure and +/// returning trimmed stdout. +pub(crate) fn run_git(path: &Path, args: &[&str]) -> String { + let output = std::process::Command::new("git") + .args(args) + .current_dir(path) + .output() + .expect("Git fixture command should execute"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout) + .expect("Git fixture output should be UTF-8") + .trim() + .to_string() +} + /// Snapshot filter that scrubs short (12-char) ULID suffixes from output, used /// when the CLI prints abbreviated run IDs. pub(crate) fn ulid_filter() -> (String, String) { diff --git a/lib/foundation/fabro-types/src/input_scalar.rs b/lib/foundation/fabro-types/src/input_scalar.rs index 512b9d407..726233dab 100644 --- a/lib/foundation/fabro-types/src/input_scalar.rs +++ b/lib/foundation/fabro-types/src/input_scalar.rs @@ -17,6 +17,46 @@ pub enum JsonScalarToTomlError { NumberOutOfRange, } +/// The reason a parsed TOML value cannot be represented as a JSON scalar. +#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] +pub enum TomlScalarToJsonError { + /// The TOML float is not finite, and JSON numbers must be finite. + #[error("must be a finite float")] + NonFiniteFloat, + /// TOML datetimes are outside the scalar-only conversion contract. + #[error("must be a scalar value")] + Datetime, + /// TOML arrays are outside the scalar-only conversion contract. + #[error("must be a scalar value")] + Array, + /// TOML tables are outside the scalar-only conversion contract. + #[error("must be a scalar value")] + Table, +} + +/// Converts an already-parsed TOML scalar into a JSON value. +/// +/// The inverse of [`json_scalar_to_toml_value`]: strings, booleans, and +/// integers map directly, and finite floats become JSON numbers. +/// +/// # Errors +/// +/// Returns [`TomlScalarToJsonError`] for TOML datetimes, arrays, tables, or a +/// non-finite float (JSON numbers must be finite). +pub fn toml_scalar_to_json_value(value: &toml::Value) -> Result { + match value { + toml::Value::String(value) => Ok(Value::String(value.clone())), + toml::Value::Integer(value) => Ok(Value::Number((*value).into())), + toml::Value::Float(value) => serde_json::Number::from_f64(*value) + .map(Value::Number) + .ok_or(TomlScalarToJsonError::NonFiniteFloat), + toml::Value::Boolean(value) => Ok(Value::Bool(*value)), + toml::Value::Datetime(_) => Err(TomlScalarToJsonError::Datetime), + toml::Value::Array(_) => Err(TomlScalarToJsonError::Array), + toml::Value::Table(_) => Err(TomlScalarToJsonError::Table), + } +} + /// Converts an already-parsed JSON scalar into a TOML value. /// /// Numbers are converted to a signed integer first and then to a float. As a @@ -50,7 +90,81 @@ pub fn json_scalar_to_toml_value(value: &Value) -> Result Result<(), TomlScalarToJsonError> { + let cases = [ + (toml::Value::String("hello".to_string()), json!("hello")), + (toml::Value::Integer(42), json!(42)), + (toml::Value::Float(1.25), json!(1.25)), + (toml::Value::Boolean(true), json!(true)), + ]; + + for (input, expected) in cases { + assert_eq!(toml_scalar_to_json_value(&input)?, expected); + } + + Ok(()) + } + + #[test] + fn rejects_non_scalar_toml_values_with_typed_errors() { + let datetime: toml::Value = "value = 1979-05-27T07:32:00Z" + .parse::() + .unwrap() + .remove("value") + .unwrap(); + let cases = [ + (datetime, TomlScalarToJsonError::Datetime), + ( + toml::Value::Array(vec![toml::Value::Integer(1)]), + TomlScalarToJsonError::Array, + ), + ( + toml::Value::Table(toml::Table::new()), + TomlScalarToJsonError::Table, + ), + ]; + + for (input, expected) in cases { + assert_eq!(toml_scalar_to_json_value(&input), Err(expected)); + } + } + + #[test] + fn rejects_non_finite_toml_floats() { + for input in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert_eq!( + toml_scalar_to_json_value(&toml::Value::Float(input)), + Err(TomlScalarToJsonError::NonFiniteFloat) + ); + } + } + + #[test] + fn toml_scalars_round_trip_through_json() -> Result<(), TomlScalarToJsonError> { + let scalars = [ + toml::Value::String("hello".to_string()), + toml::Value::Integer(i64::MIN), + toml::Value::Integer(i64::MAX), + toml::Value::Float(1.25), + toml::Value::Boolean(false), + ]; + + for input in scalars { + let json = toml_scalar_to_json_value(&input)?; + assert_eq!( + json_scalar_to_toml_value(&json).expect("round trip should stay scalar"), + input + ); + } + + Ok(()) + } #[test] fn converts_strings_to_toml_strings() -> Result<(), JsonScalarToTomlError> { diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index e412be73b..3e15fc39f 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -81,7 +81,10 @@ pub use graph::{ AttrValue, AttributeScope, ContextKeyAttr, Edge, Graph, KNOWN_HANDLER_TYPES, Node, OnFailure, ResolvedOnFailure, is_known_handler_type, is_llm_handler_type, shape_to_handler_type, }; -pub use input_scalar::{JsonScalarToTomlError, json_scalar_to_toml_value}; +pub use input_scalar::{ + JsonScalarToTomlError, TomlScalarToJsonError, json_scalar_to_toml_value, + toml_scalar_to_json_value, +}; pub use interview::{ InterviewQuestionRecord, QuestionType, ReviewTarget, ReviewTargetError, ReviewTargetKind, };