From 2b02ec8ca2cb3b0ab1d22757a4de35ceb7a04d60 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 09:50:21 +0100 Subject: [PATCH 01/18] feat(permissions): add typed repository write configuration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/compile/agentic_pipeline.rs | 73 +++----- src/compile/common.rs | 222 +++++++++++++++++++----- src/compile/types.rs | 291 +++++++++++++++++++++++++++++--- 3 files changed, 480 insertions(+), 106 deletions(-) diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 79a99fb7..41079f69 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -169,6 +169,7 @@ fn validate_pipeline_front_matter( ) -> Result<()> { common::validate_front_matter_identity(front_matter)?; common::validate_permissions_read_policy(front_matter)?; + common::validate_permissions_write_policy(front_matter)?; if let Some(minutes) = front_matter.engine.timeout_minutes() { common::validate_proxied_timeout(front_matter, minutes)?; } @@ -501,7 +502,8 @@ pub(crate) fn build_pipeline_context( front_matter .permissions .as_ref() - .and_then(|p| p.write.as_deref()), + .and_then(|p| p.write.as_ref()) + .map(crate::compile::types::WritePermissionConfig::service_connection), "SC_WRITE_TOKEN", ); // Skip integrity check resolution @@ -2274,9 +2276,7 @@ fn prepare_custom_agent_output_step(config_path: &str, output_path: &str) -> Bas fn agent_temp_filename(path: &str) -> String { let prefix = "$(Agent.TempDirectory)/"; path.strip_prefix(prefix) - .unwrap_or_else(|| panic!( - "custom-tools config path {path:?} must start with {prefix:?}" - )) + .unwrap_or_else(|| panic!("custom-tools config path {path:?} must start with {prefix:?}")) .to_string() } @@ -2508,7 +2508,8 @@ fn build_safeoutputs_job( front_matter .permissions .as_ref() - .and_then(|permissions| permissions.write.as_deref()), + .and_then(|permissions| permissions.write.as_ref()) + .map(crate::compile::types::WritePermissionConfig::service_connection), github_auth.as_ref(), ); let resolved_config_path = "$(Agent.TempDirectory)/ado-aw-resolved-config.json"; @@ -3079,7 +3080,8 @@ fn build_conclusion_job( let write_sc = front_matter .permissions .as_ref() - .and_then(|p| p.write.as_deref()); + .and_then(|p| p.write.as_ref()) + .map(crate::compile::types::WritePermissionConfig::service_connection); conclusion_step = apply_bundle_auth( conclusion_step, Bundle::Conclusion, @@ -3093,20 +3095,15 @@ fn build_conclusion_job( // defaults (type: Task, no area/iteration path). The global // report-failure-as-work-item toggle controls whether it files at all. for tool_key in &["noop", "missing-tool", "missing-data"] { - conclusion_step = - apply_conclusion_tool_config_env(conclusion_step, front_matter, tool_key); + conclusion_step = apply_conclusion_tool_config_env(conclusion_step, front_matter, tool_key); } // Pass upstream job results via job-level variables hoist. // ADO only evaluates $[...] runtime expressions inside `variables:` and // `condition:` — NOT in step env blocks. We hoist to job variables and // reference them as $(name) macros in the step env. - let (conclusion_variables, conclusion_step) = hoist_conclusion_job_results( - conclusion_step, - prefix, - custom_defs, - has_reviewed_job, - )?; + let (conclusion_variables, conclusion_step) = + hoist_conclusion_job_results(conclusion_step, prefix, custom_defs, has_reviewed_job)?; steps.push(Step::Bash(conclusion_step)); @@ -3823,8 +3820,7 @@ fn prepare_mcpg_config_step( {mcpg_sentinel}" ); let custom_tools_fragment = if let Some(custom_tools_json) = custom_tools_json { - let sentinel = - super::common::heredoc_sentinel("CUSTOM_TOOLS_JSON_EOF", custom_tools_json)?; + let sentinel = super::common::heredoc_sentinel("CUSTOM_TOOLS_JSON_EOF", custom_tools_json)?; format!( "# Write compiler-generated dynamic SafeOutputs tool definitions\n\ cat > \"$AGENT_TEMP/staging/custom-tools.json\" << '{sentinel}'\n\ @@ -4571,10 +4567,7 @@ fn execute_safe_outputs_step( // no part of it needs separate lowering. EnvValue::literal(self_repository_directory), ); - script = script.with_env( - "ADO_AW_SELF_REPOSITORY_NAME", - self_repository_name.clone(), - ); + script = script.with_env("ADO_AW_SELF_REPOSITORY_NAME", self_repository_name.clone()); Ok(script) } @@ -4901,23 +4894,14 @@ fn start_ado_proxy_step(front_matter: &FrontMatter) -> BashStep { Binding::text(ado_proxy_container_entrypoint_flattened()), ) .fragment("resolve_org", common::resolve_ado_organization_bash()) - .fragment( - "setup_workdir", - phase_body(&START_ADO_PROXY_SETUP_WORKDIR), - ) + .fragment("setup_workdir", phase_body(&START_ADO_PROXY_SETUP_WORKDIR)) .fragment("write_policy", phase_body(&START_ADO_PROXY_WRITE_POLICY)) - .fragment( - "mint_material", - phase_body(&START_ADO_PROXY_MINT_MATERIAL), - ) + .fragment("mint_material", phase_body(&START_ADO_PROXY_MINT_MATERIAL)) .fragment( "build_material", phase_body(&START_ADO_PROXY_BUILD_MATERIAL), ) - .fragment( - "run_container", - phase_body(&START_ADO_PROXY_RUN_CONTAINER), - ) + .fragment("run_container", phase_body(&START_ADO_PROXY_RUN_CONTAINER)) .fragment( "handover_material", phase_body(&START_ADO_PROXY_HANDOVER_MATERIAL), @@ -6195,7 +6179,10 @@ fn verify_mcp_backends_step() -> BashStep { ShellScript::new(&VERIFY_MCP_BACKENDS) .bind("MCPG_PORT", Binding::number(MCPG_PORT.into())) .into_step("Verify MCP backends") - .with_env("MCPG_API_KEY", EnvValue::pipeline_var("MCP_GATEWAY_API_KEY")) + .with_env( + "MCPG_API_KEY", + EnvValue::pipeline_var("MCP_GATEWAY_API_KEY"), + ) } // ───────────────────────────────────────────────────────────────────── @@ -7296,9 +7283,9 @@ safe-outputs: step.script ); assert!( - step.script.contains( - "printf '%s' \"$PROXY_MATERIAL\" | docker exec -i \"$PROXY_CONTAINER\"" - ) && step.script.contains("cat > /tmp/ado-proxy-material"), + step.script + .contains("printf '%s' \"$PROXY_MATERIAL\" | docker exec -i \"$PROXY_CONTAINER\"") + && step.script.contains("cat > /tmp/ado-proxy-material"), "material must stream through the container-private FIFO: {}", step.script ); @@ -7357,8 +7344,7 @@ safe-outputs: let copy = copy_logs_step("/tmp/copilot", false); assert!(copy.script.contains("/tmp/gh-aw/ado-proxy-logs")); assert!( - copy.script - .contains("AGENT_TEMP='$(Agent.TempDirectory)'") + copy.script.contains("AGENT_TEMP='$(Agent.TempDirectory)'") && copy .script .contains(r#""$AGENT_TEMP/staging/logs/ado-proxy""#), @@ -7395,9 +7381,8 @@ safe-outputs: ); assert!( script.contains(&format!("CA_HOST_PATH='{ADO_PROXY_PUBLIC_CA_HOST_PATH}'")) - && script.contains( - "##vso[task.setvariable variable=ADO_PROXY_CA_FILE]$CA_HOST_PATH" - ), + && script + .contains("##vso[task.setvariable variable=ADO_PROXY_CA_FILE]$CA_HOST_PATH"), "clients need the published certificate's path: {script}" ); assert!( @@ -7450,10 +7435,8 @@ safe-outputs: "docker run must reuse the bound $PROXY_IMAGE: {script}" ); assert!( - script.contains(&format!( - "PROXY_SCRIPT_PATH='{}'", - paths::ADO_PROXY_PATH - )) && script.contains("\"$PROXY_SCRIPT_PATH:/app/ado-proxy.js:ro\""), + script.contains(&format!("PROXY_SCRIPT_PATH='{}'", paths::ADO_PROXY_PATH)) + && script.contains("\"$PROXY_SCRIPT_PATH:/app/ado-proxy.js:ro\""), "docker run must mount the bound ado-proxy bundle: {script}" ); } diff --git a/src/compile/common.rs b/src/compile/common.rs index 9a1f51ab..e3dc5d77 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -691,6 +691,19 @@ pub fn validate_permissions_read_policy(front_matter: &FrontMatter) -> Result<() options.validate() } +/// Validate the expanded Stage 3 write credential and its additional scopes. +pub fn validate_permissions_write_policy(front_matter: &FrontMatter) -> Result<()> { + let Some(options) = front_matter + .permissions + .as_ref() + .and_then(|permissions| permissions.write.as_ref()) + .and_then(crate::compile::types::WritePermissionConfig::options) + else { + return Ok(()); + }; + options.validate() +} + /// Validate the `variable-groups:` front-matter block (issue #1385). /// /// Enforces two rules before the pipeline is built: @@ -897,6 +910,45 @@ pub fn validate_repo_endpoint( Ok(()) } +fn validate_repo_organization( + repo_type: &str, + endpoint: &Option, + organization: Option<&crate::secure::AdoOrganization>, + name: &str, +) -> Result<()> { + let Some(organization) = organization else { + return Ok(()); + }; + if !repo_type.eq_ignore_ascii_case("git") { + anyhow::bail!( + "Repository '{name}' sets `organization: {organization}`, but `organization` \ + is supported only for Azure Repos `type: git` entries." + ); + } + if endpoint.as_deref().is_none_or(str::is_empty) { + anyhow::bail!( + "Cross-organization repository '{name}' sets `organization: {organization}` \ + but has no `endpoint:` service connection for checkout." + ); + } + let Some((project, repository)) = name.split_once('/') else { + anyhow::bail!( + "Cross-organization repository '{name}' must use `name: project/repository`." + ); + }; + if repository.contains('/') { + anyhow::bail!( + "Cross-organization repository '{name}' must contain exactly one '/' between \ + the project and repository names." + ); + } + crate::secure::AdoProject::parse(project) + .with_context(|| format!("invalid project in cross-organization repository '{name}'"))?; + crate::secure::AdoRepository::parse(repository) + .with_context(|| format!("invalid repository in cross-organization repository '{name}'"))?; + Ok(()) +} + /// Lower a `repos:` list into the internal [`LoweredRepos`] triple consumed by /// the rest of the compiler. A reserved `self` entry (an entry whose *name* is /// exactly `self`) contributes only fetch tuning under the @@ -927,38 +979,41 @@ pub fn lower_repos(items: &[ReposItem]) -> Result { continue; } - let (name, alias, repo_type, repo_ref, endpoint, do_checkout, fetch_opts) = match item { - ReposItem::Shorthand(s) => { - let (alias, name) = parse_shorthand(s)?; - ( - name, - alias, - "git".to_string(), - "refs/heads/main".to_string(), - None, - true, - CheckoutFetchOpts::default(), - ) - } - ReposItem::Full(entry) => { - let alias = match &entry.alias { - Some(a) => a.clone(), - None => derive_alias(&entry.name)?, - }; - ( - entry.name.clone(), - alias, - entry.repo_type.clone(), - entry.repo_ref.clone(), - entry.endpoint.clone(), - entry.checkout, - CheckoutFetchOpts { - fetch_depth: entry.fetch_depth, - fetch_tags: entry.fetch_tags, - }, - ) - } - }; + let (name, alias, repo_type, repo_ref, endpoint, organization, do_checkout, fetch_opts) = + match item { + ReposItem::Shorthand(s) => { + let (alias, name) = parse_shorthand(s)?; + ( + name, + alias, + "git".to_string(), + "refs/heads/main".to_string(), + None, + None, + true, + CheckoutFetchOpts::default(), + ) + } + ReposItem::Full(entry) => { + let alias = match &entry.alias { + Some(a) => a.clone(), + None => derive_alias(&entry.name)?, + }; + ( + entry.name.clone(), + alias, + entry.repo_type.clone(), + entry.repo_ref.clone(), + entry.endpoint.clone(), + entry.organization.clone(), + entry.checkout, + CheckoutFetchOpts { + fetch_depth: entry.fetch_depth, + fetch_tags: entry.fetch_tags, + }, + ) + } + }; // Reject aliases that aren't safe as a single path segment. The alias // is used unquoted as an ADO `checkout:` value / repository resource @@ -1015,6 +1070,7 @@ pub fn lower_repos(items: &[ReposItem]) -> Result { } validate_repo_endpoint(&repo_type, &endpoint, &name)?; + validate_repo_organization(&repo_type, &endpoint, organization.as_ref(), &name)?; repositories.push(Repository { repository: alias.clone(), @@ -1022,6 +1078,7 @@ pub fn lower_repos(items: &[ReposItem]) -> Result { name, repo_ref, endpoint, + organization, }); if do_checkout { @@ -1065,6 +1122,12 @@ fn self_entry_fetch_opts(item: &ReposItem) -> Result> if entry.alias.is_some() { unsupported.push("alias"); } + if entry.endpoint.is_some() { + unsupported.push("endpoint"); + } + if entry.organization.is_some() { + unsupported.push("organization"); + } if entry.repo_type != "git" { unsupported.push("type"); } @@ -2192,11 +2255,7 @@ pub fn validate_github_issue_outputs_config(front_matter: &FrontMatter) -> Resul { for consumer in crate::compile::types::GITHUB_TEMPORARY_ID_CONSUMERS { if front_matter.safe_outputs.contains_key(*consumer) { - require_same_approval_lane( - front_matter, - "create-github-issue", - consumer, - )?; + require_same_approval_lane(front_matter, "create-github-issue", consumer)?; } } } @@ -3647,9 +3706,7 @@ pub fn generate_awf_path_step(awf_paths: &[String]) -> String { } }) .collect(); - format!( - "- bash: |\n{indented} displayName: \"Generate GITHUB_PATH file\"" - ) + format!("- bash: |\n{indented} displayName: \"Generate GITHUB_PATH file\"") } shell_script! { @@ -4405,6 +4462,7 @@ mod tests { name: "org/my-repo".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, }]; let checkout = vec!["my-repo".to_string()]; let result = validate_checkout_list(&repos, &checkout); @@ -4419,6 +4477,7 @@ mod tests { name: "org/my-repo".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, }]; let checkout = vec!["unknown-alias".to_string()]; let result = validate_checkout_list(&repos, &checkout); @@ -4434,6 +4493,7 @@ mod tests { name: "org/my-repo".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, }]; let result = validate_checkout_list(&repos, &[]); assert!(result.is_ok()); @@ -4449,6 +4509,7 @@ mod tests { name: "org/repo".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, }]; let checkout = vec!["repo".to_string()]; let err = validate_checkout_list(&repos, &checkout).unwrap_err(); @@ -8294,6 +8355,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: None, fetch_tags: None, @@ -8301,9 +8363,74 @@ safe-outputs: let (repos, checkout, _fetch) = lower_repos(&items).unwrap(); assert_eq!(repos[0].repository, "docs"); assert_eq!(repos[0].name, "my-org/docs"); + assert!(repos[0].organization.is_none()); assert_eq!(checkout, vec!["docs"]); } + #[test] + fn test_repos_object_form_preserves_cross_org_organization() { + let items = vec![ReposItem::Full(RepoEntry { + name: "Other Project/docs".to_string(), + alias: Some("cross-docs".to_string()), + repo_type: "git".to_string(), + repo_ref: "refs/heads/main".to_string(), + endpoint: Some("cross-org-checkout".to_string()), + organization: Some(crate::secure::AdoOrganization::parse("other-org").unwrap()), + checkout: true, + fetch_depth: None, + fetch_tags: None, + })]; + + let (repos, checkout, _fetch) = lower_repos(&items).unwrap(); + + assert_eq!(checkout, vec!["cross-docs"]); + assert_eq!( + repos[0].organization.as_ref().map(|value| value.as_str()), + Some("other-org") + ); + } + + #[test] + fn test_repos_cross_org_metadata_requires_git_endpoint_and_valid_name() { + for item in [ + RepoEntry { + name: "Other Project/docs".to_string(), + alias: Some("docs".to_string()), + repo_type: "github".to_string(), + repo_ref: "refs/heads/main".to_string(), + endpoint: Some("checkout".to_string()), + organization: Some(crate::secure::AdoOrganization::parse("other-org").unwrap()), + checkout: true, + fetch_depth: None, + fetch_tags: None, + }, + RepoEntry { + name: "Other Project/docs".to_string(), + alias: Some("docs".to_string()), + repo_type: "git".to_string(), + repo_ref: "refs/heads/main".to_string(), + endpoint: None, + organization: Some(crate::secure::AdoOrganization::parse("other-org").unwrap()), + checkout: true, + fetch_depth: None, + fetch_tags: None, + }, + RepoEntry { + name: "not-a-project-repository-pair".to_string(), + alias: Some("docs".to_string()), + repo_type: "git".to_string(), + repo_ref: "refs/heads/main".to_string(), + endpoint: Some("checkout".to_string()), + organization: Some(crate::secure::AdoOrganization::parse("other-org").unwrap()), + checkout: true, + fetch_depth: None, + fetch_tags: None, + }, + ] { + assert!(lower_repos(&[ReposItem::Full(item)]).is_err()); + } + } + #[test] fn test_repos_object_form_no_checkout() { let items = vec![ReposItem::Full(RepoEntry { @@ -8312,6 +8439,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: false, fetch_depth: None, fetch_tags: None, @@ -8330,6 +8458,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/release/2.x".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: None, fetch_tags: None, @@ -8348,6 +8477,7 @@ safe-outputs: repo_type: "github".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: None, fetch_tags: None, @@ -8365,6 +8495,7 @@ safe-outputs: repo_type: "githubenterprise".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: Some("shared-conn".to_string()), + organization: None, checkout: true, fetch_depth: None, fetch_tags: None, @@ -8429,6 +8560,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: None, fetch_tags: None, @@ -8446,6 +8578,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: None, fetch_tags: None, @@ -8470,6 +8603,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: None, fetch_tags: None, @@ -8488,6 +8622,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: None, fetch_tags: None, @@ -8506,6 +8641,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: false, fetch_depth: None, fetch_tags: None, @@ -8526,6 +8662,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: Some(1), fetch_tags: Some(false), @@ -8552,6 +8689,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: Some(0), fetch_tags: Some(false), @@ -8576,6 +8714,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: Some(1), fetch_tags: None, @@ -8586,6 +8725,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: true, fetch_depth: None, fetch_tags: Some(true), @@ -8605,6 +8745,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/feature".to_string(), endpoint: None, + organization: None, checkout: false, fetch_depth: Some(1), fetch_tags: None, @@ -8642,6 +8783,7 @@ safe-outputs: repo_type: "git".to_string(), repo_ref: "refs/heads/main".to_string(), endpoint: None, + organization: None, checkout: false, fetch_depth: Some(1), fetch_tags: Some(false), diff --git a/src/compile/types.rs b/src/compile/types.rs index 00b81388..8d6e55d0 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -3059,11 +3059,109 @@ pub struct PermissionsConfig { /// The raw token is not injected into the Agent process. #[serde(default)] pub read: Option, - /// ARM service connection for write ADO access. + /// Optional service connection for write ADO access. + /// + /// The scalar form is an Azure Resource Manager connection. The expanded + /// form selects AzureCLI@3's `azureRM` or `azureDevOps` connection type and + /// may authorize additional organization/project/repository targets. /// Token is minted and used only by the executor in Stage 3 (Execution). /// This token is never exposed to the agent. #[serde(default)] - pub write: Option, + pub write: Option, +} + +/// Service-connection type used to acquire an Azure DevOps bearer. +/// +/// The serialized values deliberately match AzureCLI@3's `connectionType` +/// input so front matter and generated YAML use one vocabulary. +#[derive(Debug, Deserialize, Clone, Copy, Default, PartialEq, Eq)] +pub enum WriteConnectionType { + #[default] + #[serde(rename = "azureRM")] + AzureRm, + #[serde(rename = "azureDevOps")] + AzureDevOps, +} + +/// Stage 3 Azure DevOps credential and additional write-scope policy. +/// +/// The scalar form remains shorthand for an Azure Resource Manager service +/// connection. The expanded form selects an AzureCLI@3 connection type and +/// declares additional organization/project/repository targets. +#[derive(Debug, Deserialize, Clone, PartialEq)] +#[serde(untagged)] +pub enum WritePermissionConfig { + ServiceConnection(crate::secure::ServiceConnection), + WithOptions(WritePermissionOptions), +} + +impl WritePermissionConfig { + pub fn service_connection(&self) -> &str { + match self { + Self::ServiceConnection(value) => value.as_str(), + Self::WithOptions(options) => options.service_connection.as_str(), + } + } + + pub fn connection_type(&self) -> WriteConnectionType { + match self { + Self::ServiceConnection(_) => WriteConnectionType::AzureRm, + Self::WithOptions(options) => options.connection_type, + } + } + + pub fn options(&self) -> Option<&WritePermissionOptions> { + match self { + Self::ServiceConnection(_) => None, + Self::WithOptions(options) => Some(options), + } + } + + pub fn supports_cross_organization_writes(&self) -> bool { + self.connection_type() == WriteConnectionType::AzureDevOps + } + + pub fn allows_repository(&self, organization: &str, project: &str, repository: &str) -> bool { + self.options().is_some_and(|options| { + options.allow.iter().any(|organization_scope| { + organization_scope + .organization + .as_str() + .eq_ignore_ascii_case(organization) + && organization_scope.projects.iter().any(|project_scope| { + project_scope.project.as_str().eq_ignore_ascii_case(project) + && project_scope + .repositories + .iter() + .any(|allowed| allowed.as_str().eq_ignore_ascii_case(repository)) + }) + }) + }) + } +} + +impl SanitizeConfigTrait for WritePermissionConfig { + fn sanitize_config_fields(&mut self) { + // Every string field is a validated newtype checked at deserialization. + } +} + +/// Expanded Stage 3 write credential and deny-by-default additional scopes. +#[derive(Debug, Deserialize, Clone, PartialEq)] +#[serde(deny_unknown_fields)] +pub struct WritePermissionOptions { + #[serde(rename = "service-connection")] + pub service_connection: crate::secure::ServiceConnection, + #[serde(rename = "connection-type")] + pub connection_type: WriteConnectionType, + #[serde(default)] + pub allow: Vec, +} + +impl WritePermissionOptions { + pub fn validate(&self) -> anyhow::Result<()> { + validate_ado_scope_tree(&self.allow, "permissions.write.allow", true) + } } /// Stage 1 Azure DevOps credential and policy configuration. @@ -3132,17 +3230,7 @@ impl ReadPermissionOptions { /// project-scoped reads without any repository-scoped read, so it narrows /// rather than widens. pub fn validate(&self) -> anyhow::Result<()> { - for scope in &self.allow { - if scope.projects.is_empty() { - anyhow::bail!( - "permissions.read.allow entry for organization '{}' lists no projects. \ - Name the projects to allow; an empty list would grant every project in \ - the organization.", - scope.organization.as_str() - ); - } - } - Ok(()) + validate_ado_scope_tree(&self.allow, "permissions.read.allow", false) } } @@ -3199,19 +3287,19 @@ impl AdoReadCapability { /// Explicit Azure DevOps organization scope. #[derive(Debug, Deserialize, Clone, PartialEq)] #[serde(deny_unknown_fields)] -pub struct AdoReadOrganizationScope { +pub struct AdoOrganizationScope { pub organization: crate::secure::AdoOrganization, /// Projects to allow within this organization. /// /// Required and non-empty — see [`ReadPermissionOptions::validate`]. #[serde(default)] - pub projects: Vec, + pub projects: Vec, } /// Explicit project and optional repository scope within an organization. #[derive(Debug, Deserialize, Clone, PartialEq)] #[serde(deny_unknown_fields)] -pub struct AdoReadProjectScope { +pub struct AdoProjectScope { pub project: crate::secure::AdoProject, /// Optional Azure DevOps project GUID. /// @@ -3231,6 +3319,57 @@ pub struct AdoReadProjectScope { pub repositories: Vec, } +pub type AdoReadOrganizationScope = AdoOrganizationScope; +fn validate_ado_scope_tree( + scopes: &[AdoOrganizationScope], + label: &str, + require_repositories: bool, +) -> anyhow::Result<()> { + let mut organizations = std::collections::HashSet::new(); + for organization_scope in scopes { + let organization = organization_scope.organization.as_str(); + if !organizations.insert(organization.to_ascii_lowercase()) { + anyhow::bail!("{label} contains duplicate organization '{organization}'"); + } + if organization_scope.projects.is_empty() { + anyhow::bail!( + "{label} entry for organization '{organization}' lists no projects. \ + Name the projects to allow; an empty list would grant every project in \ + the organization." + ); + } + + let mut projects = std::collections::HashSet::new(); + for project_scope in &organization_scope.projects { + let project = project_scope.project.as_str(); + if !projects.insert(project.to_ascii_lowercase()) { + anyhow::bail!( + "{label} entry for organization '{organization}' contains duplicate \ + project '{project}'" + ); + } + if require_repositories && project_scope.repositories.is_empty() { + anyhow::bail!( + "{label} entry for '{organization}/{project}' lists no repositories. \ + Cross-organization write scopes must name every repository explicitly." + ); + } + + let mut repositories = std::collections::HashSet::new(); + for repository in &project_scope.repositories { + if !repositories.insert(repository.as_str().to_ascii_lowercase()) { + anyhow::bail!( + "{label} entry for '{organization}/{project}' contains duplicate \ + repository '{}'", + repository.as_str() + ); + } + } + } + } + Ok(()) +} + /// Debug-only configuration block. /// /// Lives under the `ado-aw-debug:` top-level front-matter key. Holds knobs @@ -3464,6 +3603,8 @@ pub struct Repository { pub repo_ref: String, #[serde(default)] pub endpoint: Option, + #[serde(default)] + pub organization: Option, } fn default_ref() -> String { @@ -3492,9 +3633,15 @@ pub struct RepoEntry { /// Branch/tag ref. Defaults to `"refs/heads/main"`. #[serde(default = "default_ref", rename = "ref")] pub repo_ref: String, - /// Service connection name for GitHub/GitHub Enterprise repository resources. + /// Service connection name. Required for external repository providers and + /// for Azure Repos Git repositories in another organization. #[serde(default)] pub endpoint: Option, + /// Azure DevOps Services organization containing a cross-organization + /// `type: git` repository. Omitted for repositories in the pipeline's own + /// organization and for non-Azure-Repos providers. + #[serde(default)] + pub organization: Option, /// Whether the agent job checks out this repository. Defaults to `true`. #[serde(default = "default_checkout")] pub checkout: bool, @@ -4759,7 +4906,9 @@ imports: read: Some(ReadPermissionConfig::ServiceConnection( crate::secure::ServiceConnection::parse("read").unwrap(), )), - write: Some("write".to_string()), + write: Some(WritePermissionConfig::ServiceConnection( + crate::secure::ServiceConnection::parse("write").unwrap(), + )), })) .is_ok() ); @@ -6010,7 +6159,18 @@ github-app-token: .map(ReadPermissionConfig::service_connection), Some("my-read-sc") ); - assert_eq!(pc.write.as_deref(), Some("my-write-sc")); + assert_eq!( + pc.write + .as_ref() + .map(WritePermissionConfig::service_connection), + Some("my-write-sc") + ); + assert_eq!( + pc.write + .as_ref() + .map(WritePermissionConfig::connection_type), + Some(WriteConnectionType::AzureRm) + ); } #[test] @@ -6182,7 +6342,90 @@ read: let yaml = "write: my-write-sc"; let pc: PermissionsConfig = serde_yaml::from_str(yaml).unwrap(); assert!(pc.read.is_none()); - assert_eq!(pc.write.as_deref(), Some("my-write-sc")); + assert_eq!( + pc.write + .as_ref() + .map(WritePermissionConfig::service_connection), + Some("my-write-sc") + ); + } + + #[test] + fn test_permissions_write_object_form() { + let yaml = r#" +write: + service-connection: repository-writer + connection-type: azureDevOps + allow: + - organization: other-org + projects: + - project: Other Project + repositories: [Repo One, Repo Two] +"#; + let permissions: PermissionsConfig = serde_yaml::from_str(yaml).unwrap(); + let write = permissions.write.as_ref().unwrap(); + + assert_eq!(write.service_connection(), "repository-writer"); + assert_eq!(write.connection_type(), WriteConnectionType::AzureDevOps); + assert!(write.supports_cross_organization_writes()); + assert!(write.allows_repository("OTHER-ORG", "other project", "repo one")); + assert!(!write.allows_repository("other-org", "Other Project", "Repo Three")); + write.options().unwrap().validate().unwrap(); + } + + #[test] + fn test_permissions_write_object_rejects_unknown_connection_type() { + let yaml = r#" +write: + service-connection: repository-writer + connection-type: workload-identity +"#; + + assert!(serde_yaml::from_str::(yaml).is_err()); + } + + #[test] + fn write_policy_requires_explicit_repository_scopes() { + for yaml in [ + "write:\n service-connection: sc\n connection-type: azureDevOps\n allow:\n - organization: other-org", + "write:\n service-connection: sc\n connection-type: azureDevOps\n allow:\n - organization: other-org\n projects:\n - project: Other Project", + ] { + let permissions: PermissionsConfig = serde_yaml::from_str(yaml).unwrap(); + let error = permissions + .write + .as_ref() + .unwrap() + .options() + .unwrap() + .validate() + .unwrap_err() + .to_string(); + assert!( + error.contains("lists no projects") || error.contains("lists no repositories"), + "{error}" + ); + } + } + + #[test] + fn write_policy_rejects_case_insensitive_duplicates() { + for yaml in [ + "write:\n service-connection: sc\n connection-type: azureDevOps\n allow:\n - organization: other-org\n projects:\n - project: P\n repositories: [R]\n - organization: OTHER-ORG\n projects:\n - project: Q\n repositories: [S]", + "write:\n service-connection: sc\n connection-type: azureDevOps\n allow:\n - organization: other-org\n projects:\n - project: P\n repositories: [R]\n - project: p\n repositories: [S]", + "write:\n service-connection: sc\n connection-type: azureDevOps\n allow:\n - organization: other-org\n projects:\n - project: P\n repositories: [Repo, REPO]", + ] { + let permissions: PermissionsConfig = serde_yaml::from_str(yaml).unwrap(); + let error = permissions + .write + .as_ref() + .unwrap() + .options() + .unwrap() + .validate() + .unwrap_err() + .to_string(); + assert!(error.contains("duplicate"), "{error}"); + } } #[test] @@ -6217,7 +6460,13 @@ Body .map(ReadPermissionConfig::service_connection), Some("my-read-sc") ); - assert_eq!(perms.write.as_deref(), Some("my-write-sc")); + assert_eq!( + perms + .write + .as_ref() + .map(WritePermissionConfig::service_connection), + Some("my-write-sc") + ); } #[test] From 23954ace384a183e2d12ae5b6514a937398f1d16 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 09:51:24 +0100 Subject: [PATCH 02/18] chore(compile): preserve agentic pipeline formatting Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/compile/agentic_pipeline.rs | 63 ++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 41079f69..4e63dc24 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -2276,7 +2276,9 @@ fn prepare_custom_agent_output_step(config_path: &str, output_path: &str) -> Bas fn agent_temp_filename(path: &str) -> String { let prefix = "$(Agent.TempDirectory)/"; path.strip_prefix(prefix) - .unwrap_or_else(|| panic!("custom-tools config path {path:?} must start with {prefix:?}")) + .unwrap_or_else(|| panic!( + "custom-tools config path {path:?} must start with {prefix:?}" + )) .to_string() } @@ -3095,15 +3097,20 @@ fn build_conclusion_job( // defaults (type: Task, no area/iteration path). The global // report-failure-as-work-item toggle controls whether it files at all. for tool_key in &["noop", "missing-tool", "missing-data"] { - conclusion_step = apply_conclusion_tool_config_env(conclusion_step, front_matter, tool_key); + conclusion_step = + apply_conclusion_tool_config_env(conclusion_step, front_matter, tool_key); } // Pass upstream job results via job-level variables hoist. // ADO only evaluates $[...] runtime expressions inside `variables:` and // `condition:` — NOT in step env blocks. We hoist to job variables and // reference them as $(name) macros in the step env. - let (conclusion_variables, conclusion_step) = - hoist_conclusion_job_results(conclusion_step, prefix, custom_defs, has_reviewed_job)?; + let (conclusion_variables, conclusion_step) = hoist_conclusion_job_results( + conclusion_step, + prefix, + custom_defs, + has_reviewed_job, + )?; steps.push(Step::Bash(conclusion_step)); @@ -3820,7 +3827,8 @@ fn prepare_mcpg_config_step( {mcpg_sentinel}" ); let custom_tools_fragment = if let Some(custom_tools_json) = custom_tools_json { - let sentinel = super::common::heredoc_sentinel("CUSTOM_TOOLS_JSON_EOF", custom_tools_json)?; + let sentinel = + super::common::heredoc_sentinel("CUSTOM_TOOLS_JSON_EOF", custom_tools_json)?; format!( "# Write compiler-generated dynamic SafeOutputs tool definitions\n\ cat > \"$AGENT_TEMP/staging/custom-tools.json\" << '{sentinel}'\n\ @@ -4567,7 +4575,10 @@ fn execute_safe_outputs_step( // no part of it needs separate lowering. EnvValue::literal(self_repository_directory), ); - script = script.with_env("ADO_AW_SELF_REPOSITORY_NAME", self_repository_name.clone()); + script = script.with_env( + "ADO_AW_SELF_REPOSITORY_NAME", + self_repository_name.clone(), + ); Ok(script) } @@ -4894,14 +4905,23 @@ fn start_ado_proxy_step(front_matter: &FrontMatter) -> BashStep { Binding::text(ado_proxy_container_entrypoint_flattened()), ) .fragment("resolve_org", common::resolve_ado_organization_bash()) - .fragment("setup_workdir", phase_body(&START_ADO_PROXY_SETUP_WORKDIR)) + .fragment( + "setup_workdir", + phase_body(&START_ADO_PROXY_SETUP_WORKDIR), + ) .fragment("write_policy", phase_body(&START_ADO_PROXY_WRITE_POLICY)) - .fragment("mint_material", phase_body(&START_ADO_PROXY_MINT_MATERIAL)) + .fragment( + "mint_material", + phase_body(&START_ADO_PROXY_MINT_MATERIAL), + ) .fragment( "build_material", phase_body(&START_ADO_PROXY_BUILD_MATERIAL), ) - .fragment("run_container", phase_body(&START_ADO_PROXY_RUN_CONTAINER)) + .fragment( + "run_container", + phase_body(&START_ADO_PROXY_RUN_CONTAINER), + ) .fragment( "handover_material", phase_body(&START_ADO_PROXY_HANDOVER_MATERIAL), @@ -6179,10 +6199,7 @@ fn verify_mcp_backends_step() -> BashStep { ShellScript::new(&VERIFY_MCP_BACKENDS) .bind("MCPG_PORT", Binding::number(MCPG_PORT.into())) .into_step("Verify MCP backends") - .with_env( - "MCPG_API_KEY", - EnvValue::pipeline_var("MCP_GATEWAY_API_KEY"), - ) + .with_env("MCPG_API_KEY", EnvValue::pipeline_var("MCP_GATEWAY_API_KEY")) } // ───────────────────────────────────────────────────────────────────── @@ -7283,9 +7300,9 @@ safe-outputs: step.script ); assert!( - step.script - .contains("printf '%s' \"$PROXY_MATERIAL\" | docker exec -i \"$PROXY_CONTAINER\"") - && step.script.contains("cat > /tmp/ado-proxy-material"), + step.script.contains( + "printf '%s' \"$PROXY_MATERIAL\" | docker exec -i \"$PROXY_CONTAINER\"" + ) && step.script.contains("cat > /tmp/ado-proxy-material"), "material must stream through the container-private FIFO: {}", step.script ); @@ -7344,7 +7361,8 @@ safe-outputs: let copy = copy_logs_step("/tmp/copilot", false); assert!(copy.script.contains("/tmp/gh-aw/ado-proxy-logs")); assert!( - copy.script.contains("AGENT_TEMP='$(Agent.TempDirectory)'") + copy.script + .contains("AGENT_TEMP='$(Agent.TempDirectory)'") && copy .script .contains(r#""$AGENT_TEMP/staging/logs/ado-proxy""#), @@ -7381,8 +7399,9 @@ safe-outputs: ); assert!( script.contains(&format!("CA_HOST_PATH='{ADO_PROXY_PUBLIC_CA_HOST_PATH}'")) - && script - .contains("##vso[task.setvariable variable=ADO_PROXY_CA_FILE]$CA_HOST_PATH"), + && script.contains( + "##vso[task.setvariable variable=ADO_PROXY_CA_FILE]$CA_HOST_PATH" + ), "clients need the published certificate's path: {script}" ); assert!( @@ -7435,8 +7454,10 @@ safe-outputs: "docker run must reuse the bound $PROXY_IMAGE: {script}" ); assert!( - script.contains(&format!("PROXY_SCRIPT_PATH='{}'", paths::ADO_PROXY_PATH)) - && script.contains("\"$PROXY_SCRIPT_PATH:/app/ado-proxy.js:ro\""), + script.contains(&format!( + "PROXY_SCRIPT_PATH='{}'", + paths::ADO_PROXY_PATH + )) && script.contains("\"$PROXY_SCRIPT_PATH:/app/ado-proxy.js:ro\""), "docker run must mount the bound ado-proxy bundle: {script}" ); } From 286ef7f7af4d34c7f926be290ad8c5e296da9bcc Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 10:01:00 +0100 Subject: [PATCH 03/18] refactor(compile): move ADO token acquisition to AzureCLI@3 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/compile/ado_bundle.rs | 4 +- src/compile/agentic_pipeline.rs | 46 +++++--- src/compile/common.rs | 179 +++++++++++++++++++++--------- src/compile/ir/tasks/azure_cli.rs | 140 +++++++++++++++++++++++ src/safe_outputs/result.rs | 8 +- tests/compiler_tests.rs | 131 ++++++++++++++++------ 6 files changed, 398 insertions(+), 110 deletions(-) diff --git a/src/compile/ado_bundle.rs b/src/compile/ado_bundle.rs index 7717b287..831c55bc 100644 --- a/src/compile/ado_bundle.rs +++ b/src/compile/ado_bundle.rs @@ -98,8 +98,8 @@ pub enum TokenSource { /// The pipeline's built-in OAuth token (`$(System.AccessToken)`), scoped /// by the pipeline's job-authorization settings. SystemAccessToken, - /// A write-capable ADO token minted from an ARM service connection into - /// the `SC_WRITE_TOKEN` pipeline variable (Stage 3 executor / Conclusion). + /// A write-capable ADO token minted from a configured AzureCLI@3 service + /// connection into `SC_WRITE_TOKEN` (Stage 3 executor / Conclusion). WriteServiceConnection, } diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 4e63dc24..18deca37 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -490,20 +490,24 @@ pub(crate) fn build_pipeline_context( let pipeline_path = common::generate_pipeline_path(output_path); // Read / write tokens - let acquire_read_token = common::generate_acquire_ado_token( + let acquire_read_token = common::acquire_ado_token_step( front_matter .permissions .as_ref() .and_then(|p| p.read.as_ref()) .map(crate::compile::types::ReadPermissionConfig::service_connection), + crate::compile::types::WriteConnectionType::AzureRm, "SC_READ_TOKEN", ); - let acquire_write_token = common::generate_acquire_ado_token( - front_matter - .permissions - .as_ref() - .and_then(|p| p.write.as_ref()) - .map(crate::compile::types::WritePermissionConfig::service_connection), + let write_permission = front_matter + .permissions + .as_ref() + .and_then(|p| p.write.as_ref()); + let acquire_write_token = common::acquire_ado_token_step( + write_permission.map(crate::compile::types::WritePermissionConfig::service_connection), + write_permission + .map(crate::compile::types::WritePermissionConfig::connection_type) + .unwrap_or_default(), "SC_WRITE_TOKEN", ); // Skip integrity check resolution @@ -835,9 +839,9 @@ pub(crate) struct StandaloneCtx { /// SafeOutputs variants combine this with their job-local checkout layout. pub(crate) source_relative_path: String, pub(crate) pipeline_path: String, - /// `AzureCLI@2` task YAML body (or empty when no read service connection). - pub(crate) acquire_read_token: String, - pub(crate) acquire_write_token: String, + /// Typed AzureCLI@3 token-acquisition steps, when configured. + pub(crate) acquire_read_token: Option, + pub(crate) acquire_write_token: Option, /// `Verify pipeline integrity` step YAML (or empty when skipped). pub(crate) integrity_check_yaml: String, /// Agent prompt body (either inlined imports or @@ -1189,8 +1193,10 @@ fn build_agent_job( })); } - // 3. acquire ADO read token (AzureCLI@2 task) — only when configured. - push_raw_yaml_if_nonempty(&mut steps, &cfg.acquire_read_token)?; + // 3. acquire ADO read token (AzureCLI@3 task) — only when configured. + if let Some(step) = &cfg.acquire_read_token { + steps.push(step.clone()); + } // 4. engine install steps (Copilot CLI install). YAML string from // `Engine::install_steps`; lowered through `Step::RawYaml` @@ -2443,7 +2449,9 @@ fn build_safeoutputs_job( } } // Acquire write token (when configured) - push_raw_yaml_if_nonempty(&mut steps, &cfg.acquire_write_token)?; + if let Some(step) = &cfg.acquire_write_token { + steps.push(step.clone()); + } // Download analyzed outputs steps.push(Step::Download(DownloadStep { source: "current".to_string(), @@ -3023,7 +3031,9 @@ fn build_conclusion_job( // Azure Pipelines task.setvariable variables are job-scoped and NOT propagated // to downstream jobs without isOutput=true + dependsOn mapping. The SafeOutputs // job mints its own SC_WRITE_TOKEN copy; Conclusion must do the same. - push_raw_yaml_if_nonempty(&mut steps, &cfg.acquire_write_token)?; + if let Some(step) = &cfg.acquire_write_token { + steps.push(step.clone()); + } let mut download_artifact = TaskStep::new( "DownloadPipelineArtifact@2", @@ -6524,8 +6534,8 @@ mod tests { source_path: "$(Build.SourcesDirectory)/agents/test.md".to_string(), source_relative_path: "agents/test.md".to_string(), pipeline_path: "$(Build.SourcesDirectory)/agents/test.lock.yml".to_string(), - acquire_read_token: String::new(), - acquire_write_token: String::new(), + acquire_read_token: None, + acquire_write_token: None, integrity_check_yaml: String::new(), agent_content_value: "Test prompt".to_string(), debug_pipeline: false, @@ -7652,8 +7662,8 @@ safe-outputs: source_path: "source.md".to_string(), source_relative_path: "source.md".to_string(), pipeline_path: "source.lock.yml".to_string(), - acquire_read_token: String::new(), - acquire_write_token: String::new(), + acquire_read_token: None, + acquire_write_token: None, integrity_check_yaml: String::new(), agent_content_value: String::new(), debug_pipeline: false, diff --git a/src/compile/common.rs b/src/compile/common.rs index e3dc5d77..ee077144 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2537,44 +2537,89 @@ fn find_git_root(path: &std::path::Path) -> Option { /// ADO resource ID for minting ADO-scoped tokens via Azure CLI. const ADO_RESOURCE_ID: &str = "499b84ac-1321-427f-aa17-267ca6975798"; -/// Generate an AzureCLI@2 step to acquire an ADO-scoped token from an ARM service connection. -/// The `variable_name` parameter controls which pipeline variable the token is stored in -/// (e.g. "SC_READ_TOKEN" for the agent, "SC_WRITE_TOKEN" for the executor). -/// Returns empty string if no service connection is provided. -pub fn generate_acquire_ado_token(service_connection: Option<&str>, variable_name: &str) -> String { - match service_connection { - Some(sc) => { - let mut lines = Vec::new(); - lines.push("- task: AzureCLI@2".to_string()); - lines.push(format!( - r#" displayName: "Acquire ADO token ({variable_name})""# - )); - lines.push(" inputs:".to_string()); - lines.push(format!( - " azureSubscription: '{}'", - sc.replace('\'', "''") - )); - lines.push(" scriptType: 'bash'".to_string()); - lines.push(" scriptLocation: 'inlineScript'".to_string()); - lines.push(" addSpnToEnvironment: true".to_string()); - lines.push(" inlineScript: |".to_string()); - lines.push(" ADO_TOKEN=$(az account get-access-token \\".to_string()); - lines.push(format!(" --resource {} \\", ADO_RESOURCE_ID)); - lines.push(" --query accessToken -o tsv)".to_string()); - lines.push(format!( - " echo \"##vso[task.setvariable variable={variable_name};issecret=true]$ADO_TOKEN\"" - )); - // Trailing newline ensures the inlineScript block scalar value - // preserves its terminating newline through round-trip parse/emit; - // without it serde_yaml strips the newline and switches to the - // `|-` chomping indicator (semantically identical, but produces - // a textual diff against the committed lock files). - format!("{}\n", lines.join("\n")) - } - None => String::new(), +shell_script! { + /// Mint the Stage 1 Azure DevOps bearer inside an authenticated AzureCLI@3 + /// task and publish it as a masked, same-job pipeline variable. + ACQUIRE_ADO_READ_TOKEN { + interpreter: Bash, + bindings: [ADO_RESOURCE], + externals: [], + fragments: [], + body: r#" +set -eo pipefail +ADO_TOKEN=$(az account get-access-token \ + --resource "$ADO_RESOURCE" \ + --query accessToken -o tsv) +if [ -z "$ADO_TOKEN" ]; then + echo "Azure CLI returned an empty Azure DevOps access token" >&2 + exit 1 +fi +printf '##vso[task.setvariable variable=SC_READ_TOKEN;issecret=true]%s\n' "$ADO_TOKEN" +"#, } } +shell_script! { + /// Mint the Stage 3 Azure DevOps bearer inside an authenticated AzureCLI@3 + /// task and publish it as a masked, same-job pipeline variable. + ACQUIRE_ADO_WRITE_TOKEN { + interpreter: Bash, + bindings: [ADO_RESOURCE], + externals: [], + fragments: [], + body: r#" +set -eo pipefail +ADO_TOKEN=$(az account get-access-token \ + --resource "$ADO_RESOURCE" \ + --query accessToken -o tsv) +if [ -z "$ADO_TOKEN" ]; then + echo "Azure CLI returned an empty Azure DevOps access token" >&2 + exit 1 +fi +printf '##vso[task.setvariable variable=SC_WRITE_TOKEN;issecret=true]%s\n' "$ADO_TOKEN" +"#, + } +} + +/// Generate a typed AzureCLI@3 step that mints an ADO-scoped token. +pub fn acquire_ado_token_step( + service_connection: Option<&str>, + connection_type: crate::compile::types::WriteConnectionType, + variable_name: &str, +) -> Option { + let service_connection = service_connection?; + let script_def = match variable_name { + "SC_READ_TOKEN" => &ACQUIRE_ADO_READ_TOKEN, + "SC_WRITE_TOKEN" => &ACQUIRE_ADO_WRITE_TOKEN, + _ => panic!("unsupported compiler-owned ADO token variable {variable_name:?}"), + }; + let connection = match connection_type { + crate::compile::types::WriteConnectionType::AzureRm => { + crate::compile::ir::tasks::azure_cli::AzureCliV3Connection::AzureRm( + service_connection.to_string(), + ) + } + crate::compile::types::WriteConnectionType::AzureDevOps => { + crate::compile::ir::tasks::azure_cli::AzureCliV3Connection::AzureDevOps( + service_connection.to_string(), + ) + } + }; + let script = ShellScript::new(script_def) + .bind_text("ADO_RESOURCE", ADO_RESOURCE_ID) + .render(); + Some(crate::compile::ir::step::Step::Task( + crate::compile::ir::tasks::azure_cli::AzureCliV3::new( + connection, + crate::compile::ir::tasks::azure_cli::ScriptType::Bash, + crate::compile::ir::tasks::azure_cli::ScriptLocation::Inline(script), + ) + .visible_az_login(false) + .with_display_name(format!("Acquire ADO token ({variable_name})")) + .into_step(), + )) +} + /// Generate the env block entries for the executor step (Stage 3 Execution). /// /// Always emits a non-empty `env:` block containing at minimum @@ -2584,7 +2629,8 @@ pub fn generate_acquire_ado_token(service_connection: Option<&str>, variable_nam /// /// Sources: /// * `SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)` when `write_service_connection` -/// is `Some` — write-capable ADO token minted via an ARM service connection. +/// is `Some` — write-capable ADO token minted via the configured AzureCLI@3 +/// service connection. /// Use this for cross-org / cross-project writes or when you need /// named-identity attribution instead of the default /// `Project Collection Build Service` identity. @@ -6241,36 +6287,61 @@ safe-outputs: #[test] fn test_generate_acquire_ado_token_with_sc() { - let result = generate_acquire_ado_token(Some("my-arm-sc"), "SC_READ_TOKEN"); - assert!(result.contains("AzureCLI@2"), "Should use AzureCLI@2 task"); - assert!( - result.contains("azureSubscription: 'my-arm-sc'"), - "Should embed service connection name" - ); - assert!( - result.contains("variable=SC_READ_TOKEN;issecret=true"), - "Should set correct pipeline variable as secret" + let Some(crate::compile::ir::step::Step::Task(task)) = acquire_ado_token_step( + Some("my-arm-sc"), + crate::compile::types::WriteConnectionType::AzureRm, + "SC_READ_TOKEN", + ) else { + panic!("expected token task"); + }; + assert_eq!(task.task, "AzureCLI@3"); + assert_eq!( + task.inputs.get("connectionType").map(String::as_str), + Some("azureRM") ); - assert!( - result.contains("az account get-access-token"), - "Should call az CLI to get access token" + assert_eq!( + task.inputs.get("azureSubscription").map(String::as_str), + Some("my-arm-sc") ); + let script = task.inputs.get("inlineScript").unwrap(); + assert!(script.contains("SC_READ_TOKEN")); + assert!(script.contains("az account get-access-token")); } #[test] fn test_generate_acquire_ado_token_none_returns_empty() { - let result = generate_acquire_ado_token(None, "SC_READ_TOKEN"); assert!( - result.is_empty(), - "None service connection should return empty string" + acquire_ado_token_step( + None, + crate::compile::types::WriteConnectionType::AzureRm, + "SC_READ_TOKEN" + ) + .is_none() ); } #[test] fn test_generate_acquire_ado_token_write_token_variable() { - let result = generate_acquire_ado_token(Some("write-sc"), "SC_WRITE_TOKEN"); - assert!(result.contains("variable=SC_WRITE_TOKEN;issecret=true")); - assert!(!result.contains("SC_READ_TOKEN")); + let Some(crate::compile::ir::step::Step::Task(task)) = acquire_ado_token_step( + Some("write-sc"), + crate::compile::types::WriteConnectionType::AzureDevOps, + "SC_WRITE_TOKEN", + ) else { + panic!("expected token task"); + }; + assert_eq!( + task.inputs.get("connectionType").map(String::as_str), + Some("azureDevOps") + ); + assert_eq!( + task.inputs + .get("azureDevOpsServiceConnection") + .map(String::as_str), + Some("write-sc") + ); + let script = task.inputs.get("inlineScript").unwrap(); + assert!(script.contains("SC_WRITE_TOKEN")); + assert!(!script.contains("SC_READ_TOKEN")); } // ─── engine env / generate_executor_ado_env ──────────────────────────── diff --git a/src/compile/ir/tasks/azure_cli.rs b/src/compile/ir/tasks/azure_cli.rs index f32305da..bf6db18a 100644 --- a/src/compile/ir/tasks/azure_cli.rs +++ b/src/compile/ir/tasks/azure_cli.rs @@ -207,6 +207,97 @@ pub struct AzureCli { display_name: Option, } +/// Service-connection input for AzureCLI@3. +/// +/// Each variant emits only the input valid for its `connectionType`, making a +/// mixed `azureSubscription`/`azureDevOpsServiceConnection` task +/// unrepresentable. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AzureCliV3Connection { + AzureRm(String), + AzureDevOps(String), +} + +impl AzureCliV3Connection { + fn connection_type(&self) -> &'static str { + match self { + Self::AzureRm(_) => "azureRM", + Self::AzureDevOps(_) => "azureDevOps", + } + } + + fn apply(self, task: TaskStep) -> TaskStep { + match self { + Self::AzureRm(connection) => task.with_input("azureSubscription", connection), + Self::AzureDevOps(connection) => { + task.with_input("azureDevOpsServiceConnection", connection) + } + } + } +} + +/// Typed AzureCLI@3 builder used for Azure DevOps token acquisition. +#[derive(Debug, Clone)] +pub struct AzureCliV3 { + connection: AzureCliV3Connection, + script_type: ScriptType, + location: ScriptLocation, + visible_az_login: Option, + display_name: Option, +} + +impl AzureCliV3 { + pub fn new( + connection: AzureCliV3Connection, + script_type: ScriptType, + location: ScriptLocation, + ) -> Self { + Self { + connection, + script_type, + location, + visible_az_login: None, + display_name: None, + } + } + + pub fn visible_az_login(mut self, value: bool) -> Self { + self.visible_az_login = Some(value); + self + } + + pub fn with_display_name(mut self, value: impl Into) -> Self { + self.display_name = Some(value.into()); + self + } + + pub fn into_step(self) -> TaskStep { + let connection_type = self.connection.connection_type(); + let mut task = TaskStep::new( + "AzureCLI@3", + self.display_name.unwrap_or_else(|| "Azure CLI".into()), + ) + .with_input("connectionType", connection_type) + .with_input("scriptType", self.script_type.as_ado_str()); + task = self.connection.apply(task); + + match self.location { + ScriptLocation::Inline(script) => { + task = task + .with_input("scriptLocation", "inlineScript") + .with_input("inlineScript", script); + } + ScriptLocation::ScriptPath(path) => { + task = task + .with_input("scriptLocation", "scriptPath") + .with_input("scriptPath", path); + } + } + push_bool(&mut task, "visibleAzLogin", self.visible_az_login); + task + } +} + impl AzureCli { /// Create a new builder. /// @@ -392,6 +483,55 @@ mod tests { assert!(t.inputs.get("inlineScript").is_none()); } + #[test] + fn v3_azure_rm_uses_only_arm_connection_input() { + let task = AzureCliV3::new( + AzureCliV3Connection::AzureRm("arm-connection".to_string()), + ScriptType::Bash, + ScriptLocation::Inline("echo token\n".to_string()), + ) + .visible_az_login(false) + .into_step(); + + assert_eq!(task.task, "AzureCLI@3"); + assert_eq!( + task.inputs.get("connectionType").map(String::as_str), + Some("azureRM") + ); + assert_eq!( + task.inputs.get("azureSubscription").map(String::as_str), + Some("arm-connection") + ); + assert!(!task.inputs.contains_key("azureDevOpsServiceConnection")); + assert_eq!( + task.inputs.get("visibleAzLogin").map(String::as_str), + Some("false") + ); + } + + #[test] + fn v3_azure_devops_uses_only_devops_connection_input() { + let task = AzureCliV3::new( + AzureCliV3Connection::AzureDevOps("ado-connection".to_string()), + ScriptType::Bash, + ScriptLocation::Inline("echo token\n".to_string()), + ) + .into_step(); + + assert_eq!(task.task, "AzureCLI@3"); + assert_eq!( + task.inputs.get("connectionType").map(String::as_str), + Some("azureDevOps") + ); + assert_eq!( + task.inputs + .get("azureDevOpsServiceConnection") + .map(String::as_str), + Some("ado-connection") + ); + assert!(!task.inputs.contains_key("azureSubscription")); + } + #[test] fn optional_inputs_emit_only_when_set() { let t = AzureCli::new( diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index 14dee953..55cabec8 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -23,7 +23,7 @@ pub trait ToolResult: Serialize { /// ADO-backed tools receive a write-capable token via /// `SYSTEM_ACCESSTOKEN`: by default the pipeline's built-in /// `$(System.AccessToken)` (scoped by pipeline settings), or - /// `$(SC_WRITE_TOKEN)` minted from an ARM service connection when + /// `$(SC_WRITE_TOKEN)` minted from a configured service connection when /// `permissions.write` is configured. GitHub-backed tools use the separate /// Stage 3 GitHub credential. /// @@ -65,9 +65,7 @@ fn register_resolved_reference( value: T, lock_error: &'static str, ) -> anyhow::Result<()> { - let mut registry = registry - .lock() - .map_err(|_| anyhow::anyhow!(lock_error))?; + let mut registry = registry.lock().map_err(|_| anyhow::anyhow!(lock_error))?; if registry.contains_key(&id) { anyhow::bail!("temporary_id '{id}' was already used in this run"); } @@ -88,7 +86,7 @@ pub struct ExecutionContext { pub ado_project_id: Option, /// Write-capable ADO access token used by Stage 3 executors. Populated /// from the `SYSTEM_ACCESSTOKEN` env var, which the compiler maps to - /// `$(System.AccessToken)` by default or `$(SC_WRITE_TOKEN)` (ARM-minted) + /// `$(System.AccessToken)` by default or `$(SC_WRITE_TOKEN)` /// when `permissions.write` is configured. pub access_token: Option, /// GitHub credential used by GitHub safe outputs in Stage 3. diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 7f3c3f8e..0fff4b78 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -499,6 +499,12 @@ Do something. compiled.contains("my-read-sc"), "Compiled output should contain the read service connection name" ); + assert!( + compiled.contains("task: AzureCLI@3") + && compiled.contains("connectionType: azureRM") + && compiled.contains("azureSubscription: my-read-sc"), + "scalar permissions.read must use AzureCLI@3 azureRM inputs: {compiled}" + ); let document = parse_compiled_yaml(&compiled); assert_job_execution_env_excludes_ado_credentials( &document, @@ -516,6 +522,14 @@ Do something. compiled.contains("my-write-sc"), "Compiled output should contain the write service connection name" ); + assert!( + compiled.contains("azureSubscription: my-write-sc"), + "scalar permissions.write must retain Azure Resource Manager connection semantics" + ); + assert!( + !compiled.contains("addSpnToEnvironment"), + "ADO token acquisition must not expose service-principal material" + ); // Should NOT contain System.AccessToken in executor env assert!( @@ -587,7 +601,7 @@ Do something. "Compiled output should not contain SC_WRITE_TOKEN when permissions are omitted" ); assert!( - !compiled.contains("AzureCLI@2"), + !compiled.contains("Acquire ADO token"), "Compiled output should not contain AzureCLI task when permissions are omitted" ); @@ -604,6 +618,70 @@ Do something. let _ = fs::remove_dir_all(&temp_dir); } +#[test] +fn test_permissions_write_azure_devops_connection_compiled_output() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-permissions-ado-write-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + let test_input = temp_dir.join("perms-ado-agent.md"); + fs::write( + &test_input, + r#"--- +name: "Azure DevOps Connection Test" +description: "Expanded write connection" +permissions: + write: + service-connection: ado-write-sc + connection-type: azureDevOps + allow: + - organization: other-org + projects: + - project: Other Project + repositories: [target-repo] +safe-outputs: + create-work-item: + work-item-type: Task +--- + +Create a work item. +"#, + ) + .unwrap(); + let output_path = temp_dir.join("perms-ado-agent.yml"); + let output = std::process::Command::new(PathBuf::from(env!("CARGO_BIN_EXE_ado-aw"))) + .args([ + "compile", + test_input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let compiled = fs::read_to_string(&output_path).unwrap(); + assert!(compiled.contains("task: AzureCLI@3"), "{compiled}"); + assert!( + compiled.contains("connectionType: azureDevOps"), + "{compiled}" + ); + assert!( + compiled.contains("azureDevOpsServiceConnection: ado-write-sc"), + "{compiled}" + ); + assert!( + !compiled.contains("azureSubscription: ado-write-sc"), + "{compiled}" + ); + assert!(!compiled.contains("addSpnToEnvironment"), "{compiled}"); + let _ = fs::remove_dir_all(temp_dir); +} + /// Test that write-requiring safe-outputs compile successfully without an ARM write SC. /// Default behavior: the executor uses `$(System.AccessToken)`; ARM write SC is optional. #[test] @@ -4029,7 +4107,7 @@ fn test_conclusion_job_is_not_emitted_without_safe_outputs() { } /// When `permissions.write` is configured the Conclusion job must mint its own -/// `SC_WRITE_TOKEN` via an `AzureCLI@2` step. Azure Pipelines `task.setvariable` +/// `SC_WRITE_TOKEN` via an `AzureCLI@3` step. Azure Pipelines `task.setvariable` /// variables are job-scoped; the token minted in SafeOutputs is NOT available to /// the separate Conclusion job (issue #1688). #[test] @@ -4040,7 +4118,7 @@ fn test_conclusion_job_acquires_write_token_locally() { let conclusion_job = find_job_mapping(&doc, "Conclusion").expect("compiled YAML should contain Conclusion job"); - // The Conclusion job must contain an AzureCLI@2 step that mints SC_WRITE_TOKEN. + // The Conclusion job must contain an AzureCLI@3 step that mints SC_WRITE_TOKEN. let steps = conclusion_job .get(yaml_key("steps")) .and_then(|v| v.as_sequence()) @@ -4068,7 +4146,7 @@ fn test_conclusion_job_acquires_write_token_locally() { }); assert!( has_acquire_step, - "Conclusion job must contain an AzureCLI@2 step that mints SC_WRITE_TOKEN locally \ + "Conclusion job must contain an AzureCLI@3 step that mints SC_WRITE_TOKEN locally \ (job-scoped task.setvariable variables from SafeOutputs are not available here)" ); @@ -4088,7 +4166,7 @@ fn test_conclusion_job_acquires_write_token_locally() { } /// When no `permissions.write` is configured the Conclusion job must use the -/// built-in `$(System.AccessToken)` and must NOT emit an AzureCLI@2 token-mint +/// built-in `$(System.AccessToken)` and must NOT emit an AzureCLI@3 token-mint /// step (no-write-SC path regression guard). #[test] fn test_conclusion_job_no_write_sc_uses_system_access_token() { @@ -4098,7 +4176,7 @@ fn test_conclusion_job_no_write_sc_uses_system_access_token() { let conclusion_job = find_job_mapping(&doc, "Conclusion").expect("compiled YAML should contain Conclusion job"); - // No AzureCLI@2 step that mentions SC_WRITE_TOKEN. + // No AzureCLI@3 step that mentions SC_WRITE_TOKEN. let steps = conclusion_job .get(yaml_key("steps")) .and_then(|v| v.as_sequence()) @@ -5921,7 +5999,9 @@ fn test_byom_provider_env_compiles_and_merges() { "BYOK must bind the api-proxy image in both the Agent and Detection jobs: {compiled}" ); assert_eq!( - compiled.matches(r#"docker pull "$API_PROXY_IMAGE""#).count(), + compiled + .matches(r#"docker pull "$API_PROXY_IMAGE""#) + .count(), 2, "BYOK must pre-pull the api-proxy container image in both the Agent and Detection jobs: {compiled}" ); @@ -6010,7 +6090,9 @@ fn test_non_byom_agent_uses_always_on_api_proxy() { "Agent and Detection must bind the api-proxy image for pre-pull: {compiled}" ); assert_eq!( - compiled.matches(r#"docker pull "$API_PROXY_IMAGE""#).count(), + compiled + .matches(r#"docker pull "$API_PROXY_IMAGE""#) + .count(), 2, "Agent and Detection must pre-pull the always-on api-proxy image: {compiled}" ); @@ -9116,9 +9198,7 @@ fn assert_github_app_token_wiring(compiled: &str) { // (Agent + Detection) = 4. Both mint and revoke read the bundle path // through the bound `$GITHUB_APP_TOKEN_PATH`; only revoke follows it // with the literal `revoke` word. - let total_bundle = compiled - .matches("node \"$GITHUB_APP_TOKEN_PATH\"") - .count(); + let total_bundle = compiled.matches("node \"$GITHUB_APP_TOKEN_PATH\"").count(); let revoke_hits = compiled .matches("node \"$GITHUB_APP_TOKEN_PATH\" revoke") .count(); @@ -9243,9 +9323,7 @@ fn test_github_app_token_skip_revocation() { let compiled = compile_inline_agent("ghapp-norevoke", content); // Mint step still present in Agent + Detection. assert_eq!( - compiled - .matches("node \"$GITHUB_APP_TOKEN_PATH\"") - .count() + compiled.matches("node \"$GITHUB_APP_TOKEN_PATH\"").count() - compiled .matches("node \"$GITHUB_APP_TOKEN_PATH\" revoke") .count(), @@ -9672,9 +9750,8 @@ fn test_create_pull_request_emits_prepare_pr_base_step_in_safeoutputs() { ); let safeoutputs = job_block(&compiled, "SafeOutputs"); assert!( - safeoutputs.contains( - "PREPARE_PR_BASE_PATH='/tmp/ado-aw-scripts/ado-script/prepare-pr-base.js'" - ), + safeoutputs + .contains("PREPARE_PR_BASE_PATH='/tmp/ado-aw-scripts/ado-script/prepare-pr-base.js'"), "SafeOutputs job must project the bundle path through the prelude:\n{safeoutputs}" ); assert!( @@ -9682,9 +9759,7 @@ fn test_create_pull_request_emits_prepare_pr_base_step_in_safeoutputs() { "SafeOutputs job must project the target-worktree mode through the prelude:\n{safeoutputs}" ); assert!( - safeoutputs.contains( - "--repo-dir \"$(Build.SourcesDirectory)\" --target-branch 'main'" - ), + safeoutputs.contains("--repo-dir \"$(Build.SourcesDirectory)\" --target-branch 'main'"), "SafeOutputs job must invoke prepare-pr-base with the self dir/target pair:\n{safeoutputs}" ); assert!( @@ -9922,8 +9997,7 @@ fn test_issue_1731_safeoutputs_executor_source_path_uses_multi_checkout_layout() // `--source "$ADO_AW_SOURCE_PATH"`. assert!( safeoutputs.contains("ADO_AW_SOURCE_PATH: $(Build.SourcesDirectory)/self/") - && safeoutputs - .contains(r#"ado-aw execute --source "$ADO_AW_SOURCE_PATH""#), + && safeoutputs.contains(r#"ado-aw execute --source "$ADO_AW_SOURCE_PATH""#), "SafeOutputs executor --source must use the multi-checkout layout path:\n{safeoutputs}" ); assert!( @@ -10025,9 +10099,8 @@ fn test_issue_1731_split_approval_additional_checkouts_only_in_pr_variant() { ); assert!( reviewed.contains("ADO_AW_SOURCE_PATH: $(Build.SourcesDirectory)/self/") - && reviewed.contains( - "ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self" - ) + && reviewed + .contains("ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self") && reviewed.contains(r#"ado-aw execute --source "$ADO_AW_SOURCE_PATH""#), "PR-capable reviewed job must use its multi-checkout self path:\n{reviewed}" ); @@ -10060,9 +10133,7 @@ fn test_issue_1731_split_approval_additional_checkouts_in_auto_when_sibling_gate assert!( auto.contains("ADO_AW_SOURCE_PATH: $(Build.SourcesDirectory)/self/") && auto.contains(r#"ado-aw execute --source "$ADO_AW_SOURCE_PATH""#) - && auto.contains( - "ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self" - ), + && auto.contains("ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self"), "PR-capable automatic job must use its multi-checkout self path:\n{auto}" ); assert!( @@ -10107,9 +10178,7 @@ fn test_issue_1731_split_checkout_layout_compiles_for_every_target() { ); assert!( compiled.contains("ADO_AW_SOURCE_PATH: $(Build.SourcesDirectory)") - && compiled.contains( - "ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)" - ), + && compiled.contains("ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)"), "{target}: self-only Stage 3 sibling must use single-checkout layout:\n{compiled}" ); } From 5bfada3ee6992d999585fa4706ac1f736898427e Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 10:16:38 +0100 Subject: [PATCH 04/18] refactor(safe-outputs): resolve repository write targets centrally Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/compile/custom_tools.rs | 63 +++- src/compile/types.rs | 15 +- src/main.rs | 108 ++++--- src/safe_outputs/create_pull_request.rs | 19 +- src/safe_outputs/mod.rs | 317 ++++++++++++++++++++ src/safe_outputs/result.rs | 43 +++ src/safe_outputs/upload_build_attachment.rs | 3 + 7 files changed, 515 insertions(+), 53 deletions(-) diff --git a/src/compile/custom_tools.rs b/src/compile/custom_tools.rs index 73acb504..f6ca8a55 100644 --- a/src/compile/custom_tools.rs +++ b/src/compile/custom_tools.rs @@ -291,6 +291,7 @@ pub fn resolved_execution_config_json( "name": repository.name, "ref": repository.repo_ref, "endpoint": repository.endpoint, + "organization": repository.organization, }) }) .collect(); @@ -304,6 +305,19 @@ pub fn resolved_execution_config_json( "allowedExtensions": config.allowed_extensions(), }) }); + let write_permissions = front_matter + .permissions + .as_ref() + .and_then(|permissions| permissions.write.as_ref()) + .map(|write| { + json!({ + "connectionType": write.connection_type().as_ado_str(), + "allow": write + .options() + .map(|options| options.allow.as_slice()) + .unwrap_or(&[]), + }) + }); serde_json::to_string_pretty(&json!({ "name": front_matter.name, "toolConfigs": tool_configs, @@ -311,6 +325,7 @@ pub fn resolved_execution_config_json( "repositories": repositories, "checkout": front_matter.checkout, "repoRefs": front_matter.checkout_repo_refs(), + "writePermissions": write_permissions, "cacheMemory": cache_memory, })) .context("failed to serialize resolved safe-output configuration") @@ -412,8 +427,10 @@ fn validate_tool_name(tool_name: &str) -> Result<()> { safe-output tool" ); ensure!( - !matches!(tool_name, "scripts" | "jobs" | "require-approval" | "staged") - && !CUSTOM_JOB_SYSTEM_NEEDS.contains(&tool_name), + !matches!( + tool_name, + "scripts" | "jobs" | "require-approval" | "staged" + ) && !CUSTOM_JOB_SYSTEM_NEEDS.contains(&tool_name), "safe-outputs.jobs.{tool_name}: custom tool name is reserved" ); Ok(()) @@ -1105,6 +1122,48 @@ safe-outputs: assert_eq!(config["customTools"][0]["name"], "notify"); } + #[test] + fn resolved_execution_config_carries_repository_write_targets() { + let mut fm = parse_front_matter( + r#" +name: Test +description: Test +permissions: + write: + service-connection: ado-write + connection-type: azureDevOps + allow: + - organization: other-org + projects: + - project: Other Project + repositories: [target-repo] +repos: + - name: Other Project/target-repo + alias: target + organization: other-org + endpoint: cross-org-checkout +safe-outputs: + create-branch: {} +"#, + ); + let (repositories, checkout, checkout_fetch) = + crate::compile::common::resolve_repos(&fm).unwrap(); + fm.repositories = repositories; + fm.checkout = checkout; + fm.checkout_fetch = checkout_fetch; + + let config: Value = + serde_json::from_str(&resolved_execution_config_json(&fm, &[]).unwrap()).unwrap(); + + assert_eq!(config["repositories"][0]["organization"], "other-org"); + assert_eq!(config["repositories"][0]["endpoint"], "cross-org-checkout"); + assert_eq!(config["writePermissions"]["connectionType"], "azureDevOps"); + assert_eq!( + config["writePermissions"]["allow"][0]["projects"][0]["repositories"][0], + "target-repo" + ); + } + #[test] fn no_jobs_returns_empty_vec() { let fm = parse_front_matter("name: Test\ndescription: Test\n"); diff --git a/src/compile/types.rs b/src/compile/types.rs index 8d6e55d0..fc434dcc 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -3074,7 +3074,7 @@ pub struct PermissionsConfig { /// /// The serialized values deliberately match AzureCLI@3's `connectionType` /// input so front matter and generated YAML use one vocabulary. -#[derive(Debug, Deserialize, Clone, Copy, Default, PartialEq, Eq)] +#[derive(Debug, Deserialize, Serialize, Clone, Copy, Default, PartialEq, Eq)] pub enum WriteConnectionType { #[default] #[serde(rename = "azureRM")] @@ -3083,6 +3083,15 @@ pub enum WriteConnectionType { AzureDevOps, } +impl WriteConnectionType { + pub const fn as_ado_str(self) -> &'static str { + match self { + Self::AzureRm => "azureRM", + Self::AzureDevOps => "azureDevOps", + } + } +} + /// Stage 3 Azure DevOps credential and additional write-scope policy. /// /// The scalar form remains shorthand for an Azure Resource Manager service @@ -3285,7 +3294,7 @@ impl AdoReadCapability { } /// Explicit Azure DevOps organization scope. -#[derive(Debug, Deserialize, Clone, PartialEq)] +#[derive(Debug, Deserialize, Serialize, Clone, PartialEq)] #[serde(deny_unknown_fields)] pub struct AdoOrganizationScope { pub organization: crate::secure::AdoOrganization, @@ -3297,7 +3306,7 @@ pub struct AdoOrganizationScope { } /// Explicit project and optional repository scope within an organization. -#[derive(Debug, Deserialize, Clone, PartialEq)] +#[derive(Debug, Deserialize, Serialize, Clone, PartialEq)] #[serde(deny_unknown_fields)] pub struct AdoProjectScope { pub project: crate::secure::AdoProject, diff --git a/src/main.rs b/src/main.rs index 2e2d96c3..9d263c4b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -812,6 +812,8 @@ struct ResolvedExecutionConfig { #[serde(default)] repo_refs: std::collections::HashMap, #[serde(default)] + write_permissions: Option, + #[serde(default)] cache_memory: Option, } @@ -828,12 +830,22 @@ struct ResolvedExecutionRepository { /// `FrontMatter::checkout_cross_organization_repo_aliases`. #[serde(default)] endpoint: Option, + #[serde(default)] + organization: Option, } fn default_resolved_repo_type() -> String { "git".to_string() } +#[derive(Debug, serde::Deserialize)] +#[serde(rename_all = "camelCase")] +struct ResolvedWritePermissions { + connection_type: crate::compile::types::WriteConnectionType, + #[serde(default)] + allow: Vec, +} + #[derive(Debug, serde::Deserialize)] #[serde(rename_all = "camelCase")] struct ResolvedCacheMemory { @@ -870,30 +882,6 @@ async fn build_execution_context_from_resolved( ado_project: Option, dry_run: bool, ) -> crate::safe_outputs::ExecutionContext { - let allowed_repositories = config - .checkout - .iter() - .filter_map(|alias| { - config - .repositories - .iter() - .find(|repository| &repository.repository == alias) - .map(|repository| (alias.clone(), repository.name.clone())) - }) - .collect(); - let cross_organization_repositories = config - .checkout - .iter() - .filter(|alias| { - config.repositories.iter().any(|repository| { - &repository.repository == *alias - && repository.repo_type == "git" - && repository.endpoint.is_some() - }) - }) - .cloned() - .collect(); - let mut ctx = crate::safe_outputs::ExecutionContext::default(); if let Some(url) = ado_org_url { ctx.ado_organization = crate::safe_outputs::org_from_url(&url); @@ -904,9 +892,31 @@ async fn build_execution_context_from_resolved( } ctx.working_directory = safe_output_dir.to_path_buf(); ctx.tool_configs = config.tool_configs.clone(); - ctx.allowed_repositories = allowed_repositories; + crate::safe_outputs::configure_repository_write_context( + &mut ctx, + &config.checkout, + config + .repositories + .iter() + .map(|repository| crate::safe_outputs::RepositoryTargetSpec { + alias: repository.repository.clone(), + repo_type: repository.repo_type.clone(), + name: repository.name.clone(), + organization: repository.organization.clone(), + endpoint: repository.endpoint.clone(), + }) + .collect(), + config + .write_permissions + .as_ref() + .map(|permissions| permissions.connection_type), + config + .write_permissions + .as_ref() + .map(|permissions| permissions.allow.as_slice()) + .unwrap_or(&[]), + ); ctx.repo_refs = config.repo_refs.clone(); - ctx.cross_organization_repositories = cross_organization_repositories; ctx.dry_run = dry_run; let otel_path = safe_output_dir.join(agent_stats::OTEL_FILENAME); @@ -1056,19 +1066,6 @@ async fn build_execution_context( ado_project: Option, dry_run: bool, ) -> crate::safe_outputs::ExecutionContext { - // Map checkout aliases to ADO repo names from the repositories list - let allowed_repositories = front_matter - .checkout - .iter() - .filter_map(|alias| { - front_matter - .repositories - .iter() - .find(|r| &r.repository == alias) - .map(|repo| (alias.clone(), repo.name.clone())) - }) - .collect(); - let mut ctx = crate::safe_outputs::ExecutionContext::default(); // Only override env-derived values when CLI args are explicitly provided; // otherwise keep the defaults from SYSTEM_TEAMFOUNDATIONCOLLECTIONURI / @@ -1102,14 +1099,41 @@ async fn build_execution_context( ctx.tool_configs .insert(tool, serde_json::Value::Object(config)); } - ctx.allowed_repositories = allowed_repositories; + crate::safe_outputs::configure_repository_write_context( + &mut ctx, + &front_matter.checkout, + front_matter + .repositories + .iter() + .map(|repository| crate::safe_outputs::RepositoryTargetSpec { + alias: repository.repository.clone(), + repo_type: repository.repo_type.clone(), + name: repository.name.clone(), + organization: repository + .organization + .as_ref() + .map(|value| value.as_str().to_string()), + endpoint: repository.endpoint.clone(), + }) + .collect(), + front_matter + .permissions + .as_ref() + .and_then(|permissions| permissions.write.as_ref()) + .map(crate::compile::types::WritePermissionConfig::connection_type), + front_matter + .permissions + .as_ref() + .and_then(|permissions| permissions.write.as_ref()) + .and_then(crate::compile::types::WritePermissionConfig::options) + .map(|options| options.allow.as_slice()) + .unwrap_or(&[]), + ); // Per-checkout-alias git refs, so Stage 3 can resolve a per-repo // create-pull-request target branch (infer-target-from-checkout-ref). Uses // the same helper the compiler uses at build time, so the two paths cannot // diverge. ctx.repo_refs = front_matter.checkout_repo_refs(); - ctx.cross_organization_repositories = - front_matter.checkout_cross_organization_repo_aliases(); ctx.dry_run = dry_run; // Load agent stats from OTel JSONL if available diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 5836d411..7b09c90c 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -2650,11 +2650,17 @@ mod tests { fn cross_org_ctx() -> ExecutionContext { ExecutionContext { allowed_repositories: std::collections::HashMap::from([ - ("cross-org-repo".to_string(), "OtherProj/cross-org-repo".to_string()), - ("same-org-repo".to_string(), "Proj/same-org-repo".to_string()), + ( + "cross-org-repo".to_string(), + "OtherProj/cross-org-repo".to_string(), + ), + ( + "same-org-repo".to_string(), + "Proj/same-org-repo".to_string(), + ), ]), cross_organization_repositories: std::collections::HashSet::from([ - "cross-org-repo".to_string(), + "cross-org-repo".to_string() ]), ..Default::default() } @@ -2686,9 +2692,7 @@ mod tests { let ctx = cross_org_ctx(); // Matches through `lookup_allowed_repository_alias`'s trailing-name fallback. assert!(reject_cross_organization_repository("cross-org-repo", &ctx).is_some()); - assert!( - reject_cross_organization_repository("OtherProj/cross-org-repo", &ctx).is_some() - ); + assert!(reject_cross_organization_repository("OtherProj/cross-org-repo", &ctx).is_some()); } #[tokio::test] @@ -3312,6 +3316,9 @@ index 0000000..abcdefg repository_provider: Some("TfsGit".to_string()), github_api_url: "https://api.github.com".to_string(), allowed_repositories: std::collections::HashMap::new(), + repository_targets: std::collections::HashMap::new(), + write_connection_type: None, + write_allowed_repositories: std::collections::HashSet::new(), repo_refs: std::collections::HashMap::new(), cross_organization_repositories: std::collections::HashSet::new(), agent_stats: None, diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index e7b08bcf..1cf70780 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -317,6 +317,196 @@ pub(crate) fn canonical_repository_alias( lookup_allowed_repository_alias(repository, &ctx.allowed_repositories).cloned() } +#[derive(Debug, Clone)] +pub(crate) struct RepositoryTargetSpec { + pub alias: String, + pub repo_type: String, + pub name: String, + pub organization: Option, + pub endpoint: Option, +} + +pub(crate) fn repository_write_scope_key( + organization: &str, + project: &str, + repository: &str, +) -> String { + format!("{organization}/{project}/{repository}").to_ascii_lowercase() +} + +pub(crate) fn configure_repository_write_context( + ctx: &mut ExecutionContext, + checkout: &[String], + repositories: Vec, + write_connection_type: Option, + write_allow: &[crate::compile::types::AdoOrganizationScope], +) { + ctx.allowed_repositories.clear(); + ctx.repository_targets.clear(); + ctx.cross_organization_repositories.clear(); + for alias in checkout { + let Some(repository) = repositories + .iter() + .find(|repository| &repository.alias == alias) + else { + continue; + }; + ctx.allowed_repositories + .insert(alias.clone(), repository.name.clone()); + ctx.repository_targets.insert( + alias.clone(), + crate::safe_outputs::result::AdoRepositoryTargetConfig { + name: repository.name.clone(), + organization: repository.organization.clone(), + endpoint: repository.endpoint.clone(), + }, + ); + if repository.repo_type.eq_ignore_ascii_case("git") + && repository.endpoint.is_some() + && repository.organization.is_none() + { + ctx.cross_organization_repositories.insert(alias.clone()); + } + } + + ctx.write_connection_type = write_connection_type; + ctx.write_allowed_repositories = write_allow + .iter() + .flat_map(|organization| { + organization.projects.iter().flat_map(move |project| { + project.repositories.iter().map(move |repository| { + repository_write_scope_key( + organization.organization.as_str(), + project.project.as_str(), + repository.as_str(), + ) + }) + }) + }) + .collect(); +} + +fn split_repository_target_name( + name: &str, + current_project: &str, +) -> Result<(String, String), ExecutionResult> { + match name.split_once('/') { + Some((project, repository)) if !repository.contains('/') => { + Ok((project.to_string(), repository.to_string())) + } + None => Ok((current_project.to_string(), name.to_string())), + _ => Err(ExecutionResult::failure(format!( + "Repository '{name}' must be a repository name or project/repository" + ))), + } +} + +/// Resolve a repository selector to an exact organization/project/repository +/// destination and enforce the additional cross-organization write policy. +pub(crate) fn resolve_repository_write_target( + repository: Option<&str>, + ctx: &ExecutionContext, +) -> Result { + let selector = repository.unwrap_or("self"); + let Some(alias) = canonical_repository_alias(selector, ctx) else { + return Err(ExecutionResult::failure(format!( + "Repository '{selector}' is not in the allowed repository list" + ))); + }; + let current_org_url = ctx.ado_org_url.as_deref().ok_or_else(|| { + ExecutionResult::failure("Azure DevOps organization URL not configured") + })?; + let current_organization = ctx.ado_organization.as_deref().ok_or_else(|| { + ExecutionResult::failure("Azure DevOps organization name not configured") + })?; + let current_project = ctx + .ado_project + .as_deref() + .ok_or_else(|| ExecutionResult::failure("Azure DevOps project not configured"))?; + + if alias == "self" { + let name = ctx + .repository_name + .as_deref() + .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set"))?; + let (_, repository) = split_repository_target_name(name, current_project)?; + return Ok(crate::safe_outputs::result::AdoRepositoryTarget { + alias, + organization: current_organization.to_string(), + organization_url: current_org_url.trim_end_matches('/').to_string(), + project: current_project.to_string(), + repository, + repository_id: ctx.repository_id.clone(), + cross_organization: false, + }); + } + + let config = ctx.repository_targets.get(&alias).cloned().or_else(|| { + ctx.allowed_repositories.get(&alias).map(|name| { + crate::safe_outputs::result::AdoRepositoryTargetConfig { + name: name.clone(), + organization: None, + endpoint: None, + } + }) + }); + let Some(config) = config else { + return Err(ExecutionResult::failure(format!( + "Repository alias '{alias}' has no configured target metadata" + ))); + }; + if config.organization.is_none() + && (config.endpoint.is_some() || ctx.cross_organization_repositories.contains(&alias)) + { + return Err(ExecutionResult::failure(format!( + "Repository '{selector}' (checkout alias '{alias}') uses an endpoint-backed \ + Azure Repos checkout but has no `repos.organization`; the target organization \ + cannot be resolved safely." + ))); + } + + let (project, repository_name) = + split_repository_target_name(&config.name, current_project)?; + let organization = config + .organization + .as_deref() + .unwrap_or(current_organization); + let cross_organization = !organization.eq_ignore_ascii_case(current_organization); + if cross_organization { + if ctx.write_connection_type + != Some(crate::compile::types::WriteConnectionType::AzureDevOps) + { + return Err(ExecutionResult::failure(format!( + "Repository '{selector}' resolves to cross-organization target \ + '{organization}/{project}/{repository_name}', but permissions.write must use \ + `connection-type: azureDevOps`." + ))); + } + let scope = repository_write_scope_key(organization, &project, &repository_name); + if !ctx.write_allowed_repositories.contains(&scope) { + return Err(ExecutionResult::failure(format!( + "Repository '{selector}' resolves to cross-organization target \ + '{organization}/{project}/{repository_name}', which is not listed in \ + permissions.write.allow." + ))); + } + } + + Ok(crate::safe_outputs::result::AdoRepositoryTarget { + alias, + organization: organization.to_string(), + organization_url: if cross_organization { + format!("https://dev.azure.com/{organization}") + } else { + current_org_url.trim_end_matches('/').to_string() + }, + project, + repository: repository_name, + repository_id: None, + cross_organization, + }) +} + /// Resolve a repository selector to its checkout directory. /// /// The checkout root and `self` directory differ in multi-checkout jobs. @@ -1066,6 +1256,133 @@ mod tests { } } + fn repository_target_ctx() -> ExecutionContext { + ExecutionContext { + ado_org_url: Some("https://dev.azure.com/current-org/".to_string()), + ado_organization: Some("current-org".to_string()), + ado_project: Some("Current Project".to_string()), + repository_id: Some("self-id".to_string()), + repository_name: Some("Current Project/self-repo".to_string()), + ..Default::default() + } + } + + fn cross_org_allow() -> Vec { + vec![crate::compile::types::AdoOrganizationScope { + organization: crate::secure::AdoOrganization::parse("other-org").unwrap(), + projects: vec![crate::compile::types::AdoProjectScope { + project: crate::secure::AdoProject::parse("Other Project").unwrap(), + project_id: None, + repositories: vec![crate::secure::AdoRepository::parse("target-repo").unwrap()], + }], + }] + } + + #[test] + fn repository_write_target_resolves_self_with_repository_id() { + let ctx = repository_target_ctx(); + + let target = resolve_repository_write_target(Some("self"), &ctx).unwrap(); + + assert_eq!(target.organization, "current-org"); + assert_eq!(target.project, "Current Project"); + assert_eq!(target.repository, "self-repo"); + assert_eq!(target.repository_locator(), "self-id"); + assert!(!target.cross_organization); + } + + #[test] + fn repository_write_target_resolves_same_org_checkout_project() { + let mut ctx = repository_target_ctx(); + configure_repository_write_context( + &mut ctx, + &["tools".to_string()], + vec![RepositoryTargetSpec { + alias: "tools".to_string(), + repo_type: "git".to_string(), + name: "Tools Project/tooling".to_string(), + organization: None, + endpoint: None, + }], + None, + &[], + ); + + let target = resolve_repository_write_target(Some("tooling"), &ctx).unwrap(); + + assert_eq!(target.organization, "current-org"); + assert_eq!(target.project, "Tools Project"); + assert_eq!(target.repository, "tooling"); + assert!(!target.cross_organization); + } + + #[test] + fn repository_write_target_resolves_allowed_cross_org_checkout() { + let mut ctx = repository_target_ctx(); + configure_repository_write_context( + &mut ctx, + &["target".to_string()], + vec![RepositoryTargetSpec { + alias: "target".to_string(), + repo_type: "git".to_string(), + name: "Other Project/target-repo".to_string(), + organization: Some("other-org".to_string()), + endpoint: Some("cross-org-checkout".to_string()), + }], + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + &cross_org_allow(), + ); + + let target = resolve_repository_write_target(Some("target"), &ctx).unwrap(); + + assert_eq!(target.organization_url, "https://dev.azure.com/other-org"); + assert_eq!(target.project, "Other Project"); + assert_eq!(target.repository, "target-repo"); + assert!(target.cross_organization); + } + + #[test] + fn repository_write_target_rejects_incomplete_or_unauthorized_cross_org() { + for (organization, connection_type, allow, expected) in [ + ( + None, + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + cross_org_allow(), + "no `repos.organization`", + ), + ( + Some("other-org".to_string()), + Some(crate::compile::types::WriteConnectionType::AzureRm), + cross_org_allow(), + "connection-type: azureDevOps", + ), + ( + Some("other-org".to_string()), + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + Vec::new(), + "not listed in permissions.write.allow", + ), + ] { + let mut ctx = repository_target_ctx(); + configure_repository_write_context( + &mut ctx, + &["target".to_string()], + vec![RepositoryTargetSpec { + alias: "target".to_string(), + repo_type: "git".to_string(), + name: "Other Project/target-repo".to_string(), + organization, + endpoint: Some("cross-org-checkout".to_string()), + }], + connection_type, + &allow, + ); + + let error = resolve_repository_write_target(Some("target"), &ctx).unwrap_err(); + assert!(error.message.contains(expected), "{}", error.message); + } + } + #[test] fn test_resolve_repository_checkout_dir_distinguishes_root_and_self() { let mut ctx = ctx_with(Some("4x4/current-repo"), sample_allowed()); diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index 55cabec8..737b19ab 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -59,6 +59,40 @@ pub struct ResolvedWorkItem { pub url: String, } +/// Trusted compiler/source metadata for one checked-out repository alias. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AdoRepositoryTargetConfig { + pub name: String, + pub organization: Option, + pub endpoint: Option, +} + +/// Fully resolved Azure DevOps destination for a repository write. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AdoRepositoryTarget { + pub alias: String, + pub organization: String, + pub organization_url: String, + pub project: String, + pub repository: String, + pub repository_id: Option, + pub cross_organization: bool, +} + +impl AdoRepositoryTarget { + pub fn repository_locator(&self) -> &str { + self.repository_id.as_deref().unwrap_or(&self.repository) + } + + pub fn qualified_repository(&self) -> String { + format!("{}/{}", self.project, self.repository) + } + + pub fn display_name(&self) -> String { + format!("{}/{}/{}", self.organization, self.project, self.repository) + } +} + fn register_resolved_reference( registry: &Mutex>, id: String, @@ -123,6 +157,12 @@ pub struct ExecutionContext { /// Allowed repositories for PRs: "self" + checkout list aliases /// Maps alias to ADO repo name (e.g., "other-repo" -> "org/other-repo") pub allowed_repositories: HashMap, + /// Trusted per-alias repository routing metadata. + pub repository_targets: HashMap, + /// Service-connection type used to mint `SC_WRITE_TOKEN`, when configured. + pub write_connection_type: Option, + /// Normalized additional write scopes (`organization/project/repository`). + pub write_allowed_repositories: HashSet, /// Per-checkout-alias git ref (from `repos: ref`), used to resolve a /// per-repo `create-pull-request` target branch when /// `infer-target-from-checkout-ref` is set. Maps a checkout alias to its @@ -414,6 +454,9 @@ impl ExecutionContext { repository_name, repository_provider: env("BUILD_REPOSITORY_PROVIDER"), allowed_repositories: HashMap::new(), + repository_targets: HashMap::new(), + write_connection_type: None, + write_allowed_repositories: HashSet::new(), repo_refs: HashMap::new(), cross_organization_repositories: HashSet::new(), agent_stats: None, diff --git a/src/safe_outputs/upload_build_attachment.rs b/src/safe_outputs/upload_build_attachment.rs index 7da41b0f..1962e7b9 100644 --- a/src/safe_outputs/upload_build_attachment.rs +++ b/src/safe_outputs/upload_build_attachment.rs @@ -924,6 +924,9 @@ attachment-type: "agent-artifact" repository_provider: None, github_api_url: "https://api.github.com".to_string(), allowed_repositories: std::collections::HashMap::new(), + repository_targets: std::collections::HashMap::new(), + write_connection_type: None, + write_allowed_repositories: std::collections::HashSet::new(), repo_refs: std::collections::HashMap::new(), cross_organization_repositories: std::collections::HashSet::new(), agent_stats: None, From c4bc7de3e45864806d0d8127b703de6431a83906 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 10:19:43 +0100 Subject: [PATCH 05/18] feat(safe-outputs): support cross-org branch and tag writes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/safe_outputs/create_branch.rs | 122 ++++++++++++++++++++++------- src/safe_outputs/create_git_tag.rs | 117 +++++++++++++++++++++------ 2 files changed, 184 insertions(+), 55 deletions(-) diff --git a/src/safe_outputs/create_branch.rs b/src/safe_outputs/create_branch.rs index 0b4ac304..3e8517c2 100644 --- a/src/safe_outputs/create_branch.rs +++ b/src/safe_outputs/create_branch.rs @@ -194,23 +194,36 @@ impl Executor for CreateBranchResult { format!("create branch '{}'", self.branch_name) } + async fn execute_sanitized( + &mut self, + ctx: &ExecutionContext, + ) -> anyhow::Result { + self.sanitize_content_fields(); + let target = match crate::safe_outputs::resolve_repository_write_target( + self.repository.as_deref(), + ctx, + ) { + Ok(target) => target, + Err(failure) => return Ok(failure), + }; + if ctx.dry_run { + return Ok(ExecutionResult::success(format!( + "[DRY-RUN] Would execute: create branch '{}' in {}", + self.branch_name, + target.display_name() + ))); + } + self.execute_impl(ctx).await + } + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { info!("Creating branch: '{}'", self.branch_name); debug!("create-branch: branch_name='{}'", self.branch_name); - let org_url = ctx - .ado_org_url - .as_ref() - .context("AZURE_DEVOPS_ORG_URL not set")?; - let project = ctx - .ado_project - .as_ref() - .context("SYSTEM_TEAMPROJECT not set")?; let token = ctx .access_token .as_ref() .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - debug!("ADO org: {}, project: {}", org_url, project); let config: CreateBranchConfig = ctx.get_tool_config("create-branch")?; debug!("Branch pattern: {:?}", config.branch_pattern); @@ -250,20 +263,13 @@ impl Executor for CreateBranchResult { ))); } - // Resolve the alias to the actual ADO repo name - let repo_name = if repo_alias == "self" { - ctx.repository_name - .as_deref() - .context("BUILD_REPOSITORY_NAME not set")? - .to_string() - } else { - crate::safe_outputs::lookup_allowed_repository(repo_alias, &ctx.allowed_repositories) - .cloned() - .context(format!( - "Repository alias '{}' is not in the allowed checkout list", - repo_alias - ))? - }; + let target = + match crate::safe_outputs::resolve_repository_write_target(Some(repo_alias), ctx) { + Ok(target) => target, + Err(failure) => return Ok(failure), + }; + let repo_name = target.qualified_repository(); + debug!("Resolved repository target: {}", target.display_name()); debug!("Resolved repository: {}", repo_name); // Validate source_branch against allowed-source-branches (if configured) @@ -287,17 +293,24 @@ impl Executor for CreateBranchResult { commit.clone() } else { debug!("Resolving source branch '{}' to commit", source_branch); - resolve_branch_to_commit(&client, org_url, project, token, &repo_name, source_branch) - .await? + resolve_branch_to_commit( + &client, + &target.organization_url, + &target.project, + token, + target.repository_locator(), + source_branch, + ) + .await? }; debug!("Source commit SHA: {}", source_sha); // Build the refs update URL let url = format!( "{}/{}/_apis/git/repositories/{}/refs?api-version=7.1", - org_url.trim_end_matches('/'), - utf8_percent_encode(project, PATH_SEGMENT), - utf8_percent_encode(&repo_name, PATH_SEGMENT), + target.organization_url, + utf8_percent_encode(&target.project, PATH_SEGMENT), + utf8_percent_encode(target.repository_locator(), PATH_SEGMENT), ); debug!("API URL: {}", url); @@ -368,7 +381,8 @@ impl Executor for CreateBranchResult { "ref": ref_name, "repository": repo_name, "source_commit": source_sha, - "project": project, + "project": target.project, + "organization": target.organization, }), )) } else { @@ -600,4 +614,54 @@ allowed-source-branches: repository ); } + + #[tokio::test] + async fn test_dry_run_resolves_cross_org_target() { + let mut ctx = ExecutionContext { + ado_org_url: Some("https://dev.azure.com/current-org".to_string()), + ado_organization: Some("current-org".to_string()), + ado_project: Some("Current Project".to_string()), + dry_run: true, + ..Default::default() + }; + let allow = vec![crate::compile::types::AdoOrganizationScope { + organization: crate::secure::AdoOrganization::parse("other-org").unwrap(), + projects: vec![crate::compile::types::AdoProjectScope { + project: crate::secure::AdoProject::parse("Other Project").unwrap(), + project_id: None, + repositories: vec![crate::secure::AdoRepository::parse("target-repo").unwrap()], + }], + }]; + crate::safe_outputs::configure_repository_write_context( + &mut ctx, + &["target".to_string()], + vec![crate::safe_outputs::RepositoryTargetSpec { + alias: "target".to_string(), + repo_type: "git".to_string(), + name: "Other Project/target-repo".to_string(), + organization: Some("other-org".to_string()), + endpoint: Some("cross-org-checkout".to_string()), + }], + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + &allow, + ); + let mut result = CreateBranchResult { + name: CreateBranchResult::NAME.to_string(), + branch_name: "feature/cross-org".to_string(), + source_branch: Some("main".to_string()), + source_commit: None, + repository: Some("target".to_string()), + }; + + let execution = result.execute_sanitized(&ctx).await.unwrap(); + + assert!(execution.success, "{}", execution.message); + assert!( + execution + .message + .contains("other-org/Other Project/target-repo"), + "{}", + execution.message + ); + } } diff --git a/src/safe_outputs/create_git_tag.rs b/src/safe_outputs/create_git_tag.rs index a57527aa..cf085b60 100644 --- a/src/safe_outputs/create_git_tag.rs +++ b/src/safe_outputs/create_git_tag.rs @@ -222,23 +222,36 @@ impl Executor for CreateGitTagResult { format!("create git tag '{}'", self.tag_name) } + async fn execute_sanitized( + &mut self, + ctx: &ExecutionContext, + ) -> anyhow::Result { + self.sanitize_content_fields(); + let target = match crate::safe_outputs::resolve_repository_write_target( + self.repository.as_deref(), + ctx, + ) { + Ok(target) => target, + Err(failure) => return Ok(failure), + }; + if ctx.dry_run { + return Ok(ExecutionResult::success(format!( + "[DRY-RUN] Would execute: create git tag '{}' in {}", + self.tag_name, + target.display_name() + ))); + } + self.execute_impl(ctx).await + } + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { info!("Creating git tag: '{}'", self.tag_name); debug!("create-git-tag: tag_name='{}'", self.tag_name); - let org_url = ctx - .ado_org_url - .as_ref() - .context("AZURE_DEVOPS_ORG_URL not set")?; - let project = ctx - .ado_project - .as_ref() - .context("SYSTEM_TEAMPROJECT not set")?; let token = ctx .access_token .as_ref() .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - debug!("ADO org: {}, project: {}", org_url, project); let config: CreateGitTagConfig = ctx.get_tool_config("create-git-tag")?; debug!("Tag pattern: {:?}", config.tag_pattern); @@ -273,19 +286,13 @@ impl Executor for CreateGitTagResult { ))); } - let repo_name = if repo_alias == "self" { - ctx.repository_name - .as_deref() - .context("BUILD_REPOSITORY_NAME not set and repository is 'self'")? - .to_string() - } else { - crate::safe_outputs::lookup_allowed_repository(repo_alias, &ctx.allowed_repositories) - .cloned() - .context(format!( - "Repository alias '{}' not found in allowed repositories", - repo_alias - ))? - }; + let target = + match crate::safe_outputs::resolve_repository_write_target(Some(repo_alias), ctx) { + Ok(target) => target, + Err(failure) => return Ok(failure), + }; + let repo_name = target.qualified_repository(); + debug!("Resolved repository target: {}", target.display_name()); let client = reqwest::Client::new(); @@ -294,7 +301,14 @@ impl Executor for CreateGitTagResult { Some(sha) => sha.clone(), None => { info!("No commit specified, resolving HEAD of default branch"); - resolve_head_commit(&client, org_url, project, token, &repo_name).await? + resolve_head_commit( + &client, + &target.organization_url, + &target.project, + token, + target.repository_locator(), + ) + .await? } }; debug!("Tagging commit: {}", commit_sha); @@ -310,9 +324,9 @@ impl Executor for CreateGitTagResult { // POST annotated tag let url = format!( "{}/{}/_apis/git/repositories/{}/annotatedtags?api-version=7.1", - org_url.trim_end_matches('/'), - utf8_percent_encode(project, PATH_SEGMENT), - utf8_percent_encode(&repo_name, PATH_SEGMENT), + target.organization_url, + utf8_percent_encode(&target.project, PATH_SEGMENT), + utf8_percent_encode(target.repository_locator(), PATH_SEGMENT), ); debug!("API URL: {}", url); @@ -374,6 +388,7 @@ impl Executor for CreateGitTagResult { #[cfg(test)] mod tests { use super::*; + use crate::safe_outputs::ToolResult; #[test] fn test_params_deserializes() { @@ -538,4 +553,54 @@ message-prefix: "[release] " repository ); } + + #[tokio::test] + async fn test_dry_run_resolves_cross_org_target() { + let mut ctx = ExecutionContext { + ado_org_url: Some("https://dev.azure.com/current-org".to_string()), + ado_organization: Some("current-org".to_string()), + ado_project: Some("Current Project".to_string()), + dry_run: true, + ..Default::default() + }; + let allow = vec![crate::compile::types::AdoOrganizationScope { + organization: crate::secure::AdoOrganization::parse("other-org").unwrap(), + projects: vec![crate::compile::types::AdoProjectScope { + project: crate::secure::AdoProject::parse("Other Project").unwrap(), + project_id: None, + repositories: vec![crate::secure::AdoRepository::parse("target-repo").unwrap()], + }], + }]; + crate::safe_outputs::configure_repository_write_context( + &mut ctx, + &["target".to_string()], + vec![crate::safe_outputs::RepositoryTargetSpec { + alias: "target".to_string(), + repo_type: "git".to_string(), + name: "Other Project/target-repo".to_string(), + organization: Some("other-org".to_string()), + endpoint: Some("cross-org-checkout".to_string()), + }], + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + &allow, + ); + let mut result = CreateGitTagResult { + name: CreateGitTagResult::NAME.to_string(), + tag_name: "cross-org-v1".to_string(), + commit: None, + message: Some("Cross organization tag".to_string()), + repository: Some("target".to_string()), + }; + + let execution = result.execute_sanitized(&ctx).await.unwrap(); + + assert!(execution.success, "{}", execution.message); + assert!( + execution + .message + .contains("other-org/Other Project/target-repo"), + "{}", + execution.message + ); + } } From 09b87e1da7053f5b23a9e449b05b352b4888997e Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 10:28:18 +0100 Subject: [PATCH 06/18] feat(safe-outputs): support cross-org pull requests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/safe_outputs/create_pull_request.rs | 322 ++++++++++++------------ 1 file changed, 157 insertions(+), 165 deletions(-) diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 7b09c90c..9f28a30c 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -1,11 +1,12 @@ //! Create pull request safe output tool use log::{debug, info, warn}; +use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use tokio::process::Command; -use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, PATH_SEGMENT, Validate}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; use crate::validate::reject_pipeline_injection; @@ -161,10 +162,7 @@ async fn resolve_reviewer_identity( } // Use Identity Picker API on vssps.dev.azure.com to resolve email or display name - let identity_url = format!( - "https://vssps.dev.azure.com/{}/_apis/identitypicker/identities?api-version=7.1-preview.1", - organization - ); + let identity_url = identity_picker_url(organization); debug!("Identity lookup URL: {}", identity_url); let query_body = serde_json::json!({ @@ -221,6 +219,13 @@ async fn resolve_reviewer_identity( } } +fn identity_picker_url(organization: &str) -> String { + format!( + "https://vssps.dev.azure.com/{}/_apis/identitypicker/identities?api-version=7.1-preview.1", + organization + ) +} + /// Parameters for creating a pull request #[derive(Deserialize, JsonSchema)] pub struct CreatePrParams { @@ -493,33 +498,6 @@ pub(crate) fn short_branch(git_ref: &str) -> &str { .unwrap_or(git_ref) } -/// Returns a clear failure result when `repository` resolves to a checkout -/// alias flagged as cross-organization (`ctx.cross_organization_repositories`). -/// -/// `create-pull-request` composes every Git REST call from the pipeline's own -/// `ado_org_url`/`ado_project`, so a `repos:` alias checked out from another -/// Azure DevOps organization via an `endpoint:` service connection cannot be -/// targeted correctly today — the request would silently resolve against the -/// wrong organization (a 404, not a permissions error). Rejecting this -/// up front, before any network call, replaces that confusing failure with an -/// actionable one and lets `--dry-run` catch it too (issue #1934). -fn reject_cross_organization_repository( - repository: &str, - ctx: &ExecutionContext, -) -> Option { - let alias = crate::safe_outputs::canonical_repository_alias(repository, ctx)?; - if !ctx.cross_organization_repositories.contains(&alias) { - return None; - } - Some(ExecutionResult::failure(format!( - "Repository '{repository}' (checkout alias '{alias}') is checked out from another \ - Azure DevOps organization via a `repos:` `endpoint:` service connection. \ - create-pull-request cannot yet target a cross-organization repository — it composes \ - every Git API call against this pipeline's own organization and project. See \ - docs/safe-outputs.md for details." - ))) -} - impl CreatePrConfig { /// Resolve the target (base) branch for a PR against `repo_alias` /// (`"self"` or a `checkout:` alias). Shared by the compiler (to deepen the @@ -569,6 +547,15 @@ fn default_protected_files() -> ProtectedFiles { ProtectedFiles::Blocked } +fn repository_api_base(target: &crate::safe_outputs::result::AdoRepositoryTarget) -> String { + format!( + "{}/{}/_apis/git/repositories/{}", + target.organization_url, + utf8_percent_encode(&target.project, PATH_SEGMENT), + utf8_percent_encode(target.repository_locator(), PATH_SEGMENT), + ) +} + impl Default for CreatePrConfig { fn default() -> Self { Self { @@ -621,22 +608,22 @@ impl Executor for CreatePrResult { format!("create PR: '{}' in repo '{}'", self.title, self.repository) } - /// Rejects a cross-organization repository alias before the default - /// dry-run short-circuit, so `--dry-run` surfaces the same failure a real - /// run would hit instead of reporting a false "would execute" success - /// (issue #1934). async fn execute_sanitized( &mut self, ctx: &ExecutionContext, ) -> anyhow::Result { self.sanitize_content_fields(); - if let Some(failure) = reject_cross_organization_repository(&self.repository, ctx) { - return Ok(failure); - } + let target = + match crate::safe_outputs::resolve_repository_write_target(Some(&self.repository), ctx) + { + Ok(target) => target, + Err(failure) => return Ok(failure), + }; if ctx.dry_run { return Ok(ExecutionResult::success(format!( - "[DRY-RUN] Would execute: {}", - self.dry_run_summary() + "[DRY-RUN] Would execute: create PR: '{}' in {}", + self.title, + target.display_name() ))); } self.execute_impl(ctx).await @@ -710,56 +697,23 @@ impl Executor for CreatePrResult { .join(", ") ))); }; - let repo_id = if repository_alias == "self" { - // "self" or a name match against the pipeline's own repository - debug!("Using 'self' repository (matched '{}')", self.repository); - ctx.repository_id - .as_ref() - .or(ctx.repository_name.as_ref()) - .context("Repository ID not configured for 'self'")? - .clone() - } else if let Some(ado_repo_name) = ctx.allowed_repositories.get(&repository_alias) { - debug!( - "Repository '{}' resolved through alias '{}' to '{}'", - self.repository, repository_alias, ado_repo_name - ); - ado_repo_name.clone() - } else { - // Unreachable: `repository_alias` is either "self" (handled above) - // or a key produced by iterating `ctx.allowed_repositories`, so the - // lookup cannot miss. Kept as a fail-closed guard in case the - // canonicalization and the map ever drift apart. - debug_assert!( - false, - "canonical alias '{repository_alias}' is absent from allowed_repositories" - ); - return Ok(ExecutionResult::failure(format!( - "Repository alias '{}' has no configured repository", - repository_alias - ))); + let target = match crate::safe_outputs::resolve_repository_write_target( + Some(&repository_alias), + ctx, + ) { + Ok(target) => target, + Err(failure) => return Ok(failure), }; - debug!("Resolved repository ID: {}", repo_id); + debug!("Resolved repository ID: {}", target.repository_locator()); - // Get ADO configuration - let org_url = ctx - .ado_org_url - .as_ref() - .context("Azure DevOps organization URL not configured")?; - let organization = ctx - .ado_organization - .as_ref() - .context("Azure DevOps organization name not configured")?; - let project = ctx - .ado_project - .as_ref() - .context("Azure DevOps project not configured")?; let token = ctx .access_token .as_ref() .context("Access token not configured")?; debug!( - "ADO org: {}, organization: {}, project: {}", - org_url, organization, project + "ADO target: {} (alias '{}')", + target.display_name(), + repository_alias ); // Validate and read the patch file @@ -1091,8 +1045,9 @@ impl Executor for CreatePrResult { // Get the target branch ref to find the base commit debug!("Getting target branch ref from ADO"); let refs_url = format!( - "{}{}/_apis/git/repositories/{}/refs?filter=heads/{}&api-version=7.1", - org_url, project, repo_id, target_branch + "{}/refs?filter=heads/{}&api-version=7.1", + repository_api_base(&target), + target_branch ); debug!("Refs URL: {}", refs_url); @@ -1147,8 +1102,9 @@ impl Executor for CreatePrResult { // Retry with new random suffixes up to 3 times. for attempt in 0..3 { let check_ref_url = format!( - "{}{}/_apis/git/repositories/{}/refs?filter=heads/{}&api-version=7.1", - org_url, project, repo_id, source_branch + "{}/refs?filter=heads/{}&api-version=7.1", + repository_api_base(&target), + source_branch ); debug!( "Checking if source branch exists (attempt {}): {}", @@ -1183,10 +1139,7 @@ impl Executor for CreatePrResult { // Push changes via ADO API (this creates the branch and commits in one call) info!("Pushing changes to ADO"); - let push_url = format!( - "{}{}/_apis/git/repositories/{}/pushes?api-version=7.1", - org_url, project, repo_id - ); + let push_url = format!("{}/pushes?api-version=7.1", repository_api_base(&target)); debug!("Push URL: {}", push_url); (source_branch, source_ref) = match push_new_branch(PushBranchParams { @@ -1225,8 +1178,8 @@ impl Executor for CreatePrResult { // Create the pull request via REST API info!("Creating pull request"); let pr_url = format!( - "{}{}/_apis/git/repositories/{}/pullrequests?api-version=7.1", - org_url, project, repo_id + "{}/pullrequests?api-version=7.1", + repository_api_base(&target) ); debug!("PR URL: {}", pr_url); @@ -1335,14 +1288,12 @@ impl Executor for CreatePrResult { let pr_ctx = PrContext { client: &client, config: &config, - org_url, - project, - repo_id: &repo_id, + target: &target, pr_id, token, }; set_pr_completion_options(&pr_ctx, pr_data["createdBy"]["id"].as_str()).await; - add_reviewers_to_pr(&pr_ctx, organization).await; + add_reviewers_to_pr(&pr_ctx).await; info!( "PR #{} created successfully: {} -> {}{}", @@ -1749,9 +1700,7 @@ fn validate_and_build_labels( struct PrContext<'a> { client: &'a reqwest::Client, config: &'a CreatePrConfig, - org_url: &'a str, - project: &'a str, - repo_id: &'a str, + target: &'a crate::safe_outputs::result::AdoRepositoryTarget, pr_id: i64, token: &'a str, } @@ -1765,8 +1714,9 @@ async fn set_pr_completion_options(ctx: &PrContext<'_>, pr_created_by_id: Option ctx.config.delete_source_branch, ctx.config.squash_merge, ctx.config.auto_complete ); let pr_update_url = format!( - "{}{}/_apis/git/repositories/{}/pullrequests/{}?api-version=7.1", - ctx.org_url, ctx.project, ctx.repo_id, ctx.pr_id + "{}/pullrequests/{}?api-version=7.1", + repository_api_base(ctx.target), + ctx.pr_id ); let mut update_body = serde_json::json!({ @@ -1813,7 +1763,7 @@ async fn set_pr_completion_options(ctx: &PrContext<'_>, pr_created_by_id: Option /// Resolves each reviewer's identity (email/display-name → ADO identity ID) and /// issues a `PUT` for each one. Logs a warning if a reviewer cannot be resolved or /// if the API call fails; does not abort the overall PR creation. -async fn add_reviewers_to_pr(ctx: &PrContext<'_>, organization: &str) { +async fn add_reviewers_to_pr(ctx: &PrContext<'_>) { if ctx.config.reviewers.is_empty() { return; } @@ -1822,21 +1772,29 @@ async fn add_reviewers_to_pr(ctx: &PrContext<'_>, organization: &str) { debug!("Adding reviewer: {}", reviewer); // Resolve reviewer identity (email/name -> ID) - let reviewer_id = - match resolve_reviewer_identity(ctx.client, organization, ctx.token, reviewer).await { - Some(id) => id, - None => { - warn!( - "Could not resolve reviewer '{}' to an identity ID, skipping", - reviewer - ); - continue; - } - }; + let reviewer_id = match resolve_reviewer_identity( + ctx.client, + &ctx.target.organization, + ctx.token, + reviewer, + ) + .await + { + Some(id) => id, + None => { + warn!( + "Could not resolve reviewer '{}' to an identity ID, skipping", + reviewer + ); + continue; + } + }; let reviewer_url = format!( - "{}{}/_apis/git/repositories/{}/pullrequests/{}/reviewers/{}?api-version=7.1", - ctx.org_url, ctx.project, ctx.repo_id, ctx.pr_id, reviewer_id + "{}/pullrequests/{}/reviewers/{}?api-version=7.1", + repository_api_base(ctx.target), + ctx.pr_id, + reviewer_id ); let reviewer_body = serde_json::json!({ "vote": 0, "isRequired": false }); @@ -2647,57 +2605,66 @@ mod tests { assert_eq!(short_branch("refs/heads/"), "refs/heads/"); } - fn cross_org_ctx() -> ExecutionContext { - ExecutionContext { - allowed_repositories: std::collections::HashMap::from([ - ( - "cross-org-repo".to_string(), - "OtherProj/cross-org-repo".to_string(), - ), - ( - "same-org-repo".to_string(), - "Proj/same-org-repo".to_string(), - ), - ]), - cross_organization_repositories: std::collections::HashSet::from([ - "cross-org-repo".to_string() - ]), - ..Default::default() - } - } - #[test] - fn test_reject_cross_organization_repository_flags_cross_org_alias() { - let ctx = cross_org_ctx(); - let result = reject_cross_organization_repository("cross-org-repo", &ctx); - assert!(result.is_some()); - let result = result.unwrap(); - assert!(!result.success); - assert!( - result.message.contains("cross-organization"), - "message should explain the cross-organization limitation: {}", - result.message - ); - } + fn test_cross_org_api_and_identity_urls_use_target_context() { + let target = crate::safe_outputs::result::AdoRepositoryTarget { + alias: "target".to_string(), + organization: "other-org".to_string(), + organization_url: "https://dev.azure.com/other-org".to_string(), + project: "Other Project".to_string(), + repository: "target repo".to_string(), + repository_id: None, + cross_organization: true, + }; - #[test] - fn test_reject_cross_organization_repository_allows_same_org_alias() { - let ctx = cross_org_ctx(); - assert!(reject_cross_organization_repository("same-org-repo", &ctx).is_none()); - assert!(reject_cross_organization_repository("self", &ctx).is_none()); + assert_eq!( + repository_api_base(&target), + "https://dev.azure.com/other-org/Other%20Project/_apis/git/repositories/target%20repo" + ); + assert_eq!( + identity_picker_url(&target.organization), + "https://vssps.dev.azure.com/other-org/_apis/identitypicker/identities?api-version=7.1-preview.1" + ); } - #[test] - fn test_reject_cross_organization_repository_matches_by_trailing_name() { - let ctx = cross_org_ctx(); - // Matches through `lookup_allowed_repository_alias`'s trailing-name fallback. - assert!(reject_cross_organization_repository("cross-org-repo", &ctx).is_some()); - assert!(reject_cross_organization_repository("OtherProj/cross-org-repo", &ctx).is_some()); + fn cross_org_ctx(organization: Option<&str>, allow: bool) -> ExecutionContext { + let mut ctx = ExecutionContext { + ado_org_url: Some("https://dev.azure.com/current-org".to_string()), + ado_organization: Some("current-org".to_string()), + ado_project: Some("Current Project".to_string()), + ..Default::default() + }; + let scopes = allow.then(|| { + vec![crate::compile::types::AdoOrganizationScope { + organization: crate::secure::AdoOrganization::parse("other-org").unwrap(), + projects: vec![crate::compile::types::AdoProjectScope { + project: crate::secure::AdoProject::parse("Other Project").unwrap(), + project_id: None, + repositories: vec![ + crate::secure::AdoRepository::parse("cross-org-repo").unwrap(), + ], + }], + }] + }); + crate::safe_outputs::configure_repository_write_context( + &mut ctx, + &["cross-org-repo".to_string()], + vec![crate::safe_outputs::RepositoryTargetSpec { + alias: "cross-org-repo".to_string(), + repo_type: "git".to_string(), + name: "Other Project/cross-org-repo".to_string(), + organization: organization.map(str::to_string), + endpoint: Some("cross-org-checkout".to_string()), + }], + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + scopes.as_deref().unwrap_or(&[]), + ); + ctx } #[tokio::test] - async fn test_dry_run_surfaces_cross_organization_rejection() { - let mut ctx = cross_org_ctx(); + async fn test_dry_run_resolves_authorized_cross_organization_target() { + let mut ctx = cross_org_ctx(Some("other-org"), true); ctx.dry_run = true; let mut result = CreatePrResult { @@ -2714,16 +2681,41 @@ mod tests { let execution = result.execute_sanitized(&ctx).await.unwrap(); assert!( - !execution.success, - "dry-run must not report success for a cross-organization repository" + execution.success, + "authorized dry-run should succeed: {}", + execution.message ); assert!( - execution.message.contains("cross-organization"), - "dry-run message should explain the limitation: {}", + execution + .message + .contains("other-org/Other Project/cross-org-repo"), + "dry-run message should name the resolved target: {}", execution.message ); } + #[tokio::test] + async fn test_dry_run_rejects_cross_org_endpoint_without_organization() { + let mut ctx = cross_org_ctx(None, true); + ctx.dry_run = true; + let mut result = CreatePrResult { + name: CreatePrResult::NAME.to_string(), + title: "Fix bug in parser".to_string(), + description: "This PR fixes a critical bug in the parser module.".to_string(), + source_branch: "agent/fix".to_string(), + patch_file: "patch.diff".to_string(), + repository: "cross-org-repo".to_string(), + agent_labels: vec![], + base_commit: None, + patch_sha256: "deadbeef".to_string(), + }; + + let execution = result.execute_sanitized(&ctx).await.unwrap(); + + assert!(!execution.success); + assert!(execution.message.contains("repos.organization")); + } + #[test] fn test_target_branches_sanitizes_both_keys_and_values() { use crate::sanitize::SanitizeConfig; From d18c1cd32f26e1be520bf9b540af6e33a73a16f8 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 10:41:21 +0100 Subject: [PATCH 07/18] fix(safe-outputs): harden repository target routing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/safe_outputs/create_branch.rs | 63 ++++++++---- src/safe_outputs/create_git_tag.rs | 67 +++++++++---- src/safe_outputs/create_pull_request.rs | 83 ++++++++++------ src/safe_outputs/mod.rs | 121 ++++++++++++++++++++++-- 4 files changed, 257 insertions(+), 77 deletions(-) diff --git a/src/safe_outputs/create_branch.rs b/src/safe_outputs/create_branch.rs index 3e8517c2..2acbfd29 100644 --- a/src/safe_outputs/create_branch.rs +++ b/src/safe_outputs/create_branch.rs @@ -199,20 +199,6 @@ impl Executor for CreateBranchResult { ctx: &ExecutionContext, ) -> anyhow::Result { self.sanitize_content_fields(); - let target = match crate::safe_outputs::resolve_repository_write_target( - self.repository.as_deref(), - ctx, - ) { - Ok(target) => target, - Err(failure) => return Ok(failure), - }; - if ctx.dry_run { - return Ok(ExecutionResult::success(format!( - "[DRY-RUN] Would execute: create branch '{}' in {}", - self.branch_name, - target.display_name() - ))); - } self.execute_impl(ctx).await } @@ -220,11 +206,6 @@ impl Executor for CreateBranchResult { info!("Creating branch: '{}'", self.branch_name); debug!("create-branch: branch_name='{}'", self.branch_name); - let token = ctx - .access_token - .as_ref() - .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - let config: CreateBranchConfig = ctx.get_tool_config("create-branch")?; debug!("Branch pattern: {:?}", config.branch_pattern); debug!("Allowed repositories: {:?}", config.allowed_repositories); @@ -285,6 +266,18 @@ impl Executor for CreateBranchResult { ))); } + if ctx.dry_run { + return Ok(ExecutionResult::success(format!( + "[DRY-RUN] Would execute: create branch '{}' in {}", + self.branch_name, + target.display_name() + ))); + } + + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; let client = reqwest::Client::new(); // Resolve the source commit SHA @@ -664,4 +657,36 @@ allowed-source-branches: execution.message ); } + + #[tokio::test] + async fn test_dry_run_enforces_branch_pattern() { + let mut ctx = ExecutionContext { + ado_org_url: Some("https://dev.azure.com/current-org".to_string()), + ado_organization: Some("current-org".to_string()), + ado_project: Some("Current Project".to_string()), + repository_name: Some("self-repo".to_string()), + dry_run: true, + ..Default::default() + }; + ctx.tool_configs.insert( + "create-branch".to_string(), + serde_json::json!({"branch-pattern": "^release/"}), + ); + let mut result = CreateBranchResult { + name: CreateBranchResult::NAME.to_string(), + branch_name: "feature/not-allowed".to_string(), + source_branch: Some("main".to_string()), + source_commit: None, + repository: None, + }; + + let execution = result.execute_sanitized(&ctx).await.unwrap(); + + assert!(!execution.success); + assert!( + execution + .message + .contains("does not match required pattern") + ); + } } diff --git a/src/safe_outputs/create_git_tag.rs b/src/safe_outputs/create_git_tag.rs index cf085b60..9333dd3b 100644 --- a/src/safe_outputs/create_git_tag.rs +++ b/src/safe_outputs/create_git_tag.rs @@ -177,17 +177,17 @@ async fn resolve_head_commit( // Now resolve the HEAD commit of the default branch let url = format!( - "{}/{}/_apis/git/repositories/{}/refs?filter={}&api-version=7.1", + "{}/{}/_apis/git/repositories/{}/refs", org_url.trim_end_matches('/'), utf8_percent_encode(project, PATH_SEGMENT), utf8_percent_encode(repo_name, PATH_SEGMENT), - branch_filter, ); debug!("Resolving HEAD commit via: {}", url); let response = client .get(&url) + .query(&[("filter", branch_filter), ("api-version", "7.1")]) .basic_auth("", Some(token)) .send() .await @@ -227,20 +227,6 @@ impl Executor for CreateGitTagResult { ctx: &ExecutionContext, ) -> anyhow::Result { self.sanitize_content_fields(); - let target = match crate::safe_outputs::resolve_repository_write_target( - self.repository.as_deref(), - ctx, - ) { - Ok(target) => target, - Err(failure) => return Ok(failure), - }; - if ctx.dry_run { - return Ok(ExecutionResult::success(format!( - "[DRY-RUN] Would execute: create git tag '{}' in {}", - self.tag_name, - target.display_name() - ))); - } self.execute_impl(ctx).await } @@ -248,11 +234,6 @@ impl Executor for CreateGitTagResult { info!("Creating git tag: '{}'", self.tag_name); debug!("create-git-tag: tag_name='{}'", self.tag_name); - let token = ctx - .access_token - .as_ref() - .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - let config: CreateGitTagConfig = ctx.get_tool_config("create-git-tag")?; debug!("Tag pattern: {:?}", config.tag_pattern); debug!("Allowed repositories: {:?}", config.allowed_repositories); @@ -294,6 +275,18 @@ impl Executor for CreateGitTagResult { let repo_name = target.qualified_repository(); debug!("Resolved repository target: {}", target.display_name()); + if ctx.dry_run { + return Ok(ExecutionResult::success(format!( + "[DRY-RUN] Would execute: create git tag '{}' in {}", + self.tag_name, + target.display_name() + ))); + } + + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; let client = reqwest::Client::new(); // Resolve commit SHA — use provided value or look up HEAD @@ -603,4 +596,36 @@ message-prefix: "[release] " execution.message ); } + + #[tokio::test] + async fn test_dry_run_enforces_tag_pattern() { + let mut ctx = ExecutionContext { + ado_org_url: Some("https://dev.azure.com/current-org".to_string()), + ado_organization: Some("current-org".to_string()), + ado_project: Some("Current Project".to_string()), + repository_name: Some("self-repo".to_string()), + dry_run: true, + ..Default::default() + }; + ctx.tool_configs.insert( + "create-git-tag".to_string(), + serde_json::json!({"tag-pattern": "^release-"}), + ); + let mut result = CreateGitTagResult { + name: CreateGitTagResult::NAME.to_string(), + tag_name: "version-1".to_string(), + commit: None, + message: Some("Version one".to_string()), + repository: None, + }; + + let execution = result.execute_sanitized(&ctx).await.unwrap(); + + assert!(!execution.success); + assert!( + execution + .message + .contains("does not match required pattern") + ); + } } diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 9f28a30c..35e84e28 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -613,19 +613,6 @@ impl Executor for CreatePrResult { ctx: &ExecutionContext, ) -> anyhow::Result { self.sanitize_content_fields(); - let target = - match crate::safe_outputs::resolve_repository_write_target(Some(&self.repository), ctx) - { - Ok(target) => target, - Err(failure) => return Ok(failure), - }; - if ctx.dry_run { - return Ok(ExecutionResult::success(format!( - "[DRY-RUN] Would execute: create PR: '{}' in {}", - self.title, - target.display_name() - ))); - } self.execute_impl(ctx).await } @@ -706,6 +693,21 @@ impl Executor for CreatePrResult { }; debug!("Resolved repository ID: {}", target.repository_locator()); + let resolved_target_branch = + config.resolve_target_branch(&repository_alias, &ctx.repo_refs); + let all_labels = match validate_and_build_labels(&config, &self.agent_labels) { + Ok(labels) => labels, + Err(result) => return Ok(result), + }; + if ctx.dry_run { + return Ok(ExecutionResult::success(format!( + "[DRY-RUN] Would execute: create PR: '{}' in {} targeting '{}'", + self.title, + target.display_name(), + resolved_target_branch + ))); + } + let token = ctx .access_token .as_ref() @@ -836,8 +838,7 @@ impl Executor for CreatePrResult { // literal `target-branch`. Resolution is shared with the compiler's // prepare-pr-base deepening, so the branch we PR into is the branch that // was fetched/deepened. - let target_branch = config.resolve_target_branch(&repository_alias, &ctx.repo_refs); - let target_branch = target_branch.as_str(); + let target_branch = resolved_target_branch.as_str(); let mut source_branch = self.source_branch.clone(); let mut source_ref = format!("refs/heads/{}", source_branch); let target_ref = format!("refs/heads/{}", target_branch); @@ -1044,11 +1045,7 @@ impl Executor for CreatePrResult { // Get the target branch ref to find the base commit debug!("Getting target branch ref from ADO"); - let refs_url = format!( - "{}/refs?filter=heads/{}&api-version=7.1", - repository_api_base(&target), - target_branch - ); + let refs_url = format!("{}/refs", repository_api_base(&target)); debug!("Refs URL: {}", refs_url); // Resolve the base commit for the push. @@ -1070,6 +1067,10 @@ impl Executor for CreatePrResult { debug!("No recorded base_commit — resolving from ADO refs API"); let refs_response = client .get(&refs_url) + .query(&[ + ("filter", format!("heads/{target_branch}")), + ("api-version", "7.1".to_string()), + ]) .basic_auth("", Some(token)) .send() .await @@ -1101,11 +1102,7 @@ impl Executor for CreatePrResult { // Check if the source branch already exists (e.g. from a retry or previous run). // Retry with new random suffixes up to 3 times. for attempt in 0..3 { - let check_ref_url = format!( - "{}/refs?filter=heads/{}&api-version=7.1", - repository_api_base(&target), - source_branch - ); + let check_ref_url = format!("{}/refs", repository_api_base(&target)); debug!( "Checking if source branch exists (attempt {}): {}", attempt + 1, @@ -1114,6 +1111,10 @@ impl Executor for CreatePrResult { let check_ref_response = client .get(&check_ref_url) + .query(&[ + ("filter", format!("heads/{source_branch}")), + ("api-version", "7.1".to_string()), + ]) .basic_auth("", Some(token)) .send() .await @@ -1203,12 +1204,6 @@ impl Executor for CreatePrResult { ); } - // Validate and add labels (merge operator labels + validated agent labels) - let all_labels = match validate_and_build_labels(&config, &self.agent_labels) { - Ok(labels) => labels, - Err(result) => return Ok(result), - }; - if !all_labels.is_empty() { debug!("Adding {} labels", all_labels.len()); pr_body["labels"] = serde_json::json!( @@ -2716,6 +2711,32 @@ mod tests { assert!(execution.message.contains("repos.organization")); } + #[tokio::test] + async fn test_dry_run_enforces_agent_label_policy() { + let mut ctx = cross_org_ctx(Some("other-org"), true); + ctx.dry_run = true; + ctx.tool_configs.insert( + "create-pull-request".to_string(), + serde_json::json!({"allowed-labels": ["approved"]}), + ); + let mut result = CreatePrResult { + name: CreatePrResult::NAME.to_string(), + title: "Fix bug in parser".to_string(), + description: "This PR fixes a critical bug in the parser module.".to_string(), + source_branch: "agent/fix".to_string(), + patch_file: "patch.diff".to_string(), + repository: "cross-org-repo".to_string(), + agent_labels: vec!["unapproved".to_string()], + base_commit: None, + patch_sha256: "deadbeef".to_string(), + }; + + let execution = result.execute_sanitized(&ctx).await.unwrap(); + + assert!(!execution.success); + assert!(execution.message.contains("not in allowed-labels")); + } + #[test] fn test_target_branches_sanitizes_both_keys_and_values() { use crate::sanitize::SanitizeConfig; diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index 1cf70780..9876cf6e 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -9,7 +9,8 @@ use percent_encoding::{AsciiSet, CONTROLS, utf8_percent_encode}; /// `#` (fragment), `?` (query), `/` (path separator), and space. /// This hardens operator-controlled values (project names, wiki names, work item /// types) against accidental corruption of the URL structure. -pub(crate) const PATH_SEGMENT: &AsciiSet = &CONTROLS.add(b'#').add(b'?').add(b'/').add(b' '); +pub(crate) const PATH_SEGMENT: &AsciiSet = + &CONTROLS.add(b'#').add(b'?').add(b'/').add(b'%').add(b' '); /// Safe output tools that are always available regardless of filtering. /// These are diagnostic/transparency tools that agents should always have access to. @@ -304,17 +305,33 @@ pub(crate) fn input_refers_to_self(input: &str, ctx: &ExecutionContext) -> bool /// exact-key arm of [`lookup_allowed_repository_alias`]), so callers may /// canonicalize defensively without changing the result. /// -/// **Precedence**: self-identity wins over the alias map. A selector matching -/// the pipeline repository's name resolves to `"self"` even if an alias of the -/// same name is configured for a different repository. +/// **Precedence**: literal `"self"`/empty selects self; an exact checkout alias +/// selects that alias; name-based matches are accepted only when they identify +/// exactly one of self or the configured repositories. pub(crate) fn canonical_repository_alias( repository: &str, ctx: &ExecutionContext, ) -> Option { - if input_refers_to_self(repository, ctx) { + if repository == "self" || repository.is_empty() { return Some("self".to_string()); } - lookup_allowed_repository_alias(repository, &ctx.allowed_repositories).cloned() + if ctx.allowed_repositories.contains_key(repository) { + return Some(repository.to_string()); + } + + let mut matches = Vec::new(); + if input_refers_to_self(repository, ctx) { + matches.push("self".to_string()); + } + for (alias, value) in &ctx.allowed_repositories { + let trailing = value.rsplit('/').next().unwrap_or(value); + if value.eq_ignore_ascii_case(repository) || trailing.eq_ignore_ascii_case(repository) { + matches.push(alias.clone()); + } + } + matches.sort(); + matches.dedup(); + (matches.len() == 1).then(|| matches.remove(0)) } #[derive(Debug, Clone)] @@ -464,6 +481,19 @@ pub(crate) fn resolve_repository_write_target( cannot be resolved safely." ))); } + if config.endpoint.is_some() + && config + .organization + .as_deref() + .is_some_and(|organization| organization.eq_ignore_ascii_case(current_organization)) + { + return Err(ExecutionResult::failure(format!( + "Repository '{selector}' (checkout alias '{alias}') uses an endpoint-backed \ + Azure Repos checkout but declares the pipeline's current organization \ + '{current_organization}'. Remove the unnecessary endpoint for a same-organization \ + repository or set `repos.organization` to the actual target organization." + ))); + } let (project, repository_name) = split_repository_target_name(&config.name, current_project)?; @@ -1341,6 +1371,51 @@ mod tests { assert!(target.cross_organization); } + #[test] + fn exact_cross_org_alias_wins_over_self_repository_name() { + let mut ctx = repository_target_ctx(); + ctx.repository_name = Some("Current Project/target".to_string()); + configure_repository_write_context( + &mut ctx, + &["target".to_string()], + vec![RepositoryTargetSpec { + alias: "target".to_string(), + repo_type: "git".to_string(), + name: "Other Project/target-repo".to_string(), + organization: Some("other-org".to_string()), + endpoint: Some("cross-org-checkout".to_string()), + }], + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + &cross_org_allow(), + ); + + let target = resolve_repository_write_target(Some("target"), &ctx).unwrap(); + + assert_eq!(target.organization, "other-org"); + assert_eq!(target.repository, "target-repo"); + } + + #[test] + fn repository_name_collision_between_self_and_alias_is_ambiguous() { + let mut ctx = repository_target_ctx(); + ctx.repository_name = Some("Current Project/shared".to_string()); + configure_repository_write_context( + &mut ctx, + &["other".to_string()], + vec![RepositoryTargetSpec { + alias: "other".to_string(), + repo_type: "git".to_string(), + name: "Other Project/shared".to_string(), + organization: None, + endpoint: None, + }], + None, + &[], + ); + + assert!(canonical_repository_alias("shared", &ctx).is_none()); + } + #[test] fn repository_write_target_rejects_incomplete_or_unauthorized_cross_org() { for (organization, connection_type, allow, expected) in [ @@ -1383,6 +1458,40 @@ mod tests { } } + #[test] + fn repository_write_target_rejects_endpoint_declared_as_current_org() { + let mut ctx = repository_target_ctx(); + configure_repository_write_context( + &mut ctx, + &["target".to_string()], + vec![RepositoryTargetSpec { + alias: "target".to_string(), + repo_type: "git".to_string(), + name: "Other Project/target-repo".to_string(), + organization: Some("current-org".to_string()), + endpoint: Some("cross-org-checkout".to_string()), + }], + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + &cross_org_allow(), + ); + + let error = resolve_repository_write_target(Some("target"), &ctx).unwrap_err(); + + assert!(error.message.contains("declares the pipeline's current organization")); + } + + #[test] + fn path_segment_encodes_literal_percent_sequences() { + assert_eq!( + utf8_percent_encode("Project%2FArchive", PATH_SEGMENT).to_string(), + "Project%252FArchive" + ); + assert_eq!( + utf8_percent_encode("Repo%23Name%3F", PATH_SEGMENT).to_string(), + "Repo%2523Name%253F" + ); + } + #[test] fn test_resolve_repository_checkout_dir_distinguishes_root_and_self() { let mut ctx = ctx_with(Some("4x4/current-repo"), sample_allowed()); From 6c12a3a590d419aae0bedd36abc1d63e85a206dd Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 11:19:13 +0100 Subject: [PATCH 08/18] feat(compile): prepare cross-org pull-request bases securely Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- .../prepare-pr-base/__tests__/index.test.ts | 192 ++++++++++++++- .../ado-script/src/prepare-pr-base/index.ts | 118 +++++++++- .../src/shared/__tests__/auth.test.ts | 22 ++ scripts/ado-script/src/shared/ado-client.ts | 3 +- scripts/ado-script/src/shared/auth.ts | 26 ++- src/compile/agentic_pipeline.rs | 97 +++++++- src/compile/common.rs | 2 +- src/compile/extensions/ado_script.rs | 218 +++++++++++++++--- tests/compiler_tests.rs | 62 +++++ 9 files changed, 675 insertions(+), 65 deletions(-) diff --git a/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts b/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts index b81afa93..313d15cb 100644 --- a/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts +++ b/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts @@ -37,6 +37,7 @@ function dependencies(opts: { deps: PrepareDependencies; calls: Array<{ args: string[]; env?: Record }>; dirs: string[]; + getCommitDiffMetadata: ReturnType; } { const calls: Array<{ args: string[]; env?: Record }> = []; const dirs: string[] = []; @@ -79,6 +80,7 @@ function dependencies(opts: { deps: { runners, chdir, getCommitDiffMetadata }, calls, dirs, + getCommitDiffMetadata, }; } @@ -132,6 +134,50 @@ describe("parseArgs", () => { }); }); + it("parses complete cross-organization repository coordinates", () => { + expect( + parseArgs([ + "--mode", + "patch-base", + "--repo-dir", + "/src", + "--organization", + "other-org", + "--project", + "Other Project", + "--repository", + "target-repo", + "--target-branch", + "main", + ]), + ).toEqual({ + mode: "patch-base", + repos: [{ + dir: "/src", + organization: "other-org", + project: "Other Project", + repository: "target-repo", + sourceRef: undefined, + target: "main", + }], + fallbackTarget: "main", + fallbackSourceRef: undefined, + }); + }); + + it("rejects partial cross-organization repository coordinates", () => { + expect(() => + parseArgs([ + "--repo-dir", + "/src", + "--organization", + "other-org", + "--target-branch", + "main", + ]), + ).toThrow(/must set --organization, --project, and --repository together/); + }); + it("rejects unknown modes", () => { expect(() => parseArgs(["--mode", "everything"])).toThrow(/Unsupported/); }); @@ -172,7 +218,7 @@ describe("prepare-pr-base main", () => { vi.spyOn(process.stdout, "write").mockImplementation(() => true); const error = Object.assign(new Error("forbidden"), { statusCode: 403 }); const { deps, calls } = dependencies({ metadataError: error }); - await main( + const rc = await main( patchArgs(), { SYSTEM_COLLECTIONURI: "https://dev.azure.com/org/" }, deps, @@ -188,6 +234,123 @@ describe("prepare-pr-base main", () => { ); }); + it("uses explicit cross-org coordinates and bearer for REST and fetch", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls, getCommitDiffMetadata } = dependencies({ + remote: "https://dev.azure.com/other-org/Other%20Project/_git/target-repo", + }); + const rc = await main( + { + mode: "patch-base", + repos: [{ + dir: "/src", + target: "main", + sourceRef: "refs/heads/feature", + organization: "other-org", + project: "Other Project", + repository: "target-repo", + }], + fallbackTarget: "main", + }, + { SYSTEM_ACCESSTOKEN: "cross-token" }, + deps, + ); + + expect(getCommitDiffMetadata).toHaveBeenCalledWith( + "Other Project", + "target-repo", + "main", + HEAD, + "https://dev.azure.com/other-org/", + ); + const fetches = calls.filter((call) => call.args[0] === "fetch"); + expect(fetches[0]!.env).toMatchObject({ + GIT_CONFIG_VALUE_0: "Authorization: bearer cross-token", + }); + }); + + it("does not send the bearer when explicit coordinates mismatch the remote", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls, getCommitDiffMetadata } = dependencies({ + remote: "https://dev.azure.com/unexpected/Other%20Project/_git/target-repo", + }); + const rc = await main( + { + mode: "patch-base", + repos: [{ + dir: "/src", + target: "main", + sourceRef: "refs/heads/feature", + organization: "other-org", + project: "Other Project", + repository: "target-repo", + }], + fallbackTarget: "main", + }, + { SYSTEM_ACCESSTOKEN: "must-not-leak" }, + deps, + ); + + expect(getCommitDiffMetadata).not.toHaveBeenCalled(); + expect(calls.some((call) => call.args[0] === "fetch")).toBe(false); + expect(rc).toBe(1); + }); + + it("rejects explicit coordinates that point at the current organization", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls, getCommitDiffMetadata } = dependencies({ + remote: "https://dev.azure.com/org/Project/_git/repo", + }); + const rc = await main( + { + mode: "patch-base", + repos: [{ + dir: "/src", + target: "main", + organization: "org", + project: "Project", + repository: "repo", + }], + fallbackTarget: "main", + }, + { + SYSTEM_COLLECTIONURI: "https://dev.azure.com/org/", + SYSTEM_ACCESSTOKEN: "must-not-leak", + }, + deps, + ); + + expect(getCommitDiffMetadata).not.toHaveBeenCalled(); + expect(calls.some((call) => call.args[0] === "fetch")).toBe(false); + expect(rc).toBe(1); + }); + + it("rejects locale-equivalent but code-point-distinct repository names", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls, getCommitDiffMetadata } = dependencies({ + remote: "https://dev.azure.com/other-org/Other%20Project/_git/%EF%BD%92%EF%BD%85%EF%BD%90%EF%BD%8F", + }); + const rc = await main( + { + mode: "patch-base", + repos: [{ + dir: "/src", + target: "main", + organization: "other-org", + project: "Other Project", + repository: "repo", + }], + fallbackTarget: "main", + }, + { SYSTEM_ACCESSTOKEN: "must-not-leak" }, + deps, + ); + + expect(getCommitDiffMetadata).not.toHaveBeenCalled(); + expect(calls.some((call) => call.args[0] === "fetch")).toBe(false); + expect(rc).toBe(1); + }); + it("prefers the self resource ref over the triggering repository branch", async () => { vi.spyOn(process.stdout, "write").mockImplementation(() => true); const { deps, calls } = dependencies({ @@ -238,6 +401,33 @@ describe("prepare-pr-base main", () => { ]); }); + it("target-worktree sends the bearer to a matching explicit cross-org remote", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls } = dependencies({ + remote: "https://dev.azure.com/other-org/Other%20Project/_git/target-repo", + }); + await main( + { + mode: "target-worktree", + repos: [{ + dir: "/src", + target: "main", + organization: "other-org", + project: "Other Project", + repository: "target-repo", + }], + fallbackTarget: "main", + }, + { SYSTEM_ACCESSTOKEN: "cross-token" }, + deps, + ); + + const fetch = calls.find((call) => call.args[0] === "fetch"); + expect(fetch?.env).toMatchObject({ + GIT_CONFIG_VALUE_0: "Authorization: bearer cross-token", + }); + }); + it("does not send the ADO bearer to a non-Azure origin", async () => { vi.spyOn(process.stdout, "write").mockImplementation(() => true); const { deps, calls } = dependencies({ diff --git a/scripts/ado-script/src/prepare-pr-base/index.ts b/scripts/ado-script/src/prepare-pr-base/index.ts index 5cc63669..f36bf3de 100644 --- a/scripts/ado-script/src/prepare-pr-base/index.ts +++ b/scripts/ado-script/src/prepare-pr-base/index.ts @@ -39,6 +39,9 @@ export interface RepoTarget { dir: string; target: string; sourceRef?: string; + organization?: string; + project?: string; + repository?: string; } export interface PrepareArgs { @@ -56,6 +59,7 @@ export interface PrepareDependencies { repository: string, targetBranch: string, sourceCommit: string, + organizationUrl?: string, ) => Promise; } @@ -77,6 +81,10 @@ function oneLine(value: unknown, maxLength = 500): string { return text.length <= maxLength ? text : `${text.slice(0, maxLength)}...`; } +function sameAdoName(left: string, right: string): boolean { + return left.toLowerCase() === right.toLowerCase(); +} + function flushPending( repos: RepoTarget[], pending: Partial | null, @@ -84,10 +92,24 @@ function flushPending( fallbackSourceRef?: string, ): void { if (!pending?.dir) return; + const targetIdentity = [ + pending.organization, + pending.project, + pending.repository, + ]; + if (targetIdentity.some((value) => value !== undefined) && + targetIdentity.some((value) => value === undefined)) { + throw new Error( + `Repository '${pending.dir}' must set --organization, --project, and --repository together.`, + ); + } repos.push({ dir: pending.dir, target: pending.target ?? fallbackTarget, sourceRef: pending.sourceRef ?? fallbackSourceRef, + organization: pending.organization, + project: pending.project, + repository: pending.repository, }); } @@ -120,6 +142,15 @@ export function parseArgs(argv: string[]): PrepareArgs { if (pending?.dir) pending.target = target; else fallbackTarget = target; i++; + } else if (flag === "--organization") { + if (pending?.dir) pending.organization = value; + i++; + } else if (flag === "--project") { + if (pending?.dir) pending.project = value; + i++; + } else if (flag === "--repository") { + if (pending?.dir) pending.repository = value; + i++; } } flushPending(repos, pending, fallbackTarget, fallbackSourceRef); @@ -167,18 +198,55 @@ async function preparePatchBase( const remote = runners.gitOk(["remote", "get-url", "origin"]) ?? ""; const identity = parseAdoRepoUrl(remote); + const explicitIdentity = + repo.organization && repo.project && repo.repository + ? { + collectionUri: `https://dev.azure.com/${repo.organization}/`, + organization: repo.organization.toLowerCase(), + project: repo.project, + repository: repo.repository, + } + : null; + const identityMatchesExplicit = + identity !== null && + explicitIdentity !== null && + identity.organization === explicitIdentity.organization && + sameAdoName(identity.project, explicitIdentity.project) && + sameAdoName(identity.repository, explicitIdentity.repository); const sameOrgAdo = identity !== null && isCurrentAdoOrganization(identity, env); - const repoFetchEnv = sameOrgAdo ? fetchEnv : {}; + const eligibleIdentity = identityMatchesExplicit + ? explicitIdentity + : sameOrgAdo + ? identity + : null; + const repoFetchEnv = eligibleIdentity ? fetchEnv : {}; + if (explicitIdentity && sameOrgAdo) { + warnRepo( + repo.dir, + repo.target, + "compiler-resolved cross-organization target points at the current Azure DevOps organization", + ); + return false; + } + if (explicitIdentity && !identityMatchesExplicit) { + warnRepo( + repo.dir, + repo.target, + "checkout remote does not match the compiler-resolved Azure DevOps target", + ); + return false; + } const restDisabled = env.ADO_AW_PREPARE_PR_BASE_DISABLE_REST === "1"; if (restDisabled) { restReason = "ADO REST disabled for deterministic fallback testing"; - } else if (identity && sameOrgAdo) { + } else if (eligibleIdentity) { try { const metadata = await deps.getCommitDiffMetadata( - identity.project, - identity.repository, + eligibleIdentity.project, + eligibleIdentity.repository, repo.target, headSha, + eligibleIdentity.collectionUri, ); const exact = ensureExactMergeBaseFetched( repo.target, @@ -227,8 +295,38 @@ function prepareTargetWorktree( ): boolean { const remote = deps.runners.gitOk(["remote", "get-url", "origin"]) ?? ""; const identity = parseAdoRepoUrl(remote); + const explicitMatches = + identity !== null && + repo.organization !== undefined && + repo.project !== undefined && + repo.repository !== undefined && + identity.organization === repo.organization.toLowerCase() && + sameAdoName(identity.project, repo.project) && + sameAdoName(identity.repository, repo.repository); + if ( + repo.organization && + identity && + isCurrentAdoOrganization(identity, env) + ) { + warnRepo( + repo.dir, + repo.target, + "compiler-resolved cross-organization target points at the current Azure DevOps organization", + ); + return false; + } + if (repo.organization && !explicitMatches) { + warnRepo( + repo.dir, + repo.target, + "checkout remote does not match the compiler-resolved Azure DevOps target", + ); + return false; + } const repoFetchEnv = - identity && isCurrentAdoOrganization(identity, env) ? fetchEnv : {}; + identity && (isCurrentAdoOrganization(identity, env) || explicitMatches) + ? fetchEnv + : {}; const fetched = ensureTargetTipFetched(repo.target, repoFetchEnv, deps.runners); if (!fetched.ok) { warnRepo(repo.dir, repo.target, fetched.reason); @@ -258,21 +356,25 @@ export async function main( ]; } const fetchEnv = bearerEnv(env.SYSTEM_ACCESSTOKEN); + let requiredTargetFailed = false; for (const repo of repos) { try { deps.chdir(repo.dir); } catch (err) { warnRepo(repo.dir, repo.target, `could not enter checkout: ${oneLine(err)}`); + if (repo.organization) requiredTargetFailed = true; continue; } + let prepared: boolean; if (args.mode === "target-worktree") { - prepareTargetWorktree(repo, env, fetchEnv, deps); + prepared = prepareTargetWorktree(repo, env, fetchEnv, deps); } else { - await preparePatchBase(repo, env, fetchEnv, deps); + prepared = await preparePatchBase(repo, env, fetchEnv, deps); } + if (!prepared && repo.organization) requiredTargetFailed = true; } - return 0; + return requiredTargetFailed ? 1 : 0; } if ( diff --git a/scripts/ado-script/src/shared/__tests__/auth.test.ts b/scripts/ado-script/src/shared/__tests__/auth.test.ts index 1bc55526..30e5b534 100644 --- a/scripts/ado-script/src/shared/__tests__/auth.test.ts +++ b/scripts/ado-script/src/shared/__tests__/auth.test.ts @@ -51,4 +51,26 @@ describe("getWebApi", () => { const b = await getWebApi(); expect(a).toBe(b); }); + + it("uses a bearer handler for compiler-minted access tokens", async () => { + process.env.SYSTEM_COLLECTIONURI = "https://example.visualstudio.com/"; + process.env.SYSTEM_ACCESSTOKEN = "entra-token"; + process.env.ADO_AW_ACCESS_TOKEN_KIND = "bearer"; + + const api = await getWebApi(); + + expect(api.authHandler.constructor.name).toBe("BearerCredentialHandler"); + }); + + it("retains PAT handling for local/manual tokens", async () => { + process.env.SYSTEM_COLLECTIONURI = "https://example.visualstudio.com/"; + process.env.SYSTEM_ACCESSTOKEN = "pat-token"; + delete process.env.ADO_AW_ACCESS_TOKEN_KIND; + + const api = await getWebApi(); + + expect(api.authHandler.constructor.name).toBe( + "PersonalAccessTokenCredentialHandler", + ); + }); }); diff --git a/scripts/ado-script/src/shared/ado-client.ts b/scripts/ado-script/src/shared/ado-client.ts index 7d3e7c26..4ebf3968 100644 --- a/scripts/ado-script/src/shared/ado-client.ts +++ b/scripts/ado-script/src/shared/ado-client.ts @@ -130,10 +130,11 @@ export async function getCommitDiffMetadata( repositoryId: string, targetBranch: string, sourceCommit: string, + organizationUrl?: string, ): Promise { const sourceSha = requireSha("sourceCommit", sourceCommit); return withRetry("getCommitDiffMetadata", async () => { - const git = await (await getWebApi()).getGitApi(); + const git = await (await getWebApi(organizationUrl)).getGitApi(); const branch = await git.getBranch(repositoryId, targetBranch, project); const targetSha = requireSha("target branch commit", branch.commit?.commitId); const result = await git.getCommitDiffs( diff --git a/scripts/ado-script/src/shared/auth.ts b/scripts/ado-script/src/shared/auth.ts index 889290f2..40baf71a 100644 --- a/scripts/ado-script/src/shared/auth.ts +++ b/scripts/ado-script/src/shared/auth.ts @@ -19,6 +19,8 @@ * * Env-var contract: * - `SYSTEM_ACCESSTOKEN` ← `$(System.AccessToken)` + * - `ADO_AW_ACCESS_TOKEN_KIND=bearer` for compiler-minted OAuth/Entra tokens; + * omitted for local/manual PAT usage * - collection URI ← ADO's auto-injected `SYSTEM_COLLECTIONURI` * (falls back to `SYSTEM_TEAMFOUNDATIONCOLLECTIONURI`). Both are * predefined ADO variables auto-mapped into the env of every script @@ -30,17 +32,16 @@ import * as azdev from "azure-devops-node-api"; import type { WebApi } from "azure-devops-node-api"; import { logError } from "./vso-logger.js"; -let cached: WebApi | undefined; +const cached = new Map(); /** For tests only: clear the cached WebApi. */ export function _resetCacheForTesting(): void { - cached = undefined; + cached.clear(); } -export async function getWebApi(): Promise { - if (cached) return cached; - +export async function getWebApi(organizationUrl?: string): Promise { const orgUrl = + organizationUrl || process.env.SYSTEM_COLLECTIONURI || process.env.SYSTEM_TEAMFOUNDATIONCOLLECTIONURI; const token = process.env.SYSTEM_ACCESSTOKEN; @@ -55,8 +56,17 @@ export async function getWebApi(): Promise { logError(msg); throw new Error(msg); } + const tokenKind = + process.env.ADO_AW_ACCESS_TOKEN_KIND === "bearer" ? "bearer" : "pat"; + const cacheKey = `${tokenKind}:${orgUrl}`; + const existing = cached.get(cacheKey); + if (existing) return existing; - const handler = azdev.getPersonalAccessTokenHandler(token); - cached = new azdev.WebApi(orgUrl, handler); - return cached; + const handler = + tokenKind === "bearer" + ? azdev.getBearerHandler(token) + : azdev.getPersonalAccessTokenHandler(token); + const client = new azdev.WebApi(orgUrl, handler); + cached.set(cacheKey, client); + return client; } diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 18deca37..5463f061 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1310,10 +1310,25 @@ fn build_agent_job( // the SafeOutputs job (issue #1453). warn_create_pr_target_inference(front_matter); let repos = create_pr_prepare_repos(front_matter, &cfg.trigger_repo_directory); - steps.push(super::extensions::ado_script::prepare_pr_base_step_typed( - super::extensions::ado_script::PreparePrBaseMode::PatchBase, - &repos, - )); + let (local_repos, cross_org_repos) = partition_prepare_repos(repos); + if !local_repos.is_empty() { + steps.push(super::extensions::ado_script::prepare_pr_base_step_typed( + super::extensions::ado_script::PreparePrBaseMode::PatchBase, + &local_repos, + crate::compile::ado_bundle::TokenSource::SystemAccessToken, + )); + } + if let Some(service_connection) = + cross_org_prepare_service_connection(front_matter, &cross_org_repos) + { + steps.push( + super::extensions::ado_script::prepare_pr_base_azure_devops_step_typed( + super::extensions::ado_script::PreparePrBaseMode::PatchBase, + &cross_org_repos, + service_connection, + ), + ); + } } // When GitHub App auth is configured, mint the installation token // immediately before the Copilot run; `copilot_env` sources @@ -2357,17 +2372,72 @@ fn create_pr_prepare_repos( // ref characters subject to shell command substitution. source_ref: None, target_branch: pr_cfg.resolve_target_branch("self", &repo_refs), + organization: None, + project: None, + repository: None, }]; for alias in &front_matter.checkout { + let repository = front_matter + .repositories + .iter() + .find(|repository| &repository.repository == alias); + let explicit_target = repository.and_then(|repository| { + let organization = repository.organization.as_ref()?; + let (project, name) = repository.name.split_once('/')?; + let write = front_matter + .permissions + .as_ref() + .and_then(|permissions| permissions.write.as_ref())?; + (write.supports_cross_organization_writes() + && write.allows_repository(organization.as_str(), project, name)) + .then(|| { + ( + organization.as_str().to_string(), + project.to_string(), + name.to_string(), + ) + }) + }); repos.push(PreparePrBaseRepo { dir: format!("$(Build.SourcesDirectory)/{alias}"), source_ref: repo_refs.get(alias).cloned(), target_branch: pr_cfg.resolve_target_branch(alias, &repo_refs), + organization: explicit_target.as_ref().map(|target| target.0.clone()), + project: explicit_target.as_ref().map(|target| target.1.clone()), + repository: explicit_target.map(|target| target.2), }); } repos } +fn cross_org_prepare_service_connection<'a>( + front_matter: &'a FrontMatter, + repos: &[super::extensions::ado_script::PreparePrBaseRepo], +) -> Option<&'a str> { + if !repos.iter().any(|repo| repo.organization.is_some()) { + return None; + } + front_matter + .permissions + .as_ref() + .and_then(|permissions| permissions.write.as_ref()) + .filter(|write| { + write.connection_type() == crate::compile::types::WriteConnectionType::AzureDevOps + }) + .map(crate::compile::types::WritePermissionConfig::service_connection) +} + +fn partition_prepare_repos( + repos: Vec, +) -> ( + Vec, + Vec, +) { + repos + .into_iter() + .partition(|repo| repo.organization.is_none()) +} + /// Emit the compile-time advisory when `create-pull-request`'s /// `infer-target-from-checkout-ref` would resolve a non-branch ref (e.g. a tag) /// as a PR base. `resolve_target_branch` would hand back the whole ref, and @@ -2497,10 +2567,21 @@ fn build_safeoutputs_job( } if variant.runs_create_pull_request { let repos = create_pr_prepare_repos(front_matter, &layout.self_repository_directory); - steps.push(super::extensions::ado_script::prepare_pr_base_step_typed( - super::extensions::ado_script::PreparePrBaseMode::TargetWorktree, - &repos, - )); + let (local_repos, cross_org_repos) = partition_prepare_repos(repos); + if !local_repos.is_empty() { + steps.push(super::extensions::ado_script::prepare_pr_base_step_typed( + super::extensions::ado_script::PreparePrBaseMode::TargetWorktree, + &local_repos, + crate::compile::ado_bundle::TokenSource::SystemAccessToken, + )); + } + if !cross_org_repos.is_empty() { + steps.push(super::extensions::ado_script::prepare_pr_base_step_typed( + super::extensions::ado_script::PreparePrBaseMode::TargetWorktree, + &cross_org_repos, + crate::compile::ado_bundle::TokenSource::WriteServiceConnection, + )); + } } if let Some(app) = github_app { let permissions = diff --git a/src/compile/common.rs b/src/compile/common.rs index ee077144..0edc7574 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2535,7 +2535,7 @@ fn find_git_root(path: &std::path::Path) -> Option { // ==================== Permission helpers ==================== /// ADO resource ID for minting ADO-scoped tokens via Azure CLI. -const ADO_RESOURCE_ID: &str = "499b84ac-1321-427f-aa17-267ca6975798"; +pub(crate) const ADO_RESOURCE_ID: &str = "499b84ac-1321-427f-aa17-267ca6975798"; shell_script! { /// Mint the Stage 1 Azure DevOps bearer inside an authenticated AzureCLI@3 diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index 34846faf..debf45b4 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -176,6 +176,32 @@ node "$PREPARE_PR_BASE_PATH" --mode "$MODE" \ } } +shell_script! { + /// Run cross-organization create-pull-request preparation inside an + /// authenticated AzureCLI@3 task. The short-lived bearer remains a + /// shell-local variable and is projected only into the bundle child. + PREPARE_PR_BASE_AZURE_DEVOPS { + interpreter: Bash, + bindings: [PREPARE_PR_BASE_PATH, MODE, ADO_RESOURCE], + externals: [], + fragments: [repo_flags], + body: r#" +set -eo pipefail +ADO_TOKEN=$(az account get-access-token \ + --resource "$ADO_RESOURCE" \ + --query accessToken -o tsv) +if [ -z "$ADO_TOKEN" ]; then + echo "Azure CLI returned an empty Azure DevOps access token" >&2 + exit 1 +fi +ADO_AW_ACCESS_TOKEN_KIND=bearer SYSTEM_ACCESSTOKEN="$ADO_TOKEN" \ + node "$PREPARE_PR_BASE_PATH" --mode "$MODE" \ +# ado-aw:fragment repo_flags +unset ADO_TOKEN +"#, + } +} + shell_script! { /// The Setup-job synthetic-PR context resolver. Runs the /// `exec-context-pr-synth` bundle; its outputs are declared on the @@ -734,10 +760,7 @@ pub fn github_app_token_step_typed_for( format!("--owner {}", sh_single_quote(&cfg.owner)), // Pin the output variable name (a compiler constant) as argv so no // pipeline variable can redirect the minted token. - format!( - "--output-var {}", - sh_single_quote(output_var) - ), + format!("--output-var {}", sh_single_quote(output_var)), ]; if !cfg.repositories.is_empty() { args.push(format!( @@ -753,8 +776,8 @@ pub fn github_app_token_step_typed_for( .iter() .map(|(name, level)| (name.replace('-', "_"), level.as_str().to_string())) .collect(); - let json = serde_json::to_string(&normalized) - .context("serialize GitHub App token permissions")?; + let json = + serde_json::to_string(&normalized).context("serialize GitHub App token permissions")?; args.push(format!("--permissions-json {}", sh_single_quote(&json))); } let script = ShellScript::new(&MINT_GITHUB_APP_TOKEN) @@ -808,12 +831,15 @@ pub struct PreparePrBaseRepo { pub dir: String, pub source_ref: Option, pub target_branch: String, + pub organization: Option, + pub project: Option, + pub repository: Option, } /// Emit one of the two create-pull-request preparation modes. The Agent job /// uses `PatchBase` to recover and verify the merge-base; SafeOutputs uses /// `TargetWorktree` to fetch only the target tip required by `git worktree add`. -pub fn prepare_pr_base_step_typed(mode: PreparePrBaseMode, repos: &[PreparePrBaseRepo]) -> Step { +fn prepare_pr_base_repo_flags(mode: PreparePrBaseMode, repos: &[PreparePrBaseRepo]) -> String { let repo_flags: String = repos .iter() .map(|repo| { @@ -823,31 +849,84 @@ pub fn prepare_pr_base_step_typed(mode: PreparePrBaseMode, repos: &[PreparePrBas } _ => String::new(), }; + let target_flags = match ( + repo.organization.as_deref(), + repo.project.as_deref(), + repo.repository.as_deref(), + ) { + (Some(organization), Some(project), Some(repository)) => format!( + " --organization {} --project {} --repository {}", + sh_single_quote(organization), + sh_single_quote(project), + sh_single_quote(repository) + ), + _ => String::new(), + }; format!( - " --repo-dir \"{}\"{} --target-branch {}", + " --repo-dir \"{}\"{}{} --target-branch {}", repo.dir, source_flag, + target_flags, sh_single_quote(&repo.target_branch) ) }) .collect(); + repo_flags.trim_start().to_string() +} + +pub fn prepare_pr_base_step_typed( + mode: PreparePrBaseMode, + repos: &[PreparePrBaseRepo], + token_source: crate::compile::ado_bundle::TokenSource, +) -> Step { + let repo_flags = prepare_pr_base_repo_flags(mode, repos); let script = ShellScript::new(&PREPARE_PR_BASE) .bind_text("PREPARE_PR_BASE_PATH", PREPARE_PR_BASE_PATH) .bind_text("MODE", mode.as_arg()) - .fragment("repo_flags", repo_flags.trim_start().to_string()) + .fragment("repo_flags", repo_flags) .render(); let step = crate::compile::ado_bundle::apply_bundle_auth( BashStep::new(mode.display_name(), script).with_condition(Condition::Succeeded), crate::compile::ado_bundle::Bundle::PreparePrBase, - crate::compile::ado_bundle::TokenSource::SystemAccessToken, + token_source, ) .with_env( "ADO_AW_SELF_REPOSITORY_REF", EnvValue::runtime_expression("resources.repositories['self'].ref"), - ); + ) + .with_env("ADO_AW_ACCESS_TOKEN_KIND", EnvValue::literal("bearer")); Step::Bash(step) } +pub fn prepare_pr_base_azure_devops_step_typed( + mode: PreparePrBaseMode, + repos: &[PreparePrBaseRepo], + service_connection: &str, +) -> Step { + let script = ShellScript::new(&PREPARE_PR_BASE_AZURE_DEVOPS) + .bind_text("PREPARE_PR_BASE_PATH", PREPARE_PR_BASE_PATH) + .bind_text("MODE", mode.as_arg()) + .bind_text("ADO_RESOURCE", crate::compile::common::ADO_RESOURCE_ID) + .fragment("repo_flags", prepare_pr_base_repo_flags(mode, repos)) + .render(); + let mut task = crate::compile::ir::tasks::azure_cli::AzureCliV3::new( + crate::compile::ir::tasks::azure_cli::AzureCliV3Connection::AzureDevOps( + service_connection.to_string(), + ), + crate::compile::ir::tasks::azure_cli::ScriptType::Bash, + crate::compile::ir::tasks::azure_cli::ScriptLocation::Inline(script), + ) + .visible_az_login(false) + .with_display_name(format!("Cross-org {}", mode.display_name())) + .into_step(); + task.condition = Some(Condition::Succeeded); + task.env.insert( + "ADO_AW_SELF_REPOSITORY_REF".to_string(), + EnvValue::runtime_expression("resources.repositories['self'].ref"), + ); + Step::Task(task) +} + /// (`DELETE /installation/token`) so it does not remain valid for its full /// ~1h lifetime — matching `actions/create-github-app-token`'s default. /// @@ -888,10 +967,7 @@ pub fn github_app_token_revoke_step_typed_for( let step = BashStep::new(display_name, script) .with_condition(Condition::Always) .with_continue_on_error(true) - .with_env( - "GH_APP_TOKEN", - EnvValue::secret(token_var), - ); + .with_env("GH_APP_TOKEN", EnvValue::secret(token_var)); Ok(Step::Bash(step)) } @@ -1588,19 +1664,15 @@ mod tests { skip_token_revocation: false, permissions: std::collections::BTreeMap::from([ ("issues".to_string(), GithubAppPermissionLevel::Read), - ( - "pull-requests".to_string(), - GithubAppPermissionLevel::Read, - ), + ("pull-requests".to_string(), GithubAppPermissionLevel::Read), ]), }; let Step::Bash(step) = github_app_token_step_typed(&cfg).unwrap() else { panic!("expected bash step"); }; assert!( - step.script.contains( - "--permissions-json '{\"issues\":\"read\",\"pull_requests\":\"read\"}'" - ), + step.script + .contains("--permissions-json '{\"issues\":\"read\",\"pull_requests\":\"read\"}'"), "permissions must be deterministic normalized JSON:\n{}", step.script ); @@ -1786,9 +1858,15 @@ mod tests { dir: "$(Build.SourcesDirectory)".to_string(), source_ref: None, target_branch: "main".to_string(), + organization: None, + project: None, + repository: None, }]; - let Step::Bash(step) = prepare_pr_base_step_typed(PreparePrBaseMode::PatchBase, &repos) - else { + let Step::Bash(step) = prepare_pr_base_step_typed( + PreparePrBaseMode::PatchBase, + &repos, + crate::compile::ado_bundle::TokenSource::SystemAccessToken, + ) else { panic!("expected a bash step"); }; assert_eq!(step.display_name, "Prepare create-pull-request patch base"); @@ -1805,16 +1883,16 @@ mod tests { step.script ); assert!( - step.script.contains("node \"$PREPARE_PR_BASE_PATH\" --mode \"$MODE\""), + step.script + .contains("node \"$PREPARE_PR_BASE_PATH\" --mode \"$MODE\""), "the body must invoke the bundle through the bound path and mode:\n{}", step.script ); // The repo dir (== MCP server bounding_directory) is a double-quoted argv // flag (ADO-macro path convention); its target is a single-quoted literal. assert!( - step.script.contains( - "--repo-dir \"$(Build.SourcesDirectory)\" --target-branch 'main'" - ), + step.script + .contains("--repo-dir \"$(Build.SourcesDirectory)\" --target-branch 'main'"), "must emit typed source/target flags:\n{}", step.script ); @@ -1837,9 +1915,15 @@ mod tests { dir: "$(Build.SourcesDirectory)".to_string(), source_ref: Some("refs/heads/feature".to_string()), target_branch: "release/2.x".to_string(), + organization: None, + project: None, + repository: None, }]; - let Step::Bash(step) = prepare_pr_base_step_typed(PreparePrBaseMode::PatchBase, &repos) - else { + let Step::Bash(step) = prepare_pr_base_step_typed( + PreparePrBaseMode::PatchBase, + &repos, + crate::compile::ado_bundle::TokenSource::SystemAccessToken, + ) else { panic!("expected a bash step"); }; assert!( @@ -1859,20 +1943,32 @@ mod tests { dir: "$(Build.SourcesDirectory)".to_string(), source_ref: None, target_branch: "main".to_string(), + organization: None, + project: None, + repository: None, }, PreparePrBaseRepo { dir: "$(Build.SourcesDirectory)/tools".to_string(), source_ref: Some("refs/heads/release".to_string()), target_branch: "release".to_string(), + organization: None, + project: None, + repository: None, }, PreparePrBaseRepo { dir: "$(Build.SourcesDirectory)/docs".to_string(), source_ref: Some("refs/heads/main".to_string()), target_branch: "gh-pages".to_string(), + organization: None, + project: None, + repository: None, }, ]; - let Step::Bash(step) = prepare_pr_base_step_typed(PreparePrBaseMode::PatchBase, &repos) - else { + let Step::Bash(step) = prepare_pr_base_step_typed( + PreparePrBaseMode::PatchBase, + &repos, + crate::compile::ado_bundle::TokenSource::SystemAccessToken, + ) else { panic!("expected a bash step"); }; assert_eq!( @@ -1903,10 +1999,15 @@ mod tests { dir: "$(Build.SourcesDirectory)".to_string(), source_ref: None, target_branch: "main".to_string(), + organization: None, + project: None, + repository: None, }]; - let Step::Bash(step) = - prepare_pr_base_step_typed(PreparePrBaseMode::TargetWorktree, &repos) - else { + let Step::Bash(step) = prepare_pr_base_step_typed( + PreparePrBaseMode::TargetWorktree, + &repos, + crate::compile::ado_bundle::TokenSource::SystemAccessToken, + ) else { panic!("expected a bash step"); }; assert_eq!( @@ -1921,6 +2022,49 @@ mod tests { assert!(!step.script.contains("--source-ref")); } + #[test] + fn cross_org_prepare_task_keeps_bearer_shell_local() { + let repos = vec![PreparePrBaseRepo { + dir: "$(Build.SourcesDirectory)/target".to_string(), + source_ref: Some("refs/heads/feature".to_string()), + target_branch: "main".to_string(), + organization: Some("other-org".to_string()), + project: Some("Other Project".to_string()), + repository: Some("target-repo".to_string()), + }]; + + let Step::Task(step) = prepare_pr_base_azure_devops_step_typed( + PreparePrBaseMode::PatchBase, + &repos, + "ado-write", + ) else { + panic!("expected an AzureCLI task"); + }; + + assert_eq!(step.task, "AzureCLI@3"); + assert_eq!( + step.inputs.get("connectionType").map(String::as_str), + Some("azureDevOps") + ); + assert_eq!( + step.inputs + .get("azureDevOpsServiceConnection") + .map(String::as_str), + Some("ado-write") + ); + let script = step.inputs.get("inlineScript").unwrap(); + assert!( + script.contains("ADO_AW_ACCESS_TOKEN_KIND=bearer SYSTEM_ACCESSTOKEN=\"$ADO_TOKEN\"") + ); + assert!(script.contains("node \"$PREPARE_PR_BASE_PATH\"")); + assert!(script.contains("--organization 'other-org'")); + assert!(script.contains("--project 'Other Project'")); + assert!(script.contains("--repository 'target-repo'")); + assert!(!script.contains("task.setvariable")); + assert!(!script.contains("SC_WRITE_TOKEN")); + assert!(!step.env.contains_key("SYSTEM_ACCESSTOKEN")); + } + #[test] fn prepare_pr_base_path_consistent_with_download_dir() { assert!(PREPARE_PR_BASE_PATH.starts_with("/tmp/ado-aw-scripts/ado-script/")); @@ -1984,9 +2128,7 @@ mod tests { // closes the quoting exposure the old inline form carried: a // single-quoted assignment cannot break argument parsing. assert!( - resolver - .script - .contains("BASE='$(Build.SourcesDirectory)'") + resolver.script.contains("BASE='$(Build.SourcesDirectory)'") && resolver.script.contains("--base \"$BASE\""), "resolver step must pass --base so trigger-repo-relative markers resolve correctly: {}", resolver.script diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 0fff4b78..5af2449d 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -9785,6 +9785,68 @@ fn test_create_pull_request_emits_prepare_pr_base_step_in_safeoutputs() { ); } +#[test] +fn test_cross_org_create_pull_request_preparation_is_credential_isolated() { + let compiled = compile_inline_agent( + "prepare-pr-base-cross-org", + r#"--- +name: "Cross-org PR Agent" +description: "opens a cross-org PR" +permissions: + write: + service-connection: ado-write + connection-type: azureDevOps + allow: + - organization: other-org + projects: + - project: Other Project + repositories: [target-repo] +repos: + - name: Other Project/target-repo + alias: target + organization: other-org + endpoint: cross-org-checkout +safe-outputs: + create-pull-request: + target-branch: main +--- + +Create a pull request in target. +"#, + ); + let agent = job_block(&compiled, "Agent"); + let safeoutputs = job_block(&compiled, "SafeOutputs"); + + assert!(agent.contains("task: AzureCLI@3"), "{agent}"); + assert!(agent.contains("connectionType: azureDevOps"), "{agent}"); + assert!( + agent.contains("azureDevOpsServiceConnection: ado-write"), + "{agent}" + ); + assert!( + agent.contains("ADO_AW_ACCESS_TOKEN_KIND=bearer SYSTEM_ACCESSTOKEN=\"$ADO_TOKEN\""), + "{agent}" + ); + assert!(agent.contains("--organization 'other-org'"), "{agent}"); + assert!(agent.contains("--project 'Other Project'"), "{agent}"); + assert!(agent.contains("--repository 'target-repo'"), "{agent}"); + assert!(!agent.contains("SC_WRITE_TOKEN"), "{agent}"); + + assert!( + safeoutputs.contains("variable=SC_WRITE_TOKEN;issecret=true"), + "{safeoutputs}" + ); + assert!( + safeoutputs.contains("SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)"), + "{safeoutputs}" + ); + assert!( + safeoutputs.contains("--organization 'other-org'"), + "{safeoutputs}" + ); + assert!(!compiled.contains("persistCredentials: true"), "{compiled}"); +} + /// The SafeOutputs-job prepare step honours per-repo targets identically to the /// Agent-job step (shared `create_pr_prepare_repos` resolver), so the branch it /// deepens always matches the branch the executor opens the PR into. From 180f90e8c03cab54ae33983b355e978018609207 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 11:25:55 +0100 Subject: [PATCH 09/18] fix(compile): report cross-org write readiness Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/compile/common.rs | 112 ++++++++++++++++++++++++++++++++++++++++++ src/compile/mod.rs | 27 +--------- src/compile/types.rs | 75 ---------------------------- src/main.rs | 4 +- 4 files changed, 115 insertions(+), 103 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 0edc7574..34cb162b 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -704,6 +704,118 @@ pub fn validate_permissions_write_policy(front_matter: &FrontMatter) -> Result<( options.validate() } +pub fn repository_write_readiness_warnings(front_matter: &FrontMatter) -> Vec { + let configured_tools: Vec<&str> = ["create-pull-request", "create-branch", "create-git-tag"] + .into_iter() + .filter(|tool| front_matter.safe_outputs.contains_key(*tool)) + .collect(); + if configured_tools.is_empty() { + return Vec::new(); + } + let write = front_matter + .permissions + .as_ref() + .and_then(|permissions| permissions.write.as_ref()); + let mut warnings = Vec::new(); + for repository in front_matter.repositories.iter().filter(|repository| { + repository.repo_type.eq_ignore_ascii_case("git") + && repository.endpoint.is_some() + && front_matter + .checkout + .iter() + .any(|alias| alias == &repository.repository) + }) { + let alias = &repository.repository; + let tools = configured_tools.join(", "); + let Some(organization) = repository.organization.as_ref() else { + warnings.push(format!( + "repository-write safe output(s) {tools} may target checkout alias '{alias}', \ + but its endpoint-backed Azure Repos entry has no `organization:`. Add the \ + target Azure DevOps organization; dry-run and runtime reject this alias." + )); + continue; + }; + let Some(write) = write else { + warnings.push(format!( + "repository-write safe output(s) {tools} may target cross-organization alias \ + '{alias}', but `permissions.write` is not configured. Add expanded \ + `permissions.write` with `connection-type: azureDevOps` and an explicit allow \ + scope; dry-run and runtime reject this alias." + )); + continue; + }; + if !write.supports_cross_organization_writes() { + warnings.push(format!( + "repository-write safe output(s) {tools} may target cross-organization alias \ + '{alias}', but `permissions.write` uses `connection-type: {}`. Cross-organization \ + writes require `connection-type: azureDevOps`; dry-run and runtime reject this alias.", + write.connection_type().as_ado_str() + )); + continue; + } + let Some((project, name)) = repository.name.split_once('/') else { + continue; + }; + if !write.allows_repository(organization.as_str(), project, name) { + warnings.push(format!( + "repository-write safe output(s) {tools} may target \ + '{}/{}/{}' (alias '{alias}'), but that repository is not listed in \ + `permissions.write.allow`; dry-run and runtime reject this alias.", + organization.as_str(), + project, + name + )); + } + } + warnings.sort(); + warnings +} + +#[cfg(test)] +fn readiness_warnings(source: &str) -> Vec { + let (mut front_matter, _) = parse_markdown(source).unwrap(); + let (repositories, checkout, checkout_fetch) = resolve_repos(&front_matter).unwrap(); + front_matter.repositories = repositories; + front_matter.checkout = checkout; + front_matter.checkout_fetch = checkout_fetch; + repository_write_readiness_warnings(&front_matter) +} + +#[test] +fn repository_write_readiness_warning_matrix() { + let prefix = "---\nname: test\ndescription: test\n"; + let repo_without_org = "repos:\n - name: Other Project/target-repo\n alias: target\n endpoint: checkout\nsafe-outputs:\n create-branch: {}\n---\n"; + let warnings = readiness_warnings(&format!("{prefix}{repo_without_org}")); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("no `organization:`"), "{warnings:?}"); + + let cross_repo = "repos:\n - name: Other Project/target-repo\n alias: target\n organization: other-org\n endpoint: checkout\nsafe-outputs:\n create-pull-request: {}\n create-branch: {}\n create-git-tag: {}\n---\n"; + let warnings = readiness_warnings(&format!("{prefix}{cross_repo}")); + assert_eq!(warnings.len(), 1); + assert!( + warnings[0].contains("permissions.write") && warnings[0].contains("create-git-tag"), + "{warnings:?}" + ); + + let arm = + "permissions:\n write:\n service-connection: write\n connection-type: azureRM\n"; + let warnings = readiness_warnings(&format!("{prefix}{arm}{cross_repo}")); + assert_eq!(warnings.len(), 1); + assert!( + warnings[0].contains("connection-type: azureRM"), + "{warnings:?}" + ); + + let missing_scope = + "permissions:\n write:\n service-connection: write\n connection-type: azureDevOps\n"; + let warnings = readiness_warnings(&format!("{prefix}{missing_scope}{cross_repo}")); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("not listed"), "{warnings:?}"); + + let ready = "permissions:\n write:\n service-connection: write\n connection-type: azureDevOps\n allow:\n - organization: other-org\n projects:\n - project: Other Project\n repositories: [target-repo]\n"; + assert!(readiness_warnings(&format!("{prefix}{ready}{cross_repo}")).is_empty()); +} + /// Validate the `variable-groups:` front-matter block (issue #1385). /// /// Enforces two rules before the pipeline is built: diff --git a/src/compile/mod.rs b/src/compile/mod.rs index 85b0158d..c1c9aab9 100644 --- a/src/compile/mod.rs +++ b/src/compile/mod.rs @@ -224,31 +224,8 @@ async fn compile_pipeline_inner( // Validate checkout list against repositories common::validate_checkout_list(&front_matter.repositories, &front_matter.checkout)?; - // Cross-organization `create-pull-request` advisory (warning-only): a - // `repos:` alias whose `type: git` entry sets `endpoint:` lives in a - // different Azure DevOps organization than the pipeline. Stage 3 composes - // every ADO Git REST call from the pipeline's own organization/project, so - // `create-pull-request` cannot yet target such an alias — surface this at - // compile time rather than as a confusing runtime 404 (see issue #1934). - if front_matter.safe_outputs.contains_key("create-pull-request") { - let cross_org_aliases = front_matter.checkout_cross_organization_repo_aliases(); - if !cross_org_aliases.is_empty() { - let mut aliases: Vec<&String> = cross_org_aliases.iter().collect(); - aliases.sort(); - let aliases = aliases - .iter() - .map(|a| a.as_str()) - .collect::>() - .join(", "); - eprintln!( - "Warning: create-pull-request is enabled and repos: checks out {aliases} from \ - another Azure DevOps organization (a `type: git` entry with `endpoint:` set). \ - create-pull-request cannot yet target a cross-organization repository — it \ - composes every Git API call against this pipeline's own organization and \ - project, so a call against {aliases} will fail at runtime even though checkout \ - succeeds." - ); - } + for warning in common::repository_write_readiness_warnings(&front_matter) { + eprintln!("Warning: {warning}"); } // Checkout-aware path-layout advisories (warning-only): surface diff --git a/src/compile/types.rs b/src/compile/types.rs index fc434dcc..26528d67 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -2668,26 +2668,6 @@ impl FrontMatter { }) } - /// Checked-out repo aliases whose `repos:` entry is `type: git` with an - /// `endpoint:` set — the documented signal that the repository lives in a - /// **different** Azure DevOps organization than the pipeline (same-org - /// Azure Repos `git` entries never need an `endpoint:`, per the `repos:` - /// field reference in `docs/front-matter.md`). Stage 3 composes every ADO - /// Git REST call from the pipeline's own organization/project, so these - /// aliases cannot yet be targeted by repo-write safe outputs like - /// `create-pull-request`. - pub fn checkout_cross_organization_repo_aliases(&self) -> std::collections::HashSet { - self.repositories - .iter() - .filter(|r| { - r.repo_type == "git" - && r.endpoint.is_some() - && self.checkout.iter().any(|a| a == &r.repository) - }) - .map(|r| r.repository.clone()) - .collect() - } - /// Map each checked-out repo alias to its `repos: ref`, for resolving a /// per-repo create-pull-request target branch. `self` is intentionally /// absent (its ref is the runtime trigger branch, not a static `repos:` ref). @@ -7603,61 +7583,6 @@ Body assert!(fm.create_pr_config().is_none()); } - #[test] - fn test_checkout_cross_organization_repo_aliases_flags_endpoint_git_repos() { - let content = r#"--- -name: "Cross-org Agent" -description: "x" -repos: - - One/azlocal-overlay - - name: AzureForOperatorsIndustry/nc-api-testing - ref: refs/heads/main - endpoint: afoi-x-org-pipeline - - name: AzureForOperatorsIndustry/nc-resource-testing - ref: refs/heads/main - endpoint: afoi-x-org-pipeline - checkout: false -safe-outputs: - create-pull-request: ---- - -Body -"#; - let (mut fm, _) = super::super::common::parse_markdown(content).unwrap(); - let (repos, checkout, checkout_fetch) = super::super::common::resolve_repos(&fm).unwrap(); - fm.repositories = repos; - fm.checkout = checkout; - fm.checkout_fetch = checkout_fetch; - - let cross_org = fm.checkout_cross_organization_repo_aliases(); - // Only checked-out aliases participate; `nc-resource-testing` opts out - // of checkout, so it must not appear even though it has an `endpoint:`. - assert_eq!( - cross_org, - std::collections::HashSet::from(["nc-api-testing".to_string()]) - ); - } - - #[test] - fn test_checkout_cross_organization_repo_aliases_empty_for_same_org_repos() { - let content = r#"--- -name: "Same-org Agent" -description: "x" -repos: - - One/azlocal-overlay ---- - -Body -"#; - let (mut fm, _) = super::super::common::parse_markdown(content).unwrap(); - let (repos, checkout, checkout_fetch) = super::super::common::resolve_repos(&fm).unwrap(); - fm.repositories = repos; - fm.checkout = checkout; - fm.checkout_fetch = checkout_fetch; - - assert!(fm.checkout_cross_organization_repo_aliases().is_empty()); - } - #[test] fn test_front_matter_safe_outputs_noop_object_form() { // `noop: {}` must parse as an empty mapping — distinct from diff --git a/src/main.rs b/src/main.rs index 9d263c4b..bb834fbb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -825,9 +825,7 @@ struct ResolvedExecutionRepository { /// `"git"` for resolved-config JSON emitted before this field existed. #[serde(default = "default_resolved_repo_type", rename = "type")] repo_type: String, - /// Service connection name, when set. Present alongside `type: git` only - /// for a repository in a different Azure DevOps organization — see - /// `FrontMatter::checkout_cross_organization_repo_aliases`. + /// Checkout service connection name, when set. #[serde(default)] endpoint: Option, #[serde(default)] From 9c2cc3b1635c625b7d40d3c2feaa6907e9bb4728 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 11:52:52 +0100 Subject: [PATCH 10/18] fix(safe-outputs): enforce cross-org auth and dry-run parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- src/safe_outputs/create_branch.rs | 36 ++++--- src/safe_outputs/create_git_tag.rs | 50 +++++---- src/safe_outputs/create_pull_request.rs | 133 ++++++++++++++---------- src/safe_outputs/mod.rs | 73 ++++++++++--- 4 files changed, 186 insertions(+), 106 deletions(-) diff --git a/src/safe_outputs/create_branch.rs b/src/safe_outputs/create_branch.rs index 2acbfd29..c8dfb443 100644 --- a/src/safe_outputs/create_branch.rs +++ b/src/safe_outputs/create_branch.rs @@ -135,6 +135,7 @@ async fn resolve_branch_to_commit( org_url: &str, project: &str, token: &str, + connection_type: Option, repo_name: &str, branch: &str, ) -> anyhow::Result { @@ -146,16 +147,17 @@ async fn resolve_branch_to_commit( ); debug!("Resolving branch '{}' via: {}", branch, url); - let response = client - .get(&url) - .query(&[ + let response = crate::safe_outputs::authenticate_ado_request( + client.get(&url).query(&[ ("filter", format!("heads/{}", branch).as_str()), ("api-version", "7.1"), - ]) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to query refs API")?; + ]), + token, + connection_type, + ) + .send() + .await + .context("Failed to query refs API")?; if !response.status().is_success() { let status = response.status(); @@ -291,6 +293,7 @@ impl Executor for CreateBranchResult { &target.organization_url, &target.project, token, + ctx.write_connection_type, target.repository_locator(), source_branch, ) @@ -321,14 +324,15 @@ impl Executor for CreateBranchResult { }]); info!("Creating branch '{}' from commit {}", ref_name, source_sha); - let response = client - .post(&url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&ref_updates) - .send() - .await - .context("Failed to send request to Azure DevOps")?; + let response = crate::safe_outputs::authenticate_ado_request( + client.post(&url).header("Content-Type", "application/json"), + token, + ctx.write_connection_type, + ) + .json(&ref_updates) + .send() + .await + .context("Failed to send request to Azure DevOps")?; if response.status().is_success() { let body: serde_json::Value = response diff --git a/src/safe_outputs/create_git_tag.rs b/src/safe_outputs/create_git_tag.rs index 9333dd3b..2a28d624 100644 --- a/src/safe_outputs/create_git_tag.rs +++ b/src/safe_outputs/create_git_tag.rs @@ -132,6 +132,7 @@ async fn resolve_head_commit( org_url: &str, project: &str, token: &str, + connection_type: Option, repo_name: &str, ) -> anyhow::Result { // First, discover the default branch from the repository metadata @@ -143,12 +144,14 @@ async fn resolve_head_commit( ); debug!("Fetching repository metadata: {}", repo_url); - let repo_response = client - .get(&repo_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to query repository metadata")?; + let repo_response = crate::safe_outputs::authenticate_ado_request( + client.get(&repo_url), + token, + connection_type, + ) + .send() + .await + .context("Failed to query repository metadata")?; ensure!( repo_response.status().is_success(), @@ -185,13 +188,16 @@ async fn resolve_head_commit( debug!("Resolving HEAD commit via: {}", url); - let response = client - .get(&url) - .query(&[("filter", branch_filter), ("api-version", "7.1")]) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to query refs for HEAD resolution")?; + let response = crate::safe_outputs::authenticate_ado_request( + client + .get(&url) + .query(&[("filter", branch_filter), ("api-version", "7.1")]), + token, + connection_type, + ) + .send() + .await + .context("Failed to query refs for HEAD resolution")?; ensure!( response.status().is_success(), @@ -299,6 +305,7 @@ impl Executor for CreateGitTagResult { &target.organization_url, &target.project, token, + ctx.write_connection_type, target.repository_locator(), ) .await? @@ -332,14 +339,15 @@ impl Executor for CreateGitTagResult { }); info!("Sending annotated tag creation request to ADO"); - let response = client - .post(&url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&body) - .send() - .await - .context("Failed to send request to Azure DevOps")?; + let response = crate::safe_outputs::authenticate_ado_request( + client.post(&url).header("Content-Type", "application/json"), + token, + ctx.write_connection_type, + ) + .json(&body) + .send() + .await + .context("Failed to send request to Azure DevOps")?; if response.status().is_success() { let resp_body: serde_json::Value = response diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 35e84e28..3eab6fb8 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -154,6 +154,7 @@ async fn resolve_reviewer_identity( client: &reqwest::Client, organization: &str, token: &str, + connection_type: Option, reviewer: &str, ) -> Option { if is_reviewer_guid(reviewer) { @@ -178,12 +179,14 @@ async fn resolve_reviewer_identity( } }); - let resp = match client - .post(&identity_url) - .basic_auth("", Some(token)) - .json(&query_body) - .send() - .await + let resp = match crate::safe_outputs::authenticate_ado_request( + client.post(&identity_url), + token, + connection_type, + ) + .json(&query_body) + .send() + .await { Ok(resp) => resp, Err(e) => { @@ -1065,16 +1068,17 @@ impl Executor for CreatePrResult { recorded.clone() } else { debug!("No recorded base_commit — resolving from ADO refs API"); - let refs_response = client - .get(&refs_url) - .query(&[ + let refs_response = crate::safe_outputs::authenticate_ado_request( + client.get(&refs_url).query(&[ ("filter", format!("heads/{target_branch}")), ("api-version", "7.1".to_string()), - ]) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to get target branch ref")?; + ]), + token, + ctx.write_connection_type, + ) + .send() + .await + .context("Failed to get target branch ref")?; if !refs_response.status().is_success() { let status = refs_response.status(); @@ -1109,16 +1113,17 @@ impl Executor for CreatePrResult { check_ref_url ); - let check_ref_response = client - .get(&check_ref_url) - .query(&[ + let check_ref_response = crate::safe_outputs::authenticate_ado_request( + client.get(&check_ref_url).query(&[ ("filter", format!("heads/{source_branch}")), ("api-version", "7.1".to_string()), - ]) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to check source branch existence")?; + ]), + token, + ctx.write_connection_type, + ) + .send() + .await + .context("Failed to check source branch existence")?; if check_ref_response.status().is_success() { let check_data: serde_json::Value = check_ref_response.json().await?; @@ -1147,6 +1152,7 @@ impl Executor for CreatePrResult { client: &client, push_url: &push_url, token, + connection_type: ctx.write_connection_type, source_branch, source_ref, changes: &changes, @@ -1214,13 +1220,15 @@ impl Executor for CreatePrResult { ); } - let pr_response = client - .post(&pr_url) - .basic_auth("", Some(token)) - .json(&pr_body) - .send() - .await - .context("Failed to create pull request")?; + let pr_response = crate::safe_outputs::authenticate_ado_request( + client.post(&pr_url), + token, + ctx.write_connection_type, + ) + .json(&pr_body) + .send() + .await + .context("Failed to create pull request")?; if !pr_response.status().is_success() { let status = pr_response.status(); @@ -1286,6 +1294,7 @@ impl Executor for CreatePrResult { target: &target, pr_id, token, + connection_type: ctx.write_connection_type, }; set_pr_completion_options(&pr_ctx, pr_data["createdBy"]["id"].as_str()).await; add_reviewers_to_pr(&pr_ctx).await; @@ -1500,6 +1509,7 @@ struct PushBranchParams<'a> { client: &'a reqwest::Client, push_url: &'a str, token: &'a str, + connection_type: Option, source_branch: String, source_ref: String, changes: &'a [serde_json::Value], @@ -1518,6 +1528,7 @@ async fn push_new_branch( client, push_url, token, + connection_type, mut source_branch, mut source_ref, changes, @@ -1530,13 +1541,15 @@ async fn push_new_branch( serde_json::to_string_pretty(&push_body).unwrap_or_default() ); - let push_response = client - .post(push_url) - .basic_auth("", Some(token)) - .json(&push_body) - .send() - .await - .context("Failed to push changes")?; + let push_response = crate::safe_outputs::authenticate_ado_request( + client.post(push_url), + token, + connection_type, + ) + .json(&push_body) + .send() + .await + .context("Failed to push changes")?; if push_response.status().is_success() { return Ok(Ok((source_branch, source_ref))); @@ -1556,13 +1569,15 @@ async fn push_new_branch( info!("Retrying push with branch '{}'", source_branch); let retry_body = build_push_payload(&source_ref, effective_title, changes, base_commit); - let retry_response = client - .post(push_url) - .basic_auth("", Some(token)) - .json(&retry_body) - .send() - .await - .context("Failed to push changes (retry)")?; + let retry_response = crate::safe_outputs::authenticate_ado_request( + client.post(push_url), + token, + connection_type, + ) + .json(&retry_body) + .send() + .await + .context("Failed to push changes (retry)")?; if !retry_response.status().is_success() { let retry_status = retry_response.status(); @@ -1698,6 +1713,7 @@ struct PrContext<'a> { target: &'a crate::safe_outputs::result::AdoRepositoryTarget, pr_id: i64, token: &'a str, + connection_type: Option, } /// Set PR completion options (delete-source-branch, squash-merge) and optionally @@ -1733,13 +1749,14 @@ async fn set_pr_completion_options(ctx: &PrContext<'_>, pr_created_by_id: Option } } - match ctx - .client - .patch(&pr_update_url) - .basic_auth("", Some(ctx.token)) - .json(&update_body) - .send() - .await + match crate::safe_outputs::authenticate_ado_request( + ctx.client.patch(&pr_update_url), + ctx.token, + ctx.connection_type, + ) + .json(&update_body) + .send() + .await { Ok(resp) if resp.status().is_success() => { debug!("PR completion options set successfully"); @@ -1771,6 +1788,7 @@ async fn add_reviewers_to_pr(ctx: &PrContext<'_>) { ctx.client, &ctx.target.organization, ctx.token, + ctx.connection_type, reviewer, ) .await @@ -1793,13 +1811,14 @@ async fn add_reviewers_to_pr(ctx: &PrContext<'_>) { ); let reviewer_body = serde_json::json!({ "vote": 0, "isRequired": false }); - match ctx - .client - .put(&reviewer_url) - .basic_auth("", Some(ctx.token)) - .json(&reviewer_body) - .send() - .await + match crate::safe_outputs::authenticate_ado_request( + ctx.client.put(&reviewer_url), + ctx.token, + ctx.connection_type, + ) + .json(&reviewer_body) + .send() + .await { Ok(resp) if resp.status().is_success() => { debug!( diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index 9876cf6e..cc3d0da2 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -351,6 +351,18 @@ pub(crate) fn repository_write_scope_key( format!("{organization}/{project}/{repository}").to_ascii_lowercase() } +pub(crate) fn authenticate_ado_request( + request: reqwest::RequestBuilder, + token: &str, + connection_type: Option, +) -> reqwest::RequestBuilder { + if connection_type == Some(crate::compile::types::WriteConnectionType::AzureDevOps) { + request.bearer_auth(token) + } else { + request.basic_auth("", Some(token)) + } +} + pub(crate) fn configure_repository_write_context( ctx: &mut ExecutionContext, checkout: &[String], @@ -1492,6 +1504,40 @@ mod tests { ); } + #[test] + fn ado_request_auth_matches_connection_type() { + let client = reqwest::Client::new(); + let bearer = authenticate_ado_request( + client.get("https://example.test"), + "entra-token", + Some(crate::compile::types::WriteConnectionType::AzureDevOps), + ) + .build() + .unwrap(); + assert_eq!( + bearer + .headers() + .get(reqwest::header::AUTHORIZATION) + .and_then(|value| value.to_str().ok()), + Some("Bearer entra-token") + ); + + let basic = authenticate_ado_request( + client.get("https://example.test"), + "pat-token", + Some(crate::compile::types::WriteConnectionType::AzureRm), + ) + .build() + .unwrap(); + assert_eq!( + basic + .headers() + .get(reqwest::header::AUTHORIZATION) + .and_then(|value| value.to_str().ok()), + Some("Basic OnBhdC10b2tlbg==") + ); + } + #[test] fn test_resolve_repository_checkout_dir_distinguishes_root_and_self() { let mut ctx = ctx_with(Some("4x4/current-repo"), sample_allowed()); @@ -1542,20 +1588,23 @@ mod tests { } #[test] - fn test_resolve_repo_name_self_by_repository_name() { + fn test_resolve_repo_name_rejects_self_alias_name_collision() { let ctx = ctx_with(Some("4x4/sdk-FtdiDeviceControl"), sample_allowed()); - // Trailing-name match on ctx.repository_name (case-insensitive) - assert_eq!( - resolve_repo_name(Some("sdk-FtdiDeviceControl"), &ctx).unwrap(), - "4x4/sdk-FtdiDeviceControl" - ); - assert_eq!( - resolve_repo_name(Some("sdk-ftdidevicecontrol"), &ctx).unwrap(), - "4x4/sdk-FtdiDeviceControl" - ); - // Full-value match on ctx.repository_name (case-insensitive) + for selector in [ + "sdk-FtdiDeviceControl", + "sdk-ftdidevicecontrol", + "4X4/sdk-ftdidevicecontrol", + ] { + let error = resolve_repo_name(Some(selector), &ctx).unwrap_err(); + assert!( + error.message.contains("not in the allowed repository list"), + "{}", + error.message + ); + } + // Exact checkout aliases still win over name-based ambiguity. assert_eq!( - resolve_repo_name(Some("4X4/sdk-ftdidevicecontrol"), &ctx).unwrap(), + resolve_repo_name(Some("repo-sdk-ftdidevicecontrol"), &ctx).unwrap(), "4x4/sdk-FtdiDeviceControl" ); } From 059eb79eb8a35d15a6c1aba8b51bc24f69c21e59 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 11:53:15 +0100 Subject: [PATCH 11/18] test(safe-outputs): cover cross-org repository writes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- .../executor-e2e/__tests__/ado-rest.test.ts | 22 +++ .../__tests__/cross-org-scenarios.test.ts | 71 ++++++++ .../__tests__/execute-cli.test.ts | 31 ++++ .../src/executor-e2e/__tests__/index.test.ts | 3 + .../ado-script/src/executor-e2e/ado-rest.ts | 11 +- .../src/executor-e2e/execute-cli.ts | 27 ++- scripts/ado-script/src/executor-e2e/runner.ts | 4 +- .../ado-script/src/executor-e2e/scenario.ts | 24 +++ .../scenarios/create-pull-request.ts | 63 +++++-- .../src/executor-e2e/scenarios/cross-org.ts | 169 ++++++++++++++++++ .../src/executor-e2e/scenarios/index.ts | 2 + tests/executor-e2e/README.md | 33 ++++ tests/executor-e2e/azure-pipelines.yml | 12 ++ 13 files changed, 453 insertions(+), 19 deletions(-) create mode 100644 scripts/ado-script/src/executor-e2e/__tests__/cross-org-scenarios.test.ts create mode 100644 scripts/ado-script/src/executor-e2e/scenarios/cross-org.ts diff --git a/scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts index 9c6a72e2..5c3cba3e 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts @@ -19,6 +19,28 @@ describe("AdoRest.workItemTypeExists", () => { vi.unstubAllGlobals(); }); + describe("AdoRest authentication", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("uses Bearer auth when requested", async () => { + const fetchMock = stubFetch( + () => + new Response(JSON.stringify({ id: "repo" }), { + status: 200, + headers: { "content-type": "application/json" }, + }), + ); + + await new AdoRest({ ...options, authKind: "bearer" }).getRepository("repo"); + + expect(fetchMock.mock.calls[0]?.[1]).toMatchObject({ + headers: expect.objectContaining({ Authorization: "Bearer token" }), + }); + }); + }); + it("resolves true and encodes the project and type segments", async () => { const fetchMock = stubFetch( () => diff --git a/scripts/ado-script/src/executor-e2e/__tests__/cross-org-scenarios.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/cross-org-scenarios.test.ts new file mode 100644 index 00000000..69759fc2 --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/__tests__/cross-org-scenarios.test.ts @@ -0,0 +1,71 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import type { ScenarioContext } from "../scenario.js"; +import { SkipError } from "../scenario.js"; +import { + crossOrgSource, + resolveCrossOrgEnv, +} from "../scenarios/cross-org.js"; + +function fakeCtx(): ScenarioContext { + return { + orgUrl: "https://dev.azure.com/current/", + project: "Current", + adoRepo: "repo", + buildId: "77", + token: "token", + adoAwBin: "ado-aw", + workDir: "/tmp", + rest: {} as ScenarioContext["rest"], + log: () => {}, + prefix: (tool) => `ado-aw-det-77-${tool}`, + }; +} + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +describe("cross-org executor environment", () => { + it("skips when the pre-provisioned infrastructure is absent", () => { + expect(() => resolveCrossOrgEnv(fakeCtx())).toThrow(SkipError); + }); + + it("builds one exact repository and write scope from environment", () => { + vi.stubEnv("EXECUTOR_E2E_CROSS_ORG_ORGANIZATION", "other-org"); + vi.stubEnv("EXECUTOR_E2E_CROSS_ORG_PROJECT", "Other Project"); + vi.stubEnv("EXECUTOR_E2E_CROSS_ORG_REPOSITORY", "target-repo"); + vi.stubEnv("EXECUTOR_E2E_CROSS_ORG_ENDPOINT", "ado-write"); + vi.stubEnv("EXECUTOR_E2E_CROSS_ORG_TOKEN", "entra-token"); + + const env = resolveCrossOrgEnv(fakeCtx()); + const source = crossOrgSource(env); + + expect(env.orgUrl).toBe("https://dev.azure.com/other-org/"); + expect(source.repositories).toEqual([{ + name: "Other Project/target-repo", + alias: "cross-org-target", + organization: "other-org", + endpoint: "ado-write", + }]); + expect(source.writePermissions).toEqual({ + serviceConnection: "ado-write", + connectionType: "azureDevOps", + allow: [{ + organization: "other-org", + projects: [{ + project: "Other Project", + repositories: ["target-repo"], + }], + }], + }); + }); + + it("treats unexpanded pipeline macros as absent", () => { + vi.stubEnv( + "EXECUTOR_E2E_CROSS_ORG_ORGANIZATION", + "$(EXECUTOR_E2E_CROSS_ORG_ORGANIZATION)", + ); + expect(() => resolveCrossOrgEnv(fakeCtx())).toThrow(SkipError); + }); +}); diff --git a/scripts/ado-script/src/executor-e2e/__tests__/execute-cli.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/execute-cli.test.ts index c8170d69..8582a76d 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/execute-cli.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/execute-cli.test.ts @@ -43,6 +43,37 @@ describe("renderSourceMarkdown", () => { expect(md).toContain('"set-github-issue-type": {"target-repo":"o/r"}'); expect(md.match(/^---$/gm)?.length).toBe(2); }); + + it("emits expanded write permissions and cross-org repository metadata", () => { + const md = renderSourceMarkdown({ + tool: "create-branch", + safeOutputs: { "create-branch": { "allowed-repositories": ["target"] } }, + source: { + repositories: [{ + name: "Other Project/target-repo", + alias: "target", + organization: "other-org", + endpoint: "ado-write", + }], + writePermissions: { + serviceConnection: "ado-write", + connectionType: "azureDevOps", + allow: [{ + organization: "other-org", + projects: [{ + project: "Other Project", + repositories: ["target-repo"], + }], + }], + }, + }, + }); + + expect(md).toContain("permissions:"); + expect(md).toContain('"connection-type":"azureDevOps"'); + expect(md).toContain('"organization":"other-org"'); + expect(md).toContain('"endpoint":"ado-write"'); + }); }); describe("renderNdjsonLine", () => { diff --git a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts index 5ada7cf8..730bbb42 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts @@ -25,6 +25,9 @@ describe("scenario registry", () => { expect(new Set(ids).size).toBe(ids.length); expect(ids).toContain("create-pull-request"); expect(ids).toContain("create-pull-request-self-multi-checkout"); + expect(ids).toContain("create-pull-request-cross-org"); + expect(ids).toContain("create-branch-cross-org"); + expect(ids).toContain("create-git-tag-cross-org"); }); it("registers the GitHub issue scenarios with unique ids", () => { diff --git a/scripts/ado-script/src/executor-e2e/ado-rest.ts b/scripts/ado-script/src/executor-e2e/ado-rest.ts index 65dd24fa..c7da667e 100644 --- a/scripts/ado-script/src/executor-e2e/ado-rest.ts +++ b/scripts/ado-script/src/executor-e2e/ado-rest.ts @@ -2,9 +2,8 @@ * Minimal, self-contained Azure DevOps REST client for the deterministic * executor E2E harness. * - * Uses the global `fetch` (Node 20+) with Basic auth (empty user + token), - * matching how the `ado-aw` Rust executor authenticates - * (`reqwest ... .basic_auth("", Some(token))`). Endpoints and api-versions are + * Uses the global `fetch` (Node 20+) with Basic auth by default and Bearer auth + * for Azure DevOps service-connection tokens. Endpoints and api-versions are * chosen to line up with the executors under test so setup/assert/cleanup hit * the same surfaces the executor writes to. * @@ -15,6 +14,7 @@ export interface AdoRestOptions { orgUrl: string; project: string; token: string; + authKind?: "basic" | "bearer"; log?: (msg: string) => void; } @@ -42,7 +42,10 @@ export class AdoRest { constructor(opts: AdoRestOptions) { this.base = opts.orgUrl.replace(/\/+$/, ""); this.project = opts.project; - this.authHeader = "Basic " + Buffer.from(":" + opts.token).toString("base64"); + this.authHeader = + opts.authKind === "bearer" + ? `Bearer ${opts.token}` + : "Basic " + Buffer.from(":" + opts.token).toString("base64"); this.log = opts.log ?? (() => {}); this.timeoutMs = Number(process.env.EXECUTOR_E2E_REST_TIMEOUT_MS) || 30_000; } diff --git a/scripts/ado-script/src/executor-e2e/execute-cli.ts b/scripts/ado-script/src/executor-e2e/execute-cli.ts index e4db83a5..7e1fa021 100644 --- a/scripts/ado-script/src/executor-e2e/execute-cli.ts +++ b/scripts/ado-script/src/executor-e2e/execute-cli.ts @@ -16,7 +16,7 @@ import { mkdir, readFile, writeFile } from "node:fs/promises"; import { existsSync } from "node:fs"; import { join, resolve, sep } from "node:path"; -import type { ExecutedRecord, PriorEntry } from "./scenario.js"; +import type { ExecutedRecord, PriorEntry, ScenarioSource } from "./scenario.js"; const SAFE_OUTPUT_FILENAME = "safe_outputs.ndjson"; const EXECUTED_FILENAME = "safe-outputs-executed.ndjson"; @@ -27,6 +27,7 @@ export interface RenderSourceOptions { safeOutputs: Record>; /** ADO repo name for repo-targeting tools (emits a `repos:` block). */ adoRepo?: string; + source?: ScenarioSource; } /** @@ -45,7 +46,21 @@ export function renderSourceMarkdown(opts: RenderSourceOptions): string { "engine:", " id: copilot", ]; - if (opts.adoRepo) { + if (opts.source?.writePermissions) { + const write = opts.source.writePermissions; + lines.push("permissions:"); + lines.push(` write: ${JSON.stringify({ + "service-connection": write.serviceConnection, + "connection-type": write.connectionType, + allow: write.allow, + })}`); + } + if (opts.source?.repositories) { + lines.push("repos:"); + for (const repository of opts.source.repositories) { + lines.push(` - ${JSON.stringify(repository)}`); + } + } else if (opts.adoRepo) { lines.push("repos:"); // JSON-stringify both the alias and the repo name (valid YAML) to stay // consistent with the rest of the rendered front-matter and guard against @@ -87,6 +102,7 @@ export interface RunExecuteOptions { */ priorEntries?: PriorEntry[]; adoRepo?: string; + source?: ScenarioSource; orgUrl: string; project: string; token: string; @@ -137,7 +153,12 @@ export async function runExecute(opts: RunExecuteOptions): Promise( // Guard the auxiliary scenario methods too: a harness-level bug in any of // these must record a failed result and let the rest of the suite run, // not propagate out of runScenario and abort runAll early. - let config, entry, files, extraEnv, priorEntries; + let config, entry, files, extraEnv, priorEntries, source; try { config = scenario.config(ctx, state); entry = await scenario.ndjson(ctx, state); priorEntries = scenario.priorEntries ? await scenario.priorEntries(ctx, state) : undefined; files = scenario.files ? await scenario.files(ctx, state) : undefined; extraEnv = scenario.env ? await scenario.env(ctx, state) : undefined; + source = scenario.source ? await scenario.source(ctx, state) : undefined; } catch (err) { return finish({ ok: false, phase: "execute", message: errMessage(err) }); } @@ -88,6 +89,7 @@ export async function runScenario( entry, priorEntries, adoRepo: scenario.targetsAdoRepo ? ctx.adoRepo : undefined, + source, orgUrl: ctx.orgUrl, project: ctx.project, token: ctx.token, diff --git a/scripts/ado-script/src/executor-e2e/scenario.ts b/scripts/ado-script/src/executor-e2e/scenario.ts index 578f4b18..be8b3315 100644 --- a/scripts/ado-script/src/executor-e2e/scenario.ts +++ b/scripts/ado-script/src/executor-e2e/scenario.ts @@ -49,6 +49,28 @@ export interface PriorEntry { readonly config: Record; } +export interface ScenarioSourceRepository { + readonly name: string; + readonly alias: string; + readonly organization?: string; + readonly endpoint?: string; +} + +export interface ScenarioSource { + readonly repositories?: ScenarioSourceRepository[]; + readonly writePermissions?: { + readonly serviceConnection: string; + readonly connectionType: "azureDevOps"; + readonly allow: Array<{ + readonly organization: string; + readonly projects: Array<{ + readonly project: string; + readonly repositories: string[]; + }>; + }>; + }; +} + /** Shared, read-only context handed to every scenario phase. */export interface ScenarioContext { /** ADO collection URI, e.g. https://dev.azure.com/msazuresphere/ */ readonly orgUrl: string; @@ -93,6 +115,8 @@ export interface Scenario { * per-tool `allowed-repositories` config. */ readonly targetsAdoRepo?: boolean; + /** Optional trusted front-matter additions for cross-org executor scenarios. */ + source?(ctx: ScenarioContext, state: State): Promise; /** Per-tool `safe-outputs: :` front-matter config fragment. */ config(ctx: ScenarioContext, state: State): Record; /** diff --git a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts index dc634679..16fafb4d 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts @@ -29,6 +29,11 @@ import { join } from "node:path"; import type { Scenario, ScenarioContext } from "../scenario.js"; import { partialOutput } from "../execute-cli.js"; import { detBody, numResult, Teardown } from "./common.js"; +import { + crossOrgSource, + resolveCrossOrgEnv, + type CrossOrgEnv, +} from "./cross-org.js"; interface CreatePrState { repo: string; @@ -42,13 +47,17 @@ interface CreatePrState { sourcesDir: string; /** Actual git checkout beneath sourcesDir. */ checkoutDir: string; + rest: ScenarioContext["rest"]; + executorToken: string; + repositorySelector: string; + crossOrg?: CrossOrgEnv; /** PR id, populated in assert() so cleanup can abandon it. */ prId?: number; } interface CreatePrScenarioOptions { readonly id: string; - readonly repositorySelector: "named" | "self"; + readonly repositorySelector: "named" | "self" | "cross-org"; readonly patchRelPath: string; readonly changedFileSuffix?: string; } @@ -124,13 +133,28 @@ async function setupCreatePullRequest( ctx: ScenarioContext, options: CreatePrScenarioOptions, ): Promise { - const repo = ctx.adoRepo; - const authHeader = "Basic " + Buffer.from(":" + ctx.token).toString("base64"); + const crossOrg = + options.repositorySelector === "cross-org" + ? resolveCrossOrgEnv(ctx) + : undefined; + const repo = crossOrg?.repository ?? ctx.adoRepo; + const orgUrl = crossOrg?.orgUrl ?? ctx.orgUrl; + const project = crossOrg?.project ?? ctx.project; + const token = crossOrg?.token ?? ctx.token; + const rest = crossOrg?.rest ?? ctx.rest; + const authHeader = crossOrg + ? `Bearer ${token}` + : "Basic " + Buffer.from(":" + token).toString("base64"); const sourcesDir = join(ctx.workDir, options.id, "src-checkout"); await mkdir(sourcesDir, { recursive: true }); - const checkoutDir = join(sourcesDir, repo); + const repositorySelector = crossOrg?.alias ?? + (options.repositorySelector === "self" ? "self" : repo); + const checkoutDir = join( + sourcesDir, + crossOrg?.alias ?? repo, + ); - const cloneUrl = `${ctx.orgUrl.replace(/\/+$/, "")}/${encodeURIComponent(ctx.project)}/_git/${encodeURIComponent(repo)}`; + const cloneUrl = `${orgUrl.replace(/\/+$/, "")}/${encodeURIComponent(project)}/_git/${encodeURIComponent(repo)}`; ctx.log(`[${options.id}] cloning ${repo}`); await git(ctx, ["clone", cloneUrl, checkoutDir], sourcesDir, authHeader, options.id); @@ -188,6 +212,10 @@ async function setupCreatePullRequest( patchContent, sourcesDir, checkoutDir, + rest, + executorToken: token, + repositorySelector, + crossOrg, }; } @@ -197,7 +225,7 @@ function createPullRequestScenario( return { id: options.id, tool: "create-pull-request", - targetsAdoRepo: true, + targetsAdoRepo: options.repositorySelector === "named", config: (_ctx, state) => ({ // Target the repo's actual default branch (state.targetBranch), which is // also where base_commit was taken from, rather than hardcoding "main". @@ -208,11 +236,16 @@ function createPullRequestScenario( "include-stats": false, }), setup: (ctx) => setupCreatePullRequest(ctx, options), + source: async (_ctx, state) => + state.crossOrg ? crossOrgSource(state.crossOrg) : {}, files: async (_ctx, state) => ({ [state.patchRelPath]: state.patchContent }), env: async (_ctx, state) => { const env: Record = { BUILD_SOURCESDIRECTORY: state.sourcesDir, }; + if (state.crossOrg) { + env.SYSTEM_ACCESSTOKEN = state.executorToken; + } if (options.repositorySelector === "self") { Object.assign(env, { ADO_AW_SELF_REPOSITORY_DIRECTORY: state.checkoutDir, @@ -235,7 +268,7 @@ function createPullRequestScenario( description: detBody(ctx, options.id), source_branch: state.sourceBranch, patch_file: state.patchRelPath, - repository: options.repositorySelector === "self" ? "self" : state.repo, + repository: state.repositorySelector, agent_labels: [], base_commit: state.baseCommit, patch_sha256: state.patchSha256, @@ -245,9 +278,9 @@ function createPullRequestScenario( // Record the PR id up front so cleanup abandons it even if a later // assertion (or the getPullRequest call itself) throws. state.prId = prId; - const pr = await ctx.rest.getPullRequest(state.repo, prId); + const pr = await state.rest.getPullRequest(state.repo, prId); if (pr.status === "abandoned") throw new Error(`PR #${prId} is abandoned`); - const sha = await ctx.rest.getRefObjectId(state.repo, `heads/${state.sourceBranch}`); + const sha = await state.rest.getRefObjectId(state.repo, `heads/${state.sourceBranch}`); if (!sha) throw new Error(`source branch '${state.sourceBranch}' was not pushed`); }, cleanup: async (ctx, state) => { @@ -257,11 +290,11 @@ function createPullRequestScenario( const teardown = new Teardown(); if (state.prId !== undefined) { const prId = state.prId; - teardown.add("abandon PR", () => ctx.rest.abandonPullRequest(state.repo, prId)); + teardown.add("abandon PR", () => state.rest.abandonPullRequest(state.repo, prId)); } await teardown .add("delete source branch", () => - ctx.rest.deleteRef(state.repo, `refs/heads/${state.sourceBranch}`), + state.rest.deleteRef(state.repo, `refs/heads/${state.sourceBranch}`), ) // Remove the cloned checkout so repeated local runs don't accumulate it. .add("remove local checkout", () => @@ -285,7 +318,15 @@ export const createPullRequestSelfMultiCheckout = createPullRequestScenario({ changedFileSuffix: "-self-multi-checkout", }); +export const createPullRequestCrossOrg = createPullRequestScenario({ + id: "create-pull-request-cross-org", + repositorySelector: "cross-org", + patchRelPath: "create-pr-cross-org.patch", + changedFileSuffix: "-cross-org", +}); + export const createPullRequestScenarios: Scenario[] = [ createPullRequest, createPullRequestSelfMultiCheckout, + createPullRequestCrossOrg, ]; diff --git a/scripts/ado-script/src/executor-e2e/scenarios/cross-org.ts b/scripts/ado-script/src/executor-e2e/scenarios/cross-org.ts new file mode 100644 index 00000000..6065cbef --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/scenarios/cross-org.ts @@ -0,0 +1,169 @@ +/** + * Optional cross-organization Azure Repos write scenarios. + * + * These require pre-provisioned same-tenant Azure DevOps WIF infrastructure; + * they skip when any required environment value is absent. + */ +import { AdoRest } from "../ado-rest.js"; +import type { Scenario, ScenarioContext, ScenarioSource } from "../scenario.js"; +import { SkipError } from "../scenario.js"; +import { detBody } from "./common.js"; + +export interface CrossOrgEnv { + organization: string; + orgUrl: string; + project: string; + repository: string; + alias: string; + endpoint: string; + token: string; + rest: AdoRest; +} + +function cleanVar(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + if (!trimmed || /^\$\([^)]+\)$/.test(trimmed)) return undefined; + return trimmed; +} + +export function resolveCrossOrgEnv(ctx: ScenarioContext): CrossOrgEnv { + const organization = cleanVar(process.env.EXECUTOR_E2E_CROSS_ORG_ORGANIZATION); + const project = cleanVar(process.env.EXECUTOR_E2E_CROSS_ORG_PROJECT); + const repository = cleanVar(process.env.EXECUTOR_E2E_CROSS_ORG_REPOSITORY); + const endpoint = cleanVar(process.env.EXECUTOR_E2E_CROSS_ORG_ENDPOINT); + const token = cleanVar(process.env.EXECUTOR_E2E_CROSS_ORG_TOKEN); + const missing = [ + ["EXECUTOR_E2E_CROSS_ORG_ORGANIZATION", organization], + ["EXECUTOR_E2E_CROSS_ORG_PROJECT", project], + ["EXECUTOR_E2E_CROSS_ORG_REPOSITORY", repository], + ["EXECUTOR_E2E_CROSS_ORG_ENDPOINT", endpoint], + ["EXECUTOR_E2E_CROSS_ORG_TOKEN", token], + ].filter(([, value]) => !value).map(([name]) => name); + if (missing.length > 0) { + throw new SkipError( + `cross-org repository scenarios require ${missing.join(", ")}`, + ); + } + const orgUrl = `https://dev.azure.com/${organization}/`; + return { + organization: organization!, + orgUrl, + project: project!, + repository: repository!, + alias: "cross-org-target", + endpoint: endpoint!, + token: token!, + rest: new AdoRest({ + orgUrl, + project: project!, + token: token!, + authKind: "bearer", + log: ctx.log, + }), + }; +} + +export function crossOrgSource(env: CrossOrgEnv): ScenarioSource { + return { + repositories: [{ + name: `${env.project}/${env.repository}`, + alias: env.alias, + organization: env.organization, + endpoint: env.endpoint, + }], + writePermissions: { + serviceConnection: env.endpoint, + connectionType: "azureDevOps", + allow: [{ + organization: env.organization, + projects: [{ + project: env.project, + repositories: [env.repository], + }], + }], + }, + }; +} + +function defaultBranch(defaultBranch: string | undefined): string { + return defaultBranch?.replace(/^refs\/heads\//, "") || "main"; +} + +export const createCrossOrgBranch: Scenario<{ + env: CrossOrgEnv; + branch: string; + base: string; +}> = { + id: "create-branch-cross-org", + tool: "create-branch", + setup: async (ctx) => { + const env = resolveCrossOrgEnv(ctx); + const repository = await env.rest.getRepository(env.repository); + return { + env, + branch: ctx.prefix("create-branch-cross-org"), + base: defaultBranch(repository.defaultBranch), + }; + }, + source: async (_ctx, state) => crossOrgSource(state.env), + config: (_ctx, state) => ({ + "allowed-repositories": [state.env.alias], + max: 1, + }), + env: async (_ctx, state) => ({ + SYSTEM_ACCESSTOKEN: state.env.token, + }), + ndjson: async (_ctx, state) => ({ + branch_name: state.branch, + source_branch: state.base, + repository: state.env.alias, + }), + assert: async (_ctx, state) => { + const sha = await state.env.rest.getRefObjectId( + state.env.repository, + `heads/${state.branch}`, + ); + if (!sha) throw new Error(`cross-org branch '${state.branch}' was not created`); + }, + cleanup: async (_ctx, state) => + state.env.rest.deleteRef(state.env.repository, `refs/heads/${state.branch}`), +}; + +export const createCrossOrgGitTag: Scenario<{ + env: CrossOrgEnv; + tag: string; +}> = { + id: "create-git-tag-cross-org", + tool: "create-git-tag", + setup: async (ctx) => ({ + env: resolveCrossOrgEnv(ctx), + tag: `ado-aw-det-${ctx.buildId}-cross-org-tag`, + }), + source: async (_ctx, state) => crossOrgSource(state.env), + config: (_ctx, state) => ({ + "allowed-repositories": [state.env.alias], + max: 1, + }), + env: async (_ctx, state) => ({ + SYSTEM_ACCESSTOKEN: state.env.token, + }), + ndjson: async (ctx, state) => ({ + tag_name: state.tag, + message: detBody(ctx, "create-git-tag-cross-org"), + repository: state.env.alias, + }), + assert: async (_ctx, state) => { + const sha = await state.env.rest.getRefObjectId( + state.env.repository, + `tags/${state.tag}`, + ); + if (!sha) throw new Error(`cross-org tag '${state.tag}' was not created`); + }, + cleanup: async (_ctx, state) => + state.env.rest.deleteRef(state.env.repository, `refs/tags/${state.tag}`), +}; + +export const crossOrgScenarios: Scenario[] = [ + createCrossOrgBranch, + createCrossOrgGitTag, +]; diff --git a/scripts/ado-script/src/executor-e2e/scenarios/index.ts b/scripts/ado-script/src/executor-e2e/scenarios/index.ts index 9165e8fa..e2d277ab 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/index.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/index.ts @@ -5,6 +5,7 @@ import type { Scenario } from "../scenario.js"; import { buildScenarios } from "./build.js"; import { createPullRequestScenarios } from "./create-pull-request.js"; +import { crossOrgScenarios } from "./cross-org.js"; import { gitScenarios } from "./git.js"; import { githubIssueScenarios } from "./github-issue.js"; import { prScenarios } from "./pr.js"; @@ -19,6 +20,7 @@ export const allScenarios: Scenario[] = [ ...wikiScenarios, ...prScenarios, ...gitScenarios, + ...crossOrgScenarios, ...buildScenarios, ...createPullRequestScenarios, ...githubIssueScenarios, diff --git a/tests/executor-e2e/README.md b/tests/executor-e2e/README.md index e10c0950..fb01b062 100644 --- a/tests/executor-e2e/README.md +++ b/tests/executor-e2e/README.md @@ -56,6 +56,8 @@ All deterministically-assertable ADO-write safe outputs plus the flagship - **PR:** `add-pr-comment`, `reply-to-pr-comment`, `resolve-pr-thread`, `submit-pr-review`, `update-pr` - **Git:** `create-branch`, `create-git-tag` +- **Cross-org Git (optional infrastructure):** `create-branch-cross-org`, + `create-git-tag-cross-org`, and `create-pull-request-cross-org` - **Build:** `add-build-tag`, `queue-build`, `upload-build-attachment`, `upload-pipeline-artifact` - **Flagship:** `create-pull-request` covers both a named additional checkout at @@ -223,6 +225,37 @@ scratch repository, not a canonical one. | `EXECUTOR_E2E_GITHUB_TOKEN` | Reused from failure-issue filing. It must now also carry **Issues: write** on the scratch repository, because these scenarios create, mutate, and close issues. | | `EXECUTOR_E2E_SCENARIO_ISSUE_REPO` | Optional. `owner/repo` for scratch issues; falls back to `EXECUTOR_E2E_ISSUE_REPO`. Set it to keep scenario issues away from the failure-report repository. | | `E2E_GITHUB_ISSUE_TYPE` | Optional. Forces a native issue-type name for environments where the token cannot read org metadata but the type is known to exist. | +| `EXECUTOR_E2E_CROSS_ORG_ORGANIZATION` | Target Azure DevOps organization name. | +| `EXECUTOR_E2E_CROSS_ORG_PROJECT` | Target project containing the scratch repository. | +| `EXECUTOR_E2E_CROSS_ORG_REPOSITORY` | Scratch repository used for branch, tag, and PR writes. | +| `EXECUTOR_E2E_CROSS_ORG_ENDPOINT` | Azure DevOps WIF service-connection name used by the source fixture. | +| `EXECUTOR_E2E_CROSS_ORG_TOKEN` | Secret, short-lived Entra bearer minted from that connection. | + +### Cross-organization repository-write scenarios + +These scenarios are registered but skip unless all five variables above are +present. The target must be in the same Entra tenant, and the service +connection's identity must be added to both Azure DevOps organizations with +Read, Contribute, Create branch/tag, and Create pull request permissions on the +scratch repository. + +The harness: + +1. uses Bearer authentication for target-org setup/assert/cleanup; +2. renders `repos.organization`, the checkout `endpoint`, and expanded + `permissions.write` with `connection-type: azureDevOps`; +3. runs the real Stage 3 executor with the target token; +4. creates and deletes a branch and annotated tag; +5. clones the target repository, generates a patch, creates and abandons a PR, + deletes its source branch, and removes the local checkout. + +The registered AgentPlayground pipeline does not currently mint this token: +creating or authorizing the Azure DevOps WIF service connection, adding its +identity to the target organization, and granting repository ACLs are +administrative side effects that must be provisioned separately. After that, +add an `AzureCLI@3` token-mint step and map its secret output to +`EXECUTOR_E2E_CROSS_ORG_TOKEN`; the scenarios then become mandatory rather +than skipped. There is deliberately **no default repo** for these scenarios: when neither variable is set they skip rather than filing scratch issues onto diff --git a/tests/executor-e2e/azure-pipelines.yml b/tests/executor-e2e/azure-pipelines.yml index 83e030e1..2b6a7bd7 100644 --- a/tests/executor-e2e/azure-pipelines.yml +++ b/tests/executor-e2e/azure-pipelines.yml @@ -65,6 +65,10 @@ variables: # skip gracefully. Override at queue time (or via a variable group) to enable. # EXECUTOR_E2E_GITHUB_TOKEN must be provided as a SECRET pipeline variable # scoped to Issues:read/write on the configured issue repository. + # Cross-org repository scenarios require five definition variables: + # EXECUTOR_E2E_CROSS_ORG_ORGANIZATION / PROJECT / REPOSITORY / ENDPOINT plus + # secret EXECUTOR_E2E_CROSS_ORG_TOKEN. They remain unset in the default lane, + # so the scenarios skip until pre-provisioned WIF infrastructure is approved. steps: - checkout: self @@ -155,3 +159,11 @@ steps: # EXECUTOR_E2E_ISSUE_REPO, and skip when neither is set. EXECUTOR_E2E_SCENARIO_ISSUE_REPO: $(EFFECTIVE_EXECUTOR_E2E_SCENARIO_ISSUE_REPO) E2E_GITHUB_ISSUE_TYPE: $(EFFECTIVE_E2E_GITHUB_ISSUE_TYPE) + # Optional cross-organization Azure Repos scenarios. The token must be a + # short-lived Entra bearer minted from the Azure DevOps service connection + # named by ENDPOINT; unexpanded macros are treated as absent. + EXECUTOR_E2E_CROSS_ORG_ORGANIZATION: $(EXECUTOR_E2E_CROSS_ORG_ORGANIZATION) + EXECUTOR_E2E_CROSS_ORG_PROJECT: $(EXECUTOR_E2E_CROSS_ORG_PROJECT) + EXECUTOR_E2E_CROSS_ORG_REPOSITORY: $(EXECUTOR_E2E_CROSS_ORG_REPOSITORY) + EXECUTOR_E2E_CROSS_ORG_ENDPOINT: $(EXECUTOR_E2E_CROSS_ORG_ENDPOINT) + EXECUTOR_E2E_CROSS_ORG_TOKEN: $(EXECUTOR_E2E_CROSS_ORG_TOKEN) From 77cfe9d03c7c95447a030fcad6343cea29ba767e Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 11:53:53 +0100 Subject: [PATCH 12/18] docs: document cross-org repository writes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da --- AGENTS.md | 2 +- docs/ado-script.md | 11 ++-- docs/front-matter.md | 39 ++++++----- docs/network.md | 56 ++++++++++++++-- docs/safe-output-permissions.md | 29 +++++++-- docs/safe-outputs.md | 47 +++++++++----- .../src/content/docs/reference/ado-script.mdx | 2 +- site/src/content/docs/reference/ir.mdx | 1 + site/src/content/docs/reference/network.mdx | 23 ++++++- .../content/docs/reference/safe-outputs.mdx | 20 +++++- site/src/content/docs/setup/quick-start.mdx | 17 ++++- .../docs/setup/service-connections.mdx | 65 +++++++++++++++---- .../docs/troubleshooting/common-issues.mdx | 4 +- .../safe-output-permissions.mdx | 27 ++++++-- 14 files changed, 265 insertions(+), 78 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8742c75e..3b678296 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -317,7 +317,7 @@ fail-closed and only pauses when the agent actually proposed a reviewed output. │ ├── github-app-token/ # GitHub App token minter (bundled to github-app-token.js; mints installation token in Agent + Detection when engine.github-app-token is set) │ ├── executor-e2e/ # Stage 3 safe-output E2E test harness (not a bundle; runs deterministic scenarios against a real ADO project and files a GitHub issue on failure) │ ├── compiler-smoke-e2e/ # Smoke E2E orchestrator (not a bundle): stages each case in `tests/smoke/cases.json` to the fixed `.smoke/pipeline.yml` path on its own per-case `ado-aw-mirror` ref, queues it against its credential *lane* definition, and asserts they go green. Two modes via `SMOKE_COMPILER_SOURCE`: `candidate` (compiler built from this commit, pinned pipeline-artifact) and `released` (latest release asset, release URLs required). Built to `test-bin/` by `build:compiler-smoke-e2e`, listed in `NON_BUNDLE_DIRS`. -│ ├── prepare-pr-base/ # create-pull-request preparer (bundled to prepare-pr-base.js): Agent mode uses ADO diff metadata + bounded dual-ref fallback to make the merge-base reachable; SafeOutputs mode fetches only the target worktree tip +│ ├── prepare-pr-base/ # create-pull-request preparer (bundled to prepare-pr-base.js): Agent mode uses ADO diff metadata + bounded fallback; SafeOutputs fetches the target tip; cross-org targets use isolated credentials + exact remote matching │ ├── ado-proxy/ # Credential-isolated ADO policy proxy (bundled to ado-proxy.js). The pipeline mounts it into node:20-slim and starts it before AWF; AWF attaches the trusted container via --topology-attach. scope.ts builds the organization-relative current/additional scope index; catalog.gen.json + ../shared/ado-proxy-catalog.types.gen.ts are generated from Rust by export-ado-proxy-catalog{,-schema} and drift-guarded; a catalog_version mismatch fails closed at startup. │ ├── trigger-e2e/ # Test-only gate-spec / trigger-evaluation harness (not a bundle): mirrors Rust `Fact::ALL` in `gate-spec.ts`; `fact-catalog.gen.json` is generated by `export-fact-catalog` and drift-guarded by CI │ └── shared/ # Shared modules across bundles (auth, ado-client, env-facts, types.gen.ts) diff --git a/docs/ado-script.md b/docs/ado-script.md index cbad4b71..d96c68e2 100644 --- a/docs/ado-script.md +++ b/docs/ado-script.md @@ -79,10 +79,13 @@ pipeline** as runtime helpers. Today it produces thirteen bundles: shallow source/target ranges and verifies the base locally. Ineligible or unavailable REST falls back to bounded dual-ref depths 200/500/2000, never an automatic full-history fetch. SafeOutputs `target-worktree` mode fetches only - the target tip at depth 1. Each allowed repo is passed as a typed - `--repo-dir` / `--source-ref` / `--target-branch` tuple; per-dir failures are - isolated and surfaced as ADO warnings. The bearer - (`SYSTEM_ACCESSTOKEN`) remains in masked env and spawned-git + the target tip at depth 1. Cross-org repositories are partitioned into a + separate trusted credential scope and passed with validated + organization/project/repository coordinates. The checkout remote must match + those coordinates exactly before the Bearer reaches REST or git; mismatch or + preparation failure stops the trusted task before Agent/executor execution. + Same-org per-dir failures remain isolated warnings. The bearer remains + shell-local or in masked `SYSTEM_ACCESSTOKEN` env and spawned-git `GIT_CONFIG_*`, never argv or `.git/config`. Runs outside AWF. See [`safe-outputs.md`](safe-outputs.md#create-pull-request). diff --git a/docs/front-matter.md b/docs/front-matter.md index 122c903f..0288deb0 100644 --- a/docs/front-matter.md +++ b/docs/front-matter.md @@ -258,7 +258,7 @@ network: # optional network policy (standalone target only # variable-groups: # optional: import ADO Library variable groups (standalone/1es only) # - My Variable Group # each entry must be the exact ADO Library group name (see "Variable Groups" section) permissions: # optional ADO access token configuration (see docs/network.md#permissions-ado-access-tokens) - read: my-read-arm-connection # shorthand: proxy gets the ARM SC token; Agent/MCP/az get no real token + read: my-read-arm-connection # shorthand: AzureCLI@3 connectionType azureRM # read: # object form: narrow capabilities / add cross-org or project scope # service-connection: my-read-arm-connection # capabilities: [core, repos] # discovery is always enabled @@ -268,10 +268,15 @@ permissions: # optional ADO access token configuration (see do # - project: Shared # project-id: 33333333-3333-3333-3333-333333333333 # optional GUID-form calls # repositories: [shared-api] # empty/omitted => project reads only - write: my-write-arm-connection # OPTIONAL ARM SC for Stage 3 executor writes. - # Default: executor uses $(System.AccessToken). - # Set this only for cross-org writes or - # named-identity attribution. + write: my-write-arm-connection # shorthand: AzureCLI@3 connectionType azureRM + # write: # expanded form for scoped cross-org repo writes + # service-connection: ado-repository-writer + # connection-type: azureDevOps + # allow: + # - organization: partner-org + # projects: + # - project: Shared + # repositories: [shared-api] # permissions-required: # optional abstract capability requirements (usually set by an # read: true # imported component rather than authored directly); see # write: true # docs/imports.md#permissions-required. Unioned across all @@ -539,19 +544,20 @@ Each entry can be: | Form | Syntax | Description | |------|--------|-------------| -| **Shorthand** | `- org/repo` | Alias derived from last segment, type=git, ref=refs/heads/main, checkout=true | -| **Shorthand with alias** | `- alias=org/repo` | Explicit alias before `=` | -| **Object** | `- name: org/repo` | Full control over all fields | +| **Shorthand** | `- project/repo` | Alias derived from last segment, type=git, ref=refs/heads/main, checkout=true | +| **Shorthand with alias** | `- alias=project/repo` | Explicit alias before `=` | +| **Object** | `- name: project/repo` | Full control over all fields | Object fields: | Field | Default | Description | |---------------|------------------------|-------------| -| `name` | *(required)* | Full `org/repo` name (maps to ADO `name:`) | +| `name` | *(required)* | Azure Repos `project/repository` name (maps to ADO `name:`) | | `alias` | last segment of `name` | Repository alias (maps to ADO `repository:`) | | `type` | `git` | ADO repository resource type | | `ref` | `refs/heads/main` | Branch or tag reference | -| `endpoint` | *(none)* | Azure DevOps service connection. Required for `type: github`, `githubenterprise`, or `bitbucket`; not needed for same-org Azure Repos `git`. | +| `endpoint` | *(none)* | Checkout service connection. Required for external providers and cross-organization Azure Repos `git`. | +| `organization` | *(current organization)* | Target Azure DevOps organization for cross-org `type: git`; requires object form and `endpoint`. | | `checkout` | `true` | Whether the agent job clones this repo | | `fetch-depth` | *(ADO default)* | Shallow-clone depth for this repo's checkout (ADO `fetchDepth`). `0` = full history | | `fetch-tags` | *(ADO default)* | Whether to fetch git tags during checkout (ADO `fetchTags`) | @@ -560,12 +566,13 @@ Aliases must be unique case-insensitively because they become checkout directory names on Windows agents. `root`, `repo`, and `self` are reserved in every casing; `self` is the compiler-owned path for the pipeline repository. -> **Cross-organization `type: git` repositories.** A `type: git` entry with an -> `endpoint:` set (used for a repository outside the pipeline's own Azure -> DevOps organization) checks out correctly, but `create-pull-request` cannot -> yet target it: Stage 3 composes every ADO Git REST call from the pipeline's -> own organization/project. See the limitation note under -> [`create-pull-request`](safe-outputs.md#create-pull-request). +> **Cross-organization `type: git` repositories.** Use object form with both +> `organization:` and `endpoint:`. Checkout authorization and Stage 3 writes +> are separate: `endpoint` authenticates the repository resource, while +> expanded `permissions.write` with `connection-type: azureDevOps` and an exact +> organization/project/repository `allow` scope authorizes `create-pull-request`, +> `create-branch`, and `create-git-tag`. Incomplete entries compile with a +> warning and are rejected if targeted, including under `--dry-run`. ### Tuning checkout fetch behavior (`fetch-depth` / `fetch-tags`) diff --git a/docs/network.md b/docs/network.md index 576b991b..8f895905 100644 --- a/docs/network.md +++ b/docs/network.md @@ -239,8 +239,8 @@ See [`imports:`](imports.md) for the ADO-first compile-time `repository` and ## Permissions (ADO Access Tokens) -The ARM service-connection scope does not determine what its identity may do in -Azure DevOps. `permissions.read` and `permissions.write` describe intended +The service-connection resource scope does not determine what its identity may +do in Azure DevOps. `permissions.read` and `permissions.write` describe intended pipeline roles and token placement; operators must separately grant each underlying identity the minimum Azure DevOps permissions. The executor (Stage 3) always has a write-capable token; what changes is its *source* and @@ -249,7 +249,7 @@ underlying identity the minimum Azure DevOps permissions. The executor | Source | When | Identity | | ----------------------------------- | --------------------------------------------- | ----------------------------------------------- | | `$(System.AccessToken)` *(default)* | No `permissions.write` configured | `Project Collection Build Service (org)` | -| `$(SC_WRITE_TOKEN)` *(opt-in)* | `permissions.write: ` | The federated identity behind the ARM SC | +| `$(SC_WRITE_TOKEN)` *(opt-in)* | Any `permissions.write` service connection | The configured connection's Entra identity | The agent (Stage 1) never receives the executor's token. Stage separation — not token type — is the trust boundary. @@ -261,7 +261,7 @@ not token type — is the trust boundary. don't match (`PATCH _apis/build/builds/{id}`) and fetches PR metadata for Tier 2 filters (labels, draft status, changed files). Runs before the agent, outside the AWF sandbox. -2. **Stage 3 executor** — when no ARM write SC is configured (the default), +2. **Stage 3 executor** — when no write service connection is configured (the default), the executor's `SYSTEM_ACCESSTOKEN` env var is sourced from `$(System.AccessToken)`. @@ -287,9 +287,11 @@ agents. Set `permissions.write` only when you need: 1. **Cross-org or cross-project writes** — `System.AccessToken` is scoped to the host project. Targeting work items or repos in a different ADO - project / organization requires an ARM SC with broader scope. + project may use an Azure Resource Manager connection. Cross-organization + repository writes require an Azure DevOps connection and explicit allow + scope. 2. **Named-identity attribution** — `System.AccessToken` writes are - attributed to the `Project Collection Build Service` identity. An ARM SC + attributed to the `Project Collection Build Service` identity. A service connection attributes writes to its underlying federated identity (e.g. `safe-output-bot@contoso.com`), useful when audit logs or work-item notifications need a specific actor. @@ -343,6 +345,35 @@ agents. Set `permissions.write` only when you need: used **only** by the executor in Stage 3 (`SafeOutputs` job). Overrides the default `$(System.AccessToken)` for write operations. Never exposed to the agent. + + Scalar form preserves the Azure Resource Manager connection contract: + + ```yaml + permissions: + write: my-arm-connection + ``` + + Expanded form selects AzureCLI@3's connection type and adds exact + cross-organization repository scopes: + + ```yaml + permissions: + write: + service-connection: ado-repository-writer + connection-type: azureDevOps + allow: + - organization: other-org + projects: + - project: Other Project + repositories: [target-repo] + ``` + + `connection-type` is either `azureRM` or `azureDevOps`. Only + `azureDevOps` permits cross-organization repository writes. The Azure DevOps + service connection must be backed by Entra workload identity federation; its + identity must belong to the same tenant, be added to every target + organization, and receive repository ACLs there. `allow` is additive to the + implicit current organization and is deny-by-default for cross-org targets. - **Both omitted**: The agent has no ADO API access. The executor still has a write-capable token via `$(System.AccessToken)`, scoped by the pipeline's job-authorization settings. @@ -354,11 +385,22 @@ agents. Set `permissions.write` only when you need: permissions: read: my-read-sc -# Cross-org / named-identity attribution — executor writes via ARM SC. +# Named-identity attribution through an Azure Resource Manager connection. permissions: read: my-read-sc write: my-write-sc +# Cross-org repository writes through an Azure DevOps WIF connection. +permissions: + write: + service-connection: ado-repository-writer + connection-type: azureDevOps + allow: + - organization: other-org + projects: + - project: Other Project + repositories: [target-repo] + # Agent has no ADO read access; executor still writes via $(System.AccessToken). # (Empty front matter — no `permissions:` key at all.) ``` diff --git a/docs/safe-output-permissions.md b/docs/safe-output-permissions.md index f6ef4db9..95e1636f 100644 --- a/docs/safe-output-permissions.md +++ b/docs/safe-output-permissions.md @@ -74,7 +74,7 @@ The toggle lives in three places (most-specific wins): > which carries `PullRequestContribute` by default. If `permissions.write:` is set in the agent's front matter, Stage 3 -uses the **ARM service connection's identity** instead, and none of +uses the configured **service connection's identity** instead, and none of the above applies — see [Option 1](#option-1-wire-a-write-service-connection-recommended). --- @@ -230,9 +230,8 @@ complementary. ### Option 1: Wire a write service connection (recommended) -Add an ARM service connection whose backing identity has the -permission you need on the target repository, and reference it from -the agent front matter: +For same-organization or same-organization cross-project writes, the scalar +form keeps the Azure Resource Manager connection behavior: ```yaml permissions: @@ -240,14 +239,30 @@ permissions: write: ado-aw-write # used by Stage 3 ``` +For cross-organization repository writes, use an Azure DevOps service +connection backed by Entra workload identity federation: + +```yaml +permissions: + write: + service-connection: ado-aw-repository-writer + connection-type: azureDevOps + allow: + - organization: other-org + projects: + - project: Other Project + repositories: [target-repo] +``` + Stage 3 will mint its token via that connection instead of using `$(System.AccessToken)`, so the build-service ACEs become irrelevant. This is the most explicit option: the identity used for writes is named in the front matter, audit logs attribute every action to that named principal, and the least-privilege grant lives entirely on the -service connection's identity. It also works unchanged for -cross-organization writes. +service connection's identity. The same identity can write in multiple +same-tenant organizations after it is added to each organization and granted +the repository permissions listed above. See [`docs/network.md`](network.md) (Permissions section) and the "Service Connections" page on the documentation site for the full @@ -306,7 +321,7 @@ Option 2 unless you have a specific reason to broaden the grant. | HTTP status | Body fragment | Most likely cause | |---|---|---| -| 401 Unauthorized | `TF400813: The user '...' is not authorized to access this resource` | Token is malformed or missing — usually a misconfigured service-connection step; check that the AzureCLI@2 mint succeeded. | +| 401 Unauthorized | `TF400813: The user '...' is not authorized to access this resource` | Token is malformed or missing — usually a misconfigured service-connection step; check that the AzureCLI@3 mint succeeded. | | 403 Forbidden | `TF401027: You need the Git 'PullRequestContribute' permission` | This page — Stage 3 identity lacks PR-contribute on the target repo. | | 403 Forbidden | `TF401027: You need the Git 'GenericContribute' permission` | Same diagnosis; need `Contribute` on the repo (typically because of `create-pull-request` or `create-branch`). | | 403 Forbidden | `VS800075: The project ... does not exist, or you do not have permission to access it.` | Cross-project request blocked because "Limit job authorization scope to current project" is ON. Use Option 1 with a write service connection that has cross-project rights, or move the resource into the calling project. | diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index dc9c3a1e..48ff0690 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -292,7 +292,8 @@ Stage 3 `SafeOutputs` job and authenticate to Azure DevOps using `SYSTEM_ACCESSTOKEN`. By default this is `$(System.AccessToken)` — the pipeline's built-in OAuth token running as the *Project Collection Build Service* identity. Set `permissions.write` to override this with an -ARM-minted token, e.g. for cross-org writes or named-identity attribution. +AzureCLI@3-minted token, e.g. for cross-org writes or named-identity +attribution. See [`docs/network.md`](network.md) and [`docs/ir.md`](ir.md) for the typed SafeOutputs job wiring. @@ -1007,25 +1008,31 @@ Creates a pull request with code changes made by the agent. When invoked: 2. Saves the patch to the safe outputs directory 3. Creates a JSON record with PR metadata (title, description, source branch, repository) -During Stage 3 execution, the repository is validated against the allowed list (from `checkout:` + "self"), then the patch is applied and a PR is created in Azure DevOps. - -> **Cross-organization repositories are not yet supported.** Every ADO Git -> REST call the executor makes is composed from the pipeline's own -> organization/project. A `repos:` alias checked out from a **different** -> Azure DevOps organization (a `type: git` entry with an `endpoint:` service -> connection — see [`docs/front-matter.md`](front-matter.md#repositories-repos)) -> cannot be targeted: the compiler warns when `create-pull-request` and such an -> alias are both configured, and Stage 3 rejects the alias with a clear error -> (including under `--dry-run`) instead of silently composing a request against -> the wrong organization. +During Stage 3 execution, the repository is validated against the allowed list +(from `checkout:` + "self"), resolved to an exact +organization/project/repository target, then the patch is applied and a PR is +created in Azure DevOps. + +> **Cross-organization repositories.** `create-pull-request`, +> `create-branch`, and `create-git-tag` can target a checked-out Azure Repos +> repository in another organization when its `repos:` object declares +> `organization:` plus `endpoint:`, and expanded `permissions.write` uses +> `connection-type: azureDevOps` with an exact organization/project/repository +> allow scope. The Stage 3 token is never exposed to the Agent. Missing routing, +> connection type, or scope produces a compile warning and a target-time +> rejection, including under `--dry-run`; it never falls back to the pipeline +> organization. **Shallow-clone agent pools (automatic):** The diff base is computed at agent -time from the checked-out repository. For same-organization Azure Repos, +time from the checked-out repository. For Azure Repos, `prepare-pr-base.js` asks the ADO Diffs API for the exact `commonCommit`, `aheadCount`, and `behindCount`, then fetches only the source and target ranges -needed to make that base locally reachable. It verifies the server result with +needed to make that base locally reachable. Cross-org preparation runs in a +trusted AzureCLI@3 task and passes its short-lived bearer only to the bundle +child process; the credential is not persisted or exposed to the Agent. It +verifies the server result with `git merge-base --all` before the containerized SafeOutputs MCP server can -generate a patch. Non-Azure/cross-organization/unavailable-REST cases use bounded +generate a patch. Non-Azure/unavailable-REST cases use bounded dual-ref depths 200/500/2000 and fail clearly rather than silently fetching full history. @@ -1342,6 +1349,10 @@ safe-outputs: max: 1 # Maximum per run (default: 1) ``` +Cross-organization tag creation uses the same `repos.organization` and +expanded `permissions.write` contract described under +[`create-pull-request`](#create-pull-request). + ### add-build-tag Adds a tag to an Azure DevOps build. @@ -1378,6 +1389,10 @@ safe-outputs: max: 1 # Maximum per run (default: 1) ``` +Cross-organization branch creation uses the same `repos.organization` and +expanded `permissions.write` contract described under +[`create-pull-request`](#create-pull-request). + ### upload-workitem-attachment Uploads a workspace file as an attachment to an Azure DevOps work item. @@ -1532,7 +1547,7 @@ multiple uploads. **Notes:** - Single-file only; directory uploads are not supported. - When `build_id` is omitted and `allowed-build-ids` is configured, the allow-list check is skipped — the current build is implicitly trusted. -- Requires `BUILD_CONTAINERID`, `BUILD_BUILDID`, and `SYSTEM_TEAMPROJECTID` (all set automatically inside an Azure DevOps pipeline job) and `vso.build_execute` scope on the executor's token (granted to `$(System.AccessToken)` by default, and to the ARM-minted token when `permissions.write` is set). +- Requires `BUILD_CONTAINERID`, `BUILD_BUILDID`, and `SYSTEM_TEAMPROJECTID` (all set automatically inside an Azure DevOps pipeline job) and `vso.build_execute` scope on the executor's token (granted to `$(System.AccessToken)` by default, and to the configured service-connection token when `permissions.write` is set). ### cache-memory (moved to `tools:`) Memory is now configured as a first-class tool under `tools: cache-memory:` instead of `safe-outputs: memory:`. See the [Cache Memory section](./tools.md#cache-memory-cache-memory) in `docs/tools.md` for details. diff --git a/site/src/content/docs/reference/ado-script.mdx b/site/src/content/docs/reference/ado-script.mdx index 56883c4c..a4c24d40 100644 --- a/site/src/content/docs/reference/ado-script.mdx +++ b/site/src/content/docs/reference/ado-script.mdx @@ -24,7 +24,7 @@ pipelines** as runtime helpers. Today it produces fifteen bundles: - **`conclusion.js`** — Conclusion job reporter that files/comments ADO work items for pipeline failures and diagnostic signals (Conclusion job) - **`approval-summary.js`** — renders a sanitized per-tool summary of proposed safe outputs into the build's `ado-aw-safe-outputs` summary tab (end of Agent job) - **`github-app-token.js`** — mints (and revokes) a GitHub App installation token for the Copilot engine when `engine.github-app-token` is configured (Agent + Detection jobs) -- **`prepare-pr-base.js`** — Agent `patch-base` mode uses ADO diff metadata to fetch and verify only the source/target history needed for the merge-base (bounded 200/500/2000 dual-ref fallback); SafeOutputs `target-worktree` mode fetches only the target tip at depth 1 (issue #1453) +- **`prepare-pr-base.js`** — Agent `patch-base` mode uses ADO diff metadata to fetch and verify only the source/target history needed for the merge-base (bounded 200/500/2000 dual-ref fallback); SafeOutputs `target-worktree` mode fetches only the target tip at depth 1. Authorized cross-org repos run in a separate trusted credential scope and must match compiler-resolved organization/project/repository coordinates before receiving the bearer.