From aa26736211facdec5e38ba89877131d3c8e4026e Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 8 Apr 2026 13:00:51 +0100 Subject: [PATCH 01/15] feat: add 8 new safe output tools Implements the remaining safe output features from GitHub issues #72-#79: - add-pr-comment (#72): Create comment threads on Azure DevOps pull requests with support for general and file-specific inline comments - link-work-items (#73): Create relationships between work items (parent/child, related, predecessor/successor, duplicate) - queue-build (#74): Trigger Azure DevOps pipeline/build runs with optional branch and template parameters - create-git-tag (#75): Create annotated git tags on commits in ADO repositories - add-build-tag (#76): Add tags to Azure DevOps builds for classification - create-branch (#77): Create new branches without immediately creating a PR - update-pr (#78): Update PR metadata (reviewers, labels, auto-complete, vote, description) - upload-attachment (#79): Upload file attachments to work items Each tool includes: - Params struct with validation - Config struct for front matter configuration - Executor with full ADO REST API implementation - Sanitize impl for security - Comprehensive unit tests All 8 tools are registered in the MCP server, Stage 2 executor dispatch, budget system, and write-permissions validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 8 + src/execute.rs | 83 ++- src/mcp.rs | 201 ++++++- src/tools/add_build_tag.rs | 292 ++++++++++ src/tools/add_pr_comment.rs | 480 ++++++++++++++++ src/tools/create_branch.rs | 489 +++++++++++++++++ src/tools/create_git_tag.rs | 467 ++++++++++++++++ src/tools/link_work_items.rs | 420 ++++++++++++++ src/tools/mod.rs | 20 +- src/tools/queue_build.rs | 479 ++++++++++++++++ src/tools/update_pr.rs | 970 +++++++++++++++++++++++++++++++++ src/tools/upload_attachment.rs | 459 ++++++++++++++++ 12 files changed, 4363 insertions(+), 5 deletions(-) create mode 100644 src/tools/add_build_tag.rs create mode 100644 src/tools/add_pr_comment.rs create mode 100644 src/tools/create_branch.rs create mode 100644 src/tools/create_git_tag.rs create mode 100644 src/tools/link_work_items.rs create mode 100644 src/tools/queue_build.rs create mode 100644 src/tools/update_pr.rs create mode 100644 src/tools/upload_attachment.rs diff --git a/src/compile/common.rs b/src/compile/common.rs index 51432e7c..a2bc67fa 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -694,6 +694,14 @@ const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = &[ "update-work-item", "create-wiki-page", "update-wiki-page", + "add-pr-comment", + "link-work-items", + "queue-build", + "create-git-tag", + "add-build-tag", + "create-branch", + "update-pr", + "upload-attachment", ]; /// Validate that write-requiring safe-outputs have a write service connection configured. diff --git a/src/execute.rs b/src/execute.rs index b59c5301..4dece425 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -11,9 +11,11 @@ use std::path::Path; use crate::ndjson::{self, SAFE_OUTPUT_FILENAME}; use crate::tools::{ + AddBuildTagResult, AddPrCommentResult, CreateBranchResult, CreateGitTagResult, CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, CommentOnWorkItemResult, - ExecutionContext, ExecutionResult, Executor, ToolResult, - UpdateWikiPageResult, UpdateWorkItemResult, + ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, QueueBuildResult, + ToolResult, UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, + UploadAttachmentResult, }; // Re-export memory types for use by main.rs @@ -113,6 +115,14 @@ pub async fn execute_safe_outputs( CommentOnWorkItemResult, CreateWikiPageResult, UpdateWikiPageResult, + AddPrCommentResult, + LinkWorkItemsResult, + QueueBuildResult, + CreateGitTagResult, + AddBuildTagResult, + CreateBranchResult, + UpdatePrResult, + UploadAttachmentResult, ); let mut results = Vec::new(); @@ -264,6 +274,75 @@ pub async fn execute_safe_output( ); output.execute_sanitized(ctx).await? } + "add-pr-comment" => { + debug!("Parsing add-pr-comment payload"); + let mut output: AddPrCommentResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse add-pr-comment: {}", e))?; + debug!( + "add-pr-comment: pr_id={}, content length={}", + output.pull_request_id, + output.content.len() + ); + output.execute_sanitized(ctx).await? + } + "link-work-items" => { + debug!("Parsing link-work-items payload"); + let mut output: LinkWorkItemsResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse link-work-items: {}", e))?; + debug!( + "link-work-items: source={}, target={}, type={}", + output.source_id, output.target_id, output.link_type + ); + output.execute_sanitized(ctx).await? + } + "queue-build" => { + debug!("Parsing queue-build payload"); + let mut output: QueueBuildResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse queue-build: {}", e))?; + debug!("queue-build: pipeline_id={}", output.pipeline_id); + output.execute_sanitized(ctx).await? + } + "create-git-tag" => { + debug!("Parsing create-git-tag payload"); + let mut output: CreateGitTagResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse create-git-tag: {}", e))?; + debug!("create-git-tag: tag_name='{}'", output.tag_name); + output.execute_sanitized(ctx).await? + } + "add-build-tag" => { + debug!("Parsing add-build-tag payload"); + let mut output: AddBuildTagResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse add-build-tag: {}", e))?; + debug!("add-build-tag: build_id={}, tag='{}'", output.build_id, output.tag); + output.execute_sanitized(ctx).await? + } + "create-branch" => { + debug!("Parsing create-branch payload"); + let mut output: CreateBranchResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse create-branch: {}", e))?; + debug!("create-branch: branch_name='{}'", output.branch_name); + output.execute_sanitized(ctx).await? + } + "update-pr" => { + debug!("Parsing update-pr payload"); + let mut output: UpdatePrResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse update-pr: {}", e))?; + debug!( + "update-pr: pr_id={}, operation='{}'", + output.pull_request_id, output.operation + ); + output.execute_sanitized(ctx).await? + } + "upload-attachment" => { + debug!("Parsing upload-attachment payload"); + let mut output: UploadAttachmentResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse upload-attachment: {}", e))?; + debug!( + "upload-attachment: work_item_id={}, file_path='{}'", + output.work_item_id, output.file_path + ); + output.execute_sanitized(ctx).await? + } "noop" => { debug!("Skipping noop entry"); ExecutionResult::success("Skipped informational output: noop") diff --git a/src/mcp.rs b/src/mcp.rs index 0d1c2b37..2f05d51b 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -11,12 +11,20 @@ use std::path::PathBuf; use crate::ndjson::{self, SAFE_OUTPUT_FILENAME}; use crate::sanitize::{Sanitize, sanitize as sanitize_text}; use crate::tools::{ + AddBuildTagParams, AddBuildTagResult, + AddPrCommentParams, AddPrCommentResult, CommentOnWorkItemParams, CommentOnWorkItemResult, + CreateBranchParams, CreateBranchResult, + CreateGitTagParams, CreateGitTagResult, CreatePrParams, CreatePrResult, CreateWikiPageParams, CreateWikiPageResult, CreateWorkItemParams, CreateWorkItemResult, + LinkWorkItemsParams, LinkWorkItemsResult, UpdateWikiPageParams, UpdateWikiPageResult, MissingDataParams, MissingDataResult, - MissingToolParams, MissingToolResult, NoopParams, NoopResult, ToolResult, + MissingToolParams, MissingToolResult, NoopParams, NoopResult, QueueBuildParams, + QueueBuildResult, ToolResult, + UpdatePrParams, UpdatePrResult, UpdateWorkItemParams, UpdateWorkItemResult, + UploadAttachmentParams, UploadAttachmentResult, anyhow_to_mcp_error, }; @@ -533,6 +541,197 @@ structured output that should be visible in the project wiki." result.path ))])) } + + #[tool( + name = "add-pr-comment", + description = "Add a comment thread to an Azure DevOps pull request. Supports both \ +general comments and file-specific inline comments with optional line positioning. \ +The comment will be posted during safe output processing." + )] + async fn add_pr_comment( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: add-pr-comment - PR #{}", + params.0.pull_request_id + ); + debug!("Content length: {} chars", params.0.content.len()); + let mut sanitized = params.0; + sanitized.content = sanitize_text(&sanitized.content); + let result: AddPrCommentResult = sanitized.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + info!("PR comment queued for PR #{}", result.pull_request_id); + Ok(CallToolResult::success(vec![Content::text(format!( + "Comment queued for PR #{}. The comment will be posted during safe output processing.", + result.pull_request_id + ))])) + } + + #[tool( + name = "link-work-items", + description = "Create a relationship link between two Azure DevOps work items. \ +Supported link types: parent, child, related, predecessor, successor, duplicate, duplicate-of. \ +The link will be created during safe output processing." + )] + async fn link_work_items( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: link-work-items - {} -> {} ({})", + params.0.source_id, params.0.target_id, params.0.link_type + ); + let mut sanitized = params.0; + sanitized.comment = sanitized.comment.map(|c| sanitize_text(&c)); + let result: LinkWorkItemsResult = sanitized.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "Link queued: work item #{} → #{} ({}). The link will be created during safe output processing.", + result.source_id, result.target_id, result.link_type + ))])) + } + + #[tool( + name = "queue-build", + description = "Trigger an Azure DevOps pipeline/build run. The pipeline must be in the \ +allowed-pipelines list configured in the pipeline definition. Optionally specify a branch \ +and template parameters." + )] + async fn queue_build( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: queue-build - pipeline {}", + params.0.pipeline_id + ); + let mut sanitized = params.0; + sanitized.reason = sanitized.reason.map(|r| sanitize_text(&r)); + let result: QueueBuildResult = sanitized.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "Build queued for pipeline {}. The build will be triggered during safe output processing.", + result.pipeline_id + ))])) + } + + #[tool( + name = "create-git-tag", + description = "Create an annotated git tag on a commit in an Azure DevOps repository. \ +The tag will be created during safe output processing." + )] + async fn create_git_tag( + &self, + params: Parameters, + ) -> Result { + info!("Tool called: create-git-tag - '{}'", params.0.tag_name); + let mut sanitized = params.0; + sanitized.message = sanitized.message.map(|m| sanitize_text(&m)); + let result: CreateGitTagResult = sanitized.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "Git tag '{}' queued. The tag will be created during safe output processing.", + result.tag_name + ))])) + } + + #[tool( + name = "add-build-tag", + description = "Add a tag to an Azure DevOps build for classification and filtering. \ +The tag will be added during safe output processing." + )] + async fn add_build_tag( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: add-build-tag - build {} tag '{}'", + params.0.build_id, params.0.tag + ); + let result: AddBuildTagResult = params.0.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "Build tag '{}' queued for build #{}. The tag will be added during safe output processing.", + result.tag, result.build_id + ))])) + } + + #[tool( + name = "create-branch", + description = "Create a new branch in an Azure DevOps repository without creating a \ +pull request. The branch will be created during safe output processing." + )] + async fn create_branch( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: create-branch - '{}'", + params.0.branch_name + ); + let result: CreateBranchResult = params.0.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "Branch '{}' queued for creation. The branch will be created during safe output processing.", + result.branch_name + ))])) + } + + #[tool( + name = "update-pr", + description = "Update pull request metadata in Azure DevOps. Supports operations: \ +add-reviewers, add-labels, set-auto-complete, vote, update-description. \ +Changes will be applied during safe output processing." + )] + async fn update_pr( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: update-pr - PR #{} operation '{}'", + params.0.pull_request_id, params.0.operation + ); + let mut sanitized = params.0; + sanitized.description = sanitized.description.map(|d| sanitize_text(&d)); + let result: UpdatePrResult = sanitized.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "PR #{} '{}' operation queued. Changes will be applied during safe output processing.", + result.pull_request_id, result.operation + ))])) + } + + #[tool( + name = "upload-attachment", + description = "Upload a file attachment to an Azure DevOps work item. The file will be \ +uploaded and linked during safe output processing. File size and type restrictions may apply." + )] + async fn upload_attachment( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: upload-attachment - work item #{} file '{}'", + params.0.work_item_id, params.0.file_path + ); + let mut sanitized = params.0; + sanitized.comment = sanitized.comment.map(|c| sanitize_text(&c)); + let result: UploadAttachmentResult = sanitized.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "Attachment '{}' queued for work item #{}. The file will be uploaded during safe output processing.", + result.file_path, result.work_item_id + ))])) + } } // Implement the server handler diff --git a/src/tools/add_build_tag.rs b/src/tools/add_build_tag.rs new file mode 100644 index 00000000..8ccabf65 --- /dev/null +++ b/src/tools/add_build_tag.rs @@ -0,0 +1,292 @@ +//! Add build tag safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +// ── Stage 1: Params (agent-provided) ────────────────────────────────────── + +/// Parameters for adding a tag to an Azure DevOps build +#[derive(Deserialize, JsonSchema)] +pub struct AddBuildTagParams { + /// Build ID to tag (must be positive) + pub build_id: i32, + /// Tag string (1-100 chars, alphanumeric + dashes only) + pub tag: String, +} + +impl Validate for AddBuildTagParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!(self.build_id > 0, "build_id must be positive"); + ensure!(!self.tag.is_empty(), "tag must not be empty"); + ensure!( + self.tag.len() <= 100, + "tag must be at most 100 characters" + ); + ensure!( + self.tag + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'-'), + "tag must contain only alphanumeric characters and dashes" + ); + Ok(()) + } +} + +// ── Stage 1: Result (generated by macro) ────────────────────────────────── + +tool_result! { + name = "add-build-tag", + params = AddBuildTagParams, + /// Result of adding a tag to a build + pub struct AddBuildTagResult { + build_id: i32, + tag: String, + } +} + +// ── Stage 2: Sanitization ───────────────────────────────────────────────── + +impl Sanitize for AddBuildTagResult { + fn sanitize_fields(&mut self) { + self.tag = sanitize_text(&self.tag); + } +} + +// ── Stage 2: Configuration (from front matter) ──────────────────────────── + +/// Configuration for the add-build-tag tool +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// add-build-tag: +/// allowed-tags: +/// - verified +/// - release-candidate +/// tag-prefix: "agent-" +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct AddBuildTagConfig { + /// Restrict which tags can be applied. Empty means any tag is allowed. + /// Supports simple wildcard patterns: entries ending with `*` match by prefix. + #[serde(default, rename = "allowed-tags")] + pub allowed_tags: Vec, + + /// Prefix prepended to all tags before applying + #[serde(default, rename = "tag-prefix")] + pub tag_prefix: Option, +} + +impl Default for AddBuildTagConfig { + fn default() -> Self { + Self { + allowed_tags: Vec::new(), + tag_prefix: None, + } + } +} + +// ── Stage 2: Execution ──────────────────────────────────────────────────── + +#[async_trait::async_trait] +impl Executor for AddBuildTagResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!("Adding tag '{}' to build #{}", self.tag, self.build_id); + + // 1. Extract required context + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available")?; + debug!("ADO org: {}, project: {}", org_url, project); + + // 2. Get tool-specific configuration + let config: AddBuildTagConfig = ctx.get_tool_config("add-build-tag"); + debug!("Config: {:?}", config); + + // 3. Apply tag prefix if configured + let final_tag = match &config.tag_prefix { + Some(prefix) => format!("{}{}", prefix, self.tag), + None => self.tag.clone(), + }; + debug!("Final tag (after prefix): {}", final_tag); + + // 4. Validate against allowed tags (if non-empty) + if !config.allowed_tags.is_empty() { + let allowed = config.allowed_tags.iter().any(|pattern| { + if let Some(prefix) = pattern.strip_suffix('*') { + final_tag.starts_with(prefix) + } else { + *pattern == final_tag + } + }); + if !allowed { + return Ok(ExecutionResult::failure(format!( + "Tag '{}' is not in the allowed tags list", + final_tag + ))); + } + } + + // 5. Build the API URL + let url = format!( + "{}/{}/_apis/build/builds/{}/tags/{}?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + self.build_id, + utf8_percent_encode(&final_tag, PATH_SEGMENT), + ); + debug!("API URL: {}", url); + + // 6. PUT request (empty body) + let client = reqwest::Client::new(); + info!("Sending PUT to add tag '{}' to build #{}", final_tag, self.build_id); + let response = client + .put(&url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to send request")?; + + if response.status().is_success() { + info!( + "Tag '{}' added to build #{}", + final_tag, self.build_id + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Added tag '{}' to build #{}", + final_tag, self.build_id + ), + serde_json::json!({ + "build_id": self.build_id, + "tag": final_tag, + "project": project, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + Ok(ExecutionResult::failure(format!( + "Failed to add tag (HTTP {}): {}", + status, error_body + ))) + } + } +} + +// ── Tests ───────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(AddBuildTagResult::NAME, "add-build-tag"); + } + + #[test] + fn test_params_deserializes() { + let json = r#"{"build_id": 42, "tag": "verified"}"#; + let params: AddBuildTagParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.build_id, 42); + assert_eq!(params.tag, "verified"); + } + + #[test] + fn test_params_converts_to_result() { + let params = AddBuildTagParams { + build_id: 42, + tag: "release-candidate".to_string(), + }; + let result: AddBuildTagResult = params.try_into().unwrap(); + assert_eq!(result.name, "add-build-tag"); + assert_eq!(result.build_id, 42); + assert_eq!(result.tag, "release-candidate"); + } + + #[test] + fn test_validation_rejects_zero_build_id() { + let params = AddBuildTagParams { + build_id: 0, + tag: "verified".to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_empty_tag() { + let params = AddBuildTagParams { + build_id: 42, + tag: "".to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_invalid_tag_chars() { + let params = AddBuildTagParams { + build_id: 42, + tag: "invalid tag!".to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = AddBuildTagParams { + build_id: 42, + tag: "verified".to_string(), + }; + let result: AddBuildTagResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + assert!(json.contains(r#""name":"add-build-tag""#)); + assert!(json.contains(r#""build_id":42"#)); + assert!(json.contains(r#""tag":"verified""#)); + } + + #[test] + fn test_config_defaults() { + let config = AddBuildTagConfig::default(); + assert!(config.allowed_tags.is_empty()); + assert!(config.tag_prefix.is_none()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +allowed-tags: + - verified + - release-candidate +tag-prefix: "agent-" +"#; + let config: AddBuildTagConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.allowed_tags, vec!["verified", "release-candidate"]); + assert_eq!(config.tag_prefix, Some("agent-".to_string())); + } +} diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs new file mode 100644 index 00000000..1f4f015f --- /dev/null +++ b/src/tools/add_pr_comment.rs @@ -0,0 +1,480 @@ +//! Add PR comment safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Parameters for adding a comment thread on a pull request +#[derive(Deserialize, JsonSchema)] +pub struct AddPrCommentParams { + /// The pull request ID to comment on + pub pull_request_id: i32, + + /// Comment text in markdown format. Ensure adequate content > 10 characters. + pub content: String, + + /// Repository alias: "self" for pipeline repo, or an alias from the checkout list. + /// Defaults to "self" if omitted. + #[serde(default = "default_repository")] + pub repository: String, + + /// File path for an inline comment. When set, the comment is anchored to this file. + #[serde(default)] + pub file_path: Option, + + /// Line number for an inline comment. Requires `file_path` to be set. + #[serde(default)] + pub line: Option, + + /// Thread status: "Active" (default), "Closed", "ByDesign", or "WontFix". + #[serde(default = "default_status")] + pub status: String, +} + +fn default_repository() -> String { + "self".to_string() +} + +fn default_status() -> String { + "Active".to_string() +} + +impl Validate for AddPrCommentParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!(self.pull_request_id > 0, "pull_request_id must be positive"); + ensure!( + self.content.len() >= 10, + "content must be at least 10 characters" + ); + if self.line.is_some() { + ensure!( + self.file_path.is_some(), + "line requires file_path to be set" + ); + } + Ok(()) + } +} + +tool_result! { + name = "add-pr-comment", + params = AddPrCommentParams, + /// Result of adding a comment thread on a pull request + pub struct AddPrCommentResult { + pull_request_id: i32, + content: String, + repository: String, + file_path: Option, + line: Option, + status: String, + } +} + +impl Sanitize for AddPrCommentResult { + fn sanitize_fields(&mut self) { + self.content = sanitize_text(&self.content); + } +} + +/// Configuration for the add-pr-comment tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// add-pr-comment: +/// comment-prefix: "[Agent Review] " +/// allowed-repositories: +/// - self +/// - other-repo +/// allowed-statuses: +/// - Active +/// - Closed +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct AddPrCommentConfig { + /// Prefix prepended to all comments (e.g., "[Agent Review] ") + #[serde(default, rename = "comment-prefix")] + pub comment_prefix: Option, + + /// Restrict which repositories the agent can comment on. + /// If empty, all repositories in the checkout list (plus "self") are allowed. + #[serde(default, rename = "allowed-repositories")] + pub allowed_repositories: Vec, + + /// Restrict which thread statuses can be set. + /// If empty, all valid statuses are allowed. + #[serde(default, rename = "allowed-statuses")] + pub allowed_statuses: Vec, +} + +impl Default for AddPrCommentConfig { + fn default() -> Self { + Self { + comment_prefix: None, + allowed_repositories: Vec::new(), + allowed_statuses: Vec::new(), + } + } +} + +/// Map a thread status string to the ADO API integer value +fn status_to_int(status: &str) -> Option { + match status { + "Active" => Some(1), + "Closed" => Some(4), + "ByDesign" => Some(5), + "WontFix" => Some(6), + _ => None, + } +} + +/// All valid thread status strings +const VALID_STATUSES: &[&str] = &["Active", "Closed", "ByDesign", "WontFix"]; + +/// Validate a file path for inline comments: no `..`, not absolute +fn validate_file_path(path: &str) -> anyhow::Result<()> { + ensure!(!path.is_empty(), "file_path must not be empty"); + ensure!( + !path.contains(".."), + "file_path must not contain '..'" + ); + ensure!( + !path.starts_with('/') && !path.starts_with('\\'), + "file_path must not be absolute" + ); + Ok(()) +} + +#[async_trait::async_trait] +impl Executor for AddPrCommentResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Adding comment to PR #{}: {} chars", + self.pull_request_id, + self.content.len() + ); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: AddPrCommentConfig = ctx.get_tool_config("add-pr-comment"); + debug!("Config: {:?}", config); + + // Validate repository against allowed-repositories config + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&self.repository) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list", + self.repository + ))); + } + + // Validate status against allowed-statuses config + if !config.allowed_statuses.is_empty() + && !config.allowed_statuses.contains(&self.status) + { + return Ok(ExecutionResult::failure(format!( + "Status '{}' is not in the allowed-statuses list", + self.status + ))); + } + + // Validate status is a known value + let status_int = match status_to_int(&self.status) { + Some(v) => v, + None => { + return Ok(ExecutionResult::failure(format!( + "Invalid status '{}'. Valid statuses: {}", + self.status, + VALID_STATUSES.join(", ") + ))); + } + }; + + // Validate file_path if present + if let Some(ref fp) = self.file_path { + if let Err(e) = validate_file_path(fp) { + return Ok(ExecutionResult::failure(format!( + "Invalid file_path: {}", + e + ))); + } + } + + // Determine the repository name for the API call + let repo_name = if self.repository == "self" || self.repository.is_empty() { + ctx.repository_name + .as_ref() + .context("BUILD_REPOSITORY_NAME not set and repository is 'self'")? + .clone() + } else { + match ctx.allowed_repositories.get(&self.repository) { + Some(name) => name.clone(), + None => { + return Ok(ExecutionResult::failure(format!( + "Repository alias '{}' not found in allowed repositories", + self.repository + ))); + } + } + }; + + // Build comment content with optional prefix + let comment_body = match &config.comment_prefix { + Some(prefix) => format!("{}{}", prefix, self.content), + None => self.content.clone(), + }; + + // Build the API URL + let url = format!( + "{}/{}/_apis/git/repositories/{}/pullRequests/{}/threads?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(&repo_name, PATH_SEGMENT), + self.pull_request_id, + ); + debug!("API URL: {}", url); + + // Build the request body + let comment_obj = serde_json::json!({ + "parentCommentId": 0, + "content": comment_body, + "commentType": 1 + }); + + let mut thread_body = serde_json::json!({ + "comments": [comment_obj], + "status": status_int + }); + + // Add thread context for inline comments + if let Some(ref fp) = self.file_path { + let line_num = self.line.unwrap_or(1); + thread_body["threadContext"] = serde_json::json!({ + "filePath": format!("/{}", fp), + "rightFileStart": { "line": line_num, "offset": 1 }, + "rightFileEnd": { "line": line_num, "offset": 1 } + }); + } + + let client = reqwest::Client::new(); + + info!("Sending comment thread to PR #{}", self.pull_request_id); + let response = client + .post(&url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&thread_body) + .send() + .await + .context("Failed to send request to Azure DevOps")?; + + if response.status().is_success() { + let body: serde_json::Value = response + .json() + .await + .context("Failed to parse response JSON")?; + + let thread_id = body.get("id").and_then(|v| v.as_i64()).unwrap_or(0); + + info!( + "Comment thread added to PR #{}: thread #{}", + self.pull_request_id, thread_id + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Added comment thread #{} to PR #{}", + thread_id, self.pull_request_id + ), + serde_json::json!({ + "thread_id": thread_id, + "pull_request_id": self.pull_request_id, + "repository": repo_name, + "project": project, + "status": self.status, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + Ok(ExecutionResult::failure(format!( + "Failed to add comment to PR #{} (HTTP {}): {}", + self.pull_request_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(AddPrCommentResult::NAME, "add-pr-comment"); + } + + #[test] + fn test_params_deserializes() { + let json = r#"{"pull_request_id": 42, "content": "This is a review comment on the PR."}"#; + let params: AddPrCommentParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.pull_request_id, 42); + assert!(params.content.contains("review comment")); + assert_eq!(params.repository, "self"); + assert!(params.file_path.is_none()); + assert!(params.line.is_none()); + assert_eq!(params.status, "Active"); + } + + #[test] + fn test_params_converts_to_result() { + let params = AddPrCommentParams { + pull_request_id: 42, + content: "This is a test comment with enough characters.".to_string(), + repository: "self".to_string(), + file_path: None, + line: None, + status: "Active".to_string(), + }; + let result: AddPrCommentResult = params.try_into().unwrap(); + assert_eq!(result.name, "add-pr-comment"); + assert_eq!(result.pull_request_id, 42); + assert!(result.content.contains("test comment")); + } + + #[test] + fn test_validation_rejects_zero_pr_id() { + let params = AddPrCommentParams { + pull_request_id: 0, + content: "This is a valid comment body text.".to_string(), + repository: "self".to_string(), + file_path: None, + line: None, + status: "Active".to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_short_content() { + let params = AddPrCommentParams { + pull_request_id: 42, + content: "Too short".to_string(), + repository: "self".to_string(), + file_path: None, + line: None, + status: "Active".to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_line_without_file_path() { + let params = AddPrCommentParams { + pull_request_id: 42, + content: "This is a valid comment body text.".to_string(), + repository: "self".to_string(), + file_path: None, + line: Some(10), + status: "Active".to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = AddPrCommentParams { + pull_request_id: 42, + content: "A comment body that is definitely longer than ten characters.".to_string(), + repository: "self".to_string(), + file_path: Some("src/main.rs".to_string()), + line: Some(10), + status: "Active".to_string(), + }; + let result: AddPrCommentResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"add-pr-comment""#)); + assert!(json.contains(r#""pull_request_id":42"#)); + } + + #[test] + fn test_config_defaults() { + let config = AddPrCommentConfig::default(); + assert!(config.comment_prefix.is_none()); + assert!(config.allowed_repositories.is_empty()); + assert!(config.allowed_statuses.is_empty()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +comment-prefix: "[Agent Review] " +allowed-repositories: + - self + - other-repo +allowed-statuses: + - Active + - Closed +"#; + let config: AddPrCommentConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.comment_prefix, Some("[Agent Review] ".to_string())); + assert_eq!(config.allowed_repositories, vec!["self", "other-repo"]); + assert_eq!(config.allowed_statuses, vec!["Active", "Closed"]); + } + + #[test] + fn test_status_to_int_mapping() { + assert_eq!(status_to_int("Active"), Some(1)); + assert_eq!(status_to_int("Closed"), Some(4)); + assert_eq!(status_to_int("ByDesign"), Some(5)); + assert_eq!(status_to_int("WontFix"), Some(6)); + assert_eq!(status_to_int("Invalid"), None); + } + + #[test] + fn test_validate_file_path_rejects_traversal() { + assert!(validate_file_path("../etc/passwd").is_err()); + assert!(validate_file_path("src/../secret").is_err()); + } + + #[test] + fn test_validate_file_path_rejects_absolute() { + assert!(validate_file_path("/etc/passwd").is_err()); + assert!(validate_file_path("\\windows\\system32").is_err()); + } + + #[test] + fn test_validate_file_path_accepts_valid() { + assert!(validate_file_path("src/main.rs").is_ok()); + assert!(validate_file_path("path/to/file.txt").is_ok()); + } +} diff --git a/src/tools/create_branch.rs b/src/tools/create_branch.rs new file mode 100644 index 00000000..3d129ca2 --- /dev/null +++ b/src/tools/create_branch.rs @@ -0,0 +1,489 @@ +//! Create branch safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Parameters for creating a branch +#[derive(Deserialize, JsonSchema)] +pub struct CreateBranchParams { + /// Branch name to create (e.g., "feature/my-analysis"). 1-200 characters. + pub branch_name: String, + + /// Branch to create from (default: "main") + pub source_branch: Option, + + /// Specific commit SHA to branch from (overrides source_branch). Must be a valid 40-character hex string. + pub source_commit: Option, + + /// Repository alias: "self" for pipeline repo, or an alias from the checkout list (default: "self") + pub repository: Option, +} + +impl Validate for CreateBranchParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!(!self.branch_name.is_empty(), "branch_name must not be empty"); + ensure!( + self.branch_name.len() <= 200, + "branch_name must be at most 200 characters" + ); + ensure!( + !self.branch_name.contains(".."), + "branch_name must not contain '..'" + ); + ensure!( + !self.branch_name.contains('\0'), + "branch_name must not contain null bytes" + ); + ensure!( + !self.branch_name.starts_with('-'), + "branch_name must not start with '-'" + ); + ensure!( + !self.branch_name.contains(' '), + "branch_name must not contain spaces" + ); + + if let Some(ref commit) = self.source_commit { + ensure!( + commit.len() == 40 && commit.chars().all(|c| c.is_ascii_hexdigit()), + "source_commit must be a valid 40-character hex SHA" + ); + } + + if let Some(ref branch) = self.source_branch { + ensure!( + !branch.contains(".."), + "source_branch must not contain '..'" + ); + ensure!( + !branch.contains('\0'), + "source_branch must not contain null bytes" + ); + } + + Ok(()) + } +} + +tool_result! { + name = "create-branch", + params = CreateBranchParams, + /// Result of creating a branch + pub struct CreateBranchResult { + branch_name: String, + source_branch: Option, + source_commit: Option, + repository: Option, + } +} + +impl Sanitize for CreateBranchResult { + fn sanitize_fields(&mut self) { + self.branch_name = sanitize_text(&self.branch_name); + } +} + +/// Configuration for the create-branch tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// create-branch: +/// branch-pattern: "^agent/.*" +/// allowed-repositories: +/// - self +/// - other-repo +/// allowed-source-branches: +/// - main +/// - develop +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CreateBranchConfig { + /// Regex pattern that branch names must match + #[serde(default, rename = "branch-pattern")] + pub branch_pattern: Option, + + /// Repositories the agent is allowed to create branches in + #[serde(default, rename = "allowed-repositories")] + pub allowed_repositories: Vec, + + /// Source branches the agent is allowed to branch from + #[serde(default, rename = "allowed-source-branches")] + pub allowed_source_branches: Vec, +} + +impl Default for CreateBranchConfig { + fn default() -> Self { + Self { + branch_pattern: None, + allowed_repositories: Vec::new(), + allowed_source_branches: Vec::new(), + } + } +} + +/// Resolve a branch name to its latest commit SHA via the Azure DevOps refs API +async fn resolve_branch_to_commit( + client: &reqwest::Client, + org_url: &str, + project: &str, + token: &str, + repo_name: &str, + branch: &str, +) -> anyhow::Result { + let url = format!( + "{}/{}/_apis/git/repositories/{}/refs", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(repo_name, PATH_SEGMENT), + ); + debug!("Resolving branch '{}' via: {}", branch, url); + + let response = client + .get(&url) + .query(&[ + ("filter", format!("heads/{}", branch).as_str()), + ("api-version", "7.1"), + ]) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to query refs API")?; + + if !response.status().is_success() { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + anyhow::bail!( + "Failed to resolve branch '{}' (HTTP {}): {}", + branch, + status, + error_body + ); + } + + let body: serde_json::Value = response + .json() + .await + .context("Failed to parse refs response")?; + + body.get("value") + .and_then(|v| v.as_array()) + .and_then(|arr| arr.first()) + .and_then(|r| r.get("objectId")) + .and_then(|v| v.as_str()) + .map(|s| s.to_string()) + .context(format!( + "Branch '{}' not found in repository '{}'", + branch, repo_name + )) +} + +#[async_trait::async_trait] +impl Executor for CreateBranchResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!("Creating branch: '{}'", self.branch_name); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: CreateBranchConfig = ctx.get_tool_config("create-branch"); + debug!("Branch pattern: {:?}", config.branch_pattern); + debug!("Allowed repositories: {:?}", config.allowed_repositories); + debug!( + "Allowed source branches: {:?}", + config.allowed_source_branches + ); + + // Validate branch name against branch-pattern regex (if configured) + if let Some(ref pattern) = config.branch_pattern { + let re = regex_lite::Regex::new(pattern).context(format!( + "Invalid branch-pattern regex: '{}'", + pattern + ))?; + if !re.is_match(&self.branch_name) { + return Ok(ExecutionResult::failure(format!( + "Branch name '{}' does not match required pattern '{}'", + self.branch_name, pattern + ))); + } + debug!("Branch name matches pattern '{}'", pattern); + } + + // Determine repository name + let repo_alias = self + .repository + .as_deref() + .unwrap_or("self"); + let repo_name = if repo_alias == "self" { + ctx.repository_name + .as_deref() + .context("BUILD_REPOSITORY_NAME not set")? + .to_string() + } else { + ctx.allowed_repositories + .get(repo_alias) + .cloned() + .context(format!( + "Repository alias '{}' is not in the allowed checkout list", + repo_alias + ))? + }; + debug!("Resolved repository: {}", repo_name); + + // Validate repository against allowed-repositories (if configured) + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&repo_alias.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list", + repo_alias + ))); + } + + // Validate source_branch against allowed-source-branches (if configured) + let source_branch = self.source_branch.as_deref().unwrap_or("main"); + if !config.allowed_source_branches.is_empty() + && !config.allowed_source_branches.contains(&source_branch.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Source branch '{}' is not in the allowed-source-branches list", + source_branch + ))); + } + + let client = reqwest::Client::new(); + + // Resolve the source commit SHA + let source_sha = if let Some(ref commit) = self.source_commit { + debug!("Using explicit source commit: {}", commit); + commit.clone() + } else { + debug!("Resolving source branch '{}' to commit", source_branch); + resolve_branch_to_commit(&client, org_url, project, token, &repo_name, source_branch) + .await? + }; + debug!("Source commit SHA: {}", source_sha); + + // Build the refs update URL + let url = format!( + "{}/{}/_apis/git/repositories/{}/refs?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(&repo_name, PATH_SEGMENT), + ); + debug!("API URL: {}", url); + + // Build the ref update request body + let ref_name = if self.branch_name.starts_with("refs/heads/") { + self.branch_name.clone() + } else { + format!("refs/heads/{}", self.branch_name) + }; + + let ref_updates = serde_json::json!([{ + "name": ref_name, + "oldObjectId": "0000000000000000000000000000000000000000", + "newObjectId": source_sha, + }]); + + info!("Creating branch '{}' from commit {}", ref_name, source_sha); + let response = client + .post(&url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&ref_updates) + .send() + .await + .context("Failed to send request to Azure DevOps")?; + + if response.status().is_success() { + let body: serde_json::Value = response + .json() + .await + .context("Failed to parse response JSON")?; + + // Check for per-ref errors in the response + let success = body + .get("value") + .and_then(|v| v.as_array()) + .and_then(|arr| arr.first()) + .and_then(|r| r.get("success")) + .and_then(|v| v.as_bool()) + .unwrap_or(false); + + if !success { + let custom_message = body + .get("value") + .and_then(|v| v.as_array()) + .and_then(|arr| arr.first()) + .and_then(|r| r.get("customMessage")) + .and_then(|v| v.as_str()) + .unwrap_or("Unknown error"); + + return Ok(ExecutionResult::failure(format!( + "Failed to create branch '{}': {}", + self.branch_name, custom_message + ))); + } + + info!("Branch '{}' created successfully", self.branch_name); + + Ok(ExecutionResult::success_with_data( + format!( + "Created branch '{}' in repository '{}' from commit {}", + self.branch_name, repo_name, &source_sha[..8] + ), + serde_json::json!({ + "branch": self.branch_name, + "ref": ref_name, + "repository": repo_name, + "source_commit": source_sha, + "project": project, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + Ok(ExecutionResult::failure(format!( + "Failed to create branch '{}' (HTTP {}): {}", + self.branch_name, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(CreateBranchResult::NAME, "create-branch"); + } + + #[test] + fn test_params_deserializes() { + let json = r#"{"branch_name": "feature/my-analysis", "source_branch": "main", "repository": "self"}"#; + let params: CreateBranchParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.branch_name, "feature/my-analysis"); + assert_eq!(params.source_branch, Some("main".to_string())); + assert_eq!(params.repository, Some("self".to_string())); + } + + #[test] + fn test_params_converts_to_result() { + let params = CreateBranchParams { + branch_name: "feature/new-work".to_string(), + source_branch: Some("develop".to_string()), + source_commit: None, + repository: None, + }; + let result: CreateBranchResult = params.try_into().unwrap(); + assert_eq!(result.name, "create-branch"); + assert_eq!(result.branch_name, "feature/new-work"); + assert_eq!(result.source_branch, Some("develop".to_string())); + } + + #[test] + fn test_validation_rejects_empty_branch() { + let params = CreateBranchParams { + branch_name: "".to_string(), + source_branch: None, + source_commit: None, + repository: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_path_traversal() { + let params = CreateBranchParams { + branch_name: "feature/../main".to_string(), + source_branch: None, + source_commit: None, + repository: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_invalid_commit() { + let params = CreateBranchParams { + branch_name: "feature/valid".to_string(), + source_branch: None, + source_commit: Some("not-a-valid-sha".to_string()), + repository: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = CreateBranchParams { + branch_name: "feature/test-branch".to_string(), + source_branch: Some("main".to_string()), + source_commit: None, + repository: None, + }; + let result: CreateBranchResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"create-branch""#)); + assert!(json.contains(r#""branch_name":"feature/test-branch""#)); + } + + #[test] + fn test_config_defaults() { + let config = CreateBranchConfig::default(); + assert!(config.branch_pattern.is_none()); + assert!(config.allowed_repositories.is_empty()); + assert!(config.allowed_source_branches.is_empty()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +branch-pattern: "^agent/.*" +allowed-repositories: + - self + - other-repo +allowed-source-branches: + - main + - develop +"#; + let config: CreateBranchConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.branch_pattern, Some("^agent/.*".to_string())); + assert_eq!(config.allowed_repositories, vec!["self", "other-repo"]); + assert_eq!(config.allowed_source_branches, vec!["main", "develop"]); + } +} diff --git a/src/tools/create_git_tag.rs b/src/tools/create_git_tag.rs new file mode 100644 index 00000000..4a0f6623 --- /dev/null +++ b/src/tools/create_git_tag.rs @@ -0,0 +1,467 @@ +//! Create git tag safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Parameters for creating a git tag (agent-provided) +#[derive(Deserialize, JsonSchema)] +pub struct CreateGitTagParams { + /// Tag name (e.g., "v1.2.3"). Must be 3-100 characters, alphanumeric + /// plus dots, dashes, underscores, and slashes only. + pub tag_name: String, + + /// Commit SHA to tag. If omitted, the executor resolves HEAD of the + /// default branch. Must be a valid 40-character hex string if present. + pub commit: Option, + + /// Tag annotation message. Must be at least 5 characters if present. + pub message: Option, + + /// Repository alias: "self" for the pipeline repo, or an alias from + /// the `checkout:` list. Defaults to "self". + pub repository: Option, +} + +/// Regex pattern for valid tag names: alphanumeric, dots, dashes, underscores, slashes. +static TAG_NAME_PATTERN: std::sync::LazyLock = + std::sync::LazyLock::new(|| regex_lite::Regex::new(r"^[a-zA-Z0-9._/\-]+$").unwrap()); + +impl Validate for CreateGitTagParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!( + !self.tag_name.starts_with('-'), + "tag_name must not start with '-'" + ); + ensure!( + self.tag_name.len() >= 3, + "tag_name must be at least 3 characters" + ); + ensure!( + self.tag_name.len() <= 100, + "tag_name must be at most 100 characters" + ); + ensure!( + TAG_NAME_PATTERN.is_match(&self.tag_name), + "tag_name contains invalid characters (only alphanumeric, dots, dashes, underscores, and slashes are allowed): {}", + self.tag_name + ); + + if let Some(commit) = &self.commit { + ensure!( + commit.len() == 40, + "commit must be exactly 40 hex characters, got {} characters", + commit.len() + ); + ensure!( + commit.chars().all(|c| c.is_ascii_hexdigit()), + "commit must be a valid hex string: {}", + commit + ); + } + + if let Some(message) = &self.message { + ensure!( + message.len() >= 5, + "message must be at least 5 characters" + ); + } + + Ok(()) + } +} + +tool_result! { + name = "create-git-tag", + params = CreateGitTagParams, + /// Result of creating a git tag + pub struct CreateGitTagResult { + tag_name: String, + commit: Option, + message: Option, + repository: Option, + } +} + +impl Sanitize for CreateGitTagResult { + fn sanitize_fields(&mut self) { + // tag_name is a structural identifier — only strip control characters + self.tag_name = self + .tag_name + .chars() + .filter(|c| !c.is_control()) + .collect(); + self.message = self.message.as_ref().map(|m| sanitize_text(m)); + // commit and repository are structural identifiers; strip control chars only + self.commit = self.commit.as_ref().map(|c| { + c.chars().filter(|ch| !ch.is_control()).collect() + }); + self.repository = self.repository.as_ref().map(|r| { + r.chars().filter(|ch| !ch.is_control()).collect() + }); + } +} + +/// Configuration for the create-git-tag tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// create-git-tag: +/// tag-pattern: "^v\\d+\\.\\d+\\.\\d+$" +/// allowed-repositories: +/// - self +/// - my-lib +/// message-prefix: "[release] " +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CreateGitTagConfig { + /// Regex pattern that tag names must match (if configured) + #[serde(default, rename = "tag-pattern")] + pub tag_pattern: Option, + + /// Repositories the agent is allowed to tag (if empty, all allowed) + #[serde(default, rename = "allowed-repositories")] + pub allowed_repositories: Vec, + + /// Prefix prepended to the tag message + #[serde(default, rename = "message-prefix")] + pub message_prefix: Option, +} + +impl Default for CreateGitTagConfig { + fn default() -> Self { + Self { + tag_pattern: None, + allowed_repositories: Vec::new(), + message_prefix: None, + } + } +} + +/// Resolve HEAD of the default branch for a repository via the ADO refs API. +async fn resolve_head_commit( + client: &reqwest::Client, + org_url: &str, + project: &str, + token: &str, + repo_name: &str, +) -> anyhow::Result { + let url = format!( + "{}/{}/_apis/git/repositories/{}/refs?filter=heads/main&api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(repo_name, PATH_SEGMENT), + ); + + debug!("Resolving HEAD commit via: {}", url); + + let response = client + .get(&url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to query refs for HEAD resolution")?; + + ensure!( + response.status().is_success(), + "Failed to resolve HEAD (HTTP {})", + response.status() + ); + + let body: serde_json::Value = response + .json() + .await + .context("Failed to parse refs response")?; + + body.get("value") + .and_then(|v| v.as_array()) + .and_then(|arr| arr.first()) + .and_then(|r| r.get("objectId")) + .and_then(|v| v.as_str()) + .map(|s| s.to_string()) + .context("No refs found for heads/main — cannot resolve HEAD commit") +} + +#[async_trait::async_trait] +impl Executor for CreateGitTagResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!("Creating git tag: '{}'", self.tag_name); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: CreateGitTagConfig = ctx.get_tool_config("create-git-tag"); + debug!("Tag pattern: {:?}", config.tag_pattern); + debug!("Allowed repositories: {:?}", config.allowed_repositories); + + // Validate tag against configured pattern + if let Some(pattern) = &config.tag_pattern { + let re = regex_lite::Regex::new(pattern).context(format!( + "Invalid tag-pattern regex in config: {}", + pattern + ))?; + if !re.is_match(&self.tag_name) { + return Ok(ExecutionResult::failure(format!( + "Tag name '{}' does not match required pattern '{}'", + self.tag_name, pattern + ))); + } + } + + // Resolve repository name + let repo_alias = self.repository.as_deref().unwrap_or("self"); + let repo_name = if repo_alias == "self" { + ctx.repository_name + .as_deref() + .context("BUILD_REPOSITORY_NAME not set and repository is 'self'")? + .to_string() + } else { + ctx.allowed_repositories + .get(repo_alias) + .cloned() + .context(format!( + "Repository alias '{}' not found in allowed repositories", + repo_alias + ))? + }; + + // Validate repository against allowed-repositories config + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&repo_alias.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list", + repo_alias + ))); + } + + let client = reqwest::Client::new(); + + // Resolve commit SHA — use provided value or look up HEAD + let commit_sha = match &self.commit { + Some(sha) => sha.clone(), + None => { + info!("No commit specified, resolving HEAD of default branch"); + resolve_head_commit(&client, org_url, project, token, &repo_name).await? + } + }; + debug!("Tagging commit: {}", commit_sha); + + // Build tag message with optional prefix + let tag_message = match (&config.message_prefix, &self.message) { + (Some(prefix), Some(msg)) => format!("{}{}", prefix, msg), + (Some(prefix), None) => format!("{}Tag {}", prefix, self.tag_name), + (None, Some(msg)) => msg.clone(), + (None, None) => format!("Tag {}", self.tag_name), + }; + + // POST annotated tag + let url = format!( + "{}/{}/_apis/git/repositories/{}/annotatedtags?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(&repo_name, PATH_SEGMENT), + ); + debug!("API URL: {}", url); + + let body = serde_json::json!({ + "name": self.tag_name, + "taggedObject": { + "objectId": commit_sha + }, + "message": tag_message + }); + + info!("Sending annotated tag creation request to ADO"); + let response = client + .post(&url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&body) + .send() + .await + .context("Failed to send request to Azure DevOps")?; + + if response.status().is_success() { + let resp_body: serde_json::Value = response + .json() + .await + .context("Failed to parse response JSON")?; + + let tag_url = resp_body + .get("url") + .and_then(|v| v.as_str()) + .unwrap_or(""); + + info!("Tag created: {} -> {}", self.tag_name, commit_sha); + + Ok(ExecutionResult::success_with_data( + format!( + "Created tag '{}' on commit {} in repository '{}'", + self.tag_name, commit_sha, repo_name + ), + serde_json::json!({ + "tag": self.tag_name, + "commit": commit_sha, + "repository": repo_name, + "url": tag_url, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + Ok(ExecutionResult::failure(format!( + "Failed to create tag (HTTP {}): {}", + status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(CreateGitTagResult::NAME, "create-git-tag"); + } + + #[test] + fn test_params_deserializes() { + let json = r#"{"tag_name": "v1.2.3", "commit": "abcdef1234567890abcdef1234567890abcdef12", "message": "Release v1.2.3"}"#; + let params: CreateGitTagParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.tag_name, "v1.2.3"); + assert_eq!( + params.commit.as_deref(), + Some("abcdef1234567890abcdef1234567890abcdef12") + ); + assert_eq!(params.message.as_deref(), Some("Release v1.2.3")); + } + + #[test] + fn test_params_converts_to_result() { + let params = CreateGitTagParams { + tag_name: "v1.0.0".to_string(), + commit: Some("abcdef1234567890abcdef1234567890abcdef12".to_string()), + message: Some("Initial release".to_string()), + repository: None, + }; + let result: CreateGitTagResult = params.try_into().unwrap(); + assert_eq!(result.name, "create-git-tag"); + assert_eq!(result.tag_name, "v1.0.0"); + } + + #[test] + fn test_validation_rejects_empty_tag() { + let params = CreateGitTagParams { + tag_name: "ab".to_string(), + commit: None, + message: None, + repository: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_invalid_tag_chars() { + let params = CreateGitTagParams { + tag_name: "v1.0 invalid!".to_string(), + commit: None, + message: None, + repository: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_invalid_commit_sha() { + let params = CreateGitTagParams { + tag_name: "v1.0.0".to_string(), + commit: Some("not-a-valid-sha".to_string()), + message: None, + repository: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_short_message() { + let params = CreateGitTagParams { + tag_name: "v1.0.0".to_string(), + commit: None, + message: Some("Hi".to_string()), + repository: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = CreateGitTagParams { + tag_name: "v2.0.0".to_string(), + commit: Some("abcdef1234567890abcdef1234567890abcdef12".to_string()), + message: Some("Major release".to_string()), + repository: None, + }; + let result: CreateGitTagResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"create-git-tag""#)); + assert!(json.contains(r#""tag_name":"v2.0.0""#)); + } + + #[test] + fn test_config_defaults() { + let config = CreateGitTagConfig::default(); + assert!(config.tag_pattern.is_none()); + assert!(config.allowed_repositories.is_empty()); + assert!(config.message_prefix.is_none()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +tag-pattern: "^v\\d+\\.\\d+\\.\\d+$" +allowed-repositories: + - self + - my-lib +message-prefix: "[release] " +"#; + let config: CreateGitTagConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!( + config.tag_pattern.as_deref(), + Some("^v\\d+\\.\\d+\\.\\d+$") + ); + assert_eq!(config.allowed_repositories, vec!["self", "my-lib"]); + assert_eq!(config.message_prefix.as_deref(), Some("[release] ")); + } +} diff --git a/src/tools/link_work_items.rs b/src/tools/link_work_items.rs new file mode 100644 index 00000000..0e81e866 --- /dev/null +++ b/src/tools/link_work_items.rs @@ -0,0 +1,420 @@ +//! Link work items safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Resolve a human-friendly link type name to the ADO relation type string. +fn resolve_link_type(link_type: &str) -> Option<&'static str> { + match link_type { + "parent" => Some("System.LinkTypes.Hierarchy-Reverse"), + "child" => Some("System.LinkTypes.Hierarchy-Forward"), + "related" => Some("System.LinkTypes.Related"), + "predecessor" => Some("System.LinkTypes.Dependency-Reverse"), + "successor" => Some("System.LinkTypes.Dependency-Forward"), + "duplicate" => Some("System.LinkTypes.Duplicate-Forward"), + "duplicate-of" => Some("System.LinkTypes.Duplicate-Reverse"), + _ => None, + } +} + +/// All valid link type names accepted by this tool. +const VALID_LINK_TYPES: &[&str] = &[ + "parent", + "child", + "related", + "predecessor", + "successor", + "duplicate", + "duplicate-of", +]; + +/// Parameters for linking two work items +#[derive(Deserialize, JsonSchema)] +pub struct LinkWorkItemsParams { + /// The source work item ID (the item the link is added to) + pub source_id: i64, + + /// The target work item ID (the item being linked to) + pub target_id: i64, + + /// Link type: parent, child, related, predecessor, successor, duplicate, duplicate-of + pub link_type: String, + + /// Optional comment describing the relationship + pub comment: Option, +} + +impl Validate for LinkWorkItemsParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!(self.source_id > 0, "source_id must be positive"); + ensure!(self.target_id > 0, "target_id must be positive"); + ensure!( + self.source_id != self.target_id, + "source_id and target_id must be different" + ); + ensure!( + resolve_link_type(&self.link_type).is_some(), + "invalid link_type '{}'; must be one of: {}", + self.link_type, + VALID_LINK_TYPES.join(", ") + ); + if let Some(ref comment) = self.comment { + ensure!( + comment.len() >= 5, + "comment must be at least 5 characters" + ); + } + Ok(()) + } +} + +tool_result! { + name = "link-work-items", + params = LinkWorkItemsParams, + /// Result of linking two work items + pub struct LinkWorkItemsResult { + source_id: i64, + target_id: i64, + link_type: String, + comment: Option, + } +} + +impl Sanitize for LinkWorkItemsResult { + fn sanitize_fields(&mut self) { + self.link_type = sanitize_text(&self.link_type); + self.comment = self.comment.as_deref().map(sanitize_text); + } +} + +/// Configuration for the link-work-items tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// link-work-items: +/// allowed-link-types: +/// - parent +/// - child +/// - related +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct LinkWorkItemsConfig { + /// Restrict which link types the agent may use. + /// An empty list (the default) means all link types are allowed. + #[serde(default, rename = "allowed-link-types")] + pub allowed_link_types: Vec, +} + +impl Default for LinkWorkItemsConfig { + fn default() -> Self { + Self { + allowed_link_types: Vec::new(), + } + } +} + +#[async_trait::async_trait] +impl Executor for LinkWorkItemsResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Linking work item #{} -> #{} ({})", + self.source_id, self.target_id, self.link_type + ); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: LinkWorkItemsConfig = ctx.get_tool_config("link-work-items"); + debug!("Allowed link types: {:?}", config.allowed_link_types); + + // Validate link type against configured allow-list + if !config.allowed_link_types.is_empty() + && !config.allowed_link_types.contains(&self.link_type) + { + return Ok(ExecutionResult::failure(format!( + "Link type '{}' is not in the allowed set: {}", + self.link_type, + config.allowed_link_types.join(", ") + ))); + } + + let relation_type = match resolve_link_type(&self.link_type) { + Some(rt) => rt, + None => { + return Ok(ExecutionResult::failure(format!( + "Unknown link type '{}'; must be one of: {}", + self.link_type, + VALID_LINK_TYPES.join(", ") + ))); + } + }; + + // Build the target work item URL for the relation + let target_url = format!( + "{}/{}/_apis/wit/workitems/{}", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + self.target_id, + ); + + // Build the JSON Patch body + let mut relation_value = serde_json::json!({ + "rel": relation_type, + "url": target_url, + }); + + if let Some(ref comment) = self.comment { + relation_value["attributes"] = serde_json::json!({ + "comment": comment, + }); + } + + let patch_doc = vec![serde_json::json!({ + "op": "add", + "path": "/relations/-", + "value": relation_value, + })]; + + // PATCH https://dev.azure.com/{org}/{project}/_apis/wit/workitems/{id}?api-version=7.1 + let url = format!( + "{}/{}/_apis/wit/workitems/{}?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + self.source_id, + ); + debug!("API URL: {}", url); + + let client = reqwest::Client::new(); + + info!( + "Sending link request: #{} -[{}]-> #{}", + self.source_id, self.link_type, self.target_id + ); + let response = client + .patch(&url) + .header("Content-Type", "application/json-patch+json") + .basic_auth("", Some(token)) + .json(&patch_doc) + .send() + .await + .context("Failed to send request to Azure DevOps")?; + + if response.status().is_success() { + let body: serde_json::Value = response + .json() + .await + .context("Failed to parse response JSON")?; + + let work_item_id = body.get("id").and_then(|v| v.as_i64()).unwrap_or(0); + + info!( + "Linked work item #{} -> #{} ({})", + self.source_id, self.target_id, self.link_type + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Linked work item #{} -> #{} ({})", + self.source_id, self.target_id, self.link_type + ), + serde_json::json!({ + "source_id": self.source_id, + "target_id": self.target_id, + "link_type": self.link_type, + "relation_type": relation_type, + "work_item_id": work_item_id, + "project": project, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + Ok(ExecutionResult::failure(format!( + "Failed to link work item #{} -> #{} (HTTP {}): {}", + self.source_id, self.target_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(LinkWorkItemsResult::NAME, "link-work-items"); + } + + #[test] + fn test_params_deserializes() { + let json = + r#"{"source_id": 100, "target_id": 200, "link_type": "parent", "comment": "test linking"}"#; + let params: LinkWorkItemsParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.source_id, 100); + assert_eq!(params.target_id, 200); + assert_eq!(params.link_type, "parent"); + assert_eq!(params.comment.as_deref(), Some("test linking")); + } + + #[test] + fn test_params_converts_to_result() { + let params = LinkWorkItemsParams { + source_id: 100, + target_id: 200, + link_type: "child".to_string(), + comment: Some("Links parent to child".to_string()), + }; + let result: LinkWorkItemsResult = params.try_into().unwrap(); + assert_eq!(result.name, "link-work-items"); + assert_eq!(result.source_id, 100); + assert_eq!(result.target_id, 200); + assert_eq!(result.link_type, "child"); + assert_eq!(result.comment.as_deref(), Some("Links parent to child")); + } + + #[test] + fn test_validation_rejects_zero_source_id() { + let params = LinkWorkItemsParams { + source_id: 0, + target_id: 200, + link_type: "related".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_zero_target_id() { + let params = LinkWorkItemsParams { + source_id: 100, + target_id: 0, + link_type: "related".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_same_ids() { + let params = LinkWorkItemsParams { + source_id: 100, + target_id: 100, + link_type: "related".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_invalid_link_type() { + let params = LinkWorkItemsParams { + source_id: 100, + target_id: 200, + link_type: "unknown".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_resolve_link_type() { + assert_eq!( + resolve_link_type("parent"), + Some("System.LinkTypes.Hierarchy-Reverse") + ); + assert_eq!( + resolve_link_type("child"), + Some("System.LinkTypes.Hierarchy-Forward") + ); + assert_eq!( + resolve_link_type("related"), + Some("System.LinkTypes.Related") + ); + assert_eq!( + resolve_link_type("predecessor"), + Some("System.LinkTypes.Dependency-Reverse") + ); + assert_eq!( + resolve_link_type("successor"), + Some("System.LinkTypes.Dependency-Forward") + ); + assert_eq!( + resolve_link_type("duplicate"), + Some("System.LinkTypes.Duplicate-Forward") + ); + assert_eq!( + resolve_link_type("duplicate-of"), + Some("System.LinkTypes.Duplicate-Reverse") + ); + assert_eq!(resolve_link_type("invalid"), None); + assert_eq!(resolve_link_type(""), None); + } + + #[test] + fn test_config_defaults() { + let config = LinkWorkItemsConfig::default(); + assert!(config.allowed_link_types.is_empty()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +allowed-link-types: + - parent + - child + - related +"#; + let config: LinkWorkItemsConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.allowed_link_types.len(), 3); + assert!(config.allowed_link_types.contains(&"parent".to_string())); + assert!(config.allowed_link_types.contains(&"child".to_string())); + assert!(config.allowed_link_types.contains(&"related".to_string())); + } + + #[test] + fn test_result_serializes_correctly() { + let params = LinkWorkItemsParams { + source_id: 100, + target_id: 200, + link_type: "related".to_string(), + comment: None, + }; + let result: LinkWorkItemsResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"link-work-items""#)); + assert!(json.contains(r#""source_id":100"#)); + assert!(json.contains(r#""target_id":200"#)); + assert!(json.contains(r#""link_type":"related""#)); + } +} diff --git a/src/tools/mod.rs b/src/tools/mod.rs index ecd4dca9..2fd10330 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -104,27 +104,43 @@ pub(crate) async fn resolve_wiki_branch( } } +mod add_build_tag; +mod add_pr_comment; mod comment_on_work_item; +mod create_branch; +mod create_git_tag; mod create_pr; mod create_wiki_page; mod create_work_item; -mod update_wiki_page; +mod link_work_items; pub mod memory; mod missing_data; mod missing_tool; mod noop; +mod queue_build; mod result; +mod update_pr; +mod update_wiki_page; mod update_work_item; +mod upload_attachment; +pub use add_build_tag::*; +pub use add_pr_comment::*; pub use comment_on_work_item::*; +pub use create_branch::*; +pub use create_git_tag::*; pub use create_pr::*; pub use create_wiki_page::*; pub use create_work_item::*; -pub use update_wiki_page::*; +pub use link_work_items::*; pub use missing_data::*; pub use missing_tool::*; pub use noop::*; +pub use queue_build::*; pub use result::{ ExecutionContext, ExecutionResult, Executor, ToolResult, Validate, anyhow_to_mcp_error, }; +pub use update_pr::*; +pub use update_wiki_page::*; pub use update_work_item::*; +pub use upload_attachment::*; diff --git a/src/tools/queue_build.rs b/src/tools/queue_build.rs new file mode 100644 index 00000000..09e4c63a --- /dev/null +++ b/src/tools/queue_build.rs @@ -0,0 +1,479 @@ +//! Queue build safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Parameters for queuing a build +#[derive(Deserialize, JsonSchema)] +pub struct QueueBuildParams { + /// Pipeline definition ID to trigger (must be positive) + pub pipeline_id: i32, + + /// Branch to build (optional, defaults to configured default or "main") + pub branch: Option, + + /// Template parameter key-value pairs (optional) + pub parameters: Option>, + + /// Human-readable reason for triggering the build; at least 5 characters if provided + pub reason: Option, +} + +impl Validate for QueueBuildParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!(self.pipeline_id > 0, "pipeline_id must be positive"); + if let Some(reason) = &self.reason { + ensure!( + reason.len() >= 5, + "reason must be at least 5 characters" + ); + } + if let Some(branch) = &self.branch { + ensure!( + !branch.contains(".."), + "branch name must not contain '..'" + ); + ensure!( + !branch.contains('\0'), + "branch name must not contain null bytes" + ); + } + Ok(()) + } +} + +tool_result! { + name = "queue-build", + params = QueueBuildParams, + /// Result of queuing a build + pub struct QueueBuildResult { + pipeline_id: i32, + branch: Option, + parameters: Option>, + reason: Option, + } +} + +impl Sanitize for QueueBuildResult { + fn sanitize_fields(&mut self) { + if let Some(reason) = &self.reason { + self.reason = Some(sanitize_text(reason)); + } + } +} + +/// Configuration for the queue-build tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// queue-build: +/// allowed-pipelines: +/// - 123 +/// - 456 +/// allowed-branches: +/// - main +/// - release/* +/// allowed-parameters: +/// - environment +/// - version +/// default-branch: main +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct QueueBuildConfig { + /// Pipeline definition IDs that are allowed to be triggered (REQUIRED — empty rejects all) + #[serde(default, rename = "allowed-pipelines")] + pub allowed_pipelines: Vec, + + /// Branches that are allowed to be built (if empty, any branch is allowed) + #[serde(default, rename = "allowed-branches")] + pub allowed_branches: Vec, + + /// Parameter keys that are allowed to be passed (if empty, any parameters are allowed) + #[serde(default, rename = "allowed-parameters")] + pub allowed_parameters: Vec, + + /// Default branch to use when the agent does not specify one + #[serde(default, rename = "default-branch")] + pub default_branch: Option, +} + +impl Default for QueueBuildConfig { + fn default() -> Self { + Self { + allowed_pipelines: Vec::new(), + allowed_branches: Vec::new(), + allowed_parameters: Vec::new(), + default_branch: None, + } + } +} + +#[async_trait::async_trait] +impl Executor for QueueBuildResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!("Queuing build for pipeline definition {}", self.pipeline_id); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + // Get tool-specific configuration + let config: QueueBuildConfig = ctx.get_tool_config("queue-build"); + debug!("Allowed pipelines: {:?}", config.allowed_pipelines); + debug!("Allowed branches: {:?}", config.allowed_branches); + debug!("Allowed parameters: {:?}", config.allowed_parameters); + + // Validate pipeline_id against allowed-pipelines (REQUIRED) + if config.allowed_pipelines.is_empty() { + return Ok(ExecutionResult::failure( + "queue-build allowed-pipelines is not configured. \ + This is required to scope which pipelines the agent can trigger." + .to_string(), + )); + } + if !config.allowed_pipelines.contains(&self.pipeline_id) { + return Ok(ExecutionResult::failure(format!( + "Pipeline definition {} is not in the allowed-pipelines list", + self.pipeline_id + ))); + } + + // Resolve the effective branch + let effective_branch = self + .branch + .as_deref() + .or(config.default_branch.as_deref()) + .unwrap_or("main"); + debug!("Effective branch: {}", effective_branch); + + // Validate branch against allowed-branches (if configured) + if !config.allowed_branches.is_empty() { + let branch_allowed = config.allowed_branches.iter().any(|pattern| { + if pattern.ends_with("/*") { + let prefix = &pattern[..pattern.len() - 2]; + effective_branch.starts_with(prefix) + && (effective_branch.len() == prefix.len() + || effective_branch[prefix.len()..].starts_with('/')) + } else { + pattern == effective_branch + } + }); + if !branch_allowed { + return Ok(ExecutionResult::failure(format!( + "Branch '{}' is not in the allowed-branches list", + effective_branch + ))); + } + } + + // Validate parameter keys against allowed-parameters (if configured) + if let Some(params) = &self.parameters { + if !config.allowed_parameters.is_empty() { + for key in params.keys() { + if !config.allowed_parameters.contains(key) { + return Ok(ExecutionResult::failure(format!( + "Parameter '{}' is not in the allowed-parameters list", + key + ))); + } + } + } + } + + // Build the source branch ref + let source_branch = if effective_branch.starts_with("refs/") { + effective_branch.to_string() + } else { + format!("refs/heads/{}", effective_branch) + }; + debug!("Source branch ref: {}", source_branch); + + // Build the request body + let mut body = serde_json::json!({ + "definition": { "id": self.pipeline_id }, + "sourceBranch": source_branch, + "reason": "userCreated", + }); + + // Add template parameters as a JSON string if provided + if let Some(params) = &self.parameters { + if !params.is_empty() { + let params_json = serde_json::to_string(params) + .context("Failed to serialize template parameters")?; + body["parameters"] = serde_json::Value::String(params_json); + } + } + + // Build the API URL + let url = format!( + "{}/{}/_apis/build/builds?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + ); + debug!("API URL: {}", url); + + // Make the API call + let client = reqwest::Client::new(); + info!( + "Sending queue build request for pipeline {} on branch {}", + self.pipeline_id, source_branch + ); + let response = client + .post(&url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&body) + .send() + .await + .context("Failed to send request to Azure DevOps")?; + + if response.status().is_success() { + let resp_body: serde_json::Value = response + .json() + .await + .context("Failed to parse response JSON")?; + + let build_id = resp_body.get("id").and_then(|v| v.as_i64()).unwrap_or(0); + let build_url = resp_body + .get("_links") + .and_then(|l| l.get("web")) + .and_then(|h| h.get("href")) + .and_then(|h| h.as_str()) + .unwrap_or(""); + + info!("Build queued: #{} - {}", build_id, build_url); + + Ok(ExecutionResult::success_with_data( + format!( + "Queued build #{} for pipeline {} on branch {}", + build_id, self.pipeline_id, effective_branch + ), + serde_json::json!({ + "build_id": build_id, + "pipeline_id": self.pipeline_id, + "branch": effective_branch, + "url": build_url, + "project": project, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + Ok(ExecutionResult::failure(format!( + "Failed to queue build for pipeline {} (HTTP {}): {}", + self.pipeline_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(QueueBuildResult::NAME, "queue-build"); + } + + #[test] + fn test_params_deserializes() { + let json = r#"{"pipeline_id": 123, "branch": "main", "reason": "Nightly rebuild"}"#; + let params: QueueBuildParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.pipeline_id, 123); + assert_eq!(params.branch, Some("main".to_string())); + assert_eq!(params.reason, Some("Nightly rebuild".to_string())); + assert!(params.parameters.is_none()); + } + + #[test] + fn test_params_converts_to_result() { + let params = QueueBuildParams { + pipeline_id: 42, + branch: Some("develop".to_string()), + parameters: None, + reason: Some("Trigger nightly build".to_string()), + }; + let result: QueueBuildResult = params.try_into().unwrap(); + assert_eq!(result.name, "queue-build"); + assert_eq!(result.pipeline_id, 42); + assert_eq!(result.branch, Some("develop".to_string())); + assert_eq!(result.reason, Some("Trigger nightly build".to_string())); + } + + #[test] + fn test_validation_rejects_zero_pipeline_id() { + let params = QueueBuildParams { + pipeline_id: 0, + branch: None, + parameters: None, + reason: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_negative_pipeline_id() { + let params = QueueBuildParams { + pipeline_id: -1, + branch: None, + parameters: None, + reason: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_short_reason() { + let params = QueueBuildParams { + pipeline_id: 1, + branch: None, + parameters: None, + reason: Some("Hi".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_branch_with_dotdot() { + let params = QueueBuildParams { + pipeline_id: 1, + branch: Some("../../etc/passwd".to_string()), + parameters: None, + reason: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_branch_with_null_byte() { + let params = QueueBuildParams { + pipeline_id: 1, + branch: Some("main\0evil".to_string()), + parameters: None, + reason: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_accepts_valid_params() { + let params = QueueBuildParams { + pipeline_id: 123, + branch: Some("main".to_string()), + parameters: None, + reason: Some("Scheduled nightly build".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_ok()); + } + + #[test] + fn test_validation_accepts_minimal_params() { + let params = QueueBuildParams { + pipeline_id: 1, + branch: None, + parameters: None, + reason: None, + }; + let result: Result = params.try_into(); + assert!(result.is_ok()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = QueueBuildParams { + pipeline_id: 99, + branch: Some("release/v1".to_string()), + parameters: None, + reason: Some("Release build trigger".to_string()), + }; + let result: QueueBuildResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"queue-build""#)); + assert!(json.contains(r#""pipeline_id":99"#)); + } + + #[test] + fn test_config_defaults() { + let config = QueueBuildConfig::default(); + assert!(config.allowed_pipelines.is_empty()); + assert!(config.allowed_branches.is_empty()); + assert!(config.allowed_parameters.is_empty()); + assert!(config.default_branch.is_none()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +allowed-pipelines: + - 123 + - 456 +allowed-branches: + - main + - release/* +allowed-parameters: + - environment + - version +default-branch: main +"#; + let config: QueueBuildConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.allowed_pipelines, vec![123, 456]); + assert_eq!(config.allowed_branches, vec!["main", "release/*"]); + assert_eq!(config.allowed_parameters, vec!["environment", "version"]); + assert_eq!(config.default_branch, Some("main".to_string())); + } + + #[test] + fn test_config_partial_deserialize_uses_defaults() { + let yaml = r#" +allowed-pipelines: + - 42 +"#; + let config: QueueBuildConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.allowed_pipelines, vec![42]); + assert!(config.allowed_branches.is_empty()); + assert!(config.allowed_parameters.is_empty()); + assert!(config.default_branch.is_none()); + } + + #[test] + fn test_params_deserializes_with_parameters() { + let json = r#"{"pipeline_id": 10, "parameters": {"env": "prod", "version": "2.0"}}"#; + let params: QueueBuildParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.pipeline_id, 10); + let map = params.parameters.unwrap(); + assert_eq!(map.get("env"), Some(&"prod".to_string())); + assert_eq!(map.get("version"), Some(&"2.0".to_string())); + } +} diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs new file mode 100644 index 00000000..7cf8525f --- /dev/null +++ b/src/tools/update_pr.rs @@ -0,0 +1,970 @@ +//! Update pull request safe output tool + +use log::{debug, info, warn}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Valid operation names for update-pr +const VALID_OPERATIONS: &[&str] = &[ + "add-reviewers", + "add-labels", + "set-auto-complete", + "vote", + "update-description", +]; + +/// Valid vote values +const VALID_VOTES: &[&str] = &[ + "approve", + "approve-with-suggestions", + "wait-for-author", + "reject", + "reset", +]; + +/// Map a vote string to its ADO numeric value +fn vote_to_ado_value(vote: &str) -> Option { + match vote { + "approve" => Some(10), + "approve-with-suggestions" => Some(5), + "wait-for-author" => Some(-5), + "reject" => Some(-10), + "reset" => Some(0), + _ => None, + } +} + +/// Parameters for updating a pull request +#[derive(Deserialize, JsonSchema)] +pub struct UpdatePrParams { + /// Pull request ID (must be positive) + pub pull_request_id: i32, + + /// Repository alias: "self" for the pipeline repo, or an alias from the checkout list + #[serde(default)] + pub repository: Option, + + /// Operation to perform: "add-reviewers", "add-labels", "set-auto-complete", "vote", or "update-description" + pub operation: String, + + /// Reviewer emails (required for add-reviewers operation) + pub reviewers: Option>, + + /// Label names (required for add-labels operation) + pub labels: Option>, + + /// Vote value: "approve", "approve-with-suggestions", "wait-for-author", "reject", or "reset" + pub vote: Option, + + /// New PR description in markdown (required for update-description, must be >= 10 chars) + pub description: Option, +} + +impl Validate for UpdatePrParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!( + self.pull_request_id > 0, + "pull_request_id must be a positive integer" + ); + ensure!( + VALID_OPERATIONS.contains(&self.operation.as_str()), + "operation must be one of: {}", + VALID_OPERATIONS.join(", ") + ); + + match self.operation.as_str() { + "add-reviewers" => { + let reviewers = self + .reviewers + .as_ref() + .context("reviewers must be provided for add-reviewers operation")?; + ensure!( + !reviewers.is_empty(), + "reviewers list must not be empty for add-reviewers operation" + ); + } + "add-labels" => { + let labels = self + .labels + .as_ref() + .context("labels must be provided for add-labels operation")?; + ensure!( + !labels.is_empty(), + "labels list must not be empty for add-labels operation" + ); + } + "vote" => { + let vote = self + .vote + .as_ref() + .context("vote must be provided for vote operation")?; + ensure!( + VALID_VOTES.contains(&vote.as_str()), + "vote must be one of: {}", + VALID_VOTES.join(", ") + ); + } + "update-description" => { + let desc = self + .description + .as_ref() + .context("description must be provided for update-description operation")?; + ensure!( + desc.len() >= 10, + "description must be at least 10 characters" + ); + } + _ => {} // set-auto-complete has no extra required fields + } + + Ok(()) + } +} + +tool_result! { + name = "update-pr", + params = UpdatePrParams, + /// Result of updating a pull request + pub struct UpdatePrResult { + pull_request_id: i32, + repository: Option, + operation: String, + reviewers: Option>, + labels: Option>, + vote: Option, + description: Option, + } +} + +impl Sanitize for UpdatePrResult { + fn sanitize_fields(&mut self) { + self.repository = self.repository.as_deref().map(sanitize_text); + self.operation = sanitize_text(&self.operation); + self.reviewers = self + .reviewers + .as_ref() + .map(|rs| rs.iter().map(|r| sanitize_text(r)).collect()); + self.labels = self + .labels + .as_ref() + .map(|ls| ls.iter().map(|l| sanitize_text(l)).collect()); + self.vote = self.vote.as_deref().map(sanitize_text); + self.description = self.description.as_deref().map(sanitize_text); + } +} + +/// Configuration for the update-pr tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// update-pr: +/// allowed-operations: +/// - add-reviewers +/// - set-auto-complete +/// allowed-repositories: +/// - self +/// allowed-votes: +/// - approve +/// - reject +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct UpdatePrConfig { + /// Which operations are permitted. Empty list means all operations are allowed. + #[serde(default, rename = "allowed-operations")] + pub allowed_operations: Vec, + + /// Which repositories the agent may target. Empty list means all allowed repos. + #[serde(default, rename = "allowed-repositories")] + pub allowed_repositories: Vec, + + /// Which vote values are permitted. Empty list means all votes are allowed. + #[serde(default, rename = "allowed-votes")] + pub allowed_votes: Vec, +} + +impl Default for UpdatePrConfig { + fn default() -> Self { + Self { + allowed_operations: Vec::new(), + allowed_repositories: Vec::new(), + allowed_votes: Vec::new(), + } + } +} + +/// Resolve the repository name for use in ADO API URLs. +/// +/// "self" (or None) → `ctx.repository_name`, otherwise look up in `ctx.allowed_repositories`. +fn resolve_repo_name( + repo_alias: Option<&str>, + ctx: &ExecutionContext, +) -> Result { + let alias = repo_alias.unwrap_or("self"); + if alias == "self" { + ctx.repository_name + .clone() + .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set")) + } else { + ctx.allowed_repositories + .get(alias) + .cloned() + .ok_or_else(|| { + ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed repository list", + alias + )) + }) + } +} + +#[async_trait::async_trait] +impl Executor for UpdatePrResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Updating PR #{} — operation: {}", + self.pull_request_id, self.operation + ); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + let organization = ctx + .ado_organization + .as_ref() + .context("Could not determine ADO organization name from URL")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: UpdatePrConfig = ctx.get_tool_config("update-pr"); + debug!("Config: {:?}", config); + + // Validate operation against allowed-operations + if !config.allowed_operations.is_empty() + && !config.allowed_operations.contains(&self.operation) + { + return Ok(ExecutionResult::failure(format!( + "Operation '{}' is not in the allowed-operations list: [{}]", + self.operation, + config.allowed_operations.join(", ") + ))); + } + + // Validate repository against allowed-repositories + let repo_alias = self.repository.as_deref().unwrap_or("self"); + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&repo_alias.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + repo_alias, + config.allowed_repositories.join(", ") + ))); + } + + // Resolve repo name + let repo_name = match resolve_repo_name(self.repository.as_deref(), ctx) { + Ok(name) => name, + Err(failure) => return Ok(failure), + }; + debug!("Resolved repository: {}", repo_name); + + let client = reqwest::Client::new(); + let encoded_project = utf8_percent_encode(project, PATH_SEGMENT).to_string(); + let base_url = format!( + "{}/{}/_apis/git/repositories", + org_url.trim_end_matches('/'), + encoded_project, + ); + + match self.operation.as_str() { + "set-auto-complete" => { + self.execute_set_auto_complete(&client, &base_url, &repo_name, token) + .await + } + "vote" => { + self.execute_vote( + &client, + &base_url, + &repo_name, + token, + organization, + &config, + ) + .await + } + "add-reviewers" => { + self.execute_add_reviewers( + &client, + &base_url, + &repo_name, + token, + organization, + ) + .await + } + "add-labels" => { + self.execute_add_labels(&client, &base_url, &repo_name, token) + .await + } + "update-description" => { + self.execute_update_description(&client, &base_url, &repo_name, token) + .await + } + _ => Ok(ExecutionResult::failure(format!( + "Unknown operation: {}", + self.operation + ))), + } + } +} + +impl UpdatePrResult { + /// Set auto-complete on a pull request. + /// + /// First fetches the PR to get the `createdBy.id`, then patches the PR + /// with `autoCompleteSetBy` and default completion options. + async fn execute_set_auto_complete( + &self, + client: &reqwest::Client, + base_url: &str, + repo_name: &str, + token: &str, + ) -> anyhow::Result { + let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); + + // Fetch the PR to get createdBy.id + let get_url = format!( + "{}/{}/pullRequests/{}?api-version=7.1", + base_url, encoded_repo, self.pull_request_id + ); + debug!("GET PR URL: {}", get_url); + + let pr_response = client + .get(&get_url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to fetch pull request")?; + + if !pr_response.status().is_success() { + let status = pr_response.status(); + let error_body = pr_response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to fetch PR #{} (HTTP {}): {}", + self.pull_request_id, status, error_body + ))); + } + + let pr_body: serde_json::Value = pr_response + .json() + .await + .context("Failed to parse PR response")?; + + let created_by_id = pr_body + .get("createdBy") + .and_then(|cb| cb.get("id")) + .and_then(|id| id.as_str()) + .context("PR response missing createdBy.id")?; + debug!("PR created by: {}", created_by_id); + + // PATCH to set auto-complete + let patch_url = format!( + "{}/{}/pullRequests/{}?api-version=7.1", + base_url, encoded_repo, self.pull_request_id + ); + let patch_body = serde_json::json!({ + "autoCompleteSetBy": { + "id": created_by_id + }, + "completionOptions": { + "deleteSourceBranch": true, + "mergeStrategy": "squash" + } + }); + + info!("Setting auto-complete on PR #{}", self.pull_request_id); + let response = client + .patch(&patch_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&patch_body) + .send() + .await + .context("Failed to set auto-complete on PR")?; + + if response.status().is_success() { + info!( + "Auto-complete set on PR #{}", + self.pull_request_id + ); + Ok(ExecutionResult::success_with_data( + format!("Auto-complete set on PR #{}", self.pull_request_id), + serde_json::json!({ + "pull_request_id": self.pull_request_id, + "operation": "set-auto-complete", + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + Ok(ExecutionResult::failure(format!( + "Failed to set auto-complete on PR #{} (HTTP {}): {}", + self.pull_request_id, status, error_body + ))) + } + } + + /// Submit a vote on a pull request. + /// + /// Resolves the current user identity via `_apis/connectiondata`, then + /// PUTs the vote to the reviewers endpoint. + async fn execute_vote( + &self, + client: &reqwest::Client, + base_url: &str, + repo_name: &str, + token: &str, + organization: &str, + config: &UpdatePrConfig, + ) -> anyhow::Result { + let vote_str = self + .vote + .as_deref() + .context("vote value is required for vote operation")?; + + // Validate against allowed-votes if configured + if !config.allowed_votes.is_empty() && !config.allowed_votes.contains(&vote_str.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Vote '{}' is not in the allowed-votes list: [{}]", + vote_str, + config.allowed_votes.join(", ") + ))); + } + + let vote_value = vote_to_ado_value(vote_str).context(format!( + "Invalid vote value: '{}'. Must be one of: {}", + vote_str, + VALID_VOTES.join(", ") + ))?; + + // Resolve the current user identity + let connection_url = format!( + "https://dev.azure.com/{}/_apis/connectiondata", + utf8_percent_encode(organization, PATH_SEGMENT) + ); + debug!("Connection data URL: {}", connection_url); + + let conn_response = client + .get(&connection_url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to fetch connection data")?; + + if !conn_response.status().is_success() { + let status = conn_response.status(); + let error_body = conn_response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to fetch connection data (HTTP {}): {}", + status, error_body + ))); + } + + let conn_body: serde_json::Value = conn_response + .json() + .await + .context("Failed to parse connection data response")?; + + let user_id = conn_body + .get("authenticatedUser") + .and_then(|au| au.get("id")) + .and_then(|id| id.as_str()) + .context("Connection data response missing authenticatedUser.id")?; + debug!("Authenticated user ID: {}", user_id); + + // PUT vote to reviewers endpoint + let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); + let vote_url = format!( + "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", + base_url, encoded_repo, self.pull_request_id, user_id + ); + let vote_body = serde_json::json!({ + "vote": vote_value + }); + + info!( + "Voting '{}' ({}) on PR #{}", + vote_str, vote_value, self.pull_request_id + ); + let response = client + .put(&vote_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&vote_body) + .send() + .await + .context("Failed to submit vote")?; + + if response.status().is_success() { + info!( + "Vote '{}' submitted on PR #{}", + vote_str, self.pull_request_id + ); + Ok(ExecutionResult::success_with_data( + format!( + "Vote '{}' submitted on PR #{}", + vote_str, self.pull_request_id + ), + serde_json::json!({ + "pull_request_id": self.pull_request_id, + "operation": "vote", + "vote": vote_str, + "vote_value": vote_value, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + Ok(ExecutionResult::failure(format!( + "Failed to submit vote on PR #{} (HTTP {}): {}", + self.pull_request_id, status, error_body + ))) + } + } + + /// Add reviewers to a pull request. + /// + /// For each reviewer email, PUTs to the reviewers endpoint with vote 0. + async fn execute_add_reviewers( + &self, + client: &reqwest::Client, + base_url: &str, + repo_name: &str, + token: &str, + _organization: &str, + ) -> anyhow::Result { + let reviewers = self + .reviewers + .as_ref() + .context("reviewers list is required for add-reviewers operation")?; + + let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); + let mut added = Vec::new(); + let mut failed = Vec::new(); + + for reviewer in reviewers { + let reviewer_url = format!( + "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", + base_url, + encoded_repo, + self.pull_request_id, + utf8_percent_encode(reviewer, PATH_SEGMENT), + ); + let reviewer_body = serde_json::json!({ + "vote": 0, + "isRequired": false + }); + + debug!("Adding reviewer '{}' to PR #{}", reviewer, self.pull_request_id); + let response = client + .put(&reviewer_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&reviewer_body) + .send() + .await; + + match response { + Ok(resp) if resp.status().is_success() => { + info!("Added reviewer '{}' to PR #{}", reviewer, self.pull_request_id); + added.push(reviewer.clone()); + } + Ok(resp) => { + let status = resp.status(); + let error_body = resp + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + warn!( + "Failed to add reviewer '{}' to PR #{} (HTTP {}): {}", + reviewer, self.pull_request_id, status, error_body + ); + failed.push(format!("{} (HTTP {})", reviewer, status)); + } + Err(e) => { + warn!( + "Request failed for reviewer '{}' on PR #{}: {}", + reviewer, self.pull_request_id, e + ); + failed.push(format!("{} (request error)", reviewer)); + } + } + } + + if added.is_empty() && !failed.is_empty() { + Ok(ExecutionResult::failure(format!( + "Failed to add any reviewers to PR #{}: {}", + self.pull_request_id, + failed.join(", ") + ))) + } else { + let mut message = format!( + "Added {} reviewer(s) to PR #{}", + added.len(), + self.pull_request_id + ); + if !failed.is_empty() { + message.push_str(&format!( + " ({} failed: {})", + failed.len(), + failed.join(", ") + )); + } + Ok(ExecutionResult::success_with_data( + message, + serde_json::json!({ + "pull_request_id": self.pull_request_id, + "operation": "add-reviewers", + "added": added, + "failed": failed, + }), + )) + } + } + + /// Add labels to a pull request. + /// + /// For each label, POSTs to the labels endpoint. + async fn execute_add_labels( + &self, + client: &reqwest::Client, + base_url: &str, + repo_name: &str, + token: &str, + ) -> anyhow::Result { + let labels = self + .labels + .as_ref() + .context("labels list is required for add-labels operation")?; + + let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); + let labels_url = format!( + "{}/{}/pullRequests/{}/labels?api-version=7.1", + base_url, encoded_repo, self.pull_request_id + ); + + let mut added = Vec::new(); + let mut failed = Vec::new(); + + for label in labels { + let label_body = serde_json::json!({ + "name": label + }); + + debug!( + "Adding label '{}' to PR #{}", + label, self.pull_request_id + ); + let response = client + .post(&labels_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&label_body) + .send() + .await; + + match response { + Ok(resp) if resp.status().is_success() => { + info!("Added label '{}' to PR #{}", label, self.pull_request_id); + added.push(label.clone()); + } + Ok(resp) => { + let status = resp.status(); + let error_body = resp + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + warn!( + "Failed to add label '{}' to PR #{} (HTTP {}): {}", + label, self.pull_request_id, status, error_body + ); + failed.push(format!("{} (HTTP {})", label, status)); + } + Err(e) => { + warn!( + "Request failed for label '{}' on PR #{}: {}", + label, self.pull_request_id, e + ); + failed.push(format!("{} (request error)", label)); + } + } + } + + if added.is_empty() && !failed.is_empty() { + Ok(ExecutionResult::failure(format!( + "Failed to add any labels to PR #{}: {}", + self.pull_request_id, + failed.join(", ") + ))) + } else { + let mut message = format!( + "Added {} label(s) to PR #{}", + added.len(), + self.pull_request_id + ); + if !failed.is_empty() { + message.push_str(&format!( + " ({} failed: {})", + failed.len(), + failed.join(", ") + )); + } + Ok(ExecutionResult::success_with_data( + message, + serde_json::json!({ + "pull_request_id": self.pull_request_id, + "operation": "add-labels", + "added": added, + "failed": failed, + }), + )) + } + } + + /// Update the description of a pull request. + async fn execute_update_description( + &self, + client: &reqwest::Client, + base_url: &str, + repo_name: &str, + token: &str, + ) -> anyhow::Result { + let description = self + .description + .as_ref() + .context("description is required for update-description operation")?; + + let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); + let patch_url = format!( + "{}/{}/pullRequests/{}?api-version=7.1", + base_url, encoded_repo, self.pull_request_id + ); + let patch_body = serde_json::json!({ + "description": description + }); + + info!( + "Updating description on PR #{} ({} chars)", + self.pull_request_id, + description.len() + ); + let response = client + .patch(&patch_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&patch_body) + .send() + .await + .context("Failed to update PR description")?; + + if response.status().is_success() { + info!("Description updated on PR #{}", self.pull_request_id); + Ok(ExecutionResult::success_with_data( + format!( + "Description updated on PR #{}", + self.pull_request_id + ), + serde_json::json!({ + "pull_request_id": self.pull_request_id, + "operation": "update-description", + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + Ok(ExecutionResult::failure(format!( + "Failed to update description on PR #{} (HTTP {}): {}", + self.pull_request_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(UpdatePrResult::NAME, "update-pr"); + } + + #[test] + fn test_params_deserializes() { + let json = r#"{ + "pull_request_id": 42, + "operation": "set-auto-complete" + }"#; + let params: UpdatePrParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.pull_request_id, 42); + assert_eq!(params.operation, "set-auto-complete"); + assert!(params.repository.is_none()); + } + + #[test] + fn test_params_converts_to_result() { + let params = UpdatePrParams { + pull_request_id: 42, + repository: Some("self".to_string()), + operation: "set-auto-complete".to_string(), + reviewers: None, + labels: None, + vote: None, + description: None, + }; + let result: UpdatePrResult = params.try_into().unwrap(); + assert_eq!(result.name, "update-pr"); + assert_eq!(result.pull_request_id, 42); + assert_eq!(result.operation, "set-auto-complete"); + } + + #[test] + fn test_validation_rejects_zero_pr_id() { + let params = UpdatePrParams { + pull_request_id: 0, + repository: None, + operation: "set-auto-complete".to_string(), + reviewers: None, + labels: None, + vote: None, + description: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_invalid_operation() { + let params = UpdatePrParams { + pull_request_id: 1, + repository: None, + operation: "delete-pr".to_string(), + reviewers: None, + labels: None, + vote: None, + description: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_vote_without_value() { + let params = UpdatePrParams { + pull_request_id: 1, + repository: None, + operation: "vote".to_string(), + reviewers: None, + labels: None, + vote: None, + description: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_reviewers_without_list() { + let params = UpdatePrParams { + pull_request_id: 1, + repository: None, + operation: "add-reviewers".to_string(), + reviewers: None, + labels: None, + vote: None, + description: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = UpdatePrParams { + pull_request_id: 99, + repository: Some("self".to_string()), + operation: "vote".to_string(), + reviewers: None, + labels: None, + vote: Some("approve".to_string()), + description: None, + }; + let result: UpdatePrResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"update-pr""#)); + assert!(json.contains(r#""pull_request_id":99"#)); + assert!(json.contains(r#""operation":"vote""#)); + } + + #[test] + fn test_config_defaults() { + let config = UpdatePrConfig::default(); + assert!(config.allowed_operations.is_empty()); + assert!(config.allowed_repositories.is_empty()); + assert!(config.allowed_votes.is_empty()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +allowed-operations: + - add-reviewers + - set-auto-complete +allowed-repositories: + - self +allowed-votes: + - approve + - reject +"#; + let config: UpdatePrConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.allowed_operations.len(), 2); + assert!(config.allowed_operations.contains(&"add-reviewers".to_string())); + assert!(config.allowed_operations.contains(&"set-auto-complete".to_string())); + assert_eq!(config.allowed_repositories.len(), 1); + assert_eq!(config.allowed_votes.len(), 2); + } +} diff --git a/src/tools/upload_attachment.rs b/src/tools/upload_attachment.rs new file mode 100644 index 00000000..488f170d --- /dev/null +++ b/src/tools/upload_attachment.rs @@ -0,0 +1,459 @@ +//! Upload attachment safe output tool + +use log::{debug, info, warn}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Parameters for uploading an attachment to a work item +#[derive(Deserialize, JsonSchema)] +pub struct UploadAttachmentParams { + /// The work item ID to attach the file to + pub work_item_id: i64, + + /// Path to the file in the workspace to upload. Must be a relative path with no directory traversal. + pub file_path: String, + + /// Optional description of the attachment. Must be at least 3 characters if provided. + pub comment: Option, +} + +impl Validate for UploadAttachmentParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!(self.work_item_id > 0, "work_item_id must be positive"); + ensure!(!self.file_path.is_empty(), "file_path must not be empty"); + ensure!( + !self.file_path.contains(".."), + "file_path must not contain '..'" + ); + ensure!( + !self.file_path.starts_with('/'), + "file_path must not be an absolute path" + ); + ensure!( + !self.file_path.contains(':'), + "file_path must not contain ':'" + ); + ensure!( + !self.file_path.contains('\0'), + "file_path must not contain null bytes" + ); + ensure!( + !self + .file_path + .split('/') + .any(|component| component == ".git"), + "file_path must not contain '.git' components" + ); + if let Some(comment) = &self.comment { + ensure!( + comment.len() >= 3, + "comment must be at least 3 characters" + ); + } + Ok(()) + } +} + +tool_result! { + name = "upload-attachment", + params = UploadAttachmentParams, + /// Result of uploading an attachment to a work item + pub struct UploadAttachmentResult { + work_item_id: i64, + file_path: String, + comment: Option, + } +} + +impl Sanitize for UploadAttachmentResult { + fn sanitize_fields(&mut self) { + if let Some(comment) = &self.comment { + self.comment = Some(sanitize_text(comment)); + } + } +} + +const DEFAULT_MAX_FILE_SIZE: u64 = 5 * 1024 * 1024; // 5 MB + +/// Configuration for the upload-attachment tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// upload-attachment: +/// max-file-size: 5242880 +/// allowed-extensions: +/// - .png +/// - .pdf +/// - .log +/// comment-prefix: "[Agent] " +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct UploadAttachmentConfig { + /// Maximum file size in bytes (default: 5 MB) + #[serde(default = "default_max_file_size", rename = "max-file-size")] + pub max_file_size: u64, + + /// Allowed file extensions (e.g., [".png", ".pdf"]). Empty means all extensions allowed. + #[serde(default, rename = "allowed-extensions")] + pub allowed_extensions: Vec, + + /// Prefix to prepend to the comment + #[serde(default, rename = "comment-prefix")] + pub comment_prefix: Option, +} + +fn default_max_file_size() -> u64 { + DEFAULT_MAX_FILE_SIZE +} + +impl Default for UploadAttachmentConfig { + fn default() -> Self { + Self { + max_file_size: DEFAULT_MAX_FILE_SIZE, + allowed_extensions: Vec::new(), + comment_prefix: None, + } + } +} + +#[async_trait::async_trait] +impl Executor for UploadAttachmentResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Uploading attachment '{}' to work item #{}", + self.file_path, self.work_item_id + ); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: UploadAttachmentConfig = ctx.get_tool_config("upload-attachment"); + debug!("Max file size: {} bytes", config.max_file_size); + debug!("Allowed extensions: {:?}", config.allowed_extensions); + + // Validate file extension against allowed-extensions (if configured) + if !config.allowed_extensions.is_empty() { + let has_valid_ext = config.allowed_extensions.iter().any(|ext| { + self.file_path + .to_lowercase() + .ends_with(&ext.to_lowercase()) + }); + if !has_valid_ext { + return Ok(ExecutionResult::failure(format!( + "File '{}' has an extension not in the allowed list: {:?}", + self.file_path, config.allowed_extensions + ))); + } + } + + // Resolve file path relative to source_directory + let resolved_path = ctx.source_directory.join(&self.file_path); + debug!("Resolved file path: {}", resolved_path.display()); + + // Check file exists + if !resolved_path.exists() { + return Ok(ExecutionResult::failure(format!( + "File not found: {}", + resolved_path.display() + ))); + } + + // Check file size + let metadata = std::fs::metadata(&resolved_path) + .context("Failed to read file metadata")?; + let file_size = metadata.len(); + debug!("File size: {} bytes", file_size); + if file_size > config.max_file_size { + return Ok(ExecutionResult::failure(format!( + "File size ({} bytes) exceeds maximum allowed size ({} bytes)", + file_size, config.max_file_size + ))); + } + + // Read file bytes + let file_bytes = + std::fs::read(&resolved_path).context("Failed to read file contents")?; + + // Check if file is text (not binary) — if text, scan for ##vso[ command injection + if let Ok(text) = std::str::from_utf8(&file_bytes) { + if text.contains("##vso[") { + return Ok(ExecutionResult::failure(format!( + "File '{}' contains '##vso[' command injection sequence", + self.file_path + ))); + } + } + + // Extract filename for upload + let filename = std::path::Path::new(&self.file_path) + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or("attachment"); + debug!("Upload filename: {}", filename); + + // Apply comment-prefix to comment if configured + let effective_comment = match (&self.comment, &config.comment_prefix) { + (Some(c), Some(prefix)) => format!("{}{}", prefix, c), + (Some(c), None) => c.clone(), + (None, _) => "Uploaded by agent".to_string(), + }; + debug!("Effective comment: {}", effective_comment); + + let client = reqwest::Client::new(); + + // Step 1: Upload file + // POST {org_url}/{project}/_apis/wit/attachments?fileName={filename}&api-version=7.1 + let upload_url = format!( + "{}/{}/_apis/wit/attachments?fileName={}&api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(filename, PATH_SEGMENT), + ); + debug!("Upload URL: {}", upload_url); + + info!("Uploading file '{}' ({} bytes)", filename, file_size); + let upload_response = client + .post(&upload_url) + .header("Content-Type", "application/octet-stream") + .basic_auth("", Some(token)) + .body(file_bytes) + .send() + .await + .context("Failed to upload attachment to Azure DevOps")?; + + if !upload_response.status().is_success() { + let status = upload_response.status(); + let error_body = upload_response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to upload attachment (HTTP {}): {}", + status, error_body + ))); + } + + let upload_body: serde_json::Value = upload_response + .json() + .await + .context("Failed to parse upload response JSON")?; + + let attachment_url = upload_body + .get("url") + .and_then(|v| v.as_str()) + .context("Upload response missing 'url' field")? + .to_string(); + debug!("Attachment URL: {}", attachment_url); + + // Step 2: Link attachment to work item + // PATCH {org_url}/{project}/_apis/wit/workitems/{work_item_id}?api-version=7.1 + let link_url = format!( + "{}/{}/_apis/wit/workitems/{}?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + self.work_item_id, + ); + debug!("Link URL: {}", link_url); + + let patch_doc = serde_json::json!([{ + "op": "add", + "path": "/relations/-", + "value": { + "rel": "AttachedFile", + "url": attachment_url, + "attributes": { + "comment": effective_comment, + } + } + }]); + + info!( + "Linking attachment to work item #{}", + self.work_item_id + ); + let link_response = client + .patch(&link_url) + .header("Content-Type", "application/json-patch+json") + .basic_auth("", Some(token)) + .json(&patch_doc) + .send() + .await + .context("Failed to link attachment to work item")?; + + if link_response.status().is_success() { + info!( + "Attachment '{}' linked to work item #{}", + filename, self.work_item_id + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Uploaded '{}' and linked to work item #{}", + filename, self.work_item_id + ), + serde_json::json!({ + "work_item_id": self.work_item_id, + "file_path": self.file_path, + "attachment_url": attachment_url, + "project": project, + }), + )) + } else { + let status = link_response.status(); + let error_body = link_response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + warn!( + "Attachment uploaded but linking failed — the attachment at {} is orphaned", + attachment_url + ); + + Ok(ExecutionResult::failure(format!( + "Attachment uploaded but failed to link to work item #{} (HTTP {}): {}", + self.work_item_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(UploadAttachmentResult::NAME, "upload-attachment"); + } + + #[test] + fn test_params_deserializes() { + let json = + r#"{"work_item_id": 42, "file_path": "output/report.pdf", "comment": "Weekly report"}"#; + let params: UploadAttachmentParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.work_item_id, 42); + assert_eq!(params.file_path, "output/report.pdf"); + assert_eq!(params.comment, Some("Weekly report".to_string())); + } + + #[test] + fn test_params_converts_to_result() { + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "output/report.pdf".to_string(), + comment: Some("Weekly report".to_string()), + }; + let result: UploadAttachmentResult = params.try_into().unwrap(); + assert_eq!(result.name, "upload-attachment"); + assert_eq!(result.work_item_id, 42); + assert_eq!(result.file_path, "output/report.pdf"); + assert_eq!(result.comment, Some("Weekly report".to_string())); + } + + #[test] + fn test_validation_rejects_zero_work_item_id() { + let params = UploadAttachmentParams { + work_item_id: 0, + file_path: "output/report.pdf".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_empty_file_path() { + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_path_traversal() { + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "../etc/passwd".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_absolute_path() { + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "/etc/passwd".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "output/report.pdf".to_string(), + comment: Some("Test attachment".to_string()), + }; + let result: UploadAttachmentResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"upload-attachment""#)); + assert!(json.contains(r#""work_item_id":42"#)); + assert!(json.contains(r#""file_path":"output/report.pdf""#)); + } + + #[test] + fn test_config_defaults() { + let config = UploadAttachmentConfig::default(); + assert_eq!(config.max_file_size, 5 * 1024 * 1024); + assert!(config.allowed_extensions.is_empty()); + assert!(config.comment_prefix.is_none()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +max-file-size: 1048576 +allowed-extensions: + - .png + - .pdf + - .log +comment-prefix: "[Agent] " +"#; + let config: UploadAttachmentConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.max_file_size, 1_048_576); + assert_eq!( + config.allowed_extensions, + vec![".png", ".pdf", ".log"] + ); + assert_eq!(config.comment_prefix, Some("[Agent] ".to_string())); + } +} From 1992ad66201050033d78972ef149d8017252ad82 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 8 Apr 2026 14:34:20 +0100 Subject: [PATCH 02/15] fix: address PR review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug fixes: - create-git-tag: resolve HEAD via repository's defaultBranch instead of hardcoding 'heads/main' (works with master, develop, etc.) - update-pr: resolve reviewer emails to GUIDs via VSSPS identity API before adding to PR (ADO reviewers endpoint requires user ID, not email) Security: - update-pr: require 'allowed-votes' to be explicitly configured for vote operation — empty list now rejects all votes to prevent accidental auto-approve by agents Improvements: - update-pr: make auto-complete merge options configurable via front matter (delete-source-branch, merge-strategy) instead of hardcoding squash+delete - add-pr-comment: validate file_path in Validate::validate() at Stage 1 (was only validated at Stage 2, giving no feedback to agent) - queue-build: sanitize template parameter keys and values to prevent ##vso[] command injection in build logs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/tools/add_pr_comment.rs | 3 ++ src/tools/create_git_tag.rs | 49 +++++++++++++++++-- src/tools/queue_build.rs | 8 +++ src/tools/update_pr.rs | 98 +++++++++++++++++++++++++++++++++---- 4 files changed, 146 insertions(+), 12 deletions(-) diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index 1f4f015f..ff20b11c 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -59,6 +59,9 @@ impl Validate for AddPrCommentParams { "line requires file_path to be set" ); } + if let Some(fp) = &self.file_path { + validate_file_path(fp)?; + } Ok(()) } } diff --git a/src/tools/create_git_tag.rs b/src/tools/create_git_tag.rs index 4a0f6623..abe013eb 100644 --- a/src/tools/create_git_tag.rs +++ b/src/tools/create_git_tag.rs @@ -146,7 +146,7 @@ impl Default for CreateGitTagConfig { } } -/// Resolve HEAD of the default branch for a repository via the ADO refs API. +/// Resolve HEAD commit for a repository by querying the repository's default branch. async fn resolve_head_commit( client: &reqwest::Client, org_url: &str, @@ -154,11 +154,51 @@ async fn resolve_head_commit( token: &str, repo_name: &str, ) -> anyhow::Result { + // First, discover the default branch from the repository metadata + let repo_url = format!( + "{}/{}/_apis/git/repositories/{}?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(repo_name, PATH_SEGMENT), + ); + debug!("Fetching repository metadata: {}", repo_url); + + let repo_response = client + .get(&repo_url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to query repository metadata")?; + + ensure!( + repo_response.status().is_success(), + "Failed to fetch repository metadata (HTTP {})", + repo_response.status() + ); + + let repo_body: serde_json::Value = repo_response + .json() + .await + .context("Failed to parse repository metadata")?; + + let default_branch = repo_body + .get("defaultBranch") + .and_then(|v| v.as_str()) + .unwrap_or("refs/heads/main"); + + // Strip "refs/heads/" prefix for the filter parameter + let branch_filter = default_branch + .strip_prefix("refs/") + .unwrap_or(default_branch); + debug!("Default branch: {} (filter: {})", default_branch, branch_filter); + + // Now resolve the HEAD commit of the default branch let url = format!( - "{}/{}/_apis/git/repositories/{}/refs?filter=heads/main&api-version=7.1", + "{}/{}/_apis/git/repositories/{}/refs?filter={}&api-version=7.1", org_url.trim_end_matches('/'), utf8_percent_encode(project, PATH_SEGMENT), utf8_percent_encode(repo_name, PATH_SEGMENT), + branch_filter, ); debug!("Resolving HEAD commit via: {}", url); @@ -187,7 +227,10 @@ async fn resolve_head_commit( .and_then(|r| r.get("objectId")) .and_then(|v| v.as_str()) .map(|s| s.to_string()) - .context("No refs found for heads/main — cannot resolve HEAD commit") + .context(format!( + "No refs found for {} — cannot resolve HEAD commit", + default_branch + )) } #[async_trait::async_trait] diff --git a/src/tools/queue_build.rs b/src/tools/queue_build.rs index 09e4c63a..a2fc75fe 100644 --- a/src/tools/queue_build.rs +++ b/src/tools/queue_build.rs @@ -67,6 +67,14 @@ impl Sanitize for QueueBuildResult { if let Some(reason) = &self.reason { self.reason = Some(sanitize_text(reason)); } + if let Some(params) = &self.parameters { + self.parameters = Some( + params + .iter() + .map(|(k, v)| (sanitize_text(k), sanitize_text(v))) + .collect(), + ); + } } } diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index 7cf8525f..fcb959f3 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -185,9 +185,26 @@ pub struct UpdatePrConfig { #[serde(default, rename = "allowed-repositories")] pub allowed_repositories: Vec, - /// Which vote values are permitted. Empty list means all votes are allowed. + /// Which vote values are permitted. REQUIRED for vote operation — + /// empty list rejects all votes to prevent accidental auto-approve. #[serde(default, rename = "allowed-votes")] pub allowed_votes: Vec, + + /// Whether to delete the source branch after merge (for set-auto-complete, default: true) + #[serde(default = "default_true", rename = "delete-source-branch")] + pub delete_source_branch: bool, + + /// Merge strategy for auto-complete: "squash", "noFastForward", "rebase", "rebaseMerge" (default: "squash") + #[serde(default = "default_merge_strategy", rename = "merge-strategy")] + pub merge_strategy: String, +} + +fn default_true() -> bool { + true +} + +fn default_merge_strategy() -> String { + "squash".to_string() } impl Default for UpdatePrConfig { @@ -196,6 +213,8 @@ impl Default for UpdatePrConfig { allowed_operations: Vec::new(), allowed_repositories: Vec::new(), allowed_votes: Vec::new(), + delete_source_branch: true, + merge_strategy: "squash".to_string(), } } } @@ -294,7 +313,7 @@ impl Executor for UpdatePrResult { match self.operation.as_str() { "set-auto-complete" => { - self.execute_set_auto_complete(&client, &base_url, &repo_name, token) + self.execute_set_auto_complete(&client, &base_url, &repo_name, token, &config) .await } "vote" => { @@ -345,6 +364,7 @@ impl UpdatePrResult { base_url: &str, repo_name: &str, token: &str, + config: &UpdatePrConfig, ) -> anyhow::Result { let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); @@ -396,8 +416,8 @@ impl UpdatePrResult { "id": created_by_id }, "completionOptions": { - "deleteSourceBranch": true, - "mergeStrategy": "squash" + "deleteSourceBranch": config.delete_source_branch, + "mergeStrategy": config.merge_strategy } }); @@ -454,8 +474,17 @@ impl UpdatePrResult { .as_deref() .context("vote value is required for vote operation")?; - // Validate against allowed-votes if configured - if !config.allowed_votes.is_empty() && !config.allowed_votes.contains(&vote_str.to_string()) + // Validate against allowed-votes — REQUIRED for vote operation. + // An empty allowed-votes list means the operator hasn't opted in, so reject. + if config.allowed_votes.is_empty() { + return Ok(ExecutionResult::failure( + "vote operation requires 'allowed-votes' to be configured in safe-outputs.update-pr. \ + This prevents agents from casting unrestricted votes (including approve). \ + Example:\n safe-outputs:\n update-pr:\n allowed-votes:\n - approve-with-suggestions\n - wait-for-author" + .to_string(), + )); + } + if !config.allowed_votes.contains(&vote_str.to_string()) { return Ok(ExecutionResult::failure(format!( "Vote '{}' is not in the allowed-votes list: [{}]", @@ -563,14 +592,15 @@ impl UpdatePrResult { /// Add reviewers to a pull request. /// - /// For each reviewer email, PUTs to the reviewers endpoint with vote 0. + /// For each reviewer email, resolves the identity via VSSPS, then PUTs to + /// the reviewers endpoint with vote 0. async fn execute_add_reviewers( &self, client: &reqwest::Client, base_url: &str, repo_name: &str, token: &str, - _organization: &str, + organization: &str, ) -> anyhow::Result { let reviewers = self .reviewers @@ -582,12 +612,62 @@ impl UpdatePrResult { let mut failed = Vec::new(); for reviewer in reviewers { + // Resolve reviewer email to GUID via VSSPS identity API + let identity_url = format!( + "https://vssps.dev.azure.com/{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", + utf8_percent_encode(organization, PATH_SEGMENT), + utf8_percent_encode(reviewer, PATH_SEGMENT), + ); + debug!("Resolving identity for '{}': {}", reviewer, identity_url); + + let identity_response = client + .get(&identity_url) + .basic_auth("", Some(token)) + .send() + .await; + + let reviewer_id = match identity_response { + Ok(resp) if resp.status().is_success() => { + let body: serde_json::Value = resp.json().await.unwrap_or_default(); + body.get("value") + .and_then(|v| v.as_array()) + .and_then(|arr| arr.first()) + .and_then(|entry| entry.get("id")) + .and_then(|id| id.as_str()) + .map(|s| s.to_string()) + } + Ok(resp) => { + warn!( + "Identity lookup for '{}' returned HTTP {}", + reviewer, + resp.status() + ); + None + } + Err(e) => { + warn!("Identity lookup for '{}' failed: {}", reviewer, e); + None + } + }; + + let reviewer_id = match reviewer_id { + Some(id) => id, + None => { + warn!( + "Could not resolve identity for '{}', skipping", + reviewer + ); + failed.push(format!("{} (identity not found)", reviewer)); + continue; + } + }; + let reviewer_url = format!( "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", base_url, encoded_repo, self.pull_request_id, - utf8_percent_encode(reviewer, PATH_SEGMENT), + reviewer_id, ); let reviewer_body = serde_json::json!({ "vote": 0, From 3dcd4c9bf789919347aad3efbe8d421abcecc2c9 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 8 Apr 2026 14:53:15 +0100 Subject: [PATCH 03/15] =?UTF-8?q?feat:=20align=20with=20gh-aw=20=E2=80=94?= =?UTF-8?q?=20add=20review=20tools=20and=20enhancements?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds 4 new safe output tools to align with gh-aw's PR review capabilities: - submit-pr-review: Submit PR review with decision (approve, request-changes, comment) and optional body/rationale text. Requires allowed-events config. - reply-to-pr-review-comment: Reply to existing review comment threads - resolve-pr-review-thread: Resolve/reactivate review threads (fixed, wont-fix, closed, by-design, active) - report-incomplete: Signal task could not complete due to infrastructure failure Enhancements to existing tools: - add-pr-comment: Add start_line param for multi-line code comments (gh-aw parity) - link-work-items: Set DEFAULT_MAX=5 (matches gh-aw link-sub-issue default) - queue-build: Set DEFAULT_MAX=3 (prevents pipeline queue flooding) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 3 + src/execute.rs | 38 +++ src/mcp.rs | 96 +++++- src/tools/add_pr_comment.rs | 32 +- src/tools/link_work_items.rs | 1 + src/tools/mod.rs | 8 + src/tools/queue_build.rs | 1 + src/tools/reply_to_pr_comment.rs | 344 ++++++++++++++++++++ src/tools/report_incomplete.rs | 105 ++++++ src/tools/resolve_pr_thread.rs | 402 +++++++++++++++++++++++ src/tools/submit_pr_review.rs | 526 +++++++++++++++++++++++++++++++ 11 files changed, 1552 insertions(+), 4 deletions(-) create mode 100644 src/tools/reply_to_pr_comment.rs create mode 100644 src/tools/report_incomplete.rs create mode 100644 src/tools/resolve_pr_thread.rs create mode 100644 src/tools/submit_pr_review.rs diff --git a/src/compile/common.rs b/src/compile/common.rs index a2bc67fa..d145cf74 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -702,6 +702,9 @@ const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = &[ "create-branch", "update-pr", "upload-attachment", + "submit-pr-review", + "reply-to-pr-review-comment", + "resolve-pr-review-thread", ]; /// Validate that write-requiring safe-outputs have a write service connection configured. diff --git a/src/execute.rs b/src/execute.rs index 4dece425..30fdad16 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -14,6 +14,7 @@ use crate::tools::{ AddBuildTagResult, AddPrCommentResult, CreateBranchResult, CreateGitTagResult, CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, CommentOnWorkItemResult, ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, QueueBuildResult, + ReplyToPrCommentResult, ResolvePrThreadResult, SubmitPrReviewResult, ToolResult, UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadAttachmentResult, }; @@ -123,6 +124,9 @@ pub async fn execute_safe_outputs( CreateBranchResult, UpdatePrResult, UploadAttachmentResult, + SubmitPrReviewResult, + ReplyToPrCommentResult, + ResolvePrThreadResult, ); let mut results = Vec::new(); @@ -343,10 +347,44 @@ pub async fn execute_safe_output( ); output.execute_sanitized(ctx).await? } + "submit-pr-review" => { + debug!("Parsing submit-pr-review payload"); + let mut output: SubmitPrReviewResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse submit-pr-review: {}", e))?; + debug!( + "submit-pr-review: pr_id={}, event='{}'", + output.pull_request_id, output.event + ); + output.execute_sanitized(ctx).await? + } + "reply-to-pr-review-comment" => { + debug!("Parsing reply-to-pr-review-comment payload"); + let mut output: ReplyToPrCommentResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse reply-to-pr-review-comment: {}", e))?; + debug!( + "reply-to-pr-review-comment: pr_id={}, thread_id={}", + output.pull_request_id, output.thread_id + ); + output.execute_sanitized(ctx).await? + } + "resolve-pr-review-thread" => { + debug!("Parsing resolve-pr-review-thread payload"); + let mut output: ResolvePrThreadResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse resolve-pr-review-thread: {}", e))?; + debug!( + "resolve-pr-review-thread: pr_id={}, thread_id={}, status='{}'", + output.pull_request_id, output.thread_id, output.status + ); + output.execute_sanitized(ctx).await? + } "noop" => { debug!("Skipping noop entry"); ExecutionResult::success("Skipped informational output: noop") } + "report-incomplete" => { + debug!("Skipping report-incomplete entry"); + ExecutionResult::success("Skipped informational output: report-incomplete") + } "missing-tool" => { debug!("Skipping missing-tool entry"); ExecutionResult::success("Skipped informational output: missing-tool") diff --git a/src/mcp.rs b/src/mcp.rs index 2f05d51b..3b94bec2 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -19,9 +19,12 @@ use crate::tools::{ CreatePrParams, CreatePrResult, CreateWikiPageParams, CreateWikiPageResult, CreateWorkItemParams, CreateWorkItemResult, LinkWorkItemsParams, LinkWorkItemsResult, + ReplyToPrCommentParams, ReplyToPrCommentResult, + ReportIncompleteParams, ReportIncompleteResult, + ResolvePrThreadParams, ResolvePrThreadResult, UpdateWikiPageParams, UpdateWikiPageResult, MissingDataParams, MissingDataResult, MissingToolParams, MissingToolResult, NoopParams, NoopResult, QueueBuildParams, - QueueBuildResult, ToolResult, + QueueBuildResult, SubmitPrReviewParams, SubmitPrReviewResult, ToolResult, UpdatePrParams, UpdatePrResult, UpdateWorkItemParams, UpdateWorkItemResult, UploadAttachmentParams, UploadAttachmentResult, @@ -732,6 +735,97 @@ uploaded and linked during safe output processing. File size and type restrictio result.file_path, result.work_item_id ))])) } + + #[tool( + name = "submit-pr-review", + description = "Submit a pull request review with a decision (approve, request-changes, \ +or comment-only) and an optional body explaining the rationale. The review will be \ +submitted during safe output processing. Requires 'allowed-events' to be configured." + )] + async fn submit_pr_review( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: submit-pr-review - PR #{} event '{}'", + params.0.pull_request_id, params.0.event + ); + let mut sanitized = params.0; + sanitized.body = sanitized.body.map(|b| sanitize_text(&b)); + let result: SubmitPrReviewResult = sanitized.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "PR review '{}' queued for PR #{}. The review will be submitted during safe output processing.", + result.event, result.pull_request_id + ))])) + } + + #[tool( + name = "reply-to-pr-review-comment", + description = "Reply to an existing review comment thread on an Azure DevOps pull request. \ +Provide the PR ID, thread ID, and reply content. The reply will be posted during safe output processing." + )] + async fn reply_to_pr_review_comment( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: reply-to-pr-review-comment - PR #{} thread #{}", + params.0.pull_request_id, params.0.thread_id + ); + let mut sanitized = params.0; + sanitized.content = sanitize_text(&sanitized.content); + let result: ReplyToPrCommentResult = sanitized.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "Reply queued for thread #{} on PR #{}. The reply will be posted during safe output processing.", + result.thread_id, result.pull_request_id + ))])) + } + + #[tool( + name = "resolve-pr-review-thread", + description = "Resolve or change the status of a review thread on an Azure DevOps pull request. \ +Valid statuses: fixed, wont-fix, closed, by-design, active. \ +The status change will be applied during safe output processing." + )] + async fn resolve_pr_review_thread( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: resolve-pr-review-thread - PR #{} thread #{} → '{}'", + params.0.pull_request_id, params.0.thread_id, params.0.status + ); + let result: ResolvePrThreadResult = params.0.try_into()?; + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + Ok(CallToolResult::success(vec![Content::text(format!( + "Thread #{} status change to '{}' queued for PR #{}. The change will be applied during safe output processing.", + result.thread_id, result.status, result.pull_request_id + ))])) + } + + #[tool( + name = "report-incomplete", + description = "Signal that the task could not be completed due to infrastructure failure, \ +tool errors, or other environmental issues beyond the agent's control. Use this when the \ +agent attempted work but couldn't finish (e.g., API timeouts, build failures, resource limits)." + )] + async fn report_incomplete( + &self, + params: Parameters, + ) -> Result { + warn!("Tool called: report-incomplete - '{}'", params.0.reason); + let mut sanitized = params.0; + sanitized.reason = sanitize_text(&sanitized.reason); + sanitized.context = sanitized.context.map(|c| sanitize_text(&c)); + let result: ReportIncompleteResult = sanitized.try_into()?; + let _ = self.write_safe_output_file(&result).await; + Ok(CallToolResult::success(vec![])) + } } // Implement the server handler diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index ff20b11c..ae78b4ae 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -29,6 +29,11 @@ pub struct AddPrCommentParams { #[serde(default)] pub file_path: Option, + /// Starting line number for a multi-line inline comment. Requires `file_path` and `line`. + /// When set, the comment spans from `start_line` to `line`. + #[serde(default)] + pub start_line: Option, + /// Line number for an inline comment. Requires `file_path` to be set. #[serde(default)] pub line: Option, @@ -59,6 +64,20 @@ impl Validate for AddPrCommentParams { "line requires file_path to be set" ); } + if self.start_line.is_some() { + ensure!( + self.line.is_some(), + "start_line requires line to be set" + ); + if let (Some(start), Some(end)) = (self.start_line, self.line) { + ensure!( + start < end, + "start_line ({}) must be less than line ({})", + start, + end + ); + } + } if let Some(fp) = &self.file_path { validate_file_path(fp)?; } @@ -75,6 +94,7 @@ tool_result! { content: String, repository: String, file_path: Option, + start_line: Option, line: Option, status: String, } @@ -271,11 +291,12 @@ impl Executor for AddPrCommentResult { // Add thread context for inline comments if let Some(ref fp) = self.file_path { - let line_num = self.line.unwrap_or(1); + let end_line = self.line.unwrap_or(1); + let start_line = self.start_line.unwrap_or(end_line); thread_body["threadContext"] = serde_json::json!({ "filePath": format!("/{}", fp), - "rightFileStart": { "line": line_num, "offset": 1 }, - "rightFileEnd": { "line": line_num, "offset": 1 } + "rightFileStart": { "line": start_line, "offset": 1 }, + "rightFileEnd": { "line": end_line, "offset": 1 } }); } @@ -361,6 +382,7 @@ mod tests { content: "This is a test comment with enough characters.".to_string(), repository: "self".to_string(), file_path: None, + start_line: None, line: None, status: "Active".to_string(), }; @@ -377,6 +399,7 @@ mod tests { content: "This is a valid comment body text.".to_string(), repository: "self".to_string(), file_path: None, + start_line: None, line: None, status: "Active".to_string(), }; @@ -391,6 +414,7 @@ mod tests { content: "Too short".to_string(), repository: "self".to_string(), file_path: None, + start_line: None, line: None, status: "Active".to_string(), }; @@ -405,6 +429,7 @@ mod tests { content: "This is a valid comment body text.".to_string(), repository: "self".to_string(), file_path: None, + start_line: None, line: Some(10), status: "Active".to_string(), }; @@ -419,6 +444,7 @@ mod tests { content: "A comment body that is definitely longer than ten characters.".to_string(), repository: "self".to_string(), file_path: Some("src/main.rs".to_string()), + start_line: None, line: Some(10), status: "Active".to_string(), }; diff --git a/src/tools/link_work_items.rs b/src/tools/link_work_items.rs index 0e81e866..4410ae66 100644 --- a/src/tools/link_work_items.rs +++ b/src/tools/link_work_items.rs @@ -79,6 +79,7 @@ impl Validate for LinkWorkItemsParams { tool_result! { name = "link-work-items", params = LinkWorkItemsParams, + default_max = 5, /// Result of linking two work items pub struct LinkWorkItemsResult { source_id: i64, diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 2fd10330..c77f4689 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -118,7 +118,11 @@ mod missing_data; mod missing_tool; mod noop; mod queue_build; +mod reply_to_pr_comment; +mod report_incomplete; +mod resolve_pr_thread; mod result; +mod submit_pr_review; mod update_pr; mod update_wiki_page; mod update_work_item; @@ -137,9 +141,13 @@ pub use missing_data::*; pub use missing_tool::*; pub use noop::*; pub use queue_build::*; +pub use reply_to_pr_comment::*; +pub use report_incomplete::*; +pub use resolve_pr_thread::*; pub use result::{ ExecutionContext, ExecutionResult, Executor, ToolResult, Validate, anyhow_to_mcp_error, }; +pub use submit_pr_review::*; pub use update_pr::*; pub use update_wiki_page::*; pub use update_work_item::*; diff --git a/src/tools/queue_build.rs b/src/tools/queue_build.rs index a2fc75fe..df0fac7b 100644 --- a/src/tools/queue_build.rs +++ b/src/tools/queue_build.rs @@ -53,6 +53,7 @@ impl Validate for QueueBuildParams { tool_result! { name = "queue-build", params = QueueBuildParams, + default_max = 3, /// Result of queuing a build pub struct QueueBuildResult { pipeline_id: i32, diff --git a/src/tools/reply_to_pr_comment.rs b/src/tools/reply_to_pr_comment.rs new file mode 100644 index 00000000..8355bcb8 --- /dev/null +++ b/src/tools/reply_to_pr_comment.rs @@ -0,0 +1,344 @@ +//! Reply to PR review comment safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Parameters for replying to an existing review comment thread on a pull request +#[derive(Deserialize, JsonSchema)] +pub struct ReplyToPrCommentParams { + /// The pull request ID containing the thread + pub pull_request_id: i32, + + /// The thread ID to reply to + pub thread_id: i32, + + /// Reply text in markdown format. Ensure adequate content > 10 characters. + pub content: String, + + /// Repository alias: "self" for pipeline repo, or an alias from the checkout list. + /// Defaults to "self" if omitted. + #[serde(default = "default_repository")] + pub repository: Option, +} + +fn default_repository() -> Option { + Some("self".to_string()) +} + +impl Validate for ReplyToPrCommentParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!(self.pull_request_id > 0, "pull_request_id must be positive"); + ensure!(self.thread_id > 0, "thread_id must be positive"); + ensure!( + self.content.len() >= 10, + "content must be at least 10 characters" + ); + Ok(()) + } +} + +tool_result! { + name = "reply-to-pr-review-comment", + params = ReplyToPrCommentParams, + /// Result of replying to a review comment thread on a pull request + pub struct ReplyToPrCommentResult { + pull_request_id: i32, + thread_id: i32, + content: String, + repository: Option, + } +} + +impl Sanitize for ReplyToPrCommentResult { + fn sanitize_fields(&mut self) { + self.content = sanitize_text(&self.content); + } +} + +/// Configuration for the reply-to-pr-review-comment tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// reply-to-pr-review-comment: +/// comment-prefix: "[Agent] " +/// allowed-repositories: +/// - self +/// - other-repo +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReplyToPrCommentConfig { + /// Prefix prepended to all replies (e.g., "[Agent] ") + #[serde(default, rename = "comment-prefix")] + pub comment_prefix: Option, + + /// Restrict which repositories the agent can reply on. + /// If empty, all repositories in the checkout list (plus "self") are allowed. + #[serde(default, rename = "allowed-repositories")] + pub allowed_repositories: Vec, +} + +impl Default for ReplyToPrCommentConfig { + fn default() -> Self { + Self { + comment_prefix: None, + allowed_repositories: Vec::new(), + } + } +} + +#[async_trait::async_trait] +impl Executor for ReplyToPrCommentResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Replying to PR #{} thread #{}: {} chars", + self.pull_request_id, + self.thread_id, + self.content.len() + ); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: ReplyToPrCommentConfig = ctx.get_tool_config("reply-to-pr-review-comment"); + debug!("Config: {:?}", config); + + let repository = self + .repository + .as_deref() + .unwrap_or("self"); + + // Validate repository against allowed-repositories config + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&repository.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list", + repository + ))); + } + + // Determine the repository name for the API call + let repo_name = if repository == "self" || repository.is_empty() { + ctx.repository_name + .as_ref() + .context("BUILD_REPOSITORY_NAME not set and repository is 'self'")? + .clone() + } else { + match ctx.allowed_repositories.get(repository) { + Some(name) => name.clone(), + None => { + return Ok(ExecutionResult::failure(format!( + "Repository alias '{}' not found in allowed repositories", + repository + ))); + } + } + }; + + // Build comment content with optional prefix + let comment_body = match &config.comment_prefix { + Some(prefix) => format!("{}{}", prefix, self.content), + None => self.content.clone(), + }; + + // Build the API URL for adding a comment to an existing thread + let url = format!( + "{}/{}/_apis/git/repositories/{}/pullRequests/{}/threads/{}/comments?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(&repo_name, PATH_SEGMENT), + self.pull_request_id, + self.thread_id, + ); + debug!("API URL: {}", url); + + let request_body = serde_json::json!({ + "parentCommentId": 1, + "content": comment_body, + "commentType": 1 + }); + + let client = reqwest::Client::new(); + + info!( + "Sending reply to PR #{} thread #{}", + self.pull_request_id, self.thread_id + ); + let response = client + .post(&url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&request_body) + .send() + .await + .context("Failed to send request to Azure DevOps")?; + + if response.status().is_success() { + let body: serde_json::Value = response + .json() + .await + .context("Failed to parse response JSON")?; + + let comment_id = body.get("id").and_then(|v| v.as_i64()).unwrap_or(0); + + info!( + "Reply added to PR #{} thread #{}: comment #{}", + self.pull_request_id, self.thread_id, comment_id + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Added reply #{} to PR #{} thread #{}", + comment_id, self.pull_request_id, self.thread_id + ), + serde_json::json!({ + "comment_id": comment_id, + "pull_request_id": self.pull_request_id, + "thread_id": self.thread_id, + "repository": repo_name, + "project": project, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + Ok(ExecutionResult::failure(format!( + "Failed to reply to PR #{} thread #{} (HTTP {}): {}", + self.pull_request_id, self.thread_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(ReplyToPrCommentResult::NAME, "reply-to-pr-review-comment"); + } + + #[test] + fn test_params_deserializes() { + let json = + r#"{"pull_request_id": 42, "thread_id": 7, "content": "This is a reply to the review comment."}"#; + let params: ReplyToPrCommentParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.pull_request_id, 42); + assert_eq!(params.thread_id, 7); + assert!(params.content.contains("reply")); + assert_eq!(params.repository, Some("self".to_string())); + } + + #[test] + fn test_params_converts_to_result() { + let params = ReplyToPrCommentParams { + pull_request_id: 42, + thread_id: 7, + content: "This is a test reply with enough characters.".to_string(), + repository: Some("self".to_string()), + }; + let result: ReplyToPrCommentResult = params.try_into().unwrap(); + assert_eq!(result.name, "reply-to-pr-review-comment"); + assert_eq!(result.pull_request_id, 42); + assert_eq!(result.thread_id, 7); + assert!(result.content.contains("test reply")); + } + + #[test] + fn test_validation_rejects_zero_pr_id() { + let params = ReplyToPrCommentParams { + pull_request_id: 0, + thread_id: 7, + content: "This is a valid reply body text.".to_string(), + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_zero_thread_id() { + let params = ReplyToPrCommentParams { + pull_request_id: 42, + thread_id: 0, + content: "This is a valid reply body text.".to_string(), + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_short_content() { + let params = ReplyToPrCommentParams { + pull_request_id: 42, + thread_id: 7, + content: "Too short".to_string(), + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = ReplyToPrCommentParams { + pull_request_id: 42, + thread_id: 7, + content: "A reply body that is definitely longer than ten characters.".to_string(), + repository: Some("self".to_string()), + }; + let result: ReplyToPrCommentResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"reply-to-pr-review-comment""#)); + assert!(json.contains(r#""pull_request_id":42"#)); + assert!(json.contains(r#""thread_id":7"#)); + } + + #[test] + fn test_config_defaults() { + let config = ReplyToPrCommentConfig::default(); + assert!(config.comment_prefix.is_none()); + assert!(config.allowed_repositories.is_empty()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +comment-prefix: "[Agent] " +allowed-repositories: + - self + - other-repo +"#; + let config: ReplyToPrCommentConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.comment_prefix, Some("[Agent] ".to_string())); + assert_eq!(config.allowed_repositories, vec!["self", "other-repo"]); + } +} diff --git a/src/tools/report_incomplete.rs b/src/tools/report_incomplete.rs new file mode 100644 index 00000000..98248c63 --- /dev/null +++ b/src/tools/report_incomplete.rs @@ -0,0 +1,105 @@ +//! Report incomplete task safe output tool + +use schemars::JsonSchema; +use serde::Deserialize; + +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::Validate; +use anyhow::ensure; + +/// Parameters for reporting that a task could not be completed +#[derive(Deserialize, JsonSchema)] +pub struct ReportIncompleteParams { + /// Why the task could not be completed + pub reason: String, + + /// Additional context about what was attempted + #[serde(default)] + pub context: Option, +} + +impl Validate for ReportIncompleteParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!( + self.reason.len() >= 10, + "reason must be at least 10 characters" + ); + Ok(()) + } +} + +tool_result! { + name = "report-incomplete", + params = ReportIncompleteParams, + /// Result of reporting an incomplete task + pub struct ReportIncompleteResult { + reason: String, + #[serde(default)] + context: Option, + } +} + +impl Sanitize for ReportIncompleteResult { + fn sanitize_fields(&mut self) { + self.reason = sanitize_text(&self.reason); + if let Some(ref ctx) = self.context { + self.context = Some(sanitize_text(ctx)); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(ReportIncompleteResult::NAME, "report-incomplete"); + } + + #[test] + fn test_params_deserializes() { + let json = r#"{"reason": "API timed out after 30s", "context": "tried 3 retries"}"#; + let params: ReportIncompleteParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.reason, "API timed out after 30s"); + assert_eq!(params.context, Some("tried 3 retries".to_string())); + } + + #[test] + fn test_params_converts_to_result() { + let params = ReportIncompleteParams { + reason: "Build failed with exit code 1".to_string(), + context: Some("ran cargo build".to_string()), + }; + let result: ReportIncompleteResult = params.try_into().unwrap(); + assert_eq!(result.name, "report-incomplete"); + assert_eq!(result.reason, "Build failed with exit code 1"); + assert_eq!(result.context, Some("ran cargo build".to_string())); + } + + #[test] + fn test_validation_rejects_short_reason() { + let params = ReportIncompleteParams { + reason: "short".to_string(), + context: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let result: ReportIncompleteResult = ReportIncompleteParams { + reason: "API timed out after 30s".to_string(), + context: None, + } + .try_into() + .unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"report-incomplete""#)); + assert!(json.contains(r#""reason":"API timed out after 30s""#)); + } +} diff --git a/src/tools/resolve_pr_thread.rs b/src/tools/resolve_pr_thread.rs new file mode 100644 index 00000000..5020865e --- /dev/null +++ b/src/tools/resolve_pr_thread.rs @@ -0,0 +1,402 @@ +//! Resolve PR review thread safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// All valid thread status strings (lowercase, agent-facing) +const VALID_STATUSES: &[&str] = &["active", "fixed", "wont-fix", "closed", "by-design"]; + +/// Map a thread status string to the ADO API integer value. +/// +/// ADO thread status values: +/// - 1 = Active +/// - 2 = Fixed (resolved) +/// - 3 = WontFix +/// - 4 = Closed +/// - 5 = ByDesign +fn status_to_int(status: &str) -> Option { + match status { + "active" => Some(1), + "fixed" => Some(2), + "wont-fix" => Some(3), + "closed" => Some(4), + "by-design" => Some(5), + _ => None, + } +} + +fn default_repository() -> Option { + Some("self".to_string()) +} + +/// Parameters for resolving or reactivating a PR review thread +#[derive(Deserialize, JsonSchema)] +pub struct ResolvePrThreadParams { + /// The pull request ID containing the thread + pub pull_request_id: i32, + + /// The thread ID to resolve or reactivate + pub thread_id: i32, + + /// Target status: "fixed", "wont-fix", "closed", "by-design", or "active" (to reactivate) + pub status: String, + + /// Repository alias: "self" for pipeline repo, or an alias from the checkout list. + /// Defaults to "self" if omitted. + #[serde(default = "default_repository")] + pub repository: Option, +} + +impl Validate for ResolvePrThreadParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!( + self.pull_request_id > 0, + "pull_request_id must be positive" + ); + ensure!(self.thread_id > 0, "thread_id must be positive"); + ensure!( + VALID_STATUSES.contains(&self.status.as_str()), + "Invalid status '{}'. Valid statuses: {}", + self.status, + VALID_STATUSES.join(", ") + ); + Ok(()) + } +} + +tool_result! { + name = "resolve-pr-review-thread", + params = ResolvePrThreadParams, + /// Result of resolving or reactivating a PR review thread + pub struct ResolvePrThreadResult { + pull_request_id: i32, + thread_id: i32, + status: String, + repository: Option, + } +} + +impl Sanitize for ResolvePrThreadResult { + fn sanitize_fields(&mut self) { + self.status = sanitize_text(&self.status); + if let Some(ref repo) = self.repository { + self.repository = Some(sanitize_text(repo)); + } + } +} + +/// Configuration for the resolve-pr-review-thread tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// resolve-pr-review-thread: +/// allowed-repositories: +/// - self +/// - other-repo +/// allowed-statuses: +/// - fixed +/// - wont-fix +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ResolvePrThreadConfig { + /// Restrict which repositories the agent can operate on. + /// If empty, all repositories in the checkout list (plus "self") are allowed. + #[serde(default, rename = "allowed-repositories")] + pub allowed_repositories: Vec, + + /// Restrict which thread statuses can be set. + /// If empty, all valid statuses are allowed. + #[serde(default, rename = "allowed-statuses")] + pub allowed_statuses: Vec, +} + +impl Default for ResolvePrThreadConfig { + fn default() -> Self { + Self { + allowed_repositories: Vec::new(), + allowed_statuses: Vec::new(), + } + } +} + +#[async_trait::async_trait] +impl Executor for ResolvePrThreadResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Resolving thread #{} on PR #{} with status '{}'", + self.thread_id, self.pull_request_id, self.status + ); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: ResolvePrThreadConfig = ctx.get_tool_config("resolve-pr-review-thread"); + debug!("Config: {:?}", config); + + // Validate status against allowed-statuses config + if !config.allowed_statuses.is_empty() + && !config.allowed_statuses.contains(&self.status) + { + return Ok(ExecutionResult::failure(format!( + "Status '{}' is not in the allowed-statuses list", + self.status + ))); + } + + let effective_repo = self + .repository + .as_deref() + .unwrap_or("self"); + + // Validate repository against allowed-repositories config + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&effective_repo.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list", + effective_repo + ))); + } + + // Map status string to ADO integer + let status_int = match status_to_int(&self.status) { + Some(v) => v, + None => { + return Ok(ExecutionResult::failure(format!( + "Invalid status '{}'. Valid statuses: {}", + self.status, + VALID_STATUSES.join(", ") + ))); + } + }; + + // Resolve repository name + let repo_name = if effective_repo == "self" || effective_repo.is_empty() { + ctx.repository_name + .as_ref() + .context("BUILD_REPOSITORY_NAME not set and repository is 'self'")? + .clone() + } else { + match ctx.allowed_repositories.get(effective_repo) { + Some(name) => name.clone(), + None => { + return Ok(ExecutionResult::failure(format!( + "Repository alias '{}' not found in allowed repositories", + effective_repo + ))); + } + } + }; + + // Build the Azure DevOps REST API URL for updating a thread + // PATCH https://dev.azure.com/{org}/{project}/_apis/git/repositories/{repo}/pullRequests/{prId}/threads/{threadId}?api-version=7.1 + let url = format!( + "{}/{}/_apis/git/repositories/{}/pullRequests/{}/threads/{}?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + utf8_percent_encode(&repo_name, PATH_SEGMENT), + self.pull_request_id, + self.thread_id, + ); + debug!("API URL: {}", url); + + let body = serde_json::json!({ + "status": status_int + }); + + let client = reqwest::Client::new(); + + info!( + "Updating thread #{} on PR #{} to status '{}'", + self.thread_id, self.pull_request_id, self.status + ); + let response = client + .patch(&url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&body) + .send() + .await + .context("Failed to send request to Azure DevOps")?; + + if response.status().is_success() { + let resp_body: serde_json::Value = response + .json() + .await + .context("Failed to parse response JSON")?; + + let returned_id = resp_body.get("id").and_then(|v| v.as_i64()).unwrap_or(0); + + info!( + "Thread #{} on PR #{} updated to status '{}'", + self.thread_id, self.pull_request_id, self.status + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Updated thread #{} on PR #{} to status '{}'", + self.thread_id, self.pull_request_id, self.status + ), + serde_json::json!({ + "thread_id": returned_id, + "pull_request_id": self.pull_request_id, + "repository": repo_name, + "project": project, + "status": self.status, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + Ok(ExecutionResult::failure(format!( + "Failed to update thread #{} on PR #{} (HTTP {}): {}", + self.thread_id, self.pull_request_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(ResolvePrThreadResult::NAME, "resolve-pr-review-thread"); + } + + #[test] + fn test_params_deserializes() { + let json = + r#"{"pull_request_id": 42, "thread_id": 7, "status": "fixed"}"#; + let params: ResolvePrThreadParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.pull_request_id, 42); + assert_eq!(params.thread_id, 7); + assert_eq!(params.status, "fixed"); + assert_eq!(params.repository, Some("self".to_string())); + } + + #[test] + fn test_params_converts_to_result() { + let params = ResolvePrThreadParams { + pull_request_id: 42, + thread_id: 7, + status: "fixed".to_string(), + repository: Some("self".to_string()), + }; + let result: ResolvePrThreadResult = params.try_into().unwrap(); + assert_eq!(result.name, "resolve-pr-review-thread"); + assert_eq!(result.pull_request_id, 42); + assert_eq!(result.thread_id, 7); + assert_eq!(result.status, "fixed"); + } + + #[test] + fn test_validation_rejects_zero_pr_id() { + let params = ResolvePrThreadParams { + pull_request_id: 0, + thread_id: 7, + status: "fixed".to_string(), + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_zero_thread_id() { + let params = ResolvePrThreadParams { + pull_request_id: 42, + thread_id: 0, + status: "fixed".to_string(), + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_invalid_status() { + let params = ResolvePrThreadParams { + pull_request_id: 42, + thread_id: 7, + status: "invalid-status".to_string(), + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = ResolvePrThreadParams { + pull_request_id: 42, + thread_id: 7, + status: "fixed".to_string(), + repository: Some("self".to_string()), + }; + let result: ResolvePrThreadResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"resolve-pr-review-thread""#)); + assert!(json.contains(r#""pull_request_id":42"#)); + assert!(json.contains(r#""thread_id":7"#)); + } + + #[test] + fn test_config_defaults() { + let config = ResolvePrThreadConfig::default(); + assert!(config.allowed_repositories.is_empty()); + assert!(config.allowed_statuses.is_empty()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +allowed-repositories: + - self + - other-repo +allowed-statuses: + - fixed + - wont-fix +"#; + let config: ResolvePrThreadConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.allowed_repositories, vec!["self", "other-repo"]); + assert_eq!(config.allowed_statuses, vec!["fixed", "wont-fix"]); + } + + #[test] + fn test_status_mapping() { + assert_eq!(status_to_int("active"), Some(1)); + assert_eq!(status_to_int("fixed"), Some(2)); + assert_eq!(status_to_int("wont-fix"), Some(3)); + assert_eq!(status_to_int("closed"), Some(4)); + assert_eq!(status_to_int("by-design"), Some(5)); + assert_eq!(status_to_int("invalid"), None); + } +} diff --git a/src/tools/submit_pr_review.rs b/src/tools/submit_pr_review.rs new file mode 100644 index 00000000..8c020766 --- /dev/null +++ b/src/tools/submit_pr_review.rs @@ -0,0 +1,526 @@ +//! Submit PR review safe output tool + +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::{Sanitize, sanitize as sanitize_text}; +use crate::tool_result; +use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use anyhow::{Context, ensure}; + +/// Valid event values for submit-pr-review +const VALID_EVENTS: &[&str] = &[ + "approve", + "approve-with-suggestions", + "request-changes", + "comment", +]; + +/// Map a review event string to its ADO vote numeric value +fn event_to_vote(event: &str) -> Option { + match event { + "approve" => Some(10), + "approve-with-suggestions" => Some(5), + "request-changes" => Some(-5), + "comment" => Some(0), + _ => None, + } +} + +fn default_repository() -> String { + "self".to_string() +} + +/// Parameters for submitting a pull request review +#[derive(Deserialize, JsonSchema)] +pub struct SubmitPrReviewParams { + /// The pull request ID to review (must be positive) + pub pull_request_id: i32, + + /// Review decision: "approve", "approve-with-suggestions", "request-changes", or "comment" + pub event: String, + + /// Review rationale in markdown. Required for "request-changes", optional otherwise. + /// Must be at least 10 characters when provided. + #[serde(default)] + pub body: Option, + + /// Repository alias: "self" for pipeline repo, or an alias from the checkout list. + /// Defaults to "self" if omitted. + #[serde(default)] + pub repository: Option, +} + +impl Validate for SubmitPrReviewParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!( + self.pull_request_id > 0, + "pull_request_id must be a positive integer" + ); + ensure!( + VALID_EVENTS.contains(&self.event.as_str()), + "event must be one of: {}", + VALID_EVENTS.join(", ") + ); + if self.event == "request-changes" { + ensure!( + self.body.is_some(), + "body is required when event is 'request-changes'" + ); + } + if let Some(ref body) = self.body { + ensure!( + body.len() >= 10, + "body must be at least 10 characters" + ); + } + Ok(()) + } +} + +tool_result! { + name = "submit-pr-review", + params = SubmitPrReviewParams, + /// Result of submitting a pull request review + pub struct SubmitPrReviewResult { + pull_request_id: i32, + event: String, + body: Option, + repository: Option, + } +} + +impl Sanitize for SubmitPrReviewResult { + fn sanitize_fields(&mut self) { + self.event = sanitize_text(&self.event); + self.body = self.body.as_deref().map(sanitize_text); + self.repository = self.repository.as_deref().map(sanitize_text); + } +} + +/// Configuration for the submit-pr-review tool (specified in front matter) +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// submit-pr-review: +/// allowed-events: +/// - approve +/// - comment +/// allowed-repositories: +/// - self +/// ``` +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct SubmitPrReviewConfig { + /// Which events are permitted. REQUIRED — empty list rejects all. + #[serde(default, rename = "allowed-events")] + pub allowed_events: Vec, + + /// Which repositories the agent may target. Empty list means all allowed repos. + #[serde(default, rename = "allowed-repositories")] + pub allowed_repositories: Vec, +} + +impl Default for SubmitPrReviewConfig { + fn default() -> Self { + Self { + allowed_events: Vec::new(), + allowed_repositories: Vec::new(), + } + } +} + +/// Resolve the repository name for use in ADO API URLs. +/// +/// "self" (or None) → `ctx.repository_name`, otherwise look up in `ctx.allowed_repositories`. +fn resolve_repo_name( + repo_alias: Option<&str>, + ctx: &ExecutionContext, +) -> Result { + let alias = repo_alias.unwrap_or("self"); + if alias == "self" { + ctx.repository_name + .clone() + .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set")) + } else { + ctx.allowed_repositories + .get(alias) + .cloned() + .ok_or_else(|| { + ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed repository list", + alias + )) + }) + } +} + +#[async_trait::async_trait] +impl Executor for SubmitPrReviewResult { + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Submitting review on PR #{} — event: {}", + self.pull_request_id, self.event + ); + + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + let organization = ctx + .ado_organization + .as_ref() + .context("Could not determine ADO organization name from URL")?; + debug!("ADO org: {}, project: {}", org_url, project); + + let config: SubmitPrReviewConfig = ctx.get_tool_config("submit-pr-review"); + debug!("Config: {:?}", config); + + // Validate event against allowed-events — REQUIRED. + // An empty allowed-events list means the operator hasn't opted in, so reject. + if config.allowed_events.is_empty() { + return Ok(ExecutionResult::failure( + "submit-pr-review requires 'allowed-events' to be configured in \ + safe-outputs.submit-pr-review. This prevents agents from casting \ + unrestricted review votes. Example:\n safe-outputs:\n submit-pr-review:\n \ + allowed-events:\n - comment\n - approve-with-suggestions" + .to_string(), + )); + } + if !config.allowed_events.contains(&self.event) { + return Ok(ExecutionResult::failure(format!( + "Event '{}' is not in the allowed-events list: [{}]", + self.event, + config.allowed_events.join(", ") + ))); + } + + // Validate repository against allowed-repositories config + let repo_alias = self.repository.as_deref().unwrap_or("self"); + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&repo_alias.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + repo_alias, + config.allowed_repositories.join(", ") + ))); + } + + // Resolve repo name + let repo_name = match resolve_repo_name(self.repository.as_deref(), ctx) { + Ok(name) => name, + Err(failure) => return Ok(failure), + }; + debug!("Resolved repository: {}", repo_name); + + // Map event to vote value + let vote_value = event_to_vote(&self.event).context(format!( + "Invalid event: '{}'. Must be one of: {}", + self.event, + VALID_EVENTS.join(", ") + ))?; + + let client = reqwest::Client::new(); + let encoded_project = utf8_percent_encode(project, PATH_SEGMENT).to_string(); + let encoded_repo = utf8_percent_encode(&repo_name, PATH_SEGMENT).to_string(); + let base_url = format!( + "{}/{}/_apis/git/repositories", + org_url.trim_end_matches('/'), + encoded_project, + ); + + // Resolve the current user identity via connection data + let connection_url = format!( + "https://dev.azure.com/{}/_apis/connectiondata", + utf8_percent_encode(organization, PATH_SEGMENT) + ); + debug!("Connection data URL: {}", connection_url); + + let conn_response = client + .get(&connection_url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to fetch connection data")?; + + if !conn_response.status().is_success() { + let status = conn_response.status(); + let error_body = conn_response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to fetch connection data (HTTP {}): {}", + status, error_body + ))); + } + + let conn_body: serde_json::Value = conn_response + .json() + .await + .context("Failed to parse connection data response")?; + + let user_id = conn_body + .get("authenticatedUser") + .and_then(|au| au.get("id")) + .and_then(|id| id.as_str()) + .context("Connection data response missing authenticatedUser.id")?; + debug!("Authenticated user ID: {}", user_id); + + // PUT vote to reviewers endpoint + let vote_url = format!( + "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", + base_url, encoded_repo, self.pull_request_id, user_id + ); + let vote_body = serde_json::json!({ + "vote": vote_value + }); + + info!( + "Voting '{}' ({}) on PR #{}", + self.event, vote_value, self.pull_request_id + ); + let response = client + .put(&vote_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&vote_body) + .send() + .await + .context("Failed to submit vote")?; + + if !response.status().is_success() { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to submit vote on PR #{} (HTTP {}): {}", + self.pull_request_id, status, error_body + ))); + } + + info!( + "Vote '{}' submitted on PR #{}", + self.event, self.pull_request_id + ); + + // If body is provided, also POST a comment thread with the review rationale + if let Some(ref body) = self.body { + let thread_url = format!( + "{}/{}/pullRequests/{}/threads?api-version=7.1", + base_url, encoded_repo, self.pull_request_id + ); + let thread_body = serde_json::json!({ + "comments": [{ + "parentCommentId": 0, + "content": body, + "commentType": 1 + }], + "status": 4 + }); + + info!( + "Posting review comment on PR #{} ({} chars)", + self.pull_request_id, + body.len() + ); + let thread_response = client + .post(&thread_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&thread_body) + .send() + .await + .context("Failed to post review comment thread")?; + + if !thread_response.status().is_success() { + let status = thread_response.status(); + let error_body = thread_response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Vote submitted but failed to post review comment on PR #{} (HTTP {}): {}", + self.pull_request_id, status, error_body + ))); + } + + let thread_resp: serde_json::Value = thread_response + .json() + .await + .context("Failed to parse comment thread response")?; + + let thread_id = thread_resp + .get("id") + .and_then(|v| v.as_i64()) + .unwrap_or(0); + + info!( + "Review comment thread #{} posted on PR #{}", + thread_id, self.pull_request_id + ); + + return Ok(ExecutionResult::success_with_data( + format!( + "Review '{}' submitted on PR #{} with comment thread #{}", + self.event, self.pull_request_id, thread_id + ), + serde_json::json!({ + "pull_request_id": self.pull_request_id, + "event": self.event, + "vote_value": vote_value, + "thread_id": thread_id, + "repository": repo_name, + }), + )); + } + + Ok(ExecutionResult::success_with_data( + format!( + "Review '{}' submitted on PR #{}", + self.event, self.pull_request_id + ), + serde_json::json!({ + "pull_request_id": self.pull_request_id, + "event": self.event, + "vote_value": vote_value, + "repository": repo_name, + }), + )) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tools::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(SubmitPrReviewResult::NAME, "submit-pr-review"); + } + + #[test] + fn test_params_deserializes() { + let json = r#"{"pull_request_id": 42, "event": "approve"}"#; + let params: SubmitPrReviewParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.pull_request_id, 42); + assert_eq!(params.event, "approve"); + assert!(params.body.is_none()); + assert!(params.repository.is_none()); + } + + #[test] + fn test_params_converts_to_result() { + let params = SubmitPrReviewParams { + pull_request_id: 42, + event: "approve".to_string(), + body: None, + repository: Some("self".to_string()), + }; + let result: SubmitPrReviewResult = params.try_into().unwrap(); + assert_eq!(result.name, "submit-pr-review"); + assert_eq!(result.pull_request_id, 42); + assert_eq!(result.event, "approve"); + } + + #[test] + fn test_validation_rejects_zero_pr_id() { + let params = SubmitPrReviewParams { + pull_request_id: 0, + event: "approve".to_string(), + body: None, + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_invalid_event() { + let params = SubmitPrReviewParams { + pull_request_id: 1, + event: "merge".to_string(), + body: None, + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_request_changes_without_body() { + let params = SubmitPrReviewParams { + pull_request_id: 1, + event: "request-changes".to_string(), + body: None, + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_accepts_approve_without_body() { + let params = SubmitPrReviewParams { + pull_request_id: 1, + event: "approve".to_string(), + body: None, + repository: Some("self".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_ok()); + } + + #[test] + fn test_result_serializes_correctly() { + let params = SubmitPrReviewParams { + pull_request_id: 99, + event: "request-changes".to_string(), + body: Some("This needs significant rework before merging.".to_string()), + repository: Some("self".to_string()), + }; + let result: SubmitPrReviewResult = params.try_into().unwrap(); + let json = serde_json::to_string(&result).unwrap(); + + assert!(json.contains(r#""name":"submit-pr-review""#)); + assert!(json.contains(r#""pull_request_id":99"#)); + assert!(json.contains(r#""event":"request-changes""#)); + } + + #[test] + fn test_config_defaults() { + let config = SubmitPrReviewConfig::default(); + assert!(config.allowed_events.is_empty()); + assert!(config.allowed_repositories.is_empty()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +allowed-events: + - approve + - comment +allowed-repositories: + - self + - other-repo +"#; + let config: SubmitPrReviewConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.allowed_events, vec!["approve", "comment"]); + assert_eq!(config.allowed_repositories, vec!["self", "other-repo"]); + } +} From 3eb320bcc2beb071721317b9fabb0419f0a7af88 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 8 Apr 2026 14:58:15 +0100 Subject: [PATCH 04/15] fix: address second round of PR review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug fixes: - update-pr: derive connectiondata URL from org_url instead of hardcoding dev.azure.com — supports vanity domains and national cloud endpoints - update-pr: derive VSSPS identity URL from org_url instead of hardcoding vssps.dev.azure.com — same environment portability fix - submit-pr-review: same connectiondata URL fix (derived from org_url) Security: - upload-attachment: add canonicalize() + prefix check after resolving file path to prevent symlink escape attacks (symlinks within workspace could previously be followed to read arbitrary files outside source_directory) Improvements: - add-pr-comment: sanitize structural fields (repository, status, file_path) with control-char stripping for defense-in-depth consistency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/tools/add_pr_comment.rs | 6 ++++++ src/tools/submit_pr_review.rs | 11 ++++------- src/tools/update_pr.rs | 29 +++++++++++++++-------------- src/tools/upload_attachment.rs | 23 +++++++++++++++++++---- 4 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index ae78b4ae..975c7742 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -103,6 +103,12 @@ tool_result! { impl Sanitize for AddPrCommentResult { fn sanitize_fields(&mut self) { self.content = sanitize_text(&self.content); + // Strip control characters from structural fields for defense-in-depth + self.repository = self.repository.chars().filter(|c| !c.is_control()).collect(); + self.status = self.status.chars().filter(|c| !c.is_control()).collect(); + self.file_path = self.file_path.as_ref().map(|fp| { + fp.chars().filter(|c| !c.is_control()).collect() + }); } } diff --git a/src/tools/submit_pr_review.rs b/src/tools/submit_pr_review.rs index 8c020766..cd6f65f7 100644 --- a/src/tools/submit_pr_review.rs +++ b/src/tools/submit_pr_review.rs @@ -178,10 +178,6 @@ impl Executor for SubmitPrReviewResult { .access_token .as_ref() .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - let organization = ctx - .ado_organization - .as_ref() - .context("Could not determine ADO organization name from URL")?; debug!("ADO org: {}, project: {}", org_url, project); let config: SubmitPrReviewConfig = ctx.get_tool_config("submit-pr-review"); @@ -241,10 +237,11 @@ impl Executor for SubmitPrReviewResult { encoded_project, ); - // Resolve the current user identity via connection data + // Resolve the current user identity via connection data. + // Use the org URL — supports vanity domains and national clouds. let connection_url = format!( - "https://dev.azure.com/{}/_apis/connectiondata", - utf8_percent_encode(organization, PATH_SEGMENT) + "{}/_apis/connectiondata", + org_url.trim_end_matches('/') ); debug!("Connection data URL: {}", connection_url); diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index fcb959f3..baed3727 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -264,10 +264,6 @@ impl Executor for UpdatePrResult { .access_token .as_ref() .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - let organization = ctx - .ado_organization - .as_ref() - .context("Could not determine ADO organization name from URL")?; debug!("ADO org: {}, project: {}", org_url, project); let config: UpdatePrConfig = ctx.get_tool_config("update-pr"); @@ -322,7 +318,7 @@ impl Executor for UpdatePrResult { &base_url, &repo_name, token, - organization, + org_url, &config, ) .await @@ -333,7 +329,7 @@ impl Executor for UpdatePrResult { &base_url, &repo_name, token, - organization, + org_url, ) .await } @@ -466,7 +462,7 @@ impl UpdatePrResult { base_url: &str, repo_name: &str, token: &str, - organization: &str, + org_url: &str, config: &UpdatePrConfig, ) -> anyhow::Result { let vote_str = self @@ -499,10 +495,11 @@ impl UpdatePrResult { VALID_VOTES.join(", ") ))?; - // Resolve the current user identity + // Resolve the current user identity. + // Use the org URL for connection data — supports vanity domains and national clouds. let connection_url = format!( - "https://dev.azure.com/{}/_apis/connectiondata", - utf8_percent_encode(organization, PATH_SEGMENT) + "{}/_apis/connectiondata", + org_url.trim_end_matches('/') ); debug!("Connection data URL: {}", connection_url); @@ -600,7 +597,7 @@ impl UpdatePrResult { base_url: &str, repo_name: &str, token: &str, - organization: &str, + org_url: &str, ) -> anyhow::Result { let reviewers = self .reviewers @@ -612,10 +609,14 @@ impl UpdatePrResult { let mut failed = Vec::new(); for reviewer in reviewers { - // Resolve reviewer email to GUID via VSSPS identity API + // Resolve reviewer email to GUID via VSSPS identity API. + // Derive the VSSPS URL from org_url to support non-standard environments. + let vssps_base = org_url + .trim_end_matches('/') + .replace("://dev.azure.com/", "://vssps.dev.azure.com/"); let identity_url = format!( - "https://vssps.dev.azure.com/{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", - utf8_percent_encode(organization, PATH_SEGMENT), + "{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", + vssps_base, utf8_percent_encode(reviewer, PATH_SEGMENT), ); debug!("Resolving identity for '{}': {}", reviewer, identity_url); diff --git a/src/tools/upload_attachment.rs b/src/tools/upload_attachment.rs index 488f170d..edadac0f 100644 --- a/src/tools/upload_attachment.rs +++ b/src/tools/upload_attachment.rs @@ -169,8 +169,23 @@ impl Executor for UploadAttachmentResult { let resolved_path = ctx.source_directory.join(&self.file_path); debug!("Resolved file path: {}", resolved_path.display()); - // Check file exists - if !resolved_path.exists() { + // Canonicalize to resolve symlinks, then verify the path stays within source_directory. + let canonical = resolved_path + .canonicalize() + .context("Failed to canonicalize file path — file may not exist or contains broken symlinks")?; + let canonical_base = ctx + .source_directory + .canonicalize() + .context("Failed to canonicalize source directory")?; + if !canonical.starts_with(&canonical_base) { + return Ok(ExecutionResult::failure(format!( + "File path '{}' resolves outside the workspace (symlink escape)", + self.file_path + ))); + } + + // Check file exists (already confirmed by canonicalize, but kept for clarity) + if !canonical.exists() { return Ok(ExecutionResult::failure(format!( "File not found: {}", resolved_path.display() @@ -178,7 +193,7 @@ impl Executor for UploadAttachmentResult { } // Check file size - let metadata = std::fs::metadata(&resolved_path) + let metadata = std::fs::metadata(&canonical) .context("Failed to read file metadata")?; let file_size = metadata.len(); debug!("File size: {} bytes", file_size); @@ -191,7 +206,7 @@ impl Executor for UploadAttachmentResult { // Read file bytes let file_bytes = - std::fs::read(&resolved_path).context("Failed to read file contents")?; + std::fs::read(&canonical).context("Failed to read file contents")?; // Check if file is text (not binary) — if text, scan for ##vso[ command injection if let Ok(text) = std::str::from_utf8(&file_bytes) { From 85a9c302c69fb21e2caf8044feb463d31a70d4ff Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 8 Apr 2026 17:35:22 +0100 Subject: [PATCH 05/15] fix: branch wildcard matching + VSSPS URL warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - queue-build: drop equality arm from release/* wildcard matching so bare 'release' no longer matches 'release/*' — only 'release/...' does - update-pr: add warn! log when VSSPS URL derivation produces no change (surfaces the issue for visualstudio.com-hosted orgs at runtime) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/tools/queue_build.rs | 3 +-- src/tools/update_pr.rs | 11 +++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/tools/queue_build.rs b/src/tools/queue_build.rs index df0fac7b..7f7620d7 100644 --- a/src/tools/queue_build.rs +++ b/src/tools/queue_build.rs @@ -180,8 +180,7 @@ impl Executor for QueueBuildResult { if pattern.ends_with("/*") { let prefix = &pattern[..pattern.len() - 2]; effective_branch.starts_with(prefix) - && (effective_branch.len() == prefix.len() - || effective_branch[prefix.len()..].starts_with('/')) + && effective_branch[prefix.len()..].starts_with('/') } else { pattern == effective_branch } diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index baed3727..49e38cf9 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -611,9 +611,16 @@ impl UpdatePrResult { for reviewer in reviewers { // Resolve reviewer email to GUID via VSSPS identity API. // Derive the VSSPS URL from org_url to support non-standard environments. - let vssps_base = org_url - .trim_end_matches('/') + let trimmed_org = org_url.trim_end_matches('/'); + let vssps_base = trimmed_org .replace("://dev.azure.com/", "://vssps.dev.azure.com/"); + if vssps_base == trimmed_org { + warn!( + "Could not derive VSSPS endpoint from org URL '{}' — identity lookup \ + may fail for non-dev.azure.com hosts (e.g., *.visualstudio.com)", + trimmed_org + ); + } let identity_url = format!( "{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", vssps_base, From 15c4a0f9178abe24c4ccb8530619800ac80bc90f Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 8 Apr 2026 17:47:29 +0100 Subject: [PATCH 06/15] fix: status mapping, VSSPS rejection, branch sanitize, policy order Bug fix: - add-pr-comment: fix WontFix status mapped to 6 (should be 3). Unify status strings to kebab-case (active, fixed, wont-fix, closed, by-design) with CamelCase accepted for backwards compatibility. Security: - update-pr: reject add-reviewers for non-dev.azure.com org URLs instead of silently proceeding with a malformed VSSPS URL (*.visualstudio.com orgs) Improvements: - queue-build: sanitize branch field for defense-in-depth consistency - update-pr: document allow-list semantics asymmetry (operations=permissive, votes=secure) in UpdatePrConfig doc comment - create-branch: check config policy (allowed-repositories) before resolving repo alias, so operators see a policy error not a checkout-list error Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/tools/add_pr_comment.rs | 44 +++++++++++++++++++++++-------------- src/tools/create_branch.rs | 26 ++++++++++++---------- src/tools/queue_build.rs | 3 +++ src/tools/update_pr.rs | 15 +++++++++---- 4 files changed, 56 insertions(+), 32 deletions(-) diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index 975c7742..e22703da 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -38,7 +38,8 @@ pub struct AddPrCommentParams { #[serde(default)] pub line: Option, - /// Thread status: "Active" (default), "Closed", "ByDesign", or "WontFix". + /// Thread status: "active" (default), "fixed", "wont-fix", "closed", or "by-design". + /// CamelCase forms ("Active", "WontFix", etc.) are also accepted for backwards compatibility. #[serde(default = "default_status")] pub status: String, } @@ -48,7 +49,7 @@ fn default_repository() -> String { } fn default_status() -> String { - "Active".to_string() + "active".to_string() } impl Validate for AddPrCommentParams { @@ -153,19 +154,21 @@ impl Default for AddPrCommentConfig { } } -/// Map a thread status string to the ADO API integer value +/// Map a thread status string to the ADO API integer value. +/// Accepts both kebab-case (preferred) and CamelCase for backwards compatibility. fn status_to_int(status: &str) -> Option { match status { - "Active" => Some(1), - "Closed" => Some(4), - "ByDesign" => Some(5), - "WontFix" => Some(6), + "active" | "Active" => Some(1), + "fixed" | "Fixed" => Some(2), + "wont-fix" | "WontFix" => Some(3), + "closed" | "Closed" => Some(4), + "by-design" | "ByDesign" => Some(5), _ => None, } } -/// All valid thread status strings -const VALID_STATUSES: &[&str] = &["Active", "Closed", "ByDesign", "WontFix"]; +/// All valid thread status strings (kebab-case canonical form) +const VALID_STATUSES: &[&str] = &["active", "fixed", "wont-fix", "closed", "by-design"]; /// Validate a file path for inline comments: no `..`, not absolute fn validate_file_path(path: &str) -> anyhow::Result<()> { @@ -378,7 +381,7 @@ mod tests { assert_eq!(params.repository, "self"); assert!(params.file_path.is_none()); assert!(params.line.is_none()); - assert_eq!(params.status, "Active"); + assert_eq!(params.status, "active"); } #[test] @@ -390,7 +393,7 @@ mod tests { file_path: None, start_line: None, line: None, - status: "Active".to_string(), + status: "active".to_string(), }; let result: AddPrCommentResult = params.try_into().unwrap(); assert_eq!(result.name, "add-pr-comment"); @@ -407,7 +410,7 @@ mod tests { file_path: None, start_line: None, line: None, - status: "Active".to_string(), + status: "active".to_string(), }; let result: Result = params.try_into(); assert!(result.is_err()); @@ -422,7 +425,7 @@ mod tests { file_path: None, start_line: None, line: None, - status: "Active".to_string(), + status: "active".to_string(), }; let result: Result = params.try_into(); assert!(result.is_err()); @@ -437,7 +440,7 @@ mod tests { file_path: None, start_line: None, line: Some(10), - status: "Active".to_string(), + status: "active".to_string(), }; let result: Result = params.try_into(); assert!(result.is_err()); @@ -452,7 +455,7 @@ mod tests { file_path: Some("src/main.rs".to_string()), start_line: None, line: Some(10), - status: "Active".to_string(), + status: "active".to_string(), }; let result: AddPrCommentResult = params.try_into().unwrap(); let json = serde_json::to_string(&result).unwrap(); @@ -488,10 +491,17 @@ allowed-statuses: #[test] fn test_status_to_int_mapping() { + // Kebab-case (canonical) + assert_eq!(status_to_int("active"), Some(1)); + assert_eq!(status_to_int("fixed"), Some(2)); + assert_eq!(status_to_int("wont-fix"), Some(3)); + assert_eq!(status_to_int("closed"), Some(4)); + assert_eq!(status_to_int("by-design"), Some(5)); + // CamelCase (backwards compat) assert_eq!(status_to_int("Active"), Some(1)); - assert_eq!(status_to_int("Closed"), Some(4)); + assert_eq!(status_to_int("WontFix"), Some(3)); assert_eq!(status_to_int("ByDesign"), Some(5)); - assert_eq!(status_to_int("WontFix"), Some(6)); + // Invalid assert_eq!(status_to_int("Invalid"), None); } diff --git a/src/tools/create_branch.rs b/src/tools/create_branch.rs index 3d129ca2..8d70a269 100644 --- a/src/tools/create_branch.rs +++ b/src/tools/create_branch.rs @@ -231,11 +231,25 @@ impl Executor for CreateBranchResult { debug!("Branch name matches pattern '{}'", pattern); } - // Determine repository name + // Determine repository alias let repo_alias = self .repository .as_deref() .unwrap_or("self"); + + // Validate repository against config policy BEFORE resolving the name, + // so operators see a policy error rather than a confusing "not in checkout list" error. + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&repo_alias.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + repo_alias, + config.allowed_repositories.join(", ") + ))); + } + + // Resolve the alias to the actual ADO repo name let repo_name = if repo_alias == "self" { ctx.repository_name .as_deref() @@ -252,16 +266,6 @@ impl Executor for CreateBranchResult { }; debug!("Resolved repository: {}", repo_name); - // Validate repository against allowed-repositories (if configured) - if !config.allowed_repositories.is_empty() - && !config.allowed_repositories.contains(&repo_alias.to_string()) - { - return Ok(ExecutionResult::failure(format!( - "Repository '{}' is not in the allowed-repositories list", - repo_alias - ))); - } - // Validate source_branch against allowed-source-branches (if configured) let source_branch = self.source_branch.as_deref().unwrap_or("main"); if !config.allowed_source_branches.is_empty() diff --git a/src/tools/queue_build.rs b/src/tools/queue_build.rs index 7f7620d7..18e24e0c 100644 --- a/src/tools/queue_build.rs +++ b/src/tools/queue_build.rs @@ -68,6 +68,9 @@ impl Sanitize for QueueBuildResult { if let Some(reason) = &self.reason { self.reason = Some(sanitize_text(reason)); } + if let Some(branch) = &self.branch { + self.branch = Some(sanitize_text(branch)); + } if let Some(params) = &self.parameters { self.parameters = Some( params diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index 49e38cf9..80acdb48 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -162,6 +162,11 @@ impl Sanitize for UpdatePrResult { /// Configuration for the update-pr tool (specified in front matter) /// +/// **Allow-list semantics note:** `allowed-operations` and `allowed-repositories` use +/// permissive defaults (empty = all allowed), while `allowed-votes` uses a secure default +/// (empty = all rejected). This asymmetry is intentional — vote operations can auto-approve +/// PRs, so they require explicit opt-in to prevent accidental privilege escalation. +/// /// Example front matter: /// ```yaml /// safe-outputs: @@ -615,11 +620,13 @@ impl UpdatePrResult { let vssps_base = trimmed_org .replace("://dev.azure.com/", "://vssps.dev.azure.com/"); if vssps_base == trimmed_org { - warn!( - "Could not derive VSSPS endpoint from org URL '{}' — identity lookup \ - may fail for non-dev.azure.com hosts (e.g., *.visualstudio.com)", + return Ok(ExecutionResult::failure(format!( + "Cannot derive VSSPS identity endpoint from org URL '{}'. \ + The add-reviewers operation requires dev.azure.com-style URLs \ + to resolve reviewer identities. Legacy *.visualstudio.com \ + organizations are not currently supported for this operation.", trimmed_org - ); + ))); } let identity_url = format!( "{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", From 9afba91b88c559a7053720116417eda470b47635 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 8 Apr 2026 17:59:28 +0100 Subject: [PATCH 07/15] fix: repo policy ordering, VSSPS loop hoist, dedup resolve_repo_name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug fixes: - create-git-tag: check config allowed-repositories before resolving repo alias (same fix as create-branch — policy error before resolution error) - update-pr: hoist VSSPS URL derivation and validation out of the per-reviewer loop (avoids repeated work, fails fast before any API calls) Refactor: - Extract resolve_repo_name() to tools/mod.rs — was duplicated identically in update_pr.rs and submit_pr_review.rs Documentation: - add-pr-comment: clarify start_line must be strictly less than line - reply-to-pr-comment: explain parentCommentId=1 targets root comment - upload-attachment: document ##vso[] scan UTF-8 boundary behavior - mcp.rs: log warning on report-incomplete write failure Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/mcp.rs | 4 ++- src/tools/add_pr_comment.rs | 3 +- src/tools/create_git_tag.rs | 25 ++++++++------- src/tools/mod.rs | 25 +++++++++++++++ src/tools/reply_to_pr_comment.rs | 2 ++ src/tools/submit_pr_review.rs | 27 +--------------- src/tools/update_pr.rs | 55 +++++++++----------------------- src/tools/upload_attachment.rs | 6 +++- 8 files changed, 67 insertions(+), 80 deletions(-) diff --git a/src/mcp.rs b/src/mcp.rs index 3b94bec2..0abca5a9 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -823,7 +823,9 @@ agent attempted work but couldn't finish (e.g., API timeouts, build failures, re sanitized.reason = sanitize_text(&sanitized.reason); sanitized.context = sanitized.context.map(|c| sanitize_text(&c)); let result: ReportIncompleteResult = sanitized.try_into()?; - let _ = self.write_safe_output_file(&result).await; + if let Err(e) = self.write_safe_output_file(&result).await { + warn!("Failed to write report-incomplete safe output: {}", e); + } Ok(CallToolResult::success(vec![])) } } diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index e22703da..59a1798a 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -30,7 +30,8 @@ pub struct AddPrCommentParams { pub file_path: Option, /// Starting line number for a multi-line inline comment. Requires `file_path` and `line`. - /// When set, the comment spans from `start_line` to `line`. + /// When set, the comment spans from `start_line` to `line`. Must be strictly less than + /// `line` (use `line` alone for single-line comments — do not pass `start_line == line`). #[serde(default)] pub start_line: Option, diff --git a/src/tools/create_git_tag.rs b/src/tools/create_git_tag.rs index abe013eb..01240011 100644 --- a/src/tools/create_git_tag.rs +++ b/src/tools/create_git_tag.rs @@ -270,8 +270,21 @@ impl Executor for CreateGitTagResult { } } - // Resolve repository name + // Resolve repository let repo_alias = self.repository.as_deref().unwrap_or("self"); + + // Validate repository against config policy BEFORE resolving the name, + // so operators see a policy error rather than a confusing resolution error. + if !config.allowed_repositories.is_empty() + && !config.allowed_repositories.contains(&repo_alias.to_string()) + { + return Ok(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + repo_alias, + config.allowed_repositories.join(", ") + ))); + } + let repo_name = if repo_alias == "self" { ctx.repository_name .as_deref() @@ -287,16 +300,6 @@ impl Executor for CreateGitTagResult { ))? }; - // Validate repository against allowed-repositories config - if !config.allowed_repositories.is_empty() - && !config.allowed_repositories.contains(&repo_alias.to_string()) - { - return Ok(ExecutionResult::failure(format!( - "Repository '{}' is not in the allowed-repositories list", - repo_alias - ))); - } - let client = reqwest::Client::new(); // Resolve commit SHA — use provided value or look up HEAD diff --git a/src/tools/mod.rs b/src/tools/mod.rs index c77f4689..1a6cf2f7 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -104,6 +104,31 @@ pub(crate) async fn resolve_wiki_branch( } } +/// Resolve a repository alias to its ADO repo name. +/// +/// "self" (or None) → `ctx.repository_name`, otherwise look up in `ctx.allowed_repositories`. +pub(crate) fn resolve_repo_name( + repo_alias: Option<&str>, + ctx: &ExecutionContext, +) -> Result { + let alias = repo_alias.unwrap_or("self"); + if alias == "self" { + ctx.repository_name + .clone() + .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set")) + } else { + ctx.allowed_repositories + .get(alias) + .cloned() + .ok_or_else(|| { + ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed repository list", + alias + )) + }) + } +} + mod add_build_tag; mod add_pr_comment; mod comment_on_work_item; diff --git a/src/tools/reply_to_pr_comment.rs b/src/tools/reply_to_pr_comment.rs index 8355bcb8..2839aea8 100644 --- a/src/tools/reply_to_pr_comment.rs +++ b/src/tools/reply_to_pr_comment.rs @@ -172,6 +172,8 @@ impl Executor for ReplyToPrCommentResult { ); debug!("API URL: {}", url); + // parentCommentId=1 targets the root comment in the thread. In ADO, + // the first comment in a thread is always ID 1 (IDs are thread-scoped). let request_body = serde_json::json!({ "parentCommentId": 1, "content": comment_body, diff --git a/src/tools/submit_pr_review.rs b/src/tools/submit_pr_review.rs index cd6f65f7..c3f99aad 100644 --- a/src/tools/submit_pr_review.rs +++ b/src/tools/submit_pr_review.rs @@ -5,7 +5,7 @@ use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use super::PATH_SEGMENT; +use super::{PATH_SEGMENT, resolve_repo_name}; use crate::sanitize::{Sanitize, sanitize as sanitize_text}; use crate::tool_result; use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; @@ -133,31 +133,6 @@ impl Default for SubmitPrReviewConfig { } } -/// Resolve the repository name for use in ADO API URLs. -/// -/// "self" (or None) → `ctx.repository_name`, otherwise look up in `ctx.allowed_repositories`. -fn resolve_repo_name( - repo_alias: Option<&str>, - ctx: &ExecutionContext, -) -> Result { - let alias = repo_alias.unwrap_or("self"); - if alias == "self" { - ctx.repository_name - .clone() - .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set")) - } else { - ctx.allowed_repositories - .get(alias) - .cloned() - .ok_or_else(|| { - ExecutionResult::failure(format!( - "Repository '{}' is not in the allowed repository list", - alias - )) - }) - } -} - #[async_trait::async_trait] impl Executor for SubmitPrReviewResult { async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index 80acdb48..9eb5c7aa 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -5,7 +5,7 @@ use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use super::PATH_SEGMENT; +use super::{PATH_SEGMENT, resolve_repo_name}; use crate::sanitize::{Sanitize, sanitize as sanitize_text}; use crate::tool_result; use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; @@ -224,31 +224,6 @@ impl Default for UpdatePrConfig { } } -/// Resolve the repository name for use in ADO API URLs. -/// -/// "self" (or None) → `ctx.repository_name`, otherwise look up in `ctx.allowed_repositories`. -fn resolve_repo_name( - repo_alias: Option<&str>, - ctx: &ExecutionContext, -) -> Result { - let alias = repo_alias.unwrap_or("self"); - if alias == "self" { - ctx.repository_name - .clone() - .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set")) - } else { - ctx.allowed_repositories - .get(alias) - .cloned() - .ok_or_else(|| { - ExecutionResult::failure(format!( - "Repository '{}' is not in the allowed repository list", - alias - )) - }) - } -} - #[async_trait::async_trait] impl Executor for UpdatePrResult { async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { @@ -613,21 +588,21 @@ impl UpdatePrResult { let mut added = Vec::new(); let mut failed = Vec::new(); + // Derive VSSPS base URL once, before the loop. + let trimmed_org = org_url.trim_end_matches('/'); + let vssps_base = trimmed_org + .replace("://dev.azure.com/", "://vssps.dev.azure.com/"); + if vssps_base == trimmed_org { + return Ok(ExecutionResult::failure(format!( + "Cannot derive VSSPS identity endpoint from org URL '{}'. \ + The add-reviewers operation requires dev.azure.com-style URLs \ + to resolve reviewer identities. Legacy *.visualstudio.com \ + organizations are not currently supported for this operation.", + trimmed_org + ))); + } + for reviewer in reviewers { - // Resolve reviewer email to GUID via VSSPS identity API. - // Derive the VSSPS URL from org_url to support non-standard environments. - let trimmed_org = org_url.trim_end_matches('/'); - let vssps_base = trimmed_org - .replace("://dev.azure.com/", "://vssps.dev.azure.com/"); - if vssps_base == trimmed_org { - return Ok(ExecutionResult::failure(format!( - "Cannot derive VSSPS identity endpoint from org URL '{}'. \ - The add-reviewers operation requires dev.azure.com-style URLs \ - to resolve reviewer identities. Legacy *.visualstudio.com \ - organizations are not currently supported for this operation.", - trimmed_org - ))); - } let identity_url = format!( "{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", vssps_base, diff --git a/src/tools/upload_attachment.rs b/src/tools/upload_attachment.rs index edadac0f..e2546795 100644 --- a/src/tools/upload_attachment.rs +++ b/src/tools/upload_attachment.rs @@ -208,7 +208,11 @@ impl Executor for UploadAttachmentResult { let file_bytes = std::fs::read(&canonical).context("Failed to read file contents")?; - // Check if file is text (not binary) — if text, scan for ##vso[ command injection + // Check if file is text (valid UTF-8) — if text, scan for ##vso[ command injection. + // Binary files (where from_utf8 fails) skip this check intentionally: ADO's attachment + // viewer won't execute ##vso[ sequences from binary content. Note that a binary file + // with a valid UTF-8 preamble but malformed tail will also skip the scan, but this is + // acceptable because the injection risk from binary attachments is negligible. if let Ok(text) = std::str::from_utf8(&file_bytes) { if text.contains("##vso[") { return Ok(ExecutionResult::failure(format!( From a80669a91c9de3fee6150d66afba3bfd0f2afa27 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 9 Apr 2026 09:04:28 +0000 Subject: [PATCH 08/15] =?UTF-8?q?fix:=20address=20PR=20review=20feedback?= =?UTF-8?q?=20=E2=80=94=20report-incomplete,=20percent-encode=20user=5Fid,?= =?UTF-8?q?=20status=20validation,=20merge=5Fstrategy=20validation,=20dead?= =?UTF-8?q?=20code=20removal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/474893c9-505a-48fb-a77e-768710027c85 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/execute.rs | 8 ++++--- src/tools/add_pr_comment.rs | 39 ++++++++++++++++++++++++++++++++++ src/tools/submit_pr_review.rs | 3 ++- src/tools/update_pr.rs | 32 +++++++++++++++++++++++++++- src/tools/upload_attachment.rs | 8 ------- 5 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/execute.rs b/src/execute.rs index 30fdad16..5db7cc74 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -14,7 +14,7 @@ use crate::tools::{ AddBuildTagResult, AddPrCommentResult, CreateBranchResult, CreateGitTagResult, CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, CommentOnWorkItemResult, ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, QueueBuildResult, - ReplyToPrCommentResult, ResolvePrThreadResult, SubmitPrReviewResult, + ReplyToPrCommentResult, ReportIncompleteResult, ResolvePrThreadResult, SubmitPrReviewResult, ToolResult, UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadAttachmentResult, }; @@ -382,8 +382,10 @@ pub async fn execute_safe_output( ExecutionResult::success("Skipped informational output: noop") } "report-incomplete" => { - debug!("Skipping report-incomplete entry"); - ExecutionResult::success("Skipped informational output: report-incomplete") + let output: ReportIncompleteResult = serde_json::from_value(entry.clone()) + .map_err(|e| anyhow::anyhow!("Failed to parse report-incomplete: {}", e))?; + debug!("report-incomplete: {}", output.reason); + ExecutionResult::failure(format!("Agent reported task incomplete: {}", output.reason)) } "missing-tool" => { debug!("Skipping missing-tool entry"); diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index 59a1798a..eef8424b 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -60,6 +60,11 @@ impl Validate for AddPrCommentParams { self.content.len() >= 10, "content must be at least 10 characters" ); + ensure!( + status_to_int(&self.status).is_some(), + "status must be one of: {}", + VALID_STATUSES.join(", ") + ); if self.line.is_some() { ensure!( self.file_path.is_some(), @@ -523,4 +528,38 @@ allowed-statuses: assert!(validate_file_path("src/main.rs").is_ok()); assert!(validate_file_path("path/to/file.txt").is_ok()); } + + #[test] + fn test_validation_rejects_invalid_status() { + let params = AddPrCommentParams { + pull_request_id: 42, + content: "This is a valid comment body text.".to_string(), + repository: "self".to_string(), + file_path: None, + start_line: None, + line: None, + status: "unknown".to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("status must be one of")); + } + + #[test] + fn test_validation_accepts_valid_statuses() { + for s in &["active", "fixed", "wont-fix", "closed", "by-design", "Active", "WontFix"] { + let params = AddPrCommentParams { + pull_request_id: 42, + content: "This is a valid comment body text.".to_string(), + repository: "self".to_string(), + file_path: None, + start_line: None, + line: None, + status: s.to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_ok(), "Expected status '{}' to be valid", s); + } + } } diff --git a/src/tools/submit_pr_review.rs b/src/tools/submit_pr_review.rs index c3f99aad..3490227e 100644 --- a/src/tools/submit_pr_review.rs +++ b/src/tools/submit_pr_review.rs @@ -252,9 +252,10 @@ impl Executor for SubmitPrReviewResult { debug!("Authenticated user ID: {}", user_id); // PUT vote to reviewers endpoint + let encoded_user_id = utf8_percent_encode(user_id, PATH_SEGMENT).to_string(); let vote_url = format!( "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id, user_id + base_url, encoded_repo, self.pull_request_id, encoded_user_id ); let vote_body = serde_json::json!({ "vote": vote_value diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index 9eb5c7aa..dbec6f6c 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -29,6 +29,9 @@ const VALID_VOTES: &[&str] = &[ "reset", ]; +/// Valid merge strategy values accepted by ADO's completionOptions.mergeStrategy +const VALID_MERGE_STRATEGIES: &[&str] = &["squash", "noFastForward", "rebase", "rebaseMerge"]; + /// Map a vote string to its ADO numeric value fn vote_to_ado_value(vote: &str) -> Option { match vote { @@ -383,6 +386,13 @@ impl UpdatePrResult { debug!("PR created by: {}", created_by_id); // PATCH to set auto-complete + if !VALID_MERGE_STRATEGIES.contains(&config.merge_strategy.as_str()) { + return Ok(ExecutionResult::failure(format!( + "Invalid merge-strategy '{}'. Must be one of: {}", + config.merge_strategy, + VALID_MERGE_STRATEGIES.join(", ") + ))); + } let patch_url = format!( "{}/{}/pullRequests/{}?api-version=7.1", base_url, encoded_repo, self.pull_request_id @@ -516,9 +526,10 @@ impl UpdatePrResult { // PUT vote to reviewers endpoint let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); + let encoded_user_id = utf8_percent_encode(user_id, PATH_SEGMENT).to_string(); let vote_url = format!( "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id, user_id + base_url, encoded_repo, self.pull_request_id, encoded_user_id ); let vote_body = serde_json::json!({ "vote": vote_value @@ -1016,6 +1027,7 @@ mod tests { assert!(config.allowed_operations.is_empty()); assert!(config.allowed_repositories.is_empty()); assert!(config.allowed_votes.is_empty()); + assert_eq!(config.merge_strategy, "squash"); } #[test] @@ -1037,4 +1049,22 @@ allowed-votes: assert_eq!(config.allowed_repositories.len(), 1); assert_eq!(config.allowed_votes.len(), 2); } + + #[test] + fn test_valid_merge_strategies() { + for strategy in VALID_MERGE_STRATEGIES { + assert!( + VALID_MERGE_STRATEGIES.contains(strategy), + "'{}' should be a valid merge strategy", + strategy + ); + } + } + + #[test] + fn test_config_deserializes_merge_strategy() { + let yaml = r#"merge-strategy: rebase"#; + let config: UpdatePrConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.merge_strategy, "rebase"); + } } diff --git a/src/tools/upload_attachment.rs b/src/tools/upload_attachment.rs index e2546795..b4acb929 100644 --- a/src/tools/upload_attachment.rs +++ b/src/tools/upload_attachment.rs @@ -184,14 +184,6 @@ impl Executor for UploadAttachmentResult { ))); } - // Check file exists (already confirmed by canonicalize, but kept for clarity) - if !canonical.exists() { - return Ok(ExecutionResult::failure(format!( - "File not found: {}", - resolved_path.display() - ))); - } - // Check file size let metadata = std::fs::metadata(&canonical) .context("Failed to read file metadata")?; From 65ae3caab82794a9bdd978aa23de61e32a5ab0ae Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 9 Apr 2026 09:09:30 +0000 Subject: [PATCH 09/15] fix: improve merge_strategy tests to be non-tautological Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/474893c9-505a-48fb-a77e-768710027c85 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/tools/update_pr.rs | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index dbec6f6c..4ce79fcf 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -1051,14 +1051,11 @@ allowed-votes: } #[test] - fn test_valid_merge_strategies() { - for strategy in VALID_MERGE_STRATEGIES { - assert!( - VALID_MERGE_STRATEGIES.contains(strategy), - "'{}' should be a valid merge strategy", - strategy - ); - } + fn test_valid_merge_strategies_are_expected_values() { + assert_eq!( + VALID_MERGE_STRATEGIES, + &["squash", "noFastForward", "rebase", "rebaseMerge"] + ); } #[test] @@ -1067,4 +1064,18 @@ allowed-votes: let config: UpdatePrConfig = serde_yaml::from_str(yaml).unwrap(); assert_eq!(config.merge_strategy, "rebase"); } + + #[test] + fn test_valid_merge_strategies_are_recognized() { + for strategy in VALID_MERGE_STRATEGIES { + assert!( + VALID_MERGE_STRATEGIES.contains(strategy), + "'{}' should be a valid merge strategy", + strategy + ); + } + // Ensure invalid strategy is NOT in the list + assert!(!VALID_MERGE_STRATEGIES.contains(&"invalid")); + assert!(!VALID_MERGE_STRATEGIES.contains(&"Squash")); + } } From d9e9939ac93cad06a0c3b6f26736f559ccaf0c07 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 9 Apr 2026 17:17:03 +0000 Subject: [PATCH 10/15] fix: merge_strategy guard before GET, per-component .. check, compile-time submit-pr-review validation Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/c36b2130-22bf-49ac-929f-0de6cada3ef6 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/common.rs | 79 +++++++++++++++++ src/compile/onees.rs | 4 +- src/compile/standalone.rs | 4 +- src/tools/update_pr.rs | 16 ++-- src/tools/upload_attachment.rs | 40 ++++++++- tests/compiler_tests.rs | 154 +++++++++++++++++++++++++++++++++ 6 files changed, 286 insertions(+), 11 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index d145cf74..a800c3e9 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -786,6 +786,38 @@ pub fn validate_update_work_item_target(front_matter: &FrontMatter) -> Result<() Ok(()) } +/// Validate that submit-pr-review has a required `allowed-events` field when configured. +/// +/// An empty or missing `allowed-events` list would allow agents to cast any review vote, +/// including auto-approvals. Operators must explicitly opt in to each allowed event. +pub fn validate_submit_pr_review_events(front_matter: &FrontMatter) -> Result<()> { + if let Some(config_value) = front_matter.safe_outputs.get("submit-pr-review") { + if let Some(obj) = config_value.as_object() { + let allowed_events = obj.get("allowed-events"); + let is_empty = match allowed_events { + None => true, + Some(v) => v.as_array().map_or(true, |a| a.is_empty()), + }; + if is_empty { + anyhow::bail!( + "safe-outputs.submit-pr-review requires a non-empty 'allowed-events' list \ + to prevent agents from casting unrestricted review votes. Example:\n\n \ + safe-outputs:\n submit-pr-review:\n allowed-events:\n \ + - comment\n - approve-with-suggestions\n\n\ + Valid events: approve, approve-with-suggestions, request-changes, comment\n" + ); + } + } else { + anyhow::bail!( + "safe-outputs.submit-pr-review must be a configuration object with an \ + 'allowed-events' list. Example:\n\n \ + safe-outputs:\n submit-pr-review:\n allowed-events:\n - comment\n" + ); + } + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -1353,4 +1385,51 @@ mod tests { let result = generate_pipeline_path(&abs_path); assert_eq!(result, "{{ workspace }}/agents/ctf.yml"); } + + // ─── validate_submit_pr_review_events ──────────────────────────────────── + + #[test] + fn test_submit_pr_review_events_passes_when_not_configured() { + let fm = minimal_front_matter(); + assert!(validate_submit_pr_review_events(&fm).is_ok()); + } + + #[test] + fn test_submit_pr_review_events_fails_when_allowed_events_missing() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n submit-pr-review:\n allowed-repositories:\n - self\n---\n" + ).unwrap(); + let result = validate_submit_pr_review_events(&fm); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("allowed-events"), "message: {msg}"); + } + + #[test] + fn test_submit_pr_review_events_fails_when_allowed_events_empty() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n submit-pr-review:\n allowed-events: []\n---\n" + ).unwrap(); + let result = validate_submit_pr_review_events(&fm); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("allowed-events"), "message: {msg}"); + } + + #[test] + fn test_submit_pr_review_events_fails_when_value_is_scalar() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n submit-pr-review: true\n---\n" + ).unwrap(); + let result = validate_submit_pr_review_events(&fm); + assert!(result.is_err()); + } + + #[test] + fn test_submit_pr_review_events_passes_when_events_provided() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n submit-pr-review:\n allowed-events:\n - comment\n - approve\n---\n" + ).unwrap(); + assert!(validate_submit_pr_review_events(&fm).is_ok()); + } } diff --git a/src/compile/onees.rs b/src/compile/onees.rs index 93de9d5a..c68f93e2 100644 --- a/src/compile/onees.rs +++ b/src/compile/onees.rs @@ -23,7 +23,7 @@ use super::common::{ generate_header_comment, generate_pipeline_path, generate_pipeline_resources, generate_pr_trigger, generate_repositories, generate_schedule, generate_source_path, generate_working_directory, replace_with_indent, validate_comment_target, - validate_update_work_item_target, validate_write_permissions, + validate_update_work_item_target, validate_write_permissions, validate_submit_pr_review_events, }; use super::types::{FrontMatter, McpConfig}; @@ -137,6 +137,8 @@ displayName: "Finalize""#, validate_comment_target(front_matter)?; // Validate update-work-item has required target field validate_update_work_item_target(front_matter)?; + // Validate submit-pr-review has required allowed-events field + validate_submit_pr_review_events(front_matter)?; // Replace all template markers let compiler_version = env!("CARGO_PKG_VERSION"); diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 2fde3739..65d74621 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -21,7 +21,7 @@ use super::common::{ generate_pipeline_resources, generate_pr_trigger, generate_repositories, generate_schedule, generate_source_path, generate_working_directory, replace_with_indent, sanitize_filename, validate_write_permissions, - validate_comment_target, validate_update_work_item_target, + validate_comment_target, validate_update_work_item_target, validate_submit_pr_review_events, }; use super::types::{FrontMatter, McpConfig}; use crate::allowed_hosts::{CORE_ALLOWED_HOSTS, mcp_required_hosts}; @@ -129,6 +129,8 @@ impl Compiler for StandaloneCompiler { validate_comment_target(front_matter)?; // Validate update-work-item has required target field validate_update_work_item_target(front_matter)?; + // Validate submit-pr-review has required allowed-events field + validate_submit_pr_review_events(front_matter)?; // Load threat analysis prompt template let threat_analysis_prompt = include_str!("../../templates/threat-analysis.md"); diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index 4ce79fcf..7cb7b153 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -345,6 +345,15 @@ impl UpdatePrResult { token: &str, config: &UpdatePrConfig, ) -> anyhow::Result { + // Validate merge_strategy before any network I/O + if !VALID_MERGE_STRATEGIES.contains(&config.merge_strategy.as_str()) { + return Ok(ExecutionResult::failure(format!( + "Invalid merge-strategy '{}'. Must be one of: {}", + config.merge_strategy, + VALID_MERGE_STRATEGIES.join(", ") + ))); + } + let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); // Fetch the PR to get createdBy.id @@ -386,13 +395,6 @@ impl UpdatePrResult { debug!("PR created by: {}", created_by_id); // PATCH to set auto-complete - if !VALID_MERGE_STRATEGIES.contains(&config.merge_strategy.as_str()) { - return Ok(ExecutionResult::failure(format!( - "Invalid merge-strategy '{}'. Must be one of: {}", - config.merge_strategy, - VALID_MERGE_STRATEGIES.join(", ") - ))); - } let patch_url = format!( "{}/{}/pullRequests/{}?api-version=7.1", base_url, encoded_repo, self.pull_request_id diff --git a/src/tools/upload_attachment.rs b/src/tools/upload_attachment.rs index b4acb929..86588278 100644 --- a/src/tools/upload_attachment.rs +++ b/src/tools/upload_attachment.rs @@ -29,8 +29,8 @@ impl Validate for UploadAttachmentParams { ensure!(self.work_item_id > 0, "work_item_id must be positive"); ensure!(!self.file_path.is_empty(), "file_path must not be empty"); ensure!( - !self.file_path.contains(".."), - "file_path must not contain '..'" + !self.file_path.split('/').any(|component| component == ".."), + "file_path must not contain '..' path components" ); ensure!( !self.file_path.starts_with('/'), @@ -415,6 +415,42 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_embedded_traversal() { + // "src/../secret" has ".." as a standalone component + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "src/../secret".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_accepts_filename_with_dots_in_name() { + // "report..v2.pdf" has ".." inside a filename, not as a standalone component + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "report..v2.pdf".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_ok(), "report..v2.pdf should be a valid filename"); + } + + #[test] + fn test_validation_accepts_directory_with_dots_in_name() { + // "v2..3/notes.md" — ".." inside a directory name, not a standalone component + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "v2..3/notes.md".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_ok(), "v2..3/notes.md should be valid"); + } + #[test] fn test_validation_rejects_absolute_path() { let params = UploadAttachmentParams { diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index d6eadb79..41cc8f74 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -1660,3 +1660,157 @@ fn test_compile_auto_discover_skips_missing_source() { let _ = fs::remove_dir_all(&temp_dir); } + +/// Test that submit-pr-review fails compilation when allowed-events is missing +#[test] +fn test_submit_pr_review_requires_allowed_events() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-spr-events-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let test_input = temp_dir.join("spr-agent.md"); + let test_content = r#"--- +name: "PR Review Agent" +description: "Agent that submits PR reviews but has no allowed-events" +permissions: + write: my-write-sc +safe-outputs: + submit-pr-review: + allowed-repositories: + - self +--- + +## PR Review Agent + +Submit PR reviews. +"#; + fs::write(&test_input, test_content).expect("Failed to write test input"); + + let output_path = temp_dir.join("spr-agent.yml"); + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + test_input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + !output.status.success(), + "Compiler should fail when submit-pr-review lacks allowed-events" + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("allowed-events"), + "Error message should mention allowed-events: {stderr}" + ); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// Test that submit-pr-review fails compilation when allowed-events is an empty list +#[test] +fn test_submit_pr_review_requires_nonempty_allowed_events() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-spr-empty-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let test_input = temp_dir.join("spr-agent.md"); + let test_content = r#"--- +name: "PR Review Agent" +description: "Agent that submits PR reviews but has empty allowed-events" +permissions: + write: my-write-sc +safe-outputs: + submit-pr-review: + allowed-events: [] +--- + +## PR Review Agent + +Submit PR reviews. +"#; + fs::write(&test_input, test_content).expect("Failed to write test input"); + + let output_path = temp_dir.join("spr-agent.yml"); + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + test_input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + !output.status.success(), + "Compiler should fail when submit-pr-review has empty allowed-events" + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("allowed-events"), + "Error message should mention allowed-events: {stderr}" + ); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// Test that submit-pr-review compiles successfully with proper config +#[test] +fn test_submit_pr_review_compiles_with_allowed_events() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-spr-pass-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let test_input = temp_dir.join("spr-agent.md"); + let test_content = r#"--- +name: "PR Review Agent" +description: "Agent that submits PR reviews with proper config" +permissions: + write: my-write-sc +safe-outputs: + submit-pr-review: + allowed-events: + - comment + - approve-with-suggestions +--- + +## PR Review Agent + +Submit PR reviews. +"#; + fs::write(&test_input, test_content).expect("Failed to write test input"); + + let output_path = temp_dir.join("spr-agent.yml"); + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + test_input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + output.status.success(), + "Compiler should succeed with proper submit-pr-review config: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let _ = fs::remove_dir_all(&temp_dir); +} From 33da3b1f499809286ecb2dfce4adc121aa65d4ab Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Apr 2026 08:39:33 +0000 Subject: [PATCH 11/15] =?UTF-8?q?fix:=20review=20thread=20status=204?= =?UTF-8?q?=E2=86=921,=20case-insensitive=20allowed=5Fstatuses,=20compile-?= =?UTF-8?q?time=20update-pr=20vote=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/147051a9-a11a-4967-9c19-9fd9a2093064 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/common.rs | 114 +++++++++++++++++++++++++ src/compile/onees.rs | 3 + src/compile/standalone.rs | 3 + src/tools/add_pr_comment.rs | 46 +++++++++- src/tools/submit_pr_review.rs | 2 +- tests/compiler_tests.rs | 152 ++++++++++++++++++++++++++++++++++ 6 files changed, 317 insertions(+), 3 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index a800c3e9..2bea373b 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -818,6 +818,51 @@ pub fn validate_submit_pr_review_events(front_matter: &FrontMatter) -> Result<() Ok(()) } +/// Validate that update-pr has a required `allowed-votes` field when the `vote` operation +/// is enabled (i.e., `allowed-operations` is empty — meaning all ops — or explicitly contains +/// "vote"). +/// +/// An empty `allowed-votes` list when vote is enabled would always fail at Stage 2 with a +/// runtime error. Catching this at compile time is consistent with how +/// `validate_submit_pr_review_events` handles the analogous case. +pub fn validate_update_pr_votes(front_matter: &FrontMatter) -> Result<()> { + if let Some(config_value) = front_matter.safe_outputs.get("update-pr") { + if let Some(obj) = config_value.as_object() { + // Determine whether the vote operation is reachable: + // - allowed-operations absent or empty → all operations allowed (includes vote) + // - allowed-operations non-empty → vote is allowed only if explicitly listed + let vote_reachable = match obj.get("allowed-operations") { + None => true, + Some(v) => v + .as_array() + .map_or(true, |a| a.is_empty() || a.iter().any(|x| x == "vote")), + }; + + if vote_reachable { + let allowed_votes_empty = match obj.get("allowed-votes") { + None => true, + Some(v) => v.as_array().map_or(true, |a| a.is_empty()), + }; + if allowed_votes_empty { + anyhow::bail!( + "safe-outputs.update-pr enables the 'vote' operation but has no \ + 'allowed-votes' list. This would reject all votes at Stage 2. \ + Either restrict 'allowed-operations' to exclude 'vote', or add an \ + explicit 'allowed-votes' list:\n\n \ + safe-outputs:\n update-pr:\n allowed-votes:\n \ + - approve-with-suggestions\n - wait-for-author\n\n\ + Valid votes: approve, approve-with-suggestions, reject, \ + wait-for-author, reset\n" + ); + } + } + } + // If the value is a scalar (e.g. `update-pr: true`) we don't error here — + // the config will default to empty allowed-votes, which is safe (vote always rejected). + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -1432,4 +1477,73 @@ mod tests { ).unwrap(); assert!(validate_submit_pr_review_events(&fm).is_ok()); } + + // ─── validate_update_pr_votes ───────────────────────────────────────────── + + #[test] + fn test_update_pr_votes_passes_when_not_configured() { + let fm = minimal_front_matter(); + assert!(validate_update_pr_votes(&fm).is_ok()); + } + + #[test] + fn test_update_pr_votes_fails_when_vote_reachable_and_no_allowed_votes() { + // allowed-operations absent → vote is reachable; no allowed-votes → should fail + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n update-pr:\n allowed-repositories:\n - self\n---\n" + ).unwrap(); + let result = validate_update_pr_votes(&fm); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("allowed-votes"), "message: {msg}"); + } + + #[test] + fn test_update_pr_votes_fails_when_vote_explicit_and_no_allowed_votes() { + // allowed-operations contains "vote"; no allowed-votes → should fail + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n update-pr:\n allowed-operations:\n - vote\n---\n" + ).unwrap(); + let result = validate_update_pr_votes(&fm); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("allowed-votes"), "message: {msg}"); + } + + #[test] + fn test_update_pr_votes_fails_when_allowed_votes_empty() { + // allowed-operations absent; allowed-votes is empty list → should fail + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n update-pr:\n allowed-votes: []\n---\n" + ).unwrap(); + let result = validate_update_pr_votes(&fm); + assert!(result.is_err()); + } + + #[test] + fn test_update_pr_votes_passes_when_vote_excluded_from_allowed_operations() { + // allowed-operations is non-empty and does not contain "vote" → safe, no error + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n update-pr:\n allowed-operations:\n - add-reviewers\n - set-auto-complete\n---\n" + ).unwrap(); + assert!(validate_update_pr_votes(&fm).is_ok()); + } + + #[test] + fn test_update_pr_votes_passes_when_vote_reachable_and_allowed_votes_set() { + // allowed-operations absent; allowed-votes non-empty → OK + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n update-pr:\n allowed-votes:\n - approve-with-suggestions\n---\n" + ).unwrap(); + assert!(validate_update_pr_votes(&fm).is_ok()); + } + + #[test] + fn test_update_pr_votes_passes_when_vote_explicit_and_allowed_votes_set() { + // allowed-operations contains "vote"; allowed-votes non-empty → OK + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n update-pr:\n allowed-operations:\n - vote\n allowed-votes:\n - wait-for-author\n---\n" + ).unwrap(); + assert!(validate_update_pr_votes(&fm).is_ok()); + } } diff --git a/src/compile/onees.rs b/src/compile/onees.rs index c68f93e2..7f2d72f8 100644 --- a/src/compile/onees.rs +++ b/src/compile/onees.rs @@ -24,6 +24,7 @@ use super::common::{ generate_pr_trigger, generate_repositories, generate_schedule, generate_source_path, generate_working_directory, replace_with_indent, validate_comment_target, validate_update_work_item_target, validate_write_permissions, validate_submit_pr_review_events, + validate_update_pr_votes, }; use super::types::{FrontMatter, McpConfig}; @@ -139,6 +140,8 @@ displayName: "Finalize""#, validate_update_work_item_target(front_matter)?; // Validate submit-pr-review has required allowed-events field validate_submit_pr_review_events(front_matter)?; + // Validate update-pr vote operation has required allowed-votes field + validate_update_pr_votes(front_matter)?; // Replace all template markers let compiler_version = env!("CARGO_PKG_VERSION"); diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 65d74621..4f310fd9 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -22,6 +22,7 @@ use super::common::{ generate_schedule, generate_source_path, generate_working_directory, replace_with_indent, sanitize_filename, validate_write_permissions, validate_comment_target, validate_update_work_item_target, validate_submit_pr_review_events, + validate_update_pr_votes, }; use super::types::{FrontMatter, McpConfig}; use crate::allowed_hosts::{CORE_ALLOWED_HOSTS, mcp_required_hosts}; @@ -131,6 +132,8 @@ impl Compiler for StandaloneCompiler { validate_update_work_item_target(front_matter)?; // Validate submit-pr-review has required allowed-events field validate_submit_pr_review_events(front_matter)?; + // Validate update-pr vote operation has required allowed-votes field + validate_update_pr_votes(front_matter)?; // Load threat analysis prompt template let threat_analysis_prompt = include_str!("../../templates/threat-analysis.md"); diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index eef8424b..a1cab8a5 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -226,9 +226,12 @@ impl Executor for AddPrCommentResult { ))); } - // Validate status against allowed-statuses config + // Validate status against allowed-statuses config (case-insensitive) if !config.allowed_statuses.is_empty() - && !config.allowed_statuses.contains(&self.status) + && !config + .allowed_statuses + .iter() + .any(|s| s.eq_ignore_ascii_case(&self.status)) { return Ok(ExecutionResult::failure(format!( "Status '{}' is not in the allowed-statuses list", @@ -562,4 +565,43 @@ allowed-statuses: assert!(result.is_ok(), "Expected status '{}' to be valid", s); } } + + #[test] + fn test_allowed_statuses_case_insensitive_match() { + // Config has "Active" but agent sends "active" (canonical lowercase) — should be allowed + let config = AddPrCommentConfig { + comment_prefix: None, + allowed_repositories: Vec::new(), + allowed_statuses: vec!["Active".to_string(), "Closed".to_string()], + }; + // Test the exact comparison logic extracted from execute_impl + let status = "active"; + let matched = config + .allowed_statuses + .iter() + .any(|s| s.eq_ignore_ascii_case(status)); + assert!( + matched, + "lowercase 'active' should match config value 'Active'" + ); + } + + #[test] + fn test_allowed_statuses_case_insensitive_reverse() { + // Config has "active" but agent sends "Active" — should be allowed + let config = AddPrCommentConfig { + comment_prefix: None, + allowed_repositories: Vec::new(), + allowed_statuses: vec!["active".to_string()], + }; + let status = "Active"; + let matched = config + .allowed_statuses + .iter() + .any(|s| s.eq_ignore_ascii_case(status)); + assert!( + matched, + "uppercase 'Active' should match config value 'active'" + ); + } } diff --git a/src/tools/submit_pr_review.rs b/src/tools/submit_pr_review.rs index 3490227e..70f3dc0f 100644 --- a/src/tools/submit_pr_review.rs +++ b/src/tools/submit_pr_review.rs @@ -303,7 +303,7 @@ impl Executor for SubmitPrReviewResult { "content": body, "commentType": 1 }], - "status": 4 + "status": 1 }); info!( diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 41cc8f74..93fa998c 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -1814,3 +1814,155 @@ Submit PR reviews. let _ = fs::remove_dir_all(&temp_dir); } + +/// Test that update-pr fails compilation when vote is reachable but allowed-votes is missing +#[test] +fn test_update_pr_requires_allowed_votes_when_vote_reachable() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-uprvote-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let test_input = temp_dir.join("upr-agent.md"); + // No allowed-operations → vote is reachable; no allowed-votes → should fail + let test_content = r#"--- +name: "Update PR Agent" +description: "Agent that votes on PRs but forgot to set allowed-votes" +permissions: + write: my-write-sc +safe-outputs: + update-pr: + allowed-repositories: + - self +--- + +## Update PR Agent + +Vote on pull requests. +"#; + fs::write(&test_input, test_content).expect("Failed to write test input"); + + let output_path = temp_dir.join("upr-agent.yml"); + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + test_input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + !output.status.success(), + "Compiler should fail when update-pr lacks allowed-votes with vote reachable" + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("allowed-votes"), + "Error message should mention allowed-votes: {stderr}" + ); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// Test that update-pr compiles successfully when vote is restricted via allowed-operations +#[test] +fn test_update_pr_compiles_when_vote_excluded_from_allowed_operations() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-uprnotvote-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let test_input = temp_dir.join("upr-agent.md"); + let test_content = r#"--- +name: "Update PR Agent" +description: "Agent that sets reviewers but cannot vote" +permissions: + write: my-write-sc +safe-outputs: + update-pr: + allowed-operations: + - add-reviewers + - set-auto-complete +--- + +## Update PR Agent + +Manage pull requests. +"#; + fs::write(&test_input, test_content).expect("Failed to write test input"); + + let output_path = temp_dir.join("upr-agent.yml"); + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + test_input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + output.status.success(), + "Compiler should succeed when vote is excluded from allowed-operations: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// Test that update-pr compiles successfully when allowed-votes is set +#[test] +fn test_update_pr_compiles_when_allowed_votes_set() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-uprvoteset-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let test_input = temp_dir.join("upr-agent.md"); + let test_content = r#"--- +name: "Update PR Agent" +description: "Agent that can vote on PRs with proper config" +permissions: + write: my-write-sc +safe-outputs: + update-pr: + allowed-votes: + - approve-with-suggestions + - wait-for-author +--- + +## Update PR Agent + +Vote on pull requests. +"#; + fs::write(&test_input, test_content).expect("Failed to write test input"); + + let output_path = temp_dir.join("upr-agent.yml"); + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + test_input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + output.status.success(), + "Compiler should succeed with proper update-pr vote config: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let _ = fs::remove_dir_all(&temp_dir); +} From 9d01857fe3a0b6ce684485124e5cfac69f2240b4 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 11 Apr 2026 09:26:39 +0100 Subject: [PATCH 12/15] fix: flush NDJSON file after write to prevent stale reads tokio::fs::File::write_all may buffer data internally without flushing to the filesystem. Under load (e.g., full test suite), a subsequent read_to_string could see stale data, causing write_safe_output_file_with_maximum to miss already-written entries and exceed the configured limit. Adding an explicit flush() ensures data reaches the filesystem before the function returns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/ndjson.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ndjson.rs b/src/ndjson.rs index e75d13d8..ceb63113 100644 --- a/src/ndjson.rs +++ b/src/ndjson.rs @@ -51,6 +51,9 @@ pub async fn append_to_ndjson_file(path: &Path, value: &T) -> Res file.write_all(line.as_bytes()) .await .with_context(|| format!("Failed to write to NDJSON file: {}", path.display()))?; + file.flush() + .await + .with_context(|| format!("Failed to flush NDJSON file: {}", path.display()))?; debug!("Appended {} bytes to NDJSON", line.len()); Ok(()) } From b3d3356725badc5a7c388710c0e4c3d5a1c1cec6 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 11 Apr 2026 09:43:51 +0100 Subject: [PATCH 13/15] fix: address 3 critical security issues from review 1. Self-approval guard for submit-pr-review and update-pr vote: Both voting paths now fetch the PR createdBy.id and block positive votes (approve, approve-with-suggestions) when the authenticated identity is the PR author. 2. ##vso[ pipeline command neutralization in core sanitize(): Added neutralize_pipeline_commands() step that wraps ##vso[ and ##[ sequences in backticks, preventing ADO from interpreting them as logging commands. This provides defense-in-depth for all text fields across all safe output tools. 3. resolve-pr-review-thread fail-closed allowed-statuses: Changed from permissive default (empty = all allowed) to fail-closed (empty = all rejected). Added compile-time validator validate_resolve_pr_thread_statuses() that requires explicit allowed-statuses config, matching the pattern of submit-pr-review allowed-events and update-pr allowed-votes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 82 ++++++++++++++++++++++++++++++++++ src/compile/onees.rs | 4 +- src/compile/standalone.rs | 4 +- src/sanitize.rs | 60 +++++++++++++++++++++++++ src/tools/resolve_pr_thread.rs | 21 ++++++--- src/tools/submit_pr_review.rs | 46 +++++++++++++++++++ src/tools/update_pr.rs | 48 ++++++++++++++++++++ 7 files changed, 258 insertions(+), 7 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 71ab5707..8a97b8cf 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -847,6 +847,41 @@ pub fn validate_update_pr_votes(front_matter: &FrontMatter) -> Result<()> { Ok(()) } +/// Validate that resolve-pr-review-thread has a required `allowed-statuses` field when configured. +/// +/// An empty or missing `allowed-statuses` list would let agents set any thread status, +/// including "fixed" or "wontFix" on security-critical review threads. Operators must +/// explicitly opt in to each allowed status transition. +pub fn validate_resolve_pr_thread_statuses(front_matter: &FrontMatter) -> Result<()> { + if let Some(config_value) = front_matter.safe_outputs.get("resolve-pr-review-thread") { + if let Some(obj) = config_value.as_object() { + let allowed_statuses = obj.get("allowed-statuses"); + let is_empty = match allowed_statuses { + None => true, + Some(v) => v.as_array().map_or(true, |a| a.is_empty()), + }; + if is_empty { + anyhow::bail!( + "safe-outputs.resolve-pr-review-thread requires a non-empty \ + 'allowed-statuses' list to prevent agents from manipulating thread \ + statuses without explicit operator consent. Example:\n\n \ + safe-outputs:\n resolve-pr-review-thread:\n allowed-statuses:\n\ + \x20 - fixed\n\n\ + Valid statuses: active, fixed, wont-fix, closed, by-design, pending\n" + ); + } + } else { + anyhow::bail!( + "safe-outputs.resolve-pr-review-thread must be a configuration object \ + with an 'allowed-statuses' list. Example:\n\n \ + safe-outputs:\n resolve-pr-review-thread:\n allowed-statuses:\n\ + \x20 - fixed\n" + ); + } + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -1554,4 +1589,51 @@ mod tests { ).unwrap(); assert!(validate_update_pr_votes(&fm).is_ok()); } + + // ─── validate_resolve_pr_thread_statuses ────────────────────────────────── + + #[test] + fn test_resolve_pr_thread_passes_when_not_configured() { + let fm = minimal_front_matter(); + assert!(validate_resolve_pr_thread_statuses(&fm).is_ok()); + } + + #[test] + fn test_resolve_pr_thread_fails_when_allowed_statuses_missing() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n resolve-pr-review-thread:\n allowed-repositories:\n - self\n---\n" + ).unwrap(); + let result = validate_resolve_pr_thread_statuses(&fm); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("allowed-statuses"), "message: {msg}"); + } + + #[test] + fn test_resolve_pr_thread_fails_when_allowed_statuses_empty() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n resolve-pr-review-thread:\n allowed-statuses: []\n---\n" + ).unwrap(); + let result = validate_resolve_pr_thread_statuses(&fm); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("allowed-statuses"), "message: {msg}"); + } + + #[test] + fn test_resolve_pr_thread_fails_when_value_is_scalar() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n resolve-pr-review-thread: true\n---\n" + ).unwrap(); + let result = validate_resolve_pr_thread_statuses(&fm); + assert!(result.is_err()); + } + + #[test] + fn test_resolve_pr_thread_passes_when_statuses_provided() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n resolve-pr-review-thread:\n allowed-statuses:\n - fixed\n - wont-fix\n---\n" + ).unwrap(); + assert!(validate_resolve_pr_thread_statuses(&fm).is_ok()); + } } diff --git a/src/compile/onees.rs b/src/compile/onees.rs index d007fda0..55caf74d 100644 --- a/src/compile/onees.rs +++ b/src/compile/onees.rs @@ -25,7 +25,7 @@ use super::common::{ generate_schedule, generate_source_path, generate_working_directory, replace_with_indent, validate_comment_target, validate_update_work_item_target, validate_write_permissions, validate_submit_pr_review_events, - validate_update_pr_votes, + validate_update_pr_votes, validate_resolve_pr_thread_statuses, }; use super::types::{FrontMatter, McpConfig}; @@ -144,6 +144,8 @@ displayName: "Finalize""#, validate_submit_pr_review_events(front_matter)?; // Validate update-pr vote operation has required allowed-votes field validate_update_pr_votes(front_matter)?; + // Validate resolve-pr-review-thread has required allowed-statuses field + validate_resolve_pr_thread_statuses(front_matter)?; // Replace all template markers let compiler_version = env!("CARGO_PKG_VERSION"); diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 773b8a3c..a0dd20c1 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -22,7 +22,7 @@ use super::common::{ generate_repositories, generate_schedule, generate_source_path, generate_working_directory, replace_with_indent, sanitize_filename, validate_write_permissions, validate_comment_target, validate_update_work_item_target, validate_submit_pr_review_events, - validate_update_pr_votes, + validate_update_pr_votes, validate_resolve_pr_thread_statuses, }; use super::types::{FrontMatter, McpConfig}; use crate::allowed_hosts::{CORE_ALLOWED_HOSTS, mcp_required_hosts}; @@ -135,6 +135,8 @@ impl Compiler for StandaloneCompiler { validate_submit_pr_review_events(front_matter)?; // Validate update-pr vote operation has required allowed-votes field validate_update_pr_votes(front_matter)?; + // Validate resolve-pr-review-thread has required allowed-statuses field + validate_resolve_pr_thread_statuses(front_matter)?; // Load threat analysis prompt template let threat_analysis_prompt = include_str!("../../templates/threat-analysis.md"); diff --git a/src/sanitize.rs b/src/sanitize.rs index 598b9bfe..136ebd87 100644 --- a/src/sanitize.rs +++ b/src/sanitize.rs @@ -34,6 +34,7 @@ const MAX_LINE_COUNT: usize = 65_536; /// 7. Apply content size limits (IS-08) pub fn sanitize(input: &str) -> String { let mut s = remove_control_characters(input); + s = neutralize_pipeline_commands(&s); s = neutralize_mentions(&s); s = neutralize_bot_triggers(&s); s = remove_xml_comments(&s); @@ -85,6 +86,37 @@ fn remove_control_characters(input: &str) -> String { result } +// ── Azure DevOps pipeline command neutralization ─────────────────────────── + +/// Neutralize `##vso[` logging command sequences that Azure DevOps interprets +/// when they appear in pipeline stdout/stderr. Wraps them in backticks so they +/// render as code instead of being executed. +/// +/// Also handles `##[` (the shorthand form used for `##[section]`, `##[error]`, +/// etc.) which ADO pipelines also interpret. +fn neutralize_pipeline_commands(input: &str) -> String { + let mut result = String::with_capacity(input.len() + 32); + let mut rest = input; + + while let Some(pos) = rest.find("##") { + result.push_str(&rest[..pos]); + let after = &rest[pos + 2..]; + if after.starts_with("vso[") { + result.push_str("`##vso[`"); + rest = &after[4..]; + } else if after.starts_with('[') { + result.push_str("`##[`"); + rest = &after[1..]; + } else { + // Harmless "##" (e.g. markdown heading) + result.push_str("##"); + rest = after; + } + } + result.push_str(rest); + result +} + // ── IS-04: @mention neutralization ───────────────────────────────────────── /// Wrap @mentions in backticks to prevent unintended notifications. @@ -482,4 +514,32 @@ mod tests { let input = "This is a normal description of a work item."; assert_eq!(sanitize(input), input); } + + // ── Pipeline command neutralization tests ────────────────────────────── + + #[test] + fn test_neutralize_vso_command() { + let input = "##vso[task.setvariable variable=secret]hack"; + let result = sanitize(input); + // The raw ##vso[ should be wrapped in backticks so ADO won't interpret it + assert!(result.contains("`##vso[`")); + // Verify it's not present in its original unescaped form (i.e. without backtick prefix) + assert!(!result.contains("##vso[task.")); + } + + #[test] + fn test_neutralize_vso_shorthand() { + let input = "##[error]Something bad happened"; + let result = sanitize(input); + assert!(result.contains("`##[`")); + } + + #[test] + fn test_neutralize_vso_preserves_single_hash() { + let input = "# Heading\n## Sub-heading\nIssue #123"; + let result = sanitize(input); + assert!(result.contains("# Heading")); + assert!(result.contains("## Sub-heading")); + assert!(result.contains("#123")); + } } diff --git a/src/tools/resolve_pr_thread.rs b/src/tools/resolve_pr_thread.rs index 5020865e..aa329cc3 100644 --- a/src/tools/resolve_pr_thread.rs +++ b/src/tools/resolve_pr_thread.rs @@ -114,7 +114,7 @@ pub struct ResolvePrThreadConfig { pub allowed_repositories: Vec, /// Restrict which thread statuses can be set. - /// If empty, all valid statuses are allowed. + /// REQUIRED — empty list rejects all status transitions. #[serde(default, rename = "allowed-statuses")] pub allowed_statuses: Vec, } @@ -153,10 +153,21 @@ impl Executor for ResolvePrThreadResult { let config: ResolvePrThreadConfig = ctx.get_tool_config("resolve-pr-review-thread"); debug!("Config: {:?}", config); - // Validate status against allowed-statuses config - if !config.allowed_statuses.is_empty() - && !config.allowed_statuses.contains(&self.status) - { + // Validate status against allowed-statuses — REQUIRED. + // An empty allowed-statuses list means the operator hasn't opted in, so reject. + // This prevents agents from resolving review threads (e.g. marking security + // concerns as "fixed") without explicit operator consent. + if config.allowed_statuses.is_empty() { + return Ok(ExecutionResult::failure( + "resolve-pr-review-thread requires 'allowed-statuses' to be configured in \ + safe-outputs.resolve-pr-review-thread. This prevents agents from \ + manipulating thread statuses without explicit operator consent. Example:\n \ + safe-outputs:\n resolve-pr-review-thread:\n allowed-statuses:\n \ + - fixed\n\nValid statuses: active, fixed, wont-fix, closed, by-design, pending" + .to_string(), + )); + } + if !config.allowed_statuses.contains(&self.status) { return Ok(ExecutionResult::failure(format!( "Status '{}' is not in the allowed-statuses list", self.status diff --git a/src/tools/submit_pr_review.rs b/src/tools/submit_pr_review.rs index 70f3dc0f..5cf1d1cb 100644 --- a/src/tools/submit_pr_review.rs +++ b/src/tools/submit_pr_review.rs @@ -251,6 +251,52 @@ impl Executor for SubmitPrReviewResult { .context("Connection data response missing authenticatedUser.id")?; debug!("Authenticated user ID: {}", user_id); + // Self-approval guard: prevent the agent from approving PRs it created. + // Positive votes (approve=10, approve-with-suggestions=5) are blocked when + // the authenticated user is also the PR author. + if vote_value > 0 { + let pr_url = format!( + "{}/{}/pullRequests/{}?api-version=7.1", + base_url, encoded_repo, self.pull_request_id + ); + let pr_response = client + .get(&pr_url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to fetch PR for self-approval check")?; + + if pr_response.status().is_success() { + let pr_body: serde_json::Value = pr_response + .json() + .await + .context("Failed to parse PR response")?; + + let creator_id = pr_body + .get("createdBy") + .and_then(|cb| cb.get("id")) + .and_then(|id| id.as_str()); + + if creator_id == Some(user_id) { + return Ok(ExecutionResult::failure(format!( + "Self-approval blocked: the authenticated identity created PR #{} \ + and cannot cast a positive vote ('{}') on it", + self.pull_request_id, self.event + ))); + } + } else { + let status = pr_response.status(); + let error_body = pr_response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to fetch PR #{} for self-approval check (HTTP {}): {}", + self.pull_request_id, status, error_body + ))); + } + } + // PUT vote to reviewers endpoint let encoded_user_id = utf8_percent_encode(user_id, PATH_SEGMENT).to_string(); let vote_url = format!( diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index 7cb7b153..e5c9367a 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -526,6 +526,54 @@ impl UpdatePrResult { .context("Connection data response missing authenticatedUser.id")?; debug!("Authenticated user ID: {}", user_id); + // Self-approval guard: prevent the agent from approving PRs it created. + // Positive votes (approve=10, approve-with-suggestions=5) are blocked when + // the authenticated user is also the PR author. + if vote_value > 0 { + let encoded_repo_check = + utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); + let pr_url = format!( + "{}/{}/pullRequests/{}?api-version=7.1", + base_url, encoded_repo_check, self.pull_request_id + ); + let pr_response = client + .get(&pr_url) + .basic_auth("", Some(token)) + .send() + .await + .context("Failed to fetch PR for self-approval check")?; + + if pr_response.status().is_success() { + let pr_body: serde_json::Value = pr_response + .json() + .await + .context("Failed to parse PR response")?; + + let creator_id = pr_body + .get("createdBy") + .and_then(|cb| cb.get("id")) + .and_then(|id| id.as_str()); + + if creator_id == Some(user_id) { + return Ok(ExecutionResult::failure(format!( + "Self-approval blocked: the authenticated identity created PR #{} \ + and cannot cast a positive vote ('{}') on it", + self.pull_request_id, vote_str + ))); + } + } else { + let status = pr_response.status(); + let error_body = pr_response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to fetch PR #{} for self-approval check (HTTP {}): {}", + self.pull_request_id, status, error_body + ))); + } + } + // PUT vote to reviewers endpoint let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let encoded_user_id = utf8_percent_encode(user_id, PATH_SEGMENT).to_string(); From 738bb6e538bdc6dfbe5792b1a840f5c0f34c9e4d Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 11 Apr 2026 09:50:18 +0100 Subject: [PATCH 14/15] fix: address 7 high-severity issues from review 4. link-work-items: Added required target config field (reuses CommentTarget enum from comment-on-work-item). Execution now fails if target is not configured, preventing unrestricted cross-project work item linking. 5. queue-build: Added ADO variable injection check for template parameter values. Rejects values containing \$(, \${{ or \$[ syntax that could reference pipeline variables. 6. upload-attachment: Fixed path traversal via backslash by splitting on both / and \\ in .., .git, and absolute path checks. Added tests for backslash-based traversal. 7. create-git-tag + create-branch: Added comprehensive git ref name validation via shared validate_git_ref_name() helper. Rejects .., @{, ~, ^, :, ?, *, [, \\, //, .lock suffix, leading dots in path components, and trailing dots. 8. add-build-tag: Added allow-any-build config (default: false). When not explicitly enabled, only the current pipeline build (BUILD_BUILDID) can be tagged. 9. update-pr: Changed auto-complete to use the agent's own identity from _apis/connectiondata instead of the PR createdBy.id, providing correct audit attribution. 10. report-incomplete: Added sanitize_fields() call before using the reason field in execute.rs to prevent injection via unsanitized agent output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/execute.rs | 4 ++- src/tools/add_build_tag.rs | 24 ++++++++++++++++ src/tools/create_branch.rs | 4 ++- src/tools/create_git_tag.rs | 3 +- src/tools/link_work_items.rs | 39 +++++++++++++++++++++++++ src/tools/mod.rs | 36 +++++++++++++++++++++++ src/tools/queue_build.rs | 10 +++++++ src/tools/update_pr.rs | 52 +++++++++++++++++----------------- src/tools/upload_attachment.rs | 28 ++++++++++++++++-- 9 files changed, 168 insertions(+), 32 deletions(-) diff --git a/src/execute.rs b/src/execute.rs index 5db7cc74..9a7bea8c 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -10,6 +10,7 @@ use std::collections::HashMap; use std::path::Path; use crate::ndjson::{self, SAFE_OUTPUT_FILENAME}; +use crate::sanitize::Sanitize; use crate::tools::{ AddBuildTagResult, AddPrCommentResult, CreateBranchResult, CreateGitTagResult, CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, CommentOnWorkItemResult, @@ -382,8 +383,9 @@ pub async fn execute_safe_output( ExecutionResult::success("Skipped informational output: noop") } "report-incomplete" => { - let output: ReportIncompleteResult = serde_json::from_value(entry.clone()) + let mut output: ReportIncompleteResult = serde_json::from_value(entry.clone()) .map_err(|e| anyhow::anyhow!("Failed to parse report-incomplete: {}", e))?; + output.sanitize_fields(); debug!("report-incomplete: {}", output.reason); ExecutionResult::failure(format!("Agent reported task incomplete: {}", output.reason)) } diff --git a/src/tools/add_build_tag.rs b/src/tools/add_build_tag.rs index 8ccabf65..536ce236 100644 --- a/src/tools/add_build_tag.rs +++ b/src/tools/add_build_tag.rs @@ -71,6 +71,7 @@ impl Sanitize for AddBuildTagResult { /// - verified /// - release-candidate /// tag-prefix: "agent-" +/// allow-any-build: false /// ``` #[derive(Debug, Clone, Serialize, Deserialize)] pub struct AddBuildTagConfig { @@ -82,6 +83,11 @@ pub struct AddBuildTagConfig { /// Prefix prepended to all tags before applying #[serde(default, rename = "tag-prefix")] pub tag_prefix: Option, + + /// Whether the agent may tag any build. When false (default), only the + /// current pipeline build (BUILD_BUILDID) may be tagged. + #[serde(default, rename = "allow-any-build")] + pub allow_any_build: bool, } impl Default for AddBuildTagConfig { @@ -89,6 +95,7 @@ impl Default for AddBuildTagConfig { Self { allowed_tags: Vec::new(), tag_prefix: None, + allow_any_build: false, } } } @@ -119,6 +126,23 @@ impl Executor for AddBuildTagResult { let config: AddBuildTagConfig = ctx.get_tool_config("add-build-tag"); debug!("Config: {:?}", config); + // 2b. Scope check: by default only the current build can be tagged + if !config.allow_any_build { + let current_build_id: Option = std::env::var("BUILD_BUILDID") + .ok() + .and_then(|s| s.parse().ok()); + if let Some(current_id) = current_build_id { + if self.build_id != current_id { + return Ok(ExecutionResult::failure(format!( + "Build #{} cannot be tagged: only the current build (#{}) is \ + allowed unless 'allow-any-build: true' is configured", + self.build_id, current_id + ))); + } + } + // If BUILD_BUILDID is not set (e.g. local execution), allow any build + } + // 3. Apply tag prefix if configured let final_tag = match &config.tag_prefix { Some(prefix) => format!("{}{}", prefix, self.tag), diff --git a/src/tools/create_branch.rs b/src/tools/create_branch.rs index 8d70a269..6df71669 100644 --- a/src/tools/create_branch.rs +++ b/src/tools/create_branch.rs @@ -5,7 +5,7 @@ use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use super::PATH_SEGMENT; +use super::{PATH_SEGMENT, validate_git_ref_name}; use crate::sanitize::{Sanitize, sanitize as sanitize_text}; use crate::tool_result; use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; @@ -50,6 +50,7 @@ impl Validate for CreateBranchParams { !self.branch_name.contains(' '), "branch_name must not contain spaces" ); + validate_git_ref_name(&self.branch_name, "branch_name")?; if let Some(ref commit) = self.source_commit { ensure!( @@ -67,6 +68,7 @@ impl Validate for CreateBranchParams { !branch.contains('\0'), "source_branch must not contain null bytes" ); + validate_git_ref_name(branch, "source_branch")?; } Ok(()) diff --git a/src/tools/create_git_tag.rs b/src/tools/create_git_tag.rs index 01240011..f7986f9d 100644 --- a/src/tools/create_git_tag.rs +++ b/src/tools/create_git_tag.rs @@ -5,7 +5,7 @@ use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use super::PATH_SEGMENT; +use super::{PATH_SEGMENT, validate_git_ref_name}; use crate::sanitize::{Sanitize, sanitize as sanitize_text}; use crate::tool_result; use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; @@ -53,6 +53,7 @@ impl Validate for CreateGitTagParams { "tag_name contains invalid characters (only alphanumeric, dots, dashes, underscores, and slashes are allowed): {}", self.tag_name ); + validate_git_ref_name(&self.tag_name, "tag_name")?; if let Some(commit) = &self.commit { ensure!( diff --git a/src/tools/link_work_items.rs b/src/tools/link_work_items.rs index 4410ae66..71609cd7 100644 --- a/src/tools/link_work_items.rs +++ b/src/tools/link_work_items.rs @@ -9,6 +9,7 @@ use super::PATH_SEGMENT; use crate::sanitize::{Sanitize, sanitize as sanitize_text}; use crate::tool_result; use crate::tools::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::tools::comment_on_work_item::CommentTarget; use anyhow::{Context, ensure}; /// Resolve a human-friendly link type name to the ADO relation type string. @@ -102,6 +103,7 @@ impl Sanitize for LinkWorkItemsResult { /// ```yaml /// safe-outputs: /// link-work-items: +/// target: "*" /// allowed-link-types: /// - parent /// - child @@ -113,12 +115,19 @@ pub struct LinkWorkItemsConfig { /// An empty list (the default) means all link types are allowed. #[serde(default, rename = "allowed-link-types")] pub allowed_link_types: Vec, + + /// Target scope — which work items can be linked. + /// `None` means no target was configured; execution must reject this. + /// Accepts the same values as comment-on-work-item: "*", a single ID, + /// a list of IDs, or an area path string. + pub target: Option, } impl Default for LinkWorkItemsConfig { fn default() -> Self { Self { allowed_link_types: Vec::new(), + target: None, } } } @@ -148,6 +157,36 @@ impl Executor for LinkWorkItemsResult { let config: LinkWorkItemsConfig = ctx.get_tool_config("link-work-items"); debug!("Allowed link types: {:?}", config.allowed_link_types); + // Validate work item IDs against target scope + match &config.target { + None => { + return Ok(ExecutionResult::failure( + "link-work-items requires a 'target' field in safe-outputs configuration \ + to scope which work items can be linked. Example:\n safe-outputs:\n \ + link-work-items:\n target: \"*\"" + .to_string(), + )); + } + Some(target) => { + // Check source_id + if let Some(false) = target.allows_id(self.source_id) { + return Ok(ExecutionResult::failure(format!( + "Source work item #{} is not allowed by the configured target scope", + self.source_id + ))); + } + // Check target_id + if let Some(false) = target.allows_id(self.target_id) { + return Ok(ExecutionResult::failure(format!( + "Target work item #{} is not allowed by the configured target scope", + self.target_id + ))); + } + // Area path validation is deferred — would need API calls for both IDs. + // For now, ID-based and wildcard scoping is enforced. + } + } + // Validate link type against configured allow-list if !config.allowed_link_types.is_empty() && !config.allowed_link_types.contains(&self.link_type) diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 1a6cf2f7..38972265 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -129,6 +129,42 @@ pub(crate) fn resolve_repo_name( } } +/// Validate a string against `git check-ref-format` rules. +/// +/// Returns `Ok(())` if the name is valid, or an `Err` describing the violation. +/// This covers the structural rules that Azure DevOps also enforces — catching +/// them early gives clearer error messages than letting the API fail. +pub(crate) fn validate_git_ref_name(name: &str, label: &str) -> anyhow::Result<()> { + use anyhow::ensure; + + ensure!(!name.is_empty(), "{label} must not be empty"); + ensure!(!name.contains(".."), "{label} must not contain '..'"); + ensure!(!name.contains("@{{"), "{label} must not contain '@{{'"); + ensure!(!name.ends_with('.'), "{label} must not end with '.'"); + ensure!(!name.ends_with(".lock"), "{label} must not end with '.lock'"); + ensure!( + !name.contains('\\'), + "{label} must not contain backslash" + ); + ensure!( + !name.contains("//"), + "{label} must not contain consecutive slashes" + ); + for ch in ['~', '^', ':', '?', '*', '['] { + ensure!( + !name.contains(ch), + "{label} must not contain '{ch}'" + ); + } + for component in name.split('/') { + ensure!( + !component.starts_with('.'), + "{label} path component must not start with '.'" + ); + } + Ok(()) +} + mod add_build_tag; mod add_pr_comment; mod comment_on_work_item; diff --git a/src/tools/queue_build.rs b/src/tools/queue_build.rs index 18e24e0c..853c3f57 100644 --- a/src/tools/queue_build.rs +++ b/src/tools/queue_build.rs @@ -208,6 +208,16 @@ impl Executor for QueueBuildResult { } } } + // Reject parameter values containing ADO variable/expression syntax + for (key, value) in params { + if value.contains("$(") || value.contains("${{") || value.contains("$[") { + return Ok(ExecutionResult::failure(format!( + "Parameter '{}' value contains ADO variable/expression syntax \ + which is not allowed", + key + ))); + } + } } // Build the source branch ref diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index e5c9367a..aa670cbd 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -292,7 +292,7 @@ impl Executor for UpdatePrResult { match self.operation.as_str() { "set-auto-complete" => { - self.execute_set_auto_complete(&client, &base_url, &repo_name, token, &config) + self.execute_set_auto_complete(&client, &base_url, &repo_name, token, org_url, &config) .await } "vote" => { @@ -335,14 +335,16 @@ impl Executor for UpdatePrResult { impl UpdatePrResult { /// Set auto-complete on a pull request. /// - /// First fetches the PR to get the `createdBy.id`, then patches the PR - /// with `autoCompleteSetBy` and default completion options. + /// Resolves the authenticated user identity via `_apis/connectiondata`, then + /// patches the PR with `autoCompleteSetBy` and default completion options. + /// Uses the agent's own identity (not the PR creator) for proper audit trail. async fn execute_set_auto_complete( &self, client: &reqwest::Client, base_url: &str, repo_name: &str, token: &str, + org_url: &str, config: &UpdatePrConfig, ) -> anyhow::Result { // Validate merge_strategy before any network I/O @@ -356,52 +358,50 @@ impl UpdatePrResult { let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); - // Fetch the PR to get createdBy.id - let get_url = format!( - "{}/{}/pullRequests/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id + // Resolve the agent's identity via connection data + let connection_url = format!( + "{}/_apis/connectiondata", + org_url.trim_end_matches('/') ); - debug!("GET PR URL: {}", get_url); - - let pr_response = client - .get(&get_url) + let conn_response = client + .get(&connection_url) .basic_auth("", Some(token)) .send() .await - .context("Failed to fetch pull request")?; + .context("Failed to fetch connection data for auto-complete identity")?; - if !pr_response.status().is_success() { - let status = pr_response.status(); - let error_body = pr_response + if !conn_response.status().is_success() { + let status = conn_response.status(); + let error_body = conn_response .text() .await .unwrap_or_else(|_| "Unknown error".to_string()); return Ok(ExecutionResult::failure(format!( - "Failed to fetch PR #{} (HTTP {}): {}", - self.pull_request_id, status, error_body + "Failed to fetch connection data (HTTP {}): {}", + status, error_body ))); } - let pr_body: serde_json::Value = pr_response + let conn_body: serde_json::Value = conn_response .json() .await - .context("Failed to parse PR response")?; + .context("Failed to parse connection data response")?; - let created_by_id = pr_body - .get("createdBy") - .and_then(|cb| cb.get("id")) + let agent_user_id = conn_body + .get("authenticatedUser") + .and_then(|au| au.get("id")) .and_then(|id| id.as_str()) - .context("PR response missing createdBy.id")?; - debug!("PR created by: {}", created_by_id); + .context("Connection data response missing authenticatedUser.id")?; + debug!("Agent user ID for auto-complete: {}", agent_user_id); - // PATCH to set auto-complete + // PATCH to set auto-complete using the agent's identity let patch_url = format!( "{}/{}/pullRequests/{}?api-version=7.1", base_url, encoded_repo, self.pull_request_id ); let patch_body = serde_json::json!({ "autoCompleteSetBy": { - "id": created_by_id + "id": agent_user_id }, "completionOptions": { "deleteSourceBranch": config.delete_source_branch, diff --git a/src/tools/upload_attachment.rs b/src/tools/upload_attachment.rs index 86588278..b258dfaf 100644 --- a/src/tools/upload_attachment.rs +++ b/src/tools/upload_attachment.rs @@ -29,11 +29,11 @@ impl Validate for UploadAttachmentParams { ensure!(self.work_item_id > 0, "work_item_id must be positive"); ensure!(!self.file_path.is_empty(), "file_path must not be empty"); ensure!( - !self.file_path.split('/').any(|component| component == ".."), + !self.file_path.split(['/', '\\']).any(|component| component == ".."), "file_path must not contain '..' path components" ); ensure!( - !self.file_path.starts_with('/'), + !self.file_path.starts_with('/') && !self.file_path.starts_with('\\'), "file_path must not be an absolute path" ); ensure!( @@ -47,7 +47,7 @@ impl Validate for UploadAttachmentParams { ensure!( !self .file_path - .split('/') + .split(['/', '\\']) .any(|component| component == ".git"), "file_path must not contain '.git' components" ); @@ -427,6 +427,28 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_backslash_traversal() { + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "src\\..\\secret.txt".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + + #[test] + fn test_validation_rejects_backslash_absolute_path() { + let params = UploadAttachmentParams { + work_item_id: 42, + file_path: "\\etc\\passwd".to_string(), + comment: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + #[test] fn test_validation_accepts_filename_with_dots_in_name() { // "report..v2.pdf" has ".." inside a filename, not as a standalone component From 858284b03d78606d230e126b0915493c1ae7edf9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Apr 2026 09:06:47 +0000 Subject: [PATCH 15/15] fix: component-wise .. check in add_pr_comment, remove spurious pending status, use resolve_repo_name helper Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/91091a0b-42a6-4559-afa4-44d12b847a4e Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/common.rs | 2 +- src/tools/add_pr_comment.rs | 9 ++++++--- src/tools/resolve_pr_thread.rs | 25 ++++++++----------------- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 8a97b8cf..4c420d2e 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -867,7 +867,7 @@ pub fn validate_resolve_pr_thread_statuses(front_matter: &FrontMatter) -> Result statuses without explicit operator consent. Example:\n\n \ safe-outputs:\n resolve-pr-review-thread:\n allowed-statuses:\n\ \x20 - fixed\n\n\ - Valid statuses: active, fixed, wont-fix, closed, by-design, pending\n" + Valid statuses: active, fixed, wont-fix, closed, by-design\n" ); } } else { diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index a1cab8a5..b6672a5c 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -176,12 +176,12 @@ fn status_to_int(status: &str) -> Option { /// All valid thread status strings (kebab-case canonical form) const VALID_STATUSES: &[&str] = &["active", "fixed", "wont-fix", "closed", "by-design"]; -/// Validate a file path for inline comments: no `..`, not absolute +/// Validate a file path for inline comments: no `..` path traversal, not absolute fn validate_file_path(path: &str) -> anyhow::Result<()> { ensure!(!path.is_empty(), "file_path must not be empty"); ensure!( - !path.contains(".."), - "file_path must not contain '..'" + !path.split(['/', '\\']).any(|component| component == ".."), + "file_path must not contain a '..' path component" ); ensure!( !path.starts_with('/') && !path.starts_with('\\'), @@ -530,6 +530,9 @@ allowed-statuses: fn test_validate_file_path_accepts_valid() { assert!(validate_file_path("src/main.rs").is_ok()); assert!(validate_file_path("path/to/file.txt").is_ok()); + // ".." within a component name is not a traversal — must be accepted + assert!(validate_file_path("releases..notes/v1.md").is_ok()); + assert!(validate_file_path("v2..beta/file.txt").is_ok()); } #[test] diff --git a/src/tools/resolve_pr_thread.rs b/src/tools/resolve_pr_thread.rs index aa329cc3..7d54cbd1 100644 --- a/src/tools/resolve_pr_thread.rs +++ b/src/tools/resolve_pr_thread.rs @@ -5,6 +5,7 @@ use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use super::resolve_repo_name; use super::PATH_SEGMENT; use crate::sanitize::{Sanitize, sanitize as sanitize_text}; use crate::tool_result; @@ -163,7 +164,7 @@ impl Executor for ResolvePrThreadResult { safe-outputs.resolve-pr-review-thread. This prevents agents from \ manipulating thread statuses without explicit operator consent. Example:\n \ safe-outputs:\n resolve-pr-review-thread:\n allowed-statuses:\n \ - - fixed\n\nValid statuses: active, fixed, wont-fix, closed, by-design, pending" + - fixed\n\nValid statuses: active, fixed, wont-fix, closed, by-design" .to_string(), )); } @@ -201,22 +202,12 @@ impl Executor for ResolvePrThreadResult { } }; - // Resolve repository name - let repo_name = if effective_repo == "self" || effective_repo.is_empty() { - ctx.repository_name - .as_ref() - .context("BUILD_REPOSITORY_NAME not set and repository is 'self'")? - .clone() - } else { - match ctx.allowed_repositories.get(effective_repo) { - Some(name) => name.clone(), - None => { - return Ok(ExecutionResult::failure(format!( - "Repository alias '{}' not found in allowed repositories", - effective_repo - ))); - } - } + // Resolve repository alias to actual repo name via the shared helper. + // Treat an empty string the same as "self" (pipeline repository). + let alias = self.repository.as_deref().filter(|s| !s.is_empty()); + let repo_name = match resolve_repo_name(alias, ctx) { + Ok(name) => name, + Err(result) => return Ok(result), }; // Build the Azure DevOps REST API URL for updating a thread