From 1a1c5ab29d9b263117763a366f430cb0a8444710 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 28 Aug 2026 20:40:38 -0400 Subject: [PATCH 1/3] Add RunIntent registration support --- docs/public/api-reference/fabro-api.yaml | 15 + .../fabro-server/src/server/handler/runs.rs | 28 +- lib/apps/fabro-server/src/server/tests.rs | 145 +++++++++ .../fabro-api/tests/run_intent_round_trip.rs | 14 +- lib/foundation/fabro-client/src/client.rs | 300 +++++++++++++++++- lib/foundation/fabro-types/src/run_intent.rs | 14 +- .../fabro-types/tests/run_intent.rs | 37 ++- .../src/models/run-intent-args.ts | 12 + 8 files changed, 536 insertions(+), 29 deletions(-) diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 4b2a56e5f..15b856a55 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -9296,6 +9296,21 @@ components: type: object additionalProperties: type: string + dry_run: + type: boolean + description: >- + Overrides `run.execution.mode`: true selects `dry_run`, false + selects `normal`, and omission inherits the lower-precedence setting. + auto_approve: + type: boolean + description: >- + Overrides `run.execution.approval`: true selects `auto`, false + selects `prompt`, and omission inherits the lower-precedence setting. + preserve_sandbox: + type: boolean + description: >- + Overrides `run.environment.lifecycle.preserve`; omission inherits + the lower-precedence setting. RunTarget: description: Workspace content and location requested for a run. diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index c3867a3f4..b2b6269cb 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -18,10 +18,11 @@ use fabro_api::types::{ BoardColumn, ManifestConfigType, ManifestGoalType, RunIntent, RunManifest, SubmitAnswerRequest, UpdateRunParentRequest, UpdateRunRequest, }; -use fabro_config::{CliLayer, ReplaceMap, RunEnvironmentLayer, RunLayer, RunModelLayer, Storage}; +use fabro_config::{CliLayer, RunLayer, Storage}; use fabro_environment::{DEFAULT_ENVIRONMENT_ID, EnvironmentId}; use fabro_interview::AnswerSubmission; use fabro_llm::client::Client as LlmClient; +use fabro_manifest::RunOverrideInput; use fabro_static::EnvVars; use fabro_store::{ RunSummaryListQuery, RunSummarySort, RunSummarySortDirection, RunSummaryVisibility, @@ -701,21 +702,16 @@ pub(crate) async fn create_run_from_intent( }; input_overrides.insert(name.clone(), value); } - let mut run_overrides = RunLayer { - environment: Some(RunEnvironmentLayer { - id: Some(environment_id.to_string()), - ..RunEnvironmentLayer::default() - }), - metadata: ReplaceMap::from(intent.args.labels), - ..RunLayer::default() - }; - if intent.args.model.is_some() || intent.args.provider.is_some() { - run_overrides.model = Some(RunModelLayer { - name: intent.args.model, - provider: intent.args.provider, - ..RunModelLayer::default() - }); - } + let run_overrides = fabro_manifest::build_run_overrides(RunOverrideInput { + goal: None, + model: intent.args.model.as_deref(), + provider: intent.args.provider.as_deref(), + environment: Some(environment_id.as_str()), + preserve_sandbox: intent.args.preserve_sandbox, + dry_run: intent.args.dry_run, + auto_approve: intent.args.auto_approve, + labels: intent.args.labels, + }); let entrypoint = lowered.entrypoint.clone(); let raw_compiler_input = RawRunCompilerInput { diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index e934a02e9..c5958e307 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -3777,6 +3777,151 @@ async fn post_runs_run_intent_creates_submitted_none_target_without_git_projecti assert!(projection.spec.definition_blob.is_some()); } +#[tokio::test] +async fn post_runs_run_intent_args_true_override_resolved_settings_without_starting() { + let dir = tempfile::tempdir().unwrap(); + let workspace = dir.path().join("workspace"); + std::fs::create_dir(&workspace).unwrap(); + let state = TestAppStateBuilder::new() + .default_environment_provider(Some(EnvironmentProvider::Local)) + .env_lookup(|_| None) + .vault_entries([(EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) + .build(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; + let body = post_run_manifest( + &app, + json!({ + "workflow_version_id": workflow_version_id, + "target": { "kind": "folder", "path": &workspace }, + "args": { + "dry_run": true, + "auto_approve": true, + "preserve_sandbox": true + } + }), + ) + .await; + let run_id = body["id"].as_str().unwrap().parse::().unwrap(); + + assert_eq!(body["lifecycle"]["status"]["kind"], "submitted"); + let run_store = state.stores.runs.open_run_reader(&run_id).await.unwrap(); + let projection = run_store.state().await.unwrap(); + assert_eq!( + projection.spec.settings.run.execution.mode, + fabro_types::settings::run::RunMode::DryRun + ); + assert_eq!( + projection.spec.settings.run.execution.approval, + fabro_types::settings::run::ApprovalMode::Auto + ); + assert!(projection.spec.settings.run.environment.lifecycle.preserve); +} + +#[tokio::test] +async fn post_runs_run_intent_args_false_are_distinct_from_omitted_overrides() { + let dir = tempfile::tempdir().unwrap(); + let workspace = dir.path().join("workspace"); + std::fs::create_dir(&workspace).unwrap(); + let state = TestAppStateBuilder::new() + .runtime_settings( + default_test_server_settings(), + manifest_run_defaults_from_toml( + r#" +[run.execution] +mode = "dry_run" +approval = "auto" + +[run.environment.lifecycle] +preserve = true +"#, + ), + ) + .default_environment_provider(Some(EnvironmentProvider::Local)) + .env_lookup(|_| None) + .vault_entries([(EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) + .build(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; + + let explicit_false = post_run_manifest( + &app, + json!({ + "workflow_version_id": workflow_version_id, + "target": { "kind": "folder", "path": &workspace }, + "args": { + "dry_run": false, + "auto_approve": false, + "preserve_sandbox": false + } + }), + ) + .await; + let omitted = post_run_manifest( + &app, + json!({ + "workflow_version_id": workflow_version_id, + "target": { "kind": "folder", "path": &workspace }, + "args": {} + }), + ) + .await; + + assert_eq!(explicit_false["lifecycle"]["status"]["kind"], "submitted"); + assert_eq!(omitted["lifecycle"]["status"]["kind"], "submitted"); + let explicit_false_id = explicit_false["id"] + .as_str() + .unwrap() + .parse::() + .unwrap(); + let omitted_id = omitted["id"].as_str().unwrap().parse::().unwrap(); + let explicit_false = state + .stores + .runs + .open_run_reader(&explicit_false_id) + .await + .unwrap() + .state() + .await + .unwrap(); + let omitted = state + .stores + .runs + .open_run_reader(&omitted_id) + .await + .unwrap() + .state() + .await + .unwrap(); + + assert_eq!( + explicit_false.spec.settings.run.execution.mode, + fabro_types::settings::run::RunMode::Normal + ); + assert_eq!( + explicit_false.spec.settings.run.execution.approval, + fabro_types::settings::run::ApprovalMode::Prompt + ); + assert!( + !explicit_false + .spec + .settings + .run + .environment + .lifecycle + .preserve + ); + assert_eq!( + omitted.spec.settings.run.execution.mode, + fabro_types::settings::run::RunMode::DryRun + ); + assert_eq!( + omitted.spec.settings.run.execution.approval, + fabro_types::settings::run::ApprovalMode::Auto + ); + assert!(omitted.spec.settings.run.environment.lifecycle.preserve); +} + #[tokio::test] async fn post_runs_run_intent_canonicalizes_and_persists_a_local_folder_target() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/foundation/fabro-api/tests/run_intent_round_trip.rs b/lib/foundation/fabro-api/tests/run_intent_round_trip.rs index 475d12141..2a1b67525 100644 --- a/lib/foundation/fabro-api/tests/run_intent_round_trip.rs +++ b/lib/foundation/fabro-api/tests/run_intent_round_trip.rs @@ -24,13 +24,16 @@ fn run_intent_round_trips_the_openapi_shape() { sha: Some("abcdef0123456789abcdef0123456789abcdef01".to_string()), }), args: RunIntentArgs { - model: Some("gpt-5.6-sol".to_string()), - provider: Some("openai".to_string()), - inputs: HashMap::from([ + model: Some("gpt-5.6-sol".to_string()), + provider: Some("openai".to_string()), + inputs: HashMap::from([ ("attempts".to_string(), json!(3)), ("ship".to_string(), json!(true)), ]), - labels: HashMap::from([("team".to_string(), "platform".to_string())]), + labels: HashMap::from([("team".to_string(), "platform".to_string())]), + dry_run: Some(false), + auto_approve: Some(true), + preserve_sandbox: Some(false), }, environment_id: Some("default".to_string()), parent_id: None, @@ -41,6 +44,9 @@ fn run_intent_round_trips_the_openapi_shape() { let value = serde_json::to_value(&intent).unwrap(); let api: ApiRunIntent = serde_json::from_value(value.clone()).unwrap(); + assert_eq!(value["args"]["dry_run"], false); + assert_eq!(value["args"]["auto_approve"], true); + assert_eq!(value["args"]["preserve_sandbox"], false); assert_eq!(serde_json::to_value(api).unwrap(), value); } diff --git a/lib/foundation/fabro-client/src/client.rs b/lib/foundation/fabro-client/src/client.rs index a2734d451..29441cc13 100644 --- a/lib/foundation/fabro-client/src/client.rs +++ b/lib/foundation/fabro-client/src/client.rs @@ -16,6 +16,7 @@ use fabro_types::{ ArtifactUpload, BlobHash, EventEnvelope, PairId, PairMessageRecord, PairMessageRequest, PairRecord, PairStartRequest, PairTranscriptResponse, Run, RunEvent, RunEventDetailResponse, RunId, RunPairStatusResponse, RunProjection, SessionId, SessionRecord, StageId, + WorkflowVersion, WorkflowVersionId, }; use fabro_util::exit::{ErrorExt, ExitClass}; use futures::future::BoxFuture; @@ -700,6 +701,42 @@ impl Client { self.submit_create_run(manifest.into()).await } + pub async fn create_workflow_version( + &self, + version: &WorkflowVersion, + ) -> Result { + let response = self + .send_api(|client| { + let version = version.clone(); + async move { client.create_workflow_version().body(version).send().await } + }) + .await?; + Ok(response.into_inner().workflow_version_id) + } + + pub async fn register_workflow_versions<'a>( + &self, + versions: impl IntoIterator, + ) -> Result<()> { + for (completed, (expected_id, version)) in versions.into_iter().enumerate() { + let completed_noun = if completed == 1 { "entry" } else { "entries" }; + let returned_id = self + .create_workflow_version(version) + .await + .with_context(|| { + format!( + "failed to register workflow version {expected_id} after {completed} {completed_noun} completed" + ) + })?; + if returned_id != expected_id { + bail!( + "workflow version registration returned {returned_id} for expected {expected_id} after {completed} {completed_noun} completed" + ); + } + } + Ok(()) + } + pub async fn create_run_from_intent(&self, intent: types::RunIntent) -> Result { self.submit_create_run(intent.into()).await } @@ -2281,13 +2318,15 @@ fn add_pr_upgrade_hint(err: anyhow::Error) -> anyhow::Error { #[cfg(test)] mod tests { + use std::collections::BTreeMap; use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use chrono::Duration as ChronoDuration; use fabro_util::exit; use httpmock::Method::{GET, POST}; - use httpmock::MockServer; + use httpmock::{HttpMockResponse, MockServer}; use serde_json::json; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::TcpListener; @@ -2325,6 +2364,265 @@ mod tests { }) } + fn test_workflow_version( + name: &str, + workflow_dependencies: BTreeMap, + ) -> fabro_types::WorkflowVersion { + let entrypoint = fabro_types::WorkflowPath::new("workflow.fabro").unwrap(); + fabro_types::WorkflowVersion::new( + entrypoint.clone(), + BTreeMap::from([(entrypoint, format!("digraph {name} {{}}"))]), + workflow_dependencies, + ) + .unwrap() + } + + fn workflow_version_response( + workflow_version_id: &fabro_types::WorkflowVersionId, + ) -> HttpMockResponse { + HttpMockResponse::builder() + .status(201) + .header("content-type", "application/json") + .body(json!({ "workflow_version_id": workflow_version_id }).to_string()) + .build() + } + + #[tokio::test] + async fn create_workflow_version_posts_exact_version_and_returns_server_id() { + let server = MockServer::start_async().await; + let version = test_workflow_version("ExactVersion", BTreeMap::new()); + let expected_id = version.id().unwrap(); + let mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&version).unwrap()); + then.status(201) + .header("content-type", "application/json") + .json_body(json!({ "workflow_version_id": expected_id })); + }) + .await; + + let client = Client::new_no_proxy(&server.url("")).unwrap(); + let actual_id = client.create_workflow_version(&version).await.unwrap(); + + mock.assert_async().await; + assert_eq!(actual_id, expected_id); + } + + #[tokio::test] + async fn register_workflow_versions_preserves_dependency_first_order() { + let server = MockServer::start_async().await; + let child = test_workflow_version("Child", BTreeMap::new()); + let child_id = child.id().unwrap(); + let parent = test_workflow_version( + "Parent", + BTreeMap::from([(fabro_types::WorkflowPath::new("child").unwrap(), child_id)]), + ); + let parent_id = parent.id().unwrap(); + let child_response_id = child_id; + let child_response_completed = Arc::new(AtomicBool::new(false)); + let child_response_completed_for_child = Arc::clone(&child_response_completed); + let child_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&child).unwrap()); + then.respond_with(move |_| { + let response = workflow_version_response(&child_response_id); + child_response_completed_for_child.store(true, Ordering::SeqCst); + response + }); + }) + .await; + let parent_response_id = parent_id; + let parent_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&parent).unwrap()); + then.respond_with(move |_| { + assert!( + child_response_completed.load(Ordering::SeqCst), + "parent registration began before the child response completed" + ); + workflow_version_response(&parent_response_id) + }); + }) + .await; + + let client = Client::new_no_proxy(&server.url("")).unwrap(); + client + .register_workflow_versions([(child_id, &child), (parent_id, &parent)]) + .await + .unwrap(); + + child_mock.assert_async().await; + parent_mock.assert_async().await; + } + + #[tokio::test] + async fn register_workflow_versions_rejects_returned_id_mismatch() { + let server = MockServer::start_async().await; + let first = test_workflow_version("Expected", BTreeMap::new()); + let expected_id = first.id().unwrap(); + let returned_id = test_workflow_version("Returned", BTreeMap::new()) + .id() + .unwrap(); + let later = test_workflow_version("Later", BTreeMap::new()); + let later_id = later.id().unwrap(); + let first_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&first).unwrap()); + then.status(201) + .header("content-type", "application/json") + .json_body(json!({ "workflow_version_id": returned_id })); + }) + .await; + let later_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&later).unwrap()); + then.status(201) + .header("content-type", "application/json") + .json_body(json!({ "workflow_version_id": later_id })); + }) + .await; + + let client = Client::new_no_proxy(&server.url("")).unwrap(); + let error = client + .register_workflow_versions([(expected_id, &first), (later_id, &later)]) + .await + .unwrap_err(); + let message = error.to_string(); + + first_mock.assert_async().await; + later_mock.assert_calls_async(0).await; + assert!(message.contains(&expected_id.to_string())); + assert!(message.contains(&returned_id.to_string())); + assert!(message.contains("0 entries completed")); + } + + #[tokio::test] + async fn register_workflow_versions_stops_after_request_failure() { + let server = MockServer::start_async().await; + let first = test_workflow_version("First", BTreeMap::new()); + let first_id = first.id().unwrap(); + let failing = test_workflow_version("Failing", BTreeMap::new()); + let failing_id = failing.id().unwrap(); + let later = test_workflow_version("NeverSent", BTreeMap::new()); + let later_id = later.id().unwrap(); + let first_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&first).unwrap()); + then.status(201) + .header("content-type", "application/json") + .json_body(json!({ "workflow_version_id": first_id })); + }) + .await; + let failing_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&failing).unwrap()); + then.status(422) + .header("content-type", "application/json") + .json_body(json!({ + "errors": [{ + "status": "422", + "title": "Unprocessable Entity", + "detail": "workflow dependency was not found", + "code": "workflow_version_dependency_not_found" + }] + })); + }) + .await; + let later_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&later).unwrap()); + then.status(201) + .header("content-type", "application/json") + .json_body(json!({ "workflow_version_id": later_id })); + }) + .await; + + let client = Client::new_no_proxy(&server.url("")).unwrap(); + let error = client + .register_workflow_versions([ + (first_id, &first), + (failing_id, &failing), + (later_id, &later), + ]) + .await + .unwrap_err(); + + first_mock.assert_async().await; + failing_mock.assert_async().await; + later_mock.assert_calls_async(0).await; + assert!(error.to_string().contains(&failing_id.to_string())); + assert!(error.to_string().contains("1 entry completed")); + let failure = api_failure_for(&error).expect("API failure metadata should survive context"); + assert_eq!(failure.status, fabro_http::StatusCode::UNPROCESSABLE_ENTITY); + assert_eq!( + failure.code.as_deref(), + Some("workflow_version_dependency_not_found") + ); + } + + #[tokio::test] + async fn register_workflow_versions_accepts_empty_input_without_requests() { + let server = MockServer::start_async().await; + let unexpected = server + .mock_async(|when, then| { + when.method(POST).path("/api/v1/workflow-versions"); + then.status(500); + }) + .await; + let client = Client::new_no_proxy(&server.url("")).unwrap(); + + client + .register_workflow_versions(std::iter::empty::<( + fabro_types::WorkflowVersionId, + &fabro_types::WorkflowVersion, + )>()) + .await + .unwrap(); + + unexpected.assert_calls_async(0).await; + } + + #[tokio::test] + async fn create_workflow_version_accepts_repeated_canonical_content() { + let server = MockServer::start_async().await; + let version = test_workflow_version("Repeated", BTreeMap::new()); + let expected_id = version.id().unwrap(); + let mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(serde_json::to_value(&version).unwrap()); + then.status(201) + .header("content-type", "application/json") + .json_body(json!({ "workflow_version_id": expected_id })); + }) + .await; + let client = Client::new_no_proxy(&server.url("")).unwrap(); + + let first = client.create_workflow_version(&version).await.unwrap(); + let second = client.create_workflow_version(&version).await.unwrap(); + + mock.assert_calls_async(2).await; + assert_eq!(first, expected_id); + assert_eq!(second, expected_id); + } + #[cfg(unix)] #[tokio::test] async fn refresh_access_token_allows_plain_http_targets() { diff --git a/lib/foundation/fabro-types/src/run_intent.rs b/lib/foundation/fabro-types/src/run_intent.rs index 1deb67931..3aaa545e3 100644 --- a/lib/foundation/fabro-types/src/run_intent.rs +++ b/lib/foundation/fabro-types/src/run_intent.rs @@ -27,13 +27,19 @@ pub struct RunIntent { #[serde(deny_unknown_fields)] pub struct RunIntentArgs { #[serde(default, skip_serializing_if = "Option::is_none")] - pub model: Option, + pub model: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub provider: Option, + pub provider: Option, #[serde(default, skip_serializing_if = "HashMap::is_empty")] - pub inputs: HashMap, + pub inputs: HashMap, #[serde(default, skip_serializing_if = "HashMap::is_empty")] - pub labels: HashMap, + pub labels: HashMap, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub dry_run: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub auto_approve: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub preserve_sandbox: Option, } /// Requested workspace content, independent of sandbox placement. diff --git a/lib/foundation/fabro-types/tests/run_intent.rs b/lib/foundation/fabro-types/tests/run_intent.rs index 57b2e3c0d..5d99b612e 100644 --- a/lib/foundation/fabro-types/tests/run_intent.rs +++ b/lib/foundation/fabro-types/tests/run_intent.rs @@ -21,13 +21,16 @@ fn intent() -> RunIntent { sha: Some("ABCDEF0123456789ABCDEF0123456789ABCDEF01".to_string()), }), args: RunIntentArgs { - model: Some("gpt-5.6".to_string()), - provider: Some("openai".to_string()), - inputs: HashMap::from([ + model: Some("gpt-5.6".to_string()), + provider: Some("openai".to_string()), + inputs: HashMap::from([ ("attempts".to_string(), json!(3)), ("enabled".to_string(), json!(true)), ]), - labels: HashMap::from([("team".to_string(), "platform".to_string())]), + labels: HashMap::from([("team".to_string(), "platform".to_string())]), + dry_run: Some(false), + auto_approve: Some(true), + preserve_sandbox: None, }, environment_id: Some("production".to_string()), parent_id: None, @@ -36,6 +39,32 @@ fn intent() -> RunIntent { } } +#[test] +fn run_intent_args_preserve_tri_state_wire_semantics() { + let omitted = serde_json::to_value(RunIntentArgs::default()).expect("args should serialize"); + assert_eq!(omitted, json!({})); + + let args = RunIntentArgs { + dry_run: Some(false), + auto_approve: Some(true), + preserve_sandbox: Some(false), + ..RunIntentArgs::default() + }; + let value = serde_json::to_value(&args).expect("args should serialize"); + + assert_eq!(value["dry_run"], false); + assert_eq!(value["auto_approve"], true); + assert_eq!(value["preserve_sandbox"], false); + assert_eq!( + serde_json::from_value::(value).expect("args should deserialize"), + args + ); + assert!( + serde_json::from_value::(json!({ "unexpected": true })).is_err(), + "unknown args fields must remain rejected" + ); +} + #[test] fn run_intent_round_trips_the_strict_git_shape() { let intent = intent(); diff --git a/lib/packages/fabro-api-client/src/models/run-intent-args.ts b/lib/packages/fabro-api-client/src/models/run-intent-args.ts index 012ef85a6..2531c7425 100644 --- a/lib/packages/fabro-api-client/src/models/run-intent-args.ts +++ b/lib/packages/fabro-api-client/src/models/run-intent-args.ts @@ -28,4 +28,16 @@ export interface RunIntentArgs { 'provider'?: string; 'inputs'?: { [key: string]: RunIntentArgsInputsValue; }; 'labels'?: { [key: string]: string; }; + /** + * Overrides `run.execution.mode`: true selects `dry_run`, false selects `normal`, and omission inherits the lower-precedence setting. + */ + 'dry_run'?: boolean; + /** + * Overrides `run.execution.approval`: true selects `auto`, false selects `prompt`, and omission inherits the lower-precedence setting. + */ + 'auto_approve'?: boolean; + /** + * Overrides `run.environment.lifecycle.preserve`; omission inherits the lower-precedence setting. + */ + 'preserve_sandbox'?: boolean; } From 637d6a0d8482986e13e5f76ab08d840f30d1163b Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 31 Aug 2026 10:20:58 -0400 Subject: [PATCH 2/3] Simplify workflow version registration client and tests Move the content-derived id check into create_workflow_version so every caller gets it, drop the redundant expected_id parameter from register_workflow_versions, and collapse the duplicated httpmock setups and ordering machinery in the client tests. Use in-scope imports and the neighbouring reader idiom in the server intent tests. Co-Authored-By: Claude Fable 5 --- lib/apps/fabro-server/src/server/tests.rs | 34 +-- lib/foundation/fabro-client/src/client.rs | 291 +++++++++------------- 2 files changed, 124 insertions(+), 201 deletions(-) diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index c5958e307..8a44b9239 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -24,7 +24,7 @@ use fabro_llm::types::{Message as LlmMessage, Request as LlmRequest, TokenCounts use fabro_model::catalog::LlmCatalogSettings; use fabro_model::{Catalog, ModelRef, ProviderId, ReasoningEffort, Speed}; use fabro_types::settings::ServerAuthMethod; -use fabro_types::settings::run::EnvironmentProvider; +use fabro_types::settings::run::{ApprovalMode, EnvironmentProvider}; use fabro_types::{ AgentBackend, AttrValue, AuthMethod, BlobHash, CommandTermination, FailureCategory, FailureDetail, GitRunTarget, Graph, InterviewQuestionRecord, Node, Outcome, ParallelBranchId, @@ -3807,13 +3807,10 @@ async fn post_runs_run_intent_args_true_override_resolved_settings_without_start assert_eq!(body["lifecycle"]["status"]["kind"], "submitted"); let run_store = state.stores.runs.open_run_reader(&run_id).await.unwrap(); let projection = run_store.state().await.unwrap(); - assert_eq!( - projection.spec.settings.run.execution.mode, - fabro_types::settings::run::RunMode::DryRun - ); + assert_eq!(projection.spec.settings.run.execution.mode, RunMode::DryRun); assert_eq!( projection.spec.settings.run.execution.approval, - fabro_types::settings::run::ApprovalMode::Auto + ApprovalMode::Auto ); assert!(projection.spec.settings.run.environment.lifecycle.preserve); } @@ -3875,32 +3872,28 @@ preserve = true .parse::() .unwrap(); let omitted_id = omitted["id"].as_str().unwrap().parse::().unwrap(); - let explicit_false = state + let explicit_false_store = state .stores .runs .open_run_reader(&explicit_false_id) .await - .unwrap() - .state() - .await .unwrap(); - let omitted = state + let explicit_false = explicit_false_store.state().await.unwrap(); + let omitted_store = state .stores .runs .open_run_reader(&omitted_id) .await - .unwrap() - .state() - .await .unwrap(); + let omitted = omitted_store.state().await.unwrap(); assert_eq!( explicit_false.spec.settings.run.execution.mode, - fabro_types::settings::run::RunMode::Normal + RunMode::Normal ); assert_eq!( explicit_false.spec.settings.run.execution.approval, - fabro_types::settings::run::ApprovalMode::Prompt + ApprovalMode::Prompt ); assert!( !explicit_false @@ -3911,13 +3904,10 @@ preserve = true .lifecycle .preserve ); - assert_eq!( - omitted.spec.settings.run.execution.mode, - fabro_types::settings::run::RunMode::DryRun - ); + assert_eq!(omitted.spec.settings.run.execution.mode, RunMode::DryRun); assert_eq!( omitted.spec.settings.run.execution.approval, - fabro_types::settings::run::ApprovalMode::Auto + ApprovalMode::Auto ); assert!(omitted.spec.settings.run.environment.lifecycle.preserve); } @@ -16556,7 +16546,7 @@ level = "debug" "goal should be persisted from the manifest" ); assert!( - resolved_run.execution.mode == fabro_types::settings::run::RunMode::DryRun, + resolved_run.execution.mode == RunMode::DryRun, "run execution mode should inherit from server settings" ); assert_eq!( diff --git a/lib/foundation/fabro-client/src/client.rs b/lib/foundation/fabro-client/src/client.rs index 29441cc13..ff8762eda 100644 --- a/lib/foundation/fabro-client/src/client.rs +++ b/lib/foundation/fabro-client/src/client.rs @@ -701,38 +701,39 @@ impl Client { self.submit_create_run(manifest.into()).await } + /// Registers one workflow version and verifies the server assigned the + /// content-derived id, so a mismatched response fails loudly here rather + /// than being trusted downstream. pub async fn create_workflow_version( &self, version: &WorkflowVersion, ) -> Result { + let expected_id = version.id()?; let response = self .send_api(|client| { let version = version.clone(); async move { client.create_workflow_version().body(version).send().await } }) .await?; - Ok(response.into_inner().workflow_version_id) + let returned_id = response.into_inner().workflow_version_id; + if returned_id != expected_id { + bail!( + "workflow version registration returned {returned_id} for expected {expected_id}" + ); + } + Ok(returned_id) } + /// Registers versions in iteration order, stopping at the first failure. + /// Callers must order dependencies before the versions that reference them. pub async fn register_workflow_versions<'a>( &self, - versions: impl IntoIterator, + versions: impl IntoIterator, ) -> Result<()> { - for (completed, (expected_id, version)) in versions.into_iter().enumerate() { - let completed_noun = if completed == 1 { "entry" } else { "entries" }; - let returned_id = self - .create_workflow_version(version) + for (index, version) in versions.into_iter().enumerate() { + self.create_workflow_version(version) .await - .with_context(|| { - format!( - "failed to register workflow version {expected_id} after {completed} {completed_noun} completed" - ) - })?; - if returned_id != expected_id { - bail!( - "workflow version registration returned {returned_id} for expected {expected_id} after {completed} {completed_noun} completed" - ); - } + .with_context(|| format!("failed to register workflow version at index {index}"))?; } Ok(()) } @@ -2320,13 +2321,13 @@ fn add_pr_upgrade_hint(err: anyhow::Error) -> anyhow::Error { mod tests { use std::collections::BTreeMap; use std::sync::Arc; - use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use chrono::Duration as ChronoDuration; + use fabro_types::WorkflowPath; use fabro_util::exit; use httpmock::Method::{GET, POST}; - use httpmock::{HttpMockResponse, MockServer}; + use httpmock::MockServer; use serde_json::json; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::TcpListener; @@ -2366,10 +2367,10 @@ mod tests { fn test_workflow_version( name: &str, - workflow_dependencies: BTreeMap, - ) -> fabro_types::WorkflowVersion { - let entrypoint = fabro_types::WorkflowPath::new("workflow.fabro").unwrap(); - fabro_types::WorkflowVersion::new( + workflow_dependencies: BTreeMap, + ) -> WorkflowVersion { + let entrypoint = WorkflowPath::new("workflow.fabro").unwrap(); + WorkflowVersion::new( entrypoint.clone(), BTreeMap::from([(entrypoint, format!("digraph {name} {{}}"))]), workflow_dependencies, @@ -2377,14 +2378,27 @@ mod tests { .unwrap() } - fn workflow_version_response( - workflow_version_id: &fabro_types::WorkflowVersionId, - ) -> HttpMockResponse { - HttpMockResponse::builder() - .status(201) + /// Mocks `POST /api/v1/workflow-versions` for exactly this version body. + async fn mock_create_workflow_version<'a>( + server: &'a MockServer, + version: &WorkflowVersion, + then: impl FnOnce(httpmock::Then) -> httpmock::Then, + ) -> httpmock::Mock<'a> { + let body = serde_json::to_value(version).unwrap(); + server + .mock_async(|when, respond| { + when.method(POST) + .path("/api/v1/workflow-versions") + .json_body(body); + then(respond); + }) + .await + } + + fn created_workflow_version(then: httpmock::Then, id: WorkflowVersionId) -> httpmock::Then { + then.status(201) .header("content-type", "application/json") - .body(json!({ "workflow_version_id": workflow_version_id }).to_string()) - .build() + .json_body(json!({ "workflow_version_id": id })) } #[tokio::test] @@ -2392,16 +2406,10 @@ mod tests { let server = MockServer::start_async().await; let version = test_workflow_version("ExactVersion", BTreeMap::new()); let expected_id = version.id().unwrap(); - let mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&version).unwrap()); - then.status(201) - .header("content-type", "application/json") - .json_body(json!({ "workflow_version_id": expected_id })); - }) - .await; + let mock = mock_create_workflow_version(&server, &version, |then| { + created_workflow_version(then, expected_id) + }) + .await; let client = Client::new_no_proxy(&server.url("")).unwrap(); let actual_id = client.create_workflow_version(&version).await.unwrap(); @@ -2411,49 +2419,52 @@ mod tests { } #[tokio::test] - async fn register_workflow_versions_preserves_dependency_first_order() { + async fn create_workflow_version_rejects_returned_id_mismatch() { + let server = MockServer::start_async().await; + let version = test_workflow_version("Expected", BTreeMap::new()); + let expected_id = version.id().unwrap(); + let returned_id = test_workflow_version("Returned", BTreeMap::new()) + .id() + .unwrap(); + let mock = mock_create_workflow_version(&server, &version, |then| { + created_workflow_version(then, returned_id) + }) + .await; + + let client = Client::new_no_proxy(&server.url("")).unwrap(); + let message = client + .create_workflow_version(&version) + .await + .unwrap_err() + .to_string(); + + mock.assert_async().await; + assert!(message.contains(&expected_id.to_string())); + assert!(message.contains(&returned_id.to_string())); + } + + #[tokio::test] + async fn register_workflow_versions_registers_every_entry_in_order() { let server = MockServer::start_async().await; let child = test_workflow_version("Child", BTreeMap::new()); let child_id = child.id().unwrap(); let parent = test_workflow_version( "Parent", - BTreeMap::from([(fabro_types::WorkflowPath::new("child").unwrap(), child_id)]), + BTreeMap::from([(WorkflowPath::new("child").unwrap(), child_id)]), ); let parent_id = parent.id().unwrap(); - let child_response_id = child_id; - let child_response_completed = Arc::new(AtomicBool::new(false)); - let child_response_completed_for_child = Arc::clone(&child_response_completed); - let child_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&child).unwrap()); - then.respond_with(move |_| { - let response = workflow_version_response(&child_response_id); - child_response_completed_for_child.store(true, Ordering::SeqCst); - response - }); - }) - .await; - let parent_response_id = parent_id; - let parent_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&parent).unwrap()); - then.respond_with(move |_| { - assert!( - child_response_completed.load(Ordering::SeqCst), - "parent registration began before the child response completed" - ); - workflow_version_response(&parent_response_id) - }); - }) - .await; + let child_mock = mock_create_workflow_version(&server, &child, |then| { + created_workflow_version(then, child_id) + }) + .await; + let parent_mock = mock_create_workflow_version(&server, &parent, |then| { + created_workflow_version(then, parent_id) + }) + .await; let client = Client::new_no_proxy(&server.url("")).unwrap(); client - .register_workflow_versions([(child_id, &child), (parent_id, &parent)]) + .register_workflow_versions([&child, &parent]) .await .unwrap(); @@ -2461,113 +2472,46 @@ mod tests { parent_mock.assert_async().await; } - #[tokio::test] - async fn register_workflow_versions_rejects_returned_id_mismatch() { - let server = MockServer::start_async().await; - let first = test_workflow_version("Expected", BTreeMap::new()); - let expected_id = first.id().unwrap(); - let returned_id = test_workflow_version("Returned", BTreeMap::new()) - .id() - .unwrap(); - let later = test_workflow_version("Later", BTreeMap::new()); - let later_id = later.id().unwrap(); - let first_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&first).unwrap()); - then.status(201) - .header("content-type", "application/json") - .json_body(json!({ "workflow_version_id": returned_id })); - }) - .await; - let later_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&later).unwrap()); - then.status(201) - .header("content-type", "application/json") - .json_body(json!({ "workflow_version_id": later_id })); - }) - .await; - - let client = Client::new_no_proxy(&server.url("")).unwrap(); - let error = client - .register_workflow_versions([(expected_id, &first), (later_id, &later)]) - .await - .unwrap_err(); - let message = error.to_string(); - - first_mock.assert_async().await; - later_mock.assert_calls_async(0).await; - assert!(message.contains(&expected_id.to_string())); - assert!(message.contains(&returned_id.to_string())); - assert!(message.contains("0 entries completed")); - } - #[tokio::test] async fn register_workflow_versions_stops_after_request_failure() { let server = MockServer::start_async().await; let first = test_workflow_version("First", BTreeMap::new()); let first_id = first.id().unwrap(); let failing = test_workflow_version("Failing", BTreeMap::new()); - let failing_id = failing.id().unwrap(); let later = test_workflow_version("NeverSent", BTreeMap::new()); let later_id = later.id().unwrap(); - let first_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&first).unwrap()); - then.status(201) - .header("content-type", "application/json") - .json_body(json!({ "workflow_version_id": first_id })); - }) - .await; - let failing_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&failing).unwrap()); - then.status(422) - .header("content-type", "application/json") - .json_body(json!({ - "errors": [{ - "status": "422", - "title": "Unprocessable Entity", - "detail": "workflow dependency was not found", - "code": "workflow_version_dependency_not_found" - }] - })); - }) - .await; - let later_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&later).unwrap()); - then.status(201) - .header("content-type", "application/json") - .json_body(json!({ "workflow_version_id": later_id })); - }) - .await; + let first_mock = mock_create_workflow_version(&server, &first, |then| { + created_workflow_version(then, first_id) + }) + .await; + let failing_mock = mock_create_workflow_version(&server, &failing, |then| { + then.status(422) + .header("content-type", "application/json") + .json_body(json!({ + "errors": [{ + "status": "422", + "title": "Unprocessable Entity", + "detail": "workflow dependency was not found", + "code": "workflow_version_dependency_not_found" + }] + })) + }) + .await; + let later_mock = mock_create_workflow_version(&server, &later, |then| { + created_workflow_version(then, later_id) + }) + .await; let client = Client::new_no_proxy(&server.url("")).unwrap(); let error = client - .register_workflow_versions([ - (first_id, &first), - (failing_id, &failing), - (later_id, &later), - ]) + .register_workflow_versions([&first, &failing, &later]) .await .unwrap_err(); first_mock.assert_async().await; failing_mock.assert_async().await; later_mock.assert_calls_async(0).await; - assert!(error.to_string().contains(&failing_id.to_string())); - assert!(error.to_string().contains("1 entry completed")); + assert!(error.to_string().contains("index 1")); let failure = api_failure_for(&error).expect("API failure metadata should survive context"); assert_eq!(failure.status, fabro_http::StatusCode::UNPROCESSABLE_ENTITY); assert_eq!( @@ -2587,13 +2531,8 @@ mod tests { .await; let client = Client::new_no_proxy(&server.url("")).unwrap(); - client - .register_workflow_versions(std::iter::empty::<( - fabro_types::WorkflowVersionId, - &fabro_types::WorkflowVersion, - )>()) - .await - .unwrap(); + let none: [&WorkflowVersion; 0] = []; + client.register_workflow_versions(none).await.unwrap(); unexpected.assert_calls_async(0).await; } @@ -2603,16 +2542,10 @@ mod tests { let server = MockServer::start_async().await; let version = test_workflow_version("Repeated", BTreeMap::new()); let expected_id = version.id().unwrap(); - let mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/api/v1/workflow-versions") - .json_body(serde_json::to_value(&version).unwrap()); - then.status(201) - .header("content-type", "application/json") - .json_body(json!({ "workflow_version_id": expected_id })); - }) - .await; + let mock = mock_create_workflow_version(&server, &version, |then| { + created_workflow_version(then, expected_id) + }) + .await; let client = Client::new_no_proxy(&server.url("")).unwrap(); let first = client.create_workflow_version(&version).await.unwrap(); From 7514e676e08c7925f2102fb30082ac6de089d5b5 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 31 Aug 2026 13:24:25 -0400 Subject: [PATCH 3/3] Prove workflow version registration order --- lib/foundation/fabro-client/src/client.rs | 33 ++++++++++++++++++----- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/lib/foundation/fabro-client/src/client.rs b/lib/foundation/fabro-client/src/client.rs index ff8762eda..93fd868f4 100644 --- a/lib/foundation/fabro-client/src/client.rs +++ b/lib/foundation/fabro-client/src/client.rs @@ -2321,13 +2321,14 @@ fn add_pr_upgrade_hint(err: anyhow::Error) -> anyhow::Error { mod tests { use std::collections::BTreeMap; use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use chrono::Duration as ChronoDuration; use fabro_types::WorkflowPath; use fabro_util::exit; use httpmock::Method::{GET, POST}; - use httpmock::MockServer; + use httpmock::{HttpMockResponse, MockServer}; use serde_json::json; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::TcpListener; @@ -2401,6 +2402,14 @@ mod tests { .json_body(json!({ "workflow_version_id": id })) } + fn workflow_version_response(id: WorkflowVersionId) -> HttpMockResponse { + HttpMockResponse::builder() + .status(201) + .header("content-type", "application/json") + .body(json!({ "workflow_version_id": id }).to_string()) + .build() + } + #[tokio::test] async fn create_workflow_version_posts_exact_version_and_returns_server_id() { let server = MockServer::start_async().await; @@ -2444,7 +2453,7 @@ mod tests { } #[tokio::test] - async fn register_workflow_versions_registers_every_entry_in_order() { + async fn register_workflow_versions_preserves_dependency_first_order() { let server = MockServer::start_async().await; let child = test_workflow_version("Child", BTreeMap::new()); let child_id = child.id().unwrap(); @@ -2453,12 +2462,24 @@ mod tests { BTreeMap::from([(WorkflowPath::new("child").unwrap(), child_id)]), ); let parent_id = parent.id().unwrap(); - let child_mock = mock_create_workflow_version(&server, &child, |then| { - created_workflow_version(then, child_id) + let child_response_completed = Arc::new(AtomicBool::new(false)); + let child_response_completed_for_child = Arc::clone(&child_response_completed); + let child_mock = mock_create_workflow_version(&server, &child, move |then| { + then.respond_with(move |_| { + let response = workflow_version_response(child_id); + child_response_completed_for_child.store(true, Ordering::SeqCst); + response + }) }) .await; - let parent_mock = mock_create_workflow_version(&server, &parent, |then| { - created_workflow_version(then, parent_id) + let parent_mock = mock_create_workflow_version(&server, &parent, move |then| { + then.respond_with(move |_| { + assert!( + child_response_completed.load(Ordering::SeqCst), + "parent registration began before the child response completed" + ); + workflow_version_response(parent_id) + }) }) .await;