diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 957335e1f..fce929ef1 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -30,7 +30,8 @@ use fabro_types::{ AutomationRef, ManifestPath, Principal, Run, RunClientProvenance, RunId, RunProvenance, RunServerProvenance, RunStatusKind, RunTarget, SandboxProviderKind, StageContextWindow, StageContextWindowStaleness, StageContextWindowUnavailableReason, StageHandler, - StageModelUsage, StageProjection, SystemActorKind, json_scalar_to_toml_value, parse_blob_ref, + StageModelUsage, StageProjection, SystemActorKind, ValidatedRunTarget, + json_scalar_to_toml_value, parse_blob_ref, }; use fabro_util::error as error_util; use fabro_util::version::FABRO_VERSION; @@ -610,12 +611,10 @@ async fn create_run_from_intent( ) -> Response { // Validate the pure, in-memory request facts before paying for // blob-store reads and closure lowering. - let validated_target = match intent.target.validate() { + let ValidatedRunTarget { target, git } = match intent.target.validate() { Ok(validated) => validated, Err(error) => return run_intent_admission_error(error.into()), }; - let target = validated_target.target; - let git = validated_target.git; let environment_id = match select_intent_environment_id( &state, intent @@ -1081,19 +1080,17 @@ async fn validate_intent_environment( SandboxProviderKind::Docker => image.docker.is_none() && image.dockerfile.is_some(), SandboxProviderKind::Daytona => image.docker.is_some(), }; - let target_incompatible = match target { - RunTarget::Git { .. } => { - provider == SandboxProviderKind::Local || !settings.run.clone.enabled - } - RunTarget::None => provider == SandboxProviderKind::Local, + let (target_incompatible, detail) = match target { + RunTarget::Git { .. } => ( + provider == SandboxProviderKind::Local || !settings.run.clone.enabled, + "Git targets require a compatible clone-enabled Docker or Daytona environment", + ), + RunTarget::None {} => ( + provider == SandboxProviderKind::Local, + "none targets require a compatible Docker or Daytona environment", + ), }; if image_incompatible || target_incompatible { - let detail = match target { - RunTarget::Git { .. } => { - "Git targets require a compatible clone-enabled Docker or Daytona environment" - } - RunTarget::None => "none targets require a compatible Docker or Daytona environment", - }; return Err(EnvironmentSelectionError::TargetUnsupported { detail }); } if let Some(detail) = diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 83f28dcaa..e9d387a81 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -3744,7 +3744,10 @@ async fn post_runs_run_intent_creates_submitted_none_target_without_git_projecti vec!["run.created", "run.submitted"] ); let projection = run_store.state().await.unwrap(); - assert_eq!(projection.spec.target, Some(fabro_types::RunTarget::None)); + assert_eq!( + projection.spec.target, + Some(fabro_types::RunTarget::None {}) + ); assert_eq!( projection.spec.workflow_version_id, Some(workflow_version_id) @@ -3790,7 +3793,10 @@ async fn post_runs_run_intent_accepts_none_target_with_ready_daytona_environment .state() .await .unwrap(); - assert_eq!(projection.spec.target, Some(fabro_types::RunTarget::None)); + assert_eq!( + projection.spec.target, + Some(fabro_types::RunTarget::None {}) + ); assert_eq!( projection.spec.settings.run.environment.provider, EnvironmentProvider::Daytona @@ -3998,6 +4004,51 @@ async fn post_runs_run_intent_rejects_none_target_with_local_environment_before_ ); } +/// Posts a Git and a `none` run intent against `state` and asserts both are +/// rejected as `integration_unavailable` without persisting anything. +async fn assert_run_intent_targets_unavailable(state: &Arc) { + let version_id = store_workflow_version(state, MINIMAL_DOT, None).await; + let app = crate::test_support::build_test_router(Arc::clone(state)); + for target in [ + json!({ + "kind": "git", + "repo": "fabro-sh/fabro", + "branch": "feature/run-intent" + }), + json!({ "kind": "none" }), + ] { + let intent = json!({ + "workflow_version_id": version_id, + "target": target, + "args": {} + }); + let response = app + .clone() + .oneshot( + Request::builder() + .method("POST") + .uri(api("/runs")) + .header("content-type", "application/json") + .body(Body::from(intent.to_string())) + .unwrap(), + ) + .await + .unwrap(); + let body = response_json!(response, StatusCode::SERVICE_UNAVAILABLE).await; + assert_eq!(body["errors"][0]["code"], "integration_unavailable"); + } + assert!(state.runs.lock().expect("runs lock poisoned").is_empty()); + assert!( + state + .stores + .run_summaries + .list_identities() + .await + .unwrap() + .is_empty() + ); +} + #[tokio::test] async fn post_runs_run_intent_rejects_disabled_or_unready_sandbox_integrations() { let disabled_state = test_app_state_with_options( @@ -4015,103 +4066,13 @@ enabled = false RunLayer::default(), 5, ); - let disabled_version_id = store_workflow_version(&disabled_state, MINIMAL_DOT, None).await; - let disabled_app = crate::test_support::build_test_router(Arc::clone(&disabled_state)); - for target in [ - json!({ - "kind": "git", - "repo": "fabro-sh/fabro", - "branch": "feature/run-intent" - }), - json!({ "kind": "none" }), - ] { - let intent = json!({ - "workflow_version_id": disabled_version_id, - "target": target, - "args": {} - }); - let response = disabled_app - .clone() - .oneshot( - Request::builder() - .method("POST") - .uri(api("/runs")) - .header("content-type", "application/json") - .body(Body::from(intent.to_string())) - .unwrap(), - ) - .await - .unwrap(); - let body = response_json!(response, StatusCode::SERVICE_UNAVAILABLE).await; - assert_eq!(body["errors"][0]["code"], "integration_unavailable"); - } - assert!( - disabled_state - .runs - .lock() - .expect("runs lock poisoned") - .is_empty() - ); - assert!( - disabled_state - .stores - .run_summaries - .list_identities() - .await - .unwrap() - .is_empty() - ); + assert_run_intent_targets_unavailable(&disabled_state).await; let daytona_state = TestAppStateBuilder::new() .default_environment_provider(Some(EnvironmentProvider::Daytona)) .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) .build(); - let daytona_version_id = store_workflow_version(&daytona_state, MINIMAL_DOT, None).await; - let daytona_app = crate::test_support::build_test_router(Arc::clone(&daytona_state)); - for target in [ - json!({ - "kind": "git", - "repo": "fabro-sh/fabro", - "branch": "feature/run-intent" - }), - json!({ "kind": "none" }), - ] { - let intent = json!({ - "workflow_version_id": daytona_version_id, - "target": target, - "args": {} - }); - let response = daytona_app - .clone() - .oneshot( - Request::builder() - .method("POST") - .uri(api("/runs")) - .header("content-type", "application/json") - .body(Body::from(intent.to_string())) - .unwrap(), - ) - .await - .unwrap(); - let body = response_json!(response, StatusCode::SERVICE_UNAVAILABLE).await; - assert_eq!(body["errors"][0]["code"], "integration_unavailable"); - } - assert!( - daytona_state - .runs - .lock() - .expect("runs lock poisoned") - .is_empty() - ); - assert!( - daytona_state - .stores - .run_summaries - .list_identities() - .await - .unwrap() - .is_empty() - ); + assert_run_intent_targets_unavailable(&daytona_state).await; } #[tokio::test] diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 3382260e9..7d9673ef4 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -475,11 +475,10 @@ pub async fn persist_create_run( source_directory, labels, } = materialized; - let (source_directory, git) = if matches!(target.as_ref(), Some(RunTarget::None)) { - (None, None) - } else { - (Some(source_directory), git) - }; + // An empty-workspace target has no submitter cwd to project; `git` is + // already `None` for it because admission derives it from the target. + let source_directory = + (!matches!(target.as_ref(), Some(RunTarget::None {}))).then_some(source_directory); let persisted_run_dir = run_dir.clone(); let persisted = spawn_blocking(move || { let run_spec = RunSpec { @@ -2294,17 +2293,12 @@ reasoning = false workflow_slug: None, workflow_path: None, workflow_bundle: None, - target: Some(RunTarget::None), + target: Some(RunTarget::None {}), submitted_manifest_bytes: None, run_id: Some(fixtures::RUN_2), title: None, automation: None, - git: Some(fabro_types::GitContext { - origin_url: "https://github.com/fabro-sh/fabro".to_string(), - branch: "main".to_string(), - sha: None, - dirty: fabro_types::DirtyStatus::Clean, - }), + git: None, fork_source_ref: None, parent_id: None, provenance: test_support::test_run_provenance(), @@ -2318,7 +2312,10 @@ reasoning = false .unwrap(); assert_eq!(created.persisted.run_spec().source_directory, None); - assert_eq!(created.persisted.run_spec().target, Some(RunTarget::None)); + assert_eq!( + created.persisted.run_spec().target, + Some(RunTarget::None {}) + ); assert_eq!(created.persisted.run_spec().git, None); } diff --git a/lib/components/fabro-workflow/src/operations/retry.rs b/lib/components/fabro-workflow/src/operations/retry.rs index 67765ca3d..c492274cf 100644 --- a/lib/components/fabro-workflow/src/operations/retry.rs +++ b/lib/components/fabro-workflow/src/operations/retry.rs @@ -467,7 +467,7 @@ mod tests { source_directory: None, workflow_slug: Some("none-target-retry".to_string()), workflow_version_id: Some(test_support::test_workflow_version_id()), - target: Some(RunTarget::None), + target: Some(RunTarget::None {}), automation: None, provenance: provenance("source-user"), manifest_blob: None, @@ -491,7 +491,7 @@ mod tests { assert_eq!(source_state.status, RunStatus::Failed { reason: FailureReason::WorkflowError, }); - assert_eq!(source_state.spec.target, Some(RunTarget::None)); + assert_eq!(source_state.spec.target, Some(RunTarget::None {})); assert_eq!(source_state.spec.git, None); assert_eq!(source_state.spec.source_directory, None); @@ -510,7 +510,7 @@ mod tests { assert_eq!(retry_events.len(), 2); assert_eq!(retry_state.status, RunStatus::Submitted); assert_eq!(retry_state.retried_from, Some(source_run_id)); - assert_eq!(retry_state.spec.target, Some(RunTarget::None)); + assert_eq!(retry_state.spec.target, Some(RunTarget::None {})); assert_eq!(retry_state.spec.git, None); assert_eq!(retry_state.spec.source_directory, None); } diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 8892b0d1b..f9e38e76f 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -22,8 +22,7 @@ use fabro_types::settings::run::{ RunPrepareSettings as ResolvedRunPrepareSettings, }; use fabro_types::{ - ManifestPath, RunId, RunRunnableSource, RunSpec, RunTarget, SandboxProviderKind, - TargetValidationError, + ManifestPath, RunId, RunRunnableSource, RunSpec, SandboxProviderKind, TargetValidationError, }; use fabro_util::error::collect_chain; use fabro_vault::Vault; @@ -419,12 +418,11 @@ impl RunSession { let sandbox_provider = resolve_sandbox_provider(resolved).effective_for(resolved.execution.mode); let clone_source = clone_source_for_run(record)?; - let target_requires_empty_workspace = target_requires_empty_workspace(record); - let runtime_origin_url = if target_requires_empty_workspace { - None - } else { - record.repo_origin_url().map(str::to_string) - }; + // An empty-workspace run has no repository for PR creation or the + // sandbox environment, regardless of any persisted Git metadata. + let runtime_origin_url = (!clone_source.skip_clone) + .then(|| record.repo_origin_url().map(str::to_string)) + .flatten(); let catalog = Arc::clone(&services.catalog); let configured = configured_providers_for_start(&services.vault, Arc::clone(&catalog)).await; @@ -464,18 +462,11 @@ impl RunSession { let sandbox = match sandbox_provider { SandboxProviderKind::Local => { - match record.target.as_ref() { - Some(RunTarget::Git { .. }) => { - return Err(Error::engine( - "persisted Git run targets require a clone-based sandbox provider", - )); - } - Some(RunTarget::None) => { - return Err(Error::engine( - "persisted none run targets require a clone-based sandbox provider", - )); - } - None => {} + if let Some(target) = &record.target { + return Err(Error::engine(format!( + "persisted {} run targets require a clone-based sandbox provider", + target.kind_name() + ))); } let working_directory = local_working_directory_from_environment( &resolved.environment, @@ -491,7 +482,7 @@ impl RunSession { } SandboxProviderKind::Docker => { let mut config = resolve_docker_config(resolved, secret_lookup)?; - config.skip_clone |= target_requires_empty_workspace; + config.skip_clone |= clone_source.skip_clone; SandboxSpec::Docker { config, github_app: services.github_app.clone(), @@ -506,7 +497,7 @@ impl RunSession { .get(EnvVars::DAYTONA_API_KEY) .map(str::to_string); let mut config = resolve_daytona_config(resolved); - config.skip_clone |= target_requires_empty_workspace; + config.skip_clone |= clone_source.skip_clone; SandboxSpec::Daytona { config: Box::new(config), github_app: services.github_app.clone(), @@ -596,10 +587,9 @@ struct CloneSourceForRun { origin_url: Option, branch: Option, commit_sha: Option, -} - -fn target_requires_empty_workspace(record: &RunSpec) -> bool { - matches!(record.target.as_ref(), Some(RunTarget::None)) + /// The target asked for an empty workspace, so the provider must not + /// clone even when it would otherwise inherit an origin. + skip_clone: bool, } fn clone_source_for_run(record: &RunSpec) -> Result { @@ -608,17 +598,10 @@ fn clone_source_for_run(record: &RunSpec) -> Result { origin_url: record.repo_origin_url().map(str::to_string), branch: record.base_branch().map(str::to_string), commit_sha: None, + skip_clone: false, }); }; - if matches!(target, RunTarget::None) { - return Ok(CloneSourceForRun { - origin_url: None, - branch: None, - commit_sha: None, - }); - } - // The Git-target grammar is owned by `RunTarget::validate` in fabro-types; // admission accepts targets through the same rules, and this start path // re-derives the clone source from the persisted target alone. The @@ -633,13 +616,20 @@ fn clone_source_for_run(record: &RunSpec) -> Result { TargetValidationError::Sha => "persisted Git run target has an invalid SHA", }) })?; - let git = validated.git.ok_or_else(|| { - Error::engine("persisted run target has no supported clone-source projection") - })?; - Ok(CloneSourceForRun { - origin_url: Some(git.origin_url), - branch: Some(git.branch), - commit_sha: git.sha, + // A target with no Git projection (`none`) asks for an empty workspace. + Ok(match validated.git { + Some(git) => CloneSourceForRun { + origin_url: Some(git.origin_url), + branch: Some(git.branch), + commit_sha: git.sha, + skip_clone: false, + }, + None => CloneSourceForRun { + origin_url: None, + branch: None, + commit_sha: None, + skip_clone: true, + }, }) } @@ -1820,7 +1810,7 @@ reasoning = false MINIMAL_DOT, &storage_root, settings, - Some(RunTarget::None), + Some(RunTarget::None {}), ) .await; let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); @@ -1882,7 +1872,7 @@ reasoning = false MINIMAL_DOT, &storage_root, settings, - Some(RunTarget::None), + Some(RunTarget::None {}), ) .await; let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); @@ -1942,7 +1932,7 @@ reasoning = false MINIMAL_DOT, &storage_root, settings, - Some(RunTarget::None), + Some(RunTarget::None {}), ) .await; let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); @@ -2883,7 +2873,7 @@ reasoning = false #[test] fn none_target_forces_an_empty_clone_source_and_workspace() { let mut spec = test_support::test_run_spec(); - spec.target = Some(RunTarget::None); + spec.target = Some(RunTarget::None {}); spec.git = Some(fabro_types::GitContext { origin_url: "https://github.com/fabro-sh/fabro".to_string(), branch: "main".to_string(), @@ -2896,7 +2886,7 @@ reasoning = false assert_eq!(source.origin_url, None); assert_eq!(source.branch, None); assert_eq!(source.commit_sha, None); - assert!(target_requires_empty_workspace(&spec)); + assert!(source.skip_clone); } #[test] 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 ed9616c97..c065c7114 100644 --- a/lib/foundation/fabro-api/tests/run_intent_round_trip.rs +++ b/lib/foundation/fabro-api/tests/run_intent_round_trip.rs @@ -46,7 +46,7 @@ fn run_intent_round_trips_the_openapi_shape() { fn run_intent_none_target_round_trips_the_openapi_shape() { let intent = RunIntent { workflow_version_id: test_support::test_workflow_version_id(), - target: RunTarget::None, + target: RunTarget::None {}, args: RunIntentArgs::default(), environment_id: Some("default".to_string()), parent_id: None, diff --git a/lib/foundation/fabro-types/src/run_intent.rs b/lib/foundation/fabro-types/src/run_intent.rs index 99385620a..42327cb88 100644 --- a/lib/foundation/fabro-types/src/run_intent.rs +++ b/lib/foundation/fabro-types/src/run_intent.rs @@ -1,7 +1,6 @@ use std::collections::HashMap; -use serde::de::Error as _; -use serde::{Deserialize, Deserializer, Serialize}; +use serde::{Deserialize, Serialize}; use serde_json::Value; use crate::{DirtyStatus, GitContext, GitHubRepositorySlug, RunId, WorkflowVersionId, repository}; @@ -38,8 +37,13 @@ pub struct RunIntentArgs { } /// Requested workspace content, independent of sandbox placement. -#[derive(Debug, Clone, PartialEq, Eq, Serialize)] -#[serde(tag = "kind", rename_all = "snake_case")] +/// +/// `None` is an empty struct variant rather than a unit variant so that the +/// derived deserializer enforces `deny_unknown_fields` on `{"kind": "none"}` +/// (serde ignores sibling fields on internally tagged unit variants). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, strum::IntoStaticStr)] +#[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] +#[strum(serialize_all = "snake_case")] pub enum RunTarget { Git { repo: String, @@ -47,68 +51,15 @@ pub enum RunTarget { #[serde(default, skip_serializing_if = "Option::is_none")] sha: Option, }, - None, -} - -// Serde's derived internally tagged unit variants accept sibling fields even -// with `deny_unknown_fields`, so deserialize through strict arm-specific maps. -#[derive(Deserialize)] -#[serde(rename_all = "snake_case")] -enum RunTargetKindWire { - Git, - None, -} - -#[derive(Deserialize)] -#[serde(deny_unknown_fields)] -struct GitRunTargetWire { - kind: RunTargetKindWire, - repo: String, - branch: String, - #[serde(default)] - sha: Option, -} - -#[derive(Deserialize)] -#[serde(deny_unknown_fields)] -struct NoneRunTargetWire { - kind: RunTargetKindWire, -} - -#[derive(Deserialize)] -#[serde(untagged)] -enum RunTargetWire { - Git(GitRunTargetWire), - None(NoneRunTargetWire), -} - -impl<'de> Deserialize<'de> for RunTarget { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - match RunTargetWire::deserialize(deserializer)? { - RunTargetWire::Git(wire) => match wire.kind { - RunTargetKindWire::Git => Ok(Self::Git { - repo: wire.repo, - branch: wire.branch, - sha: wire.sha, - }), - RunTargetKindWire::None => { - Err(D::Error::custom("none target must not contain Git fields")) - } - }, - RunTargetWire::None(wire) => match wire.kind { - RunTargetKindWire::None => Ok(Self::None), - RunTargetKindWire::Git => Err(D::Error::custom( - "git target requires repository and branch fields", - )), - }, - } - } + None {}, } impl RunTarget { + /// The wire `kind` discriminator (`git`, `none`), for diagnostics. + pub fn kind_name(&self) -> &'static str { + self.into() + } + /// Validates and canonicalizes the target without any network resolution. /// /// Git targets include their derived operational Git projection. Targets @@ -146,8 +97,8 @@ impl RunTarget { git: Some(git), }) } - Self::None => Ok(ValidatedRunTarget { - target: Self::None, + Self::None {} => Ok(ValidatedRunTarget { + target: Self::None {}, git: None, }), } diff --git a/lib/foundation/fabro-types/tests/run_intent.rs b/lib/foundation/fabro-types/tests/run_intent.rs index 15a7bf95a..b2b3f75fa 100644 --- a/lib/foundation/fabro-types/tests/run_intent.rs +++ b/lib/foundation/fabro-types/tests/run_intent.rs @@ -52,7 +52,7 @@ fn run_intent_round_trips_the_strict_git_shape() { #[test] fn run_intent_round_trips_the_strict_none_shape() { let mut intent = intent(); - intent.target = RunTarget::None; + intent.target = RunTarget::None {}; let value = serde_json::to_value(&intent).expect("intent should serialize"); @@ -147,8 +147,8 @@ fn target_validation_normalizes_sha_without_network_resolution() { #[test] fn run_intent_none_target_validates_without_a_git_projection() { - let validated = RunTarget::None.validate().unwrap(); - assert_eq!(validated.target, RunTarget::None); + let validated = RunTarget::None {}.validate().unwrap(); + assert_eq!(validated.target, RunTarget::None {}); assert_eq!(validated.git, None); }