From da7c02c45706644d2b9cc61bc10611b62e25e8a7 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 09:04:56 +0000 Subject: [PATCH] refactor(safe-outputs): reduce complexity of link_github_sub_issue execute_impl Split the 190-line execute_impl into focused helpers: - resolve_targets: resolve and validate parent/sub-issue targets - fetch_and_validate_metadata: fetch issue metadata and validate capability/mutation filters - check_existing_parent: preflight check for already-linked sub-issues - link_sub_issue: perform the addSubIssue mutation and validate the response No behaviour change; all 9 existing unit tests for this module pass unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/safe_outputs/link_github_sub_issue.rs | 335 +++++++++++++--------- 1 file changed, 200 insertions(+), 135 deletions(-) diff --git a/src/safe_outputs/link_github_sub_issue.rs b/src/safe_outputs/link_github_sub_issue.rs index 4a5e48ea..e7d0cb4a 100644 --- a/src/safe_outputs/link_github_sub_issue.rs +++ b/src/safe_outputs/link_github_sub_issue.rs @@ -8,10 +8,10 @@ use serde_json::Value; use crate::safe_outputs::{ ExecutionContext, ExecutionResult, Executor, GithubClient, GithubIssueNumber, - GithubMutationFilters, GithubRepositoryPolicy, GithubTargetCapabilities, Validate, - resolve_github_issue_target, validate_github_mutation_filter_config, - validate_github_mutation_filters, validate_github_repository, - validate_github_target_capability, + GithubMutationFilters, GithubRepositoryPolicy, GithubTargetCapabilities, GithubTargetMetadata, + ResolvedGithubIssueTarget, Validate, resolve_github_issue_target, + validate_github_mutation_filter_config, validate_github_mutation_filters, + validate_github_repository, validate_github_target_capability, }; use crate::sanitize::{SanitizeContent, sanitize_config}; use crate::tool_result; @@ -143,6 +143,52 @@ impl Executor for LinkGithubSubIssueResult { if let Err(error) = validate_link_github_sub_issue_config(&config) { return Ok(ExecutionResult::failure(error.to_string())); } + + let (parent, sub_issue) = match self.resolve_targets(ctx, &config)? { + Ok(targets) => targets, + Err(result) => return Ok(result), + }; + + let client = GithubClient::new(&ctx.github_api_url, token)?; + let (parent_metadata, sub_metadata) = + match fetch_and_validate_metadata(&client, &parent, &sub_issue, &config).await? { + Ok(metadata) => metadata, + Err(result) => return Ok(result), + }; + let Some(parent_node_id) = parent_metadata.node_id.as_deref() else { + return Ok(ExecutionResult::failure(format!( + "GitHub parent issue {}#{} has no GraphQL node ID; sub-issues are unsupported or unavailable", + parent.repository, parent.number + ))); + }; + let Some(sub_node_id) = sub_metadata.node_id.as_deref() else { + return Ok(ExecutionResult::failure(format!( + "GitHub sub-issue {}#{} has no GraphQL node ID; sub-issues are unsupported or unavailable", + sub_issue.repository, sub_issue.number + ))); + }; + + if let Some(result) = + check_existing_parent(&client, sub_node_id, parent_node_id, &parent, &sub_issue) + .await? + { + return Ok(result); + } + + link_sub_issue(&client, parent_node_id, sub_node_id, &parent, &sub_issue).await + } +} + +impl LinkGithubSubIssueResult { + /// Resolves the parent and sub-issue targets and checks they refer to the + /// same repository and to two distinct issues. + fn resolve_targets( + &self, + ctx: &ExecutionContext, + config: &LinkGithubSubIssueConfig, + ) -> anyhow::Result< + Result<(ResolvedGithubIssueTarget, ResolvedGithubIssueTarget), ExecutionResult>, + > { let policy = GithubRepositoryPolicy::new(config.target_repo.as_deref(), &config.allowed_repos); let parent = match resolve_github_issue_target( @@ -152,7 +198,7 @@ impl Executor for LinkGithubSubIssueResult { ctx, )? { Ok(target) => target, - Err(result) => return Ok(result), + Err(result) => return Ok(Err(result)), }; let sub_issue = match resolve_github_issue_target( &self.sub_issue_number, @@ -161,165 +207,184 @@ impl Executor for LinkGithubSubIssueResult { ctx, )? { Ok(target) => target, - Err(result) => return Ok(result), + Err(result) => return Ok(Err(result)), }; if !parent .repository .eq_ignore_ascii_case(&sub_issue.repository) { - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "parent issue repository '{}' and sub-issue repository '{}' must be the same", parent.repository, sub_issue.repository - ))); + )))); } if parent.number == sub_issue.number { - return Ok(ExecutionResult::failure( + return Ok(Err(ExecutionResult::failure( "parent_issue_number and sub_issue_number resolved to the same GitHub issue", - )); - } - - let client = GithubClient::new(&ctx.github_api_url, token)?; - let parent_metadata = match client.get_issue(&parent.repository, parent.number).await? { - Ok(metadata) => metadata, - Err(error) => return Ok(ExecutionResult::failure(error.to_string())), - }; - let sub_metadata = match client - .get_issue(&sub_issue.repository, sub_issue.number) - .await? - { - Ok(metadata) => metadata, - Err(error) => return Ok(ExecutionResult::failure(error.to_string())), - }; - for metadata in [&parent_metadata, &sub_metadata] { - if let Err(result) = - validate_github_target_capability(metadata, GithubTargetCapabilities::ISSUES_ONLY) - { - return Ok(result); - } - } - let parent_filters = GithubMutationFilters { - required_labels: &config.parent_required_labels, - required_title_prefix: config.parent_title_prefix.as_deref(), - }; - let sub_filters = GithubMutationFilters { - required_labels: &config.sub_required_labels, - required_title_prefix: config.sub_title_prefix.as_deref(), - }; - if let Err(result) = validate_github_mutation_filters(&parent_metadata, parent_filters) { - return Ok(result); - } - if let Err(result) = validate_github_mutation_filters(&sub_metadata, sub_filters) { - return Ok(result); - } - let Some(parent_node_id) = parent_metadata.node_id.as_deref() else { - return Ok(ExecutionResult::failure(format!( - "GitHub parent issue {}#{} has no GraphQL node ID; sub-issues are unsupported or unavailable", - parent.repository, parent.number ))); - }; - let Some(sub_node_id) = sub_metadata.node_id.as_deref() else { - return Ok(ExecutionResult::failure(format!( - "GitHub sub-issue {}#{} has no GraphQL node ID; sub-issues are unsupported or unavailable", - sub_issue.repository, sub_issue.number - ))); - }; + } + Ok(Ok((parent, sub_issue))) + } +} - let preflight = match client - .graphql( - "Check GitHub sub-issue parent", - GET_SUB_ISSUE_PARENT, - serde_json::json!({ "id": sub_node_id }), - ) - .await? +/// Fetches parent and sub-issue metadata and validates capability and mutation filters. +async fn fetch_and_validate_metadata( + client: &GithubClient, + parent: &ResolvedGithubIssueTarget, + sub_issue: &ResolvedGithubIssueTarget, + config: &LinkGithubSubIssueConfig, +) -> anyhow::Result> { + let parent_metadata = match client.get_issue(&parent.repository, parent.number).await? { + Ok(metadata) => metadata, + Err(error) => return Ok(Err(ExecutionResult::failure(error.to_string()))), + }; + let sub_metadata = match client + .get_issue(&sub_issue.repository, sub_issue.number) + .await? + { + Ok(metadata) => metadata, + Err(error) => return Ok(Err(ExecutionResult::failure(error.to_string()))), + }; + for metadata in [&parent_metadata, &sub_metadata] { + if let Err(result) = + validate_github_target_capability(metadata, GithubTargetCapabilities::ISSUES_ONLY) { - Ok(data) => data, - Err(error) => { - return Ok(ExecutionResult::failure(format!( - "GitHub sub-issues are unsupported or unavailable: {error}" - ))); - } - }; - if let Some(existing) = match parse_existing_parent(&preflight) { - Ok(parent) => parent, - Err(message) => return Ok(ExecutionResult::failure(message)), - } { - let same_parent = existing.id == parent_node_id; - if same_parent { - info!( - "GitHub issue {}#{} is already a sub-issue of #{}", - parent.repository, sub_issue.number, parent.number - ); - return Ok(ExecutionResult::success_with_data( - format!( - "GitHub issue {}#{} is already a sub-issue of #{}", - parent.repository, sub_issue.number, parent.number - ), - serde_json::json!({ - "parent_issue_number": parent.number, - "sub_issue_number": sub_issue.number, - "target_repo": parent.repository, - "already_linked": true, - }), - )); - } - let existing_target = format!("{}#{}", existing.repository, existing.number); - return Ok(ExecutionResult::failure(format!( - "GitHub issue {}#{} is already linked to a different parent ({existing_target}); refusing to replace it", - sub_issue.repository, sub_issue.number - ))); + return Ok(Err(result)); } + } + let parent_filters = GithubMutationFilters { + required_labels: &config.parent_required_labels, + required_title_prefix: config.parent_title_prefix.as_deref(), + }; + let sub_filters = GithubMutationFilters { + required_labels: &config.sub_required_labels, + required_title_prefix: config.sub_title_prefix.as_deref(), + }; + if let Err(result) = validate_github_mutation_filters(&parent_metadata, parent_filters) { + return Ok(Err(result)); + } + if let Err(result) = validate_github_mutation_filters(&sub_metadata, sub_filters) { + return Ok(Err(result)); + } + Ok(Ok((parent_metadata, sub_metadata))) +} - debug!( - "Linking GitHub issue {}#{} as a sub-issue of #{}", - parent.repository, sub_issue.number, parent.number - ); - let mutation = match client - .graphql( - "Link GitHub sub-issue", - ADD_SUB_ISSUE, - serde_json::json!({ - "parentId": parent_node_id, - "subIssueId": sub_node_id, - }), - ) - .await? - { - Ok(data) => data, - Err(error) => { - return Ok(ExecutionResult::failure(format!( - "GitHub addSubIssue mutation is unsupported or failed: {error}" - ))); - } - }; - let mutated_parent = mutation - .pointer("/addSubIssue/issue/number") - .and_then(Value::as_u64); - let mutated_sub = mutation - .pointer("/addSubIssue/subIssue/number") - .and_then(Value::as_u64); - if mutated_parent != Some(parent.number) || mutated_sub != Some(sub_issue.number) { - return Ok(ExecutionResult::failure( - "GitHub addSubIssue response did not identify the requested parent and sub-issue", - )); +/// Checks whether the sub-issue already has a parent, returning an early +/// result (success if already linked to the same parent, failure if linked +/// to a different one) or `None` if linking should proceed. +async fn check_existing_parent( + client: &GithubClient, + sub_node_id: &str, + parent_node_id: &str, + parent: &ResolvedGithubIssueTarget, + sub_issue: &ResolvedGithubIssueTarget, +) -> anyhow::Result> { + let preflight = match client + .graphql( + "Check GitHub sub-issue parent", + GET_SUB_ISSUE_PARENT, + serde_json::json!({ "id": sub_node_id }), + ) + .await? + { + Ok(data) => data, + Err(error) => { + return Ok(Some(ExecutionResult::failure(format!( + "GitHub sub-issues are unsupported or unavailable: {error}" + )))); } + }; + let Some(existing) = (match parse_existing_parent(&preflight) { + Ok(parent) => parent, + Err(message) => return Ok(Some(ExecutionResult::failure(message))), + }) else { + return Ok(None); + }; + if existing.id == parent_node_id { info!( - "Linked GitHub issue {}#{} as a sub-issue of #{}", + "GitHub issue {}#{} is already a sub-issue of #{}", parent.repository, sub_issue.number, parent.number ); - Ok(ExecutionResult::success_with_data( + return Ok(Some(ExecutionResult::success_with_data( format!( - "Linked GitHub issue {}#{} as a sub-issue of #{}", + "GitHub issue {}#{} is already a sub-issue of #{}", parent.repository, sub_issue.number, parent.number ), serde_json::json!({ "parent_issue_number": parent.number, "sub_issue_number": sub_issue.number, "target_repo": parent.repository, - "already_linked": false, + "already_linked": true, + }), + ))); + } + let existing_target = format!("{}#{}", existing.repository, existing.number); + Ok(Some(ExecutionResult::failure(format!( + "GitHub issue {}#{} is already linked to a different parent ({existing_target}); refusing to replace it", + sub_issue.repository, sub_issue.number + )))) +} + +/// Performs the `addSubIssue` mutation and validates the response identifies +/// the requested parent and sub-issue. +async fn link_sub_issue( + client: &GithubClient, + parent_node_id: &str, + sub_node_id: &str, + parent: &ResolvedGithubIssueTarget, + sub_issue: &ResolvedGithubIssueTarget, +) -> anyhow::Result { + debug!( + "Linking GitHub issue {}#{} as a sub-issue of #{}", + parent.repository, sub_issue.number, parent.number + ); + let mutation = match client + .graphql( + "Link GitHub sub-issue", + ADD_SUB_ISSUE, + serde_json::json!({ + "parentId": parent_node_id, + "subIssueId": sub_node_id, }), - )) + ) + .await? + { + Ok(data) => data, + Err(error) => { + return Ok(ExecutionResult::failure(format!( + "GitHub addSubIssue mutation is unsupported or failed: {error}" + ))); + } + }; + let mutated_parent = mutation + .pointer("/addSubIssue/issue/number") + .and_then(Value::as_u64); + let mutated_sub = mutation + .pointer("/addSubIssue/subIssue/number") + .and_then(Value::as_u64); + if mutated_parent != Some(parent.number) || mutated_sub != Some(sub_issue.number) { + return Ok(ExecutionResult::failure( + "GitHub addSubIssue response did not identify the requested parent and sub-issue", + )); } + + info!( + "Linked GitHub issue {}#{} as a sub-issue of #{}", + parent.repository, sub_issue.number, parent.number + ); + Ok(ExecutionResult::success_with_data( + format!( + "Linked GitHub issue {}#{} as a sub-issue of #{}", + parent.repository, sub_issue.number, parent.number + ), + serde_json::json!({ + "parent_issue_number": parent.number, + "sub_issue_number": sub_issue.number, + "target_repo": parent.repository, + "already_linked": false, + }), + )) } pub(crate) fn validate_link_github_sub_issue_config(