From 7a3f58c87ee2603d91de3c35e685f912aa1277bf Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 31 Aug 2026 18:02:08 -0400 Subject: [PATCH] Tighten automation Git validation types --- Cargo.lock | 1 - .../src/automation_materializer.rs | 132 ++++++++-------- lib/apps/fabro-server/src/git_checkout.rs | 39 ++--- lib/components/fabro-automation/Cargo.toml | 1 - lib/components/fabro-automation/src/error.rs | 4 +- lib/components/fabro-automation/src/model.rs | 28 ++-- lib/components/fabro-github/src/lib.rs | 91 +++++++---- .../tests/it/daytona_integration.rs | 8 +- .../fabro-api/tests/automation_round_trip.rs | 3 +- lib/foundation/fabro-types/src/lib.rs | 3 +- lib/foundation/fabro-types/src/run_intent.rs | 146 +++++++++++++----- .../fabro-types/tests/run_intent.rs | 19 +++ 12 files changed, 303 insertions(+), 172 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9075063d9..7f48470ed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2383,7 +2383,6 @@ dependencies = [ "serde_json", "sha2 0.10.9", "sqlx", - "strum 0.28.0", "tempfile", "thiserror 2.0.18", "tokio", diff --git a/lib/apps/fabro-server/src/automation_materializer.rs b/lib/apps/fabro-server/src/automation_materializer.rs index fad2958d5..80d8cb917 100644 --- a/lib/apps/fabro-server/src/automation_materializer.rs +++ b/lib/apps/fabro-server/src/automation_materializer.rs @@ -2,20 +2,18 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use async_trait::async_trait; -use fabro_automation::{ - AutomationGitWorkflowSource, AutomationId, AutomationValidationError, validate_workflow_source, -}; +use fabro_automation::{AutomationGitWorkflowSource, AutomationId}; use fabro_manifest::WorkflowVersionCollectError; use fabro_types::{ - GitHubRepositorySlug, GitRunTarget, ResolvedAutomationGitWorkflowSource, RunId, RunIntent, - RunIntentArgs, RunTarget, TargetValidationError, WorkflowVersionId, + GitCoordinateValidationError, GitHubRepositorySlug, GitRunTarget, + ResolvedAutomationGitWorkflowSource, RunId, RunIntent, RunIntentArgs, RunTarget, + WorkflowVersionId, }; use fabro_workflow_version::{WorkflowVersionStore, WorkflowVersionStoreError}; use tokio::{fs, task}; use crate::git_checkout::{ - GitAuthConfig, GitCheckoutError, GitCheckoutSelector, GitRepoCache, WorktreePrepareInput, - github_clone_url, resolve_git_read_auth_config, + self, GitAuthConfig, GitCheckoutError, GitCheckoutSelector, GitRepoCache, WorktreePrepareInput, }; #[derive(Debug, Clone, PartialEq, Eq)] @@ -56,12 +54,12 @@ pub(crate) enum RunMaterializeError { #[error("invalid automation Git target")] InvalidTarget { #[source] - source: TargetValidationError, + source: GitCoordinateValidationError, }, #[error("invalid automation workflow source")] InvalidWorkflowSource { #[source] - source: AutomationValidationError, + source: GitCoordinateValidationError, }, #[error("failed to resolve automation {role} credentials")] Credentials { @@ -153,7 +151,7 @@ struct ServerGitHubRemoteResolver { #[async_trait] impl AutomationGitRemoteResolver for ServerGitHubRemoteResolver { async fn resolve(&self, repo: &GitHubRepositorySlug) -> anyhow::Result { - let auth = resolve_git_read_auth_config( + let auth = git_checkout::resolve_git_read_auth_config( self.credentials.as_ref(), repo, &self.api_base_url, @@ -161,7 +159,7 @@ impl AutomationGitRemoteResolver for ServerGitHubRemoteResolver { ) .await?; Ok(GitRemote { - clone_url: github_clone_url(repo), + clone_url: git_checkout::github_clone_url(repo), auth, }) } @@ -232,45 +230,24 @@ impl AutomationRunMaterializer for ProductionAutomationRunMaterializer { &self, input: AutomationRunMaterializeInput, ) -> Result { - let validated_target = RunTarget::Git(input.target) + let validated_target = input + .target .validate() .map_err(|source| RunMaterializeError::InvalidTarget { source })?; - let RunTarget::Git(mut exact_target) = validated_target.target else { - unreachable!("a validated Git target remains Git-backed"); - }; - let target_repo: GitHubRepositorySlug = - exact_target - .repo - .parse() - .map_err(|_| RunMaterializeError::InvalidTarget { - source: TargetValidationError::Repository, - })?; + let target_repo = validated_target.repository().clone(); + let mut exact_target = validated_target.into_target(); let workflow_source = input .workflow_source - .map(validate_workflow_source) + .map(GitRunTarget::validate) .transpose() .map_err(|source| RunMaterializeError::InvalidWorkflowSource { source })?; // A workflow source naming the target's exact coordinate shares its // checkout; anything else needs a second worktree. - let separate_source = workflow_source - .as_ref() - .map(|source| { - source - .repo - .parse::() - .map(|repo| (repo, source)) - .map_err(|_| RunMaterializeError::InvalidWorkflowSource { - source: AutomationValidationError::InvalidWorkflowSource { - source: TargetValidationError::Repository, - }, - }) - }) - .transpose()? - .filter(|(repo, source)| { - *repo != target_repo - || GitCheckoutSelector::from(*source) - != GitCheckoutSelector::from(&exact_target) - }); + let separate_source = workflow_source.as_ref().filter(|source| { + source.repository() != &target_repo + || GitCheckoutSelector::from(source.target()) + != GitCheckoutSelector::from(&exact_target) + }); fs::create_dir_all(&input.temp_root) .await @@ -304,20 +281,21 @@ impl AutomationRunMaterializer for ProductionAutomationRunMaterializer { .await?; let (workflow_checkout_dir, workflow_checkout_sha) = match separate_source { None => (target_checkout_dir, checked_out_sha.clone()), - Some((repo, source)) => { - let remote = if repo == target_repo { + Some(source) => { + let repo = source.repository(); + let remote = if repo == &target_repo { target_remote } else { - self.resolve_remote(CheckoutRole::WorkflowSource, &repo) + self.resolve_remote(CheckoutRole::WorkflowSource, repo) .await? }; let source_checkout_dir = temp_dir.path().join("workflow-source"); let source_sha = self .prepare_checkout( CheckoutRole::WorkflowSource, - &repo, + repo, &remote, - GitCheckoutSelector::from(source), + GitCheckoutSelector::from(source.target()), &source_checkout_dir, ) .await?; @@ -327,7 +305,7 @@ impl AutomationRunMaterializer for ProductionAutomationRunMaterializer { exact_target.sha = Some(checked_out_sha); let resolved_workflow_source = workflow_source.map(|source| { Box::new(ResolvedAutomationGitWorkflowSource::from_requested( - source, + source.into_target(), workflow_checkout_sha, )) }); @@ -385,10 +363,8 @@ struct TestAutomationRunMaterializerState { #[cfg(any(test, feature = "test-support"))] #[derive(Clone)] enum TestMaterializeFailure { - InvalidTarget(TargetValidationError), - /// Unit because `AutomationValidationError` is not `Clone`; any variant - /// exercises the same handler path. - InvalidWorkflowSource, + InvalidTarget(GitCoordinateValidationError), + InvalidWorkflowSource(GitCoordinateValidationError), } #[cfg(any(test, feature = "test-support"))] @@ -396,11 +372,9 @@ impl From for RunMaterializeError { fn from(failure: TestMaterializeFailure) -> Self { match failure { TestMaterializeFailure::InvalidTarget(source) => Self::InvalidTarget { source }, - TestMaterializeFailure::InvalidWorkflowSource => Self::InvalidWorkflowSource { - source: AutomationValidationError::InvalidWorkflowSource { - source: TargetValidationError::Branch, - }, - }, + TestMaterializeFailure::InvalidWorkflowSource(source) => { + Self::InvalidWorkflowSource { source } + } } } } @@ -433,12 +407,14 @@ impl TestAutomationRunMaterializer { pub fn fail_invalid_target() -> Self { Self::new(Err(TestMaterializeFailure::InvalidTarget( - TargetValidationError::Repository, + GitCoordinateValidationError::Repository, ))) } pub fn fail_invalid_workflow_source() -> Self { - Self::new(Err(TestMaterializeFailure::InvalidWorkflowSource)) + Self::new(Err(TestMaterializeFailure::InvalidWorkflowSource( + GitCoordinateValidationError::Branch, + ))) } fn new(response: Result, TestMaterializeFailure>) -> Self { @@ -622,10 +598,7 @@ mod tests { .clone(); Ok(GitRemote { clone_url, - auth: Some(GitAuthConfig::new( - Some("x-access-token".to_string()), - Some(FAKE_TOKEN.to_string()), - )), + auth: Some(GitAuthConfig::from_parts("x-access-token", FAKE_TOKEN)), }) } } @@ -817,6 +790,39 @@ mod tests { })) } + #[tokio::test] + async fn coordinate_validation_errors_have_role_specific_nonduplicated_chains() { + let temp = TempDir::new().unwrap(); + let materializer = production_materializer( + temp.path(), + test_version_store(), + Arc::new(RecordingCredentialResolver::succeeds()), + HashMap::new(), + ); + + let mut invalid_target = + input("fabro-sh/target", None, &temp.path().join("invalid-target")); + invalid_target.target.branch = "refs/heads/main".to_string(); + let error = materializer.materialize(invalid_target).await.unwrap_err(); + assert_eq!(fabro_util::error::collect_chain(&error), [ + "invalid automation Git target", + "branch must be a non-empty branch name, not a ref or commit selector", + ]); + + let error = materializer + .materialize(input( + "fabro-sh/target", + Some(source("fabro-sh/workflows", "refs/heads/main", None, None)), + &temp.path().join("invalid-source"), + )) + .await + .unwrap_err(); + assert_eq!(fabro_util::error::collect_chain(&error), [ + "invalid automation workflow source", + "branch must be a non-empty branch name, not a ref or commit selector", + ]); + } + #[tokio::test] async fn collected_closure_stores_dependency_first_and_idempotently() { let temp = TempDir::new().unwrap(); diff --git a/lib/apps/fabro-server/src/git_checkout.rs b/lib/apps/fabro-server/src/git_checkout.rs index 5241ef973..9a2e8c6f0 100644 --- a/lib/apps/fabro-server/src/git_checkout.rs +++ b/lib/apps/fabro-server/src/git_checkout.rs @@ -270,40 +270,36 @@ pub(crate) fn github_clone_url(repo: &GitHubRepositorySlug) -> String { #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct GitAuthConfig { - extraheader: Option, + extraheader: String, sensitive_values: Vec, } impl GitAuthConfig { - pub(crate) fn new(username: Option, password: Option) -> Self { - let Some(password) = password.filter(|value| !value.is_empty()) else { - return Self { - extraheader: None, - sensitive_values: Vec::new(), - }; - }; - let username = username - .filter(|value| !value.is_empty()) - .unwrap_or_else(|| "x-access-token".to_string()); + pub(crate) fn new(credentials: &fabro_github::GitCloneCredentials) -> Self { + Self::from_parts(credentials.username(), credentials.password()) + } + + pub(crate) fn from_parts(username: &str, password: &str) -> Self { let encoded_credentials = BASE64_STANDARD.encode(format!("{username}:{password}")); let extraheader = basic_auth_header_from_encoded(&encoded_credentials); Self { - sensitive_values: vec![password, encoded_credentials, extraheader.clone()], - extraheader: Some(extraheader), + sensitive_values: vec![ + password.to_string(), + encoded_credentials, + extraheader.clone(), + ], + extraheader, } } fn git_env(&self, clone_url: &str) -> Vec<(String, String)> { - let Some(extraheader) = self.extraheader.as_ref() else { - return Vec::new(); - }; vec![ ("GIT_CONFIG_COUNT".to_string(), "1".to_string()), ( "GIT_CONFIG_KEY_0".to_string(), format!("http.{clone_url}.extraheader"), ), - ("GIT_CONFIG_VALUE_0".to_string(), extraheader.clone()), + ("GIT_CONFIG_VALUE_0".to_string(), self.extraheader.clone()), ] } @@ -327,10 +323,10 @@ pub(crate) async fn resolve_git_read_auth_config( } None => fabro_github::GitHubContext::new(credentials, github_api_base_url), }; - let (username, password) = + let credentials = fabro_github::resolve_read_only_clone_credentials(&context, repo.owner(), repo.repo()) .await?; - Ok(Some(GitAuthConfig::new(username, password))) + Ok(Some(GitAuthConfig::new(&credentials))) } #[cfg(test)] @@ -748,10 +744,7 @@ mod tests { fn credential_config_env_keeps_clone_url_uncredentialed() { let repo = repository_slug("fabro-sh/fabro"); let clone_url = github_clone_url(&repo); - let auth = GitAuthConfig::new( - Some("x-access-token".to_string()), - Some("ghu_secret".to_string()), - ); + let auth = GitAuthConfig::from_parts("x-access-token", "ghu_secret"); let plan = build_bare_clone_plan(&clone_url, Path::new("/tmp/fabro-checkout"), Some(&auth)); assert!( diff --git a/lib/components/fabro-automation/Cargo.toml b/lib/components/fabro-automation/Cargo.toml index 83ea184d7..6677b28c6 100644 --- a/lib/components/fabro-automation/Cargo.toml +++ b/lib/components/fabro-automation/Cargo.toml @@ -21,7 +21,6 @@ hex.workspace = true serde.workspace = true sha2.workspace = true sqlx.workspace = true -strum.workspace = true thiserror.workspace = true tokio.workspace = true toml.workspace = true diff --git a/lib/components/fabro-automation/src/error.rs b/lib/components/fabro-automation/src/error.rs index 44246bde7..55104055a 100644 --- a/lib/components/fabro-automation/src/error.rs +++ b/lib/components/fabro-automation/src/error.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use croner::errors::CronError; -use fabro_types::TargetValidationError; +use fabro_types::{GitCoordinateValidationError, TargetValidationError}; use toml::de::Error as TomlDeError; use toml::ser::Error as TomlSerError; @@ -27,7 +27,7 @@ pub enum AutomationValidationError { #[error("automation workflow source is invalid")] InvalidWorkflowSource { #[source] - source: TargetValidationError, + source: GitCoordinateValidationError, }, #[error("workflow selector {value:?} is not safe")] InvalidWorkflowSelector { value: String }, diff --git a/lib/components/fabro-automation/src/model.rs b/lib/components/fabro-automation/src/model.rs index 62ea4e4a2..6df1e8e0c 100644 --- a/lib/components/fabro-automation/src/model.rs +++ b/lib/components/fabro-automation/src/model.rs @@ -157,17 +157,17 @@ pub type AutomationGitWorkflowSource = GitRunTarget; /// Validate and canonicalize a saved workflow source without resolving remote /// repository state. +/// +/// # Errors +/// +/// Returns an error when the repository slug, branch, tag, or exact commit +/// does not use the canonical Git-coordinate grammar. pub fn validate_workflow_source( source: AutomationGitWorkflowSource, ) -> Result { - RunTarget::Git(source) + source .validate() - .map(|validated| match validated.target { - RunTarget::Git(source) => source, - RunTarget::None {} | RunTarget::Folder { .. } => { - unreachable!("a validated Git workflow source remains Git-backed") - } - }) + .map(fabro_types::ValidatedGitRunTarget::into_target) .map_err(|source| AutomationValidationError::InvalidWorkflowSource { source }) } @@ -470,7 +470,9 @@ fn validate_triggers(triggers: &[AutomationTrigger]) -> Result<(), AutomationVal #[cfg(test)] mod tests { - use fabro_types::{GitRunTarget, RunTarget, TargetValidationError}; + use fabro_types::{ + GitCoordinateValidationError, GitRunTarget, RunTarget, TargetValidationError, + }; use crate::{ ApiTrigger, Automation, AutomationGitWorkflowSource, AutomationId, AutomationReplace, @@ -634,24 +636,24 @@ mod tests { let cases = [ ( workflow_source("main", None, None), - TargetValidationError::Repository, + GitCoordinateValidationError::Repository, ), ( workflow_source("refs/heads/main", None, None), - TargetValidationError::Branch, + GitCoordinateValidationError::Branch, ), ( workflow_source("main", Some("tags/v1"), None), - TargetValidationError::Tag, + GitCoordinateValidationError::Tag, ), ( workflow_source("main", None, Some("short")), - TargetValidationError::Sha, + GitCoordinateValidationError::Sha, ), ]; for (mut source, expected) in cases { - if expected == TargetValidationError::Repository { + if expected == GitCoordinateValidationError::Repository { source.repo = "not/a/github/slug".to_string(); } let error = Automation::from_replace( diff --git a/lib/components/fabro-github/src/lib.rs b/lib/components/fabro-github/src/lib.rs index 7a5e0b080..76ed229eb 100644 --- a/lib/components/fabro-github/src/lib.rs +++ b/lib/components/fabro-github/src/lib.rs @@ -9,6 +9,8 @@ use fabro_types::settings::run::MergeStrategy; use serde::Deserialize; use tokio::process::Command; +use crate::token_source::SecretString; + pub mod access; pub mod token_source; @@ -1266,16 +1268,57 @@ pub async fn update_app_webhook_config( Ok(()) } -/// Resolve git clone credentials for a GitHub repository. +/// Required credentials for an authenticated GitHub HTTPS clone. /// -/// Returns `(username, password)` for authenticated cloning and pushing. -/// Always generates a token regardless of repo visibility, since the token -/// is needed for pushing from the sandbox. +/// The password is redacted from `Debug` output and should only be exposed at +/// the point where it is passed to Git. +#[derive(Clone, Debug)] +pub struct GitCloneCredentials { + username: String, + password: SecretString, +} + +impl GitCloneCredentials { + fn from_token(token: String) -> anyhow::Result { + if token.is_empty() { + bail!("GitHub clone credential token is empty"); + } + Ok(Self { + username: "x-access-token".to_string(), + password: SecretString::new(token), + }) + } + + /// The username passed to Git's HTTPS basic authentication. + #[must_use] + pub fn username(&self) -> &str { + &self.username + } + + /// The secret passed to Git's HTTPS basic authentication. + /// + /// Callers must not log or persist the returned value. + #[must_use] + pub fn password(&self) -> &str { + self.password.expose() + } +} + +/// Resolve Git clone credentials for a GitHub repository. +/// +/// Always generates credentials regardless of repository visibility because +/// the token is needed for pushing from the sandbox. +/// +/// # Errors +/// +/// Returns an error when an installation token is expired, a GitHub App token +/// cannot be minted, the HTTP client cannot be created, or the resolved token +/// is empty. pub async fn resolve_clone_credentials( ctx: &GitHubContext<'_>, owner: &str, repo: &str, -) -> anyhow::Result<(Option, Option)> { +) -> anyhow::Result { let token = match ctx.creds { GitHubCredentials::Pat(token) => token.clone(), GitHubCredentials::Installation(token) => token.valid_token()?.to_string(), @@ -1291,7 +1334,7 @@ pub async fn resolve_clone_credentials( .await? } }; - Ok((Some("x-access-token".to_string()), Some(token))) + GitCloneCredentials::from_token(token) } /// Resolve credentials for fetching repository contents without granting a @@ -1300,11 +1343,17 @@ pub async fn resolve_clone_credentials( /// Static PATs and pre-minted installation tokens retain their configured /// permissions. App credentials mint a repository-scoped token with /// `contents: read`. +/// +/// # Errors +/// +/// Returns an error when an installation token is expired, a GitHub App token +/// cannot be minted, the HTTP client cannot be created, or the resolved token +/// is empty. pub async fn resolve_read_only_clone_credentials( ctx: &GitHubContext<'_>, owner: &str, repo: &str, -) -> anyhow::Result<(Option, Option)> { +) -> anyhow::Result { let token = match ctx.creds { GitHubCredentials::Pat(token) => token.clone(), GitHubCredentials::Installation(token) => token.valid_token()?.to_string(), @@ -1320,7 +1369,7 @@ pub async fn resolve_read_only_clone_credentials( .await? } }; - Ok((Some("x-access-token".to_string()), Some(token))) + GitCloneCredentials::from_token(token) } async fn mint_git_token( @@ -1360,11 +1409,8 @@ pub async fn resolve_authenticated_url( url: &str, ) -> anyhow::Result { let (owner, repo) = parse_github_owner_repo(url)?; - let (_username, password) = resolve_clone_credentials(ctx, &owner, &repo).await?; - match password { - Some(token) => embed_token_in_url(url, &token), - None => DisplaySafeUrl::parse(url).context("Failed to parse GitHub HTTPS URL"), - } + let credentials = resolve_clone_credentials(ctx, &owner, &repo).await?; + embed_token_in_url(url, credentials.password()) } /// Fetch detailed information about a pull request. @@ -2568,13 +2614,9 @@ mod tests { .await .unwrap(); - assert_eq!( - credentials, - ( - Some("x-access-token".to_string()), - Some("ghu_test".to_string()) - ) - ); + assert_eq!(credentials.username(), "x-access-token"); + assert_eq!(credentials.password(), "ghu_test"); + assert!(!format!("{credentials:?}").contains("ghu_test")); } #[tokio::test] @@ -2586,13 +2628,8 @@ mod tests { .await .unwrap(); - assert_eq!( - credentials, - ( - Some("x-access-token".to_string()), - Some("ghu_test".to_string()) - ) - ); + assert_eq!(credentials.username(), "x-access-token"); + assert_eq!(credentials.password(), "ghu_test"); } #[tokio::test] diff --git a/lib/components/fabro-workflow/tests/it/daytona_integration.rs b/lib/components/fabro-workflow/tests/it/daytona_integration.rs index 407500c9d..02c9c3d9e 100644 --- a/lib/components/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/components/fabro-workflow/tests/it/daytona_integration.rs @@ -1195,7 +1195,7 @@ async fn daytona_clone_public_repo_gets_credentials() { // Directly test resolve_clone_credentials against a repo in an org where the // app is installed - let (username, password) = fabro_github::resolve_clone_credentials( + let credentials = fabro_github::resolve_clone_credentials( &fabro_github::GitHubContext::new(&creds, &fabro_github::github_api_base_url()), "fabro-sh", "fabro", @@ -1204,12 +1204,12 @@ async fn daytona_clone_public_repo_gets_credentials() { .unwrap(); assert_eq!( - username.as_deref(), - Some("x-access-token"), + credentials.username(), + "x-access-token", "installed org repo should get credentials for pushing" ); assert!( - password.is_some(), + !credentials.password().is_empty(), "installed org repo should get a token for pushing" ); } diff --git a/lib/foundation/fabro-api/tests/automation_round_trip.rs b/lib/foundation/fabro-api/tests/automation_round_trip.rs index 6ba104929..4d46df63f 100644 --- a/lib/foundation/fabro-api/tests/automation_round_trip.rs +++ b/lib/foundation/fabro-api/tests/automation_round_trip.rs @@ -6,7 +6,6 @@ use fabro_api::types::{ }; use fabro_automation::{ Automation, AutomationDraft, AutomationGitWorkflowSource, AutomationReplace, AutomationTrigger, - validate_workflow_source, }; use serde_json::json; @@ -160,5 +159,5 @@ fn automation_workflow_source_rejects_unknown_or_incomplete_coordinates() { "sha": "short" })) .unwrap(); - assert!(validate_workflow_source(invalid_commit).is_err()); + assert!(fabro_automation::validate_workflow_source(invalid_commit).is_err()); } diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 2c619e11d..e412be73b 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -132,7 +132,8 @@ pub use run_event::{ pub use run_failure::RunFailure; pub use run_id::{RunId, fixtures}; pub use run_intent::{ - GitRunTarget, RunIntent, RunIntentArgs, RunTarget, TargetValidationError, ValidatedRunTarget, + GitCoordinateValidationError, GitRunTarget, RunIntent, RunIntentArgs, RunTarget, + TargetValidationError, ValidatedGitRunTarget, ValidatedRunTarget, }; pub use run_projection::{ ActivatedSkill, AgentControlState, CheckpointRecord, McpServerProjection, McpServerStatus, diff --git a/lib/foundation/fabro-types/src/run_intent.rs b/lib/foundation/fabro-types/src/run_intent.rs index 4f223343e..4afca2229 100644 --- a/lib/foundation/fabro-types/src/run_intent.rs +++ b/lib/foundation/fabro-types/src/run_intent.rs @@ -66,6 +66,56 @@ pub struct GitRunTarget { pub sha: Option, } +impl GitRunTarget { + /// Validates and canonicalizes this Git coordinate without resolving remote + /// repository state. + /// + /// # Errors + /// + /// Returns an error when the repository slug, branch, tag, or exact commit + /// does not use the canonical grammar accepted for Git-backed runs. + pub fn validate(self) -> Result { + let Self { + repo, + branch, + tag, + sha, + } = self; + let repository = + GitHubRepositorySlug::try_new(&repo).ok_or(GitCoordinateValidationError::Repository)?; + if !repository::is_valid_git_branch_name(&branch) { + return Err(GitCoordinateValidationError::Branch); + } + if tag + .as_deref() + .is_some_and(|tag| !repository::is_valid_git_tag_name(tag)) + { + return Err(GitCoordinateValidationError::Tag); + } + let sha = sha + .map(|sha| { + repository::normalize_git_commit_sha(&sha).ok_or(GitCoordinateValidationError::Sha) + }) + .transpose()?; + let git = GitContext { + origin_url: repository.https_url(), + branch: branch.clone(), + sha: sha.clone(), + dirty: DirtyStatus::Clean, + }; + Ok(ValidatedGitRunTarget { + target: Self { + repo, + branch, + tag, + sha, + }, + repository, + git, + }) + } +} + impl RunTarget { /// The wire `kind` discriminator (`git`, `none`, or `folder`), for /// diagnostics. @@ -78,44 +128,18 @@ impl RunTarget { /// Git targets include their derived operational Git projection. Targets /// without a repository return no projection. Folder paths require /// filesystem validation and canonicalization during provider admission. + /// + /// # Errors + /// + /// Returns an error when a Git target's repository slug, branch, tag, or + /// exact commit does not use the canonical grammar accepted for runs. pub fn validate(self) -> Result { match self { - Self::Git(GitRunTarget { - repo, - branch, - tag, - sha, - }) => { - let slug = GitHubRepositorySlug::try_new(&repo) - .ok_or(TargetValidationError::Repository)?; - if !repository::is_valid_git_branch_name(&branch) { - return Err(TargetValidationError::Branch); - } - if tag - .as_deref() - .is_some_and(|tag| !repository::is_valid_git_tag_name(tag)) - { - return Err(TargetValidationError::Tag); - } - let sha = sha - .map(|sha| { - repository::normalize_git_commit_sha(&sha).ok_or(TargetValidationError::Sha) - }) - .transpose()?; - let git = GitContext { - origin_url: slug.https_url(), - branch: branch.clone(), - sha: sha.clone(), - dirty: DirtyStatus::Clean, - }; + Self::Git(target) => { + let validated = target.validate().map_err(TargetValidationError::from)?; Ok(ValidatedRunTarget { - target: Self::Git(GitRunTarget { - repo, - branch, - tag, - sha, - }), - git: Some(git), + target: Self::Git(validated.target), + git: Some(validated.git), }) } Self::None {} => Ok(ValidatedRunTarget { @@ -130,6 +154,34 @@ impl RunTarget { } } +/// A [`GitRunTarget`] whose local grammar has been validated and canonicalized. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ValidatedGitRunTarget { + target: GitRunTarget, + repository: GitHubRepositorySlug, + git: GitContext, +} + +impl ValidatedGitRunTarget { + /// The canonical Git target coordinate. + #[must_use] + pub fn target(&self) -> &GitRunTarget { + &self.target + } + + /// The parsed GitHub repository named by the target. + #[must_use] + pub fn repository(&self) -> &GitHubRepositorySlug { + &self.repository + } + + /// Consume the validation proof and return the canonical Git target. + #[must_use] + pub fn into_target(self) -> GitRunTarget { + self.target + } +} + /// A [`RunTarget`] whose grammar has been validated, together with its /// optional operational Git projection. #[derive(Debug, Clone, PartialEq, Eq)] @@ -138,6 +190,19 @@ pub struct ValidatedRunTarget { pub git: Option, } +/// A Git coordinate that failed local grammar validation. +#[derive(Debug, thiserror::Error, Clone, Copy, PartialEq, Eq)] +pub enum GitCoordinateValidationError { + #[error("repository must be a valid GitHub owner/name slug")] + Repository, + #[error("branch must be a non-empty branch name, not a ref or commit selector")] + Branch, + #[error("tag must be a non-empty bare tag name, not a ref or commit selector")] + Tag, + #[error("SHA must be exactly 40 ASCII hexadecimal characters")] + Sha, +} + /// A [`RunTarget`] that failed grammar validation. #[derive(Debug, thiserror::Error, Clone, Copy, PartialEq, Eq)] pub enum TargetValidationError { @@ -150,3 +215,14 @@ pub enum TargetValidationError { #[error("target SHA must be exactly 40 ASCII hexadecimal characters")] Sha, } + +impl From for TargetValidationError { + fn from(error: GitCoordinateValidationError) -> Self { + match error { + GitCoordinateValidationError::Repository => Self::Repository, + GitCoordinateValidationError::Branch => Self::Branch, + GitCoordinateValidationError::Tag => Self::Tag, + GitCoordinateValidationError::Sha => Self::Sha, + } + } +} diff --git a/lib/foundation/fabro-types/tests/run_intent.rs b/lib/foundation/fabro-types/tests/run_intent.rs index 57b2e3c0d..f9f7c5114 100644 --- a/lib/foundation/fabro-types/tests/run_intent.rs +++ b/lib/foundation/fabro-types/tests/run_intent.rs @@ -188,6 +188,25 @@ fn target_validation_normalizes_sha_without_network_resolution() { ); } +#[test] +fn git_target_validation_carries_the_parsed_repository_proof() { + let validated = GitRunTarget { + repo: "Fabro-Sh/Fabro".to_string(), + branch: "feature/run-intent".to_string(), + tag: None, + sha: Some("ABCDEF0123456789ABCDEF0123456789ABCDEF01".to_string()), + } + .validate() + .unwrap(); + + assert_eq!(validated.repository().owner(), "Fabro-Sh"); + assert_eq!(validated.repository().repo(), "Fabro"); + assert_eq!( + validated.target().sha.as_deref(), + Some("abcdef0123456789abcdef0123456789abcdef01") + ); +} + #[test] fn run_intent_none_target_validates_without_a_git_projection() { let validated = RunTarget::None {}.validate().unwrap();