diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 713838555..d3f21e78a 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -1762,7 +1762,10 @@ mod tests { .parse::() .expect("test provider should parse"); run.integrations = RunIntegrationsSettings { - github: RunIntegrationsGithubSettings { permissions }, + github: RunIntegrationsGithubSettings { + permissions, + ..RunIntegrationsGithubSettings::default() + }, }; run } diff --git a/lib/foundation/fabro-config/src/layers/run.rs b/lib/foundation/fabro-config/src/layers/run.rs index 8b020666f..7b6f9d57d 100644 --- a/lib/foundation/fabro-config/src/layers/run.rs +++ b/lib/foundation/fabro-config/src/layers/run.rs @@ -88,13 +88,23 @@ pub struct RunIntegrationsLayer { #[serde(deny_unknown_fields)] pub struct RunIntegrationsGithubLayer { #[serde(default, skip_serializing_if = "Option::is_none")] - pub permissions: Option>, + pub permissions: Option>, + /// Extra `owner/repository` slugs the minted `GITHUB_TOKEN` must cover in + /// addition to the implicit run origin. Kept as raw strings in this + /// sparse layer; slug validation happens at resolve time so diagnostics + /// can carry indexed paths. The higher-precedence list replaces the lower + /// one wholesale (`Some(vec![])` is an explicit clear); no `...` splice. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub additional_repositories: Option>, } impl Combine for RunIntegrationsGithubLayer { fn combine(self, other: Self) -> Self { Self { - permissions: self.permissions.or(other.permissions), + permissions: self.permissions.or(other.permissions), + additional_repositories: self + .additional_repositories + .or(other.additional_repositories), } } } diff --git a/lib/foundation/fabro-config/src/resolve/run.rs b/lib/foundation/fabro-config/src/resolve/run.rs index 9010f25f8..3fea93ab4 100644 --- a/lib/foundation/fabro-config/src/resolve/run.rs +++ b/lib/foundation/fabro-config/src/resolve/run.rs @@ -1,6 +1,8 @@ -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, BTreeSet, HashMap}; +use fabro_types::GitHubRepositorySlug; use fabro_types::settings::InterpString; +use fabro_types::settings::interp::ResolveCtx; use fabro_types::settings::run::{ ArtifactsSettings, GitAuthorSettings, HookDefinition, HookType, InterviewProviderSettings, McpServerSettings, McpTransport, MergeStrategy, NotificationProviderSettings, @@ -17,9 +19,9 @@ use crate::{ EnvironmentLayer, HookAgentMarker, HookEntry, HookTlsMode, InterviewProviderLayer, InterviewsLayer, McpEntryLayer, MergeMap, ModelRefOrSplice, NotificationProviderLayer, NotificationRouteLayer, RunAgentLayer, RunArtifactsLayer, RunCheckpointLayer, RunCloneLayer, - RunExecutionLayer, RunGitLayer, RunGoalLayer, RunIntegrationsLayer, RunLayer, - RunMetaBranchLayer, RunModelLayer, RunPrepareLayer, RunPullRequestLayer, RunRunBranchLayer, - RunScmLayer, StickyMap, StringOrSplice, + RunExecutionLayer, RunGitLayer, RunGoalLayer, RunIntegrationsGithubLayer, RunIntegrationsLayer, + RunLayer, RunMetaBranchLayer, RunModelLayer, RunPrepareLayer, RunPullRequestLayer, + RunRunBranchLayer, RunScmLayer, StickyMap, StringOrSplice, }; pub fn resolve_run( @@ -85,23 +87,140 @@ pub fn resolve_run( scm: resolve_scm(layer.scm.as_ref()), pull_request, artifacts: resolve_artifacts(layer.artifacts.as_ref(), errors), - integrations: resolve_integrations(layer.integrations.as_ref()), + integrations: resolve_integrations(layer.integrations.as_ref(), errors), } } -fn resolve_integrations(layer: Option<&RunIntegrationsLayer>) -> RunIntegrationsSettings { +fn resolve_integrations( + layer: Option<&RunIntegrationsLayer>, + errors: &mut Vec, +) -> RunIntegrationsSettings { let github = layer .and_then(|integrations| integrations.github.as_ref()) - .map(|github| RunIntegrationsGithubSettings { - // Collapse `Option>` -> `HashMap<...>`: both `None` - // and `Some({})` resolve to an empty map (no token requested). - // The presence distinction is only meaningful at merge time. - permissions: github.permissions.clone().unwrap_or_default(), - }) + .map(|github| resolve_integrations_github(github, errors)) .unwrap_or_default(); RunIntegrationsSettings { github } } +/// GitHub caps one installation token at 500 repositories; the implicit run +/// origin takes one slot. +const MAX_ADDITIONAL_REPOSITORIES: usize = 499; + +fn resolve_integrations_github( + github: &RunIntegrationsGithubLayer, + errors: &mut Vec, +) -> RunIntegrationsGithubSettings { + // Collapse `Option>` -> `HashMap<...>`: both `None` + // and `Some({})` resolve to an empty map (no token requested). + // The presence distinction is only meaningful at merge time. The same + // collapse applies to `additional_repositories` (`Some(vec![])` is an + // explicit clear that resolves to the empty set). + let permissions = github.permissions.clone().unwrap_or_default(); + let raw_repositories = github + .additional_repositories + .as_deref() + .unwrap_or_default(); + + if raw_repositories.len() > MAX_ADDITIONAL_REPOSITORIES { + errors.push(ResolveError::Invalid { + path: "run.integrations.github.additional_repositories".to_string(), + reason: format!( + "at most {MAX_ADDITIONAL_REPOSITORIES} additional repositories are supported (the \ + run origin takes the remaining slot of GitHub's 500-repository token limit), got \ + {}", + raw_repositories.len() + ), + }); + } + + let mut additional_repositories: BTreeSet = BTreeSet::new(); + for (index, value) in raw_repositories.iter().enumerate() { + let path = format!("run.integrations.github.additional_repositories[{index}]"); + let Ok(slug) = value.parse::() else { + errors.push(ResolveError::Invalid { + path, + reason: format!( + "`{value}` is not a full GitHub `owner/repository` slug (no scheme, host, \ + ref, or extra path component)" + ), + }); + continue; + }; + if let Some(existing) = additional_repositories.get(&slug) { + errors.push(ResolveError::Invalid { + path, + reason: format!( + "`{value}` duplicates `{existing}` (repository identity is case-insensitive)" + ), + }); + continue; + } + if let Some(first) = additional_repositories.first() { + if !first.same_owner(&slug) { + errors.push(ResolveError::Invalid { + path, + reason: format!( + "`{value}` has owner `{}` but `{first}` has owner `{}`; all repositories \ + must share one owner because one GitHub App installation covers one \ + account", + slug.owner(), + first.owner() + ), + }); + continue; + } + } + additional_repositories.insert(slug); + } + + if !additional_repositories.is_empty() { + validate_additional_repository_permissions(&permissions, errors); + } + + RunIntegrationsGithubSettings { + permissions, + additional_repositories, + } +} + +/// A non-empty additional-repository set needs a token that can reach +/// repository contents. Only a literal `contents` value is checked here; a +/// templated value is re-checked after interpolation at the runtime boundary. +fn validate_additional_repository_permissions( + permissions: &HashMap, + errors: &mut Vec, +) { + if permissions.is_empty() { + errors.push(ResolveError::Invalid { + path: "run.integrations.github.additional_repositories".to_string(), + reason: "additional repositories require [run.integrations.github.permissions] with \ + a `contents` permission; a higher layer may have cleared the permissions" + .to_string(), + }); + return; + } + let Some(contents) = permissions.get("contents") else { + errors.push(ResolveError::Invalid { + path: "run.integrations.github.permissions".to_string(), + reason: "additional repositories require the `contents` permission (`read` or \ + `write`)" + .to_string(), + }); + return; + }; + if let Ok(literal) = contents.resolve_with(&mut ResolveCtx::new()) { + if literal != "read" && literal != "write" { + errors.push(ResolveError::Invalid { + path: "run.integrations.github.permissions.contents".to_string(), + reason: format!( + "additional repositories require `contents = \"read\"` or `contents = \ + \"write\"`, got `{literal}`" + ), + }); + } + } +} + fn resolve_goal(goal: Option<&RunGoalLayer>) -> Option { match goal? { RunGoalLayer::Inline(value) => Some(RunGoal::Inline(value.clone())), diff --git a/lib/foundation/fabro-config/src/tests/resolve_run.rs b/lib/foundation/fabro-config/src/tests/resolve_run.rs index d416eb651..2867ca011 100644 --- a/lib/foundation/fabro-config/src/tests/resolve_run.rs +++ b/lib/foundation/fabro-config/src/tests/resolve_run.rs @@ -879,6 +879,447 @@ issues = "{{ env.GH_PERM_LEVEL }}" } } +mod run_integrations_github_additional_repositories { + //! Layer + resolver tests for + //! `[run.integrations.github].additional_repositories`. + //! + //! The list replaces wholesale across layers (`[]` is an explicit clear), + //! resolves independently from `permissions`, and validates each entry as + //! a full `owner/repository` slug with indexed error paths. + + use crate::SettingsLayer; + use crate::layers::Combine; + + fn parse_settings(source: &str) -> SettingsLayer { + source + .parse::() + .expect("fixture should parse via SettingsLayer") + } + + fn invalid_paths_and_reasons(error: crate::Error) -> Vec<(String, String)> { + let errors = match error { + crate::Error::Resolve { errors, .. } => errors, + other => panic!("expected structured resolve errors, got {other:#}"), + }; + errors + .into_iter() + .map(|error| match error { + crate::ResolveError::Invalid { path, reason } => (path, reason), + other => panic!("expected invalid-value error, got {other}"), + }) + .collect() + } + + fn resolved_repositories(settings: &fabro_types::WorkflowSettings) -> Vec { + settings + .run + .integrations + .github + .additional_repositories + .iter() + .map(ToString::to_string) + .collect() + } + + #[test] + fn resolves_one_and_multiple_repositories() { + let one = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +permissions = { contents = "read" } +"#, + ) + .expect("one additional repository should resolve"); + assert_eq!(resolved_repositories(&one), vec!["fabro-sh/keystone"]); + assert!(one.run.integrations.github.has_additional_repositories()); + + let many = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone", "fabro-sh/arc"] +permissions = { contents = "write" } +"#, + ) + .expect("multiple additional repositories should resolve"); + assert_eq!(resolved_repositories(&many), vec![ + "fabro-sh/arc", + "fabro-sh/keystone", + ]); + } + + #[test] + fn rejects_malformed_slugs_with_indexed_paths() { + let error = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = [ + "fabro-sh/keystone", + "https://github.com/fabro-sh/arc", + "git@github.com:fabro-sh/arc.git", + "fabro-sh/arc@main", + "not-a-slug", +] +permissions = { contents = "read" } +"#, + ) + .expect_err("malformed slugs should not resolve"); + + let invalid = invalid_paths_and_reasons(error); + assert_eq!( + invalid + .iter() + .map(|(path, _)| path.as_str()) + .collect::>(), + vec![ + "run.integrations.github.additional_repositories[1]", + "run.integrations.github.additional_repositories[2]", + "run.integrations.github.additional_repositories[3]", + "run.integrations.github.additional_repositories[4]", + ] + ); + assert!( + invalid[0].1.contains("owner/repository"), + "reason should explain the slug grammar: {}", + invalid[0].1 + ); + } + + #[test] + fn rejects_duplicate_and_case_variant_duplicate_slugs() { + let error = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone", "Fabro-SH/Keystone"] +permissions = { contents = "read" } +"#, + ) + .expect_err("case-variant duplicate slugs should not resolve"); + + let invalid = invalid_paths_and_reasons(error); + assert_eq!( + invalid + .iter() + .map(|(path, _)| path.as_str()) + .collect::>(), + vec!["run.integrations.github.additional_repositories[1]"] + ); + assert!( + invalid[0].1.contains("case-insensitive"), + "reason should mention case-insensitive identity: {}", + invalid[0].1 + ); + } + + #[test] + fn rejects_cross_owner_additional_repositories() { + let error = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone", "lithoscomputer/conveyor"] +permissions = { contents = "read" } +"#, + ) + .expect_err("cross-owner additional repositories should not resolve"); + + let invalid = invalid_paths_and_reasons(error); + assert_eq!( + invalid + .iter() + .map(|(path, _)| path.as_str()) + .collect::>(), + vec!["run.integrations.github.additional_repositories[1]"] + ); + assert!( + invalid[0].1.contains("share one owner"), + "reason should explain the single-owner requirement: {}", + invalid[0].1 + ); + } + + #[test] + fn rejects_more_than_the_installation_token_repository_limit() { + let repositories = (0..500) + .map(|index| format!("\"owner/repo-{index}\"")) + .collect::>() + .join(", "); + let error = super::workflow_settings_from_toml(&format!( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = [{repositories}] +permissions = {{ contents = "read" }} +"#, + )) + .expect_err("500 additional repositories should not resolve"); + + let invalid = invalid_paths_and_reasons(error); + assert_eq!( + invalid[0].0, + "run.integrations.github.additional_repositories" + ); + assert!(invalid[0].1.contains("499"), "{}", invalid[0].1); + } + + #[test] + fn rejects_additional_repositories_without_permissions() { + let error = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +"#, + ) + .expect_err("additional repositories without permissions should not resolve"); + + let invalid = invalid_paths_and_reasons(error); + assert_eq!( + invalid + .iter() + .map(|(path, _)| path.as_str()) + .collect::>(), + vec!["run.integrations.github.additional_repositories"] + ); + } + + #[test] + fn rejects_additional_repositories_without_the_contents_permission() { + let error = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +permissions = { issues = "read" } +"#, + ) + .expect_err("additional repositories require the contents permission"); + + let invalid = invalid_paths_and_reasons(error); + assert_eq!( + invalid + .iter() + .map(|(path, _)| path.as_str()) + .collect::>(), + vec!["run.integrations.github.permissions"] + ); + } + + #[test] + fn rejects_a_literal_contents_permission_that_is_not_read_or_write() { + let error = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +permissions = { contents = "admin" } +"#, + ) + .expect_err("literal contents permission must be read or write"); + + let invalid = invalid_paths_and_reasons(error); + assert_eq!( + invalid + .iter() + .map(|(path, _)| path.as_str()) + .collect::>(), + vec!["run.integrations.github.permissions.contents"] + ); + } + + #[test] + fn defers_a_templated_contents_permission_to_the_runtime_boundary() { + let settings = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +permissions = { contents = "{{ vars.GH_CONTENTS }}" } +"#, + ) + .expect("templated contents permission resolves; the value is re-checked at runtime"); + + assert_eq!(resolved_repositories(&settings), vec!["fabro-sh/keystone"]); + } + + #[test] + fn higher_layer_replaces_the_repository_list_wholesale() { + let workflow = parse_settings( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +"#, + ); + let user = parse_settings( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/arc", "fabro-sh/widgets"] +permissions = { contents = "read" } +"#, + ); + let merged = workflow.combine(user); + + let resolved = + super::workflow_settings_from_layer(merged).expect("merged settings should resolve"); + + // The lists never union: the higher layer's single entry wins, while + // the permission map inherits independently from the lower layer. + assert_eq!(resolved_repositories(&resolved), vec!["fabro-sh/keystone"]); + assert_eq!( + resolved.run.integrations.github.permissions.len(), + 1, + "permissions should inherit from the lower layer" + ); + } + + #[test] + fn absent_higher_layer_inherits_the_lower_repository_list() { + let workflow = parse_settings("_version = 1\n"); + let user = parse_settings( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +permissions = { contents = "read" } +"#, + ); + let merged = workflow.combine(user); + + let resolved = + super::workflow_settings_from_layer(merged).expect("merged settings should resolve"); + assert_eq!(resolved_repositories(&resolved), vec!["fabro-sh/keystone"]); + } + + #[test] + fn empty_higher_layer_list_clears_inherited_repositories() { + let workflow = parse_settings( + r" +_version = 1 + +[run.integrations.github] +additional_repositories = [] +", + ); + let user = parse_settings( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +permissions = { contents = "read" } +"#, + ); + let merged = workflow.combine(user); + + let resolved = + super::workflow_settings_from_layer(merged).expect("merged settings should resolve"); + + assert!( + resolved + .run + .integrations + .github + .additional_repositories + .is_empty(), + "explicit [] should clear the inherited repository list" + ); + // Permissions survive the repository clear: each field resolves + // independently. + assert!(resolved.run.integrations.github.is_token_requested()); + } + + #[test] + fn rejects_repositories_that_survive_a_cross_layer_permission_clear() { + let workflow = parse_settings( + r" +_version = 1 + +[run.integrations.github] +permissions = {} +", + ); + let user = parse_settings( + r#" +_version = 1 + +[run.integrations.github] +additional_repositories = ["fabro-sh/keystone"] +permissions = { contents = "read" } +"#, + ); + let merged = workflow.combine(user); + + let error = super::workflow_settings_from_layer(merged) + .map(|_| ()) + .expect_err("repositories with cleared permissions should not resolve"); + + let message = error.to_string(); + assert!( + message.contains("additional_repositories"), + "error should name the invalid combination: {message}" + ); + } + + #[test] + fn permissions_only_and_fully_empty_shapes_are_preserved() { + let permissions_only = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.integrations.github.permissions] +issues = "read" +"#, + ) + .expect("permissions-only settings should resolve"); + assert!( + permissions_only + .run + .integrations + .github + .additional_repositories + .is_empty() + ); + assert!( + permissions_only + .run + .integrations + .github + .is_token_requested() + ); + + let empty = super::workflow_settings_from_toml("_version = 1\n") + .expect("empty settings should resolve"); + assert!( + empty + .run + .integrations + .github + .additional_repositories + .is_empty() + ); + assert!(!empty.run.integrations.github.is_token_requested()); + } +} + mod run_agent { use crate::SettingsLayer; use crate::layers::Combine; diff --git a/lib/foundation/fabro-types/src/repository.rs b/lib/foundation/fabro-types/src/repository.rs index 778b3ebf0..9c98d1d88 100644 --- a/lib/foundation/fabro-types/src/repository.rs +++ b/lib/foundation/fabro-types/src/repository.rs @@ -1,3 +1,8 @@ +use std::cmp::Ordering; +use std::fmt; +use std::hash::{Hash, Hasher}; +use std::str::FromStr; + use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -25,8 +30,10 @@ impl RepositoryRef { /// /// 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)] +/// is performed. The original spelling is preserved for `Display` and +/// serialization, while identity (`Eq`, `Ord`, `Hash`) is case-insensitive +/// to match GitHub's treatment of owner and repository names. +#[derive(Debug, Clone)] pub struct GitHubRepositorySlug { owner: String, repo: String, @@ -57,6 +64,90 @@ impl GitHubRepositorySlug { pub fn repo(&self) -> &str { &self.repo } + + /// Whether `other` names the same repository owner, ignoring ASCII case. + /// Owner and repository names are validated ASCII, so ASCII folding is + /// exact. + #[must_use] + pub fn same_owner(&self, other: &Self) -> bool { + self.owner.eq_ignore_ascii_case(&other.owner) + } + + fn canonical_key(&self) -> (String, String) { + ( + self.owner.to_ascii_lowercase(), + self.repo.to_ascii_lowercase(), + ) + } +} + +impl PartialEq for GitHubRepositorySlug { + fn eq(&self, other: &Self) -> bool { + self.owner.eq_ignore_ascii_case(&other.owner) && self.repo.eq_ignore_ascii_case(&other.repo) + } +} + +impl Eq for GitHubRepositorySlug {} + +impl PartialOrd for GitHubRepositorySlug { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for GitHubRepositorySlug { + fn cmp(&self, other: &Self) -> Ordering { + self.canonical_key().cmp(&other.canonical_key()) + } +} + +impl Hash for GitHubRepositorySlug { + fn hash(&self, state: &mut H) { + self.canonical_key().hash(state); + } +} + +impl fmt::Display for GitHubRepositorySlug { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}/{}", self.owner, self.repo) + } +} + +/// Parse failure for [`GitHubRepositorySlug`]. The offending input is not +/// echoed back because config surfaces already attach the value and its path. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +#[error( + "expected a GitHub `owner/repository` slug with no scheme, host, ref, or extra path component" +)] +pub struct GitHubRepositorySlugError; + +impl FromStr for GitHubRepositorySlug { + type Err = GitHubRepositorySlugError; + + fn from_str(value: &str) -> Result { + Self::try_new(value).ok_or(GitHubRepositorySlugError) + } +} + +impl Serialize for GitHubRepositorySlug { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.collect_str(self) + } +} + +impl<'de> Deserialize<'de> for GitHubRepositorySlug { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::Error as _; + + let value = String::deserialize(deserializer)?; + value.parse().map_err(D::Error::custom) + } } fn valid_github_owner(value: &str) -> bool { @@ -234,6 +325,69 @@ mod tests { assert!(GitHubRepositorySlug::try_new(&over_repo).is_none()); } + #[test] + fn slug_identity_is_case_insensitive_but_display_preserves_case() { + let mixed: GitHubRepositorySlug = "Fabro-SH/Keystone".parse().unwrap(); + let lower: GitHubRepositorySlug = "fabro-sh/keystone".parse().unwrap(); + + assert_eq!(mixed, lower); + assert_eq!(mixed.cmp(&lower), std::cmp::Ordering::Equal); + assert!(mixed.same_owner(&lower)); + assert_eq!(mixed.to_string(), "Fabro-SH/Keystone"); + + let mut hashes = std::collections::HashSet::new(); + hashes.insert(mixed.clone()); + assert!( + !hashes.insert(lower.clone()), + "case variants share identity" + ); + + let mut ordered = std::collections::BTreeSet::new(); + ordered.insert(mixed); + assert!(!ordered.insert(lower), "case variants share ordering"); + } + + #[test] + fn slug_ordering_sorts_by_canonical_form() { + let mut slugs: Vec = ["owner/Zeta", "Owner/alpha", "owner/Beta"] + .iter() + .map(|value| value.parse().unwrap()) + .collect(); + slugs.sort(); + let rendered: Vec = slugs.iter().map(ToString::to_string).collect(); + assert_eq!(rendered, ["Owner/alpha", "owner/Beta", "owner/Zeta"]); + } + + #[test] + fn slug_from_str_rejects_urls_and_hosts() { + let cases = [ + "https://github.com/owner/repo", + "git@github.com:owner/repo.git", + "ssh://git@github.com/owner/repo", + "github.com/owner/repo", + "owner/repo@main", + "owner/repo#ref", + " owner/repo", + "owner/repo ", + ]; + for input in cases { + assert!(input.parse::().is_err(), "{input}"); + } + } + + #[test] + fn slug_serde_round_trips_as_a_string() { + let slug: GitHubRepositorySlug = "Fabro-SH/Keystone".parse().unwrap(); + let json = serde_json::to_string(&slug).unwrap(); + assert_eq!(json, "\"Fabro-SH/Keystone\""); + + let parsed: GitHubRepositorySlug = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed, slug); + + let err = serde_json::from_str::("\"not a slug\"").unwrap_err(); + assert!(err.to_string().contains("owner/repository"), "{err}"); + } + #[test] fn valid_ref_selectors_are_accepted() { let max = "a".repeat(255); diff --git a/lib/foundation/fabro-types/src/settings/run.rs b/lib/foundation/fabro-types/src/settings/run.rs index 8e589a57a..12cd04fce 100644 --- a/lib/foundation/fabro-types/src/settings/run.rs +++ b/lib/foundation/fabro-types/src/settings/run.rs @@ -6,7 +6,7 @@ //! notifications, interviews, agent knobs, hooks, SCM targeting, pull-request //! behavior, and artifact collection. -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::path::PathBuf; use std::time::Duration as StdDuration; @@ -603,9 +603,19 @@ pub struct RunIntegrationsSettings { /// presence-vs-clear distinction is only meaningful at the layer-merge /// stage; the resolved form collapses both `None` and `Some({})` into an /// empty map. +/// +/// `additional_repositories` lists repositories, beyond the implicit run +/// origin, that the minted `GITHUB_TOKEN` must cover. Configuration +/// resolution guarantees a non-empty set comes with a non-empty permission +/// map that includes `contents`; runs persisted before the field existed +/// deserialize to an empty set via the serde default. #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct RunIntegrationsGithubSettings { - pub permissions: HashMap, + pub permissions: HashMap, + /// Omitted when empty so settings serialized by this release stay + /// byte-identical to earlier releases for single-repository runs. + #[serde(default, skip_serializing_if = "BTreeSet::is_empty")] + pub additional_repositories: BTreeSet, } impl RunIntegrationsGithubSettings { @@ -616,6 +626,11 @@ impl RunIntegrationsGithubSettings { !self.permissions.is_empty() } + /// Whether the run declares additional repositories beyond the origin. + pub fn has_additional_repositories(&self) -> bool { + !self.additional_repositories.is_empty() + } + /// Resolve every `permissions` value. `{{ vars.* }}` is substituted /// server-side at run creation, so values are literal by this point; a /// still-unresolved token fails closed rather than reaching the GitHub API @@ -627,6 +642,42 @@ impl RunIntegrationsGithubSettings { .map(|(name, value)| Ok((name.clone(), value.resolve_with(&mut ctx)?))) .collect() } + + /// Resolve the whole runtime integration request: interpolated + /// permissions plus the declared additional repositories, produced + /// together so consumers cannot pick up one without the other. + pub fn resolve_integration(&self) -> Result { + Ok(ResolvedGithubIntegration { + permissions: self.resolve_permissions()?, + additional_repositories: self.additional_repositories.clone(), + }) + } +} + +/// The resolved runtime GitHub integration request for one run: interpolated +/// permission values plus the declared additional repositories. +/// +/// This is the single value carried from run materialization into workflow +/// startup, replacing parallel permission/repository collections that could +/// drift apart. +#[derive(Debug, Clone, Default, PartialEq)] +pub struct ResolvedGithubIntegration { + pub permissions: HashMap, + pub additional_repositories: BTreeSet, +} + +impl ResolvedGithubIntegration { + /// Mirrors [`RunIntegrationsGithubSettings::is_token_requested`] for the + /// resolved form. + #[must_use] + pub fn is_token_requested(&self) -> bool { + !self.permissions.is_empty() + } + + #[must_use] + pub fn has_additional_repositories(&self) -> bool { + !self.additional_repositories.is_empty() + } } #[cfg(test)] @@ -635,10 +686,11 @@ mod run_integrations_github_tests { fn settings(permissions: &[(&str, &str)]) -> RunIntegrationsGithubSettings { RunIntegrationsGithubSettings { - permissions: permissions + permissions: permissions .iter() .map(|(k, v)| ((*k).to_string(), InterpString::parse(v))) .collect(), + additional_repositories: std::collections::BTreeSet::new(), } } @@ -670,6 +722,53 @@ mod run_integrations_github_tests { fn resolve_permissions_is_empty_for_empty_settings() { assert!(settings(&[]).resolve_permissions().unwrap().is_empty()); } + + #[test] + fn settings_without_additional_repositories_field_deserialize_to_empty_set() { + // Persisted run.created events from releases before + // `additional_repositories` existed omit the field entirely. + let parsed: RunIntegrationsGithubSettings = serde_json::from_value(serde_json::json!({ + "permissions": { "contents": "read" } + })) + .expect("legacy settings should deserialize"); + + assert!(parsed.additional_repositories.is_empty()); + assert!(!parsed.has_additional_repositories()); + } + + #[test] + fn resolve_integration_carries_permissions_and_repositories_together() { + let mut s = settings(&[("contents", "read")]); + s.additional_repositories + .insert("fabro-sh/keystone".parse().unwrap()); + + let resolved = s.resolve_integration().unwrap(); + + assert!(resolved.is_token_requested()); + assert!(resolved.has_additional_repositories()); + assert_eq!( + resolved.permissions.get("contents"), + Some(&"read".to_string()) + ); + assert_eq!( + resolved + .additional_repositories + .iter() + .map(ToString::to_string) + .collect::>(), + vec!["fabro-sh/keystone"] + ); + } + + #[test] + fn resolve_integration_fails_on_an_unresolved_permission_token() { + let mut s = settings(&[("contents", "{{ env.GH_PERM_LEVEL }}")]); + s.additional_repositories + .insert("fabro-sh/keystone".parse().unwrap()); + + let err = s.resolve_integration().unwrap_err(); + assert_eq!(err.namespace, Namespace::Env); + } } /// The resolved source of a run goal.