refactor: reduce complexity of execute_impl in create_pull_request.rs

[github-actions]

Summary

Refactors CreatePrResult::execute_impl in src/safe_outputs/create_pull_request.rs, which clippy flagged as the most complex function in the codebase (clippy::too_many_lines).

Before / After

• Before: 582 lines (clippy too_many_lines warning threshold is 100)
• After: 237 lines

Approach

Extracted the monolithic function body into focused, well-named helper methods on CreatePrResult:

• build_effective_title — apply title prefix + validate the ADO 400-char title limit
• resolve_repo_alias_and_id — repository alias/ID resolution
• read_and_verify_patch — patch existence, size limit, SHA-256 integrity check
• validate_patch_content — path validation, protected-files check, max-files check
• setup_worktree_and_collect_changes — git worktree creation, branch checkout, patch application, change collection
• resolve_base_commit — recorded base commit or ADO refs API lookup
• ensure_unique_source_branch — retry-with-suffix loop for branch name collisions
• build_pr_description — PR body assembly (stats + symlink notice + footer)
• build_fallback_failure_result — fallback branch-recorded failure result

Also added two small grouping structs, AdoConnection and WorktreeChanges, following the existing PushBranchParams/PrContext convention already used elsewhere in this file to avoid clippy::too_many_arguments.

execute_impl itself is now a short orchestration function that calls these helpers in sequence.

Behavior

No public API or observable behavior changes — this is a pure refactor.

Verification

• cargo build — clean
• cargo test (full workspace suite, including all 58 create_pull_request tests) — all pass
• cargo clippy --all-targets --all-features — clean, no new warnings
• Re-ran clippy::too_many_lines scoped to the file: execute_impl dropped from 582/100 to 237/100

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• spsprodeus21.vssps.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "spsprodeus21.vssps.visualstudio.com"

See Network Configuration for more information.

Generated by Cyclomatic Complexity Reducer · auto · 186.3 AIC · ⌖ 15.8 AIC · ⊞ 11.4K · ◷

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

✅ PR Security Reviewer completed the security review.

Warning

Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.

What happened

The threat detection results could not be parsed.

Review the workflow run logs for details.

Reviewed PR #1946 (pure refactor of CreatePrResult::execute_impl into helper methods). Confirmed all security-relevant checks (patch size limit, SHA-256 integrity verification, path validation, protected-files check, max-files check, ADO token scoping via AdoConnection, sanitize_text usage in fallback description) are preserved verbatim across the extraction -- no logic changes, no new inputs, no weakened validation. Diff is security-neutral; no findings to post.

🔒 Security review by PR Security Reviewer

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

Reviewed PR #1946 as Test Quality Sentinel. The diff is a pure refactor of CreatePrResult::execute_impl into smaller private helper methods (build_effective_title, resolve_repo_alias_and_id, read_and_verify_patch, validate_patch_content, setup_worktree_and_collect_changes, resolve_base_commit, ensure_unique_source_branch, build_pr_description, build_fallback_failure_result) — no new public behavior, no test files touched, and no assertions added, weakened, or removed. The existing 58 tests in this file already exercise execute_impl end-to-end and continue to do so unchanged, since the extracted helpers are called from the same orchestration path. There is no new behavior surface requiring new tests and no regression risk in test coverage from this change, so no review comments are warranted from a test-quality perspective.

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✅ Compiler Contract Reviewer completed the compiler contract review.

Reviewed PR #1946 as compiler-contract reviewer. This is a pure internal refactor of CreatePrResult::execute_impl in src/safe_outputs/create_pull_request.rs, extracting helper methods without changing the Params struct, front-matter grammar, safe-output tool registration, typed IR, generated shell scripts, or any documented public contract. No codegen/lock/bundle drift: only one file changed (per pr-meta.json files list), no touches to filter_ir.rs, types.gen.ts, fact-catalog.gen.json, .github/workflows/*.md, or docs/. No raw String identifier fields introduced on Params (all new helper params are internal function args, not deserialized safe-output input). No compiler-contract findings to raise.

🏗️ Compiler contract review by Compiler Contract Reviewer

[github-actions]

✅ Rust Code Quality Reviewer completed the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

Rust code quality review

Clean mechanical refactor. execute_impl (582 → 237 lines) was split into well-named helper methods, each keeping its original logic and error-handling paths intact — I diffed each extracted block against the original inline code and found no behavior drift, no swallowed context, and no new unwrap()/expect() on user-reachable paths. The anyhow::Result<Result<T, ExecutionResult>> double-Result pattern used for the new helpers already matches existing convention elsewhere in this file and in github_issue_common.rs/hide_github_issue_comment.rs, so it's consistent rather than novel.

Spot-checked for the common refactor failure modes (state not threaded through to helpers, reordered calls, borrow→clone regressions, lossy casts): none found. cargo build is clean.

Note: the rust-critic sub-agent I dispatched in parallel did not return a response within the session, so this review reflects only my own pass — no additional findings to reconcile.

No merge-blocking issues.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 50.1 AIC · ⌖ 12.1 AIC · ⊞ 11.5K
_{Comment /review to run again}