diff --git a/Cargo.lock b/Cargo.lock index 5403886..1480372 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9,13 +9,16 @@ dependencies = [ "anyhow", "async-trait", "axum", + "base64", "chrono", "clap", "dirs", "env_logger", + "glob-match", "inquire", "log", "percent-encoding", + "rand 0.10.1", "regex-lite", "reqwest", "rmcp", @@ -219,6 +222,17 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" +[[package]] +name = "chacha20" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6f8d983286843e49675a4b7a2d174efe136dc93a18d69130dd18198a6c167601" +dependencies = [ + "cfg-if", + "cpufeatures", + "rand_core 0.10.0", +] + [[package]] name = "chrono" version = "0.4.43" @@ -304,6 +318,15 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" +[[package]] +name = "cpufeatures" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b2a41393f66f16b0823bb79094d54ac5fbd34ab292ddafb9a0456ac9f87d201" +dependencies = [ + "libc", +] + [[package]] name = "crossterm" version = "0.29.0" @@ -661,10 +684,17 @@ dependencies = [ "cfg-if", "libc", "r-efi 6.0.0", + "rand_core 0.10.0", "wasip2", "wasip3", ] +[[package]] +name = "glob-match" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9985c9503b412198aa4197559e9a318524ebc4519c229bfa05a535828c950b9d" + [[package]] name = "h2" version = "0.4.13" @@ -1364,7 +1394,18 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "44c5af06bb1b7d3216d91932aed5265164bf384dc89cd6ba05cf59a35f5f76ea" dependencies = [ "rand_chacha", - "rand_core", + "rand_core 0.9.5", +] + +[[package]] +name = "rand" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d2e8e8bcc7961af1fdac401278c6a831614941f6164ee3bf4ce61b7edb162207" +dependencies = [ + "chacha20", + "getrandom 0.4.2", + "rand_core 0.10.0", ] [[package]] @@ -1374,7 +1415,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d3022b5f1df60f26e1ffddd6c66e8aa15de382ae63b3a0c1bfc0e4d3e3f325cb" dependencies = [ "ppv-lite86", - "rand_core", + "rand_core 0.9.5", ] [[package]] @@ -1386,6 +1427,12 @@ dependencies = [ "getrandom 0.3.4", ] +[[package]] +name = "rand_core" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0c8d0fd677905edcbeedbf2edb6494d676f0e98d54d5cf9bda0b061cb8fb8aba" + [[package]] name = "redox_syscall" version = "0.5.18" @@ -1532,7 +1579,7 @@ dependencies = [ "http-body-util", "paste", "pin-project-lite", - "rand", + "rand 0.9.4", "rmcp-macros", "schemars", "serde", diff --git a/Cargo.toml b/Cargo.toml index 6d12966..4d16944 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,9 @@ terminal_size = "0.4.3" url = "2.5.8" axum = { version = "0.8.8", features = ["tokio"] } subtle = "2.6.1" +rand = "0.10.1" +base64 = "0.22.1" +glob-match = "0.2.1" [dev-dependencies] reqwest = { version = "0.12", features = ["blocking"] } diff --git a/src/execute.rs b/src/execute.rs index e4f2438..7583199 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -156,7 +156,15 @@ pub async fn execute_safe_outputs( match execute_safe_output(entry, ctx).await { Ok((tool_name, result)) => { - if result.success { + if result.is_warning() { + warn!( + "[{}/{}] {} warning: {}", + i + 1, + entries.len(), + tool_name, + result.message + ); + } else if result.success { info!( "[{}/{}] {} succeeded: {}", i + 1, @@ -173,12 +181,13 @@ pub async fn execute_safe_outputs( result.message ); } + let symbol = if result.is_warning() { "⚠" } else if result.success { "✓" } else { "✗" }; println!( "[{}/{}] {} - {} - {}", i + 1, entries.len(), tool_name, - if result.success { "✓" } else { "✗" }, + symbol, result.message ); results.push(result); @@ -193,11 +202,12 @@ pub async fn execute_safe_outputs( } // Log final summary - let success_count = results.iter().filter(|r| r.success).count(); - let failure_count = results.len() - success_count; + let success_count = results.iter().filter(|r| r.success && !r.is_warning()).count(); + let warning_count = results.iter().filter(|r| r.is_warning()).count(); + let failure_count = results.iter().filter(|r| !r.success).count(); info!( - "Stage 2 execution complete: {} succeeded, {} failed", - success_count, failure_count + "Stage 2 execution complete: {} succeeded, {} warnings, {} failed", + success_count, warning_count, failure_count ); Ok(results) diff --git a/src/main.rs b/src/main.rs index 9a766e3..aea8067 100644 --- a/src/main.rs +++ b/src/main.rs @@ -254,19 +254,25 @@ async fn main() -> Result<()> { } // Print summary - let success_count = results.iter().filter(|r| r.success).count(); - let failure_count = results.len() - success_count; + let success_count = results.iter().filter(|r| r.success && !r.is_warning()).count(); + let warning_count = results.iter().filter(|r| r.is_warning()).count(); + let failure_count = results.iter().filter(|r| !r.success).count(); println!("\n--- Execution Summary ---"); println!( - "Total: {} | Success: {} | Failed: {}", + "Total: {} | Success: {} | Warnings: {} | Failed: {}", results.len(), success_count, + warning_count, failure_count ); if failure_count > 0 { std::process::exit(1); + } else if warning_count > 0 { + // Exit code 2 signals "succeeded with issues" — the pipeline + // step wraps this to emit ##vso[task.complete result=SucceededWithIssues;] + std::process::exit(2); } } Commands::Proxy { allowed_hosts } => { diff --git a/src/mcp.rs b/src/mcp.rs index ac8ebcd..25e1fe5 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -52,15 +52,11 @@ fn slugify_title(title: &str) -> String { collapsed.chars().take(50).collect() } -/// Generate a short random suffix for branch uniqueness +/// Generate a short cryptographically random suffix for branch uniqueness fn generate_short_id() -> String { - use std::time::{SystemTime, UNIX_EPOCH}; - let timestamp = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap_or_default() - .as_millis(); - // Take last 6 hex digits of timestamp for short unique suffix - format!("{:06x}", (timestamp & 0xFFFFFF) as u32) + use rand::RngExt; + let value: u32 = rand::rng().random(); + format!("{:08x}", value) } // Re-export from tools module @@ -180,7 +176,7 @@ impl SafeOutputs { /// Generate a git diff patch from a specific directory /// If `repository` is Some, it's treated as a subdirectory of bounding_directory /// If `repository` is None or "self", use bounding_directory directly - async fn generate_patch(&self, repository: Option<&str>) -> Result { + async fn generate_patch(&self, repository: Option<&str>) -> Result<(String, String), McpError> { use tokio::process::Command; // Determine the git directory based on repository @@ -226,81 +222,172 @@ impl SafeOutputs { } }; - // Run git diff against the target branch to capture all changes - // Try origin/main first (remote tracking), then main, then HEAD as fallback - let diff_targets = ["origin/main", "main", "HEAD"]; - let mut last_error = String::new(); - let mut diff_output = None; + // Generate patch using git format-patch for proper commit metadata, + // rename detection, and binary file handling. + // + // Handles both committed and uncommitted changes: + // 1. Find the merge-base with the upstream branch (origin/HEAD or origin/main) + // 2. If there are uncommitted changes, stage and create a temporary commit + // 3. Generate format-patch from merge-base..HEAD to capture ALL changes + // 4. If a temporary commit was created, reset it (preserving working tree) - for target in &diff_targets { - let output = Command::new("git") - .args(["diff", target]) + // Find the merge-base to diff against + let merge_base = Self::find_merge_base(&git_dir).await?; + debug!("Using merge base: {}", merge_base); + + // Check if there are uncommitted changes (staged or unstaged) + let status_output = Command::new("git") + .args(["status", "--porcelain"]) + .current_dir(&git_dir) + .output() + .await + .map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git status: {}", e)) + })?; + + if !status_output.status.success() { + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "git status failed: {}", + String::from_utf8_lossy(&status_output.stderr) + ))); + } + + let has_uncommitted = !String::from_utf8_lossy(&status_output.stdout) + .trim() + .is_empty(); + let mut made_synthetic_commit = false; + + if has_uncommitted { + debug!("Uncommitted changes detected, creating synthetic commit"); + + // Stage all changes including untracked files + let add_output = Command::new("git") + .args(["add", "-A"]) .current_dir(&git_dir) .output() .await .map_err(|e| { - anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git diff: {}", e)) + anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git add -A: {}", e)) })?; - if output.status.success() { - diff_output = Some(output); - break; + if !add_output.status.success() { + // Reset index to clean state on failure + let _ = Command::new("git") + .args(["reset", "HEAD", "--quiet"]) + .current_dir(&git_dir) + .output() + .await; + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "git add -A failed: {}", + String::from_utf8_lossy(&add_output.stderr) + ))); } - last_error = String::from_utf8_lossy(&output.stderr).to_string(); - } - let mut patch = if let Some(output) = diff_output { - String::from_utf8_lossy(&output.stdout).to_string() + // Create a temporary commit with git identity flags to avoid config dependency + let commit_output = Command::new("git") + .args([ + "-c", "user.email=agent@ado-aw", + "-c", "user.name=ADO Agent", + "commit", "-m", "agent changes", "--allow-empty", "--no-verify", + ]) + .current_dir(&git_dir) + .output() + .await + .map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to create temporary commit: {}", + e + )) + })?; + + if !commit_output.status.success() { + // Reset staging on failure + let _ = Command::new("git") + .args(["reset", "HEAD", "--quiet"]) + .current_dir(&git_dir) + .output() + .await; + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to create temporary commit: {}", + String::from_utf8_lossy(&commit_output.stderr) + ))); + } + made_synthetic_commit = true; } else { - return Err(anyhow_to_mcp_error(anyhow::anyhow!( - "git diff failed against all targets (origin/main, main, HEAD): {}", - last_error - ))); - }; + debug!("No uncommitted changes — capturing committed changes only"); + } - // Also include untracked files that have been added - let status_output = Command::new("git") - .args(["status", "--porcelain"]) + // Generate format-patch from merge-base..HEAD to capture all changes + let format_patch_result = Command::new("git") + .args([ + "format-patch", + &format!("{}..HEAD", merge_base), + "--stdout", + "-M", + ]) .current_dir(&git_dir) .output() - .await - .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git status: {}", e)))?; + .await; + + // Always undo the temporary commit before propagating errors. + // Reset the synthetic commit, restoring changes to the working tree. + // `git reset --mixed HEAD~1` undoes the commit and resets the index + // to the parent tree, which leaves modified files as unstaged changes + // and previously-untracked files as untracked again. + if made_synthetic_commit { + // Capture the synthetic commit SHA for diagnostics + let head_sha = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(&git_dir) + .output() + .await + .ok() + .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string()) + .unwrap_or_else(|| "".to_string()); - if !status_output.status.success() { + // Reset the synthetic commit, restoring changes to working tree + let reset_output = Command::new("git") + .args(["reset", "HEAD~1", "--mixed", "--quiet"]) + .current_dir(&git_dir) + .output() + .await + .map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to run git reset (synthetic commit {} may remain): {}", + head_sha, + e + )) + })?; + + if !reset_output.status.success() { + warn!( + "WARNING: synthetic commit {} was not cleaned up; \ + run `git reset HEAD~1` to restore state", + head_sha + ); + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "git reset HEAD~1 failed (synthetic commit {} may remain): {}", + head_sha, + String::from_utf8_lossy(&reset_output.stderr) + ))); + } + } + + // Now check the format-patch result after cleanup + let format_patch_output = format_patch_result.map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!("Failed to run git format-patch: {}", e)) + })?; + + if !format_patch_output.status.success() { return Err(anyhow_to_mcp_error(anyhow::anyhow!( - "git status failed: {}", - String::from_utf8_lossy(&status_output.stderr) + "git format-patch failed: {}", + String::from_utf8_lossy(&format_patch_output.stderr) ))); } - let status = String::from_utf8_lossy(&status_output.stdout); - for line in status.lines() { - if line.starts_with("?? ") { - // Untracked file - generate a diff for it - let file_path = line[3..].trim(); - let file_full_path = git_dir.join(file_path); - - if file_full_path.is_file() { - if let Ok(content) = tokio::fs::read_to_string(&file_full_path).await { - patch.push_str(&format!("diff --git a/{} b/{}\n", file_path, file_path)); - patch.push_str("new file mode 100644\n"); - patch.push_str("--- /dev/null\n"); - patch.push_str(&format!("+++ b/{}\n", file_path)); - - let line_count = content.lines().count(); - patch.push_str(&format!("@@ -0,0 +1,{} @@\n", line_count)); - - for line in content.lines() { - patch.push('+'); - patch.push_str(line); - patch.push('\n'); - } - } - } - } - } + let patch = String::from_utf8_lossy(&format_patch_output.stdout).to_string(); - Ok(patch) + Ok((patch, merge_base)) } /// Generate a unique patch filename @@ -315,6 +402,131 @@ impl SafeOutputs { format!("pr-{}-{}.patch", safe_repo, timestamp) } + /// Find the merge-base commit to diff against. + /// + /// Tries (in order): + /// 1. Detect actual default branch via `git symbolic-ref refs/remotes/origin/HEAD` + /// 2. Common default branch names: `origin/main`, `origin/master` + /// 3. Root commit via `git rev-list --max-parents=0 HEAD` (handles single-commit repos) + async fn find_merge_base(git_dir: &std::path::Path) -> Result { + use tokio::process::Command; + + // First, try to discover the actual default branch from origin/HEAD + let symbolic_output = Command::new("git") + .args(["symbolic-ref", "refs/remotes/origin/HEAD"]) + .current_dir(git_dir) + .output() + .await + .ok(); + + let mut candidates: Vec = Vec::new(); + + if let Some(out) = symbolic_output.filter(|o| o.status.success()) { + let refname = String::from_utf8_lossy(&out.stdout).trim().to_string(); + // e.g. "refs/remotes/origin/main" → "origin/main" + if let Some(branch) = refname.strip_prefix("refs/remotes/") { + candidates.push(branch.to_string()); + } + } + + // Always try common defaults as fallbacks + for name in &["origin/main", "origin/master"] { + if !candidates.iter().any(|c| c == *name) { + candidates.push(name.to_string()); + } + } + + let mut found_remote_ref = false; + for remote_ref in &candidates { + let output = Command::new("git") + .args(["merge-base", "HEAD", remote_ref]) + .current_dir(git_dir) + .output() + .await + .ok(); + + if let Some(out) = output { + if out.status.success() { + let base = String::from_utf8_lossy(&out.stdout).trim().to_string(); + if !base.is_empty() { + return Ok(base); + } + } + // Check if the ref exists even though merge-base failed (diverged branches) + let ref_check = Command::new("git") + .args(["rev-parse", "--verify", remote_ref]) + .current_dir(git_dir) + .output() + .await + .ok(); + if ref_check.is_some_and(|o| o.status.success()) { + found_remote_ref = true; + } + } + } + + // Fallback: find the root commit. Only valid for single-commit repos where HEAD~1 + // doesn't exist. Verify the repo truly has only one commit total before + // using the root commit — most repos have exactly one root commit regardless + // of total commits (roots.len() == 1 would match virtually any repo). + let root_output = Command::new("git") + .args(["rev-list", "--max-parents=0", "HEAD"]) + .current_dir(git_dir) + .output() + .await + .ok(); + + if let Some(out) = root_output.filter(|o| o.status.success()) { + let output_str = String::from_utf8_lossy(&out.stdout).to_string(); + let roots: Vec<&str> = output_str.trim().lines().collect(); + + if roots.len() == 1 { + // Check total commit count to distinguish single-commit repos + // from normal repos that happen to have one root + let count_output = Command::new("git") + .args(["rev-list", "--count", "HEAD"]) + .current_dir(git_dir) + .output() + .await + .ok(); + + let commit_count = count_output + .filter(|o| o.status.success()) + .and_then(|o| { + String::from_utf8_lossy(&o.stdout) + .trim() + .parse::() + .ok() + }) + .unwrap_or(0); + + if commit_count <= 1 { + let sha = roots[0].to_string(); + warn!( + "Could not find merge-base with origin; using root commit {} \ + (single-commit repository)", + sha + ); + return Ok(sha); + } + } + } + + if found_remote_ref { + Err(anyhow_to_mcp_error(anyhow::anyhow!( + "Cannot determine diff base: remote tracking branch exists but shares no \ + common ancestry with HEAD (orphan or force-pushed branch). \ + Ensure HEAD is based on the target branch." + ))) + } else { + Err(anyhow_to_mcp_error(anyhow::anyhow!( + "Cannot determine diff base: no remote tracking branch found. \ + This can happen with shallow clones (fetchDepth: 1). \ + Ensure the pipeline fetches full history or push a tracking branch." + ))) + } + } + #[tool( description = "Log a transparency message when no significant actions are needed. Use this to confirm workflow completion and provide visibility when analysis is complete but no changes or outputs are required (e.g., 'No issues found', 'All checks passed'). This ensures the workflow produces human-visible output even when no other actions are taken." )] @@ -435,7 +647,9 @@ fields you want to update." #[tool( name = "create-pull-request", - description = "Create a new pull request to propose code changes. Use this after making file edits to submit them for review and merging. The PR will be created from the current branch with your committed changes. Use 'self' for the pipeline's own repository, or a repository alias from the checkout list." + description = "Create a new pull request to propose code changes. This tool captures all \ +changes in the repository (both committed and uncommitted) and creates a PR from them. \ +Use 'self' for the pipeline's own repository, or a repository alias from the checkout list." )] async fn create_pr( &self, @@ -453,7 +667,7 @@ fields you want to update." // Generate the patch from current git changes in the specified repository debug!("Generating patch for repository: {}", repository); - let patch_content = self.generate_patch(Some(repository)).await?; + let (patch_content, merge_base) = self.generate_patch(Some(repository)).await?; if patch_content.trim().is_empty() { warn!("No changes detected in repository '{}'", repository); @@ -492,6 +706,8 @@ fields you want to update." source_branch, patch_filename, repository.to_string(), + sanitized.labels, + Some(merge_base), ); // Write to safe outputs @@ -1064,7 +1280,7 @@ mod tests { #[test] fn test_generate_short_id_format() { let id = generate_short_id(); - assert_eq!(id.len(), 6); + assert_eq!(id.len(), 8); assert!(id.chars().all(|c| c.is_ascii_hexdigit())); } diff --git a/src/safeoutputs/create_pr.rs b/src/safeoutputs/create_pr.rs index 00020e4..54d7ef9 100644 --- a/src/safeoutputs/create_pr.rs +++ b/src/safeoutputs/create_pr.rs @@ -12,6 +12,88 @@ use anyhow::{Context, ensure}; /// Maximum allowed patch file size (5 MB) const MAX_PATCH_SIZE_BYTES: u64 = 5 * 1024 * 1024; +/// Default maximum files allowed in a single PR +const DEFAULT_MAX_FILES: usize = 100; + +/// Runtime manifest files that are protected by default (all lowercase for +/// case-insensitive comparison). +/// These are dependency/build files that could be modified to introduce supply chain attacks. +const PROTECTED_MANIFEST_BASENAMES: &[&str] = &[ + // npm / Node.js + "package.json", + "package-lock.json", + "yarn.lock", + "pnpm-lock.yaml", + "npm-shrinkwrap.json", + // Go + "go.mod", + "go.sum", + // Python + "requirements.txt", + "pipfile", + "pipfile.lock", + "pyproject.toml", + "setup.py", + "setup.cfg", + "poetry.lock", + // Ruby + "gemfile", + "gemfile.lock", + // Java / Kotlin / Gradle + "pom.xml", + "build.gradle", + "build.gradle.kts", + "settings.gradle", + "settings.gradle.kts", + "gradle.properties", + // .NET / C# + "directory.build.props", + "directory.build.targets", + "global.json", + // Rust + "cargo.toml", + "cargo.lock", + // Bun + "bun.lockb", + "bunfig.toml", + // Deno + "deno.json", + "deno.jsonc", + "deno.lock", + // Elixir + "mix.exs", + "mix.lock", + // Haskell + "stack.yaml", + "stack.yaml.lock", + // Python (uv) + "uv.lock", + // .NET (additional) + "nuget.config", + "directory.packages.props", + // Docker / container + "dockerfile", + "docker-compose.yml", + "docker-compose.yaml", + "compose.yml", + "compose.yaml", +]; + +/// Path prefixes that are protected by default. +/// Files under these paths are blocked unless protected-files is set to "allowed". +const PROTECTED_PATH_PREFIXES: &[&str] = &[ + ".github/", + ".pipelines/", + ".azure-pipelines/", + ".agents/", + ".claude/", + ".codex/", + ".copilot/", +]; + +/// Exact filenames (at repo root) that are protected by default. +const PROTECTED_EXACT_PATHS: &[&str] = &["CODEOWNERS", "docs/CODEOWNERS"]; + /// Resolve a reviewer identifier (email, display name, or ID) to an Azure DevOps identity ID. /// /// If the input is already a GUID, returns it directly. Otherwise, uses the Azure DevOps @@ -149,6 +231,11 @@ pub struct CreatePrParams { /// Required when multiple repositories are checked out. #[serde(default)] pub repository: Option, + + /// Labels to add to the PR for categorization. + /// These may be subject to an operator-configured allowlist. + #[serde(default)] + pub labels: Vec, } impl Validate for CreatePrParams { @@ -184,6 +271,21 @@ pub struct CreatePrResult { pub patch_file: String, /// Repository alias ("self" or alias from checkout list) pub repository: String, + /// Agent-provided labels (validated against allowed-labels at execution time) + #[serde(default)] + pub agent_labels: Vec, + /// Base commit SHA recorded at patch generation time (merge-base of HEAD and + /// the upstream branch). When present, Stage 2 uses this as the parent commit + /// for the ADO Push API, ensuring the patch applies cleanly even if the target + /// branch has advanced since the agent ran. Falls back to resolving the live + /// target branch HEAD via the ADO refs API when absent (backward compatibility). + /// + /// Note: this is the merge-base, not the target branch HEAD. The PR diff in ADO + /// compares file states and displays correctly regardless; however, the branch + /// history shows a parent older than current main. This is normal for topic + /// branches and is resolved when the PR is merged. + #[serde(skip_serializing_if = "Option::is_none")] + pub base_commit: Option, } impl crate::safeoutputs::ToolResult for CreatePrResult { @@ -195,6 +297,9 @@ impl Sanitize for CreatePrResult { fn sanitize_fields(&mut self) { self.title = sanitize_text(&self.title); self.description = sanitize_text(&self.description); + for label in &mut self.agent_labels { + *label = sanitize_text(label); + } } } @@ -206,6 +311,8 @@ impl CreatePrResult { source_branch: String, patch_file: String, repository: String, + agent_labels: Vec, + base_commit: Option, ) -> Self { Self { name: Self::NAME.to_string(), @@ -214,19 +321,53 @@ impl CreatePrResult { source_branch, patch_file, repository, + agent_labels, + base_commit, } } } +/// Behavior when the patch is empty or all files were excluded +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum IfNoChanges { + /// Succeed with a warning (default) + Warn, + /// Fail the pipeline step + Error, + /// Succeed silently + Ignore, +} + +/// File protection policy controlling whether manifest/CI files can be modified +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum ProtectedFiles { + /// Block modifications to protected files (default) + Blocked, + /// Allow modifications to all files + Allowed, +} + /// Configuration for the create_pr tool (specified in front matter) /// /// Example front matter: /// ```yaml /// safe-outputs: -/// create_pr: -/// auto_complete: true -/// delete_source_branch: true -/// squash_merge: true +/// create-pull-request: +/// target-branch: main +/// draft: true +/// auto-complete: true +/// delete-source-branch: true +/// squash-merge: true +/// title-prefix: "[Bot] " +/// if-no-changes: warn +/// max-files: 100 +/// protected-files: blocked +/// excluded-files: +/// - "*.lock" +/// allowed-labels: +/// - "automated" /// reviewers: /// - "user@example.com" /// labels: @@ -239,6 +380,10 @@ pub struct CreatePrConfig { #[serde(default = "default_target_branch", rename = "target-branch")] pub target_branch: String, + /// Whether to create the PR as a draft (default: true) + #[serde(default = "default_draft")] + pub draft: bool, + /// Whether to set auto-complete on the PR (default: false) #[serde(default, rename = "auto-complete")] pub auto_complete: bool, @@ -251,6 +396,32 @@ pub struct CreatePrConfig { #[serde(default = "default_true", rename = "squash-merge")] pub squash_merge: bool, + /// Prefix to prepend to all PR titles + #[serde(default, rename = "title-prefix")] + pub title_prefix: Option, + + /// Behavior when the patch is empty: "warn" (default), "error", "ignore" + #[serde(default = "default_if_no_changes", rename = "if-no-changes")] + pub if_no_changes: IfNoChanges, + + /// Maximum number of files allowed in a single PR (default: 100) + #[serde(default = "default_max_files", rename = "max-files")] + pub max_files: usize, + + /// File protection policy: "blocked" (default) or "allowed" + /// Controls whether manifest/CI files can be modified + #[serde(default = "default_protected_files", rename = "protected-files")] + pub protected_files: ProtectedFiles, + + /// Glob patterns for files to exclude from the patch + #[serde(default, rename = "excluded-files")] + pub excluded_files: Vec, + + /// Allowlist of labels the agent is permitted to use. + /// If empty, any labels are accepted. + #[serde(default, rename = "allowed-labels")] + pub allowed_labels: Vec, + /// Reviewers to add to the PR (email addresses or user IDs) #[serde(default)] pub reviewers: Vec, @@ -262,26 +433,56 @@ pub struct CreatePrConfig { /// Work item IDs to link to the PR #[serde(default, rename = "work-items")] pub work_items: Vec, + + /// Whether to record branch info in failure data when PR creation fails (default: true). + /// When enabled, the failure response includes the pushed branch name and target branch + /// so operators can manually create the PR. No work item is created automatically. + #[serde(default = "default_true", rename = "fallback-record-branch")] + pub fallback_record_branch: bool, } fn default_target_branch() -> String { "main".to_string() } +fn default_draft() -> bool { + true +} + fn default_true() -> bool { true } +fn default_if_no_changes() -> IfNoChanges { + IfNoChanges::Warn +} + +fn default_max_files() -> usize { + DEFAULT_MAX_FILES +} + +fn default_protected_files() -> ProtectedFiles { + ProtectedFiles::Blocked +} + impl Default for CreatePrConfig { fn default() -> Self { Self { target_branch: default_target_branch(), + draft: true, auto_complete: false, delete_source_branch: true, squash_merge: true, + title_prefix: None, + if_no_changes: default_if_no_changes(), + max_files: default_max_files(), + protected_files: default_protected_files(), + excluded_files: Vec::new(), + allowed_labels: Vec::new(), reviewers: Vec::new(), labels: Vec::new(), work_items: Vec::new(), + fallback_record_branch: true, } } } @@ -320,9 +521,32 @@ impl Executor for CreatePrResult { 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" + ); + } + + // Apply title prefix if configured + 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!( + "PR title too long after applying title-prefix ({} chars, max 400)", + title_char_count + ))); + } + // Validate repository against allowed list debug!( "Validating repository '{}' against allowed list", @@ -420,6 +644,24 @@ impl Executor for CreatePrResult { .context("Failed to read patch file")?; debug!("Patch content size: {} bytes", patch_content.len()); + // 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) { @@ -431,9 +673,53 @@ impl Executor for CreatePrResult { } debug!("Patch path validation passed"); + // Extract file paths from patch for validation. + // 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) + .into_iter() + .filter(|p| { + !config.excluded_files.iter().any(|pat| glob_match_simple(pat, p)) + }) + .collect(); + + // Security: File protection check + if config.protected_files != ProtectedFiles::Allowed { + let protected = find_protected_files(&patch_paths); + if !protected.is_empty() { + warn!( + "Patch modifies {} protected file(s): {:?}", + protected.len(), + protected + ); + return Ok(ExecutionResult::failure(format!( + "Patch modifies protected files (set protected-files: allowed to override): {}", + protected.join(", ") + ))); + } + } + + // 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); + if file_count > config.max_files { + warn!( + "Patch contains {} files, exceeding max of {}", + file_count, + config.max_files + ); + return Ok(ExecutionResult::failure(format!( + "Patch contains {} files, exceeding maximum of {} files per PR", + file_count, + config.max_files + ))); + } + // Use target branch from config let target_branch = &config.target_branch; - let source_ref = format!("refs/heads/{}", self.source_branch); + 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); @@ -519,10 +805,13 @@ impl Executor for CreatePrResult { worktree_path: worktree_path.clone(), }; - // Create and checkout the source branch in the worktree - debug!("Creating source branch: {}", self.source_branch); + // Create and checkout a local branch in the worktree for patch application. + // Note: this local branch name may differ from the final remote branch name + // if a collision is detected later — the ADO push is REST-only, so the local + // branch name is not used for the remote ref. + debug!("Creating source branch: {}", source_branch); let checkout_output = Command::new("git") - .args(["checkout", "-b", &self.source_branch]) + .args(["checkout", "-b", &source_branch]) .current_dir(&worktree_path) .output() .await @@ -540,79 +829,212 @@ impl Executor for CreatePrResult { } debug!("Source branch created"); - // Security: Validate patch with git apply --check first (dry run) - debug!("Running git apply --check (dry run)"); - let check_output = Command::new("git") - .args(["apply", "--check", &patch_path.to_string_lossy()]) + // Record the worktree HEAD before applying the patch so we can diff against + // it later. For multi-commit patches, git am creates N commits and diff-tree HEAD + // alone only shows the last commit's changes — we need base_sha..HEAD. + let base_sha_output = Command::new("git") + .args(["rev-parse", "HEAD"]) .current_dir(&worktree_path) .output() .await - .context("Failed to run git apply --check")?; + .context("Failed to get worktree HEAD SHA")?; + let base_sha = String::from_utf8_lossy(&base_sha_output.stdout).trim().to_string(); + debug!("Worktree base SHA before patch: {}", base_sha); + + // Apply the patch. Strategy depends on whether excluded-files are configured: + // - Without exclusions: prefer git am --3way (preserves commit metadata) + // with git apply --3way as fallback + // - With exclusions: use git apply --3way directly (git am does not support + // --exclude flags; git apply does) + let use_git_apply = !exclude_args.is_empty(); + let patch_committed; + + if use_git_apply { + debug!("Using git apply --3way (excluded-files configured)"); + let mut apply_args: Vec = vec!["apply".into(), "--3way".into()]; + apply_args.extend(exclude_args.iter().cloned()); + apply_args.push(patch_path.to_string_lossy().into_owned()); + let apply_output = Command::new("git") + .args(&apply_args) + .current_dir(&worktree_path) + .output() + .await + .context("Failed to run git apply --3way")?; - if !check_output.status.success() { - warn!( - "Patch dry-run failed: {}", - String::from_utf8_lossy(&check_output.stderr) - ); - return Ok(ExecutionResult::failure(format!( - "Patch validation failed (git apply --check): {}", - String::from_utf8_lossy(&check_output.stderr) - ))); - } - debug!("Patch dry-run passed"); + if !apply_output.status.success() { + let err_msg = format!( + "Patch could not be applied (conflicts): {}", + String::from_utf8_lossy(&apply_output.stderr) + ); + warn!("{}", err_msg); + return Ok(ExecutionResult::failure(err_msg)); + } + debug!("Patch applied with git apply --3way"); + + // Scan for unresolved conflict markers in working tree files. + // Use specific patterns: <<<<<<< and >>>>>>> always have a trailing space + // and marker (e.g., "<<<<<<< HEAD"), while ======= is on its own line. + // We require the trailing space on <<<<<<< and >>>>>>> to avoid false + // positives from reStructuredText/markdown heading underlines. + let conflict_check = Command::new("git") + .args(["grep", "-l", "-E", r"^(<<<<<<<\s|>>>>>>>\s)"]) + .current_dir(&worktree_path) + .output() + .await + .context("Failed to run git grep for conflict markers")?; + + if conflict_check.status.success() { + let conflicting_files = + String::from_utf8_lossy(&conflict_check.stdout).trim().to_string(); + let err_msg = format!( + "Patch applied with unresolved conflict markers in: {}", + conflicting_files + ); + warn!("{}", err_msg); + return Ok(ExecutionResult::failure(err_msg)); + } - // Apply the patch - debug!("Applying patch"); - let apply_output = Command::new("git") - .args(["apply", &patch_path.to_string_lossy()]) - .current_dir(&worktree_path) - .output() - .await - .context("Failed to run git apply")?; + patch_committed = false; + } else { + // No exclusions — use git am --3way for proper commit metadata preservation + debug!("Applying patch with git am --3way"); + let am_output = Command::new("git") + .args(["am", "--3way", &patch_path.to_string_lossy()]) + .current_dir(&worktree_path) + .output() + .await + .context("Failed to run git am")?; + + patch_committed = am_output.status.success(); + + if !patch_committed { + let stderr = String::from_utf8_lossy(&am_output.stderr); + debug!("git am --3way failed: {}", stderr); + + // Abort the failed am to leave worktree clean + let _ = Command::new("git") + .args(["am", "--abort"]) + .current_dir(&worktree_path) + .output() + .await; + + // Fallback: try git apply --3way + debug!("Falling back to git apply --3way"); + let apply_output = Command::new("git") + .args(["apply", "--3way", &patch_path.to_string_lossy()]) + .current_dir(&worktree_path) + .output() + .await + .context("Failed to run git apply --3way")?; - if !apply_output.status.success() { - warn!( - "Failed to apply patch: {}", - String::from_utf8_lossy(&apply_output.stderr) - ); - return Ok(ExecutionResult::failure(format!( - "Failed to apply patch: {}", - String::from_utf8_lossy(&apply_output.stderr) - ))); + if !apply_output.status.success() { + let err_msg = format!( + "Patch could not be applied (conflicts): {}", + String::from_utf8_lossy(&apply_output.stderr) + ); + warn!("{}", err_msg); + return Ok(ExecutionResult::failure(err_msg)); + } + debug!("Patch applied with git apply --3way fallback"); + + // Scan for unresolved conflict markers + let conflict_check = Command::new("git") + .args(["grep", "-l", "-E", r"^(<<<<<<<\s|>>>>>>>\s)"]) + .current_dir(&worktree_path) + .output() + .await + .context("Failed to run git grep for conflict markers")?; + + if conflict_check.status.success() { + let conflicting_files = + String::from_utf8_lossy(&conflict_check.stdout).trim().to_string(); + let err_msg = format!( + "Patch applied with unresolved conflict markers in: {}", + conflicting_files + ); + warn!("{}", err_msg); + return Ok(ExecutionResult::failure(err_msg)); + } + } else { + debug!("Patch applied successfully with git am"); + } } - debug!("Patch applied successfully"); - // Get list of changed files using git status + // Collect changed files. The method depends on how the patch was applied: + // - git am: changes are committed → use git diff-tree to compare base_sha..HEAD + // (covers all commits in multi-commit patches, not just the last one) + // - git apply: changes are in working tree → use git status --porcelain debug!("Getting list of changed files"); - let status_output = Command::new("git") - .args(["status", "--porcelain"]) - .current_dir(&worktree_path) - .output() - .await - .context("Failed to run git status")?; + let (status_str, use_diff_tree) = if patch_committed { + let diff_tree_output = Command::new("git") + .args(["diff-tree", "-r", "--name-status", &base_sha, "HEAD"]) + .current_dir(&worktree_path) + .output() + .await + .context("Failed to run git diff-tree")?; - if !status_output.status.success() { - warn!( - "Failed to get git status: {}", - String::from_utf8_lossy(&status_output.stderr) - ); - return Ok(ExecutionResult::failure(format!( - "Failed to get git status: {}", - String::from_utf8_lossy(&status_output.stderr) - ))); - } + if !diff_tree_output.status.success() { + warn!( + "Failed to get diff-tree: {}", + String::from_utf8_lossy(&diff_tree_output.stderr) + ); + return Ok(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(), true) + } else { + let status_output = Command::new("git") + .args(["status", "--porcelain"]) + .current_dir(&worktree_path) + .output() + .await + .context("Failed to run git status")?; - // Parse changed files and build ADO push payload - let status_str = String::from_utf8_lossy(&status_output.stdout); - debug!("Git status output:\n{}", status_str); - let changes = collect_changes_from_worktree(&worktree_path, &status_str).await?; + if !status_output.status.success() { + warn!( + "Failed to get git status: {}", + String::from_utf8_lossy(&status_output.stderr) + ); + return Ok(ExecutionResult::failure(format!( + "Failed to get git status: {}", + String::from_utf8_lossy(&status_output.stderr) + ))); + } + (String::from_utf8_lossy(&status_output.stdout).to_string(), false) + }; + + debug!("Change detection output:\n{}", status_str); + let changes = if use_diff_tree { + collect_changes_from_diff_tree(&worktree_path, &status_str).await? + } else { + collect_changes_from_worktree(&worktree_path, &status_str).await? + }; debug!("Collected {} file changes for push", changes.len()); if changes.is_empty() { - warn!("No changes detected after applying patch"); - return Ok(ExecutionResult::failure( - "No changes detected after applying patch".to_string(), - )); + // Handle no-changes based on config + match config.if_no_changes { + IfNoChanges::Error => { + warn!("No changes detected after applying patch (if-no-changes: error)"); + return Ok(ExecutionResult::failure( + "No changes detected after applying patch".to_string(), + )); + } + IfNoChanges::Ignore => { + info!("No changes detected after applying patch (if-no-changes: ignore)"); + return Ok(ExecutionResult::success( + "No changes detected — nothing to do".to_string(), + )); + } + IfNoChanges::Warn => { + warn!("No changes detected after applying patch (if-no-changes: warn)"); + return Ok(ExecutionResult::warning( + "No changes detected after applying patch".to_string(), + )); + } + } } // Use ADO REST API to create branch and push changes @@ -626,27 +1048,49 @@ impl Executor for CreatePrResult { ); debug!("Refs URL: {}", refs_url); - let refs_response = client - .get(&refs_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to get target branch ref")?; + // 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 { + // Validate SHA format before trusting Stage 1 data + if recorded.len() != 40 || !recorded.chars().all(|c| c.is_ascii_hexdigit()) { + anyhow::bail!( + "Invalid base_commit SHA from Stage 1 NDJSON: {:?}", + recorded + ); + } + 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 - ))); - } + 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 base_commit = refs_data["value"][0]["objectId"] - .as_str() - .context("Could not find target branch commit")?; + 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); info!( @@ -654,6 +1098,46 @@ impl Executor for CreatePrResult { target_branch, base_commit ); + // Check if the source branch already exists (e.g. from a retry or previous run). + // Retry with new random suffixes up to 3 times. + 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 + ); + debug!("Checking if source branch exists (attempt {}): {}", attempt + 1, check_ref_url); + + let check_ref_response = client + .get(&check_ref_url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to check source branch existence")?; + + if check_ref_response.status().is_success() { + let check_data: serde_json::Value = check_ref_response.json().await?; + let refs = check_data["value"].as_array(); + if refs.is_some_and(|r| !r.is_empty()) { + warn!( + "Branch '{}' already exists, generating new suffix (attempt {})", + source_branch, attempt + 1 + ); + use rand::RngExt; + let new_suffix: u32 = rand::rng().random(); + let new_hex = format!("{:08x}", new_suffix); + source_branch = if let Some(pos) = source_branch.rfind('-') { + format!("{}-{}", &source_branch[..pos], new_hex) + } else { + format!("{}-{}", source_branch, new_hex) + }; + source_ref = format!("refs/heads/{}", source_branch); + info!("Renamed source branch to '{}'", source_branch); + continue; + } + } + break; + } + // Push changes via ADO API (this creates the branch and commits in one call) info!("Pushing changes to ADO"); let push_url = format!( @@ -672,7 +1156,7 @@ impl Executor for CreatePrResult { "oldObjectId": "0000000000000000000000000000000000000000" }], "commits": [{ - "comment": self.title, + "comment": effective_title, "changes": changes, "parents": [base_commit] }] @@ -694,14 +1178,69 @@ impl Executor for CreatePrResult { if !push_response.status().is_success() { let status = push_response.status(); let body = push_response.text().await.unwrap_or_default(); - warn!("Failed to push changes: {} - {}", status, body); - return Ok(ExecutionResult::failure(format!( - "Failed to push changes: {} - {}", - status, body - ))); + + // Handle TOCTOU branch collision: if the push fails because the branch + // was created between our check and the push, retry with a new suffix + if status.as_u16() == 409 + || (status.as_u16() == 400 && body.contains("already exists")) + { + warn!( + "Push failed due to branch collision, retrying with new suffix: {}", + body + ); + use rand::RngExt; + let new_suffix: u32 = rand::rng().random(); + let new_hex = format!("{:08x}", new_suffix); + source_branch = if let Some(pos) = source_branch.rfind('-') { + format!("{}-{}", &source_branch[..pos], new_hex) + } else { + format!("{}-{}", source_branch, new_hex) + }; + source_ref = format!("refs/heads/{}", source_branch); + info!("Retrying push with branch '{}'", source_branch); + + let retry_body = serde_json::json!({ + "refUpdates": [{ + "name": source_ref, + "oldObjectId": "0000000000000000000000000000000000000000" + }], + "commits": [{ + "comment": effective_title, + "changes": changes, + "parents": [base_commit] + }] + }); + + let retry_response = client + .post(&push_url) + .basic_auth("", Some(token)) + .json(&retry_body) + .send() + .await + .context("Failed to push changes (retry)")?; + + if !retry_response.status().is_success() { + let retry_status = retry_response.status(); + let retry_body_text = retry_response.text().await.unwrap_or_default(); + warn!("Retry push also failed: {} - {}", retry_status, retry_body_text); + return Ok(ExecutionResult::failure(format!( + "Failed to push changes after retry: {} - {}", + retry_status, retry_body_text + ))); + } + } else { + warn!("Failed to push changes: {} - {}", status, body); + return Ok(ExecutionResult::failure(format!( + "Failed to push changes: {} - {}", + status, body + ))); + } } debug!("Changes pushed successfully"); + // Append provenance footer to description + let description_with_footer = format!("{}{}", self.description, generate_pr_footer()); + // Create the pull request via REST API info!("Creating pull request"); let pr_url = format!( @@ -713,8 +1252,9 @@ impl Executor for CreatePrResult { let mut pr_body = serde_json::json!({ "sourceRefName": source_ref, "targetRefName": target_ref, - "title": self.title, - "description": self.description, + "title": effective_title, + "description": description_with_footer, + "isDraft": config.draft, }); // Add work item links if configured @@ -729,12 +1269,45 @@ impl Executor for CreatePrResult { ); } - // Add labels if configured - if !config.labels.is_empty() { - debug!("Adding {} labels", config.labels.len()); + // Validate and add labels (merge operator labels + validated agent labels) + let mut all_labels = config.labels.clone(); + + // Validate agent-provided labels against allowed-labels + if !self.agent_labels.is_empty() { + if !config.allowed_labels.is_empty() { + let disallowed: Vec<_> = self + .agent_labels + .iter() + .filter(|l| { + !config + .allowed_labels + .iter() + .any(|al| al.eq_ignore_ascii_case(l)) + }) + .collect(); + if !disallowed.is_empty() { + warn!( + "Agent labels not in allowed-labels: {:?}", + disallowed + ); + return Ok(ExecutionResult::failure(format!( + "Agent-provided labels not in allowed-labels: {}", + disallowed.iter().map(|l| l.as_str()).collect::>().join(", ") + ))); + } + } + // Merge agent labels (case-insensitive dedup to match allowlist comparison) + for label in &self.agent_labels { + if !all_labels.iter().any(|l| l.eq_ignore_ascii_case(label)) { + all_labels.push(label.clone()); + } + } + } + + if !all_labels.is_empty() { + debug!("Adding {} labels", all_labels.len()); pr_body["labels"] = serde_json::json!( - config - .labels + all_labels .iter() .map(|l| serde_json::json!({"name": l})) .collect::>() @@ -753,6 +1326,41 @@ impl Executor for CreatePrResult { let status = pr_response.status(); let body = pr_response.text().await.unwrap_or_default(); warn!("Failed to create pull request: {} - {}", status, body); + + // 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, 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 + }), + )); + } + return Ok(ExecutionResult::failure(format!( "Failed to create pull request: {} - {}", status, body @@ -867,17 +1475,19 @@ impl Executor for CreatePrResult { } info!( - "PR #{} created successfully: {} -> {}", - pr_id, self.source_branch, target_branch + "PR #{} created successfully: {} -> {}{}", + pr_id, source_branch, target_branch, + if config.draft { " (draft)" } else { "" } ); Ok(ExecutionResult::success_with_data( - format!("Created pull request #{}: {}", pr_id, self.title), + format!("Created pull request #{}: {}", pr_id, effective_title), serde_json::json!({ "pull_request_id": pr_id, "url": pr_web_url, - "source_branch": self.source_branch, - "target_branch": target_branch + "source_branch": source_branch, + "target_branch": target_branch, + "draft": config.draft }), )) } @@ -924,42 +1534,20 @@ async fn collect_changes_from_worktree( // New/untracked files "??" | "A " | " A" | "AM" => { if full_path.is_file() { - let content = tokio::fs::read_to_string(&full_path) - .await - .with_context(|| format!("Failed to read new file: {}", file_path))?; - - changes.push(serde_json::json!({ - "changeType": "add", - "item": { - "path": format!("/{}", file_path) - }, - "newContent": { - "content": content, - "contentType": "rawtext" - } - })); + changes.push(read_file_change("add", file_path, &full_path).await?); } } // Modified files " M" | "M " | "MM" => { if full_path.is_file() { - let content = tokio::fs::read_to_string(&full_path) - .await - .with_context(|| format!("Failed to read modified file: {}", file_path))?; - - changes.push(serde_json::json!({ - "changeType": "edit", - "item": { - "path": format!("/{}", file_path) - }, - "newContent": { - "content": content, - "contentType": "rawtext" - } - })); + changes.push(read_file_change("edit", file_path, &full_path).await?); } } // Renamed files - format is "R old_path -> new_path" + // For "RM" (renamed + modified), we emit both a rename and an edit change. + // The ADO Pushes API processes changes sequentially within a single push, + // so the rename establishes the file at the new path, then the edit updates + // its content — this is the correct way to express rename-with-modification. "R " | " R" | "RM" => { if let Some((old_path, new_path)) = file_path.split_once(" -> ") { validate_single_path(old_path.trim())?; @@ -972,26 +1560,104 @@ async fn collect_changes_from_worktree( "path": format!("/{}", new_path.trim()) } })); + + // If status is "RM" (renamed + modified), also emit content + if status_code == "RM" { + let new_full_path = worktree_path.join(new_path.trim()); + if new_full_path.is_file() { + changes.push(read_file_change("edit", new_path.trim(), &new_full_path).await?); + } + } } } // Other statuses - try to handle as edit if file exists _ => { if full_path.is_file() { - let content = tokio::fs::read_to_string(&full_path) - .await - .with_context(|| format!("Failed to read file: {}", file_path))?; + changes.push(read_file_change("edit", file_path, &full_path).await?); + } + } + } + } - changes.push(serde_json::json!({ - "changeType": "edit", - "item": { - "path": format!("/{}", file_path) - }, - "newContent": { - "content": content, - "contentType": "rawtext" - } - })); + Ok(changes) +} + +/// Collect file changes from a git diff-tree --name-status output. +/// +/// Used when git am has already committed the changes. Parses the output format: +/// `M\tpath`, `A\tpath`, `D\tpath`, `R100\told_path\tnew_path` +async fn collect_changes_from_diff_tree( + worktree_path: &std::path::Path, + diff_tree_output: &str, +) -> anyhow::Result> { + let mut changes = Vec::new(); + + for line in diff_tree_output.lines() { + let line = line.trim(); + if line.is_empty() { + continue; + } + + let parts: Vec<&str> = line.split('\t').collect(); + if parts.len() < 2 { + continue; + } + + let status_code = parts[0]; + let file_path = parts[1]; + + // Validate path for security + validate_single_path(file_path)?; + + let full_path = worktree_path.join(file_path); + + if status_code == "D" { + // Deleted file + changes.push(serde_json::json!({ + "changeType": "delete", + "item": { + "path": format!("/{}", file_path) } + })); + } else if status_code == "A" { + // Added file + if full_path.is_file() { + changes.push(read_file_change("add", file_path, &full_path).await?); + } + } else if status_code.starts_with('R') && parts.len() >= 3 { + // Renamed file: R100\told_path\tnew_path + let old_path = file_path; + let new_path = parts[2]; + // old_path (= file_path) is already validated above + validate_single_path(new_path)?; + + // Emit the rename + changes.push(serde_json::json!({ + "changeType": "rename", + "sourceServerItem": format!("/{}", old_path), + "item": { + "path": format!("/{}", new_path) + } + })); + + // If the file was also modified (similarity < 100), emit an edit with content + let new_full_path = worktree_path.join(new_path); + if status_code != "R100" && new_full_path.is_file() { + changes.push(read_file_change("edit", new_path, &new_full_path).await?); + } + } else if status_code.starts_with('C') && parts.len() >= 3 { + // Copied file: C100\tsrc_path\tdest_path + let dest_path = parts[2]; + validate_single_path(dest_path)?; + + let dest_full_path = worktree_path.join(dest_path); + if dest_full_path.is_file() { + changes.push(read_file_change("add", dest_path, &dest_full_path).await?); + } + } else { + // Modified or other — read current content + if full_path.is_file() { + changes.push(read_file_change("edit", file_path, &full_path).await?); } } } @@ -999,7 +1665,45 @@ async fn collect_changes_from_worktree( Ok(changes) } -/// Validate that patch file paths don't contain dangerous patterns +/// Read a file and produce an ADO push change entry. +/// Handles both text (rawtext) and binary (base64encoded) content. +async fn read_file_change( + change_type: &str, + file_path: &str, + full_path: &std::path::Path, +) -> anyhow::Result { + let bytes = tokio::fs::read(full_path) + .await + .with_context(|| format!("Failed to read file: {}", file_path))?; + + // Try UTF-8 first; fall back to base64 for binary files + match String::from_utf8(bytes.clone()) { + Ok(content) => Ok(serde_json::json!({ + "changeType": change_type, + "item": { + "path": format!("/{}", file_path) + }, + "newContent": { + "content": content, + "contentType": "rawtext" + } + })), + Err(_) => { + use base64::Engine; + let encoded = base64::engine::general_purpose::STANDARD.encode(&bytes); + Ok(serde_json::json!({ + "changeType": change_type, + "item": { + "path": format!("/{}", file_path) + }, + "newContent": { + "content": encoded, + "contentType": "base64encoded" + } + })) + } + } +} /// /// Security checks: /// - No path traversal (..) @@ -1007,34 +1711,74 @@ async fn collect_changes_from_worktree( /// - No absolute paths /// - No null bytes fn validate_patch_paths(patch_content: &str) -> anyhow::Result<()> { + let mut in_diff = false; for line in patch_content.lines() { - // Check diff headers for file paths + // Only validate paths within diff blocks, not commit message bodies. + // format-patch output includes commit messages before each diff section. if line.starts_with("diff --git") { - // Extract paths from "diff --git a/path b/path" - let parts: Vec<&str> = line.split_whitespace().collect(); - for part in parts.iter().skip(2) { - let path = part.trim_start_matches("a/").trim_start_matches("b/"); - validate_single_path(path)?; - } - } else if line.starts_with("---") || line.starts_with("+++") { - // Extract path from "--- a/path" or "+++ b/path" - if let Some(path) = line.split_whitespace().nth(1) { - if path != "/dev/null" { - let clean_path = path.trim_start_matches("a/").trim_start_matches("b/"); - validate_single_path(clean_path)?; + in_diff = true; + // Extract paths using strip_prefix for correct handling of spaces + if let Some(rest) = line.strip_prefix("diff --git a/") { + // The b/ path starts after the last " b/" — but for simple validation, + // validate the a/ path (everything before " b/") and the b/ path + if let Some((a_path, b_path)) = rest.rsplit_once(" b/") { + validate_single_path(a_path.trim_matches('"'))?; + validate_single_path(b_path.trim_matches('"'))?; } } - } else if line.starts_with("rename from ") || line.starts_with("rename to ") { - let path = line.split_whitespace().last().unwrap_or(""); + continue; + } + // Reset on commit envelope boundaries + if line.starts_with("From ") && in_diff { + in_diff = false; + continue; + } + if !in_diff { + continue; + } + if let Some(rest) = line.strip_prefix("--- a/") { + let path = rest.trim().trim_matches('"'); + validate_single_path(path)?; + } else if let Some(rest) = line.strip_prefix("+++ b/") { + let path = rest.trim().trim_matches('"'); validate_single_path(path)?; - } else if line.starts_with("copy from ") || line.starts_with("copy to ") { - let path = line.split_whitespace().last().unwrap_or(""); + } else if line.starts_with("--- /dev/null") || line.starts_with("+++ /dev/null") { + // New or deleted files — no path to validate + } else if line.starts_with("rename from ") || line.starts_with("rename to ") + || line.starts_with("copy from ") || line.starts_with("copy to ") + { + let path = line.splitn(3, ' ').nth(2).unwrap_or("").trim_matches('"'); validate_single_path(path)?; } } Ok(()) } +/// Truncate an error response body to avoid embedding large or sensitive content. +fn truncate_error_body(body: &str, max_len: usize) -> &str { + match body.char_indices().nth(max_len) { + Some((idx, _)) => &body[..idx], + None => body, + } +} + +/// Simple glob matching for excluded-files patterns. +/// Supports `*` (match any sequence within a path segment) and leading `**/` +/// (match any directory prefix). Patterns without `/` are treated as basename +/// matches (e.g., `*.lock` matches `subdir/Cargo.lock`). +/// Match a file path against a glob pattern for excluded-files filtering. +/// Patterns without `/` are treated as basename matches (e.g., `*.lock` matches +/// `subdir/Cargo.lock`). Patterns with `**/` prefix match at any depth. +/// Uses the `glob-match` crate for correct glob semantics (`*` does not cross `/`). +fn glob_match_simple(pattern: &str, path: &str) -> bool { + if !pattern.contains('/') { + // Basename-only pattern: auto-prefix with **/ for any-depth matching + let full_pattern = format!("**/{}", pattern); + return glob_match::glob_match(&full_pattern, path); + } + glob_match::glob_match(pattern, path) +} + /// Validate a single file path for security issues fn validate_single_path(path: &str) -> anyhow::Result<()> { // Check for null bytes @@ -1079,6 +1823,120 @@ fn validate_single_path(path: &str) -> anyhow::Result<()> { Ok(()) } +/// Extract all file paths from a patch/diff content. +/// Returns deduplicated list of file paths referenced in the patch (both source and destination). +/// Uses `--- a/` and `+++ b/` lines for robust parsing (handles quoted paths +/// with spaces that break `diff --git` header parsing via split_whitespace). +fn extract_paths_from_patch(patch_content: &str) -> Vec { + let mut paths = Vec::new(); + let mut in_diff = false; + for line in patch_content.lines() { + // Only extract paths after the first diff --git header to avoid + // false positives from commit messages that quote patch fragments + if line.starts_with("diff --git") { + in_diff = true; + continue; + } + if !in_diff { + continue; + } + // A new commit envelope resets — skip until next diff --git + if line.starts_with("From ") { + in_diff = false; + continue; + } + if let Some(rest) = line.strip_prefix("--- a/") { + let path = rest.trim().trim_matches('"'); + if !path.is_empty() { + paths.push(path.to_string()); + } + } else if let Some(rest) = line.strip_prefix("+++ b/") { + let path = rest.trim().trim_matches('"'); + if !path.is_empty() { + paths.push(path.to_string()); + } + } else if line.starts_with("rename from ") || line.starts_with("rename to ") || + line.starts_with("copy from ") || line.starts_with("copy to ") { + // "rename from " / "rename to " + let path = line.splitn(3, ' ').nth(2).unwrap_or("").trim_matches('"'); + if !path.is_empty() { + paths.push(path.to_string()); + } + } + } + paths.sort(); + paths.dedup(); + paths +} + +/// Count the number of distinct files changed in a patch. +/// Reuses `extract_paths_from_patch` which correctly handles quoted paths, +/// renames, copies, and multi-commit deduplication. +fn count_patch_files(patch_content: &str) -> usize { + extract_paths_from_patch(patch_content).len() +} + +/// Check if any file paths in the patch are protected. +/// +/// Protected files include: +/// - Runtime manifests (package.json, go.mod, Cargo.toml, etc.) +/// - CI/pipeline configurations (.github/, .pipelines/, etc.) +/// - Agent instruction files (.agents/, .claude/, .codex/, .copilot/) +/// - Access control files (CODEOWNERS) +/// +/// Returns a list of protected file paths found, or empty vec if none. +fn find_protected_files(paths: &[String]) -> Vec { + let mut protected = Vec::new(); + for path in paths { + let lower_path = path.to_lowercase(); + + // Check basename against manifest list + let basename = path.rsplit('/').next().unwrap_or(path); + let lower_basename = basename.to_lowercase(); + for manifest in PROTECTED_MANIFEST_BASENAMES { + if lower_basename == *manifest { + protected.push(path.clone()); + break; + } + } + + // Check path prefixes (git diffs always use forward slashes) + for prefix in PROTECTED_PATH_PREFIXES { + if lower_path.starts_with(prefix) { + if !protected.contains(path) { + protected.push(path.clone()); + } + break; + } + } + + // Check exact paths + for exact in PROTECTED_EXACT_PATHS { + if lower_path == exact.to_lowercase() { + if !protected.contains(path) { + protected.push(path.clone()); + } + break; + } + } + } + protected +} + +/// Generate a provenance footer for the PR body +fn generate_pr_footer() -> String { + let timestamp = chrono::Utc::now().format("%Y-%m-%dT%H:%M:%SZ"); + format!( + "\n\n---\n\ + > 🤖 *This pull request was created by an automated agent.*\n\ + > Generated at: {}\n\ + > Compiler: ado-aw v{}", + timestamp, + env!("CARGO_PKG_VERSION") + ) +} + + #[cfg(test)] mod tests { use super::*; @@ -1089,6 +1947,7 @@ mod tests { title: "Fix bug in parser".to_string(), description: "This PR fixes a critical bug in the parser module.".to_string(), repository: None, + labels: vec![], }; assert!(params.validate().is_ok()); } @@ -1099,6 +1958,7 @@ mod tests { title: "Fix".to_string(), description: "This PR fixes a critical bug in the parser module.".to_string(), repository: None, + labels: vec![], }; assert!(params.validate().is_err()); } @@ -1109,6 +1969,7 @@ mod tests { title: "Fix bug in parser".to_string(), description: "Fix bug".to_string(), repository: None, + labels: vec![], }; assert!(params.validate().is_err()); } @@ -1117,9 +1978,16 @@ mod tests { fn test_config_default_target_branch() { let config = CreatePrConfig::default(); assert_eq!(config.target_branch, "main"); + assert!(config.draft); assert!(!config.auto_complete); assert!(config.delete_source_branch); assert!(config.squash_merge); + assert_eq!(config.if_no_changes, IfNoChanges::Warn); + assert_eq!(config.max_files, 100); + assert_eq!(config.protected_files, ProtectedFiles::Blocked); + assert!(config.excluded_files.is_empty()); + assert!(config.allowed_labels.is_empty()); + assert!(config.fallback_record_branch); } #[test] @@ -1181,6 +2049,28 @@ new file mode 100755 assert!(validate_patch_paths(patch).is_err()); } + #[test] + fn test_validate_patch_paths_rename_with_spaces() { + // "rename from some dir/../.git/config" must be rejected. + // Previously split_whitespace().last() would only see "dir/../.git/config" + // (or just "config"), missing the traversal in the full path. + let patch = "diff --git a/old b/new\n\ + rename from some dir/../.git/config\n\ + rename to new name\n"; + let result = validate_patch_paths(patch); + assert!(result.is_err(), "rename with spaces and traversal should be rejected"); + + // Also verify copy paths with spaces are validated correctly + let patch_copy = "diff --git a/old b/new\n\ + copy from some dir/../.git/config\n\ + copy to new name\n"; + let result_copy = validate_patch_paths(patch_copy); + assert!( + result_copy.is_err(), + "copy with spaces and traversal should be rejected" + ); + } + #[test] fn test_validate_single_path_valid() { assert!(validate_single_path("src/main.rs").is_ok()); @@ -1214,4 +2104,153 @@ new file mode 100755 fn test_validate_single_path_null_byte() { assert!(validate_single_path("file\0.txt").is_err()); } + + // ─── Protected files detection ────────────────────────────────────────── + + #[test] + fn test_find_protected_files_manifests() { + let paths = vec![ + "src/main.rs".to_string(), + "package.json".to_string(), + "go.mod".to_string(), + ]; + let protected = find_protected_files(&paths); + assert_eq!(protected, vec!["package.json", "go.mod"]); + } + + #[test] + fn test_find_protected_files_ci_configs() { + let paths = vec![ + "src/lib.rs".to_string(), + ".github/workflows/ci.yml".to_string(), + ".pipelines/build.yml".to_string(), + ]; + let protected = find_protected_files(&paths); + assert!(protected.contains(&".github/workflows/ci.yml".to_string())); + assert!(protected.contains(&".pipelines/build.yml".to_string())); + } + + #[test] + fn test_find_protected_files_agent_configs() { + let paths = vec![ + ".agents/config.md".to_string(), + ".claude/settings.json".to_string(), + ".copilot/instructions.md".to_string(), + ]; + let protected = find_protected_files(&paths); + assert_eq!(protected.len(), 3); + } + + #[test] + fn test_find_protected_files_codeowners() { + let paths = vec!["CODEOWNERS".to_string(), "README.md".to_string()]; + let protected = find_protected_files(&paths); + assert_eq!(protected, vec!["CODEOWNERS"]); + } + + #[test] + fn test_find_protected_files_none() { + let paths = vec![ + "src/main.rs".to_string(), + "docs/README.md".to_string(), + ]; + let protected = find_protected_files(&paths); + assert!(protected.is_empty()); + } + + #[test] + fn test_find_protected_files_nested_manifest() { + let paths = vec!["services/api/package.json".to_string()]; + let protected = find_protected_files(&paths); + assert_eq!(protected, vec!["services/api/package.json"]); + } + + #[test] + fn test_find_protected_files_mixed_case_manifests() { + let paths = vec![ + "Cargo.toml".to_string(), + "Cargo.lock".to_string(), + "Pipfile".to_string(), + "Gemfile".to_string(), + "Gemfile.lock".to_string(), + "Directory.Build.props".to_string(), + "sub/Directory.Build.targets".to_string(), + ]; + let protected = find_protected_files(&paths); + assert_eq!(protected.len(), 7); + } + + // ─── Glob matching ──────────────────────────────────────────────────── + + #[test] + fn test_glob_match_simple_basename_wildcard() { + assert!(glob_match_simple("*.lock", "Cargo.lock")); + assert!(glob_match_simple("*.lock", "subdir/Cargo.lock")); + assert!(glob_match_simple("*.lock", "a/b/c/yarn.lock")); + assert!(!glob_match_simple("*.lock", "Cargo.toml")); + } + + #[test] + fn test_glob_match_simple_basename_exact() { + assert!(glob_match_simple("package.json", "package.json")); + assert!(glob_match_simple("package.json", "subdir/package.json")); + assert!(!glob_match_simple("package.json", "other.json")); + } + + #[test] + fn test_glob_match_simple_double_star_prefix() { + assert!(glob_match_simple("**/Cargo.lock", "Cargo.lock")); + assert!(glob_match_simple("**/Cargo.lock", "subdir/Cargo.lock")); + assert!(glob_match_simple("**/Cargo.lock", "a/b/c/Cargo.lock")); + assert!(!glob_match_simple("**/Cargo.lock", "Cargo.toml")); + } + + #[test] + fn test_glob_match_simple_double_star_wildcard() { + assert!(glob_match_simple("**/*.json", "package.json")); + assert!(glob_match_simple("**/*.json", "subdir/package.json")); + assert!(!glob_match_simple("**/*.json", "package.lock")); + } + + #[test] + fn test_glob_match_simple_path_with_slash() { + assert!(glob_match_simple("src/*.rs", "src/main.rs")); + assert!(!glob_match_simple("src/*.rs", "tests/main.rs")); + // * should NOT cross directory boundaries + assert!(!glob_match_simple("src/*.rs", "src/nested/file.rs")); + } + + #[test] + fn test_glob_match_does_not_match_adjacent_protected() { + // *.lock should not match package.json + assert!(!glob_match_simple("*.lock", "package.json")); + assert!(!glob_match_simple("*.lock", "go.mod")); + } + + // ─── Extract paths from patch ─────────────────────────────────────────── + + #[test] + fn test_extract_paths_from_patch() { + let patch = "diff --git a/src/main.rs b/src/main.rs\nindex abc..def\n--- a/src/main.rs\n+++ b/src/main.rs\ndiff --git a/README.md b/README.md\n--- a/README.md\n+++ b/README.md\n"; + let paths = extract_paths_from_patch(patch); + assert!(paths.contains(&"src/main.rs".to_string())); + assert!(paths.contains(&"README.md".to_string())); + } + + #[test] + fn test_extract_paths_from_patch_with_spaces() { + let patch = "diff --git \"a/path with spaces/file.txt\" \"b/path with spaces/file.txt\"\n--- a/path with spaces/file.txt\n+++ b/path with spaces/file.txt\n"; + let paths = extract_paths_from_patch(patch); + assert!(paths.contains(&"path with spaces/file.txt".to_string())); + } + + #[test] + fn test_extract_paths_new_file() { + let patch = "diff --git a/new.txt b/new.txt\nnew file mode 100644\n--- /dev/null\n+++ b/new.txt\n"; + let paths = extract_paths_from_patch(patch); + assert!(paths.contains(&"new.txt".to_string())); + // /dev/null from --- should not be included (no a/ prefix) + assert!(!paths.contains(&"/dev/null".to_string())); + } + } diff --git a/src/safeoutputs/result.rs b/src/safeoutputs/result.rs index 3173586..a7dcfb8 100644 --- a/src/safeoutputs/result.rs +++ b/src/safeoutputs/result.rs @@ -111,6 +111,10 @@ impl Default for ExecutionContext { pub struct ExecutionResult { /// Whether the execution succeeded pub success: bool, + /// Whether this is a warning (succeeded with issues). + /// Invariant: warning == true implies success == true. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + warning: bool, /// Human-readable message describing the outcome pub message: String, /// Optional additional data (e.g., work item ID) @@ -119,10 +123,15 @@ pub struct ExecutionResult { } impl ExecutionResult { + /// Whether this is a warning (succeeded with issues) + pub fn is_warning(&self) -> bool { + self.warning + } /// Create a successful execution result pub fn success(message: impl Into) -> Self { Self { success: true, + warning: false, message: message.into(), data: None, } @@ -132,19 +141,43 @@ impl ExecutionResult { pub fn success_with_data(message: impl Into, data: serde_json::Value) -> Self { Self { success: true, + warning: false, message: message.into(), data: Some(data), } } + /// Create a warning result (succeeded with issues). + /// The action completed but something noteworthy occurred. + /// Exit code 2 signals the pipeline to set SucceededWithIssues. + pub fn warning(message: impl Into) -> Self { + Self { + success: true, + warning: true, + message: message.into(), + data: None, + } + } + /// Create a failed execution result pub fn failure(message: impl Into) -> Self { Self { success: false, + warning: false, message: message.into(), data: None, } } + + /// Create a failed execution result with additional data + pub fn failure_with_data(message: impl Into, data: serde_json::Value) -> Self { + Self { + success: false, + warning: false, + message: message.into(), + data: Some(data), + } + } } /// Trait for executing tool results in Stage 2 of the pipeline. diff --git a/templates/base.yml b/templates/base.yml index 203c484..99f2d48 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -641,8 +641,15 @@ jobs: mkdir -p "$(Agent.TempDirectory)/staging" displayName: "Prepare output directory" - - bash: ado-aw execute --source "{{ source_path }}" --safe-output-dir "$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)" --output-dir "$(Agent.TempDirectory)/staging" - displayName: Process safe outputs + - bash: | + ado-aw execute --source "{{ source_path }}" --safe-output-dir "$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)" --output-dir "$(Agent.TempDirectory)/staging" + EXIT_CODE=$? + if [ $EXIT_CODE -eq 2 ]; then + echo "##vso[task.complete result=SucceededWithIssues;]Executor completed with warnings" + exit 0 + fi + exit $EXIT_CODE + displayName: Execute safe outputs (Stage 2) workingDirectory: {{ working_directory }} env: {{ executor_ado_env }}