mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-09 03:20:56 +00:00
feat(config): add validated additional github repositories
Add `additional_repositories` to `[run.integrations.github]`: a list of full `owner/repository` slugs, beyond the implicit run origin, that the minted GITHUB_TOKEN must cover. - `GitHubRepositorySlug` gains FromStr, Display, string serde, and case-insensitive Eq/Ord/Hash identity while preserving the submitted spelling for display and serialization. - The config layer keeps raw strings; the higher-precedence list replaces the lower one wholesale, with `[]` as an explicit clear, resolving independently from the `permissions` map. - Resolution validates each entry with indexed error paths: slug grammar, case-insensitive duplicates, one shared owner, the 499-repository cap, and a required `contents = "read"|"write"` permission (templated values are re-checked at the runtime boundary). - `RunIntegrationsGithubSettings` resolves permissions and repositories together through `resolve_integration()` so consumers cannot pick up one without the other; the field is omitted from serialization when empty, keeping single-repository settings byte-identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
6e65e93a2f
commit
f2047ad9a9
6 changed files with 846 additions and 20 deletions
|
|
@ -1762,7 +1762,10 @@ mod tests {
|
|||
.parse::<EnvironmentProvider>()
|
||||
.expect("test provider should parse");
|
||||
run.integrations = RunIntegrationsSettings {
|
||||
github: RunIntegrationsGithubSettings { permissions },
|
||||
github: RunIntegrationsGithubSettings {
|
||||
permissions,
|
||||
..RunIntegrationsGithubSettings::default()
|
||||
},
|
||||
};
|
||||
run
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<HashMap<String, InterpString>>,
|
||||
pub permissions: Option<HashMap<String, InterpString>>,
|
||||
/// 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<Vec<String>>,
|
||||
}
|
||||
|
||||
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),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<ResolveError>,
|
||||
) -> RunIntegrationsSettings {
|
||||
let github = layer
|
||||
.and_then(|integrations| integrations.github.as_ref())
|
||||
.map(|github| RunIntegrationsGithubSettings {
|
||||
// Collapse `Option<HashMap<...>>` -> `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<ResolveError>,
|
||||
) -> RunIntegrationsGithubSettings {
|
||||
// Collapse `Option<HashMap<...>>` -> `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<GitHubRepositorySlug> = BTreeSet::new();
|
||||
for (index, value) in raw_repositories.iter().enumerate() {
|
||||
let path = format!("run.integrations.github.additional_repositories[{index}]");
|
||||
let Ok(slug) = value.parse::<GitHubRepositorySlug>() 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<String, InterpString>,
|
||||
errors: &mut Vec<ResolveError>,
|
||||
) {
|
||||
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<RunGoal> {
|
||||
match goal? {
|
||||
RunGoalLayer::Inline(value) => Some(RunGoal::Inline(value.clone())),
|
||||
|
|
|
|||
|
|
@ -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::<SettingsLayer>()
|
||||
.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<String> {
|
||||
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<_>>(),
|
||||
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<_>>(),
|
||||
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<_>>(),
|
||||
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::<Vec<_>>()
|
||||
.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<_>>(),
|
||||
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<_>>(),
|
||||
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<_>>(),
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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<Ordering> {
|
||||
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<H: Hasher>(&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, Self::Err> {
|
||||
Self::try_new(value).ok_or(GitHubRepositorySlugError)
|
||||
}
|
||||
}
|
||||
|
||||
impl Serialize for GitHubRepositorySlug {
|
||||
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
|
||||
where
|
||||
S: serde::Serializer,
|
||||
{
|
||||
serializer.collect_str(self)
|
||||
}
|
||||
}
|
||||
|
||||
impl<'de> Deserialize<'de> for GitHubRepositorySlug {
|
||||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
|
||||
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<GitHubRepositorySlug> = ["owner/Zeta", "Owner/alpha", "owner/Beta"]
|
||||
.iter()
|
||||
.map(|value| value.parse().unwrap())
|
||||
.collect();
|
||||
slugs.sort();
|
||||
let rendered: Vec<String> = 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::<GitHubRepositorySlug>().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::<GitHubRepositorySlug>("\"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);
|
||||
|
|
|
|||
|
|
@ -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<String, InterpString>,
|
||||
pub permissions: HashMap<String, InterpString>,
|
||||
/// 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<crate::GitHubRepositorySlug>,
|
||||
}
|
||||
|
||||
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<ResolvedGithubIntegration, ResolveError> {
|
||||
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<String, String>,
|
||||
pub additional_repositories: BTreeSet<crate::GitHubRepositorySlug>,
|
||||
}
|
||||
|
||||
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<_>>(),
|
||||
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.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue