Move GitHub coordinate validation to shared types

GitHub repository slug and git ref selector syntax now has one owner:
fabro-types::repository defines GitHubRepositorySlug with a try_new
constructor and the is_valid_github_ref_selector predicate.
fabro-automation keeps its public type path as a re-export of the same
type and delegates its existing parser and ref validation to the shared
grammar, preserving its exact error variants and messages. Server
checkout and materialization code imports the type from its canonical
owner. No wire, API, or behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-08-03 14:53:46 -04:00
parent e7fc25f62b
commit cdc88b3158
8 changed files with 234 additions and 87 deletions

1
Cargo.lock generated
View file

@ -2374,6 +2374,7 @@ dependencies = [
"chrono",
"croner",
"fabro-db",
"fabro-types",
"hex",
"serde",
"sha2 0.10.9",

View file

@ -4,10 +4,10 @@ use std::sync::Arc;
use anyhow::Context as _;
use async_trait::async_trait;
use fabro_api::types::RunManifest;
use fabro_automation::{AutomationId, AutomationTarget, GitHubRepositorySlug};
use fabro_automation::{AutomationId, AutomationTarget};
use fabro_config::{EnvironmentLayer, MergeMap};
use fabro_manifest::ManifestBuildInput;
use fabro_types::{DirtyStatus, GitContext, PreRunPushOutcome, RunId};
use fabro_types::{DirtyStatus, GitContext, GitHubRepositorySlug, PreRunPushOutcome, RunId};
use fabro_util::error::collect_chain;
use tokio::{fs, task};

View file

@ -3,8 +3,8 @@ use std::time::Duration;
use base64::Engine as _;
use base64::engine::general_purpose::STANDARD as BASE64_STANDARD;
use fabro_automation::GitHubRepositorySlug;
use fabro_store::KeyedMutex;
use fabro_types::GitHubRepositorySlug;
use tokio::process::Command;
use tokio::{fs, time};
@ -465,7 +465,7 @@ mod tests {
use super::*;
fn repository_slug(value: &str) -> GitHubRepositorySlug {
fabro_automation::parse_github_repository_slug(value).expect("slug should parse")
GitHubRepositorySlug::try_new(value).expect("slug should parse")
}
#[test]

View file

@ -16,6 +16,7 @@ workspace = true
chrono.workspace = true
croner.workspace = true
fabro-db = { path = "../../foundation/fabro-db" }
fabro-types = { path = "../../foundation/fabro-types" }
hex.workspace = true
serde.workspace = true
sha2.workspace = true

View file

@ -5,11 +5,11 @@ mod model;
mod store;
pub use error::{AutomationStoreError, AutomationValidationError};
pub use fabro_types::GitHubRepositorySlug;
pub use id::{AutomationId, AutomationRevision, AutomationRevisionParseError, AutomationTriggerId};
pub use migrations::{ImportReport, import_legacy_directory_once};
pub use model::{
ApiTrigger, Automation, AutomationDraft, AutomationReplace, AutomationTarget,
AutomationTrigger, GitHubRepositorySlug, ScheduleTrigger, parse_github_repository_slug,
parse_schedule_expression,
AutomationTrigger, ScheduleTrigger, parse_github_repository_slug, parse_schedule_expression,
};
pub use store::AutomationStore;

View file

@ -4,6 +4,7 @@ use std::sync::LazyLock;
use croner::Cron;
use croner::errors::CronError;
use croner::parser::{CronParser, Seconds, Year};
use fabro_types::{GitHubRepositorySlug, repository};
use serde::{Deserialize, Serialize};
use crate::{
@ -158,24 +159,6 @@ pub struct AutomationTarget {
pub workflow: String,
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct GitHubRepositorySlug {
owner: String,
repo: String,
}
impl GitHubRepositorySlug {
#[must_use]
pub fn owner(&self) -> &str {
&self.owner
}
#[must_use]
pub fn repo(&self) -> &str {
&self.repo
}
}
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(tag = "type", rename_all = "snake_case", deny_unknown_fields)]
pub enum AutomationTrigger {
@ -367,19 +350,10 @@ fn normalize_replace(
pub fn parse_github_repository_slug(
value: &str,
) -> Result<GitHubRepositorySlug, AutomationValidationError> {
let Some((owner, repo)) = value.split_once('/') else {
return Err(AutomationValidationError::InvalidRepositorySlug {
GitHubRepositorySlug::try_new(value).ok_or_else(|| {
AutomationValidationError::InvalidRepositorySlug {
value: value.to_string(),
});
};
if repo.contains('/') || !valid_github_owner(owner) || !valid_github_repo(repo) {
return Err(AutomationValidationError::InvalidRepositorySlug {
value: value.to_string(),
});
}
Ok(GitHubRepositorySlug {
owner: owner.to_string(),
repo: repo.to_string(),
}
})
}
@ -387,47 +361,8 @@ fn validate_repository_slug(value: &str) -> Result<(), AutomationValidationError
parse_github_repository_slug(value).map(|_| ())
}
fn valid_github_owner(value: &str) -> bool {
if value.is_empty() || value.len() > 39 {
return false;
}
let bytes = value.as_bytes();
let first = bytes[0];
let last = bytes[bytes.len() - 1];
(first.is_ascii_alphanumeric() && last.is_ascii_alphanumeric())
&& bytes
.iter()
.all(|byte| byte.is_ascii_alphanumeric() || *byte == b'-')
}
fn valid_github_repo(value: &str) -> bool {
!value.is_empty()
&& value.len() <= 100
&& value != "."
&& value != ".."
&& value
.bytes()
.all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-'))
}
fn validate_git_ref_selector(value: &str) -> Result<(), AutomationValidationError> {
let valid = !value.is_empty()
&& value.len() <= 255
&& value.trim() == value
&& !value.starts_with(['/', '-', '.'])
&& !value.ends_with(['/', '.'])
&& !has_lock_suffix(value)
&& value != "@"
&& !value.contains("..")
&& !value.contains("//")
&& !value.contains("@{")
&& value
.bytes()
.all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'/' | b'.' | b'_' | b'-'))
&& value
.split('/')
.all(|part| !part.is_empty() && !part.starts_with('.') && !has_lock_suffix(part));
if valid {
if repository::is_valid_github_ref_selector(value) {
Ok(())
} else {
Err(AutomationValidationError::InvalidGitRefSelector {
@ -458,12 +393,6 @@ fn validate_workflow_selector(value: &str) -> Result<(), AutomationValidationErr
}
}
fn has_lock_suffix(value: &str) -> bool {
value
.rsplit_once('.')
.is_some_and(|(_, extension)| extension == "lock")
}
fn validate_triggers(triggers: &[AutomationTrigger]) -> Result<(), AutomationValidationError> {
let mut seen = HashSet::new();
let mut has_api_trigger = false;
@ -505,7 +434,7 @@ fn validate_triggers(triggers: &[AutomationTrigger]) -> Result<(), AutomationVal
mod tests {
use crate::{
ApiTrigger, Automation, AutomationId, AutomationReplace, AutomationTarget,
AutomationTrigger, AutomationTriggerId, ScheduleTrigger,
AutomationTrigger, AutomationTriggerId, AutomationValidationError, ScheduleTrigger,
};
fn target() -> AutomationTarget {
@ -616,12 +545,41 @@ enabled = true
}
#[test]
fn repository_slug_parser_returns_validated_parts() {
let slug = crate::parse_github_repository_slug("owner/.github").unwrap();
fn repository_slug_parser_returns_the_shared_type() {
let slug: fabro_types::GitHubRepositorySlug =
crate::parse_github_repository_slug("owner/.github").unwrap();
assert_eq!(slug.owner(), "owner");
assert_eq!(slug.repo(), ".github");
assert!(crate::parse_github_repository_slug("not/github/slug").is_err());
}
#[test]
fn invalid_repository_slug_preserves_the_automation_error() {
let error = crate::parse_github_repository_slug("not/github/slug").unwrap_err();
assert!(matches!(
&error,
AutomationValidationError::InvalidRepositorySlug { value }
if value == "not/github/slug"
));
assert_eq!(
error.to_string(),
"repository slug \"not/github/slug\" must be a GitHub owner/repo slug"
);
}
#[test]
fn invalid_git_ref_selector_preserves_the_automation_error() {
let error = super::validate_git_ref_selector("main;rm").unwrap_err();
assert!(matches!(
&error,
AutomationValidationError::InvalidGitRefSelector { value } if value == "main;rm"
));
assert_eq!(
error.to_string(),
"git ref selector \"main;rm\" is not safe"
);
}
#[test]

View file

@ -105,7 +105,7 @@ pub use pull_request::{
PullRequestRef, PullRequestResponse, PullRequestTimestamps, PullRequestUser,
};
pub use reasoning::ReasoningOutput;
pub use repository::{RepositoryProvider, RepositoryRef};
pub use repository::{GitHubRepositorySlug, RepositoryProvider, RepositoryRef};
pub use run::{
DirtyStatus, ForkSourceRef, GitContext, PreRunPushOutcome, RunClientProvenance, RunProvenance,
RunServerProvenance, RunSpec,

View file

@ -21,6 +21,97 @@ impl RepositoryRef {
}
}
/// A validated GitHub `owner/repo` coordinate.
///
/// Construction enforces GitHub's owner and repository name syntax on the
/// exact submitted bytes; no trimming, case folding, or other normalization
/// is performed.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct GitHubRepositorySlug {
owner: String,
repo: String,
}
impl GitHubRepositorySlug {
/// Validates `value` as a GitHub `owner/repo` slug, returning `None` when
/// it is not exactly one `/`-separated pair of a valid owner and
/// repository name.
#[must_use]
pub fn try_new(value: &str) -> Option<Self> {
let (owner, repo) = value.split_once('/')?;
if repo.contains('/') || !valid_github_owner(owner) || !valid_github_repo(repo) {
return None;
}
Some(Self {
owner: owner.to_string(),
repo: repo.to_string(),
})
}
#[must_use]
pub fn owner(&self) -> &str {
&self.owner
}
#[must_use]
pub fn repo(&self) -> &str {
&self.repo
}
}
fn valid_github_owner(value: &str) -> bool {
if value.is_empty() || value.len() > 39 {
return false;
}
let bytes = value.as_bytes();
let first = bytes[0];
let last = bytes[bytes.len() - 1];
(first.is_ascii_alphanumeric() && last.is_ascii_alphanumeric())
&& bytes
.iter()
.all(|byte| byte.is_ascii_alphanumeric() || *byte == b'-')
}
fn valid_github_repo(value: &str) -> bool {
!value.is_empty()
&& value.len() <= 100
&& value != "."
&& value != ".."
&& value
.bytes()
.all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-'))
}
/// Reports whether `value` is a safe GitHub git ref selector.
///
/// The grammar checks the exact untrimmed ASCII byte content; no
/// normalization is performed.
#[must_use]
pub fn is_valid_github_ref_selector(value: &str) -> bool {
!value.is_empty()
&& value.len() <= 255
&& value.trim() == value
&& !value.starts_with(['/', '-', '.'])
&& !value.ends_with(['/', '.'])
&& !has_lock_suffix(value)
&& value != "@"
&& !value.contains("..")
&& !value.contains("//")
&& !value.contains("@{")
&& value
.bytes()
.all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'/' | b'.' | b'_' | b'-'))
&& value
.split('/')
.all(|part| !part.is_empty() && !part.starts_with('.') && !has_lock_suffix(part))
}
fn has_lock_suffix(value: &str) -> bool {
value
.rsplit_once('.')
.is_some_and(|(_, extension)| extension == "lock")
}
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
pub enum RepositoryProvider {
@ -91,3 +182,99 @@ fn repository_name_from_path(path: &str) -> Option<&str> {
fn path_basename(path: &str) -> Option<&str> {
path.rsplit(['/', '\\']).find(|segment| !segment.is_empty())
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn valid_slugs_preserve_exact_parts() {
let cases = [
("fabro-sh/fabro", "fabro-sh", "fabro"),
("owner/.github", "owner", ".github"),
];
for (input, owner, repo) in cases {
let slug = GitHubRepositorySlug::try_new(input).expect(input);
assert_eq!(slug.owner(), owner, "{input}");
assert_eq!(slug.repo(), repo, "{input}");
}
let max_owner = "a".repeat(39);
let max_repo = "b".repeat(100);
let boundary = format!("{max_owner}/{max_repo}");
let slug = GitHubRepositorySlug::try_new(&boundary).expect("boundary-length slug");
assert_eq!(slug.owner(), max_owner);
assert_eq!(slug.repo(), max_repo);
}
#[test]
fn invalid_slugs_are_rejected() {
let cases = [
"",
"fabro",
"not/github/slug",
"/repo",
"owner/",
"-owner/repo",
"owner-/repo",
"own_er/repo",
"owner/.",
"owner/..",
"owner/re po",
"öwner/repo",
"owner/rëpo",
];
for input in cases {
assert!(GitHubRepositorySlug::try_new(input).is_none(), "{input}");
}
let over_owner = format!("{}/repo", "a".repeat(40));
let over_repo = format!("owner/{}", "b".repeat(101));
assert!(GitHubRepositorySlug::try_new(&over_owner).is_none());
assert!(GitHubRepositorySlug::try_new(&over_repo).is_none());
}
#[test]
fn valid_ref_selectors_are_accepted() {
let max = "a".repeat(255);
let cases = [
"main",
"feature/release-1.2_rc",
"refs/tags/v1.0.0",
max.as_str(),
];
for input in cases {
assert!(is_valid_github_ref_selector(input), "{input}");
}
}
#[test]
fn invalid_ref_selectors_are_rejected() {
let cases = [
"",
" main",
"main ",
"/main",
"-main",
".main",
"main/",
"main.",
"feature//x",
"feature/.hidden",
"main.lock",
"branch.lock/x",
"@",
"a..b",
"a@{b",
"main;rm",
"ma\tin",
"mäin",
];
for input in cases {
assert!(!is_valid_github_ref_selector(input), "{input:?}");
}
let over = "a".repeat(256);
assert!(!is_valid_github_ref_selector(&over));
}
}