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

Move Git-target validation onto RunTarget in fabro-types

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-08-22 10:27:30 -04:00
parent 040bc6c043
commit 18d98794ae
6 changed files with 191 additions and 147 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

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

@ -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<ValidatedGitTarget, TargetValidationError> {
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<LoweredWorkflowClosure, WorkflowClosureLoweringError> {
@ -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"
);
}
}

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

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

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

@ -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<CloneSourceForRun, Error> {
});
};
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;

4
lib/foundation/fabro-types/src/lib.rs
View file

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

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

@ -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<String>,
},
}
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<ValidatedGitTarget, TargetValidationError> {
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,
}

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

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