Skip to content

Commit f43b22e

Browse files
feat(mcp): filter SafeOutputs tools based on front matter config (#156)
* 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> * 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> * fix: trailing space for enabled_tools_args and move ALWAYS_ON_TOOLS - 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> * 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> * 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> * fix: skip non-MCP keys and warn on all-unrecognized safe-outputs 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> * fix: memory-only safe-outputs should not restrict MCP tools 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> * refactor: derive safe-output tool lists from types at compile time 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> * chore: document 1ES safe-outputs filtering gap and clarify HashSet purpose - 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> * fix: correct all_safe_output_names! doc comment and rename variable - Fix doc comment: example showed two semicolons but macro only accepts one. Clarify that all types go before the semicolon, string keys after. - Rename user_tool_count → effective_mcp_tool_count for clarity — it counts recognized, valid, non-MCP keys, not all user-specified keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: promote unmatched enabled-tools log to warn and add disjointness checks - Promote debug! to warn! for --enabled-tools entries with no matching route, matching the compile-time warning severity. A typo like "crate-pull-request" would otherwise be invisible in production logs. - Add explicit disjointness assertions in test_all_known_completeness so overlapping lists produce a clear "tool X in both lists" message rather than a confusing count mismatch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ffdd1f9 commit f43b22e

27 files changed

Lines changed: 788 additions & 36 deletions

AGENTS.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,16 @@ Should be replaced with the comma-separated domain list for AWF's `--allow-domai
605605

606606
The output is formatted as a comma-separated string (e.g., `github.com,*.dev.azure.com,api.github.com`).
607607

608+
## {{ enabled_tools_args }}
609+
610+
Should be replaced with `--enabled-tools <name>` 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`).
611+
612+
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.
613+
614+
Tool names are validated at compile time:
615+
- Names must contain only ASCII alphanumerics and hyphens (shell injection prevention)
616+
- Unrecognized names (not in `ALL_KNOWN_SAFE_OUTPUTS`) emit a warning to catch typos
617+
608618
## {{ cancel_previous_builds }}
609619

610620
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).

src/compile/common.rs

Lines changed: 169 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -671,29 +671,94 @@ pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> Stri
671671
}
672672
}
673673

674-
/// Safe-output names that require write access to ADO.
675-
const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = &[
676-
"create-pull-request",
677-
"create-work-item",
678-
"comment-on-work-item",
679-
"update-work-item",
680-
"create-wiki-page",
681-
"update-wiki-page",
682-
"add-pr-comment",
683-
"link-work-items",
684-
"queue-build",
685-
"create-git-tag",
686-
"add-build-tag",
687-
"create-branch",
688-
"update-pr",
689-
"upload-attachment",
690-
"submit-pr-review",
691-
"reply-to-pr-review-comment",
692-
"resolve-pr-review-thread",
693-
];
674+
/// Returns true if the name contains only ASCII alphanumerics and hyphens.
675+
/// This value is embedded inline in a shell command, so control characters
676+
/// (including newlines) and whitespace must be rejected to prevent corruption.
677+
fn is_safe_tool_name(name: &str) -> bool {
678+
!name.is_empty() && name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-')
679+
}
680+
681+
/// Generate `--enabled-tools` CLI args for the SafeOutputs MCP server.
682+
///
683+
/// Derives the tool list from `safe-outputs:` front matter keys plus always-on
684+
/// diagnostic tools. If `safe-outputs:` is empty, returns an empty string
685+
/// (all tools enabled for backward compatibility).
686+
///
687+
/// Non-MCP keys (like `memory`) are silently skipped — they are handled by the
688+
/// executor and have no corresponding MCP route.
689+
///
690+
/// Tool names are validated to contain only ASCII alphanumerics and hyphens
691+
/// to prevent shell injection when the args are embedded in bash commands.
692+
/// Unrecognized tool names emit a compile-time warning and are skipped.
693+
pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String {
694+
use crate::tools::{ALL_KNOWN_SAFE_OUTPUTS, ALWAYS_ON_TOOLS, NON_MCP_SAFE_OUTPUT_KEYS};
695+
use std::collections::HashSet;
696+
697+
if front_matter.safe_outputs.is_empty() {
698+
return String::new();
699+
}
700+
701+
// `seen` deduplicates across user keys and ALWAYS_ON_TOOLS (e.g. if the user
702+
// configures `noop` explicitly, it shouldn't appear twice in the output).
703+
let mut seen = HashSet::new();
704+
let mut tools: Vec<String> = Vec::new();
705+
let mut effective_mcp_tool_count = 0usize;
706+
for key in front_matter.safe_outputs.keys() {
707+
if !is_safe_tool_name(key) {
708+
eprintln!(
709+
"Warning: skipping invalid safe-output tool name '{}' (must be ASCII alphanumeric/hyphens only)",
710+
key
711+
);
712+
continue;
713+
}
714+
if NON_MCP_SAFE_OUTPUT_KEYS.contains(&key.as_str()) {
715+
continue;
716+
}
717+
if !ALL_KNOWN_SAFE_OUTPUTS.contains(&key.as_str()) {
718+
eprintln!(
719+
"Warning: unrecognized safe-output tool '{}' — skipping (no registered tool matches this name)",
720+
key
721+
);
722+
continue;
723+
}
724+
effective_mcp_tool_count += 1;
725+
if seen.insert(key.clone()) {
726+
tools.push(key.clone());
727+
}
728+
}
729+
730+
if effective_mcp_tool_count == 0 {
731+
// Every user-specified key was either invalid, unrecognized, or non-MCP
732+
// (e.g. memory-only). Return empty to keep all tools available (backward compat).
733+
return String::new();
734+
}
735+
736+
// Always include diagnostic tools
737+
for tool in ALWAYS_ON_TOOLS {
738+
let name = tool.to_string();
739+
if seen.insert(name.clone()) {
740+
tools.push(name);
741+
}
742+
}
743+
744+
tools.sort();
745+
746+
let args = tools
747+
.iter()
748+
.map(|t| format!("--enabled-tools {}", t))
749+
.collect::<Vec<_>>()
750+
.join(" ");
751+
752+
// Trailing space so the args don't concatenate with the next positional
753+
// argument when embedded inline in the shell template.
754+
// `args` is never empty here because ALWAYS_ON_TOOLS always contributes entries.
755+
args + " "
756+
}
694757

695758
/// Validate that write-requiring safe-outputs have a write service connection configured.
696759
pub fn validate_write_permissions(front_matter: &FrontMatter) -> Result<()> {
760+
use crate::tools::WRITE_REQUIRING_SAFE_OUTPUTS;
761+
697762
let has_write_sc = front_matter
698763
.permissions
699764
.as_ref()
@@ -1637,4 +1702,88 @@ mod tests {
16371702
).unwrap();
16381703
assert!(validate_resolve_pr_thread_statuses(&fm).is_ok());
16391704
}
1705+
1706+
// ─── Enabled tools args generation ──────────────────────────────────
1707+
1708+
#[test]
1709+
fn test_generate_enabled_tools_args_empty_safe_outputs() {
1710+
let (fm, _) = parse_markdown(
1711+
"---\nname: test\ndescription: test\n---\n"
1712+
).unwrap();
1713+
let args = generate_enabled_tools_args(&fm);
1714+
assert!(args.is_empty(), "Empty safe-outputs should produce no args");
1715+
}
1716+
1717+
#[test]
1718+
fn test_generate_enabled_tools_args_with_configured_tools() {
1719+
let (fm, _) = parse_markdown(
1720+
"---\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"
1721+
).unwrap();
1722+
let args = generate_enabled_tools_args(&fm);
1723+
assert!(args.contains("--enabled-tools create-pull-request"));
1724+
assert!(args.contains("--enabled-tools create-work-item"));
1725+
// Always-on tools should also be included
1726+
assert!(args.contains("--enabled-tools noop"));
1727+
assert!(args.contains("--enabled-tools missing-data"));
1728+
assert!(args.contains("--enabled-tools missing-tool"));
1729+
assert!(args.contains("--enabled-tools report-incomplete"));
1730+
}
1731+
1732+
#[test]
1733+
fn test_generate_enabled_tools_args_no_duplicates() {
1734+
// If a diagnostic tool is also in safe-outputs, it shouldn't appear twice
1735+
let (fm, _) = parse_markdown(
1736+
"---\nname: test\ndescription: test\nsafe-outputs:\n noop:\n max: 5\n---\n"
1737+
).unwrap();
1738+
let args = generate_enabled_tools_args(&fm);
1739+
let noop_count = args.matches("--enabled-tools noop").count();
1740+
assert_eq!(noop_count, 1, "noop should appear exactly once");
1741+
}
1742+
1743+
#[test]
1744+
fn test_is_safe_tool_name() {
1745+
assert!(is_safe_tool_name("create-pull-request"));
1746+
assert!(is_safe_tool_name("noop"));
1747+
assert!(is_safe_tool_name("my-tool-123"));
1748+
assert!(!is_safe_tool_name(""));
1749+
assert!(!is_safe_tool_name("$(curl evil.com)"));
1750+
assert!(!is_safe_tool_name("foo; rm -rf /"));
1751+
assert!(!is_safe_tool_name("tool name"));
1752+
assert!(!is_safe_tool_name("tool\ttab"));
1753+
}
1754+
1755+
#[test]
1756+
fn test_generate_enabled_tools_args_skips_unknown_tool() {
1757+
// An unrecognized (but safe-formatted) tool name should be skipped.
1758+
// When no valid MCP tools remain, return empty (all tools available).
1759+
let (fm, _) = parse_markdown(
1760+
"---\nname: test\ndescription: test\nsafe-outputs:\n crate-pull-request:\n target-branch: main\n---\n"
1761+
).unwrap();
1762+
let args = generate_enabled_tools_args(&fm);
1763+
assert!(!args.contains("crate-pull-request"), "Unrecognized tool should be skipped");
1764+
assert!(args.is_empty(), "All-unrecognized safe-outputs should produce no args (all tools available)");
1765+
}
1766+
1767+
#[test]
1768+
fn test_generate_enabled_tools_args_skips_memory_key() {
1769+
// `memory` is a non-MCP executor-only key. It must not appear in
1770+
// --enabled-tools or it would cause real MCP tools to be filtered out.
1771+
let (fm, _) = parse_markdown(
1772+
"---\nname: test\ndescription: test\nsafe-outputs:\n memory:\n allowed-extensions:\n - .md\n create-pull-request:\n target-branch: main\n---\n"
1773+
).unwrap();
1774+
let args = generate_enabled_tools_args(&fm);
1775+
assert!(!args.contains("--enabled-tools memory"), "Non-MCP key 'memory' should be skipped");
1776+
assert!(args.contains("--enabled-tools create-pull-request"), "Real MCP tool should be present");
1777+
}
1778+
1779+
#[test]
1780+
fn test_generate_enabled_tools_args_memory_only_does_not_filter() {
1781+
// When `memory` is the only safe-output key, no --enabled-tools args should
1782+
// be generated so all tools remain available (backward compat).
1783+
let (fm, _) = parse_markdown(
1784+
"---\nname: test\ndescription: test\nsafe-outputs:\n memory:\n allowed-extensions:\n - .md\n---\n"
1785+
).unwrap();
1786+
let args = generate_enabled_tools_args(&fm);
1787+
assert!(args.is_empty(), "memory-only safe-outputs should produce no args (all tools available)");
1788+
}
16401789
}

src/compile/onees.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,12 @@ displayName: "Finalize""#,
159159
// Validate resolve-pr-review-thread has required allowed-statuses field
160160
validate_resolve_pr_thread_statuses(front_matter)?;
161161

162+
// NOTE: 1ES target does not support --enabled-tools filtering (safe-outputs
163+
// tool filtering). 1ES uses service connections for MCP servers rather than
164+
// mcp-http, so generate_enabled_tools_args is not called here. If safe-outputs
165+
// filtering is needed for 1ES, it would require changes to the 1ES pipeline
166+
// template and agency job configuration.
167+
162168
// Replace all template markers
163169
let compiler_version = env!("CARGO_PKG_VERSION");
164170
let replacements: Vec<(&str, &str)> = vec![

src/compile/standalone.rs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,13 @@ use super::common::{
1717
self, AWF_VERSION, COPILOT_CLI_VERSION, DEFAULT_POOL, MCPG_PORT, MCPG_VERSION,
1818
compute_effective_workspace, generate_acquire_ado_token, generate_cancel_previous_builds,
1919
generate_checkout_self, generate_checkout_steps, generate_ci_trigger, generate_copilot_ado_env,
20-
generate_copilot_params, generate_executor_ado_env, generate_header_comment,
21-
generate_job_timeout, generate_pipeline_path, generate_pipeline_resources, generate_pr_trigger,
22-
generate_repositories, generate_schedule, generate_source_path, generate_working_directory,
23-
replace_with_indent, sanitize_filename, validate_comment_target,
24-
validate_resolve_pr_thread_statuses, validate_submit_pr_review_events,
25-
validate_update_pr_votes, validate_update_work_item_target, validate_write_permissions,
20+
generate_copilot_params, generate_enabled_tools_args, generate_executor_ado_env,
21+
generate_header_comment, generate_job_timeout, generate_pipeline_path,
22+
generate_pipeline_resources, generate_pr_trigger, generate_repositories, generate_schedule,
23+
generate_source_path, generate_working_directory, replace_with_indent, sanitize_filename,
24+
validate_comment_target, validate_resolve_pr_thread_statuses,
25+
validate_submit_pr_review_events, validate_update_pr_votes,
26+
validate_update_work_item_target, validate_write_permissions,
2627
};
2728
use super::types::{FrontMatter, McpConfig};
2829
use crate::allowed_hosts::{CORE_ALLOWED_HOSTS, mcp_required_hosts};
@@ -85,6 +86,9 @@ impl Compiler for StandaloneCompiler {
8586
// Generate comma-separated domain list for AWF
8687
let allowed_domains = generate_allowed_domains(front_matter);
8788

89+
// Generate --enabled-tools args for SafeOutputs tool filtering
90+
let enabled_tools_args = generate_enabled_tools_args(front_matter);
91+
8892
// Pool name
8993
let pool = front_matter
9094
.pool
@@ -183,6 +187,7 @@ impl Compiler for StandaloneCompiler {
183187
("{{ working_directory }}", &working_directory),
184188
("{{ workspace }}", &working_directory),
185189
("{{ allowed_domains }}", &allowed_domains),
190+
("{{ enabled_tools_args }}", &enabled_tools_args),
186191
("{{ agent_content }}", markdown_body),
187192
("{{ acquire_ado_token }}", &acquire_read_token),
188193
("{{ copilot_ado_env }}", &copilot_ado_env),

src/main.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ enum Commands {
4747
output_directory: String,
4848
/// Guard against directory traversal attacks by specifying the agent cannot influence folders outside this path
4949
bounding_directory: String,
50+
/// Only expose these safe output tools (can be repeated). If omitted, all tools are exposed.
51+
#[arg(long = "enabled-tools")]
52+
enabled_tools: Vec<String>,
5053
},
5154
/// Execute safe outputs from Stage 1 (Stage 2 of the pipeline)
5255
Execute {
@@ -84,6 +87,9 @@ enum Commands {
8487
output_directory: String,
8588
/// Guard against directory traversal attacks
8689
bounding_directory: String,
90+
/// Only expose these safe output tools (can be repeated). If omitted, all tools are exposed.
91+
#[arg(long = "enabled-tools")]
92+
enabled_tools: Vec<String>,
8793
},
8894
/// Detect agentic pipelines and update GITHUB_TOKEN on their ADO definitions
8995
Configure {
@@ -167,7 +173,11 @@ async fn main() -> Result<()> {
167173
Commands::Mcp {
168174
output_directory,
169175
bounding_directory,
170-
} => mcp::run(&output_directory, &bounding_directory).await?,
176+
enabled_tools,
177+
} => {
178+
let filter = if enabled_tools.is_empty() { None } else { Some(enabled_tools) };
179+
mcp::run(&output_directory, &bounding_directory, filter.as_deref()).await?
180+
}
171181
Commands::Execute {
172182
source,
173183
safe_output_dir,
@@ -273,8 +283,10 @@ async fn main() -> Result<()> {
273283
api_key,
274284
output_directory,
275285
bounding_directory,
286+
enabled_tools,
276287
} => {
277-
mcp::run_http(&output_directory, &bounding_directory, port, api_key.as_deref())
288+
let filter = if enabled_tools.is_empty() { None } else { Some(enabled_tools) };
289+
mcp::run_http(&output_directory, &bounding_directory, port, api_key.as_deref(), filter.as_deref())
278290
.await?;
279291
}
280292
Commands::Configure {

0 commit comments

Comments
 (0)