From ccad08af07d4ddedcd67198772292ac12c15bb10 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 15 Jul 2026 09:31:04 +0000 Subject: [PATCH] refactor: reduce complexity of execute_impl in comment_on_work_item.rs Extract validate_target_policy helper to flatten the 3-arm match/nested-match block that combined allows_id, area_path_prefix, and get_work_item_area_path into a single nesting depth 6 block. The new helper returns Ok(None) for "allowed", Ok(Some(msg)) for "rejected", keeping execute_impl to a single early-return guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/safe_outputs/comment_on_work_item.rs | 116 +++++++++++++---------- 1 file changed, 65 insertions(+), 51 deletions(-) diff --git a/src/safe_outputs/comment_on_work_item.rs b/src/safe_outputs/comment_on_work_item.rs index 628b95fd..7adab3ae 100644 --- a/src/safe_outputs/comment_on_work_item.rs +++ b/src/safe_outputs/comment_on_work_item.rs @@ -118,6 +118,66 @@ impl Default for CommentOnWorkItemConfig { } } +/// Validate that a work item is allowed by the configured target policy. +/// +/// Returns `Ok(None)` when the work item is allowed, `Ok(Some(msg))` when it is +/// rejected by policy (the caller should return `ExecutionResult::failure(msg)`), +/// or `Err(…)` on an unexpected infrastructure failure. +async fn validate_target_policy( + target: &CommentTarget, + client: &reqwest::Client, + org_url: &str, + project: &str, + token: &str, + work_item_id: i64, +) -> anyhow::Result> { + match target.allows_id(work_item_id) { + Some(true) => { + debug!("Work item #{} allowed by target policy", work_item_id); + Ok(None) + } + Some(false) => Ok(Some(format!( + "Work item #{} is not in the allowed target set", + work_item_id + ))), + None => { + // Area path validation — `allows_id` returns `None` only for + // `StringTarget(s != "*")`, so `area_path_prefix` is always `Some` here. + let prefix = target + .area_path_prefix() + .expect("allows_id returned None but area_path_prefix is also None"); + debug!( + "Validating area path for work item #{} against prefix '{}'", + work_item_id, prefix + ); + match get_work_item_area_path(client, org_url, project, token, work_item_id).await { + Ok(area_path) => { + // ADO area paths are case-insensitive and use backslash separators. + // Require the match to land on a path boundary so that prefix "4x4" + // doesn't accidentally match "4x4Production". + let ap = area_path.to_lowercase(); + let pf = prefix.to_lowercase(); + let is_match = + ap == pf || (ap.starts_with(&*pf) && ap[pf.len()..].starts_with('\\')); + if is_match { + debug!("Area path '{}' validated against '{}'", area_path, prefix); + Ok(None) + } else { + Ok(Some(format!( + "Work item #{} has area path '{}' which is not under allowed prefix '{}'", + work_item_id, area_path, prefix + ))) + } + } + Err(e) => Ok(Some(format!( + "Failed to validate area path for work item #{}: {}", + work_item_id, e + ))), + } + } + } +} + /// Fetch a work item's area path from the ADO API async fn get_work_item_area_path( client: &reqwest::Client, @@ -216,57 +276,11 @@ impl Executor for CommentOnWorkItemResult { let client = reqwest::Client::new(); // Validate work item ID against target policy - match target.allows_id(self.work_item_id) { - Some(true) => { - debug!("Work item #{} allowed by target policy", self.work_item_id); - } - Some(false) => { - return Ok(ExecutionResult::failure(format!( - "Work item #{} is not in the allowed target set", - self.work_item_id - ))); - } - None => { - // Area path validation — need to fetch the work item. - // Invariant: allows_id returns None only for StringTarget(s != "*"), - // and area_path_prefix returns Some for exactly that case. - let prefix = match target.area_path_prefix() { - Some(p) => p, - None => { - unreachable!("allows_id returned None but area_path_prefix is also None") - } - }; - debug!( - "Validating area path for work item #{} against prefix '{}'", - self.work_item_id, prefix - ); - match get_work_item_area_path(&client, org_url, project, token, self.work_item_id) - .await - { - Ok(area_path) => { - // ADO area paths are case-insensitive and use backslash separators. - // Require the match to land on a path boundary so that prefix "4x4" - // doesn't accidentally match "4x4Production". - let ap = area_path.to_lowercase(); - let pf = prefix.to_lowercase(); - let is_match = - ap == pf || (ap.starts_with(&*pf) && ap[pf.len()..].starts_with('\\')); - if !is_match { - return Ok(ExecutionResult::failure(format!( - "Work item #{} has area path '{}' which is not under allowed prefix '{}'", - self.work_item_id, area_path, prefix - ))); - } - debug!("Area path '{}' validated against '{}'", area_path, prefix); - } - Err(e) => { - return Ok(ExecutionResult::failure(format!( - "Failed to validate area path for work item #{}: {}", - self.work_item_id, e - ))); - } - } - } + if let Some(rejection_msg) = + validate_target_policy(target, &client, org_url, project, token, self.work_item_id) + .await? + { + return Ok(ExecutionResult::failure(rejection_msg)); } // Build the Azure DevOps REST API URL for adding a comment