From 9dc39ce9fafa555a64e900fc126f80a447b5218a Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 25 Aug 2026 14:53:31 -0400 Subject: [PATCH] Simplify folder target admission and startup checks Collapse the triple Folder dispatch in run-intent admission into a single prepare_intent_target call that canonicalizes and observes Git under one provider gate, and stop feeding target/git into the compiler input only to overwrite them afterwards. In run start, hoist the duplicated Folder rejection out of the Docker and Daytona arms, restore kind_name() for the Git/None arm, and drop the unreachable absolute/symlink checks that follow canonicalize. Dedupe the folder-target test fixtures in both crates. Co-Authored-By: Claude Fable 5 --- lib/apps/fabro-server/src/run_intent.rs | 56 ++++----- .../fabro-server/src/server/handler/runs.rs | 14 +-- lib/apps/fabro-server/src/server/tests.rs | 114 ++++++------------ .../fabro-workflow/src/operations/start.rs | 99 ++++++--------- 4 files changed, 99 insertions(+), 184 deletions(-) diff --git a/lib/apps/fabro-server/src/run_intent.rs b/lib/apps/fabro-server/src/run_intent.rs index f9714038d..f255bfee4 100644 --- a/lib/apps/fabro-server/src/run_intent.rs +++ b/lib/apps/fabro-server/src/run_intent.rs @@ -64,8 +64,9 @@ pub(crate) struct PreparedIntentTarget { /// Materialize filesystem-backed target facts after the effective environment /// has been admitted as Local and before run allocation. Folder targets are -/// canonicalized once for durable identity. Optional Git observation follows -/// this step under the same provider gate. +/// canonicalized once for durable identity and their optional Git metadata is +/// observed under the same provider gate, so rejected requests never scan host +/// repositories. Other targets pass through with their validated projection. pub(crate) async fn prepare_intent_target( target: RunTarget, git: Option, @@ -86,46 +87,31 @@ pub(crate) async fn prepare_intent_target( if !metadata.is_dir() { return Err(FolderTargetValidationError::NotDirectory); } - let canonical_text = canonical_folder_text(&canonical)?; - - Ok(PreparedIntentTarget { - target: RunTarget::Folder { - path: canonical_text, - }, - git, - }) -} - -/// Observe optional Git metadata only after provider policy has admitted the -/// folder target. This keeps rejected requests from scanning host repositories. -pub(crate) async fn observe_folder_git_context(target: &RunTarget) -> Option { - let RunTarget::Folder { path } = target else { - return None; - }; - let canonical = PathBuf::from(path); - let observed_path = canonical.clone(); - let observed = task::spawn_blocking(move || git::observe_git_context(&observed_path)).await; - let git = match observed { - Ok(Ok(git)) => git, - Ok(Err(error)) => { + let path = canonical_folder_text(&canonical)?; + let git = task::spawn_blocking(move || { + git::observe_git_context(&canonical).unwrap_or_else(|error| { tracing::warn!( error = %error, path = %canonical.display(), "Failed to observe optional Git metadata for folder target" ); None - } - Err(error) => { - tracing::warn!( - error = %error, - path = %canonical.display(), - "Folder target Git observation task failed" - ); - None - } - }; + }) + }) + .await + .unwrap_or_else(|error| { + tracing::warn!( + error = %error, + path, + "Folder target Git observation task failed" + ); + None + }); - git + Ok(PreparedIntentTarget { + target: RunTarget::Folder { path }, + git, + }) } fn canonical_folder_text(path: &Path) -> Result { diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 76f8baca4..22a4df4b8 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -61,8 +61,7 @@ use crate::run_compiler::{ use crate::run_files::{list_run_commits, list_run_files}; use crate::run_intent::{ EnvironmentSelectionError, PreparedIntentTarget, RunIntentAdmissionError, - lower_workflow_closure, observe_folder_git_context, pin_workflow_environment_authority, - prepare_intent_target, + lower_workflow_closure, pin_workflow_environment_authority, prepare_intent_target, }; use crate::run_manifest; use crate::run_selector::{ResolveRunError, resolve_run_by_selector}; @@ -716,11 +715,13 @@ async fn create_run_from_intent( run_id: None, title, parent_id: intent.parent_id, - git: git.clone(), + // Target identity and its Git projection are attached after provider + // admission via `with_target_and_git`; the compiler never reads them. + git: None, storage_root: state.server_storage_dir(), workflow_slug: None, workflow_version_id: Some(intent.workflow_version_id), - target: Some(target.clone()), + target: None, provenance: run_provenance(&headers, &actor), web_url: None, submitted_manifest_bytes: None, @@ -749,13 +750,10 @@ async fn create_run_from_intent( if let Err(error) = validate_intent_environment(&state, prepared.settings(), &target).await { return run_intent_admission_error(error.into()); } - let PreparedIntentTarget { target, mut git } = match prepare_intent_target(target, git).await { + let PreparedIntentTarget { target, git } = match prepare_intent_target(target, git).await { Ok(prepared) => prepared, Err(error) => return run_intent_admission_error(error.into()), }; - if matches!(target, RunTarget::Folder { .. }) { - git = observe_folder_git_context(&target).await; - } prepared = prepared.with_target_and_git(target, git); let (prepared, run_id) = prepared.resolve_run_id(); if let Err(response) = validate_optional_parent(&state, run_id, prepared.parent_id()).await { diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index c7c9c583f..75e63a5cf 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -3529,35 +3529,37 @@ async fn generated_title_does_not_overwrite_user_title_edit() { } async fn post_run_manifest(app: &Router, manifest: serde_json::Value) -> serde_json::Value { - let response = app - .clone() - .oneshot( - Request::builder() - .method("POST") - .uri(api("/runs")) - .header("content-type", "application/json") - .body(Body::from(manifest.to_string())) - .unwrap(), - ) - .await - .unwrap(); + let response = post_run_intent_response(app, manifest).await; response_json!(response, StatusCode::CREATED).await } async fn post_run_intent_response(app: &Router, intent: serde_json::Value) -> Response { app.clone() - .oneshot( - Request::builder() - .method("POST") - .uri(api("/runs")) - .header("content-type", "application/json") - .body(Body::from(intent.to_string())) - .unwrap(), - ) + .oneshot(json_request(Method::POST, "/runs", &intent)) .await .unwrap() } +/// App state whose default environment runs in place on the server, which is +/// the only placement folder targets admit. +fn local_test_app_state() -> Arc { + TestAppStateBuilder::new() + .default_environment_provider(Some(EnvironmentProvider::Local)) + .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) + .build() +} + +fn folder_intent( + workflow_version_id: fabro_types::WorkflowVersionId, + path: impl serde::Serialize, +) -> serde_json::Value { + json!({ + "workflow_version_id": workflow_version_id, + "target": { "kind": "folder", "path": path }, + "args": {} + }) +} + async fn store_workflow_version( state: &AppState, graph: &str, @@ -3789,10 +3791,7 @@ async fn post_runs_run_intent_canonicalizes_and_persists_a_local_folder_target() .unwrap() .to_string_lossy() .to_string(); - let state = TestAppStateBuilder::new() - .default_environment_provider(Some(EnvironmentProvider::Local)) - .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) - .build(); + let state = local_test_app_state(); let app = crate::test_support::build_test_router(Arc::clone(&state)); let workflow_version_id = store_workflow_version( &state, @@ -3803,14 +3802,7 @@ async fn post_runs_run_intent_canonicalizes_and_persists_a_local_folder_target() let body = post_run_manifest( &app, - json!({ - "workflow_version_id": workflow_version_id, - "target": { - "kind": "folder", - "path": submitted.to_string_lossy() - }, - "args": {} - }), + folder_intent(workflow_version_id, submitted.to_string_lossy()), ) .await; let run_id = body["id"].as_str().unwrap().parse::().unwrap(); @@ -3872,22 +3864,11 @@ async fn post_runs_run_intent_observes_folder_git_metadata_without_a_remote_call .unwrap() .to_string_lossy() .to_string(); - let state = TestAppStateBuilder::new() - .default_environment_provider(Some(EnvironmentProvider::Local)) - .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) - .build(); + let state = local_test_app_state(); 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": canonical }, - "args": {} - }), - ) - .await; + let body = post_run_manifest(&app, folder_intent(workflow_version_id, canonical)).await; let run_id = body["id"].as_str().unwrap().parse::().unwrap(); let projection = state .stores @@ -3911,10 +3892,7 @@ async fn post_runs_run_intent_rejects_invalid_folder_paths_before_persistence() let dir = tempfile::tempdir().unwrap(); let file = dir.path().join("file"); std::fs::write(&file, "not a directory").unwrap(); - let state = TestAppStateBuilder::new() - .default_environment_provider(Some(EnvironmentProvider::Local)) - .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) - .build(); + let state = local_test_app_state(); let app = crate::test_support::build_test_router(Arc::clone(&state)); let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; let invalid_paths = [ @@ -3925,15 +3903,8 @@ async fn post_runs_run_intent_rejects_invalid_folder_paths_before_persistence() ]; for path in invalid_paths { - let response = post_run_intent_response( - &app, - json!({ - "workflow_version_id": workflow_version_id, - "target": { "kind": "folder", "path": path }, - "args": {} - }), - ) - .await; + let response = + post_run_intent_response(&app, folder_intent(workflow_version_id, path)).await; let body = response_json!(response, StatusCode::UNPROCESSABLE_ENTITY).await; assert_eq!(body["errors"][0]["code"], "target_invalid"); } @@ -3967,15 +3938,8 @@ async fn post_runs_run_intent_applies_the_folder_target_environment_matrix() { ] { let app = crate::test_support::build_test_router(Arc::clone(&state)); let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; - let response = post_run_intent_response( - &app, - json!({ - "workflow_version_id": workflow_version_id, - "target": { "kind": "folder", "path": target }, - "args": {} - }), - ) - .await; + let response = + post_run_intent_response(&app, folder_intent(workflow_version_id, &target)).await; let body = response_json!(response, StatusCode::UNPROCESSABLE_ENTITY).await; assert_eq!(body["errors"][0]["code"], "target_environment_unsupported"); assert!( @@ -4009,15 +3973,8 @@ enabled = false .build(); let app = crate::test_support::build_test_router(Arc::clone(&disabled_state)); let workflow_version_id = store_workflow_version(&disabled_state, MINIMAL_DOT, None).await; - let response = post_run_intent_response( - &app, - json!({ - "workflow_version_id": workflow_version_id, - "target": { "kind": "folder", "path": target }, - "args": {} - }), - ) - .await; + let response = + post_run_intent_response(&app, folder_intent(workflow_version_id, &target)).await; let body = response_json!(response, StatusCode::SERVICE_UNAVAILABLE).await; assert_eq!(body["errors"][0]["code"], "integration_unavailable"); assert!( @@ -4237,10 +4194,7 @@ async fn post_runs_run_intent_maps_missing_version_environment_and_target_errors #[tokio::test] async fn post_runs_run_intent_rejects_none_target_with_local_environment_before_persistence() { - let state = TestAppStateBuilder::new() - .default_environment_provider(Some(EnvironmentProvider::Local)) - .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) - .build(); + let state = local_test_app_state(); let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; let app = crate::test_support::build_test_router(Arc::clone(&state)); let response = app diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index d355cc4c7..600cbee1f 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -461,17 +461,20 @@ impl RunSession { }) .collect::, _>>()?; + if sandbox_provider != SandboxProviderKind::Local + && matches!(record.target, Some(RunTarget::Folder { .. })) + { + return Err(Error::engine( + "persisted folder run targets require the Local sandbox provider", + )); + } 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", - )); + Some(target @ (RunTarget::Git { .. } | RunTarget::None {})) => { + return Err(Error::engine(format!( + "persisted {} run targets require a clone-based sandbox provider", + target.kind_name() + ))); } Some(RunTarget::Folder { path }) => SandboxSpec::Local { working_directory: folder_working_directory_from_record(record, path).await?, @@ -491,11 +494,6 @@ impl RunSession { } }, SandboxProviderKind::Docker => { - if matches!(record.target.as_ref(), Some(RunTarget::Folder { .. })) { - return Err(Error::engine( - "persisted folder run targets require the Local sandbox provider", - )); - } let mut config = resolve_docker_config(resolved, secret_lookup)?; config.skip_clone |= clone_source.skip_clone; SandboxSpec::Docker { @@ -508,11 +506,6 @@ impl RunSession { } } SandboxProviderKind::Daytona => { - if matches!(record.target.as_ref(), Some(RunTarget::Folder { .. })) { - return Err(Error::engine( - "persisted folder run targets require the Local sandbox provider", - )); - } let api_key = vault_guard .get(EnvVars::DAYTONA_API_KEY) .map(str::to_string); @@ -616,13 +609,6 @@ async fn folder_working_directory_from_record( record: &RunSpec, target_path: &str, ) -> Result { - let target = Path::new(target_path); - if !target.is_absolute() { - return Err(Error::engine( - "persisted folder run target path must be absolute", - )); - } - let source_directory = record.source_directory.as_deref().ok_or_else(|| { Error::engine("persisted folder run target is missing its source-directory projection") })?; @@ -632,32 +618,26 @@ async fn folder_working_directory_from_record( )); } - let canonical = fs::canonicalize(target).await.map_err(|source| { + // The persisted path was canonical at admission, so it is absolute and + // symlink-free. Re-canonicalizing detects any redirection since then. + let canonical = fs::canonicalize(target_path).await.map_err(|source| { Error::engine_with_source( "persisted folder run target path could not be canonicalized", source, ) })?; - let canonical_text = canonical.to_str().ok_or_else(|| { - Error::engine("persisted folder run target canonical path is not valid UTF-8") - })?; - if canonical_text != target_path { + if canonical.to_str() != Some(target_path) { return Err(Error::engine( "persisted folder run target path is no longer canonical", )); } - let metadata = fs::symlink_metadata(&canonical).await.map_err(|source| { + let metadata = fs::metadata(&canonical).await.map_err(|source| { Error::engine_with_source( "persisted folder run target path could not be inspected", source, ) })?; - if metadata.file_type().is_symlink() { - return Err(Error::engine( - "persisted folder run target path was redirected during startup", - )); - } if !metadata.is_dir() { return Err(Error::engine( "persisted folder run target path is not a directory", @@ -2031,12 +2011,9 @@ reasoning = false async fn run_session_new_folder_target_uses_canonical_path_over_environment_cwd() { let temp = tempfile::tempdir().unwrap(); let (storage_root, _run_dir) = storage_root_and_run_dir(&temp); - let folder = temp.path().join("folder-target"); + let (canonical_folder, canonical_text) = canonical_folder(&temp); let environment_cwd = temp.path().join("environment-cwd"); - std::fs::create_dir_all(&folder).unwrap(); std::fs::create_dir_all(&environment_cwd).unwrap(); - let canonical_folder = folder.canonicalize().unwrap(); - let canonical_text = canonical_folder.to_str().unwrap().to_string(); let mut settings = settings_from_run_layer(RunLayer::default()); settings.run.environment.provider = EnvironmentProvider::Local; settings.run.environment.cwd = Some(environment_cwd.to_string_lossy().into_owned()); @@ -2071,10 +2048,7 @@ reasoning = false for provider in [EnvironmentProvider::Docker, EnvironmentProvider::Daytona] { let temp = tempfile::tempdir().unwrap(); let (storage_root, _run_dir) = storage_root_and_run_dir(&temp); - let folder = temp.path().join("folder-target"); - std::fs::create_dir_all(&folder).unwrap(); - let canonical_folder = folder.canonicalize().unwrap(); - let canonical_text = canonical_folder.to_str().unwrap().to_string(); + let (_, canonical_text) = canonical_folder(&temp); let mut settings = settings_from_run_layer(RunLayer::default()); settings.run.environment.provider = provider; settings.run.environment.image.docker = match provider { @@ -2140,10 +2114,7 @@ reasoning = false #[tokio::test] async fn folder_target_start_rejects_projection_drift() { let temp = tempfile::tempdir().unwrap(); - let folder = temp.path().join("folder-target"); - std::fs::create_dir_all(&folder).unwrap(); - let canonical_folder = folder.canonicalize().unwrap(); - let canonical_text = canonical_folder.to_str().unwrap().to_string(); + let (_, canonical_text) = canonical_folder(&temp); let mut record = test_folder_run_spec(&canonical_text); record.source_directory = None; @@ -2166,12 +2137,14 @@ reasoning = false let relative_error = folder_working_directory_from_record(&relative_record, relative) .await .expect_err("relative persisted target should fail"); - assert!(relative_error.to_string().contains("must be absolute")); + assert!( + relative_error + .to_string() + .contains("persisted folder run target path") + ); let temp = tempfile::tempdir().unwrap(); - let folder = temp.path().join("folder-target"); - std::fs::create_dir_all(&folder).unwrap(); - let canonical_folder = folder.canonicalize().unwrap(); + let (canonical_folder, _) = canonical_folder(&temp); let noncanonical = canonical_folder .join("..") .join(canonical_folder.file_name().unwrap()); @@ -2187,10 +2160,7 @@ reasoning = false #[tokio::test] async fn folder_target_start_rejects_disappeared_or_retyped_path() { let temp = tempfile::tempdir().unwrap(); - let folder = temp.path().join("folder-target"); - std::fs::create_dir_all(&folder).unwrap(); - let canonical_folder = folder.canonicalize().unwrap(); - let canonical_text = canonical_folder.to_str().unwrap().to_string(); + let (canonical_folder, canonical_text) = canonical_folder(&temp); let record = test_folder_run_spec(&canonical_text); std::fs::remove_dir(&canonical_folder).unwrap(); @@ -2219,11 +2189,8 @@ reasoning = false use std::os::unix::fs::symlink; let temp = tempfile::tempdir().unwrap(); - let folder = temp.path().join("folder-target"); + let (canonical_folder, canonical_text) = canonical_folder(&temp); let redirected = temp.path().join("redirected-target"); - std::fs::create_dir_all(&folder).unwrap(); - let canonical_folder = folder.canonicalize().unwrap(); - let canonical_text = canonical_folder.to_str().unwrap().to_string(); let record = test_folder_run_spec(&canonical_text); std::fs::rename(&canonical_folder, &redirected).unwrap(); symlink(&redirected, &canonical_folder).unwrap(); @@ -2364,6 +2331,16 @@ reasoning = false Persisted::new(graph, source, diagnostics, run_dir, run_spec) } + /// Create `folder-target` under `temp` and return its canonical path and + /// the UTF-8 text a persisted folder target would carry. + fn canonical_folder(temp: &tempfile::TempDir) -> (PathBuf, String) { + let folder = temp.path().join("folder-target"); + std::fs::create_dir_all(&folder).unwrap(); + let canonical_folder = folder.canonicalize().unwrap(); + let canonical_text = canonical_folder.to_str().unwrap().to_string(); + (canonical_folder, canonical_text) + } + fn test_folder_run_spec(path: &str) -> RunSpec { let mut record = test_support::test_run_spec(); record.target = Some(RunTarget::Folder {