From bc0ea0ab41694609885dddb88f7a8e13170907c8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:06:14 +0000 Subject: [PATCH 1/4] Add update-pull-request safe output Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/common.rs | 5 + src/compile/types.rs | 22 + src/execute.rs | 6 +- src/inspect/catalog.rs | 1 + src/mcp.rs | 21 +- src/safe_outputs/mod.rs | 4 + src/safe_outputs/update_pull_request.rs | 687 ++++++++++++++++++++++++ 7 files changed, 740 insertions(+), 6 deletions(-) create mode 100644 src/safe_outputs/update_pull_request.rs diff --git a/src/compile/common.rs b/src/compile/common.rs index b24239e47..53dda72e0 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2427,6 +2427,11 @@ pub fn validate_github_issue_outputs_config(front_matter: &FrontMatter) -> Resul crate::safe_outputs::validate_update_github_issue_config(&config)?; } } + "update-pull-request" => { + if let Some(config) = front_matter.update_pull_request_config()? { + crate::safe_outputs::validate_update_pull_request_config(&config)?; + } + } "set-github-issue-field" => { if let Some(config) = front_matter.set_github_issue_field_config()? { crate::safe_outputs::validate_set_github_issue_field_config(&config)?; diff --git a/src/compile/types.rs b/src/compile/types.rs index 08eb3afe4..7057c6664 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -709,6 +709,7 @@ pub const SAFE_OUTPUTS_GITHUB_ACTOR_LOGIN_ENV: &str = "ADO_AW_GITHUB_ACTOR_LOGIN /// intentionally does not depend on the safe-output module registry. pub const GITHUB_ISSUE_SAFE_OUTPUT_TOOLS: &[&str] = &[ "create-github-issue", + "update-pull-request", "set-github-issue-type", "comment-on-github-issue", "hide-github-issue-comment", @@ -2297,6 +2298,12 @@ impl FrontMatter { self.typed_safe_output_config("update-github-issue") } + pub fn update_pull_request_config( + &self, + ) -> anyhow::Result> { + self.typed_safe_output_config("update-pull-request") + } + pub fn set_github_issue_field_config( &self, ) -> anyhow::Result> { @@ -2344,6 +2351,18 @@ impl FrontMatter { ..Default::default() })) } + "update-pull-request" => { + Ok(self + .update_pull_request_config()? + .map(|config| GithubIssueCompilerConfig { + target_repo: config.target_repo, + allowed_repos: config.allowed_repos, + required_labels: config.required_labels, + required_title_prefix: config.required_title_prefix, + pull_requests: Some(true), + ..Default::default() + })) + } "set-github-issue-type" => { Ok(self .set_github_issue_type_config()? @@ -2494,6 +2513,9 @@ impl FrontMatter { continue; }; match tool.as_str() { + "update-pull-request" => { + pull_requests = true; + } "comment-on-github-issue" | "add-github-issue-labels" | "update-github-issue" => { let tool_issues = config.issues.unwrap_or(true); let tool_pull_requests = config.pull_requests.unwrap_or(false); diff --git a/src/execute.rs b/src/execute.rs index 7740caa53..b3fb6afd5 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -26,8 +26,8 @@ use crate::safe_outputs::{ ReplyToPrCommentResult, ReportIncompleteResult, ResolvePrThreadResult, SetGithubIssueFieldResult, SetGithubIssueTypeResult, SubmitPrReviewResult, ToolResult, UnassignGithubIssueFromUserResult, UpdateGithubIssueResult, UpdatePrResult, - UpdateWikiPageResult, UpdateWorkItemResult, UploadBuildAttachmentResult, - UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, + UpdatePullRequestResult, UpdateWikiPageResult, UpdateWorkItemResult, + UploadBuildAttachmentResult, UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, }; use crate::sanitize::neutralize_pipeline_commands; @@ -259,6 +259,7 @@ pub async fn execute_safe_outputs( AddGithubIssueLabelsResult, RemoveGithubIssueLabelsResult, CloseGithubIssueResult, + UpdatePullRequestResult, UpdateGithubIssueResult, SetGithubIssueFieldResult, AssignGithubIssueMilestoneResult, @@ -777,6 +778,7 @@ async fn dispatch_github_tools( "add-github-issue-labels" => AddGithubIssueLabelsResult, "remove-github-issue-labels" => RemoveGithubIssueLabelsResult, "close-github-issue" => CloseGithubIssueResult, + "update-pull-request" => UpdatePullRequestResult, "update-github-issue" => UpdateGithubIssueResult, "set-github-issue-field" => SetGithubIssueFieldResult, "assign-github-issue-milestone" => AssignGithubIssueMilestoneResult, diff --git a/src/inspect/catalog.rs b/src/inspect/catalog.rs index 2c2796d79..d336c52a0 100644 --- a/src/inspect/catalog.rs +++ b/src/inspect/catalog.rs @@ -310,6 +310,7 @@ fn safe_output_description(name: &str) -> &'static str { "set-github-issue-field" => "Sets a repository-defined field on a GitHub issue", "submit-pr-review" => "Parameters for submitting a pull request review", "update-pr" => "Parameters for updating a pull request", + "update-pull-request" => "Updates a GitHub pull request title, body, or branch", "unassign-github-issue-from-user" => { "Removes operator-permitted GitHub users from an issue" } diff --git a/src/mcp.rs b/src/mcp.rs index 17af63e48..c1b80f9bb 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -30,10 +30,11 @@ use crate::safe_outputs::{ SetGithubIssueFieldResult, SetGithubIssueTypeParams, SetGithubIssueTypeResult, SubmitPrReviewParams, SubmitPrReviewResult, ToolResult, UnassignGithubIssueFromUserParams, UnassignGithubIssueFromUserResult, UpdateGithubIssueParams, UpdateGithubIssueResult, - UpdatePrParams, UpdatePrResult, UpdateWikiPageParams, UpdateWikiPageResult, - UpdateWorkItemParams, UpdateWorkItemResult, UploadBuildAttachmentParams, - UploadBuildAttachmentResult, UploadPipelineArtifactParams, UploadPipelineArtifactResult, - UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, Validate, anyhow_to_mcp_error, + UpdatePrParams, UpdatePrResult, UpdatePullRequestParams, UpdatePullRequestResult, + UpdateWikiPageParams, UpdateWikiPageResult, UpdateWorkItemParams, UpdateWorkItemResult, + UploadBuildAttachmentParams, UploadBuildAttachmentResult, UploadPipelineArtifactParams, + UploadPipelineArtifactResult, UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, + Validate, anyhow_to_mcp_error, }; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_markdown}; use crate::secure::WorkItemTemporaryId; @@ -957,6 +958,18 @@ issue_number may be a positive number or a temporary_id from create-github-issue self.queue_sanitized_output(result).await } + #[tool( + name = "update-pull-request", + description = "Update a configured GitHub pull request title or body, or update its branch from the base branch." + )] + async fn update_pull_request( + &self, + params: Parameters, + ) -> Result { + let result: UpdatePullRequestResult = params.0.try_into()?; + self.queue_sanitized_output(result).await + } + #[tool( name = "set-github-issue-field", description = "Set an operator-permitted repository-defined field on a configured GitHub issue." diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index bd86a5085..38767834f 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -48,6 +48,7 @@ pub const DEBUG_ONLY_TOOLS: &[&str] = &[]; pub const CONFIGURED_ONLY_TOOLS: &[&str] = tool_names![ AssignWorkItemResult, CreateGithubIssueResult, + UpdatePullRequestResult, SetGithubIssueTypeResult, CommentOnGithubIssueResult, HideGithubIssueCommentResult, @@ -92,6 +93,7 @@ pub const ALL_KNOWN_SAFE_OUTPUTS: &[&str] = all_safe_output_names![ ReplyToPrCommentResult, ResolvePrThreadResult, CreateGithubIssueResult, + UpdatePullRequestResult, SetGithubIssueTypeResult, CommentOnGithubIssueResult, HideGithubIssueCommentResult, @@ -796,6 +798,7 @@ mod set_github_issue_type; mod submit_pr_review; mod unassign_github_issue_from_user; mod update_github_issue; +mod update_pull_request; mod update_pr; mod update_wiki_page; mod update_work_item; @@ -840,6 +843,7 @@ pub use set_github_issue_type::*; pub use submit_pr_review::*; pub use unassign_github_issue_from_user::*; pub use update_github_issue::*; +pub use update_pull_request::*; pub use update_pr::*; pub use update_wiki_page::*; pub use update_work_item::*; diff --git a/src/safe_outputs/update_pull_request.rs b/src/safe_outputs/update_pull_request.rs new file mode 100644 index 000000000..3b928f7df --- /dev/null +++ b/src/safe_outputs/update_pull_request.rs @@ -0,0 +1,687 @@ +//! `update-pull-request` GitHub safe output. + +use anyhow::ensure; +use log::{debug, info, warn}; +use reqwest::Method; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use serde_json::{Map, Value}; +use url::Url; + +use ado_aw_derive::SanitizeConfig; + +use crate::safe_outputs::{ + ExecutionContext, ExecutionResult, Executor, GithubBodyOperation, GithubClient, + GithubMutationFilters, GithubRepositoryPolicy, GithubTargetCapabilities, GithubTargetKind, + GithubTargetMetadata, Validate, build_github_trace_footer, resolve_github_repository, + validate_github_mutation_filter_config, validate_github_mutation_filters, + validate_github_repository, validate_github_target_capability, +}; +use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; +use crate::tool_result; + +const MAX_TITLE_LEN: usize = 256; +const MAX_BODY_LEN: usize = 65_536; + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(untagged)] +pub enum GithubPullRequestNumber { + Number(u64), + String(String), +} + +impl GithubPullRequestNumber { + fn parse(&self, field: &str) -> anyhow::Result { + let number = match self { + Self::Number(number) => *number, + Self::String(value) => value + .trim() + .strip_prefix('#') + .unwrap_or_else(|| value.trim()) + .parse::() + .map_err(|_| anyhow::anyhow!("{field} must be a positive pull request number"))?, + }; + ensure!(number > 0, "{field} must be positive"); + Ok(number) + } +} + +#[derive(Deserialize, JsonSchema)] +pub struct UpdatePullRequestParams { + /// New pull request title. + #[serde(default)] + pub title: Option, + /// Pull request body content in Markdown. + #[serde(default)] + pub body: Option, + /// Body update operation. Defaults to the configured operation, then `replace`. + #[serde(default)] + pub operation: Option, + /// When true, update the PR branch from the base branch before other updates. + #[serde(default, rename = "update_branch", alias = "updateBranch")] + pub update_branch: Option, + /// Pull request number. Required when front matter uses `target: "*"`. + #[serde(default, rename = "pull_request_number", alias = "pullRequestNumber")] + pub pull_request_number: Option, + /// Alias for pull_request_number. + #[serde(default, rename = "pr_number", alias = "prNumber")] + pub pr_number: Option, + /// Alias for pull_request_number. + #[serde(default)] + pub pr: Option, + /// Optional target repository. + #[serde(default)] + pub repository: Option, +} + +impl UpdatePullRequestParams { + fn requested_number(&self) -> anyhow::Result> { + let mut found = None; + for (field, value) in [ + ("pull_request_number", self.pull_request_number.as_ref()), + ("pr_number", self.pr_number.as_ref()), + ("pr", self.pr.as_ref()), + ] { + if let Some(value) = value { + let number = value.parse(field)?; + if let Some(existing) = found { + ensure!( + existing == number, + "pull request number aliases must all refer to the same PR" + ); + } + found = Some(number); + } + } + Ok(found) + } +} + +impl Validate for UpdatePullRequestParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!( + self.title.is_some() || self.body.is_some() || self.update_branch == Some(true), + "at least one of title, body, or update_branch: true is required" + ); + if let Some(title) = self.title.as_deref() { + ensure!(!title.trim().is_empty(), "title must not be empty"); + ensure!( + title.len() <= MAX_TITLE_LEN, + "title must be {MAX_TITLE_LEN} characters or fewer" + ); + } + if let Some(body) = self.body.as_deref() { + ensure!( + body.len() <= MAX_BODY_LEN, + "body must be {MAX_BODY_LEN} characters or fewer" + ); + } else { + ensure!( + self.operation.is_none(), + "operation may only be provided when body is provided" + ); + } + if let Some(repository) = self.repository.as_deref() { + validate_github_repository(repository)?; + } + let _ = self.requested_number()?; + Ok(()) + } +} + +tool_result! { + name = "update-pull-request", + write = true, + params = UpdatePullRequestParams, + default_max = 1, + /// Result of updating a GitHub pull request. + pub struct UpdatePullRequestResult { + #[serde(default)] + title: Option, + #[serde(default)] + body: Option, + #[serde(default)] + operation: Option, + #[serde(default, rename = "update_branch")] + update_branch: Option, + #[serde(default, rename = "pull_request_number")] + pull_request_number: Option, + #[serde(default, rename = "pr_number")] + pr_number: Option, + #[serde(default)] + pr: Option, + #[serde(default)] + repository: Option, + } +} + +impl SanitizeContent for UpdatePullRequestResult { + fn sanitize_content_fields(&mut self) { + self.title = self.title.as_deref().map(sanitize_text); + self.body = self.body.as_deref().map(sanitize_text); + self.repository = self.repository.as_deref().map(sanitize_config); + } +} + +fn default_true() -> bool { + true +} + +fn default_operation() -> GithubBodyOperation { + GithubBodyOperation::Replace +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(untagged)] +pub enum UpdatePullRequestTarget { + Number(u64), + Named(String), +} + +impl Default for UpdatePullRequestTarget { + fn default() -> Self { + Self::Named("triggering".to_string()) + } +} + +#[derive(Debug, Clone, SanitizeConfig, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct UpdatePullRequestConfig { + /// Whether title updates are enabled. Defaults to true. + #[serde(default = "default_true")] + #[sanitize_config(skip)] + pub title: bool, + /// Whether body updates are enabled. Defaults to true. + #[serde(default = "default_true")] + #[sanitize_config(skip)] + pub body: bool, + /// Default branch update policy. Defaults to false. + #[serde(default, rename = "update-branch")] + #[sanitize_config(skip)] + pub update_branch: bool, + /// gh-aw-compatible stacked-PR fallback knob. Parsed for config parity. + #[serde(default = "default_true", rename = "sync-stack")] + #[sanitize_config(skip)] + pub sync_stack: bool, + /// Include the standard ado-aw trace footer in body updates. + #[serde(default = "default_true")] + #[sanitize_config(skip)] + pub footer: bool, + /// Body update operation. Defaults to replace. + #[serde(default = "default_operation")] + #[sanitize_config(skip)] + pub operation: GithubBodyOperation, + /// `"triggering"` (default), `"*"`, or a fixed PR number. + #[serde(default)] + pub target: UpdatePullRequestTarget, + #[serde(default, rename = "target-repo")] + pub target_repo: Option, + #[serde(default, rename = "allowed-repos")] + pub allowed_repos: Vec, + #[serde(default, rename = "required-labels")] + pub required_labels: Vec, + #[serde(default, rename = "required-title-prefix")] + pub required_title_prefix: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + #[sanitize_config(skip)] + pub max: Option, +} + +impl Default for UpdatePullRequestConfig { + fn default() -> Self { + Self { + title: true, + body: true, + update_branch: false, + sync_stack: true, + footer: true, + operation: GithubBodyOperation::Replace, + target: UpdatePullRequestTarget::default(), + target_repo: None, + allowed_repos: Vec::new(), + required_labels: Vec::new(), + required_title_prefix: None, + max: None, + } + } +} + +pub(crate) fn validate_update_pull_request_config( + config: &UpdatePullRequestConfig, +) -> anyhow::Result<()> { + match &config.target { + UpdatePullRequestTarget::Number(number) => { + ensure!(*number > 0, "target PR number must be positive"); + } + UpdatePullRequestTarget::Named(target) => { + ensure!( + matches!(target.as_str(), "triggering" | "*"), + "target must be \"triggering\", \"*\", or a positive pull request number" + ); + } + } + validate_github_mutation_filter_config(GithubMutationFilters { + required_labels: &config.required_labels, + required_title_prefix: config.required_title_prefix.as_deref(), + })?; + Ok(()) +} + +#[derive(Debug, Deserialize)] +struct RawPullRequestTarget { + number: u64, + title: String, + #[serde(default)] + body: Option, + state: String, + #[serde(default)] + labels: Vec, + html_url: Option, +} + +#[derive(Debug, Deserialize)] +struct RawLabel { + name: String, +} + +impl RawPullRequestTarget { + fn metadata(&self) -> GithubTargetMetadata { + GithubTargetMetadata { + number: self.number, + node_id: None, + title: self.title.clone(), + state: self.state.clone(), + labels: self.labels.iter().map(|label| label.name.clone()).collect(), + kind: GithubTargetKind::PullRequest, + html_url: self.html_url.clone(), + } + } +} + +fn repository_route(client: &GithubClient, repository: &str, tail: &[&str]) -> anyhow::Result { + validate_github_repository(repository)?; + let (owner, name) = repository + .split_once('/') + .expect("validated GitHub repository contains slash"); + let mut url = client.rest_api_url().clone(); + { + let mut path = url + .path_segments_mut() + .map_err(|_| anyhow::anyhow!("GitHub API URL cannot be a base URL"))?; + path.pop_if_empty(); + path.push("repos"); + path.push(owner); + path.push(name); + for segment in tail { + path.push(segment); + } + } + Ok(url) +} + +fn body_with_footer(body: &str, include_footer: bool, ctx: &ExecutionContext) -> String { + if include_footer { + format!("{body}\n\n{}", build_github_trace_footer(ctx)) + } else { + body.to_string() + } +} + +fn island_markers(ctx: &ExecutionContext) -> Result<(String, String), ExecutionResult> { + let Some(definition_id) = ctx.definition_id else { + return Err(ExecutionResult::failure( + "SYSTEM_DEFINITIONID is required for replace-island", + )); + }; + Ok(( + format!(""), + format!(""), + )) +} + +fn replace_island( + current: &str, + replacement: &str, + ctx: &ExecutionContext, +) -> Result { + let (start_marker, end_marker) = island_markers(ctx)?; + let starts: Vec = current + .match_indices(&start_marker) + .map(|(index, _)| index) + .collect(); + let ends: Vec = current + .match_indices(&end_marker) + .map(|(index, _)| index) + .collect(); + if starts.len() != 1 || ends.len() != 1 { + let island = format!("{start_marker}\n{replacement}\n{end_marker}"); + return Ok(if current.is_empty() { + island + } else { + format!("{current}\n\n---\n\n{island}") + }); + } + let start = starts[0]; + let end = ends[0]; + if end <= start { + return Err(ExecutionResult::failure( + "replace-island markers are out of order", + )); + } + let end_after_marker = end + end_marker.len(); + Ok(format!( + "{}{}\n{}\n{}{}", + ¤t[..start], + start_marker, + replacement, + end_marker, + ¤t[end_after_marker..] + )) +} + +fn build_updated_body( + current: &str, + new_content: &str, + operation: GithubBodyOperation, + include_footer: bool, + ctx: &ExecutionContext, +) -> Result { + let section = body_with_footer(new_content, include_footer, ctx); + let updated = match operation { + GithubBodyOperation::Append => { + if current.is_empty() { + section + } else { + format!("{current}\n\n---\n\n{section}") + } + } + GithubBodyOperation::Prepend => { + if current.is_empty() { + section + } else { + format!("{section}\n\n---\n\n{current}") + } + } + GithubBodyOperation::Replace => section, + GithubBodyOperation::ReplaceIsland => replace_island(current, §ion, ctx)?, + }; + if updated.len() > MAX_BODY_LEN { + return Err(ExecutionResult::failure(format!( + "updated body exceeds GitHub's {MAX_BODY_LEN}-character limit" + ))); + } + Ok(updated) +} + +impl UpdatePullRequestResult { + fn requested_number(&self) -> anyhow::Result> { + let params = UpdatePullRequestParams { + title: self.title.clone(), + body: self.body.clone(), + operation: self.operation, + update_branch: self.update_branch, + pull_request_number: self.pull_request_number.clone(), + pr_number: self.pr_number.clone(), + pr: self.pr.clone(), + repository: self.repository.clone(), + }; + params.requested_number() + } + + fn requested_fields(&self, config: &UpdatePullRequestConfig) -> Vec<&'static str> { + let mut fields = Vec::new(); + if self.title.is_some() { + fields.push("title"); + } + if self.body.is_some() { + fields.push("body"); + } + if self.update_branch.unwrap_or(config.update_branch) { + fields.push("update_branch"); + } + fields + } + + fn resolve_number( + &self, + config: &UpdatePullRequestConfig, + ) -> Result { + let requested = self + .requested_number() + .map_err(|error| ExecutionResult::failure(error.to_string()))?; + match &config.target { + UpdatePullRequestTarget::Number(number) => { + if let Some(requested) = requested + && requested != *number + { + return Err(ExecutionResult::failure(format!( + "requested pull_request_number #{requested} does not match configured target #{number}" + ))); + } + Ok(*number) + } + UpdatePullRequestTarget::Named(target) if target == "*" => { + requested.ok_or_else(|| { + ExecutionResult::failure( + "pull_request_number is required when safe-outputs.update-pull-request.target is \"*\"", + ) + }) + } + UpdatePullRequestTarget::Named(target) if target == "triggering" => { + requested.ok_or_else(|| { + ExecutionResult::failure( + "SYSTEM_PULLREQUEST_PULLREQUESTID is required for target \"triggering\"", + ) + }) + } + UpdatePullRequestTarget::Named(target) => Err(ExecutionResult::failure(format!( + "unsupported update-pull-request target '{}'", + crate::sanitize::neutralize_pipeline_commands(target) + ))), + } + } + + async fn fetch_target( + &self, + client: &GithubClient, + repository: &str, + number: u64, + ) -> anyhow::Result> { + let response = client + .send( + Method::GET, + repository_route(client, repository, &["pulls", &number.to_string()])?, + None, + ) + .await?; + let response = match response.require_success("Failed to fetch GitHub pull request") { + Ok(response) => response, + Err(error) => return Ok(Err(ExecutionResult::failure(error.to_string()))), + }; + match response.json("Failed to parse GitHub pull request") { + Ok(target) => Ok(Ok(target)), + Err(error) => Ok(Err(ExecutionResult::failure(error.to_string()))), + } + } + + async fn update_branch( + &self, + client: &GithubClient, + repository: &str, + number: u64, + ) -> anyhow::Result> { + let response = client + .send( + Method::PUT, + repository_route( + client, + repository, + &["pulls", &number.to_string(), "update-branch"], + )?, + None, + ) + .await?; + if response.is_success() { + return Ok(Ok(())); + } + let error = response + .require_success("Failed to update GitHub pull request branch") + .expect_err("non-success response must produce an API error"); + let message = error.message.to_ascii_lowercase(); + if message.contains("there are no new commits on the base branch") + || message.contains("merge conflict between base and head") + || message.contains("head ref does not exist") + { + warn!("Non-fatal update-pull-request branch update failure: {error}"); + return Ok(Ok(())); + } + Ok(Err(ExecutionResult::failure(error.to_string()))) + } +} + +fn ctx_pull_request_id_from(ctx: &ExecutionContext) -> Result { + let raw = ctx.pull_request_id.as_deref().ok_or_else(|| { + ExecutionResult::failure("SYSTEM_PULLREQUEST_PULLREQUESTID is required for target \"triggering\"") + })?; + raw.parse::() + .ok() + .filter(|number| *number > 0) + .ok_or_else(|| { + ExecutionResult::failure(format!( + "SYSTEM_PULLREQUEST_PULLREQUESTID '{}' is not a positive pull request number", + crate::sanitize::neutralize_pipeline_commands(raw) + )) + }) +} + +#[async_trait::async_trait] +impl Executor for UpdatePullRequestResult { + fn dry_run_summary(&self) -> String { + "update GitHub pull request".to_string() + } + + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + if !ctx.tool_configs.contains_key("update-pull-request") { + return Ok(ExecutionResult::failure( + "update-pull-request is not configured for this workflow", + )); + } + let Some(token) = ctx.github_token.as_ref() else { + return Ok(ExecutionResult::failure( + "ADO_AW_GITHUB_TOKEN is not set; configure safe-outputs.github-token \ + or safe-outputs.github-app", + )); + }; + let config: UpdatePullRequestConfig = ctx.get_tool_config("update-pull-request")?; + validate_update_pull_request_config(&config)?; + if self.title.is_some() && !config.title { + return Ok(ExecutionResult::failure( + "update-pull-request field 'title' is not enabled by configuration", + )); + } + if self.body.is_some() && !config.body { + return Ok(ExecutionResult::failure( + "update-pull-request field 'body' is not enabled by configuration", + )); + } + let number = match &config.target { + UpdatePullRequestTarget::Named(target) if target == "triggering" => { + match self.requested_number()? { + Some(number) => number, + None => match ctx_pull_request_id_from(ctx) { + Ok(number) => number, + Err(result) => return Ok(result), + }, + } + } + _ => match self.resolve_number(&config) { + Ok(number) => number, + Err(result) => return Ok(result), + }, + }; + let repository = match resolve_github_repository( + self.repository.as_deref(), + GithubRepositoryPolicy::new(config.target_repo.as_deref(), &config.allowed_repos), + ctx, + ) { + Ok(repository) => repository, + Err(result) => return Ok(result), + }; + let client = GithubClient::new(&ctx.github_api_url, token)?; + let current = match self.fetch_target(&client, &repository, number).await? { + Ok(target) => target, + Err(result) => return Ok(result), + }; + let metadata = current.metadata(); + if let Err(result) = validate_github_target_capability( + &metadata, + GithubTargetCapabilities { + issues: false, + pull_requests: true, + }, + ) { + return Ok(result); + } + let filters = GithubMutationFilters { + required_labels: &config.required_labels, + required_title_prefix: config.required_title_prefix.as_deref(), + }; + if let Err(result) = validate_github_mutation_filters(&metadata, filters) { + return Ok(result); + } + let update_branch = self.update_branch.unwrap_or(config.update_branch); + if update_branch + && let Err(result) = self.update_branch(&client, &repository, number).await? + { + return Ok(result); + } + + let mut payload = Map::new(); + if let Some(title) = self.title.as_ref() { + payload.insert("title".to_string(), Value::String(title.clone())); + } + if let Some(body) = self.body.as_deref() { + let updated = match build_updated_body( + current.body.as_deref().unwrap_or_default(), + body, + self.operation.unwrap_or(config.operation), + config.footer, + ctx, + ) { + Ok(body) => body, + Err(result) => return Ok(result), + }; + payload.insert("body".to_string(), Value::String(updated)); + } + if !payload.is_empty() { + debug!("Updating GitHub pull request {repository}#{number}"); + let response = client + .send( + Method::PATCH, + repository_route(&client, &repository, &["pulls", &number.to_string()])?, + Some(&Value::Object(payload)), + ) + .await?; + if !response.is_success() { + let error = response + .require_success("Failed to update GitHub pull request") + .expect_err("non-success response must produce an API error"); + return Ok(ExecutionResult::failure(error.to_string())); + } + } + + info!("Updated GitHub pull request {repository}#{number}"); + Ok(ExecutionResult::success_with_data( + format!( + "Updated GitHub pull request {repository}#{number}: {}", + self.requested_fields(&config).join(", ") + ), + serde_json::json!({ + "number": number, + "target_repo": repository, + "pull_request_url": metadata.html_url, + "fields": self.requested_fields(&config), + }), + )) + } +} From 772282eafd485914cff27ff0e4c8445854b26007 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:10:52 +0000 Subject: [PATCH 2/4] Document and test update-pull-request safe output Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- docs/safe-outputs.md | 42 +++++ src/safe_outputs/update_pull_request.rs | 239 ++++++++++++++++++++++-- 2 files changed, 261 insertions(+), 20 deletions(-) diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index c93a07934..e6f40f163 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -828,6 +828,48 @@ their complete existing lists, and `milestone` selects an existing milestone by positive number. All requested changes are preflighted before the first write. +#### Pull request updates (`update-pull-request`) + +`update-pull-request` matches gh-aw's front matter configuration for updating +GitHub pull requests. It updates the PR title or body; both fields are enabled +by default. The `operation` field controls body updates: `replace` (default), +`append`, `prepend`, or `replace-island`. + +```yaml +safe-outputs: + update-pull-request: + title: true # enable title updates (default: true) + body: true # enable body updates (default: true) + update-branch: false # update the PR branch from its base first (default: false) + sync-stack: true # accepted for gh-aw configuration parity + footer: false # omit the ado-aw trace footer from body updates (default: true) + operation: replace # replace, append, prepend, or replace-island + max: 1 # maximum updates per run (default: 1) + target: "*" # "triggering" (default), "*", or a PR number + target-repo: octo-org/octo-repo + allowed-repos: [octo-org/octo-repo] + required-labels: [automated] + required-title-prefix: "[bot] " +``` + +**Agent parameters:** + +- `title` *(optional)* - Replacement PR title. +- `body` *(optional)* - PR body content in Markdown. +- `operation` *(optional)* - Overrides the configured body operation for this + update. +- `update_branch` *(optional)* - When `true`, update the PR branch with the + latest base branch changes before title/body updates. +- `pull_request_number`, `pr_number`, or `pr` - Required when `target: "*"` is + configured. With `target: "triggering"`, any supplied number must match the + triggering PR. +- `repository` *(optional)* - Target repository, constrained by `target-repo` + and `allowed-repos`. + +`update-branch` calls GitHub's pull request branch update API. GitHub's benign +"already up to date", merge-conflict, and missing-head responses are treated as +non-fatal, matching gh-aw's best-effort behavior. + #### Fields, milestones, and assignees `set-github-issue-field` rejects built-in fields and limits repository-defined diff --git a/src/safe_outputs/update_pull_request.rs b/src/safe_outputs/update_pull_request.rs index 3b928f7df..683864908 100644 --- a/src/safe_outputs/update_pull_request.rs +++ b/src/safe_outputs/update_pull_request.rs @@ -445,6 +445,7 @@ impl UpdatePullRequestResult { fn resolve_number( &self, config: &UpdatePullRequestConfig, + ctx: &ExecutionContext, ) -> Result { let requested = self .requested_number() @@ -468,11 +469,15 @@ impl UpdatePullRequestResult { }) } UpdatePullRequestTarget::Named(target) if target == "triggering" => { - requested.ok_or_else(|| { - ExecutionResult::failure( - "SYSTEM_PULLREQUEST_PULLREQUESTID is required for target \"triggering\"", - ) - }) + let triggering = ctx_pull_request_id_from(ctx)?; + if let Some(requested) = requested + && requested != triggering + { + return Err(ExecutionResult::failure(format!( + "requested pull_request_number #{requested} does not match triggering pull request #{triggering}" + ))); + } + Ok(triggering) } UpdatePullRequestTarget::Named(target) => Err(ExecutionResult::failure(format!( "unsupported update-pull-request target '{}'", @@ -541,7 +546,9 @@ impl UpdatePullRequestResult { fn ctx_pull_request_id_from(ctx: &ExecutionContext) -> Result { let raw = ctx.pull_request_id.as_deref().ok_or_else(|| { - ExecutionResult::failure("SYSTEM_PULLREQUEST_PULLREQUESTID is required for target \"triggering\"") + ExecutionResult::failure( + "SYSTEM_PULLREQUEST_PULLREQUESTID is required for target \"triggering\"", + ) })?; raw.parse::() .ok() @@ -561,6 +568,19 @@ impl Executor for UpdatePullRequestResult { } async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + let params = UpdatePullRequestParams { + title: self.title.clone(), + body: self.body.clone(), + operation: self.operation, + update_branch: self.update_branch, + pull_request_number: self.pull_request_number.clone(), + pr_number: self.pr_number.clone(), + pr: self.pr.clone(), + repository: self.repository.clone(), + }; + if let Err(error) = params.validate() { + return Ok(ExecutionResult::failure(error.to_string())); + } if !ctx.tool_configs.contains_key("update-pull-request") { return Ok(ExecutionResult::failure( "update-pull-request is not configured for this workflow", @@ -584,20 +604,9 @@ impl Executor for UpdatePullRequestResult { "update-pull-request field 'body' is not enabled by configuration", )); } - let number = match &config.target { - UpdatePullRequestTarget::Named(target) if target == "triggering" => { - match self.requested_number()? { - Some(number) => number, - None => match ctx_pull_request_id_from(ctx) { - Ok(number) => number, - Err(result) => return Ok(result), - }, - } - } - _ => match self.resolve_number(&config) { - Ok(number) => number, - Err(result) => return Ok(result), - }, + let number = match self.resolve_number(&config, ctx) { + Ok(number) => number, + Err(result) => return Ok(result), }; let repository = match resolve_github_repository( self.repository.as_deref(), @@ -685,3 +694,193 @@ impl Executor for UpdatePullRequestResult { )) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::safe_outputs::ToolResult; + use std::collections::HashMap; + use wiremock::matchers::{body_json, method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + fn context(server: &MockServer, config: serde_json::Value) -> ExecutionContext { + let mut tool_configs = HashMap::new(); + tool_configs.insert("update-pull-request".to_string(), config); + ExecutionContext { + github_token: Some("token".to_string()), + github_api_url: server.uri(), + repository_provider: Some("GitHub".to_string()), + repository_name: Some("octo/repo".to_string()), + pull_request_id: Some("7".to_string()), + tool_configs, + definition_id: Some(123), + ..Default::default() + } + } + + fn pull_request(number: u64) -> serde_json::Value { + serde_json::json!({ + "number": number, + "title": "[bot] Existing", + "body": "Existing body", + "state": "open", + "labels": [{"name": "automated"}], + "html_url": format!("https://github.example/octo/repo/pull/{number}") + }) + } + + fn params() -> UpdatePullRequestParams { + UpdatePullRequestParams { + title: Some("Updated title".to_string()), + body: None, + operation: None, + update_branch: None, + pull_request_number: None, + pr_number: None, + pr: None, + repository: None, + } + } + + #[test] + fn contract_name_and_budget() { + assert_eq!(UpdatePullRequestResult::NAME, "update-pull-request"); + assert_eq!(UpdatePullRequestResult::DEFAULT_MAX, 1); + } + + #[test] + fn config_matches_gh_aw_defaults_and_rejects_unknown_fields() { + let config: UpdatePullRequestConfig = + serde_json::from_value(serde_json::json!({})).unwrap(); + assert!(config.title); + assert!(config.body); + assert!(!config.update_branch); + assert!(config.sync_stack); + assert!(config.footer); + assert_eq!(config.operation, GithubBodyOperation::Replace); + assert!( + serde_json::from_value::(serde_json::json!({ + "allow-title": true + })) + .is_err() + ); + } + + #[test] + fn validates_meaningful_update_and_number_aliases() { + let mut empty = params(); + empty.title = None; + assert!(empty.validate().is_err()); + + let mut aliases = params(); + aliases.pull_request_number = Some(GithubPullRequestNumber::Number(1)); + aliases.pr_number = Some(GithubPullRequestNumber::String("#2".to_string())); + assert!(aliases.validate().is_err()); + } + + #[tokio::test] + async fn updates_triggering_pull_request_title_and_body() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/octo/repo/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(pull_request(7))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path("/repos/octo/repo/pulls/7")) + .and(body_json(serde_json::json!({ + "title": "Updated title", + "body": "Existing body\n\n---\n\nNew body" + }))) + .respond_with(ResponseTemplate::new(200).set_body_json(pull_request(7))) + .expect(1) + .mount(&server) + .await; + let ctx = context( + &server, + serde_json::json!({ + "target-repo": "octo/repo", + "title": true, + "body": true, + "footer": false, + "operation": "append", + "required-labels": ["automated"], + "required-title-prefix": "[bot]" + }), + ); + let mut result: UpdatePullRequestResult = UpdatePullRequestParams { + title: Some("Updated title".to_string()), + body: Some("New body".to_string()), + operation: None, + update_branch: None, + pull_request_number: None, + pr_number: None, + pr: None, + repository: None, + } + .try_into() + .unwrap(); + let execution = result.execute_sanitized(&ctx).await.unwrap(); + assert!(execution.success, "{}", execution.message); + } + + #[tokio::test] + async fn target_star_requires_agent_pull_request_number() { + let server = MockServer::start().await; + let ctx = context( + &server, + serde_json::json!({ + "target-repo": "octo/repo", + "target": "*" + }), + ); + let mut result: UpdatePullRequestResult = params().try_into().unwrap(); + let execution = result.execute_sanitized(&ctx).await.unwrap(); + assert!(!execution.success); + assert!( + execution + .message + .contains("pull_request_number is required") + ); + assert!(server.received_requests().await.unwrap().is_empty()); + } + + #[tokio::test] + async fn update_branch_only_calls_update_branch_endpoint() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/octo/repo/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(pull_request(7))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("PUT")) + .and(path("/repos/octo/repo/pulls/7/update-branch")) + .respond_with(ResponseTemplate::new(202).set_body_json(serde_json::json!({}))) + .expect(1) + .mount(&server) + .await; + let ctx = context( + &server, + serde_json::json!({ + "target-repo": "octo/repo", + "update-branch": true + }), + ); + let mut result: UpdatePullRequestResult = UpdatePullRequestParams { + title: None, + body: None, + operation: None, + update_branch: Some(true), + pull_request_number: None, + pr_number: None, + pr: None, + repository: None, + } + .try_into() + .unwrap(); + let execution = result.execute_sanitized(&ctx).await.unwrap(); + assert!(execution.success, "{}", execution.message); + } +} From 2b086575246472d650b273570ffef07fdd8aa6ac Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:18:19 +0000 Subject: [PATCH 3/4] Address update-pull-request review feedback Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/safe_outputs/mod.rs | 4 +- src/safe_outputs/update_pull_request.rs | 88 ++++++++++++++++++++----- 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index 38767834f..b381c52e4 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -798,8 +798,8 @@ mod set_github_issue_type; mod submit_pr_review; mod unassign_github_issue_from_user; mod update_github_issue; -mod update_pull_request; mod update_pr; +mod update_pull_request; mod update_wiki_page; mod update_work_item; mod upload_build_attachment; @@ -843,8 +843,8 @@ pub use set_github_issue_type::*; pub use submit_pr_review::*; pub use unassign_github_issue_from_user::*; pub use update_github_issue::*; -pub use update_pull_request::*; pub use update_pr::*; +pub use update_pull_request::*; pub use update_wiki_page::*; pub use update_work_item::*; pub use upload_build_attachment::*; diff --git a/src/safe_outputs/update_pull_request.rs b/src/safe_outputs/update_pull_request.rs index 683864908..4df698124 100644 --- a/src/safe_outputs/update_pull_request.rs +++ b/src/safe_outputs/update_pull_request.rs @@ -20,8 +20,8 @@ use crate::safe_outputs::{ use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; -const MAX_TITLE_LEN: usize = 256; -const MAX_BODY_LEN: usize = 65_536; +const MAX_TITLE_CHARS: usize = 256; +const MAX_BODY_CHARS: usize = 65_536; #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] #[serde(untagged)] @@ -99,21 +99,17 @@ impl UpdatePullRequestParams { impl Validate for UpdatePullRequestParams { fn validate(&self) -> anyhow::Result<()> { - ensure!( - self.title.is_some() || self.body.is_some() || self.update_branch == Some(true), - "at least one of title, body, or update_branch: true is required" - ); if let Some(title) = self.title.as_deref() { ensure!(!title.trim().is_empty(), "title must not be empty"); ensure!( - title.len() <= MAX_TITLE_LEN, - "title must be {MAX_TITLE_LEN} characters or fewer" + title.chars().count() <= MAX_TITLE_CHARS, + "title must be {MAX_TITLE_CHARS} characters or fewer" ); } if let Some(body) = self.body.as_deref() { ensure!( - body.len() <= MAX_BODY_LEN, - "body must be {MAX_BODY_LEN} characters or fewer" + body.chars().count() <= MAX_BODY_CHARS, + "body must be {MAX_BODY_CHARS} characters or fewer" ); } else { ensure!( @@ -405,9 +401,9 @@ fn build_updated_body( GithubBodyOperation::Replace => section, GithubBodyOperation::ReplaceIsland => replace_island(current, §ion, ctx)?, }; - if updated.len() > MAX_BODY_LEN { + if updated.chars().count() > MAX_BODY_CHARS { return Err(ExecutionResult::failure(format!( - "updated body exceeds GitHub's {MAX_BODY_LEN}-character limit" + "updated body exceeds GitHub's {MAX_BODY_CHARS}-character limit" ))); } Ok(updated) @@ -604,6 +600,12 @@ impl Executor for UpdatePullRequestResult { "update-pull-request field 'body' is not enabled by configuration", )); } + let update_branch = self.update_branch.unwrap_or(config.update_branch); + if self.title.is_none() && self.body.is_none() && !update_branch { + return Ok(ExecutionResult::failure( + "at least one of title, body, or effective update_branch: true is required", + )); + } let number = match self.resolve_number(&config, ctx) { Ok(number) => number, Err(result) => return Ok(result), @@ -638,7 +640,6 @@ impl Executor for UpdatePullRequestResult { if let Err(result) = validate_github_mutation_filters(&metadata, filters) { return Ok(result); } - let update_branch = self.update_branch.unwrap_or(config.update_branch); if update_branch && let Err(result) = self.update_branch(&client, &repository, number).await? { @@ -767,10 +768,10 @@ mod tests { } #[test] - fn validates_meaningful_update_and_number_aliases() { + fn validates_number_aliases() { let mut empty = params(); empty.title = None; - assert!(empty.validate().is_err()); + assert!(empty.validate().is_ok()); let mut aliases = params(); aliases.pull_request_number = Some(GithubPullRequestNumber::Number(1)); @@ -778,6 +779,61 @@ mod tests { assert!(aliases.validate().is_err()); } + #[test] + fn replace_island_appends_then_replaces_pipeline_scoped_section() { + let ctx = ExecutionContext { + definition_id: Some(123), + ..Default::default() + }; + let first = build_updated_body( + "before", + "new", + GithubBodyOperation::ReplaceIsland, + false, + &ctx, + ) + .unwrap(); + assert!(first.contains("before\n\n---\n\n")); + assert!(first.contains("")); + assert!(first.contains("\nnew\n")); + + let second = build_updated_body( + &first, + "next", + GithubBodyOperation::ReplaceIsland, + false, + &ctx, + ) + .unwrap(); + assert!(second.contains("\nnext\n")); + assert!(!second.contains("\nnew\n")); + } + + #[test] + fn replace_island_requires_definition_id_and_ordered_markers() { + let missing_id = ExecutionContext::default(); + assert!( + build_updated_body( + "", + "new", + GithubBodyOperation::ReplaceIsland, + false, + &missing_id + ) + .is_err() + ); + + let ctx = ExecutionContext { + definition_id: Some(123), + ..Default::default() + }; + let bad = "\nold\n"; + assert!( + build_updated_body(bad, "new", GithubBodyOperation::ReplaceIsland, false, &ctx) + .is_err() + ); + } + #[tokio::test] async fn updates_triggering_pull_request_title_and_body() { let server = MockServer::start().await; @@ -872,7 +928,7 @@ mod tests { title: None, body: None, operation: None, - update_branch: Some(true), + update_branch: None, pull_request_number: None, pr_number: None, pr: None, From 9277c06b80defbfde06063d488f4ed9ce1547200 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:07:10 +0000 Subject: [PATCH 4/4] Correct update-pull-request for Azure DevOps Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- docs/safe-outputs.md | 39 +- src/compile/common.rs | 8 +- src/compile/types.rs | 16 - src/execute.rs | 2 +- src/inspect/catalog.rs | 2 +- src/mcp.rs | 2 +- src/safe_outputs/mod.rs | 1 - src/safe_outputs/update_pull_request.rs | 757 +++++++++++------------- 8 files changed, 376 insertions(+), 451 deletions(-) diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index e6f40f163..04b6babd9 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -831,23 +831,22 @@ write. #### Pull request updates (`update-pull-request`) `update-pull-request` matches gh-aw's front matter configuration for updating -GitHub pull requests. It updates the PR title or body; both fields are enabled -by default. The `operation` field controls body updates: `replace` (default), -`append`, `prepend`, or `replace-island`. +Azure DevOps pull requests. It updates the PR title or description (`body`); +both fields are enabled by default. The `operation` field controls description +updates: `replace` (default), `append`, `prepend`, or `replace-island`. ```yaml safe-outputs: update-pull-request: title: true # enable title updates (default: true) - body: true # enable body updates (default: true) - update-branch: false # update the PR branch from its base first (default: false) - sync-stack: true # accepted for gh-aw configuration parity - footer: false # omit the ado-aw trace footer from body updates (default: true) + body: true # enable description updates (default: true) + update-branch: false # must be false; ADO has no equivalent branch-update API + sync-stack: true # accepted for gh-aw front matter parity + footer: false # omit agent stats from body updates (default: true) operation: replace # replace, append, prepend, or replace-island max: 1 # maximum updates per run (default: 1) - target: "*" # "triggering" (default), "*", or a PR number - target-repo: octo-org/octo-repo - allowed-repos: [octo-org/octo-repo] + target: "*" # "triggering" (default), "*", or an ADO PR ID + allowed-repositories: [self] required-labels: [automated] required-title-prefix: "[bot] " ``` @@ -855,20 +854,16 @@ safe-outputs: **Agent parameters:** - `title` *(optional)* - Replacement PR title. -- `body` *(optional)* - PR body content in Markdown. +- `body` *(optional)* - PR description content in Markdown. - `operation` *(optional)* - Overrides the configured body operation for this update. -- `update_branch` *(optional)* - When `true`, update the PR branch with the - latest base branch changes before title/body updates. -- `pull_request_number`, `pr_number`, or `pr` - Required when `target: "*"` is - configured. With `target: "triggering"`, any supplied number must match the - triggering PR. -- `repository` *(optional)* - Target repository, constrained by `target-repo` - and `allowed-repos`. - -`update-branch` calls GitHub's pull request branch update API. GitHub's benign -"already up to date", merge-conflict, and missing-head responses are treated as -non-fatal, matching gh-aw's best-effort behavior. +- `update_branch` *(optional)* - Must be omitted or `false`; Azure DevOps does + not expose the gh-aw branch-update behavior. +- `pull_request_id`, `pull_request_number`, `pr_number`, or `pr` - Required + when `target: "*"` is configured. With `target: "triggering"`, any supplied ID + must match the triggering PR. +- `repository` *(optional)* - Target repository alias, constrained by + `allowed-repositories`. #### Fields, milestones, and assignees diff --git a/src/compile/common.rs b/src/compile/common.rs index 53dda72e0..79f53e710 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2427,11 +2427,6 @@ pub fn validate_github_issue_outputs_config(front_matter: &FrontMatter) -> Resul crate::safe_outputs::validate_update_github_issue_config(&config)?; } } - "update-pull-request" => { - if let Some(config) = front_matter.update_pull_request_config()? { - crate::safe_outputs::validate_update_pull_request_config(&config)?; - } - } "set-github-issue-field" => { if let Some(config) = front_matter.set_github_issue_field_config()? { crate::safe_outputs::validate_set_github_issue_field_config(&config)?; @@ -2460,6 +2455,9 @@ pub fn validate_github_issue_outputs_config(front_matter: &FrontMatter) -> Resul _ => {} } } + if let Some(config) = front_matter.update_pull_request_config()? { + crate::safe_outputs::validate_update_pull_request_config(&config)?; + } if let Some(config) = front_matter.create_github_issue_config()? { if let Some(prefix) = config.title_prefix.as_deref() { crate::validate::reject_pipeline_injection( diff --git a/src/compile/types.rs b/src/compile/types.rs index 7057c6664..a23061771 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -709,7 +709,6 @@ pub const SAFE_OUTPUTS_GITHUB_ACTOR_LOGIN_ENV: &str = "ADO_AW_GITHUB_ACTOR_LOGIN /// intentionally does not depend on the safe-output module registry. pub const GITHUB_ISSUE_SAFE_OUTPUT_TOOLS: &[&str] = &[ "create-github-issue", - "update-pull-request", "set-github-issue-type", "comment-on-github-issue", "hide-github-issue-comment", @@ -2351,18 +2350,6 @@ impl FrontMatter { ..Default::default() })) } - "update-pull-request" => { - Ok(self - .update_pull_request_config()? - .map(|config| GithubIssueCompilerConfig { - target_repo: config.target_repo, - allowed_repos: config.allowed_repos, - required_labels: config.required_labels, - required_title_prefix: config.required_title_prefix, - pull_requests: Some(true), - ..Default::default() - })) - } "set-github-issue-type" => { Ok(self .set_github_issue_type_config()? @@ -2513,9 +2500,6 @@ impl FrontMatter { continue; }; match tool.as_str() { - "update-pull-request" => { - pull_requests = true; - } "comment-on-github-issue" | "add-github-issue-labels" | "update-github-issue" => { let tool_issues = config.issues.unwrap_or(true); let tool_pull_requests = config.pull_requests.unwrap_or(false); diff --git a/src/execute.rs b/src/execute.rs index b3fb6afd5..9d2fa97c1 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -740,6 +740,7 @@ async fn dispatch_pr_tools( "create-pull-request" => CreatePrResult, "add-pr-comment" => AddPrCommentResult, "update-pr" => UpdatePrResult, + "update-pull-request" => UpdatePullRequestResult, "submit-pr-review" => SubmitPrReviewResult, "reply-to-pr-comment" => ReplyToPrCommentResult, "resolve-pr-thread" => ResolvePrThreadResult, @@ -778,7 +779,6 @@ async fn dispatch_github_tools( "add-github-issue-labels" => AddGithubIssueLabelsResult, "remove-github-issue-labels" => RemoveGithubIssueLabelsResult, "close-github-issue" => CloseGithubIssueResult, - "update-pull-request" => UpdatePullRequestResult, "update-github-issue" => UpdateGithubIssueResult, "set-github-issue-field" => SetGithubIssueFieldResult, "assign-github-issue-milestone" => AssignGithubIssueMilestoneResult, diff --git a/src/inspect/catalog.rs b/src/inspect/catalog.rs index d336c52a0..2864fbc1b 100644 --- a/src/inspect/catalog.rs +++ b/src/inspect/catalog.rs @@ -310,7 +310,7 @@ fn safe_output_description(name: &str) -> &'static str { "set-github-issue-field" => "Sets a repository-defined field on a GitHub issue", "submit-pr-review" => "Parameters for submitting a pull request review", "update-pr" => "Parameters for updating a pull request", - "update-pull-request" => "Updates a GitHub pull request title, body, or branch", + "update-pull-request" => "Updates an Azure DevOps pull request title or description", "unassign-github-issue-from-user" => { "Removes operator-permitted GitHub users from an issue" } diff --git a/src/mcp.rs b/src/mcp.rs index c1b80f9bb..c0829c459 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -960,7 +960,7 @@ issue_number may be a positive number or a temporary_id from create-github-issue #[tool( name = "update-pull-request", - description = "Update a configured GitHub pull request title or body, or update its branch from the base branch." + description = "Update an Azure DevOps pull request title or description. Uses gh-aw-style title/body/operation inputs." )] async fn update_pull_request( &self, diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index b381c52e4..0bfb47ccc 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -48,7 +48,6 @@ pub const DEBUG_ONLY_TOOLS: &[&str] = &[]; pub const CONFIGURED_ONLY_TOOLS: &[&str] = tool_names![ AssignWorkItemResult, CreateGithubIssueResult, - UpdatePullRequestResult, SetGithubIssueTypeResult, CommentOnGithubIssueResult, HideGithubIssueCommentResult, diff --git a/src/safe_outputs/update_pull_request.rs b/src/safe_outputs/update_pull_request.rs index 4df698124..e2ca5d195 100644 --- a/src/safe_outputs/update_pull_request.rs +++ b/src/safe_outputs/update_pull_request.rs @@ -1,96 +1,104 @@ -//! `update-pull-request` GitHub safe output. +//! `update-pull-request` Azure DevOps safe output. -use anyhow::ensure; -use log::{debug, info, warn}; -use reqwest::Method; +use anyhow::{Context, ensure}; +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use serde_json::{Map, Value}; -use url::Url; use ado_aw_derive::SanitizeConfig; -use crate::safe_outputs::{ - ExecutionContext, ExecutionResult, Executor, GithubBodyOperation, GithubClient, - GithubMutationFilters, GithubRepositoryPolicy, GithubTargetCapabilities, GithubTargetKind, - GithubTargetMetadata, Validate, build_github_trace_footer, resolve_github_repository, - validate_github_mutation_filter_config, validate_github_mutation_filters, - validate_github_repository, validate_github_target_capability, -}; +use super::{PATH_SEGMENT, resolve_repo_name}; +use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, Validate}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; +use crate::validate::reject_pipeline_injection; const MAX_TITLE_CHARS: usize = 256; const MAX_BODY_CHARS: usize = 65_536; +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "kebab-case")] +pub enum AdoPullRequestBodyOperation { + Replace, + Append, + Prepend, + ReplaceIsland, +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] #[serde(untagged)] -pub enum GithubPullRequestNumber { - Number(u64), +pub enum AdoPullRequestId { + Number(i32), String(String), } -impl GithubPullRequestNumber { - fn parse(&self, field: &str) -> anyhow::Result { - let number = match self { - Self::Number(number) => *number, +impl AdoPullRequestId { + fn parse(&self, field: &str) -> anyhow::Result { + let id = match self { + Self::Number(id) => *id, Self::String(value) => value .trim() .strip_prefix('#') .unwrap_or_else(|| value.trim()) - .parse::() - .map_err(|_| anyhow::anyhow!("{field} must be a positive pull request number"))?, + .parse::() + .map_err(|_| anyhow::anyhow!("{field} must be a positive pull request ID"))?, }; - ensure!(number > 0, "{field} must be positive"); - Ok(number) + ensure!(id > 0, "{field} must be positive"); + Ok(id) } } #[derive(Deserialize, JsonSchema)] pub struct UpdatePullRequestParams { - /// New pull request title. + /// Replacement Azure DevOps pull request title. #[serde(default)] pub title: Option, - /// Pull request body content in Markdown. + /// Pull request description content in Markdown. #[serde(default)] pub body: Option, - /// Body update operation. Defaults to the configured operation, then `replace`. + /// Description update operation. Defaults to the configured operation, then `replace`. #[serde(default)] - pub operation: Option, - /// When true, update the PR branch from the base branch before other updates. + pub operation: Option, + /// Not supported for Azure DevOps PRs; accepted for gh-aw schema compatibility but must be false/omitted. #[serde(default, rename = "update_branch", alias = "updateBranch")] pub update_branch: Option, - /// Pull request number. Required when front matter uses `target: "*"`. + /// Azure DevOps pull request ID. Required when front matter uses `target: "*"`. + #[serde(default, rename = "pull_request_id", alias = "pullRequestId")] + pub pull_request_id: Option, + /// gh-aw-compatible alias for pull_request_id. #[serde(default, rename = "pull_request_number", alias = "pullRequestNumber")] - pub pull_request_number: Option, - /// Alias for pull_request_number. + pub pull_request_number: Option, + /// Alias for pull_request_id. #[serde(default, rename = "pr_number", alias = "prNumber")] - pub pr_number: Option, - /// Alias for pull_request_number. + pub pr_number: Option, + /// Alias for pull_request_id. #[serde(default)] - pub pr: Option, - /// Optional target repository. + pub pr: Option, + /// Repository alias: "self" for the pipeline repo, or an alias from the checkout list. #[serde(default)] pub repository: Option, } impl UpdatePullRequestParams { - fn requested_number(&self) -> anyhow::Result> { + fn requested_id(&self) -> anyhow::Result> { let mut found = None; for (field, value) in [ + ("pull_request_id", self.pull_request_id.as_ref()), ("pull_request_number", self.pull_request_number.as_ref()), ("pr_number", self.pr_number.as_ref()), ("pr", self.pr.as_ref()), ] { if let Some(value) = value { - let number = value.parse(field)?; + let id = value.parse(field)?; if let Some(existing) = found { ensure!( - existing == number, - "pull request number aliases must all refer to the same PR" + existing == id, + "pull request ID aliases must all refer to the same PR" ); } - found = Some(number); + found = Some(id); } } Ok(found) @@ -118,9 +126,9 @@ impl Validate for UpdatePullRequestParams { ); } if let Some(repository) = self.repository.as_deref() { - validate_github_repository(repository)?; + reject_pipeline_injection(repository, "repository")?; } - let _ = self.requested_number()?; + let _ = self.requested_id()?; Ok(()) } } @@ -130,22 +138,24 @@ tool_result! { write = true, params = UpdatePullRequestParams, default_max = 1, - /// Result of updating a GitHub pull request. + /// Result of updating an Azure DevOps pull request. pub struct UpdatePullRequestResult { #[serde(default)] title: Option, #[serde(default)] body: Option, #[serde(default)] - operation: Option, + operation: Option, #[serde(default, rename = "update_branch")] update_branch: Option, + #[serde(default, rename = "pull_request_id")] + pull_request_id: Option, #[serde(default, rename = "pull_request_number")] - pull_request_number: Option, + pull_request_number: Option, #[serde(default, rename = "pr_number")] - pr_number: Option, + pr_number: Option, #[serde(default)] - pr: Option, + pr: Option, #[serde(default)] repository: Option, } @@ -163,14 +173,14 @@ fn default_true() -> bool { true } -fn default_operation() -> GithubBodyOperation { - GithubBodyOperation::Replace +fn default_operation() -> AdoPullRequestBodyOperation { + AdoPullRequestBodyOperation::Replace } #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(untagged)] pub enum UpdatePullRequestTarget { - Number(u64), + Id(i32), Named(String), } @@ -183,37 +193,36 @@ impl Default for UpdatePullRequestTarget { #[derive(Debug, Clone, SanitizeConfig, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct UpdatePullRequestConfig { - /// Whether title updates are enabled. Defaults to true. + /// Whether title updates are enabled. Defaults to true to match gh-aw. #[serde(default = "default_true")] #[sanitize_config(skip)] pub title: bool, - /// Whether body updates are enabled. Defaults to true. + /// Whether description updates are enabled. Defaults to true to match gh-aw. #[serde(default = "default_true")] #[sanitize_config(skip)] pub body: bool, - /// Default branch update policy. Defaults to false. + /// Not supported for Azure DevOps PRs; must remain false. #[serde(default, rename = "update-branch")] #[sanitize_config(skip)] pub update_branch: bool, - /// gh-aw-compatible stacked-PR fallback knob. Parsed for config parity. + /// Accepted for gh-aw front matter parity; unused for Azure DevOps. #[serde(default = "default_true", rename = "sync-stack")] #[sanitize_config(skip)] pub sync_stack: bool, - /// Include the standard ado-aw trace footer in body updates. + /// Include agent stats in body updates. #[serde(default = "default_true")] #[sanitize_config(skip)] pub footer: bool, /// Body update operation. Defaults to replace. #[serde(default = "default_operation")] #[sanitize_config(skip)] - pub operation: GithubBodyOperation, - /// `"triggering"` (default), `"*"`, or a fixed PR number. + pub operation: AdoPullRequestBodyOperation, + /// `"triggering"` (default), `"*"`, or a fixed PR ID. #[serde(default)] pub target: UpdatePullRequestTarget, - #[serde(default, rename = "target-repo")] - pub target_repo: Option, - #[serde(default, rename = "allowed-repos")] - pub allowed_repos: Vec, + /// Repository aliases the agent may target. Empty means any checkout alias accepted by the compiler. + #[serde(default, rename = "allowed-repositories")] + pub allowed_repositories: Vec, #[serde(default, rename = "required-labels")] pub required_labels: Vec, #[serde(default, rename = "required-title-prefix")] @@ -231,10 +240,9 @@ impl Default for UpdatePullRequestConfig { update_branch: false, sync_stack: true, footer: true, - operation: GithubBodyOperation::Replace, + operation: AdoPullRequestBodyOperation::Replace, target: UpdatePullRequestTarget::default(), - target_repo: None, - allowed_repos: Vec::new(), + allowed_repositories: Vec::new(), required_labels: Vec::new(), required_title_prefix: None, max: None, @@ -245,34 +253,58 @@ impl Default for UpdatePullRequestConfig { pub(crate) fn validate_update_pull_request_config( config: &UpdatePullRequestConfig, ) -> anyhow::Result<()> { + ensure!( + !config.update_branch, + "safe-outputs.update-pull-request.update-branch is not supported for Azure DevOps PRs" + ); match &config.target { - UpdatePullRequestTarget::Number(number) => { - ensure!(*number > 0, "target PR number must be positive"); - } - UpdatePullRequestTarget::Named(target) => { - ensure!( - matches!(target.as_str(), "triggering" | "*"), - "target must be \"triggering\", \"*\", or a positive pull request number" - ); - } + UpdatePullRequestTarget::Id(id) => ensure!(*id > 0, "target PR ID must be positive"), + UpdatePullRequestTarget::Named(target) => ensure!( + matches!(target.as_str(), "triggering" | "*"), + "target must be \"triggering\", \"*\", or a positive pull request ID" + ), + } + for repository in &config.allowed_repositories { + ensure!( + !repository.trim().is_empty(), + "allowed-repositories entries must not be empty" + ); + reject_pipeline_injection( + repository, + "safe-outputs.update-pull-request.allowed-repositories", + )?; + } + for label in &config.required_labels { + ensure!( + !label.is_empty(), + "required-labels entries must not be empty" + ); + reject_pipeline_injection(label, "safe-outputs.update-pull-request.required-labels")?; + } + if let Some(prefix) = config.required_title_prefix.as_deref() { + ensure!( + !prefix.is_empty(), + "required-title-prefix must not be empty" + ); + reject_pipeline_injection( + prefix, + "safe-outputs.update-pull-request.required-title-prefix", + )?; } - validate_github_mutation_filter_config(GithubMutationFilters { - required_labels: &config.required_labels, - required_title_prefix: config.required_title_prefix.as_deref(), - })?; Ok(()) } #[derive(Debug, Deserialize)] -struct RawPullRequestTarget { - number: u64, +struct RawPullRequest { + #[serde(rename = "pullRequestId")] + pull_request_id: i32, title: String, #[serde(default)] - body: Option, - state: String, + description: Option, #[serde(default)] labels: Vec, - html_url: Option, + #[serde(default)] + url: Option, } #[derive(Debug, Deserialize)] @@ -280,49 +312,6 @@ struct RawLabel { name: String, } -impl RawPullRequestTarget { - fn metadata(&self) -> GithubTargetMetadata { - GithubTargetMetadata { - number: self.number, - node_id: None, - title: self.title.clone(), - state: self.state.clone(), - labels: self.labels.iter().map(|label| label.name.clone()).collect(), - kind: GithubTargetKind::PullRequest, - html_url: self.html_url.clone(), - } - } -} - -fn repository_route(client: &GithubClient, repository: &str, tail: &[&str]) -> anyhow::Result { - validate_github_repository(repository)?; - let (owner, name) = repository - .split_once('/') - .expect("validated GitHub repository contains slash"); - let mut url = client.rest_api_url().clone(); - { - let mut path = url - .path_segments_mut() - .map_err(|_| anyhow::anyhow!("GitHub API URL cannot be a base URL"))?; - path.pop_if_empty(); - path.push("repos"); - path.push(owner); - path.push(name); - for segment in tail { - path.push(segment); - } - } - Ok(url) -} - -fn body_with_footer(body: &str, include_footer: bool, ctx: &ExecutionContext) -> String { - if include_footer { - format!("{body}\n\n{}", build_github_trace_footer(ctx)) - } else { - body.to_string() - } -} - fn island_markers(ctx: &ExecutionContext) -> Result<(String, String), ExecutionResult> { let Some(definition_id) = ctx.definition_id else { return Err(ExecutionResult::failure( @@ -375,102 +364,109 @@ fn replace_island( )) } +fn body_with_footer(body: &str, include_footer: bool, ctx: &ExecutionContext) -> String { + if include_footer { + crate::agent_stats::append_stats_to_body(body, ctx, true) + } else { + body.to_string() + } +} + fn build_updated_body( current: &str, new_content: &str, - operation: GithubBodyOperation, + operation: AdoPullRequestBodyOperation, include_footer: bool, ctx: &ExecutionContext, ) -> Result { let section = body_with_footer(new_content, include_footer, ctx); let updated = match operation { - GithubBodyOperation::Append => { + AdoPullRequestBodyOperation::Append => { if current.is_empty() { section } else { format!("{current}\n\n---\n\n{section}") } } - GithubBodyOperation::Prepend => { + AdoPullRequestBodyOperation::Prepend => { if current.is_empty() { section } else { format!("{section}\n\n---\n\n{current}") } } - GithubBodyOperation::Replace => section, - GithubBodyOperation::ReplaceIsland => replace_island(current, §ion, ctx)?, + AdoPullRequestBodyOperation::Replace => section, + AdoPullRequestBodyOperation::ReplaceIsland => replace_island(current, §ion, ctx)?, }; if updated.chars().count() > MAX_BODY_CHARS { return Err(ExecutionResult::failure(format!( - "updated body exceeds GitHub's {MAX_BODY_CHARS}-character limit" + "updated body exceeds Azure DevOps' {MAX_BODY_CHARS}-character limit" ))); } Ok(updated) } +fn ctx_pull_request_id(ctx: &ExecutionContext) -> Result { + let raw = ctx.pull_request_id.as_deref().ok_or_else(|| { + ExecutionResult::failure( + "SYSTEM_PULLREQUEST_PULLREQUESTID is required for target \"triggering\"", + ) + })?; + raw.parse::().ok().filter(|id| *id > 0).ok_or_else(|| { + ExecutionResult::failure(format!( + "SYSTEM_PULLREQUEST_PULLREQUESTID '{}' is not a positive pull request ID", + crate::sanitize::neutralize_pipeline_commands(raw) + )) + }) +} + impl UpdatePullRequestResult { - fn requested_number(&self) -> anyhow::Result> { - let params = UpdatePullRequestParams { + fn requested_id(&self) -> anyhow::Result> { + UpdatePullRequestParams { title: self.title.clone(), body: self.body.clone(), operation: self.operation, update_branch: self.update_branch, + pull_request_id: self.pull_request_id.clone(), pull_request_number: self.pull_request_number.clone(), pr_number: self.pr_number.clone(), pr: self.pr.clone(), repository: self.repository.clone(), - }; - params.requested_number() - } - - fn requested_fields(&self, config: &UpdatePullRequestConfig) -> Vec<&'static str> { - let mut fields = Vec::new(); - if self.title.is_some() { - fields.push("title"); } - if self.body.is_some() { - fields.push("body"); - } - if self.update_branch.unwrap_or(config.update_branch) { - fields.push("update_branch"); - } - fields + .requested_id() } - fn resolve_number( + fn resolve_id( &self, config: &UpdatePullRequestConfig, ctx: &ExecutionContext, - ) -> Result { + ) -> Result { let requested = self - .requested_number() + .requested_id() .map_err(|error| ExecutionResult::failure(error.to_string()))?; match &config.target { - UpdatePullRequestTarget::Number(number) => { + UpdatePullRequestTarget::Id(id) => { if let Some(requested) = requested - && requested != *number + && requested != *id { return Err(ExecutionResult::failure(format!( - "requested pull_request_number #{requested} does not match configured target #{number}" + "requested pull_request_id #{requested} does not match configured target #{id}" ))); } - Ok(*number) - } - UpdatePullRequestTarget::Named(target) if target == "*" => { - requested.ok_or_else(|| { - ExecutionResult::failure( - "pull_request_number is required when safe-outputs.update-pull-request.target is \"*\"", - ) - }) + Ok(*id) } + UpdatePullRequestTarget::Named(target) if target == "*" => requested.ok_or_else(|| { + ExecutionResult::failure( + "pull_request_id is required when safe-outputs.update-pull-request.target is \"*\"", + ) + }), UpdatePullRequestTarget::Named(target) if target == "triggering" => { - let triggering = ctx_pull_request_id_from(ctx)?; + let triggering = ctx_pull_request_id(ctx)?; if let Some(requested) = requested && requested != triggering { return Err(ExecutionResult::failure(format!( - "requested pull_request_number #{requested} does not match triggering pull request #{triggering}" + "requested pull_request_id #{requested} does not match triggering pull request #{triggering}" ))); } Ok(triggering) @@ -482,85 +478,90 @@ impl UpdatePullRequestResult { } } - async fn fetch_target( - &self, - client: &GithubClient, - repository: &str, - number: u64, - ) -> anyhow::Result> { - let response = client - .send( - Method::GET, - repository_route(client, repository, &["pulls", &number.to_string()])?, - None, - ) - .await?; - let response = match response.require_success("Failed to fetch GitHub pull request") { - Ok(response) => response, - Err(error) => return Ok(Err(ExecutionResult::failure(error.to_string()))), - }; - match response.json("Failed to parse GitHub pull request") { - Ok(target) => Ok(Ok(target)), - Err(error) => Ok(Err(ExecutionResult::failure(error.to_string()))), + fn requested_fields(&self) -> Vec<&'static str> { + let mut fields = Vec::new(); + if self.title.is_some() { + fields.push("title"); } + if self.body.is_some() { + fields.push("body"); + } + fields } - async fn update_branch( + async fn fetch_pr( &self, - client: &GithubClient, - repository: &str, - number: u64, - ) -> anyhow::Result> { + client: &reqwest::Client, + base_url: &str, + repo_name: &str, + token: &str, + pr_id: i32, + ) -> anyhow::Result> { + let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); + let url = format!("{base_url}/{encoded_repo}/pullRequests/{pr_id}?api-version=7.1"); let response = client - .send( - Method::PUT, - repository_route( - client, - repository, - &["pulls", &number.to_string(), "update-branch"], - )?, - None, - ) - .await?; - if response.is_success() { - return Ok(Ok(())); - } - let error = response - .require_success("Failed to update GitHub pull request branch") - .expect_err("non-success response must produce an API error"); - let message = error.message.to_ascii_lowercase(); - if message.contains("there are no new commits on the base branch") - || message.contains("merge conflict between base and head") - || message.contains("head ref does not exist") + .get(&url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to fetch Azure DevOps pull request")?; + if !response.status().is_success() { + let status = response.status(); + let body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(Err(ExecutionResult::failure(format!( + "Failed to fetch PR #{pr_id} (HTTP {status}): {body}" + )))); + } + match response.json::().await { + Ok(pr) => Ok(Ok(pr)), + Err(error) => Ok(Err(ExecutionResult::failure(format!( + "Failed to parse PR #{pr_id}: {error}" + )))), + } + } + + fn validate_filters( + &self, + pr: &RawPullRequest, + config: &UpdatePullRequestConfig, + ) -> Result<(), ExecutionResult> { + let missing: Vec<&str> = config + .required_labels + .iter() + .map(String::as_str) + .filter(|required| { + !pr.labels + .iter() + .any(|label| label.name.eq_ignore_ascii_case(required)) + }) + .collect(); + if !missing.is_empty() { + return Err(ExecutionResult::failure(format!( + "PR #{} is missing required labels: {}", + pr.pull_request_id, + missing.join(", ") + ))); + } + if let Some(prefix) = config.required_title_prefix.as_deref() + && !pr.title.starts_with(prefix) { - warn!("Non-fatal update-pull-request branch update failure: {error}"); - return Ok(Ok(())); + return Err(ExecutionResult::failure(format!( + "PR #{} title does not start with required-title-prefix '{}'", + pr.pull_request_id, + crate::sanitize::neutralize_pipeline_commands(prefix) + ))); } - Ok(Err(ExecutionResult::failure(error.to_string()))) + Ok(()) } } -fn ctx_pull_request_id_from(ctx: &ExecutionContext) -> Result { - let raw = ctx.pull_request_id.as_deref().ok_or_else(|| { - ExecutionResult::failure( - "SYSTEM_PULLREQUEST_PULLREQUESTID is required for target \"triggering\"", - ) - })?; - raw.parse::() - .ok() - .filter(|number| *number > 0) - .ok_or_else(|| { - ExecutionResult::failure(format!( - "SYSTEM_PULLREQUEST_PULLREQUESTID '{}' is not a positive pull request number", - crate::sanitize::neutralize_pipeline_commands(raw) - )) - }) -} - #[async_trait::async_trait] impl Executor for UpdatePullRequestResult { fn dry_run_summary(&self) -> String { - "update GitHub pull request".to_string() + "update Azure DevOps pull request".to_string() } async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { @@ -569,6 +570,7 @@ impl Executor for UpdatePullRequestResult { body: self.body.clone(), operation: self.operation, update_branch: self.update_branch, + pull_request_id: self.pull_request_id.clone(), pull_request_number: self.pull_request_number.clone(), pr_number: self.pr_number.clone(), pr: self.pr.clone(), @@ -582,12 +584,18 @@ impl Executor for UpdatePullRequestResult { "update-pull-request is not configured for this workflow", )); } - let Some(token) = ctx.github_token.as_ref() else { - return Ok(ExecutionResult::failure( - "ADO_AW_GITHUB_TOKEN is not set; configure safe-outputs.github-token \ - or safe-outputs.github-app", - )); - }; + 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)")?; let config: UpdatePullRequestConfig = ctx.get_tool_config("update-pull-request")?; validate_update_pull_request_config(&config)?; if self.title.is_some() && !config.title { @@ -600,97 +608,106 @@ impl Executor for UpdatePullRequestResult { "update-pull-request field 'body' is not enabled by configuration", )); } - let update_branch = self.update_branch.unwrap_or(config.update_branch); - if self.title.is_none() && self.body.is_none() && !update_branch { + if self.update_branch == Some(true) { return Ok(ExecutionResult::failure( - "at least one of title, body, or effective update_branch: true is required", + "update_branch is not supported for Azure DevOps PRs", )); } - let number = match self.resolve_number(&config, ctx) { - Ok(number) => number, + if self.title.is_none() && self.body.is_none() { + return Ok(ExecutionResult::failure( + "at least one of title or body is required", + )); + } + let pr_id = match self.resolve_id(&config, ctx) { + Ok(pr_id) => pr_id, Err(result) => return Ok(result), }; - let repository = match resolve_github_repository( - self.repository.as_deref(), - GithubRepositoryPolicy::new(config.target_repo.as_deref(), &config.allowed_repos), - ctx, - ) { - Ok(repository) => repository, - Err(result) => return Ok(result), + let repo_alias = self.repository.as_deref().unwrap_or("self"); + if !config.allowed_repositories.is_empty() + && !config + .allowed_repositories + .iter() + .any(|allowed| allowed == repo_alias) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + crate::sanitize::neutralize_pipeline_commands(repo_alias), + config.allowed_repositories.join(", ") + ))); + } + let repo_name = match resolve_repo_name(self.repository.as_deref(), ctx) { + Ok(name) => name, + Err(failure) => return Ok(failure), }; - let client = GithubClient::new(&ctx.github_api_url, token)?; - let current = match self.fetch_target(&client, &repository, number).await? { - Ok(target) => target, + let client = reqwest::Client::new(); + let encoded_project = utf8_percent_encode(project, PATH_SEGMENT).to_string(); + let base_url = format!( + "{}/{}/_apis/git/repositories", + org_url.trim_end_matches('/'), + encoded_project, + ); + let current = match self + .fetch_pr(&client, &base_url, &repo_name, token, pr_id) + .await? + { + Ok(pr) => pr, Err(result) => return Ok(result), }; - let metadata = current.metadata(); - if let Err(result) = validate_github_target_capability( - &metadata, - GithubTargetCapabilities { - issues: false, - pull_requests: true, - }, - ) { - return Ok(result); - } - let filters = GithubMutationFilters { - required_labels: &config.required_labels, - required_title_prefix: config.required_title_prefix.as_deref(), - }; - if let Err(result) = validate_github_mutation_filters(&metadata, filters) { - return Ok(result); - } - if update_branch - && let Err(result) = self.update_branch(&client, &repository, number).await? - { + if let Err(result) = self.validate_filters(¤t, &config) { return Ok(result); } - - let mut payload = Map::new(); + let mut patch = Map::new(); if let Some(title) = self.title.as_ref() { - payload.insert("title".to_string(), Value::String(title.clone())); + patch.insert("title".to_string(), Value::String(title.clone())); } if let Some(body) = self.body.as_deref() { - let updated = match build_updated_body( - current.body.as_deref().unwrap_or_default(), + let description = match build_updated_body( + current.description.as_deref().unwrap_or_default(), body, self.operation.unwrap_or(config.operation), config.footer, ctx, ) { - Ok(body) => body, + Ok(description) => description, Err(result) => return Ok(result), }; - payload.insert("body".to_string(), Value::String(updated)); - } - if !payload.is_empty() { - debug!("Updating GitHub pull request {repository}#{number}"); - let response = client - .send( - Method::PATCH, - repository_route(&client, &repository, &["pulls", &number.to_string()])?, - Some(&Value::Object(payload)), - ) - .await?; - if !response.is_success() { - let error = response - .require_success("Failed to update GitHub pull request") - .expect_err("non-success response must produce an API error"); - return Ok(ExecutionResult::failure(error.to_string())); - } + patch.insert("description".to_string(), Value::String(description)); } - - info!("Updated GitHub pull request {repository}#{number}"); + let encoded_repo = utf8_percent_encode(&repo_name, PATH_SEGMENT).to_string(); + let patch_url = format!("{base_url}/{encoded_repo}/pullRequests/{pr_id}?api-version=7.1"); + debug!( + "Updating Azure DevOps PR #{pr_id}: {}", + self.requested_fields().join(", ") + ); + let response = client + .patch(&patch_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&Value::Object(patch)) + .send() + .await + .context("Failed to update Azure DevOps pull request")?; + if !response.status().is_success() { + let status = response.status(); + let body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to update PR #{pr_id} (HTTP {status}): {body}" + ))); + } + info!("Updated Azure DevOps PR #{pr_id}"); Ok(ExecutionResult::success_with_data( format!( - "Updated GitHub pull request {repository}#{number}: {}", - self.requested_fields(&config).join(", ") + "Updated Azure DevOps PR #{pr_id}: {}", + self.requested_fields().join(", ") ), serde_json::json!({ - "number": number, - "target_repo": repository, - "pull_request_url": metadata.html_url, - "fields": self.requested_fields(&config), + "pull_request_id": pr_id, + "operation": "update-pull-request", + "fields": self.requested_fields(), + "url": current.url, }), )) } @@ -708,10 +725,10 @@ mod tests { let mut tool_configs = HashMap::new(); tool_configs.insert("update-pull-request".to_string(), config); ExecutionContext { - github_token: Some("token".to_string()), - github_api_url: server.uri(), - repository_provider: Some("GitHub".to_string()), - repository_name: Some("octo/repo".to_string()), + ado_org_url: Some(server.uri()), + ado_project: Some("project".to_string()), + access_token: Some("token".to_string()), + repository_name: Some("repo".to_string()), pull_request_id: Some("7".to_string()), tool_configs, definition_id: Some(123), @@ -719,14 +736,13 @@ mod tests { } } - fn pull_request(number: u64) -> serde_json::Value { + fn pr(id: i32) -> serde_json::Value { serde_json::json!({ - "number": number, + "pullRequestId": id, "title": "[bot] Existing", - "body": "Existing body", - "state": "open", + "description": "Existing body", "labels": [{"name": "automated"}], - "html_url": format!("https://github.example/octo/repo/pull/{number}") + "url": format!("https://dev.azure.example/pr/{id}") }) } @@ -736,6 +752,7 @@ mod tests { body: None, operation: None, update_branch: None, + pull_request_id: None, pull_request_number: None, pr_number: None, pr: None, @@ -750,7 +767,7 @@ mod tests { } #[test] - fn config_matches_gh_aw_defaults_and_rejects_unknown_fields() { + fn config_matches_gh_aw_shape_but_rejects_update_branch_true() { let config: UpdatePullRequestConfig = serde_json::from_value(serde_json::json!({})).unwrap(); assert!(config.title); @@ -758,24 +775,26 @@ mod tests { assert!(!config.update_branch); assert!(config.sync_stack); assert!(config.footer); - assert_eq!(config.operation, GithubBodyOperation::Replace); - assert!( - serde_json::from_value::(serde_json::json!({ - "allow-title": true - })) - .is_err() - ); + assert_eq!(config.operation, AdoPullRequestBodyOperation::Replace); + + let unsupported: UpdatePullRequestConfig = serde_json::from_value(serde_json::json!({ + "update-branch": true + })) + .unwrap(); + assert!(validate_update_pull_request_config(&unsupported).is_err()); } #[test] - fn validates_number_aliases() { - let mut empty = params(); - empty.title = None; + fn validates_id_aliases() { + let empty = UpdatePullRequestParams { + title: None, + ..params() + }; assert!(empty.validate().is_ok()); let mut aliases = params(); - aliases.pull_request_number = Some(GithubPullRequestNumber::Number(1)); - aliases.pr_number = Some(GithubPullRequestNumber::String("#2".to_string())); + aliases.pull_request_id = Some(AdoPullRequestId::Number(1)); + aliases.pull_request_number = Some(AdoPullRequestId::String("#2".to_string())); assert!(aliases.validate().is_err()); } @@ -788,19 +807,17 @@ mod tests { let first = build_updated_body( "before", "new", - GithubBodyOperation::ReplaceIsland, + AdoPullRequestBodyOperation::ReplaceIsland, false, &ctx, ) .unwrap(); assert!(first.contains("before\n\n---\n\n")); assert!(first.contains("")); - assert!(first.contains("\nnew\n")); - let second = build_updated_body( &first, "next", - GithubBodyOperation::ReplaceIsland, + AdoPullRequestBodyOperation::ReplaceIsland, false, &ctx, ) @@ -809,54 +826,28 @@ mod tests { assert!(!second.contains("\nnew\n")); } - #[test] - fn replace_island_requires_definition_id_and_ordered_markers() { - let missing_id = ExecutionContext::default(); - assert!( - build_updated_body( - "", - "new", - GithubBodyOperation::ReplaceIsland, - false, - &missing_id - ) - .is_err() - ); - - let ctx = ExecutionContext { - definition_id: Some(123), - ..Default::default() - }; - let bad = "\nold\n"; - assert!( - build_updated_body(bad, "new", GithubBodyOperation::ReplaceIsland, false, &ctx) - .is_err() - ); - } - #[tokio::test] - async fn updates_triggering_pull_request_title_and_body() { + async fn updates_triggering_pr_title_and_body() { let server = MockServer::start().await; Mock::given(method("GET")) - .and(path("/repos/octo/repo/pulls/7")) - .respond_with(ResponseTemplate::new(200).set_body_json(pull_request(7))) + .and(path("/project/_apis/git/repositories/repo/pullRequests/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(pr(7))) .expect(1) .mount(&server) .await; Mock::given(method("PATCH")) - .and(path("/repos/octo/repo/pulls/7")) + .and(path("/project/_apis/git/repositories/repo/pullRequests/7")) .and(body_json(serde_json::json!({ "title": "Updated title", - "body": "Existing body\n\n---\n\nNew body" + "description": "Existing body\n\n---\n\nNew body" }))) - .respond_with(ResponseTemplate::new(200).set_body_json(pull_request(7))) + .respond_with(ResponseTemplate::new(200).set_body_json(pr(7))) .expect(1) .mount(&server) .await; let ctx = context( &server, serde_json::json!({ - "target-repo": "octo/repo", "title": true, "body": true, "footer": false, @@ -870,6 +861,7 @@ mod tests { body: Some("New body".to_string()), operation: None, update_branch: None, + pull_request_id: None, pull_request_number: None, pr_number: None, pr: None, @@ -882,61 +874,18 @@ mod tests { } #[tokio::test] - async fn target_star_requires_agent_pull_request_number() { + async fn target_star_requires_agent_pr_id() { let server = MockServer::start().await; let ctx = context( &server, serde_json::json!({ - "target-repo": "octo/repo", "target": "*" }), ); let mut result: UpdatePullRequestResult = params().try_into().unwrap(); let execution = result.execute_sanitized(&ctx).await.unwrap(); assert!(!execution.success); - assert!( - execution - .message - .contains("pull_request_number is required") - ); + assert!(execution.message.contains("pull_request_id is required")); assert!(server.received_requests().await.unwrap().is_empty()); } - - #[tokio::test] - async fn update_branch_only_calls_update_branch_endpoint() { - let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/repos/octo/repo/pulls/7")) - .respond_with(ResponseTemplate::new(200).set_body_json(pull_request(7))) - .expect(1) - .mount(&server) - .await; - Mock::given(method("PUT")) - .and(path("/repos/octo/repo/pulls/7/update-branch")) - .respond_with(ResponseTemplate::new(202).set_body_json(serde_json::json!({}))) - .expect(1) - .mount(&server) - .await; - let ctx = context( - &server, - serde_json::json!({ - "target-repo": "octo/repo", - "update-branch": true - }), - ); - let mut result: UpdatePullRequestResult = UpdatePullRequestParams { - title: None, - body: None, - operation: None, - update_branch: None, - pull_request_number: None, - pr_number: None, - pr: None, - repository: None, - } - .try_into() - .unwrap(); - let execution = result.execute_sanitized(&ctx).await.unwrap(); - assert!(execution.success, "{}", execution.message); - } }