From 88518170d31ddc71068b70393bb2fabc1cdd7a6a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 15 Jun 2026 11:04:50 +0000 Subject: [PATCH] refactor: reduce complexity of execute_add_reviewers in update_pr.rs Extract per-reviewer loop body into a standalone async helper resolve_and_add_reviewer, reducing execute_add_reviewers cognitive complexity from 21 to below 15 (at threshold=15). The extracted function handles: - VSSPS identity resolution (3 match arms) - PR reviewer PUT request (3 match arms) A new ReviewerAddResult enum carries the per-reviewer outcome back to the caller, replacing the inline push-to-added/push-to-failed pattern. Observable behaviour (success/failure messages, JSON output shape) is unchanged; all 1858 tests continue to pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/safeoutputs/update_pr.rs | 207 ++++++++++++++++++++--------------- 1 file changed, 116 insertions(+), 91 deletions(-) diff --git a/src/safeoutputs/update_pr.rs b/src/safeoutputs/update_pr.rs index 7341c4ef..4386cd50 100644 --- a/src/safeoutputs/update_pr.rs +++ b/src/safeoutputs/update_pr.rs @@ -345,6 +345,12 @@ impl Executor for UpdatePrResult { } } +/// Outcome of a single reviewer resolution + add attempt. +enum ReviewerAddResult { + Added, + Failed(String), +} + impl UpdatePrResult { /// Set auto-complete on a pull request. /// @@ -677,99 +683,20 @@ impl UpdatePrResult { } for reviewer in reviewers { - let identity_url = format!( - "{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", - vssps_base, - utf8_percent_encode(reviewer, PATH_SEGMENT), - ); - debug!("Resolving identity for '{}': {}", reviewer, identity_url); - - let identity_response = client - .get(&identity_url) - .basic_auth("", Some(token)) - .send() - .await; - - let reviewer_id = match identity_response { - Ok(resp) if resp.status().is_success() => { - let body: serde_json::Value = resp.json().await.unwrap_or_default(); - body.get("value") - .and_then(|v| v.as_array()) - .and_then(|arr| arr.first()) - .and_then(|entry| entry.get("id")) - .and_then(|id| id.as_str()) - .map(|s| s.to_string()) - } - Ok(resp) => { - warn!( - "Identity lookup for '{}' returned HTTP {}", - reviewer, - resp.status() - ); - None - } - Err(e) => { - warn!("Identity lookup for '{}' failed: {}", reviewer, e); - None - } - }; - - let reviewer_id = match reviewer_id { - Some(id) => id, - None => { - warn!( - "Could not resolve identity for '{}', skipping", - reviewer - ); - failed.push(format!("{} (identity not found)", reviewer)); - continue; - } - }; - - let reviewer_url = format!( - "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", + match resolve_and_add_reviewer( + client, + &vssps_base, base_url, - encoded_repo, + &encoded_repo, self.pull_request_id, - reviewer_id, - ); - let reviewer_body = serde_json::json!({ - "vote": 0, - "isRequired": false - }); - - debug!("Adding reviewer '{}' to PR #{}", reviewer, self.pull_request_id); - let response = client - .put(&reviewer_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&reviewer_body) - .send() - .await; - - match response { - Ok(resp) if resp.status().is_success() => { - info!("Added reviewer '{}' to PR #{}", reviewer, self.pull_request_id); - added.push(reviewer.clone()); - } - Ok(resp) => { - let status = resp.status(); - let error_body = resp - .text() - .await - .unwrap_or_else(|_| "Unknown error".to_string()); - warn!( - "Failed to add reviewer '{}' to PR #{} (HTTP {}): {}", - reviewer, self.pull_request_id, status, error_body - ); - failed.push(format!("{} (HTTP {})", reviewer, status)); - } - Err(e) => { - warn!( - "Request failed for reviewer '{}' on PR #{}: {}", - reviewer, self.pull_request_id, e - ); - failed.push(format!("{} (request error)", reviewer)); + reviewer, + token, + ) + .await + { + ReviewerAddResult::Added => added.push(reviewer.clone()), + ReviewerAddResult::Failed(reason) => { + failed.push(format!("{} ({})", reviewer, reason)); } } } @@ -966,6 +893,104 @@ impl UpdatePrResult { } } +/// Resolve an ADO identity for `reviewer` via the VSSPS identities API, then +/// PUT the reviewer onto the PR. Returns [`ReviewerAddResult::Added`] on success +/// or [`ReviewerAddResult::Failed`] with a short reason string on any failure. +async fn resolve_and_add_reviewer( + client: &reqwest::Client, + vssps_base: &str, + base_url: &str, + encoded_repo: &str, + pr_id: i32, + reviewer: &str, + token: &str, +) -> ReviewerAddResult { + // Step 1: Resolve the reviewer's Azure DevOps identity via VSSPS. + let identity_url = format!( + "{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", + vssps_base, + utf8_percent_encode(reviewer, PATH_SEGMENT), + ); + debug!("Resolving identity for '{}': {}", reviewer, identity_url); + + let identity_response = client + .get(&identity_url) + .basic_auth("", Some(token)) + .send() + .await; + + let reviewer_id = match identity_response { + Ok(resp) if resp.status().is_success() => { + let body: serde_json::Value = resp.json().await.unwrap_or_default(); + body.get("value") + .and_then(|v| v.as_array()) + .and_then(|arr| arr.first()) + .and_then(|entry| entry.get("id")) + .and_then(|id| id.as_str()) + .map(|s| s.to_string()) + } + Ok(resp) => { + warn!( + "Identity lookup for '{}' returned HTTP {}", + reviewer, + resp.status() + ); + None + } + Err(e) => { + warn!("Identity lookup for '{}' failed: {}", reviewer, e); + None + } + }; + + let Some(reviewer_id) = reviewer_id else { + warn!("Could not resolve identity for '{}', skipping", reviewer); + return ReviewerAddResult::Failed("identity not found".to_string()); + }; + + // Step 2: Add the resolved identity to the PR as a reviewer. + let reviewer_url = format!( + "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", + base_url, encoded_repo, pr_id, reviewer_id, + ); + let reviewer_body = serde_json::json!({ "vote": 0, "isRequired": false }); + + debug!("Adding reviewer '{}' to PR #{}", reviewer, pr_id); + let response = client + .put(&reviewer_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&reviewer_body) + .send() + .await; + + match response { + Ok(resp) if resp.status().is_success() => { + info!("Added reviewer '{}' to PR #{}", reviewer, pr_id); + ReviewerAddResult::Added + } + Ok(resp) => { + let status = resp.status(); + let error_body = resp + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + warn!( + "Failed to add reviewer '{}' to PR #{} (HTTP {}): {}", + reviewer, pr_id, status, error_body + ); + ReviewerAddResult::Failed(format!("HTTP {}", status)) + } + Err(e) => { + warn!( + "Request failed for reviewer '{}' on PR #{}: {}", + reviewer, pr_id, e + ); + ReviewerAddResult::Failed("request error".to_string()) + } + } +} + #[cfg(test)] mod tests { use super::*;