From 18d98794aee90f4af1f87cb70029beb564ca7743 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sat, 22 Aug 2026 10:27:30 -0400 Subject: [PATCH] Move Git-target validation onto RunTarget in fabro-types The Git-target grammar (slug, branch, and SHA rules plus the derived origin URL) was implemented twice with no shared code path: once in server admission and again in sandbox start, so the two could drift and disagree about which persisted targets are valid. Own it once as RunTarget::validate() in fabro-types, next to the primitives it uses, returning the canonical target together with its derived GitContext projection. Admission consumes it directly, and the start path re-derives the expected clone source from the same rules before checking the persisted projection against it. The start path now also moves the derived strings into the sandbox spec instead of cloning them. While reordering admission around the shared validator, run the pure, in-memory checks (target grammar, environment id) before the blob-store closure fetch and lowering so malformed requests no longer pay for version-store I/O. Co-Authored-By: Claude Fable 5 --- lib/apps/fabro-server/src/run_intent.rs | 77 +------------- .../fabro-server/src/server/handler/runs.rs | 32 +++--- .../fabro-workflow/src/operations/start.rs | 100 ++++++++---------- lib/foundation/fabro-types/src/lib.rs | 4 +- lib/foundation/fabro-types/src/run_intent.rs | 63 ++++++++++- .../fabro-types/tests/run_intent.rs | 62 +++++++++++ 6 files changed, 191 insertions(+), 147 deletions(-) diff --git a/lib/apps/fabro-server/src/run_intent.rs b/lib/apps/fabro-server/src/run_intent.rs index 0c22ccd6b..bed25992a 100644 --- a/lib/apps/fabro-server/src/run_intent.rs +++ b/lib/apps/fabro-server/src/run_intent.rs @@ -6,8 +6,7 @@ use fabro_config::{RunGoalLayer, SettingsLayer}; use fabro_environment::{EnvironmentId, EnvironmentValidationError}; use fabro_types::settings::InterpString; use fabro_types::{ - DirtyStatus, GitContext, GitHubRepositorySlug, ManifestPath, RunTarget, SandboxProviderKind, - WorkflowPath, WorkflowVersionId, normalize_git_commit_sha, repository, + ManifestPath, SandboxProviderKind, TargetValidationError, WorkflowPath, WorkflowVersionId, }; use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle}; use fabro_workflow_version::LoadedWorkflowVersionClosure; @@ -103,61 +102,6 @@ pub(crate) enum WorkflowClosureLoweringError { }, } -#[derive(Debug, Error, PartialEq, Eq)] -pub(crate) enum TargetValidationError { - #[error("target repository must be a valid GitHub owner/name slug")] - Repository, - #[error("target branch must be a non-empty branch name, not a ref or commit selector")] - Branch, - #[error("target SHA must be exactly 40 ASCII hexadecimal characters")] - Sha, -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct ValidatedGitTarget { - pub(crate) target: RunTarget, - pub(crate) git: GitContext, -} - -pub(crate) fn validate_target( - target: RunTarget, -) -> Result { - match target { - RunTarget::Git { repo, branch, sha } => { - let slug = - GitHubRepositorySlug::try_new(&repo).ok_or(TargetValidationError::Repository)?; - let selector = format!("heads/{branch}"); - if branch.is_empty() - || branch.starts_with("heads/") - || branch.starts_with("tags/") - || branch.starts_with("refs/") - || normalize_git_commit_sha(&branch).is_some() - || !repository::is_valid_github_ref_selector(&selector) - { - return Err(TargetValidationError::Branch); - } - let sha = sha - .map(|sha| normalize_git_commit_sha(&sha).ok_or(TargetValidationError::Sha)) - .transpose()?; - let repo = format!("{}/{}", slug.owner(), slug.repo()); - let origin_url = format!("https://github.com/{repo}"); - Ok(ValidatedGitTarget { - target: RunTarget::Git { - repo, - branch: branch.clone(), - sha: sha.clone(), - }, - git: GitContext { - origin_url, - branch, - sha, - dirty: DirtyStatus::Clean, - }, - }) - } - } -} - pub(crate) fn lower_workflow_closure( closure: &LoadedWorkflowVersionClosure, ) -> Result { @@ -622,23 +566,4 @@ mod tests { WorkflowClosureLoweringError::InvalidMount { .. } )); } - - #[test] - fn target_validation_normalizes_sha_without_network_resolution() { - let validated = validate_target(RunTarget::Git { - repo: "fabro-sh/fabro".to_string(), - branch: "feature/run-intent".to_string(), - sha: Some("ABCDEF0123456789ABCDEF0123456789ABCDEF01".to_string()), - }) - .unwrap(); - - assert_eq!( - validated.git.sha.as_deref(), - Some("abcdef0123456789abcdef0123456789abcdef01") - ); - assert_eq!( - validated.git.origin_url, - "https://github.com/fabro-sh/fabro" - ); - } } diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 2c0ccc9c5..4b2638bf4 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -60,7 +60,7 @@ use crate::run_compiler::{ use crate::run_files::{list_run_commits, list_run_files}; use crate::run_intent::{ EnvironmentSelectionError, RunIntentAdmissionError, lower_workflow_closure, - pin_workflow_environment_authority, validate_target, + pin_workflow_environment_authority, }; use crate::run_manifest; use crate::run_selector::{ResolveRunError, resolve_run_by_selector}; @@ -587,6 +587,22 @@ async fn create_run_from_intent( actor: Principal, headers: HeaderMap, ) -> Response { + // Validate the pure, in-memory request facts before paying for + // blob-store reads and closure lowering. + let validated_target = match intent.target.validate() { + Ok(target) => target, + Err(error) => return run_intent_admission_error(error.into()), + }; + let environment_id = match select_intent_environment_id( + &state, + intent + .environment_id + .as_deref() + .unwrap_or(DEFAULT_ENVIRONMENT_ID), + ) { + Ok(id) => id, + Err(error) => return run_intent_admission_error(error.into()), + }; let blobs = match state.store_ref().blobs().await { Ok(blobs) => blobs, Err(source) => { @@ -611,20 +627,6 @@ async fn create_run_from_intent( Ok(lowered) => lowered, Err(error) => return run_intent_admission_error(error.into()), }; - let validated_target = match validate_target(intent.target) { - Ok(target) => target, - Err(error) => return run_intent_admission_error(error.into()), - }; - let environment_id = match select_intent_environment_id( - &state, - intent - .environment_id - .as_deref() - .unwrap_or(DEFAULT_ENVIRONMENT_ID), - ) { - Ok(id) => id, - Err(error) => return run_intent_admission_error(error.into()), - }; if let Some(layer) = lowered.workflow_layer.as_mut() { pin_workflow_environment_authority(layer, environment_id.as_str()); } diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 668132211..19cd41587 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -22,8 +22,8 @@ use fabro_types::settings::run::{ RunPrepareSettings as ResolvedRunPrepareSettings, }; use fabro_types::{ - GitHubRepositorySlug, ManifestPath, RunId, RunRunnableSource, RunSpec, RunTarget, - SandboxProviderKind, normalize_git_commit_sha, repository, + ManifestPath, RunId, RunRunnableSource, RunSpec, SandboxProviderKind, TargetValidationError, + normalize_git_commit_sha, }; use fabro_util::error::collect_chain; use fabro_vault::Vault; @@ -479,9 +479,9 @@ impl RunSession { config: resolve_docker_config(resolved, secret_lookup)?, github_app: services.github_app.clone(), run_id: Some(record.run_id), - clone_origin_url: clone_source.origin_url.clone(), - clone_branch: clone_source.branch.clone(), - clone_commit_sha: clone_source.commit_sha.clone(), + clone_origin_url: clone_source.origin_url, + clone_branch: clone_source.branch, + clone_commit_sha: clone_source.commit_sha, }, SandboxProviderKind::Daytona => { let api_key = vault_guard @@ -491,9 +491,9 @@ impl RunSession { config: Box::new(resolve_daytona_config(resolved)), github_app: services.github_app.clone(), run_id: Some(record.run_id), - clone_origin_url: clone_source.origin_url.clone(), - clone_branch: clone_source.branch.clone(), - clone_commit_sha: clone_source.commit_sha.clone(), + clone_origin_url: clone_source.origin_url, + clone_branch: clone_source.branch, + clone_commit_sha: clone_source.commit_sha, api_key, } } @@ -587,53 +587,44 @@ fn clone_source_for_run(record: &RunSpec) -> Result { }); }; - match target { - RunTarget::Git { repo, branch, sha } => { - let slug = GitHubRepositorySlug::try_new(repo).ok_or_else(|| { - Error::engine("persisted Git run target has an invalid repository slug") - })?; - let selector = format!("heads/{branch}"); - if branch.starts_with("heads/") - || branch.starts_with("tags/") - || branch.starts_with("refs/") - || normalize_git_commit_sha(branch).is_some() - || !repository::is_valid_github_ref_selector(&selector) - { - return Err(Error::engine( - "persisted Git run target has an invalid branch", - )); + // The Git-target grammar is owned by `RunTarget::validate` in fabro-types; + // admission accepts targets through the same rules this start path + // re-derives the clone source from. + let validated = target.clone().validate().map_err(|error| { + Error::engine(match error { + TargetValidationError::Repository => { + "persisted Git run target has an invalid repository slug" } - let sha = sha - .as_deref() - .map(|value| { - normalize_git_commit_sha(value) - .ok_or_else(|| Error::engine("persisted Git run target has an invalid SHA")) - }) - .transpose()?; - let expected_origin = format!("https://github.com/{}/{}", slug.owner(), slug.repo()); - let git = record.git.as_ref().ok_or_else(|| { - Error::engine("persisted Git run target is missing its Git projection") - })?; - let projected_sha = git - .sha - .as_deref() - .map(|value| { - normalize_git_commit_sha(value) - .ok_or_else(|| Error::engine("persisted Git projection has an invalid SHA")) - }) - .transpose()?; - if git.origin_url != expected_origin || git.branch != *branch || projected_sha != sha { - return Err(Error::engine( - "persisted Git run target disagrees with its Git projection", - )); - } - Ok(CloneSourceForRun { - origin_url: Some(expected_origin), - branch: Some(branch.clone()), - commit_sha: sha, - }) - } + TargetValidationError::Branch => "persisted Git run target has an invalid branch", + TargetValidationError::Sha => "persisted Git run target has an invalid SHA", + }) + })?; + let git = record + .git + .as_ref() + .ok_or_else(|| Error::engine("persisted Git run target is missing its Git projection"))?; + let projected_sha = git + .sha + .as_deref() + .map(|value| { + normalize_git_commit_sha(value) + .ok_or_else(|| Error::engine("persisted Git projection has an invalid SHA")) + }) + .transpose()?; + let expected = validated.git; + if git.origin_url != expected.origin_url + || git.branch != expected.branch + || projected_sha != expected.sha + { + return Err(Error::engine( + "persisted Git run target disagrees with its Git projection", + )); } + Ok(CloneSourceForRun { + origin_url: Some(expected.origin_url), + branch: Some(expected.branch), + commit_sha: expected.sha, + }) } async fn configured_providers_for_start( @@ -1212,7 +1203,8 @@ mod tests { RunPrepareSettings, }; use fabro_types::{ - BilledModelUsage, ManifestPath, StageTiming, WorkflowSettings, fixtures, test_support, + BilledModelUsage, ManifestPath, RunTarget, StageTiming, WorkflowSettings, fixtures, + test_support, }; use fabro_vault::SecretType; use object_store::memory::InMemory; diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 9e33bcec6..cf4fe31d6 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -130,7 +130,9 @@ pub use run_event::{ }; pub use run_failure::RunFailure; pub use run_id::{RunId, fixtures}; -pub use run_intent::{RunIntent, RunIntentArgs, RunTarget}; +pub use run_intent::{ + RunIntent, RunIntentArgs, RunTarget, TargetValidationError, ValidatedGitTarget, +}; pub use run_projection::{ ActivatedSkill, AgentControlState, CheckpointRecord, McpServerProjection, McpServerStatus, PendingInterviewRecord, RunProjection, SkillsProjection, StageContextWindow, diff --git a/lib/foundation/fabro-types/src/run_intent.rs b/lib/foundation/fabro-types/src/run_intent.rs index 7ab81e84c..74d678788 100644 --- a/lib/foundation/fabro-types/src/run_intent.rs +++ b/lib/foundation/fabro-types/src/run_intent.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use serde::{Deserialize, Serialize}; use serde_json::Value; -use crate::{RunId, WorkflowVersionId}; +use crate::{DirtyStatus, GitContext, GitHubRepositorySlug, RunId, WorkflowVersionId, repository}; /// A request to create a run from an immutable workflow version. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -47,3 +47,64 @@ pub enum RunTarget { sha: Option, }, } + +impl RunTarget { + /// Validates the target's grammar without any network resolution and + /// derives its operational Git projection. + /// + /// This is the single owner of the Git-target grammar: admission uses it + /// to reject invalid targets, and sandbox start re-derives the clone + /// source from the persisted target through the same rules. + pub fn validate(self) -> Result { + match self { + Self::Git { repo, branch, sha } => { + let slug = GitHubRepositorySlug::try_new(&repo) + .ok_or(TargetValidationError::Repository)?; + let selector = format!("heads/{branch}"); + if branch.is_empty() + || branch.starts_with("heads/") + || branch.starts_with("tags/") + || branch.starts_with("refs/") + || repository::normalize_git_commit_sha(&branch).is_some() + || !repository::is_valid_github_ref_selector(&selector) + { + return Err(TargetValidationError::Branch); + } + let sha = sha + .map(|sha| { + repository::normalize_git_commit_sha(&sha).ok_or(TargetValidationError::Sha) + }) + .transpose()?; + let git = GitContext { + origin_url: format!("https://github.com/{}/{}", slug.owner(), slug.repo()), + branch: branch.clone(), + sha: sha.clone(), + dirty: DirtyStatus::Clean, + }; + Ok(ValidatedGitTarget { + target: Self::Git { repo, branch, sha }, + git, + }) + } + } + } +} + +/// A [`RunTarget`] whose grammar has been validated, together with the +/// operational Git projection derived from it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ValidatedGitTarget { + pub target: RunTarget, + pub git: GitContext, +} + +/// A [`RunTarget`] that failed grammar validation. +#[derive(Debug, thiserror::Error, Clone, Copy, PartialEq, Eq)] +pub enum TargetValidationError { + #[error("target repository must be a valid GitHub owner/name slug")] + Repository, + #[error("target branch must be a non-empty branch name, not a ref or commit selector")] + Branch, + #[error("target SHA must be exactly 40 ASCII hexadecimal characters")] + Sha, +} diff --git a/lib/foundation/fabro-types/tests/run_intent.rs b/lib/foundation/fabro-types/tests/run_intent.rs index 7cdc03d21..3c8cff931 100644 --- a/lib/foundation/fabro-types/tests/run_intent.rs +++ b/lib/foundation/fabro-types/tests/run_intent.rs @@ -97,3 +97,65 @@ fn git_commit_sha_normalization_is_exact_and_pure() { assert_eq!(normalize_git_commit_sha(invalid), None); } } + +#[test] +fn target_validation_normalizes_sha_without_network_resolution() { + let validated = RunTarget::Git { + repo: "fabro-sh/fabro".to_string(), + branch: "feature/run-intent".to_string(), + sha: Some("ABCDEF0123456789ABCDEF0123456789ABCDEF01".to_string()), + } + .validate() + .unwrap(); + + assert_eq!( + validated.git.sha.as_deref(), + Some("abcdef0123456789abcdef0123456789abcdef01") + ); + assert_eq!( + validated.git.origin_url, + "https://github.com/fabro-sh/fabro" + ); + assert_eq!(validated.target, RunTarget::Git { + repo: "fabro-sh/fabro".to_string(), + branch: "feature/run-intent".to_string(), + sha: Some("abcdef0123456789abcdef0123456789abcdef01".to_string()), + }); +} + +#[test] +fn target_validation_rejects_invalid_grammar() { + use fabro_types::TargetValidationError; + + let validate = |repo: &str, branch: &str, sha: Option<&str>| { + RunTarget::Git { + repo: repo.to_string(), + branch: branch.to_string(), + sha: sha.map(str::to_string), + } + .validate() + }; + + assert_eq!( + validate("not-a-slug", "main", None).unwrap_err(), + TargetValidationError::Repository + ); + for branch in [ + "", + "heads/main", + "tags/v1", + "refs/heads/main", + "abcdef0123456789abcdef0123456789abcdef01", + "bad..branch", + ] { + assert_eq!( + validate("fabro-sh/fabro", branch, None).unwrap_err(), + TargetValidationError::Branch, + "{branch:?}" + ); + } + assert_eq!( + validate("fabro-sh/fabro", "main", Some("short")).unwrap_err(), + TargetValidationError::Sha + ); +}