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

Simplify the empty workspace run target plumbing

Browse source 
- Use a derived deserializer for RunTarget by making `None` an empty struct
  variant, which keeps `deny_unknown_fields` strict without a hand-rolled impl
- Make clone_source_for_run the single owner of the empty-workspace decision
  and drop the duplicated target checks in RunSession::new
- Collapse duplicated target/provider compatibility matches in admission and
  start into single matches, using a strum-derived kind name for messages
- Drop the redundant git override in persist_create_run
- Extract a shared helper for the duplicated unavailable-integration test loop

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-08-24 17:21:54 -04:00
parent 2f3b6477f2
commit 3e0adde73d
8 changed files with 136 additions and 240 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

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

@ -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) =

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

@ -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<AppState>) {
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]

23
lib/components/fabro-workflow/src/operations/create.rs
View file

@ -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);
}

6
lib/components/fabro-workflow/src/operations/retry.rs
View file

@ -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);
}

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

@ -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<String>,
branch: Option<String>,
commit_sha: Option<String>,
}
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<CloneSourceForRun, Error> {
@ -608,17 +598,10 @@ fn clone_source_for_run(record: &RunSpec) -> Result<CloneSourceForRun, Error> {
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<CloneSourceForRun, Error> {
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]

2
lib/foundation/fabro-api/tests/run_intent_round_trip.rs
View file

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

81
lib/foundation/fabro-types/src/run_intent.rs
View file

@ -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<String>,
},
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<String>,
}
#[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<D>(deserializer: D) -> Result<Self, D::Error>
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,
}),
}

6
lib/foundation/fabro-types/tests/run_intent.rs
View file

@ -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);
}