From c1c4cfafb884a632062795ff97d4b88e6d08b548 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 11 Apr 2026 17:30:02 +0100 Subject: [PATCH 1/9] feat(mcp): filter SafeOutputs tools based on front matter config Only expose safe output tools that are configured in the front matter's safe-outputs section. This reduces agent confusion by hiding tools that aren't configured for the current pipeline. Implementation: - Add --enabled-tools repeatable CLI arg to mcp and mcp-http commands - SafeOutputs::new() accepts optional enabled tools list and uses ToolRouter::remove_route() to remove unconfigured tools at startup - Compiler derives tool list from safe-outputs keys and emits --enabled-tools args in the pipeline template - Always-on diagnostic tools (noop, missing-data, missing-tool, report-incomplete) are never filtered out Backward compatible: if --enabled-tools is not passed (empty safe-outputs or omitted), all tools remain available. Note: MCPG has a tools field in its config schema but does not enforce it at runtime. This change filters at the SafeOutputs server level instead, making it self-contained. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 72 ++++++++++++++++++++++ src/compile/standalone.rs | 17 ++++-- src/main.rs | 16 ++++- src/mcp.rs | 123 +++++++++++++++++++++++++++++++++++--- templates/base.yml | 1 + 5 files changed, 214 insertions(+), 15 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index a75ae6b..7df09b0 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -671,6 +671,41 @@ pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> Stri } } +/// 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). +pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { + use crate::mcp::ALWAYS_ON_TOOLS; + + if front_matter.safe_outputs.is_empty() { + return String::new(); + } + + let mut tools: Vec = front_matter + .safe_outputs + .keys() + .cloned() + .collect(); + + // Always include diagnostic tools + for tool in ALWAYS_ON_TOOLS { + let name = tool.to_string(); + if !tools.contains(&name) { + tools.push(name); + } + } + + tools.sort(); + + tools + .iter() + .map(|t| format!("--enabled-tools {}", t)) + .collect::>() + .join(" ") +} + /// Safe-output names that require write access to ADO. const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = &[ "create-pull-request", @@ -1637,4 +1672,41 @@ 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"); + } } 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..fbf4407 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -63,6 +63,15 @@ fn generate_short_id() -> String { format!("{:06x}", (timestamp & 0xFFFFFF) as u32) } +/// Safe output tools that are always available regardless of filtering. +/// These are diagnostic/transparency tools that agents should always have access to. +pub const ALWAYS_ON_TOOLS: &[&str] = &[ + "noop", + "missing-data", + "missing-tool", + "report-incomplete", +]; + // ============================================================================ // SafeOutputs MCP Server // ============================================================================ @@ -119,6 +128,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 +152,27 @@ 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(); + 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); + } + } + let remaining: Vec = tool_router.list_all().iter().map(|t| t.name.to_string()).collect(); + info!("Tool filtering applied: {} of {} tools enabled: {:?}", remaining.len(), all_tools.len(), remaining); + } + Ok(Self { bounding_directory: bounding_dir, output_directory: output_dir, - tool_router: Self::tool_router(), + tool_router, }) } @@ -847,9 +874,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 +899,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 +944,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 +1017,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 +1078,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 +1092,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 +1266,85 @@ 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(); + + // Enabled tools + always-on + let expected_count = enabled.len() + ALWAYS_ON_TOOLS.len(); + assert_eq!( + tool_names.len(), + expected_count, + "Expected {} tools, got {}: {:?}", + expected_count, + tool_names.len(), + tool_names + ); + } } diff --git a/templates/base.yml b/templates/base.yml index cb85e60..8f11ef4 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -216,6 +216,7 @@ jobs: nohup /tmp/awf-tools/ado-aw mcp-http \ --port "$SAFE_OUTPUTS_PORT" \ --api-key "$SAFE_OUTPUTS_API_KEY" \ + {{ enabled_tools_args }}\ "/tmp/awf-tools/staging" \ "{{ working_directory }}" \ > "$(Agent.TempDirectory)/staging/logs/safeoutputs.log" 2>&1 & From 61ca32faebb49a4a19c180fd5c3e8d8387a1f70b Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 11 Apr 2026 17:49:19 +0100 Subject: [PATCH 2/9] fix: address PR review feedback for SafeOutputs tool filtering - Validate tool names against safe regex (ASCII alphanumeric + hyphens) to prevent shell injection from malicious YAML keys - Fix dangling backslash in base.yml when enabled_tools_args is empty - Replace fragile exact-count assertion in test_tool_filtering_multiple_tools with explicit presence/absence checks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 50 ++++++++++++++++++++++++++++++++++++++----- src/mcp.rs | 31 ++++++++++++++++++--------- templates/base.yml | 3 +-- 3 files changed, 67 insertions(+), 17 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 7df09b0..a36c877 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -671,11 +671,19 @@ pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> Stri } } +/// Returns true if the name contains only ASCII alphanumerics and hyphens. +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). +/// +/// Tool names are validated to contain only ASCII alphanumerics and hyphens +/// to prevent shell injection when the args are embedded in bash commands. pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { use crate::mcp::ALWAYS_ON_TOOLS; @@ -683,11 +691,17 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { return String::new(); } - let mut tools: Vec = front_matter - .safe_outputs - .keys() - .cloned() - .collect(); + let mut tools: Vec = Vec::new(); + 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; + } + tools.push(key.clone()); + } // Always include diagnostic tools for tool in ALWAYS_ON_TOOLS { @@ -1709,4 +1723,30 @@ mod tests { 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_unsafe_names() { + // We can't easily inject unsafe names through parse_markdown (YAML + // keys are validated), so we test the validation function directly. + // The is_safe_tool_name + generate_enabled_tools_args integration + // is covered by the unit tests for is_safe_tool_name above. + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n" + ).unwrap(); + let args = generate_enabled_tools_args(&fm); + // All tool names from YAML are safe alphanumeric-hyphen names + assert!(args.contains("--enabled-tools create-pull-request")); + } } diff --git a/src/mcp.rs b/src/mcp.rs index fbf4407..ad873f9 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -1336,15 +1336,26 @@ mod tests { let tools = so.tool_router.list_all(); let tool_names: Vec = tools.iter().map(|t| t.name.to_string()).collect(); - // Enabled tools + always-on - let expected_count = enabled.len() + ALWAYS_ON_TOOLS.len(); - assert_eq!( - tool_names.len(), - expected_count, - "Expected {} tools, got {}: {:?}", - expected_count, - tool_names.len(), - tool_names - ); + // 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"); } } diff --git a/templates/base.yml b/templates/base.yml index 8f11ef4..f9ef1ea 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -216,8 +216,7 @@ jobs: nohup /tmp/awf-tools/ado-aw mcp-http \ --port "$SAFE_OUTPUTS_PORT" \ --api-key "$SAFE_OUTPUTS_API_KEY" \ - {{ enabled_tools_args }}\ - "/tmp/awf-tools/staging" \ + {{ enabled_tools_args }}"/tmp/awf-tools/staging" \ "{{ working_directory }}" \ > "$(Agent.TempDirectory)/staging/logs/safeoutputs.log" 2>&1 & SAFE_OUTPUTS_PID=$! From 15e5f9588798a3e19b60477d9a73a35ebc74ef46 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 09:46:25 +0100 Subject: [PATCH 3/9] fix: trailing space for enabled_tools_args and move ALWAYS_ON_TOOLS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add trailing space to generate_enabled_tools_args output when non-empty, preventing the last --enabled-tools value from concatenating with the next positional argument in the shell command - Move ALWAYS_ON_TOOLS constant from mcp.rs to tools/mod.rs to avoid compile→mcp coupling (common.rs now imports from tools directly) - Reduce list_all() calls in tool filtering to a single collect pass Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 10 +++++++--- src/mcp.rs | 13 ++++--------- src/tools/mod.rs | 9 +++++++++ 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index a36c877..86820bb 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -685,7 +685,7 @@ fn is_safe_tool_name(name: &str) -> bool { /// Tool names are validated to contain only ASCII alphanumerics and hyphens /// to prevent shell injection when the args are embedded in bash commands. pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { - use crate::mcp::ALWAYS_ON_TOOLS; + use crate::tools::ALWAYS_ON_TOOLS; if front_matter.safe_outputs.is_empty() { return String::new(); @@ -713,11 +713,15 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { tools.sort(); - tools + let args = tools .iter() .map(|t| format!("--enabled-tools {}", t)) .collect::>() - .join(" ") + .join(" "); + + // Trailing space so the args don't concatenate with the next positional + // argument when embedded inline in the shell template. + if args.is_empty() { args } else { args + " " } } /// Safe-output names that require write access to ADO. diff --git a/src/mcp.rs b/src/mcp.rs index ad873f9..b25699a 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -63,14 +63,8 @@ fn generate_short_id() -> String { format!("{:06x}", (timestamp & 0xFFFFFF) as u32) } -/// Safe output tools that are always available regardless of filtering. -/// These are diagnostic/transparency tools that agents should always have access to. -pub const ALWAYS_ON_TOOLS: &[&str] = &[ - "noop", - "missing-data", - "missing-tool", - "report-incomplete", -]; +// Re-export from tools module +use crate::tools::ALWAYS_ON_TOOLS; // ============================================================================ // SafeOutputs MCP Server @@ -157,6 +151,7 @@ impl SafeOutputs { // 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); @@ -166,7 +161,7 @@ impl SafeOutputs { } } let remaining: Vec = tool_router.list_all().iter().map(|t| t.name.to_string()).collect(); - info!("Tool filtering applied: {} of {} tools enabled: {:?}", remaining.len(), all_tools.len(), remaining); + info!("Tool filtering applied: {} of {} tools enabled: {:?}", remaining.len(), total, remaining); } Ok(Self { diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 3897226..38a9c6c 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -10,6 +10,15 @@ 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. +pub const ALWAYS_ON_TOOLS: &[&str] = &[ + "noop", + "missing-data", + "missing-tool", + "report-incomplete", +]; + /// Resolve the effective branch for a wiki. /// /// If `configured_branch` is `Some`, that value is returned directly. From cd0d7af5a5c18e032ed2d57f7e15ca15ad6c502b Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 09:56:44 +0100 Subject: [PATCH 4/9] fix: warn on unrecognized safe-output keys at compile time - Add ALL_KNOWN_SAFE_OUTPUTS constant in tools/mod.rs enumerating every valid safe-output key (MCP tools + always-on diagnostics + memory) - Emit compile-time warning when a safe-outputs key doesn't match any known tool, catching typos like 'crate-pull-request' early - Use HashSet for O(1) deduplication when merging always-on tools - Rename misleading test to test_generate_enabled_tools_args_warns_on_unknown_tool and exercise the typo path (crate-pull-request) - Document {{ enabled_tools_args }} template marker in AGENTS.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 10 ++++++++++ src/compile/common.rs | 34 +++++++++++++++++++++++----------- src/tools/mod.rs | 31 +++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 11 deletions(-) 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 86820bb..94b3ff7 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -684,13 +684,16 @@ fn is_safe_tool_name(name: &str) -> bool { /// /// 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. pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { - use crate::tools::ALWAYS_ON_TOOLS; + use crate::tools::{ALL_KNOWN_SAFE_OUTPUTS, ALWAYS_ON_TOOLS}; + use std::collections::HashSet; if front_matter.safe_outputs.is_empty() { return String::new(); } + let mut seen = HashSet::new(); let mut tools: Vec = Vec::new(); for key in front_matter.safe_outputs.keys() { if !is_safe_tool_name(key) { @@ -700,13 +703,21 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { ); continue; } - tools.push(key.clone()); + if !ALL_KNOWN_SAFE_OUTPUTS.contains(&key.as_str()) { + eprintln!( + "Warning: unrecognized safe-output tool '{}' — will be ignored at runtime", + key + ); + } + if seen.insert(key.clone()) { + tools.push(key.clone()); + } } // Always include diagnostic tools for tool in ALWAYS_ON_TOOLS { let name = tool.to_string(); - if !tools.contains(&name) { + if seen.insert(name.clone()) { tools.push(name); } } @@ -1741,16 +1752,17 @@ mod tests { } #[test] - fn test_generate_enabled_tools_args_skips_unsafe_names() { - // We can't easily inject unsafe names through parse_markdown (YAML - // keys are validated), so we test the validation function directly. - // The is_safe_tool_name + generate_enabled_tools_args integration - // is covered by the unit tests for is_safe_tool_name above. + fn test_generate_enabled_tools_args_warns_on_unknown_tool() { + // An unrecognized (but safe-formatted) tool name should still appear + // in the output (it passes is_safe_tool_name) but a warning is emitted + // to stderr. We verify it's included in the args string. let (fm, _) = parse_markdown( - "---\nname: test\ndescription: test\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n" + "---\nname: test\ndescription: test\nsafe-outputs:\n crate-pull-request:\n target-branch: main\n---\n" ).unwrap(); let args = generate_enabled_tools_args(&fm); - // All tool names from YAML are safe alphanumeric-hyphen names - assert!(args.contains("--enabled-tools create-pull-request")); + // Typo'd name is still forwarded (warning is printed to stderr) + assert!(args.contains("--enabled-tools crate-pull-request")); + // Always-on tools are also included + assert!(args.contains("--enabled-tools noop")); } } diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 38a9c6c..653ed6e 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -19,6 +19,37 @@ pub const ALWAYS_ON_TOOLS: &[&str] = &[ "report-incomplete", ]; +/// All recognised safe-output keys accepted in front matter `safe-outputs:`. +/// This is the union of MCP tool names, always-on diagnostics, and non-MCP +/// safe-output keys (like `memory`) that are handled by the compiler/executor. +pub const ALL_KNOWN_SAFE_OUTPUTS: &[&str] = &[ + // Always-on diagnostics + "noop", + "missing-data", + "missing-tool", + "report-incomplete", + // Write-requiring MCP tools + "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", + // Non-MCP safe-output keys (handled by compiler/executor) + "memory", +]; + /// Resolve the effective branch for a wiki. /// /// If `configured_branch` is `Some`, that value is returned directly. From 972a5f5b7fe32e8772c4d3d2caf80a03bbe8aa63 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 10:12:33 +0100 Subject: [PATCH 5/9] fix: skip unrecognized safe-output tool names in enabled-tools args Previously, unrecognized tool names (e.g. typos like "crate-pull-request") were warned about but still appended to --enabled-tools. This caused the real tool (create-pull-request) to be silently filtered out at runtime because it was absent from the enabled list. Now unrecognized names are skipped entirely, making the warning and behavior consistent. The warning message is also updated to be clearer. Additional improvements: - Add test asserting ALL_KNOWN_SAFE_OUTPUTS covers every router-registered tool, preventing the list from drifting when new tools are added - Add integration test verifying --enabled-tools flags appear in compiled pipeline YAML (end-to-end: standalone.rs + generate_enabled_tools_args + template substitution) - Document is_safe_tool_name newline-safety requirement - Add explanatory comment in base.yml template for the inline substitution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 18 +++++----- src/mcp.rs | 27 ++++++++++++++ templates/base.yml | 2 ++ tests/compiler_tests.rs | 79 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 8 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 94b3ff7..f2260f6 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -672,6 +672,8 @@ pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> Stri } /// 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 == '-') } @@ -705,9 +707,10 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { } if !ALL_KNOWN_SAFE_OUTPUTS.contains(&key.as_str()) { eprintln!( - "Warning: unrecognized safe-output tool '{}' — will be ignored at runtime", + "Warning: unrecognized safe-output tool '{}' — skipping (no registered tool matches this name)", key ); + continue; } if seen.insert(key.clone()) { tools.push(key.clone()); @@ -1752,17 +1755,16 @@ mod tests { } #[test] - fn test_generate_enabled_tools_args_warns_on_unknown_tool() { - // An unrecognized (but safe-formatted) tool name should still appear - // in the output (it passes is_safe_tool_name) but a warning is emitted - // to stderr. We verify it's included in the args string. + fn test_generate_enabled_tools_args_skips_unknown_tool() { + // An unrecognized (but safe-formatted) tool name should be skipped + // from the output and a warning is emitted to stderr. 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); - // Typo'd name is still forwarded (warning is printed to stderr) - assert!(args.contains("--enabled-tools crate-pull-request")); - // Always-on tools are also included + // Typo'd name is NOT forwarded — it would silently disable the real tool + assert!(!args.contains("crate-pull-request"), "Unrecognized tool should be skipped"); + // Always-on tools are still included assert!(args.contains("--enabled-tools noop")); } } diff --git a/src/mcp.rs b/src/mcp.rs index b25699a..c4eee93 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -1353,4 +1353,31 @@ mod tests { 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/templates/base.yml b/templates/base.yml index f9ef1ea..68ec983 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -213,6 +213,8 @@ 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. nohup /tmp/awf-tools/ado-aw mcp-http \ --port "$SAFE_OUTPUTS_PORT" \ --api-key "$SAFE_OUTPUTS_API_KEY" \ 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); +} From cb04273111b1239ec572d510d1b847c2afc9020b Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 22:35:34 +0100 Subject: [PATCH 6/9] fix: skip non-MCP keys and warn on all-unrecognized safe-outputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR review feedback: - Skip non-MCP safe-output keys (e.g. `memory`) from --enabled-tools generation. These keys have no MCP route, so including them would cause real MCP tools to be filtered out at runtime. - Add prominent warning when all user-specified safe-output keys are invalid/unrecognized/non-MCP, since the agent would be restricted to diagnostic tools only. - Remove unreachable `args.is_empty()` branch — ALWAYS_ON_TOOLS guarantees the args string is never empty when safe-outputs is configured. - Add tests for memory key skipping and memory-only configuration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 55 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index f2260f6..9090312 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -684,9 +684,12 @@ fn is_safe_tool_name(name: &str) -> bool { /// 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. +/// 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}; use std::collections::HashSet; @@ -695,8 +698,14 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { return String::new(); } + // 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). + const NON_MCP_SAFE_OUTPUT_KEYS: &[&str] = &["memory"]; + 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!( @@ -705,6 +714,9 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { ); 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)", @@ -712,11 +724,23 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { ); continue; } + user_tool_count += 1; if seen.insert(key.clone()) { tools.push(key.clone()); } } + if user_tool_count == 0 && !front_matter.safe_outputs.is_empty() { + // Every user-specified key was either invalid, unrecognized, or non-MCP. + // Only diagnostic tools will be available — this is almost certainly a mistake. + let keys: Vec<&String> = front_matter.safe_outputs.keys().collect(); + eprintln!( + "Warning: no valid MCP tools resolved from safe-outputs keys {:?} — \ + the agent will only have access to diagnostic tools (noop, missing-data, etc.)", + keys + ); + } + // Always include diagnostic tools for tool in ALWAYS_ON_TOOLS { let name = tool.to_string(); @@ -735,7 +759,8 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { // Trailing space so the args don't concatenate with the next positional // argument when embedded inline in the shell template. - if args.is_empty() { args } else { args + " " } + // `args` is never empty here because ALWAYS_ON_TOOLS always contributes entries. + args + " " } /// Safe-output names that require write access to ADO. @@ -1767,4 +1792,30 @@ mod tests { // Always-on tools are still included assert!(args.contains("--enabled-tools noop")); } + + #[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 MCP tool args should be + // generated (backward compat: all tools available). But since safe_outputs + // is non-empty, we still get ALWAYS_ON_TOOLS in the args. + 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.contains("--enabled-tools memory"), "Non-MCP key 'memory' should be skipped"); + // Always-on tools are still present + assert!(args.contains("--enabled-tools noop")); + } } From 2215cf332dc35e696a4ebf11061bc7c24dab8778 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 23:09:06 +0100 Subject: [PATCH 7/9] fix: memory-only safe-outputs should not restrict MCP tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When safe-outputs contains only non-MCP keys (e.g. just `memory`) or only unrecognized names (typos), return empty args so all tools remain available — matching backward-compatible behavior. Previously this path would emit only ALWAYS_ON_TOOLS in --enabled-tools, silently restricting the agent to 4 diagnostic tools. Also: - Add debug! log in server-side filtering for enabled-tools entries that have no matching route, aiding troubleshooting - Add template comment documenting positional arg ordering requirement Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 30 ++++++++++-------------------- src/mcp.rs | 6 ++++++ templates/base.yml | 2 ++ 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 9090312..776ff66 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -730,15 +730,10 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { } } - if user_tool_count == 0 && !front_matter.safe_outputs.is_empty() { - // Every user-specified key was either invalid, unrecognized, or non-MCP. - // Only diagnostic tools will be available — this is almost certainly a mistake. - let keys: Vec<&String> = front_matter.safe_outputs.keys().collect(); - eprintln!( - "Warning: no valid MCP tools resolved from safe-outputs keys {:?} — \ - the agent will only have access to diagnostic tools (noop, missing-data, etc.)", - keys - ); + 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 @@ -1781,16 +1776,14 @@ mod tests { #[test] fn test_generate_enabled_tools_args_skips_unknown_tool() { - // An unrecognized (but safe-formatted) tool name should be skipped - // from the output and a warning is emitted to stderr. + // 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); - // Typo'd name is NOT forwarded — it would silently disable the real tool assert!(!args.contains("crate-pull-request"), "Unrecognized tool should be skipped"); - // Always-on tools are still included - assert!(args.contains("--enabled-tools noop")); + assert!(args.is_empty(), "All-unrecognized safe-outputs should produce no args (all tools available)"); } #[test] @@ -1807,15 +1800,12 @@ mod tests { #[test] fn test_generate_enabled_tools_args_memory_only_does_not_filter() { - // When `memory` is the only safe-output key, no MCP tool args should be - // generated (backward compat: all tools available). But since safe_outputs - // is non-empty, we still get ALWAYS_ON_TOOLS in the args. + // 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.contains("--enabled-tools memory"), "Non-MCP key 'memory' should be skipped"); - // Always-on tools are still present - assert!(args.contains("--enabled-tools noop")); + assert!(args.is_empty(), "memory-only safe-outputs should produce no args (all tools available)"); } } diff --git a/src/mcp.rs b/src/mcp.rs index c4eee93..5875dff 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -160,6 +160,12 @@ impl SafeOutputs { 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); } diff --git a/templates/base.yml b/templates/base.yml index 68ec983..e61ba91 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -215,6 +215,8 @@ jobs: # 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" \ From bbb99887c151ff7cb07b173d5ad986d8528430b6 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 23:37:24 +0100 Subject: [PATCH 8/9] refactor: derive safe-output tool lists from types at compile time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace hand-maintained string arrays with compile-time type-derived lists using two new macros: - tool_names![Type1, Type2, ...] → extracts ToolResult::NAME from each type into a &[&str] array - all_safe_output_names![types...; "extra"] → combines type names with non-MCP string keys Changes: - Add ToolResult::REQUIRES_WRITE associated constant (default false) - Extend tool_result! macro with `write = true` parameter (4 arms) - Annotate all 17 write-requiring tools with write = true - CreatePrResult (manual impl) also gets REQUIRES_WRITE = true - Move WRITE_REQUIRING_SAFE_OUTPUTS from common.rs to tools/mod.rs - Derive ALWAYS_ON_TOOLS, WRITE_REQUIRING_SAFE_OUTPUTS, and ALL_KNOWN_SAFE_OUTPUTS from type lists — no more string duplication - Add NON_MCP_SAFE_OUTPUT_KEYS as public const in mod.rs - Add 5 subset/consistency tests: - WRITE_REQUIRING ⊆ ALL_KNOWN - ALWAYS_ON ⊆ ALL_KNOWN - NON_MCP ⊆ ALL_KNOWN - REQUIRES_WRITE consistency (true for write tools, false for diag) - ALL_KNOWN count = write + diagnostic + non-MCP Adding a new tool now requires adding its type to the list in mod.rs; the name string is derived automatically from ToolResult::NAME, eliminating the risk of typo drift between lists. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 30 +---- src/tools/add_build_tag.rs | 1 + src/tools/add_pr_comment.rs | 1 + src/tools/comment_on_work_item.rs | 1 + src/tools/create_branch.rs | 1 + src/tools/create_git_tag.rs | 1 + src/tools/create_pr.rs | 1 + src/tools/create_wiki_page.rs | 1 + src/tools/create_work_item.rs | 1 + src/tools/link_work_items.rs | 1 + src/tools/mod.rs | 182 ++++++++++++++++++++++++------ src/tools/queue_build.rs | 1 + src/tools/reply_to_pr_comment.rs | 1 + src/tools/resolve_pr_thread.rs | 1 + src/tools/result.rs | 143 +++++++++++++++++++++++ src/tools/submit_pr_review.rs | 1 + src/tools/update_pr.rs | 1 + src/tools/update_wiki_page.rs | 1 + src/tools/update_work_item.rs | 1 + src/tools/upload_attachment.rs | 1 + 20 files changed, 313 insertions(+), 59 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 776ff66..8c9e031 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -691,18 +691,13 @@ fn is_safe_tool_name(name: &str) -> bool { /// 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}; + 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(); } - // 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). - const NON_MCP_SAFE_OUTPUT_KEYS: &[&str] = &["memory"]; - let mut seen = HashSet::new(); let mut tools: Vec = Vec::new(); let mut user_tool_count = 0usize; @@ -758,29 +753,10 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { args + " " } -/// 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", -]; - /// 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() 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 653ed6e..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}; @@ -12,42 +13,77 @@ pub(crate) const PATH_SEGMENT: &AsciiSet = &CONTROLS.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. -pub const ALWAYS_ON_TOOLS: &[&str] = &[ - "noop", - "missing-data", - "missing-tool", - "report-incomplete", +/// +/// 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 MCP tool names, always-on diagnostics, and non-MCP -/// safe-output keys (like `memory`) that are handled by the compiler/executor. -pub const ALL_KNOWN_SAFE_OUTPUTS: &[&str] = &[ - // Always-on diagnostics - "noop", - "missing-data", - "missing-tool", - "report-incomplete", +/// 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 - "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", - // Non-MCP safe-output keys (handled by compiler/executor) - "memory", + 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. @@ -253,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 { From f69cd3d477ce1c709aab7a4033515beddad8bb04 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 23:39:15 +0100 Subject: [PATCH 9/9] chore: document 1ES safe-outputs filtering gap and clarify HashSet purpose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add note in onees.rs explaining that 1ES target does not support --enabled-tools filtering (uses service connections, not mcp-http) - Clarify HashSet comment in generate_enabled_tools_args — it deduplicates across user keys and ALWAYS_ON_TOOLS, not within HashMap keys Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 2 ++ src/compile/onees.rs | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/src/compile/common.rs b/src/compile/common.rs index 8c9e031..5b203c7 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -698,6 +698,8 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { 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; 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![