scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-08 03:10:26 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

Simplify folder target admission and startup checks

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-08-25 14:53:31 -04:00
parent b5d517e4b5
commit 9dc39ce9fa
4 changed files with 99 additions and 184 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

56
lib/apps/fabro-server/src/run_intent.rs
View file

@ -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<GitContext>,
@ -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<GitContext> {
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<String, FolderTargetValidationError> {

14
lib/apps/fabro-server/src/server/handler/runs.rs
View file

@ -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 {

114
lib/apps/fabro-server/src/server/tests.rs
View file

@ -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<AppState> {
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::<RunId>().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::<RunId>().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

99
lib/components/fabro-workflow/src/operations/start.rs
View file

@ -461,17 +461,20 @@ impl RunSession {
})
.collect::<Result<Vec<_>, _>>()?;
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<PathBuf, Error> {
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 {