From 43dd9a234eca0089fa894310e41d7ac1a61b8070 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 29 Apr 2026 21:54:54 +0000 Subject: [PATCH 1/2] refactor: reduce complexity of execute_impl in create_pr.rs Extract five focused helper functions from the 992-line execute_impl: - check_for_conflict_markers: deduplicates the git-grep conflict scan that was copy-pasted in both the git-apply and git-am fallback paths - apply_patch_with_exclusions: the git-apply --3way path (excluded-files) - apply_patch_without_exclusions: the git-am --3way + git-apply fallback path - apply_patch_to_worktree: thin dispatcher selecting the right strategy - set_pr_completion_options: sets delete-source-branch / squash-merge / auto-complete on the newly created PR - add_reviewers_to_pr: resolves and attaches each configured reviewer execute_impl shrinks from 992 lines to 789 lines (-203 lines). The extracted functions are independently documented and testable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/safeoutputs/create_pr.rs | 486 +++++++++++++++++++---------------- 1 file changed, 271 insertions(+), 215 deletions(-) diff --git a/src/safeoutputs/create_pr.rs b/src/safeoutputs/create_pr.rs index ea246f89..0e159f6b 100644 --- a/src/safeoutputs/create_pr.rs +++ b/src/safeoutputs/create_pr.rs @@ -876,119 +876,10 @@ impl Executor for CreatePrResult { // with git apply --3way as fallback // - With exclusions: use git apply --3way directly (git am does not support // --exclude flags; git apply does) - let use_git_apply = !exclude_args.is_empty(); - let patch_committed; - - if use_git_apply { - debug!("Using git apply --3way (excluded-files configured)"); - let mut apply_args: Vec = vec!["apply".into(), "--3way".into()]; - apply_args.extend(exclude_args.iter().cloned()); - apply_args.push(patch_path.to_string_lossy().into_owned()); - let apply_output = Command::new("git") - .args(&apply_args) - .current_dir(&worktree_path) - .output() - .await - .context("Failed to run git apply --3way")?; - - if !apply_output.status.success() { - let err_msg = format!( - "Patch could not be applied (conflicts): {}", - String::from_utf8_lossy(&apply_output.stderr) - ); - warn!("{}", err_msg); - return Ok(ExecutionResult::failure(err_msg)); - } - debug!("Patch applied with git apply --3way"); - - // Scan for unresolved conflict markers in working tree files. - // Use specific patterns: <<<<<<< and >>>>>>> always have a trailing space - // and marker (e.g., "<<<<<<< HEAD"), while ======= is on its own line. - // We require the trailing space on <<<<<<< and >>>>>>> to avoid false - // positives from reStructuredText/markdown heading underlines. - let conflict_check = Command::new("git") - .args(["grep", "-l", "-E", r"^(<<<<<<<\s|>>>>>>>\s)"]) - .current_dir(&worktree_path) - .output() - .await - .context("Failed to run git grep for conflict markers")?; - - if conflict_check.status.success() { - let conflicting_files = - String::from_utf8_lossy(&conflict_check.stdout).trim().to_string(); - let err_msg = format!( - "Patch applied with unresolved conflict markers in: {}", - conflicting_files - ); - warn!("{}", err_msg); - return Ok(ExecutionResult::failure(err_msg)); - } - - patch_committed = false; - } else { - // No exclusions — use git am --3way for proper commit metadata preservation - debug!("Applying patch with git am --3way"); - let am_output = Command::new("git") - .args(["am", "--3way", &patch_path.to_string_lossy()]) - .current_dir(&worktree_path) - .output() - .await - .context("Failed to run git am")?; - - patch_committed = am_output.status.success(); - - if !patch_committed { - let stderr = String::from_utf8_lossy(&am_output.stderr); - debug!("git am --3way failed: {}", stderr); - - // Abort the failed am to leave worktree clean - let _ = Command::new("git") - .args(["am", "--abort"]) - .current_dir(&worktree_path) - .output() - .await; - - // Fallback: try git apply --3way - debug!("Falling back to git apply --3way"); - let apply_output = Command::new("git") - .args(["apply", "--3way", &patch_path.to_string_lossy()]) - .current_dir(&worktree_path) - .output() - .await - .context("Failed to run git apply --3way")?; - - if !apply_output.status.success() { - let err_msg = format!( - "Patch could not be applied (conflicts): {}", - String::from_utf8_lossy(&apply_output.stderr) - ); - warn!("{}", err_msg); - return Ok(ExecutionResult::failure(err_msg)); - } - debug!("Patch applied with git apply --3way fallback"); - - // Scan for unresolved conflict markers - let conflict_check = Command::new("git") - .args(["grep", "-l", "-E", r"^(<<<<<<<\s|>>>>>>>\s)"]) - .current_dir(&worktree_path) - .output() - .await - .context("Failed to run git grep for conflict markers")?; - - if conflict_check.status.success() { - let conflicting_files = - String::from_utf8_lossy(&conflict_check.stdout).trim().to_string(); - let err_msg = format!( - "Patch applied with unresolved conflict markers in: {}", - conflicting_files - ); - warn!("{}", err_msg); - return Ok(ExecutionResult::failure(err_msg)); - } - } else { - debug!("Patch applied successfully with git am"); - } - } + let patch_committed = match apply_patch_to_worktree(&worktree_path, &patch_path, &exclude_args).await? { + Ok(committed) => committed, + Err(result) => return Ok(result), + }; // Collect changed files. The method depends on how the patch was applied: // - git am: changes are committed → use git diff-tree to compare base_sha..HEAD @@ -1408,108 +1299,15 @@ impl Executor for CreatePrResult { let pr_web_url = pr_data["url"].as_str().unwrap_or(""); info!("Pull request created: #{} - {}", pr_id, pr_web_url); - // Set completion options (delete source branch, squash merge) and optionally auto-complete - // completionOptions apply when the PR is completed by anyone, auto_complete makes it complete automatically - { - debug!( - "Setting PR completion options: delete_source_branch={}, squash_merge={}, auto_complete={}", - config.delete_source_branch, config.squash_merge, config.auto_complete - ); - let pr_update_url = format!( - "{}{}/_apis/git/repositories/{}/pullrequests/{}?api-version=7.1", - org_url, project, repo_id, pr_id - ); - - let mut update_body = serde_json::json!({ - "completionOptions": { - "deleteSourceBranch": config.delete_source_branch, - "squashMerge": config.squash_merge - } - }); - - // Only set autoCompleteSetBy if auto_complete is enabled and PR is not a draft - // (ADO silently ignores auto-complete on draft PRs, so skip the API call) - if config.auto_complete && !config.draft { - update_body["autoCompleteSetBy"] = serde_json::json!({ - "id": pr_data["createdBy"]["id"] - }); - } - - match client - .patch(&pr_update_url) - .basic_auth("", Some(token)) - .json(&update_body) - .send() - .await - { - Ok(resp) if resp.status().is_success() => { - debug!("PR completion options set successfully"); - } - Ok(resp) => { - warn!("Failed to set PR completion options: {}", resp.status()); - } - Err(e) => { - warn!("Failed to set PR completion options: {}", e); - } - } - } - - // Add reviewers if configured - if !config.reviewers.is_empty() { - debug!("Adding {} reviewers", config.reviewers.len()); - for reviewer in &config.reviewers { - debug!("Adding reviewer: {}", reviewer); - - // Resolve reviewer identity (email/name -> ID) - let reviewer_id = - match resolve_reviewer_identity(&client, organization, 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", - org_url, project, repo_id, pr_id, reviewer_id - ); - - let reviewer_body = serde_json::json!({ - "vote": 0, - "isRequired": false - }); - - match client - .put(&reviewer_url) - .basic_auth("", Some(token)) - .json(&reviewer_body) - .send() - .await - { - Ok(resp) if resp.status().is_success() => { - debug!( - "Reviewer '{}' (ID: {}) added successfully", - reviewer, reviewer_id - ); - } - Ok(resp) => { - warn!( - "Failed to add reviewer '{}' (ID: {}): {}", - reviewer, - reviewer_id, - resp.status() - ); - } - Err(e) => { - warn!("Failed to add reviewer '{}': {}", reviewer, e); - } - } - } - } + // Set completion options (delete source branch, squash merge, auto-complete) + // and add reviewers. + set_pr_completion_options( + &client, &config, org_url, project, &repo_id, pr_id, + pr_data["createdBy"]["id"].as_str(), token, + ).await; + add_reviewers_to_pr( + &client, &config, org_url, project, &repo_id, pr_id, organization, token, + ).await; info!( "PR #{} created successfully: {} -> {}{}", @@ -1530,6 +1328,264 @@ impl Executor for CreatePrResult { } } +/// Check for unresolved conflict markers in the working tree. +/// +/// Uses `git grep` to look for `<<<<<<<` or `>>>>>>>` markers (with trailing +/// space to avoid false positives from reStructuredText heading underlines). +/// Returns `Some(failure)` if conflicts are found, `None` if the tree is clean. +async fn check_for_conflict_markers(worktree_path: &std::path::Path) -> anyhow::Result> { + let conflict_check = Command::new("git") + .args(["grep", "-l", "-E", r"^(<<<<<<<\s|>>>>>>>\s)"]) + .current_dir(worktree_path) + .output() + .await + .context("Failed to run git grep for conflict markers")?; + + if conflict_check.status.success() { + let conflicting_files = String::from_utf8_lossy(&conflict_check.stdout).trim().to_string(); + let err_msg = format!( + "Patch applied with unresolved conflict markers in: {}", + conflicting_files + ); + warn!("{}", err_msg); + return Ok(Some(ExecutionResult::failure(err_msg))); + } + Ok(None) +} + +/// Apply a patch to a git worktree using `git apply --3way` with `--exclude` flags. +/// +/// Used when `excluded-files` are configured (git am does not support `--exclude`). +/// Returns `Ok(false)` on success (`false` = changes are staged, not committed). +async fn apply_patch_with_exclusions( + worktree_path: &std::path::Path, + patch_path: &std::path::Path, + exclude_args: &[String], +) -> anyhow::Result> { + debug!("Using git apply --3way (excluded-files configured)"); + let mut apply_args: Vec = vec!["apply".into(), "--3way".into()]; + apply_args.extend(exclude_args.iter().cloned()); + apply_args.push(patch_path.to_string_lossy().into_owned()); + + let apply_output = Command::new("git") + .args(&apply_args) + .current_dir(worktree_path) + .output() + .await + .context("Failed to run git apply --3way")?; + + if !apply_output.status.success() { + let err_msg = format!( + "Patch could not be applied (conflicts): {}", + String::from_utf8_lossy(&apply_output.stderr) + ); + warn!("{}", err_msg); + return Ok(Err(ExecutionResult::failure(err_msg))); + } + debug!("Patch applied with git apply --3way"); + + if let Some(conflict_result) = check_for_conflict_markers(worktree_path).await? { + return Ok(Err(conflict_result)); + } + Ok(Ok(false)) +} + +/// Apply a patch to a git worktree using `git am --3way`, with a `git apply` fallback. +/// +/// Used when no `excluded-files` are configured. Prefers `git am` because it +/// preserves original commit metadata. Falls back to `git apply --3way` if `git am` +/// fails. Returns `Ok(true)` when `git am` committed the changes, `Ok(false)` when +/// `git apply` left them staged. +async fn apply_patch_without_exclusions( + worktree_path: &std::path::Path, + patch_path: &std::path::Path, +) -> anyhow::Result> { + // No exclusions — use git am --3way for proper commit metadata preservation + debug!("Applying patch with git am --3way"); + let am_output = Command::new("git") + .args(["am", "--3way", &patch_path.to_string_lossy()]) + .current_dir(worktree_path) + .output() + .await + .context("Failed to run git am")?; + + if am_output.status.success() { + debug!("Patch applied successfully with git am"); + return Ok(Ok(true)); + } + + let stderr = String::from_utf8_lossy(&am_output.stderr); + debug!("git am --3way failed: {}", stderr); + + // Abort the failed am to leave worktree clean + let _ = Command::new("git") + .args(["am", "--abort"]) + .current_dir(worktree_path) + .output() + .await; + + // Fallback: try git apply --3way + debug!("Falling back to git apply --3way"); + let apply_output = Command::new("git") + .args(["apply", "--3way", &patch_path.to_string_lossy()]) + .current_dir(worktree_path) + .output() + .await + .context("Failed to run git apply --3way")?; + + if !apply_output.status.success() { + let err_msg = format!( + "Patch could not be applied (conflicts): {}", + String::from_utf8_lossy(&apply_output.stderr) + ); + warn!("{}", err_msg); + return Ok(Err(ExecutionResult::failure(err_msg))); + } + debug!("Patch applied with git apply --3way fallback"); + + if let Some(conflict_result) = check_for_conflict_markers(worktree_path).await? { + return Ok(Err(conflict_result)); + } + Ok(Ok(false)) +} + +/// Apply a patch to a git worktree, choosing the right strategy automatically. +/// +/// Delegates to [`apply_patch_with_exclusions`] when `exclude_args` is non-empty +/// (because `git am` doesn't support `--exclude`), otherwise delegates to +/// [`apply_patch_without_exclusions`] which prefers `git am` for commit metadata. +/// +/// Returns `Ok(patch_committed)` on success (`true` = changes are committed via +/// `git am`, `false` = changes are staged via `git apply`). +/// Returns `Err(ExecutionResult)` on expected failures (conflicts, patch errors). +async fn apply_patch_to_worktree( + worktree_path: &std::path::Path, + patch_path: &std::path::Path, + exclude_args: &[String], +) -> anyhow::Result> { + if !exclude_args.is_empty() { + apply_patch_with_exclusions(worktree_path, patch_path, exclude_args).await + } else { + apply_patch_without_exclusions(worktree_path, patch_path).await + } +} + +/// Set PR completion options (delete-source-branch, squash-merge) and optionally +/// enable auto-complete. Logs a warning on failure but does not propagate the error +/// because these are best-effort settings that do not affect the PR's existence. +async fn set_pr_completion_options( + client: &reqwest::Client, + config: &CreatePrConfig, + org_url: &str, + project: &str, + repo_id: &str, + pr_id: i64, + pr_created_by_id: Option<&str>, + token: &str, +) { + debug!( + "Setting PR completion options: delete_source_branch={}, squash_merge={}, auto_complete={}", + config.delete_source_branch, config.squash_merge, config.auto_complete + ); + let pr_update_url = format!( + "{}{}/_apis/git/repositories/{}/pullrequests/{}?api-version=7.1", + org_url, project, repo_id, pr_id + ); + + let mut update_body = serde_json::json!({ + "completionOptions": { + "deleteSourceBranch": config.delete_source_branch, + "squashMerge": config.squash_merge + } + }); + + // Only set autoCompleteSetBy if auto_complete is enabled and PR is not a draft + // (ADO silently ignores auto-complete on draft PRs, so skip the API call) + if config.auto_complete && !config.draft { + if let Some(creator_id) = pr_created_by_id { + update_body["autoCompleteSetBy"] = serde_json::json!({ "id": creator_id }); + } + } + + match client + .patch(&pr_update_url) + .basic_auth("", Some(token)) + .json(&update_body) + .send() + .await + { + Ok(resp) if resp.status().is_success() => { + debug!("PR completion options set successfully"); + } + Ok(resp) => { + warn!("Failed to set PR completion options: {}", resp.status()); + } + Err(e) => { + warn!("Failed to set PR completion options: {}", e); + } + } +} + +/// Add configured reviewers to a pull request. +/// +/// 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( + client: &reqwest::Client, + config: &CreatePrConfig, + org_url: &str, + project: &str, + repo_id: &str, + pr_id: i64, + organization: &str, + token: &str, +) { + if config.reviewers.is_empty() { + return; + } + debug!("Adding {} reviewers", config.reviewers.len()); + for reviewer in &config.reviewers { + debug!("Adding reviewer: {}", reviewer); + + // Resolve reviewer identity (email/name -> ID) + let reviewer_id = match resolve_reviewer_identity(client, organization, 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", + org_url, project, repo_id, pr_id, reviewer_id + ); + let reviewer_body = serde_json::json!({ "vote": 0, "isRequired": false }); + + match client + .put(&reviewer_url) + .basic_auth("", Some(token)) + .json(&reviewer_body) + .send() + .await + { + Ok(resp) if resp.status().is_success() => { + debug!("Reviewer '{}' (ID: {}) added successfully", reviewer, reviewer_id); + } + Ok(resp) => { + warn!( + "Failed to add reviewer '{}' (ID: {}): {}", + reviewer, reviewer_id, resp.status() + ); + } + Err(e) => { + warn!("Failed to add reviewer '{}': {}", reviewer, e); + } + } + } +} + /// Collect file changes from a worktree based on git status output /// /// Parses git status --porcelain output and reads file contents to build From ea21a33e23637c9e0756baff1e9bfee967f86cc1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Apr 2026 05:39:15 +0000 Subject: [PATCH 2/2] fix: add warn! when auto_complete creator ID is unavailable in set_pr_completion_options Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/1c554779-676a-488c-b5b4-fcf961e2c542 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/safeoutputs/create_pr.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/safeoutputs/create_pr.rs b/src/safeoutputs/create_pr.rs index 0e159f6b..6e3e2fdc 100644 --- a/src/safeoutputs/create_pr.rs +++ b/src/safeoutputs/create_pr.rs @@ -1504,6 +1504,8 @@ async fn set_pr_completion_options( if config.auto_complete && !config.draft { if let Some(creator_id) = pr_created_by_id { update_body["autoCompleteSetBy"] = serde_json::json!({ "id": creator_id }); + } else { + warn!("auto_complete requested but creator ID is unavailable; skipping autoCompleteSetBy"); } }