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