diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 47430e88..8560c900 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -588,54 +588,46 @@ impl Drop for WorktreeGuard { } } -#[async_trait::async_trait] -impl Executor for CreatePrResult { - fn dry_run_summary(&self) -> String { - format!("create PR: '{}' in repo '{}'", self.title, self.repository) - } - - async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { - info!( - "Creating PR: '{}' in repository '{}'", - self.title, self.repository - ); - debug!( - "create-pull-request: title='{}', repo='{}', branch='{}', patch='{}'", - self.title, self.repository, self.source_branch, self.patch_file - ); - debug!("PR description length: {} chars", self.description.len()); - debug!("Source branch: {}", self.source_branch); - debug!("Patch file: {}", self.patch_file); - - let config: CreatePrConfig = ctx.get_tool_config("create-pull-request")?; - debug!("Target branch from config: {}", config.target_branch); - debug!("Draft: {}", config.draft); - debug!("Auto-complete: {}", config.auto_complete); - debug!("Squash merge: {}", config.squash_merge); +/// Resolved Azure DevOps connection details needed to call the REST API. +struct AdoConnection<'a> { + org_url: &'a str, + project: &'a str, + token: &'a str, +} - if config.draft && config.auto_complete { - warn!( - "auto-complete cannot be set on a draft PR; set draft: false to enable auto-complete" - ); - } +/// Outcome of applying the patch and collecting the resulting file changes in +/// the worktree, ready to push (or `None` if there were no changes). +struct WorktreeChanges { + changes: Vec, + skipped_symlinks: Vec, +} - // Apply title prefix if configured +impl CreatePrResult { + /// Apply the `title-prefix` config and validate the ADO 400-character PR title limit. + /// Returns `Err(ExecutionResult)` when the resulting title is too long. + fn build_effective_title(&self, config: &CreatePrConfig) -> Result { let effective_title = if let Some(ref prefix) = config.title_prefix { format!("{}{}", prefix, self.title) } else { self.title.clone() }; - // ADO PR titles have a 400-character limit let title_char_count = effective_title.chars().count(); if title_char_count > 400 { - return Ok(ExecutionResult::failure(format!( + return Err(ExecutionResult::failure(format!( "PR title too long after applying title-prefix ({} chars, max 400)", title_char_count ))); } + Ok(effective_title) + } - // Validate repository against allowed list + /// Resolve `self.repository` against the allowed-repository list and return the + /// canonical alias plus the concrete ADO repository ID/name to use for REST calls. + fn resolve_repo_alias_and_id( + &self, + ctx: &ExecutionContext, + ) -> anyhow::Result> { debug!( "Validating repository '{}' against allowed list", self.repository @@ -652,7 +644,7 @@ impl Executor for CreatePrResult { self.repository, ctx.allowed_repositories.keys().collect::>() ); - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Repository '{}' is not in the allowed list. Allowed: self, {}", self.repository, ctx.allowed_repositories @@ -660,7 +652,7 @@ impl Executor for CreatePrResult { .cloned() .collect::>() .join(", ") - ))); + )))); }; let repo_id = if repository_alias == "self" { // "self" or a name match against the pipeline's own repository @@ -685,42 +677,27 @@ impl Executor for CreatePrResult { false, "canonical alias '{repository_alias}' is absent from allowed_repositories" ); - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Repository alias '{}' has no configured repository", repository_alias - ))); + )))); }; debug!("Resolved repository ID: {}", repo_id); + Ok(Ok((repository_alias, repo_id))) + } - // Get ADO configuration - let org_url = ctx - .ado_org_url - .as_ref() - .context("Azure DevOps organization URL not configured")?; - let organization = ctx - .ado_organization - .as_ref() - .context("Azure DevOps organization name not configured")?; - let project = ctx - .ado_project - .as_ref() - .context("Azure DevOps project not configured")?; - let token = ctx - .access_token - .as_ref() - .context("Access token not configured")?; - debug!( - "ADO org: {}, organization: {}, project: {}", - org_url, organization, project - ); - - // Validate and read the patch file + /// Read the patch file from disk, enforce the size limit, and verify its + /// SHA-256 integrity against the value recorded at Stage 1. + async fn read_and_verify_patch( + &self, + ctx: &ExecutionContext, + ) -> anyhow::Result> { let patch_path = ctx.working_directory.join(&self.patch_file); if !patch_path.exists() { - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Patch file not found: {}", self.patch_file - ))); + )))); } // Security: Enforce patch file size limit @@ -728,11 +705,11 @@ impl Executor for CreatePrResult { .await .context("Failed to get patch file metadata")?; if metadata.len() > MAX_PATCH_SIZE_BYTES { - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Patch file exceeds maximum size of {} bytes (got {} bytes)", MAX_PATCH_SIZE_BYTES, metadata.len() - ))); + )))); } // Read patch content for validation @@ -746,37 +723,37 @@ impl Executor for CreatePrResult { // with between Stage 1 and Stage 3. let live_hash = crate::hash::sha256_hex(patch_content.as_bytes()); if live_hash != self.patch_sha256 { - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Patch file SHA-256 mismatch: expected {}, got {} — \ the file may have been tampered with between stages", self.patch_sha256, live_hash - ))); + )))); } debug!("Patch file SHA-256 verified: {}", live_hash); + Ok(Ok((patch_path, patch_content))) + } + + /// Security checks against the patch content: unsafe paths, protected-file + /// modifications, and the max-files-per-PR limit. `exclude_args` is the set of + /// `--exclude=` flags derived from `config.excluded_files`, used to drop + /// excluded paths before the protected-file check. + fn validate_patch_content( + patch_content: &str, + config: &CreatePrConfig, + ) -> Result<(), ExecutionResult> { // Excluded files are handled via --exclude flags on git am / git apply, // which filters them at the git level rather than post-processing patch content. // This is the same approach used by gh-aw (via :(exclude) pathspecs). // Note: Exclusion happens during patch application (before the protection check). // If a protected file matches an excluded-files pattern, it is silently dropped // from the patch rather than triggering a protection error. - let exclude_args: Vec = config - .excluded_files - .iter() - .map(|p| format!("--exclude={}", p)) - .collect(); - if !exclude_args.is_empty() { - debug!( - "Will apply {} excluded-files patterns via --exclude flags", - exclude_args.len() - ); - } // Security: Validate patch paths before applying debug!("Validating patch paths for security"); - if let Err(e) = validate_patch_paths(&patch_content) { + if let Err(e) = validate_patch_paths(patch_content) { warn!("Patch path validation failed: {}", e); - return Ok(ExecutionResult::failure(format!( + return Err(ExecutionResult::failure(format!( "Patch validation failed: {}", e ))); @@ -787,7 +764,7 @@ impl Executor for CreatePrResult { // Filter out excluded files before the protection check — if a protected file // matches an excluded-files pattern, it will be excluded from the patch by // git am/apply --exclude and should not trigger a protection error. - let patch_paths: Vec = extract_paths_from_patch(&patch_content) + let patch_paths: Vec = extract_paths_from_patch(patch_content) .into_iter() .filter(|p| { !config @@ -806,7 +783,7 @@ impl Executor for CreatePrResult { protected.len(), protected ); - return Ok(ExecutionResult::failure(format!( + return Err(ExecutionResult::failure(format!( "Patch modifies protected files (set protected-files: allowed to override): {}", protected.join(", ") ))); @@ -815,51 +792,51 @@ impl Executor for CreatePrResult { // Security: Max files per PR check (count diff blocks, not paths, to avoid // double-counting renames which appear in both --- and +++ lines) - let file_count = count_patch_files(&patch_content); + let file_count = count_patch_files(patch_content); if file_count > config.max_files { warn!( "Patch contains {} files, exceeding max of {}", file_count, config.max_files ); - return Ok(ExecutionResult::failure(format!( + return Err(ExecutionResult::failure(format!( "Patch contains {} files, exceeding maximum of {} files per PR", file_count, config.max_files ))); } - // Resolve the target (base) branch for THIS repo. In a multi-checkout - // ("meta repo") setup the target can differ per repo (explicit - // `target-branches` override, or inferred from the repo's checkout ref - // when `infer-target-from-checkout-ref` is set); falls back to the - // literal `target-branch`. Resolution is shared with the compiler's - // prepare-pr-base deepening, so the branch we PR into is the branch that - // was fetched/deepened. - let target_branch = config.resolve_target_branch(&repository_alias, &ctx.repo_refs); - let target_branch = target_branch.as_str(); - let mut source_branch = self.source_branch.clone(); - let mut source_ref = format!("refs/heads/{}", source_branch); - let target_ref = format!("refs/heads/{}", target_branch); - debug!("Source ref: {}, Target ref: {}", source_ref, target_ref); - - let repo_git_dir = - crate::safe_outputs::resolve_repository_checkout_dir(&repository_alias, ctx)?; - debug!("Git repository directory: {}", repo_git_dir.display()); + Ok(()) + } + /// Create a git worktree at `target_branch`, check out `source_branch` in it, + /// apply the patch, and collect the resulting file changes. + /// + /// Returns the temp-dir guard (keeps the worktree alive), the worktree cleanup + /// guard, and the collected changes. The temp dir must be kept alive by the + /// caller for as long as the worktree is used. + #[allow(clippy::too_many_arguments)] + async fn setup_worktree_and_collect_changes( + repo_git_dir: &std::path::Path, + source_branch: &str, + target_branch: &str, + patch_path: &std::path::Path, + exclude_args: &[String], + ) -> anyhow::Result> + { // Verify this is a git repository debug!("Verifying git repository"); let git_check = Command::new("git") .args(["rev-parse", "--git-dir"]) - .current_dir(&repo_git_dir) + .current_dir(repo_git_dir) .output() .await .context("Failed to verify git repository")?; if !git_check.status.success() { warn!("Not a git repository: {}", repo_git_dir.display()); - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Not a git repository: {}", repo_git_dir.display() - ))); + )))); } debug!("Git repository verified"); @@ -876,7 +853,7 @@ impl Executor for CreatePrResult { &worktree_path.to_string_lossy(), &format!("origin/{}", target_branch), ]) - .current_dir(&repo_git_dir) + .current_dir(repo_git_dir) .output() .await .context("Failed to create git worktree")?; @@ -894,7 +871,7 @@ impl Executor for CreatePrResult { &worktree_path.to_string_lossy(), target_branch, ]) - .current_dir(&repo_git_dir) + .current_dir(repo_git_dir) .output() .await .context("Failed to create git worktree")?; @@ -904,17 +881,17 @@ impl Executor for CreatePrResult { "Failed to create worktree: {}", String::from_utf8_lossy(&worktree_output.stderr) ); - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Failed to create worktree: {}", String::from_utf8_lossy(&worktree_output.stderr) - ))); + )))); } } debug!("Worktree created successfully"); // Ensure worktree cleanup on exit - let _worktree_guard = WorktreeGuard { - repo_dir: repo_git_dir.clone(), + let worktree_guard = WorktreeGuard { + repo_dir: repo_git_dir.to_path_buf(), worktree_path: worktree_path.clone(), }; @@ -924,7 +901,7 @@ impl Executor for CreatePrResult { // branch name is not used for the remote ref. debug!("Creating source branch: {}", source_branch); let checkout_output = Command::new("git") - .args(["checkout", "-b", &source_branch]) + .args(["checkout", "-b", source_branch]) .current_dir(&worktree_path) .output() .await @@ -935,10 +912,10 @@ impl Executor for CreatePrResult { "Failed to create source branch: {}", String::from_utf8_lossy(&checkout_output.stderr) ); - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Failed to create source branch: {}", String::from_utf8_lossy(&checkout_output.stderr) - ))); + )))); } debug!("Source branch created"); @@ -962,9 +939,9 @@ impl Executor for CreatePrResult { // - With exclusions: use git apply --3way directly (git am does not support // --exclude flags; git apply does) let patch_committed = - match apply_patch_to_worktree(&worktree_path, &patch_path, &exclude_args).await? { + match apply_patch_to_worktree(&worktree_path, patch_path, exclude_args).await? { Ok(committed) => committed, - Err(result) => return Ok(result), + Err(result) => return Ok(Err(result)), }; // Collect changed files. The method depends on how the patch was applied: @@ -985,10 +962,10 @@ impl Executor for CreatePrResult { "Failed to get diff-tree: {}", String::from_utf8_lossy(&diff_tree_output.stderr) ); - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Failed to get diff-tree: {}", String::from_utf8_lossy(&diff_tree_output.stderr) - ))); + )))); } ( String::from_utf8_lossy(&diff_tree_output.stdout).to_string(), @@ -1007,10 +984,10 @@ impl Executor for CreatePrResult { "Failed to get git status: {}", String::from_utf8_lossy(&status_output.stderr) ); - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "Failed to get git status: {}", String::from_utf8_lossy(&status_output.stderr) - ))); + )))); } ( String::from_utf8_lossy(&status_output.stdout).to_string(), @@ -1033,27 +1010,27 @@ impl Executor for CreatePrResult { ); } - if changes.is_empty() { - return Ok(handle_no_changes(&config, &skipped_symlinks)); - } - - // Use ADO REST API to create branch and push changes - let client = reqwest::Client::new(); - - // Get the target branch ref to find the base commit - debug!("Getting target branch ref from ADO"); - let refs_url = format!( - "{}{}/_apis/git/repositories/{}/refs?filter=heads/{}&api-version=7.1", - org_url, project, repo_id, target_branch - ); - debug!("Refs URL: {}", refs_url); + Ok(Ok(( + temp_dir, + worktree_guard, + WorktreeChanges { + changes, + skipped_symlinks, + }, + ))) + } - // Resolve the base commit for the push. - // Prefer the merge-base SHA recorded at patch generation time (Stage 1) so the - // patch is applied against the exact commit it was created from. Fall back to - // querying the ADO refs API when the field is absent (backward compat with old - // NDJSON entries). - let base_commit: String = if let Some(ref recorded) = self.base_commit { + /// Resolve the base commit to push against: prefer the merge-base SHA recorded + /// at Stage 1, falling back to querying the ADO refs API for the target branch + /// HEAD when absent (backward compatibility with old NDJSON entries). + async fn resolve_base_commit( + &self, + client: &reqwest::Client, + ado: &AdoConnection<'_>, + repo_id: &str, + target_branch: &str, + ) -> anyhow::Result> { + if let Some(ref recorded) = self.base_commit { // Validate SHA format before trusting Stage 1 data if recorded.len() != 40 || !recorded.chars().all(|c| c.is_ascii_hexdigit()) { anyhow::bail!( @@ -1062,45 +1039,53 @@ impl Executor for CreatePrResult { ); } info!("Using recorded base_commit from Stage 1: {}", recorded); - recorded.clone() - } else { - debug!("No recorded base_commit — resolving from ADO refs API"); - let refs_response = client - .get(&refs_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to get target branch ref")?; - - if !refs_response.status().is_success() { - let status = refs_response.status(); - let body = refs_response.text().await.unwrap_or_default(); - warn!("Failed to get target branch ref: {} - {}", status, body); - return Ok(ExecutionResult::failure(format!( - "Failed to get target branch ref: {} - {}", - status, body - ))); - } - - let refs_data: serde_json::Value = refs_response.json().await?; - let resolved = refs_data["value"][0]["objectId"] - .as_str() - .context("Could not find target branch commit")?; - resolved.to_string() - }; - debug!("Base commit: {}", base_commit); + return Ok(Ok(recorded.clone())); + } - info!( - "Base commit for target branch '{}': {}", - target_branch, base_commit + debug!("No recorded base_commit — resolving from ADO refs API"); + let refs_url = format!( + "{}{}/_apis/git/repositories/{}/refs?filter=heads/{}&api-version=7.1", + ado.org_url, ado.project, repo_id, target_branch ); + debug!("Refs URL: {}", refs_url); - // Check if the source branch already exists (e.g. from a retry or previous run). - // Retry with new random suffixes up to 3 times. + let refs_response = client + .get(&refs_url) + .basic_auth("", Some(ado.token)) + .send() + .await + .context("Failed to get target branch ref")?; + + if !refs_response.status().is_success() { + let status = refs_response.status(); + let body = refs_response.text().await.unwrap_or_default(); + warn!("Failed to get target branch ref: {} - {}", status, body); + return Ok(Err(ExecutionResult::failure(format!( + "Failed to get target branch ref: {} - {}", + status, body + )))); + } + + let refs_data: serde_json::Value = refs_response.json().await?; + let resolved = refs_data["value"][0]["objectId"] + .as_str() + .context("Could not find target branch commit")?; + Ok(Ok(resolved.to_string())) + } + + /// Retry up to 3 times with a new random suffix if `source_branch` already + /// exists on the remote (e.g. from a retry or previous run). + async fn ensure_unique_source_branch( + client: &reqwest::Client, + ado: &AdoConnection<'_>, + repo_id: &str, + mut source_branch: String, + mut source_ref: String, + ) -> anyhow::Result<(String, String)> { for attempt in 0..3 { let check_ref_url = format!( "{}{}/_apis/git/repositories/{}/refs?filter=heads/{}&api-version=7.1", - org_url, project, repo_id, source_branch + ado.org_url, ado.project, repo_id, source_branch ); debug!( "Checking if source branch exists (attempt {}): {}", @@ -1110,7 +1095,7 @@ impl Executor for CreatePrResult { let check_ref_response = client .get(&check_ref_url) - .basic_auth("", Some(token)) + .basic_auth("", Some(ado.token)) .send() .await .context("Failed to check source branch existence")?; @@ -1132,6 +1117,230 @@ impl Executor for CreatePrResult { } break; } + Ok((source_branch, source_ref)) + } + + /// Build the final PR description: agent stats, skipped-symlink notice, then + /// the provenance footer (must stay last as the unambiguous provenance marker). + fn build_pr_description( + &self, + ctx: &ExecutionContext, + config: &CreatePrConfig, + skipped_symlinks: &[String], + ) -> String { + let description_with_stats = + crate::agent_stats::append_stats_to_body(&self.description, ctx, config.include_stats); + let description_with_symlink_notice = + append_skipped_symlink_notice(&description_with_stats, skipped_symlinks); + format!( + "{}{}", + description_with_symlink_notice, + generate_pr_footer() + ) + } + + /// Build the fallback result recorded when PR creation fails but the branch + /// was already pushed (only used when `fallback_record_branch` is enabled). + #[allow(clippy::too_many_arguments)] + fn build_fallback_failure_result( + &self, + source_branch: &str, + target_branch: &str, + status: reqwest::StatusCode, + body: &str, + ) -> ExecutionResult { + let fallback_description = format!( + "## Pull Request Creation Failed\n\n\ + A pull request could not be created automatically.\n\n\ + **Branch:** `{}`\n\ + **Target:** `{}`\n\ + **Repository:** `{}`\n\n\ + **Error:** {} - {}\n\n\ + ### Original PR Description\n\n\ + {}\n\n\ + ---\n\ + *To create the PR manually, merge branch `{}` into `{}`.*", + source_branch, + target_branch, + self.repository, + status, + sanitize_text(truncate_error_body(body, 500)), + sanitize_text(&self.description), + source_branch, + target_branch + ); + ExecutionResult::failure_with_data( + format!( + "Failed to create pull request: {} - {}. Branch '{}' was pushed — create the PR manually.", + status, + sanitize_text(truncate_error_body(body, 500)), + source_branch, + ), + serde_json::json!({ + "fallback": "branch-recorded", + "branch": source_branch, + "target_branch": target_branch, + "repository": self.repository, + "description": fallback_description + }), + ) + } +} + +#[async_trait::async_trait] +impl Executor for CreatePrResult { + fn dry_run_summary(&self) -> String { + format!("create PR: '{}' in repo '{}'", self.title, self.repository) + } + + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Creating PR: '{}' in repository '{}'", + self.title, self.repository + ); + debug!( + "create-pull-request: title='{}', repo='{}', branch='{}', patch='{}'", + self.title, self.repository, self.source_branch, self.patch_file + ); + debug!("PR description length: {} chars", self.description.len()); + debug!("Source branch: {}", self.source_branch); + debug!("Patch file: {}", self.patch_file); + + let config: CreatePrConfig = ctx.get_tool_config("create-pull-request")?; + debug!("Target branch from config: {}", config.target_branch); + debug!("Draft: {}", config.draft); + debug!("Auto-complete: {}", config.auto_complete); + debug!("Squash merge: {}", config.squash_merge); + + if config.draft && config.auto_complete { + warn!( + "auto-complete cannot be set on a draft PR; set draft: false to enable auto-complete" + ); + } + + let effective_title = match self.build_effective_title(&config) { + Ok(title) => title, + Err(result) => return Ok(result), + }; + + let (repository_alias, repo_id) = match self.resolve_repo_alias_and_id(ctx)? { + Ok(pair) => pair, + Err(result) => return Ok(result), + }; + + // Get ADO configuration + let org_url = ctx + .ado_org_url + .as_ref() + .context("Azure DevOps organization URL not configured")?; + let organization = ctx + .ado_organization + .as_ref() + .context("Azure DevOps organization name not configured")?; + let project = ctx + .ado_project + .as_ref() + .context("Azure DevOps project not configured")?; + let token = ctx + .access_token + .as_ref() + .context("Access token not configured")?; + debug!( + "ADO org: {}, organization: {}, project: {}", + org_url, organization, project + ); + let ado = AdoConnection { + org_url, + project, + token, + }; + + // Validate, read, and verify the patch file + let (patch_path, patch_content) = match self.read_and_verify_patch(ctx).await? { + Ok(pair) => pair, + Err(result) => return Ok(result), + }; + + let exclude_args: Vec = config + .excluded_files + .iter() + .map(|p| format!("--exclude={}", p)) + .collect(); + if !exclude_args.is_empty() { + debug!( + "Will apply {} excluded-files patterns via --exclude flags", + exclude_args.len() + ); + } + + if let Err(result) = Self::validate_patch_content(&patch_content, &config) { + return Ok(result); + } + + // Resolve the target (base) branch for THIS repo. In a multi-checkout + // ("meta repo") setup the target can differ per repo (explicit + // `target-branches` override, or inferred from the repo's checkout ref + // when `infer-target-from-checkout-ref` is set); falls back to the + // literal `target-branch`. Resolution is shared with the compiler's + // prepare-pr-base deepening, so the branch we PR into is the branch that + // was fetched/deepened. + let target_branch = config.resolve_target_branch(&repository_alias, &ctx.repo_refs); + let target_branch = target_branch.as_str(); + let mut source_branch = self.source_branch.clone(); + let mut source_ref = format!("refs/heads/{}", source_branch); + let target_ref = format!("refs/heads/{}", target_branch); + debug!("Source ref: {}, Target ref: {}", source_ref, target_ref); + + let repo_git_dir = + crate::safe_outputs::resolve_repository_checkout_dir(&repository_alias, ctx)?; + debug!("Git repository directory: {}", repo_git_dir.display()); + + let (_temp_dir, _worktree_guard, worktree_changes) = match Self::setup_worktree_and_collect_changes( + &repo_git_dir, + &source_branch, + target_branch, + &patch_path, + &exclude_args, + ) + .await? + { + Ok(triple) => triple, + Err(result) => return Ok(result), + }; + let WorktreeChanges { + changes, + skipped_symlinks, + } = worktree_changes; + + if changes.is_empty() { + return Ok(handle_no_changes(&config, &skipped_symlinks)); + } + + // Use ADO REST API to create branch and push changes + let client = reqwest::Client::new(); + + let base_commit = match self + .resolve_base_commit(&client, &ado, &repo_id, target_branch) + .await? + { + Ok(commit) => commit, + Err(result) => return Ok(result), + }; + debug!("Base commit: {}", base_commit); + info!( + "Base commit for target branch '{}': {}", + target_branch, base_commit + ); + + // Check if the source branch already exists (e.g. from a retry or previous run). + (source_branch, source_ref) = Self::ensure_unique_source_branch( + &client, + &ado, + &repo_id, + source_branch, + source_ref, + ) + .await?; // Push changes via ADO API (this creates the branch and commits in one call) info!("Pushing changes to ADO"); @@ -1158,21 +1367,7 @@ impl Executor for CreatePrResult { }; debug!("Changes pushed successfully"); - // Append agent stats then provenance footer to description. - // Footer goes last as the final unambiguous provenance marker. - // If any symlinks were skipped during file collection, surface that in the - // PR description so the agent/PR author can see that some intended file - // content was dropped for safety (otherwise the warning only appears in - // Stage 3 infrastructure logs). - let description_with_stats = - crate::agent_stats::append_stats_to_body(&self.description, ctx, config.include_stats); - let description_with_symlink_notice = - append_skipped_symlink_notice(&description_with_stats, &skipped_symlinks); - let description_final = format!( - "{}{}", - description_with_symlink_notice, - generate_pr_footer() - ); + let description_final = self.build_pr_description(ctx, &config, &skipped_symlinks); // Create the pull request via REST API info!("Creating pull request"); @@ -1234,40 +1429,11 @@ impl Executor for CreatePrResult { // Record branch info for manual recovery if enabled if config.fallback_record_branch { info!("PR creation failed, recording branch info for manual recovery"); - let fallback_description = format!( - "## Pull Request Creation Failed\n\n\ - A pull request could not be created automatically.\n\n\ - **Branch:** `{}`\n\ - **Target:** `{}`\n\ - **Repository:** `{}`\n\n\ - **Error:** {} - {}\n\n\ - ### Original PR Description\n\n\ - {}\n\n\ - ---\n\ - *To create the PR manually, merge branch `{}` into `{}`.*", - source_branch, + return Ok(self.build_fallback_failure_result( + &source_branch, target_branch, - self.repository, status, - sanitize_text(truncate_error_body(&body, 500)), - sanitize_text(&self.description), - source_branch, - target_branch - ); - return Ok(ExecutionResult::failure_with_data( - format!( - "Failed to create pull request: {} - {}. Branch '{}' was pushed — create the PR manually.", - status, - sanitize_text(truncate_error_body(&body, 500)), - source_branch, - ), - serde_json::json!({ - "fallback": "branch-recorded", - "branch": source_branch, - "target_branch": target_branch, - "repository": self.repository, - "description": fallback_description - }), + &body, )); }