From 3a2a7a2a7b2851ce7f9b3c64aae74b5fa9acd09a Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 5 May 2026 12:52:07 +0100 Subject: [PATCH 1/4] feat(safe-outputs): rename upload-build-artifact to upload-build-attachment and add upload-pipeline-artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename upload-build-artifact → upload-build-attachment to match ADO terminology (the tool uses the build attachments REST API, not the deprecated build artifacts API) - Add central canonical_safe_output_name() alias layer for backward compatibility: old front-matter keys, NDJSON entries, and config are normalized to the new name - Implement new upload-pipeline-artifact safe output that publishes files as pipeline artifacts visible in the ADO Artifacts tab (3-step REST: create container → upload file → associate artifact with build) - Add ado_project_id field to ExecutionContext (from SYSTEM_TEAMPROJECTID) - Update docs/safe-outputs.md with renamed tool, new tool, and attachment-type clarification Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/safe-outputs.md | 68 +- src/compile/common.rs | 24 +- src/execute.rs | 18 +- src/main.rs | 21 +- src/mcp.rs | 138 ++- src/safeoutputs/create_pr.rs | 1 + src/safeoutputs/mod.rs | 26 +- src/safeoutputs/result.rs | 3 + ...artifact.rs => upload_build_attachment.rs} | 141 +-- src/safeoutputs/upload_pipeline_artifact.rs | 854 ++++++++++++++++++ 10 files changed, 1192 insertions(+), 102 deletions(-) rename src/safeoutputs/{upload_build_artifact.rs => upload_build_attachment.rs} (90%) create mode 100644 src/safeoutputs/upload_pipeline_artifact.rs diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index d8dfb6c8..70a9f9c5 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -418,11 +418,21 @@ safe-outputs: max: 1 # Maximum per run (default: 1) ``` -### upload-build-artifact -Attaches a workspace file to an Azure DevOps build as a build attachment via the -ADO build attachments REST API +### upload-build-attachment + +> **Renamed from `upload-build-artifact`** — the old name is accepted as a +> deprecated alias for backward compatibility. + +Attaches a workspace file to an Azure DevOps build as a **build attachment** via +the ADO build attachments REST API (`PUT /_apis/build/builds/{buildId}/attachments/{type}/{name}`). +> **Important:** Build attachments are **not visible** in the standard Azure +> DevOps build summary UI. They are only accessible via the REST API or through +> a custom Azure DevOps extension that registers a tab matching the +> `attachment-type` value. For artifacts that should appear in the **Artifacts +> tab**, use [`upload-pipeline-artifact`](#upload-pipeline-artifact) instead. + **Omit `build_id` to target the current pipeline run** — the executor resolves the build ID from the `BUILD_BUILDID` environment variable automatically. When `build_id` is provided, the file is attached to that specific build — useful for @@ -435,13 +445,13 @@ API. **Agent parameters:** - `build_id` *(optional)* - Target build ID. Omit to attach to the current pipeline run. Must be positive when specified. -- `artifact_name` - Artifact name (1–100 chars, alphanumeric / `-` / `_` / `.`, no leading `.`) +- `artifact_name` - Attachment name (1–100 chars, alphanumeric / `-` / `_` / `.`, no leading `.`) - `file_path` - Relative path to the file in the workspace (no directory traversal) **Configuration options (front matter):** ```yaml safe-outputs: - upload-build-artifact: + upload-build-attachment: max-file-size: 52428800 # Maximum file size in bytes (default: 50 MB) allowed-extensions: [] # Optional — restrict file types (e.g., [".png", ".pdf", ".log"]) allowed-artifact-names: [] # Optional — restrict names (suffix `*` = prefix match) @@ -454,7 +464,53 @@ safe-outputs: **Notes:** - Single-file only; directory uploads are not supported. - When `build_id` is omitted and `allowed-build-ids` is configured, the allow-list check is skipped — the current build is implicitly trusted. -- The default `attachment-type` is `agent-artifact` so executor contributions are visually distinguishable from the build's own artifacts. + +**About `attachment-type`:** This is the `{type}` segment in the ADO build +attachments URL (`PUT .../attachments/{type}/{name}`). It acts as a category +label. Azure DevOps extensions can register to display attachments of a specific +type — for example, the built-in code coverage extension displays attachments +with type `CodeCoverageSummary`. The default `agent-artifact` is a custom type; +without a matching ADO extension installed, attachments with this type are only +accessible via the REST API. Change this only if you have a custom extension +that displays attachments of a specific type. Most users should use +[`upload-pipeline-artifact`](#upload-pipeline-artifact) for user-visible +artifacts instead. + +### upload-pipeline-artifact + +Publishes a workspace file as an Azure DevOps **pipeline artifact** that appears +in the **Artifacts tab** of the build summary page. Uses the ADO build artifacts +REST API (container creation + file upload + artifact association). + +**Omit `build_id` to target the current pipeline run** — the executor resolves +the build ID from the `BUILD_BUILDID` environment variable automatically. When +`build_id` is provided, the artifact is published to that specific build. + +The tool stages the file during Stage 1 (MCP) by copying it into the +safe-outputs directory; Stage 3 reads the staged copy and executes the three-step +REST API flow to create the artifact. + +**Agent parameters:** +- `build_id` *(optional)* - Target build ID. Omit to publish to the current pipeline run. Must be positive when specified. +- `artifact_name` - Artifact name shown in the Artifacts tab (1–100 chars, alphanumeric / `-` / `_` / `.`, no leading `.`) +- `file_path` - Relative path to the file in the workspace (no directory traversal) + +**Configuration options (front matter):** +```yaml +safe-outputs: + upload-pipeline-artifact: + max-file-size: 52428800 # Maximum file size in bytes (default: 50 MB) + allowed-extensions: [] # Optional — restrict file types (e.g., [".png", ".pdf", ".log"]) + allowed-artifact-names: [] # Optional — restrict names (suffix `*` = prefix match) + allowed-build-ids: [] # Optional — restrict target builds (skipped when targeting current build) + name-prefix: "" # Optional — prepended to the agent-supplied artifact name + max: 3 # Maximum per run (default: 3) +``` + +**Notes:** +- Single-file only; directory uploads are not supported. +- When `build_id` is omitted and `allowed-build-ids` is configured, the allow-list check is skipped — the current build is implicitly trusted. +- Requires `SYSTEM_TEAMPROJECTID` to be available in the execution environment (set automatically by Azure DevOps). ### cache-memory (moved to `tools:`) Memory is now configured as a first-class tool under `tools: cache-memory:` instead of `safe-outputs: memory:`. See the [Cache Memory section](./tools.md#cache-memory-cache-memory) in `docs/tools.md` for details. diff --git a/src/compile/common.rs b/src/compile/common.rs index 5e018087..77598429 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -929,7 +929,7 @@ pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> Stri /// to prevent shell injection when the args are embedded in bash commands. /// Unrecognized tool names emit a compile-time warning and are skipped. pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { - use crate::safeoutputs::{ALL_KNOWN_SAFE_OUTPUTS, ALWAYS_ON_TOOLS, NON_MCP_SAFE_OUTPUT_KEYS}; + use crate::safeoutputs::{ALL_KNOWN_SAFE_OUTPUTS, ALWAYS_ON_TOOLS, NON_MCP_SAFE_OUTPUT_KEYS, canonical_safe_output_name}; use std::collections::HashSet; if front_matter.safe_outputs.is_empty() { @@ -949,7 +949,15 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { ); continue; } - if NON_MCP_SAFE_OUTPUT_KEYS.contains(&key.as_str()) { + // Canonicalize deprecated aliases (e.g. upload-build-artifact → upload-build-attachment) + let canonical = canonical_safe_output_name(key); + if canonical != key { + eprintln!( + "Warning: safe-output key '{}' is deprecated — use '{}' instead", + key, canonical + ); + } + if NON_MCP_SAFE_OUTPUT_KEYS.contains(&canonical) { continue; } if key == "memory" { @@ -960,7 +968,7 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { ); continue; } - if !ALL_KNOWN_SAFE_OUTPUTS.contains(&key.as_str()) { + if !ALL_KNOWN_SAFE_OUTPUTS.contains(&canonical) { eprintln!( "Warning: unrecognized safe-output tool '{}' — skipping (no registered tool matches this name)", key @@ -968,8 +976,8 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { continue; } effective_mcp_tool_count += 1; - if seen.insert(key.clone()) { - tools.push(key.clone()); + if seen.insert(canonical.to_string()) { + tools.push(canonical.to_string()); } } @@ -1003,7 +1011,7 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { /// Validate that write-requiring safe-outputs have a write service connection configured. pub fn validate_write_permissions(front_matter: &FrontMatter) -> Result<()> { - use crate::safeoutputs::WRITE_REQUIRING_SAFE_OUTPUTS; + use crate::safeoutputs::{WRITE_REQUIRING_SAFE_OUTPUTS, canonical_safe_output_name}; let has_write_sc = front_matter .permissions @@ -1016,7 +1024,9 @@ pub fn validate_write_permissions(front_matter: &FrontMatter) -> Result<()> { let missing: Vec<&str> = WRITE_REQUIRING_SAFE_OUTPUTS .iter() - .filter(|name| front_matter.safe_outputs.contains_key(**name)) + .filter(|name| { + front_matter.safe_outputs.keys().any(|k| canonical_safe_output_name(k) == **name) + }) .copied() .collect(); diff --git a/src/execute.rs b/src/execute.rs index 743af2fd..8b9d65ae 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -18,8 +18,8 @@ use crate::safeoutputs::{ ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, MissingDataResult, MissingToolResult, NoopResult, QueueBuildResult, ReplyToPrCommentResult, ReportIncompleteResult, ResolvePrThreadResult, SubmitPrReviewResult, ToolResult, - UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadBuildArtifactResult, - UploadWorkitemAttachmentResult, + UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadBuildAttachmentResult, + UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, canonical_safe_output_name, }; // Re-export memory types for use by main.rs @@ -93,7 +93,8 @@ pub async fn execute_safe_outputs( AddBuildTagResult, CreateBranchResult, UpdatePrResult, - UploadBuildArtifactResult, + UploadBuildAttachmentResult, + UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, SubmitPrReviewResult, ReplyToPrCommentResult, @@ -191,7 +192,8 @@ fn enforce_budget( total: usize, i: usize, ) -> Option { - let tool_name = entry.get("name").and_then(|n| n.as_str())?; + let raw_name = entry.get("name").and_then(|n| n.as_str())?; + let tool_name = canonical_safe_output_name(raw_name); let (executed, max) = budgets.get_mut(tool_name)?; let context_id = extract_entry_context(entry); if let Some(result) = check_budget(total, i, tool_name, &context_id, *executed, *max) { @@ -249,12 +251,13 @@ pub async fn execute_safe_output( ctx: &ExecutionContext, ) -> Result<(String, ExecutionResult)> { // First check the name field to dispatch correctly - let tool_name = entry + let raw_tool_name = entry .get("name") .and_then(|n| n.as_str()) .ok_or_else(|| anyhow::anyhow!("Safe output missing 'name' field"))?; + let tool_name = canonical_safe_output_name(raw_tool_name); - debug!("Dispatching tool: {}", tool_name); + debug!("Dispatching tool: {} (raw: {})", tool_name, raw_tool_name); // Dispatch based on tool name. All registered tools go through `dispatch_tool`, // which handles deserialization and sanitized execution uniformly. @@ -346,7 +349,8 @@ async fn dispatch_resource_tools( "create-git-tag" => CreateGitTagResult, "add-build-tag" => AddBuildTagResult, "create-branch" => CreateBranchResult, - "upload-build-artifact" => UploadBuildArtifactResult, + "upload-build-attachment" => UploadBuildAttachmentResult, + "upload-pipeline-artifact" => UploadPipelineArtifactResult, }) } diff --git a/src/main.rs b/src/main.rs index 43ba366d..6afe680c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,6 +20,7 @@ pub mod validate; use anyhow::{Context, Result}; use clap::{Parser, Subcommand}; +use std::collections::HashMap; use std::path::PathBuf; #[derive(Subcommand, Debug)] @@ -248,7 +249,25 @@ async fn build_execution_context( ctx.ado_org_url = ado_org_url; ctx.ado_project = ado_project; ctx.working_directory = safe_output_dir.clone(); - ctx.tool_configs = front_matter.safe_outputs.clone(); + // Normalize deprecated safe-output config keys to their canonical names. + // This ensures old front matter (e.g. `upload-build-artifact:`) maps to + // the new tool name for config lookup at execution time. + ctx.tool_configs = { + use crate::safeoutputs::canonical_safe_output_name; + let mut normalized = HashMap::new(); + for (key, value) in &front_matter.safe_outputs { + let canonical = canonical_safe_output_name(key).to_string(); + if canonical != *key && normalized.contains_key(&canonical) { + eprintln!( + "Warning: both '{}' and '{}' are configured in safe-outputs — using '{}'", + key, canonical, canonical + ); + continue; + } + normalized.insert(canonical, value.clone()); + } + normalized + }; ctx.allowed_repositories = allowed_repositories; ctx.dry_run = dry_run; diff --git a/src/mcp.rs b/src/mcp.rs index 0b796584..5a056abe 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -27,7 +27,8 @@ use crate::safeoutputs::{ QueueBuildResult, SubmitPrReviewParams, SubmitPrReviewResult, ToolResult, UpdatePrParams, UpdatePrResult, UpdateWorkItemParams, UpdateWorkItemResult, - UploadBuildArtifactParams, UploadBuildArtifactResult, DEFAULT_MAX_FILE_SIZE, + UploadBuildAttachmentParams, UploadBuildAttachmentResult, DEFAULT_MAX_FILE_SIZE, + UploadPipelineArtifactParams, UploadPipelineArtifactResult, PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE, UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, anyhow_to_mcp_error, }; @@ -990,7 +991,7 @@ uploaded and linked during safe output processing. File size and type restrictio } #[tool( - name = "upload-build-artifact", + name = "upload-build-attachment", description = "Attach a workspace file to an Azure DevOps build as a build attachment. \ Omit `build_id` to target the current pipeline run (the executor resolves it from the \ BUILD_BUILDID environment variable automatically). When `build_id` is provided, the file is \ @@ -999,12 +1000,12 @@ generated report, screenshot, or log bundle. The file will be staged now and upl ADO build attachments REST API during safe output processing. File size, extension, \ artifact-name and build-id restrictions may apply per the workflow's safe-outputs config." )] - async fn upload_build_artifact( + async fn upload_build_attachment( &self, - params: Parameters, + params: Parameters, ) -> Result { info!( - "Tool called: upload-build-artifact - artifact '{}' file '{}' build {:?}", + "Tool called: upload-build-attachment - artifact '{}' file '{}' build {:?}", params.0.artifact_name, params.0.file_path, params.0.build_id ); @@ -1116,7 +1117,7 @@ artifact-name and build-id restrictions may apply per the workflow's safe-output )) })?; - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( params.0.build_id, params.0.artifact_name.clone(), params.0.file_path.clone(), @@ -1137,6 +1138,131 @@ artifact-name and build-id restrictions may apply per the workflow's safe-output ))])) } + #[tool( + name = "upload-pipeline-artifact", + description = "Publish a workspace file as an Azure DevOps pipeline artifact that appears in \ +the Artifacts tab of the build summary. Omit `build_id` to target the current pipeline run \ +(the executor resolves it from BUILD_BUILDID automatically). When `build_id` is provided, the \ +file is published to that specific build. The file will be staged now and uploaded via the ADO \ +build artifacts REST API during safe output processing. File size, extension, artifact-name and \ +build-id restrictions may apply per the workflow's safe-outputs config." + )] + async fn upload_pipeline_artifact( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: upload-pipeline-artifact - artifact '{}' file '{}' build {:?}", + params.0.artifact_name, params.0.file_path, params.0.build_id + ); + + crate::safeoutputs::Validate::validate(¶ms.0).map_err(anyhow_to_mcp_error)?; + + let resolved = self.bounding_directory.join(¶ms.0.file_path); + let canonical = resolved.canonicalize().map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "File '{}' could not be located inside the workspace: {}", + params.0.file_path, + e + )) + })?; + let canonical_root = self.bounding_directory.canonicalize().map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to canonicalize bounding directory: {}", + e + )) + })?; + if !canonical.starts_with(&canonical_root) { + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "File '{}' resolves outside the workspace (symlink escape)", + params.0.file_path + ))); + } + + let metadata = tokio::fs::metadata(&canonical).await.map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!("Failed to stat '{}': {}", params.0.file_path, e)) + })?; + if metadata.is_dir() { + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "File '{}' is a directory; upload-pipeline-artifact only supports single files", + params.0.file_path + ))); + } + let file_size = metadata.len(); + + if file_size > PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE { + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "File '{}' is {} bytes, exceeding the maximum staging size of {} bytes", + params.0.file_path, + file_size, + PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE + ))); + } + + let extension = std::path::Path::new(¶ms.0.file_path) + .extension() + .and_then(|s| s.to_str()) + .map(|s| { + s.chars() + .filter(|c| c.is_ascii_alphanumeric()) + .take(16) + .collect::() + }) + .unwrap_or_default(); + let staged_filename = if extension.is_empty() { + format!( + "upload-pipeline-artifact-{}-{}", + params.0.artifact_name, + generate_short_id() + ) + } else { + format!( + "upload-pipeline-artifact-{}-{}.{}", + params.0.artifact_name, + generate_short_id(), + extension + ) + }; + + let source_bytes = tokio::fs::read(&canonical).await.map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to read source file '{}': {}", + params.0.file_path, + e + )) + })?; + let staged_sha256 = crate::hash::sha256_hex(&source_bytes); + + let staged_path = self.output_directory.join(&staged_filename); + tokio::fs::write(&staged_path, &source_bytes).await.map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to stage file '{}' into safe-outputs directory: {}", + params.0.file_path, + e + )) + })?; + + let result = UploadPipelineArtifactResult::new( + params.0.build_id, + params.0.artifact_name.clone(), + params.0.file_path.clone(), + staged_filename.clone(), + file_size, + staged_sha256, + ); + self.write_safe_output_file(&result).await + .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; + + let build_desc = match params.0.build_id { + Some(id) => format!("build #{}", id), + None => "the current build".to_string(), + }; + Ok(CallToolResult::success(vec![Content::text(format!( + "Pipeline artifact '{}' queued from file '{}' ({} bytes) for {}. The artifact will appear in the Artifacts tab after safe output processing.", + result.artifact_name, result.file_path, file_size, build_desc + ))])) + } + #[tool( name = "submit-pr-review", description = "Submit a pull request review with a decision (approve, request-changes, \ diff --git a/src/safeoutputs/create_pr.rs b/src/safeoutputs/create_pr.rs index 154b4fa1..f571892e 100644 --- a/src/safeoutputs/create_pr.rs +++ b/src/safeoutputs/create_pr.rs @@ -2444,6 +2444,7 @@ new file mode 100755 ado_org_url: Some("https://dev.azure.com/test".to_string()), ado_organization: Some("test".to_string()), ado_project: Some("TestProject".to_string()), + ado_project_id: None, access_token: Some("fake-token".to_string()), source_directory: dir.path().to_path_buf(), working_directory: dir.path().to_path_buf(), diff --git a/src/safeoutputs/mod.rs b/src/safeoutputs/mod.rs index 1331e1ef..45b698be 100644 --- a/src/safeoutputs/mod.rs +++ b/src/safeoutputs/mod.rs @@ -42,7 +42,8 @@ pub const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = tool_names![ AddBuildTagResult, CreateBranchResult, UpdatePrResult, - UploadBuildArtifactResult, + UploadBuildAttachmentResult, + UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, SubmitPrReviewResult, ReplyToPrCommentResult, @@ -54,6 +55,17 @@ pub const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = tool_names![ /// filtered out (the router has no route for them). pub const NON_MCP_SAFE_OUTPUT_KEYS: &[&str] = &[]; +/// Canonicalize a safe-output tool name, resolving deprecated aliases to their +/// current names. Callers should apply this before budget enforcement, +/// config lookup, and dispatch so that old NDJSON entries / front-matter keys +/// consume the same budgets and configs as new ones. +pub fn canonical_safe_output_name(name: &str) -> &str { + match name { + "upload-build-artifact" => "upload-build-attachment", + other => other, + } +} + /// All recognised safe-output keys accepted in front matter `safe-outputs:`. /// This is the union of write-requiring tool types and diagnostic tool types. /// @@ -76,7 +88,8 @@ pub const ALL_KNOWN_SAFE_OUTPUTS: &[&str] = all_safe_output_names![ AddBuildTagResult, CreateBranchResult, UpdatePrResult, - UploadBuildArtifactResult, + UploadBuildAttachmentResult, + UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, SubmitPrReviewResult, ReplyToPrCommentResult, @@ -264,7 +277,8 @@ mod submit_pr_review; mod update_pr; mod update_wiki_page; mod update_work_item; -mod upload_build_artifact; +mod upload_build_attachment; +mod upload_pipeline_artifact; mod upload_workitem_attachment; pub use add_build_tag::*; @@ -290,7 +304,8 @@ pub use submit_pr_review::*; pub use update_pr::*; pub use update_wiki_page::*; pub use update_work_item::*; -pub use upload_build_artifact::*; +pub use upload_build_attachment::*; +pub use upload_pipeline_artifact::*; pub use upload_workitem_attachment::*; #[cfg(test)] @@ -348,7 +363,8 @@ mod tests { assert!(AddBuildTagResult::REQUIRES_WRITE); assert!(CreateBranchResult::REQUIRES_WRITE); assert!(UpdatePrResult::REQUIRES_WRITE); - assert!(UploadBuildArtifactResult::REQUIRES_WRITE); + assert!(UploadBuildAttachmentResult::REQUIRES_WRITE); + assert!(UploadPipelineArtifactResult::REQUIRES_WRITE); assert!(UploadWorkitemAttachmentResult::REQUIRES_WRITE); assert!(SubmitPrReviewResult::REQUIRES_WRITE); assert!(ReplyToPrCommentResult::REQUIRES_WRITE); diff --git a/src/safeoutputs/result.rs b/src/safeoutputs/result.rs index 5de9f2c3..b9ffb57a 100644 --- a/src/safeoutputs/result.rs +++ b/src/safeoutputs/result.rs @@ -42,6 +42,8 @@ pub struct ExecutionContext { pub ado_organization: Option, /// Azure DevOps project name pub ado_project: Option, + /// Azure DevOps project GUID (`SYSTEM_TEAMPROJECTID`) + pub ado_project_id: Option, /// Personal access token or system access token pub access_token: Option, /// Working directory for file operations (safe outputs directory) @@ -159,6 +161,7 @@ impl ExecutionContext { ado_org_url, ado_organization, ado_project: env("SYSTEM_TEAMPROJECT"), + ado_project_id: env("SYSTEM_TEAMPROJECTID"), access_token: env("SYSTEM_ACCESSTOKEN").or_else(|| env("AZURE_DEVOPS_EXT_PAT")), working_directory: std::env::current_dir().unwrap_or_default(), source_directory, diff --git a/src/safeoutputs/upload_build_artifact.rs b/src/safeoutputs/upload_build_attachment.rs similarity index 90% rename from src/safeoutputs/upload_build_artifact.rs rename to src/safeoutputs/upload_build_attachment.rs index e84b0abc..6b80dc44 100644 --- a/src/safeoutputs/upload_build_artifact.rs +++ b/src/safeoutputs/upload_build_attachment.rs @@ -1,4 +1,4 @@ -//! Upload build artifact safe output tool. +//! Upload build attachment safe output tool. //! //! Lets an agent propose attaching a workspace file to an Azure DevOps build //! via the **build attachments** REST API @@ -37,7 +37,7 @@ use anyhow::{Context, ensure}; /// Parameters for attaching a workspace file to an ADO build. #[derive(Deserialize, JsonSchema)] -pub struct UploadBuildArtifactParams { +pub struct UploadBuildAttachmentParams { /// The build ID to attach the file to. **Omit to target the current /// pipeline run** — the executor resolves the build ID from the /// `BUILD_BUILDID` environment variable automatically. When provided, @@ -56,7 +56,7 @@ pub struct UploadBuildArtifactParams { pub file_path: String, } -impl Validate for UploadBuildArtifactParams { +impl Validate for UploadBuildAttachmentParams { fn validate(&self) -> anyhow::Result<()> { // build_id: if present, must be positive. if let Some(id) = self.build_id { @@ -100,13 +100,13 @@ impl Validate for UploadBuildArtifactParams { } } -/// Internal params struct mirroring `UploadBuildArtifactResult` fields for the +/// Internal params struct mirroring `UploadBuildAttachmentResult` fields for the /// `tool_result!` macro's `TryFrom` plumbing. The actual MCP parameters come -/// from `UploadBuildArtifactParams`; this struct only exists so the macro can +/// from `UploadBuildAttachmentParams`; this struct only exists so the macro can /// wire up `Validate`/`TryFrom` while the real construction happens in MCP via -/// `UploadBuildArtifactResult::new()` after the file is staged. +/// `UploadBuildAttachmentResult::new()` after the file is staged. #[derive(Deserialize, JsonSchema)] -struct UploadBuildArtifactResultFields { +struct UploadBuildAttachmentResultFields { build_id: Option, artifact_name: String, file_path: String, @@ -115,15 +115,15 @@ struct UploadBuildArtifactResultFields { staged_sha256: String, } -impl Validate for UploadBuildArtifactResultFields {} +impl Validate for UploadBuildAttachmentResultFields {} tool_result! { - name = "upload-build-artifact", + name = "upload-build-attachment", write = true, - params = UploadBuildArtifactResultFields, + params = UploadBuildAttachmentResultFields, default_max = 3, /// Result of attaching a workspace file to an ADO build. - pub struct UploadBuildArtifactResult { + pub struct UploadBuildAttachmentResult { /// Build ID the file should be attached to. `None` means "current /// build" — resolved at execution time from `BUILD_BUILDID`. build_id: Option, @@ -147,14 +147,14 @@ tool_result! { } } -impl SanitizeContent for UploadBuildArtifactResult { +impl SanitizeContent for UploadBuildAttachmentResult { fn sanitize_content_fields(&mut self) { // All textual fields are strictly validated to safe charsets; no // additional textual sanitization is required. } } -impl UploadBuildArtifactResult { +impl UploadBuildAttachmentResult { /// Construct a result after the agent's file has been staged into the /// safe-outputs directory. pub fn new( @@ -177,18 +177,18 @@ impl UploadBuildArtifactResult { } } -/// Default maximum file size for upload-build-artifact (50 MB). +/// Default maximum file size for upload-build-attachment (50 MB). /// Also used by the MCP handler as the Stage 1 staging cap. pub const DEFAULT_MAX_FILE_SIZE: u64 = 50 * 1024 * 1024; const DEFAULT_ATTACHMENT_TYPE: &str = "agent-artifact"; -/// Configuration for the upload-build-artifact tool (specified in front +/// Configuration for the upload-build-attachment tool (specified in front /// matter). /// /// Example front matter: /// ```yaml /// safe-outputs: -/// upload-build-artifact: +/// upload-build-attachment: /// max-file-size: 52428800 /// allowed-extensions: /// - .png @@ -204,7 +204,7 @@ const DEFAULT_ATTACHMENT_TYPE: &str = "agent-artifact"; /// max: 5 /// ``` #[derive(Debug, Clone, SanitizeConfig, Serialize, Deserialize)] -pub struct UploadBuildArtifactConfig { +pub struct UploadBuildAttachmentConfig { /// Maximum file size in bytes (default: 50 MB). #[serde(default = "default_max_file_size", rename = "max-file-size")] pub max_file_size: u64, @@ -241,7 +241,7 @@ fn default_max_file_size() -> u64 { DEFAULT_MAX_FILE_SIZE } -impl Default for UploadBuildArtifactConfig { +impl Default for UploadBuildAttachmentConfig { fn default() -> Self { Self { max_file_size: DEFAULT_MAX_FILE_SIZE, @@ -255,7 +255,7 @@ impl Default for UploadBuildArtifactConfig { } #[async_trait::async_trait] -impl Executor for UploadBuildArtifactResult { +impl Executor for UploadBuildAttachmentResult { fn dry_run_summary(&self) -> String { match self.build_id { Some(id) => format!( @@ -291,11 +291,11 @@ impl Executor for UploadBuildArtifactResult { if self.build_id.is_none() { " (current build)" } else { "" } ); debug!( - "upload-build-artifact: build_id={}, artifact_name='{}', file_path='{}'", + "upload-build-attachment: build_id={}, artifact_name='{}', file_path='{}'", effective_build_id, self.artifact_name, self.file_path ); - let config: UploadBuildArtifactConfig = ctx.get_tool_config("upload-build-artifact"); + let config: UploadBuildAttachmentConfig = ctx.get_tool_config("upload-build-attachment"); debug!("Max file size: {} bytes", config.max_file_size); debug!("Allowed extensions: {:?}", config.allowed_extensions); debug!("Allowed artifact names: {:?}", config.allowed_artifact_names); @@ -429,7 +429,7 @@ impl Executor for UploadBuildArtifactResult { let metadata = std::fs::metadata(&canonical).context("Failed to read file metadata")?; if metadata.is_dir() { return Ok(ExecutionResult::failure(format!( - "Staged path '{}' is a directory; upload-build-artifact only supports single files", + "Staged path '{}' is a directory; upload-build-attachment only supports single files", self.staged_file ))); } @@ -576,15 +576,15 @@ mod tests { #[test] fn test_result_has_correct_name() { - assert_eq!(UploadBuildArtifactResult::NAME, "upload-build-artifact"); + assert_eq!(UploadBuildAttachmentResult::NAME, "upload-build-attachment"); } fn make_params( build_id: Option, artifact_name: &str, file_path: &str, - ) -> UploadBuildArtifactParams { - UploadBuildArtifactParams { + ) -> UploadBuildAttachmentParams { + UploadBuildAttachmentParams { build_id, artifact_name: artifact_name.to_string(), file_path: file_path.to_string(), @@ -605,7 +605,7 @@ mod tests { fn test_params_deserializes_with_build_id() { let json = r#"{"build_id": 1234, "artifact_name": "agent-report", "file_path": "out/report.pdf"}"#; - let params: UploadBuildArtifactParams = serde_json::from_str(json).unwrap(); + let params: UploadBuildAttachmentParams = serde_json::from_str(json).unwrap(); assert_eq!(params.build_id, Some(1234)); assert_eq!(params.artifact_name, "agent-report"); assert_eq!(params.file_path, "out/report.pdf"); @@ -614,7 +614,7 @@ mod tests { #[test] fn test_params_deserializes_without_build_id() { let json = r#"{"artifact_name": "agent-report", "file_path": "out/report.pdf"}"#; - let params: UploadBuildArtifactParams = serde_json::from_str(json).unwrap(); + let params: UploadBuildAttachmentParams = serde_json::from_str(json).unwrap(); assert_eq!(params.build_id, None); assert_eq!(params.artifact_name, "agent-report"); assert_eq!(params.file_path, "out/report.pdf"); @@ -720,41 +720,41 @@ mod tests { #[test] fn test_result_serializes_correctly_with_build_id() { - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1234), "agent-report".to_string(), "out/report.pdf".to_string(), - "upload-build-artifact-agent-report-1234.bin".to_string(), + "upload-build-attachment-agent-report-1234.bin".to_string(), 42, DUMMY_HASH.to_string(), ); let json = serde_json::to_string(&result).unwrap(); - assert!(json.contains(r#""name":"upload-build-artifact""#)); + assert!(json.contains(r#""name":"upload-build-attachment""#)); assert!(json.contains(r#""build_id":1234"#)); assert!(json.contains(r#""artifact_name":"agent-report""#)); assert!(json.contains(r#""file_path":"out/report.pdf""#)); - assert!(json.contains(r#""staged_file":"upload-build-artifact-agent-report-1234.bin""#)); + assert!(json.contains(r#""staged_file":"upload-build-attachment-agent-report-1234.bin""#)); } #[test] fn test_result_serializes_correctly_without_build_id() { - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( None, "agent-report".to_string(), "out/report.pdf".to_string(), - "upload-build-artifact-agent-report-1234.bin".to_string(), + "upload-build-attachment-agent-report-1234.bin".to_string(), 42, DUMMY_HASH.to_string(), ); let json = serde_json::to_string(&result).unwrap(); - assert!(json.contains(r#""name":"upload-build-artifact""#)); + assert!(json.contains(r#""name":"upload-build-attachment""#)); assert!(json.contains(r#""build_id":null"#)); assert!(json.contains(r#""artifact_name":"agent-report""#)); } #[test] fn test_config_defaults() { - let config = UploadBuildArtifactConfig::default(); + let config = UploadBuildAttachmentConfig::default(); assert_eq!(config.max_file_size, 50 * 1024 * 1024); assert!(config.allowed_extensions.is_empty()); assert!(config.allowed_artifact_names.is_empty()); @@ -779,7 +779,7 @@ allowed-build-ids: name-prefix: "agent-" attachment-type: "agent-artifact" "#; - let config: UploadBuildArtifactConfig = serde_yaml::from_str(yaml).unwrap(); + let config: UploadBuildAttachmentConfig = serde_yaml::from_str(yaml).unwrap(); assert_eq!(config.max_file_size, 1_048_576); assert_eq!(config.allowed_extensions, vec![".png", ".pdf"]); assert_eq!(config.allowed_artifact_names, vec!["agent-*", "report"]); @@ -793,6 +793,7 @@ attachment-type: "agent-artifact" ado_org_url: None, ado_organization: None, ado_project: None, + ado_project_id: None, access_token: None, source_directory: working_directory.clone(), working_directory, @@ -822,10 +823,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_reads_staged_file_with_explicit_build_id() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-deadbeef.pdf"; + let staged = "upload-build-attachment-agent-report-deadbeef.pdf"; std::fs::write(dir.path().join(staged), b"%PDF-1.4 hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1234), "agent-report".to_string(), "out/report.pdf".to_string(), @@ -844,10 +845,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_resolves_current_build_when_build_id_omitted() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-feedf00d.pdf"; + let staged = "upload-build-attachment-agent-report-feedf00d.pdf"; std::fs::write(dir.path().join(staged), b"%PDF-1.4 hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( None, "agent-report".to_string(), "out/report.pdf".to_string(), @@ -867,10 +868,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_fails_when_build_id_omitted_and_not_in_env() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-cafef00d.pdf"; + let staged = "upload-build-attachment-agent-report-cafef00d.pdf"; std::fs::write(dir.path().join(staged), b"hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( None, "agent-report".to_string(), "out/report.pdf".to_string(), @@ -891,7 +892,7 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_rejects_missing_staged_file() { let dir = tempfile::tempdir().unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1234), "agent-report".to_string(), "out/report.pdf".to_string(), @@ -911,10 +912,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_rejects_disallowed_build_id() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-cafef00d.pdf"; + let staged = "upload-build-attachment-agent-report-cafef00d.pdf"; std::fs::write(dir.path().join(staged), b"hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(999), "agent-report".to_string(), "out/report.pdf".to_string(), @@ -924,7 +925,7 @@ attachment-type: "agent-artifact" ); let mut ctx = make_ctx(dir.path().to_path_buf(), true); ctx.tool_configs.insert( - "upload-build-artifact".to_string(), + "upload-build-attachment".to_string(), serde_json::json!({ "allowed-build-ids": [100, 200] }), ); let outcome = result.execute_impl(&ctx).await.unwrap(); @@ -939,10 +940,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_skips_build_id_allowlist_for_current_build() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-aabbccdd.pdf"; + let staged = "upload-build-attachment-agent-report-aabbccdd.pdf"; std::fs::write(dir.path().join(staged), b"hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( None, // targeting current build "agent-report".to_string(), "out/report.pdf".to_string(), @@ -953,7 +954,7 @@ attachment-type: "agent-artifact" let mut ctx = make_ctx(dir.path().to_path_buf(), true); ctx.build_id = Some(999); // current build not in allowed list ctx.tool_configs.insert( - "upload-build-artifact".to_string(), + "upload-build-attachment".to_string(), serde_json::json!({ "allowed-build-ids": [100, 200] }), ); let outcome = result.execute_impl(&ctx).await.unwrap(); @@ -967,10 +968,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_accepts_allowed_build_id() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-feedf00d.pdf"; + let staged = "upload-build-attachment-agent-report-feedf00d.pdf"; std::fs::write(dir.path().join(staged), b"hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(100), "agent-report".to_string(), "out/report.pdf".to_string(), @@ -980,7 +981,7 @@ attachment-type: "agent-artifact" ); let mut ctx = make_ctx(dir.path().to_path_buf(), true); ctx.tool_configs.insert( - "upload-build-artifact".to_string(), + "upload-build-attachment".to_string(), serde_json::json!({ "allowed-build-ids": [100, 200] }), ); let outcome = result.execute_impl(&ctx).await.unwrap(); @@ -990,10 +991,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_rejects_oversized_file() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-big-deadbeef.bin"; + let staged = "upload-build-attachment-agent-big-deadbeef.bin"; std::fs::write(dir.path().join(staged), vec![0u8; 1024]).unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1), "agent-big".to_string(), "out/big.bin".to_string(), @@ -1003,7 +1004,7 @@ attachment-type: "agent-artifact" ); let mut ctx = make_ctx(dir.path().to_path_buf(), true); ctx.tool_configs.insert( - "upload-build-artifact".to_string(), + "upload-build-attachment".to_string(), serde_json::json!({ "max-file-size": 100 }), ); let outcome = result.execute_impl(&ctx).await.unwrap(); @@ -1018,10 +1019,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_rejects_disallowed_extension() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-aabb1122.exe"; + let staged = "upload-build-attachment-agent-report-aabb1122.exe"; std::fs::write(dir.path().join(staged), b"MZ hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1), "agent-report".to_string(), "out/report.exe".to_string(), @@ -1031,7 +1032,7 @@ attachment-type: "agent-artifact" ); let mut ctx = make_ctx(dir.path().to_path_buf(), true); ctx.tool_configs.insert( - "upload-build-artifact".to_string(), + "upload-build-attachment".to_string(), serde_json::json!({ "allowed-extensions": [".pdf", ".png"] }), ); let outcome = result.execute_impl(&ctx).await.unwrap(); @@ -1046,10 +1047,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_rejects_disallowed_artifact_name() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-evil-report-ccdd3344.pdf"; + let staged = "upload-build-attachment-evil-report-ccdd3344.pdf"; std::fs::write(dir.path().join(staged), b"hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1), "evil-report".to_string(), "out/report.pdf".to_string(), @@ -1059,7 +1060,7 @@ attachment-type: "agent-artifact" ); let mut ctx = make_ctx(dir.path().to_path_buf(), true); ctx.tool_configs.insert( - "upload-build-artifact".to_string(), + "upload-build-attachment".to_string(), serde_json::json!({ "allowed-artifact-names": ["agent-*", "safe-report"] }), ); let outcome = result.execute_impl(&ctx).await.unwrap(); @@ -1074,10 +1075,10 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_applies_name_prefix() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-report-eeff5566.pdf"; + let staged = "upload-build-attachment-report-eeff5566.pdf"; std::fs::write(dir.path().join(staged), b"hello").unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1), "report".to_string(), "out/report.pdf".to_string(), @@ -1087,7 +1088,7 @@ attachment-type: "agent-artifact" ); let mut ctx = make_ctx(dir.path().to_path_buf(), true); ctx.tool_configs.insert( - "upload-build-artifact".to_string(), + "upload-build-attachment".to_string(), serde_json::json!({ "name-prefix": "agent-" }), ); let outcome = result.execute_impl(&ctx).await.unwrap(); @@ -1103,11 +1104,11 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_rejects_tampered_staged_file() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-tampered.pdf"; + let staged = "upload-build-attachment-agent-report-tampered.pdf"; // Write 100 bytes but record 50 in the result — simulates tampering. std::fs::write(dir.path().join(staged), vec![0u8; 100]).unwrap(); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1), "agent-report".to_string(), "out/report.pdf".to_string(), @@ -1128,14 +1129,14 @@ attachment-type: "agent-artifact" #[tokio::test] async fn test_executor_rejects_sha256_mismatch() { let dir = tempfile::tempdir().unwrap(); - let staged = "upload-build-artifact-agent-report-sha-mismatch.pdf"; + let staged = "upload-build-attachment-agent-report-sha-mismatch.pdf"; let content = b"real file content"; std::fs::write(dir.path().join(staged), content).unwrap(); // Record the hash of different content — same size but wrong hash. let wrong_hash = crate::hash::sha256_hex(b"wrong file content"); - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(1), "agent-report".to_string(), "out/report.pdf".to_string(), @@ -1161,7 +1162,7 @@ attachment-type: "agent-artifact" #[test] fn test_dry_run_summary_with_build_id() { - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( Some(42), "report".to_string(), "out/report.pdf".to_string(), @@ -1176,7 +1177,7 @@ attachment-type: "agent-artifact" #[test] fn test_dry_run_summary_without_build_id() { - let result = UploadBuildArtifactResult::new( + let result = UploadBuildAttachmentResult::new( None, "report".to_string(), "out/report.pdf".to_string(), diff --git a/src/safeoutputs/upload_pipeline_artifact.rs b/src/safeoutputs/upload_pipeline_artifact.rs new file mode 100644 index 00000000..916a7ff5 --- /dev/null +++ b/src/safeoutputs/upload_pipeline_artifact.rs @@ -0,0 +1,854 @@ +//! Upload pipeline artifact safe output tool. +//! +//! Lets an agent propose publishing a workspace file as an Azure DevOps build +//! artifact via the **build artifacts** REST API. Unlike build attachments +//! (which are invisible in the standard UI without a custom extension), +//! build artifacts published through this tool appear in the **Artifacts tab** +//! of the build summary page in Azure DevOps. +//! +//! The upload is a three-step REST flow: +//! +//! 1. **Create container** — `POST /_apis/resources/containers` returns a +//! `containerId`. +//! 2. **Upload file** — `PUT /_apis/resources/containers/{containerId}` sends +//! the file bytes with `itemPath` and `scope` query params. +//! 3. **Associate artifact** — `POST /_apis/build/builds/{buildId}/artifacts` +//! links the container to the build as a named artifact. +//! +//! The flow mirrors `upload-build-attachment`: +//! +//! * **Stage 1 (MCP, in the agent sandbox):** the MCP server validates the +//! agent-supplied params, resolves `file_path` against the agent's workspace, +//! rejects symlink escapes / directories, and **copies the file** into the +//! safe-outputs `output_directory` under a generated unique name. +//! * **Stage 3 (executor, outside the sandbox):** reads the staged file from +//! `ctx.working_directory.join(staged_file)`, applies operator-supplied +//! limits (`max-file-size`, `allowed-extensions`, `allowed-artifact-names`, +//! `allowed-build-ids`, `name-prefix`), resolves the target build ID, and +//! executes the three-step upload flow. + +use ado_aw_derive::SanitizeConfig; +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::SanitizeContent; +use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::tool_result; +use crate::validate::{is_safe_path_segment, is_valid_artifact_name}; +use anyhow::{Context, ensure}; + +/// Parameters for publishing a workspace file as an ADO pipeline artifact. +#[derive(Deserialize, JsonSchema)] +pub struct UploadPipelineArtifactParams { + /// The build ID to publish the artifact to. **Omit to target the current + /// pipeline run** — the executor resolves the build ID from the + /// `BUILD_BUILDID` environment variable automatically. When provided, + /// must be a positive integer. + pub build_id: Option, + + /// The artifact name shown in the Artifacts tab. ADO requires a non-empty + /// name made of alphanumerics, `-`, `_`, or `.`. Must be 1-100 characters + /// and must not start with `.`. + pub artifact_name: String, + + /// Path to the file in the workspace to publish. Must be a relative path + /// with no directory traversal, no absolute prefix, and no `.git` + /// segments. + pub file_path: String, +} + +impl Validate for UploadPipelineArtifactParams { + fn validate(&self) -> anyhow::Result<()> { + if let Some(id) = self.build_id { + ensure!(id > 0, "build_id must be positive when specified"); + } + + ensure!( + self.artifact_name.len() <= 100, + "artifact_name must be at most 100 characters" + ); + ensure!( + !self.artifact_name.starts_with('.'), + "artifact_name must not start with '.'" + ); + ensure!( + is_valid_artifact_name(&self.artifact_name), + "artifact_name must be non-empty and contain only alphanumeric characters, '-', '_' or '.'" + ); + + ensure!(!self.file_path.is_empty(), "file_path must not be empty"); + ensure!( + !self.file_path.contains('\0'), + "file_path must not contain null bytes" + ); + ensure!( + !self.file_path.contains(':'), + "file_path must not contain ':'" + ); + for component in self.file_path.split(['/', '\\']) { + ensure!( + is_safe_path_segment(component), + "file_path component '{}' is not a safe path segment (no empty, '..', or leading '.' allowed)", + component + ); + } + Ok(()) + } +} + +/// Internal params struct for the `tool_result!` macro's `TryFrom` plumbing. +#[derive(Deserialize, JsonSchema)] +struct UploadPipelineArtifactResultFields { + build_id: Option, + artifact_name: String, + file_path: String, + staged_file: String, + file_size: u64, + staged_sha256: String, +} + +impl Validate for UploadPipelineArtifactResultFields {} + +tool_result! { + name = "upload-pipeline-artifact", + write = true, + params = UploadPipelineArtifactResultFields, + default_max = 3, + /// Result of publishing a workspace file as an ADO pipeline artifact. + pub struct UploadPipelineArtifactResult { + /// Build ID the artifact should be published to. `None` means "current + /// build" — resolved at execution time from `BUILD_BUILDID`. + build_id: Option, + /// Artifact name as proposed by the agent (pre-prefix). + artifact_name: String, + /// Original file path proposed by the agent. + file_path: String, + /// Filename of the staged copy inside the safe-outputs directory. + staged_file: String, + /// Size in bytes of the staged file at copy time. + file_size: u64, + /// SHA-256 hex digest of the staged file recorded at copy time. + staged_sha256: String, + } +} + +impl SanitizeContent for UploadPipelineArtifactResult { + fn sanitize_content_fields(&mut self) { + // All textual fields are strictly validated to safe charsets. + } +} + +impl UploadPipelineArtifactResult { + /// Construct a result after the agent's file has been staged. + pub fn new( + build_id: Option, + artifact_name: String, + file_path: String, + staged_file: String, + file_size: u64, + staged_sha256: String, + ) -> Self { + Self { + name: ::NAME.to_string(), + build_id, + artifact_name, + file_path, + staged_file, + file_size, + staged_sha256, + } + } +} + +/// Default maximum file size (50 MB). +pub const PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE: u64 = 50 * 1024 * 1024; + +/// Configuration for the upload-pipeline-artifact tool (specified in front +/// matter). +#[derive(Debug, Clone, SanitizeConfig, Serialize, Deserialize)] +pub struct UploadPipelineArtifactConfig { + /// Maximum file size in bytes (default: 50 MB). + #[serde(default = "default_pipeline_max_file_size", rename = "max-file-size")] + pub max_file_size: u64, + + /// Allowed file extensions (e.g., `[".png", ".pdf"]`). Empty means all + /// extensions are allowed. + #[serde(default, rename = "allowed-extensions")] + pub allowed_extensions: Vec, + + /// Restrict which artifact names may be published. Empty means any name + /// (subject to charset rules) is allowed. Entries ending with `*` match + /// by prefix; otherwise the comparison is exact. + #[serde(default, rename = "allowed-artifact-names")] + pub allowed_artifact_names: Vec, + + /// Restrict which build IDs the agent may publish to. Empty means any + /// build ID accessible to the executor's token is allowed. This check + /// is skipped when `build_id` is omitted (targeting the current build). + #[serde(default, rename = "allowed-build-ids")] + pub allowed_build_ids: Vec, + + /// Prefix prepended to the agent-supplied artifact name before publishing. + #[serde(default, rename = "name-prefix")] + pub name_prefix: Option, +} + +fn default_pipeline_max_file_size() -> u64 { + PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE +} + +impl Default for UploadPipelineArtifactConfig { + fn default() -> Self { + Self { + max_file_size: PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE, + allowed_extensions: Vec::new(), + allowed_artifact_names: Vec::new(), + allowed_build_ids: Vec::new(), + name_prefix: None, + } + } +} + +#[async_trait::async_trait] +impl Executor for UploadPipelineArtifactResult { + fn dry_run_summary(&self) -> String { + match self.build_id { + Some(id) => format!( + "publish '{}' as pipeline artifact '{}' on build #{}", + self.file_path, self.artifact_name, id + ), + None => format!( + "publish '{}' as pipeline artifact '{}' on current build", + self.file_path, self.artifact_name + ), + } + } + + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + let effective_build_id: i64 = match self.build_id { + Some(id) => id, + None => { + let current = ctx.build_id.context( + "build_id was not specified and BUILD_BUILDID is not set — \ + cannot determine which build to publish the artifact to", + )?; + i64::try_from(current).context("BUILD_BUILDID value overflows i64")? + } + }; + + info!( + "Publishing '{}' as pipeline artifact '{}' on build #{}{}", + self.file_path, + self.artifact_name, + effective_build_id, + if self.build_id.is_none() { " (current build)" } else { "" } + ); + + let config: UploadPipelineArtifactConfig = ctx.get_tool_config("upload-pipeline-artifact"); + + // ── Build-ID allow-list ────────────────────────────────────────── + if self.build_id.is_some() + && !config.allowed_build_ids.is_empty() + && !config.allowed_build_ids.contains(&effective_build_id) + { + return Ok(ExecutionResult::failure(format!( + "Build ID {} is not in the allowed-build-ids list", + effective_build_id + ))); + } + + // ── Name-prefix ───────────────────────────────────────────────── + if let Some(prefix) = &config.name_prefix { + if prefix.len() > 50 { + return Ok(ExecutionResult::failure(format!( + "name-prefix '{}...' is too long ({} chars, max 50)", + prefix.chars().take(20).collect::(), + prefix.len() + ))); + } + } + let final_name = match &config.name_prefix { + Some(prefix) => format!("{}{}", prefix, self.artifact_name), + None => self.artifact_name.clone(), + }; + if final_name.starts_with('.') || final_name.len() > 100 || !is_valid_artifact_name(&final_name) { + return Ok(ExecutionResult::failure(format!( + "Resolved artifact name '{}' is not a valid Azure DevOps artifact name", + final_name + ))); + } + + // ── Artifact-name allow-list ───────────────────────────────────── + if !config.allowed_artifact_names.is_empty() { + let allowed = config.allowed_artifact_names.iter().any(|pattern| { + if let Some(prefix) = pattern.strip_suffix('*') { + final_name.starts_with(prefix) + } else { + *pattern == final_name + } + }); + if !allowed { + return Ok(ExecutionResult::failure(format!( + "Artifact name '{}' is not in the allowed list", + final_name + ))); + } + } + + // ── Extension allow-list ───────────────────────────────────────── + if !config.allowed_extensions.is_empty() { + let file_ext = std::path::Path::new(&self.file_path) + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""); + let has_valid_ext = config.allowed_extensions.iter().any(|ext| { + ext.trim_start_matches('.').eq_ignore_ascii_case(file_ext) + }); + if !has_valid_ext { + return Ok(ExecutionResult::failure(format!( + "File '{}' has an extension not in the allowed list: {:?}", + self.file_path, config.allowed_extensions + ))); + } + } + + // ── Staged file resolution ─────────────────────────────────────── + let staged_path = ctx.working_directory.join(&self.staged_file); + let canonical = staged_path.canonicalize().context( + "Failed to canonicalize staged file path — file may be missing or contains broken symlinks", + )?; + let canonical_base = ctx + .working_directory + .canonicalize() + .context("Failed to canonicalize working directory")?; + if !canonical.starts_with(&canonical_base) { + return Ok(ExecutionResult::failure(format!( + "Staged file '{}' resolves outside the safe-outputs directory", + self.staged_file + ))); + } + let metadata = std::fs::metadata(&canonical).context("Failed to read file metadata")?; + if metadata.is_dir() { + return Ok(ExecutionResult::failure(format!( + "Staged path '{}' is a directory; upload-pipeline-artifact only supports single files", + self.staged_file + ))); + } + let file_size = metadata.len(); + + // ── Integrity checks ───────────────────────────────────────────── + if file_size != self.file_size { + return Ok(ExecutionResult::failure(format!( + "Staged file size ({} bytes) differs from size recorded at Stage 1 ({} bytes) — \ + the file may have been modified between stages", + file_size, self.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 + ))); + } + + if ctx.dry_run { + return Ok(ExecutionResult::success(format!( + "[dry-run] would publish '{}' ({} bytes) as pipeline artifact '{}' on build #{}{}", + self.file_path, + file_size, + final_name, + effective_build_id, + if self.build_id.is_none() { " (current build)" } else { "" } + ))); + } + + let file_bytes = tokio::fs::read(&canonical).await.context("Failed to read file contents")?; + + let live_hash = crate::hash::sha256_hex(&file_bytes); + if live_hash != self.staged_sha256 { + return Ok(ExecutionResult::failure(format!( + "Staged file SHA-256 mismatch: expected {} (recorded at Stage 1), got {} — \ + the file may have been tampered with between stages", + self.staged_sha256, live_hash + ))); + } + + // ── Resolve ADO 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 project_id = ctx + .ado_project_id + .as_ref() + .context("SYSTEM_TEAMPROJECTID not set — required for pipeline artifact upload")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + + let client = reqwest::Client::new(); + + // Derive a filename from the original file path for use as the + // itemPath inside the container (e.g. "report.pdf" from "out/report.pdf"). + let filename = std::path::Path::new(&self.file_path) + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(&self.staged_file); + + // ── Step 1: Create container ───────────────────────────────────── + let container_url = format!( + "{}/_apis/resources/containers?api-version=7.1-preview.4", + org_url.trim_end_matches('/'), + ); + debug!("Creating container for artifact '{}': {}", final_name, container_url); + + let container_body = serde_json::json!({ + "name": final_name, + }); + let container_resp = client + .post(&container_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&container_body) + .send() + .await + .context("Failed to create artifact container")?; + + if !container_resp.status().is_success() { + let status = container_resp.status(); + let error_body = container_resp + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to create artifact container (HTTP {}): {}", + status, error_body + ))); + } + let container_json: serde_json::Value = container_resp + .json() + .await + .context("Failed to parse container creation response")?; + let container_id = container_json + .get("containerId") + .and_then(|v| v.as_u64()) + .context("Container creation response missing 'containerId'")?; + debug!("Container created: id={}", container_id); + + // ── Step 2: Upload file to container ───────────────────────────── + let upload_url = format!( + "{}/_apis/resources/containers/{}?itemPath={}/{}&scope={}&api-version=7.1-preview.4", + org_url.trim_end_matches('/'), + container_id, + utf8_percent_encode(&final_name, PATH_SEGMENT), + utf8_percent_encode(filename, PATH_SEGMENT), + utf8_percent_encode(project_id, PATH_SEGMENT), + ); + debug!("Uploading {} bytes to container: {}", file_size, upload_url); + + let upload_resp = client + .put(&upload_url) + .header("Content-Type", "application/octet-stream") + .basic_auth("", Some(token)) + .body(file_bytes) + .send() + .await + .context("Failed to upload file to artifact container")?; + + if !upload_resp.status().is_success() { + let status = upload_resp.status(); + let error_body = upload_resp + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to upload file to container (HTTP {}): {}", + status, error_body + ))); + } + debug!("File uploaded to container {}", container_id); + + // ── Step 3: Associate artifact with build ──────────────────────── + let artifact_url = format!( + "{}/{}/_apis/build/builds/{}/artifacts?api-version=7.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + effective_build_id, + ); + debug!("Associating artifact '{}' with build #{}: {}", final_name, effective_build_id, artifact_url); + + let artifact_body = serde_json::json!({ + "name": final_name, + "resource": { + "data": format!("#/{}/{}", container_id, final_name), + "type": "container", + }, + }); + let artifact_resp = client + .post(&artifact_url) + .header("Content-Type", "application/json") + .basic_auth("", Some(token)) + .json(&artifact_body) + .send() + .await + .context("Failed to associate artifact with build")?; + + if artifact_resp.status().is_success() { + let resp_body: serde_json::Value = artifact_resp.json().await.unwrap_or_else(|e| { + warn!( + "Pipeline artifact created for build #{} but the response JSON could not be parsed: {} — proceeding without download URL", + effective_build_id, e + ); + serde_json::Value::Null + }); + let download_url = resp_body + .get("resource") + .and_then(|r| r.get("downloadUrl")) + .and_then(|v| v.as_str()) + .map(|s| s.to_string()); + info!( + "Published '{}' as pipeline artifact '{}' on build #{}", + self.file_path, final_name, effective_build_id + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Published '{}' as pipeline artifact '{}' on build #{}", + self.file_path, effective_build_id, final_name + ), + serde_json::json!({ + "build_id": effective_build_id, + "artifact_name": final_name, + "file_path": self.file_path, + "size_bytes": file_size, + "download_url": download_url, + "project": project, + }), + )) + } else { + let status = artifact_resp.status(); + let error_body = artifact_resp + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + Ok(ExecutionResult::failure(format!( + "Failed to associate artifact with build #{} (HTTP {}): {}", + effective_build_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::safeoutputs::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(UploadPipelineArtifactResult::NAME, "upload-pipeline-artifact"); + } + + fn make_params( + build_id: Option, + artifact_name: &str, + file_path: &str, + ) -> UploadPipelineArtifactParams { + UploadPipelineArtifactParams { + build_id, + artifact_name: artifact_name.to_string(), + file_path: file_path.to_string(), + } + } + + const DUMMY_HASH: &str = "0000000000000000000000000000000000000000000000000000000000000000"; + + #[test] + fn test_params_validate_accepts_valid() { + assert!(make_params(Some(1), "agent-report", "out/report.pdf") + .validate() + .is_ok()); + assert!(make_params(None, "agent-report", "out/report.pdf") + .validate() + .is_ok()); + } + + #[test] + fn test_validation_rejects_zero_build_id() { + assert!(make_params(Some(0), "report", "out/report.pdf") + .validate() + .is_err()); + } + + #[test] + fn test_validation_rejects_negative_build_id() { + assert!(make_params(Some(-1), "report", "out/report.pdf") + .validate() + .is_err()); + } + + #[test] + fn test_validation_rejects_empty_artifact_name() { + assert!(make_params(None, "", "out/report.pdf").validate().is_err()); + } + + #[test] + fn test_validation_rejects_artifact_name_with_spaces() { + assert!(make_params(None, "my report", "out/report.pdf") + .validate() + .is_err()); + } + + #[test] + fn test_validation_rejects_leading_dot_artifact_name() { + assert!(make_params(None, ".hidden", "out/report.pdf") + .validate() + .is_err()); + } + + #[test] + fn test_validation_rejects_long_artifact_name() { + let long_name = "a".repeat(101); + assert!(make_params(None, &long_name, "out/report.pdf") + .validate() + .is_err()); + } + + #[test] + fn test_validation_rejects_empty_file_path() { + assert!(make_params(None, "report", "").validate().is_err()); + } + + #[test] + fn test_validation_rejects_traversal_in_file_path() { + assert!(make_params(None, "report", "../etc/passwd") + .validate() + .is_err()); + } + + #[test] + fn test_validation_rejects_null_bytes_in_file_path() { + assert!(make_params(None, "report", "out/report\0.pdf") + .validate() + .is_err()); + } + + #[test] + fn test_validation_rejects_colon_in_file_path() { + assert!(make_params(None, "report", "C:\\out\\report.pdf") + .validate() + .is_err()); + } + + #[test] + fn test_dry_run_summary() { + let result = UploadPipelineArtifactResult::new( + None, + "agent-report".to_string(), + "out/report.pdf".to_string(), + "staged-abc123.pdf".to_string(), + 1024, + DUMMY_HASH.to_string(), + ); + assert!(result.dry_run_summary().contains("agent-report")); + assert!(result.dry_run_summary().contains("current build")); + + let result_with_id = UploadPipelineArtifactResult::new( + Some(42), + "agent-report".to_string(), + "out/report.pdf".to_string(), + "staged-abc123.pdf".to_string(), + 1024, + DUMMY_HASH.to_string(), + ); + assert!(result_with_id.dry_run_summary().contains("build #42")); + } + + #[test] + fn test_config_defaults() { + let config = UploadPipelineArtifactConfig::default(); + assert_eq!(config.max_file_size, PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE); + assert!(config.allowed_extensions.is_empty()); + assert!(config.allowed_artifact_names.is_empty()); + assert!(config.allowed_build_ids.is_empty()); + assert!(config.name_prefix.is_none()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +max-file-size: 10485760 +allowed-extensions: + - .pdf + - .log +allowed-artifact-names: + - agent-* +name-prefix: "ci-" +"#; + let config: UploadPipelineArtifactConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.max_file_size, 10485760); + assert_eq!(config.allowed_extensions, vec![".pdf", ".log"]); + assert_eq!(config.allowed_artifact_names, vec!["agent-*"]); + assert_eq!(config.name_prefix.as_deref(), Some("ci-")); + } + + #[tokio::test] + async fn test_dry_run_succeeds() { + let dir = tempfile::tempdir().unwrap(); + let staged = dir.path().join("staged-file.pdf"); + let content = b"test content"; + std::fs::write(&staged, content).unwrap(); + let hash = crate::hash::sha256_hex(content); + + let result = UploadPipelineArtifactResult::new( + None, + "agent-report".to_string(), + "out/report.pdf".to_string(), + "staged-file.pdf".to_string(), + content.len() as u64, + hash, + ); + + let ctx = ExecutionContext { + working_directory: dir.path().to_path_buf(), + build_id: Some(123), + dry_run: true, + ..Default::default() + }; + + let exec_result = result.execute_impl(&ctx).await.unwrap(); + assert!(exec_result.success); + assert!(exec_result.message.contains("[dry-run]")); + assert!(exec_result.message.contains("agent-report")); + } + + #[tokio::test] + async fn test_rejects_file_size_mismatch() { + let dir = tempfile::tempdir().unwrap(); + let staged = dir.path().join("staged-file.pdf"); + std::fs::write(&staged, b"test content").unwrap(); + + let result = UploadPipelineArtifactResult::new( + None, + "report".to_string(), + "out/report.pdf".to_string(), + "staged-file.pdf".to_string(), + 9999, // wrong size + DUMMY_HASH.to_string(), + ); + + let ctx = ExecutionContext { + working_directory: dir.path().to_path_buf(), + build_id: Some(123), + dry_run: false, + ..Default::default() + }; + + let exec_result = result.execute_impl(&ctx).await.unwrap(); + assert!(!exec_result.success); + assert!(exec_result.message.contains("differs from size")); + } + + #[tokio::test] + async fn test_rejects_exceeding_max_file_size() { + let dir = tempfile::tempdir().unwrap(); + let staged = dir.path().join("staged-file.pdf"); + let content = b"x"; + std::fs::write(&staged, content).unwrap(); + + let result = UploadPipelineArtifactResult::new( + None, + "report".to_string(), + "out/report.pdf".to_string(), + "staged-file.pdf".to_string(), + 1, + crate::hash::sha256_hex(content), + ); + + let mut ctx = ExecutionContext { + working_directory: dir.path().to_path_buf(), + build_id: Some(123), + dry_run: false, + ..Default::default() + }; + // Set max-file-size to 0 to trigger rejection + ctx.tool_configs.insert( + "upload-pipeline-artifact".to_string(), + serde_json::json!({"max-file-size": 0}), + ); + + let exec_result = result.execute_impl(&ctx).await.unwrap(); + assert!(!exec_result.success); + assert!(exec_result.message.contains("exceeds maximum")); + } + + #[tokio::test] + async fn test_rejects_disallowed_build_id() { + let dir = tempfile::tempdir().unwrap(); + let staged = dir.path().join("staged-file.pdf"); + std::fs::write(&staged, b"test").unwrap(); + + let result = UploadPipelineArtifactResult::new( + Some(999), + "report".to_string(), + "out/report.pdf".to_string(), + "staged-file.pdf".to_string(), + 4, + DUMMY_HASH.to_string(), + ); + + let mut ctx = ExecutionContext { + working_directory: dir.path().to_path_buf(), + dry_run: false, + ..Default::default() + }; + ctx.tool_configs.insert( + "upload-pipeline-artifact".to_string(), + serde_json::json!({"allowed-build-ids": [123, 456]}), + ); + + let exec_result = result.execute_impl(&ctx).await.unwrap(); + assert!(!exec_result.success); + assert!(exec_result.message.contains("not in the allowed-build-ids")); + } + + #[tokio::test] + async fn test_rejects_disallowed_extension() { + let dir = tempfile::tempdir().unwrap(); + let staged = dir.path().join("staged-file.exe"); + let content = b"test"; + std::fs::write(&staged, content).unwrap(); + + let result = UploadPipelineArtifactResult::new( + None, + "report".to_string(), + "out/report.exe".to_string(), + "staged-file.exe".to_string(), + content.len() as u64, + crate::hash::sha256_hex(content), + ); + + let mut ctx = ExecutionContext { + working_directory: dir.path().to_path_buf(), + build_id: Some(123), + dry_run: false, + ..Default::default() + }; + ctx.tool_configs.insert( + "upload-pipeline-artifact".to_string(), + serde_json::json!({"allowed-extensions": [".pdf", ".log"]}), + ); + + let exec_result = result.execute_impl(&ctx).await.unwrap(); + assert!(!exec_result.success); + assert!(exec_result.message.contains("not in the allowed list")); + } +} From 8f95924334f7fd60329b0956c4bd496d485c6490 Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 5 May 2026 13:41:33 +0100 Subject: [PATCH 2/4] refactor(safe-outputs): remove backward-compat alias, improve tool descriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove canonical_safe_output_name() alias layer — no backward compatibility for the old upload-build-artifact name - Fix stale upload-build-artifact references in mcp.rs staged filenames/comments - Improve MCP tool descriptions: clearly state that build attachments are NOT visible in ADO UI and recommend upload-pipeline-artifact for user-visible files - Add upload-build-attachment and upload-pipeline-artifact to AGENTS.md architecture tree and README.md safe-outputs table - Remove deprecation notice from docs/safe-outputs.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 2 ++ README.md | 2 ++ docs/safe-outputs.md | 3 --- src/compile/common.rs | 24 +++++++----------------- src/execute.rs | 10 ++++------ src/main.rs | 21 +-------------------- src/mcp.rs | 33 +++++++++++++++++---------------- src/safeoutputs/mod.rs | 11 ----------- 8 files changed, 33 insertions(+), 73 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 84a93381..309ba85b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -96,6 +96,8 @@ Every compiled pipeline runs as three sequential jobs: │ │ ├── update_pr.rs │ │ ├── update_wiki_page.rs │ │ ├── update_work_item.rs +│ │ ├── upload_build_attachment.rs +│ │ ├── upload_pipeline_artifact.rs │ │ └── upload_workitem_attachment.rs │ ├── runtimes/ # Runtime environment implementations (one dir per runtime) │ │ ├── mod.rs # Module entry point diff --git a/README.md b/README.md index f1fb9d85..6ac9419c 100644 --- a/README.md +++ b/README.md @@ -389,6 +389,8 @@ actions, and the executor processes them after threat analysis. | `create-git-tag` | Creates a git tag on a repository ref | | `create-branch` | Creates a new branch from an existing ref | | `add-build-tag` | Adds a tag to an ADO build | +| `upload-build-attachment` | Attaches a file to a build (accessible via REST API or custom extension only) | +| `upload-pipeline-artifact` | Publishes a file as a pipeline artifact visible in the ADO Artifacts tab | | `upload-workitem-attachment` | Uploads a workspace file as an attachment to a work item | | `report-incomplete` | Reports that a task could not be completed | | `noop` | Reports no action was needed | diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 70a9f9c5..18274054 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -420,9 +420,6 @@ safe-outputs: ### upload-build-attachment -> **Renamed from `upload-build-artifact`** — the old name is accepted as a -> deprecated alias for backward compatibility. - Attaches a workspace file to an Azure DevOps build as a **build attachment** via the ADO build attachments REST API (`PUT /_apis/build/builds/{buildId}/attachments/{type}/{name}`). diff --git a/src/compile/common.rs b/src/compile/common.rs index 77598429..5e018087 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -929,7 +929,7 @@ pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> Stri /// to prevent shell injection when the args are embedded in bash commands. /// Unrecognized tool names emit a compile-time warning and are skipped. pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { - use crate::safeoutputs::{ALL_KNOWN_SAFE_OUTPUTS, ALWAYS_ON_TOOLS, NON_MCP_SAFE_OUTPUT_KEYS, canonical_safe_output_name}; + use crate::safeoutputs::{ALL_KNOWN_SAFE_OUTPUTS, ALWAYS_ON_TOOLS, NON_MCP_SAFE_OUTPUT_KEYS}; use std::collections::HashSet; if front_matter.safe_outputs.is_empty() { @@ -949,15 +949,7 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { ); continue; } - // Canonicalize deprecated aliases (e.g. upload-build-artifact → upload-build-attachment) - let canonical = canonical_safe_output_name(key); - if canonical != key { - eprintln!( - "Warning: safe-output key '{}' is deprecated — use '{}' instead", - key, canonical - ); - } - if NON_MCP_SAFE_OUTPUT_KEYS.contains(&canonical) { + if NON_MCP_SAFE_OUTPUT_KEYS.contains(&key.as_str()) { continue; } if key == "memory" { @@ -968,7 +960,7 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { ); continue; } - if !ALL_KNOWN_SAFE_OUTPUTS.contains(&canonical) { + if !ALL_KNOWN_SAFE_OUTPUTS.contains(&key.as_str()) { eprintln!( "Warning: unrecognized safe-output tool '{}' — skipping (no registered tool matches this name)", key @@ -976,8 +968,8 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { continue; } effective_mcp_tool_count += 1; - if seen.insert(canonical.to_string()) { - tools.push(canonical.to_string()); + if seen.insert(key.clone()) { + tools.push(key.clone()); } } @@ -1011,7 +1003,7 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { /// Validate that write-requiring safe-outputs have a write service connection configured. pub fn validate_write_permissions(front_matter: &FrontMatter) -> Result<()> { - use crate::safeoutputs::{WRITE_REQUIRING_SAFE_OUTPUTS, canonical_safe_output_name}; + use crate::safeoutputs::WRITE_REQUIRING_SAFE_OUTPUTS; let has_write_sc = front_matter .permissions @@ -1024,9 +1016,7 @@ pub fn validate_write_permissions(front_matter: &FrontMatter) -> Result<()> { let missing: Vec<&str> = WRITE_REQUIRING_SAFE_OUTPUTS .iter() - .filter(|name| { - front_matter.safe_outputs.keys().any(|k| canonical_safe_output_name(k) == **name) - }) + .filter(|name| front_matter.safe_outputs.contains_key(**name)) .copied() .collect(); diff --git a/src/execute.rs b/src/execute.rs index 8b9d65ae..02223b0d 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -19,7 +19,7 @@ use crate::safeoutputs::{ MissingToolResult, NoopResult, QueueBuildResult, ReplyToPrCommentResult, ReportIncompleteResult, ResolvePrThreadResult, SubmitPrReviewResult, ToolResult, UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadBuildAttachmentResult, - UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, canonical_safe_output_name, + UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, }; // Re-export memory types for use by main.rs @@ -192,8 +192,7 @@ fn enforce_budget( total: usize, i: usize, ) -> Option { - let raw_name = entry.get("name").and_then(|n| n.as_str())?; - let tool_name = canonical_safe_output_name(raw_name); + let tool_name = entry.get("name").and_then(|n| n.as_str())?; let (executed, max) = budgets.get_mut(tool_name)?; let context_id = extract_entry_context(entry); if let Some(result) = check_budget(total, i, tool_name, &context_id, *executed, *max) { @@ -251,13 +250,12 @@ pub async fn execute_safe_output( ctx: &ExecutionContext, ) -> Result<(String, ExecutionResult)> { // First check the name field to dispatch correctly - let raw_tool_name = entry + let tool_name = entry .get("name") .and_then(|n| n.as_str()) .ok_or_else(|| anyhow::anyhow!("Safe output missing 'name' field"))?; - let tool_name = canonical_safe_output_name(raw_tool_name); - debug!("Dispatching tool: {} (raw: {})", tool_name, raw_tool_name); + debug!("Dispatching tool: {}", tool_name); // Dispatch based on tool name. All registered tools go through `dispatch_tool`, // which handles deserialization and sanitized execution uniformly. diff --git a/src/main.rs b/src/main.rs index 6afe680c..43ba366d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,7 +20,6 @@ pub mod validate; use anyhow::{Context, Result}; use clap::{Parser, Subcommand}; -use std::collections::HashMap; use std::path::PathBuf; #[derive(Subcommand, Debug)] @@ -249,25 +248,7 @@ async fn build_execution_context( ctx.ado_org_url = ado_org_url; ctx.ado_project = ado_project; ctx.working_directory = safe_output_dir.clone(); - // Normalize deprecated safe-output config keys to their canonical names. - // This ensures old front matter (e.g. `upload-build-artifact:`) maps to - // the new tool name for config lookup at execution time. - ctx.tool_configs = { - use crate::safeoutputs::canonical_safe_output_name; - let mut normalized = HashMap::new(); - for (key, value) in &front_matter.safe_outputs { - let canonical = canonical_safe_output_name(key).to_string(); - if canonical != *key && normalized.contains_key(&canonical) { - eprintln!( - "Warning: both '{}' and '{}' are configured in safe-outputs — using '{}'", - key, canonical, canonical - ); - continue; - } - normalized.insert(canonical, value.clone()); - } - normalized - }; + ctx.tool_configs = front_matter.safe_outputs.clone(); ctx.allowed_repositories = allowed_repositories; ctx.dry_run = dry_run; diff --git a/src/mcp.rs b/src/mcp.rs index 5a056abe..70075a4b 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -992,13 +992,14 @@ uploaded and linked during safe output processing. File size and type restrictio #[tool( name = "upload-build-attachment", - description = "Attach a workspace file to an Azure DevOps build as a build attachment. \ -Omit `build_id` to target the current pipeline run (the executor resolves it from the \ -BUILD_BUILDID environment variable automatically). When `build_id` is provided, the file is \ + description = "Attach a workspace file to an Azure DevOps build as a build attachment via \ +the ADO build attachments REST API. Build attachments are NOT visible in the standard ADO UI — \ +they are only accessible via the REST API or a custom Azure DevOps extension. For files that \ +should appear in the Artifacts tab, use upload-pipeline-artifact instead. \ +Omit `build_id` to target the current pipeline run. When `build_id` is provided, the file is \ attached to that specific build — useful for posthumously decorating a finished build with a \ -generated report, screenshot, or log bundle. The file will be staged now and uploaded via the \ -ADO build attachments REST API during safe output processing. File size, extension, \ -artifact-name and build-id restrictions may apply per the workflow's safe-outputs config." +generated report or log bundle. File size, extension, artifact-name and build-id restrictions \ +may apply per the workflow's safe-outputs config." )] async fn upload_build_attachment( &self, @@ -1039,13 +1040,13 @@ artifact-name and build-id restrictions may apply per the workflow's safe-output ))); } - // Reject directories — upload-build-artifact is single-file only. + // Reject directories — upload-build-attachment is single-file only. let metadata = tokio::fs::metadata(&canonical).await.map_err(|e| { anyhow_to_mcp_error(anyhow::anyhow!("Failed to stat '{}': {}", params.0.file_path, e)) })?; if metadata.is_dir() { return Err(anyhow_to_mcp_error(anyhow::anyhow!( - "File '{}' is a directory; upload-build-artifact only supports single files", + "File '{}' is a directory; upload-build-attachment only supports single files", params.0.file_path ))); } @@ -1083,13 +1084,13 @@ artifact-name and build-id restrictions may apply per the workflow's safe-output // max length (~140 chars) is well within filesystem limits. let staged_filename = if extension.is_empty() { format!( - "upload-build-artifact-{}-{}", + "upload-build-attachment-{}-{}", params.0.artifact_name, generate_short_id() ) } else { format!( - "upload-build-artifact-{}-{}.{}", + "upload-build-attachment-{}-{}.{}", params.0.artifact_name, generate_short_id(), extension @@ -1140,12 +1141,12 @@ artifact-name and build-id restrictions may apply per the workflow's safe-output #[tool( name = "upload-pipeline-artifact", - description = "Publish a workspace file as an Azure DevOps pipeline artifact that appears in \ -the Artifacts tab of the build summary. Omit `build_id` to target the current pipeline run \ -(the executor resolves it from BUILD_BUILDID automatically). When `build_id` is provided, the \ -file is published to that specific build. The file will be staged now and uploaded via the ADO \ -build artifacts REST API during safe output processing. File size, extension, artifact-name and \ -build-id restrictions may apply per the workflow's safe-outputs config." + description = "Publish a workspace file as an Azure DevOps pipeline artifact that appears \ +in the Artifacts tab of the build summary page — visible to all users viewing the build. Use \ +this tool when you want users to be able to find and download the file from the ADO UI. \ +Omit `build_id` to target the current pipeline run. When `build_id` is provided, the artifact \ +is published to that specific build. File size, extension, artifact-name and build-id \ +restrictions may apply per the workflow's safe-outputs config." )] async fn upload_pipeline_artifact( &self, diff --git a/src/safeoutputs/mod.rs b/src/safeoutputs/mod.rs index 45b698be..fccfacd3 100644 --- a/src/safeoutputs/mod.rs +++ b/src/safeoutputs/mod.rs @@ -55,17 +55,6 @@ pub const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = tool_names![ /// filtered out (the router has no route for them). pub const NON_MCP_SAFE_OUTPUT_KEYS: &[&str] = &[]; -/// Canonicalize a safe-output tool name, resolving deprecated aliases to their -/// current names. Callers should apply this before budget enforcement, -/// config lookup, and dispatch so that old NDJSON entries / front-matter keys -/// consume the same budgets and configs as new ones. -pub fn canonical_safe_output_name(name: &str) -> &str { - match name { - "upload-build-artifact" => "upload-build-attachment", - other => other, - } -} - /// All recognised safe-output keys accepted in front matter `safe-outputs:`. /// This is the union of write-requiring tool types and diagnostic tool types. /// From 172bc26afe4b6363bb8f0d279f02d6428e7f0dc8 Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 5 May 2026 14:39:20 +0100 Subject: [PATCH 3/4] fix(safe-outputs): fix swapped format args and TOCTOU file_size in MCP handlers - Fix swapped effective_build_id/final_name in upload-pipeline-artifact success message (the info!() log was correct but the returned ExecutionResult message had them reversed) - Use source_bytes.len() instead of metadata.len() for recorded file_size in both upload-build-attachment and upload-pipeline-artifact MCP handlers, closing a TOCTOU window where the source file could change between stat and read Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/mcp.rs | 20 ++++++++++++++------ src/safeoutputs/upload_pipeline_artifact.rs | 2 +- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/mcp.rs b/src/mcp.rs index 70075a4b..a72ca3d6 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -1050,16 +1050,16 @@ may apply per the workflow's safe-outputs config." params.0.file_path ))); } - let file_size = metadata.len(); + let metadata_size = metadata.len(); // Defense-in-depth: reject files exceeding the default max size at // Stage 1 to prevent a misbehaving agent from filling the staging // disk before Stage 3 gets a chance to enforce the operator's limit. - if file_size > DEFAULT_MAX_FILE_SIZE { + if metadata_size > DEFAULT_MAX_FILE_SIZE { return Err(anyhow_to_mcp_error(anyhow::anyhow!( "File '{}' is {} bytes, exceeding the maximum staging size of {} bytes", params.0.file_path, - file_size, + metadata_size, DEFAULT_MAX_FILE_SIZE ))); } @@ -1108,6 +1108,10 @@ may apply per the workflow's safe-outputs config." )) })?; let staged_sha256 = crate::hash::sha256_hex(&source_bytes); + // Use the actual byte count rather than the earlier metadata.len() so + // that the recorded size matches the staged content exactly, closing + // a TOCTOU window if the source file changes between stat and read. + let file_size = source_bytes.len() as u64; let staged_path = self.output_directory.join(&staged_filename); tokio::fs::write(&staged_path, &source_bytes).await.map_err(|e| { @@ -1189,13 +1193,13 @@ restrictions may apply per the workflow's safe-outputs config." params.0.file_path ))); } - let file_size = metadata.len(); + let metadata_size = metadata.len(); - if file_size > PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE { + if metadata_size > PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE { return Err(anyhow_to_mcp_error(anyhow::anyhow!( "File '{}' is {} bytes, exceeding the maximum staging size of {} bytes", params.0.file_path, - file_size, + metadata_size, PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE ))); } @@ -1233,6 +1237,10 @@ restrictions may apply per the workflow's safe-outputs config." )) })?; let staged_sha256 = crate::hash::sha256_hex(&source_bytes); + // Use the actual byte count rather than the earlier metadata.len() so + // that the recorded size matches the staged content exactly, closing + // a TOCTOU window if the source file changes between stat and read. + let file_size = source_bytes.len() as u64; let staged_path = self.output_directory.join(&staged_filename); tokio::fs::write(&staged_path, &source_bytes).await.map_err(|e| { diff --git a/src/safeoutputs/upload_pipeline_artifact.rs b/src/safeoutputs/upload_pipeline_artifact.rs index 916a7ff5..7d12a723 100644 --- a/src/safeoutputs/upload_pipeline_artifact.rs +++ b/src/safeoutputs/upload_pipeline_artifact.rs @@ -522,7 +522,7 @@ impl Executor for UploadPipelineArtifactResult { Ok(ExecutionResult::success_with_data( format!( "Published '{}' as pipeline artifact '{}' on build #{}", - self.file_path, effective_build_id, final_name + self.file_path, final_name, effective_build_id ), serde_json::json!({ "build_id": effective_build_id, From 6b6153abf38b39df027f3c2e7f60979b4a83db39 Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 5 May 2026 14:49:12 +0100 Subject: [PATCH 4/4] fix(safe-outputs): add project scope to container creation request Include the project GUID in the container creation POST body so the container is scoped to the project, matching the scope query param used in the subsequent file upload step. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/safeoutputs/upload_pipeline_artifact.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/safeoutputs/upload_pipeline_artifact.rs b/src/safeoutputs/upload_pipeline_artifact.rs index 7d12a723..172f457f 100644 --- a/src/safeoutputs/upload_pipeline_artifact.rs +++ b/src/safeoutputs/upload_pipeline_artifact.rs @@ -412,6 +412,7 @@ impl Executor for UploadPipelineArtifactResult { let container_body = serde_json::json!({ "name": final_name, + "scope": project_id, }); let container_resp = client .post(&container_url)