diff --git a/src/engine.rs b/src/engine.rs index 02a06fe4..7a552a52 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -469,11 +469,48 @@ fn collect_allowed_tools( edit_enabled: bool, ) -> Result> { let mut allowed_tools = collect_extension_allowed_tools(extension_declarations); + add_mcp_server_tools(front_matter, &mut allowed_tools); - // Tools from user-defined MCP servers (sorted for deterministic output). - // Only add --allow-tool for MCPs that will actually produce an MCPG entry (i.e., - // WithOptions that have a container or url). McpConfig::Enabled(true) has no backing - // server in MCPG, so granting the permission would cause confusing runtime errors. + // Intentional: with restricted bash, both --allow-tool write (tool identity) + // and --allow-all-paths (path scope) are emitted. --allow-all-tools subsumes + // --allow-tool write, so only --allow-all-paths is needed on that path. + if edit_enabled { + allowed_tools.push("write".to_string()); + } + + let bash_commands = build_bash_command_list(front_matter, extension_declarations); + for cmd in bash_commands { + // Reject single quotes — copilot_params are embedded inside a single-quoted + // bash string in the AWF command. + if cmd.contains('\'') { + anyhow::bail!( + "Bash command '{}' contains a single quote, which is not allowed \ + (would break AWF shell quoting).", + cmd + ); + } + allowed_tools.push(format!("shell({})", cmd)); + } + + Ok(allowed_tools) +} + +/// Returns `true` when an MCP server config has a real backing server (container +/// or URL). `McpConfig::Enabled(true)` has no backing server in MCPG, so granting +/// its permission would cause confusing runtime errors. +fn mcp_server_has_backing_server(config: &McpConfig) -> bool { + match config { + McpConfig::Enabled(_) => false, + McpConfig::WithOptions(opts) => { + opts.enabled.unwrap_or(true) && (opts.container.is_some() || opts.url.is_some()) + } + } +} + +/// Appends user-defined MCP server names to `allowed_tools` (sorted for +/// deterministic output), skipping any server already provided by an extension +/// and any server without a backing MCPG entry. +fn add_mcp_server_tools(front_matter: &FrontMatter, allowed_tools: &mut Vec) { let mut sorted_mcps: Vec<_> = front_matter.mcp_servers.iter().collect(); sorted_mcps.sort_by_key(|(a, _)| *a); for (name, config) in sorted_mcps { @@ -482,46 +519,28 @@ fn collect_allowed_tools( if allowed_tools.iter().any(|t| t.eq_ignore_ascii_case(name)) { continue; } - // Only add MCPs that have a backing server (container or url) - let has_backing_server = match config { - McpConfig::Enabled(_) => false, - McpConfig::WithOptions(opts) => { - opts.enabled.unwrap_or(true) && (opts.container.is_some() || opts.url.is_some()) - } - }; - if has_backing_server { + if mcp_server_has_backing_server(config) { allowed_tools.push(name.clone()); } } +} - // Intentional: with restricted bash, both --allow-tool write (tool identity) - // and --allow-all-paths (path scope) are emitted. --allow-all-tools subsumes - // --allow-tool write, so only --allow-all-paths is needed on that path. - if edit_enabled { - allowed_tools.push("write".to_string()); - } - - // Bash tool: use the explicitly configured list. - // When bash is None (not specified), use_allow_all_tools is true and this - // function is not called — that invariant is upheld by the caller. +/// Builds the deduplicated bash command list from the front-matter configuration +/// augmented with any extension-declared commands (runtimes + first-party tools). +/// +/// When bash is `None`, `use_allow_all_tools` is `true` and this function is never +/// called — the caller upholds that invariant. +fn build_bash_command_list( + front_matter: &FrontMatter, + extension_declarations: &[Declarations], +) -> Vec { let mut bash_commands: Vec = match front_matter.tools.as_ref().and_then(|t| t.bash.as_ref()) { - Some(cmds) if cmds.is_empty() => { - // Explicitly disabled: no bash commands - vec![] - } - Some(cmds) => { - // Explicit list of commands - cmds.clone() - } - None => { - // Invariant: bash=None → use_allow_all_tools=true → this function is - // not called. Panic if the invariant is ever broken. - unreachable!("bash=None should imply use_allow_all_tools=true") - } + Some(cmds) if cmds.is_empty() => vec![], + Some(cmds) => cmds.clone(), + // Invariant: bash=None → use_allow_all_tools=true → not called. + None => unreachable!("bash=None should imply use_allow_all_tools=true"), }; - - // Auto-add extension-declared bash commands (runtimes + first-party tools) for decl in extension_declarations { for cmd in &decl.bash_commands { if !bash_commands.contains(cmd) { @@ -529,21 +548,7 @@ fn collect_allowed_tools( } } } - - for cmd in &bash_commands { - // Reject single quotes in bash commands — copilot_params are embedded inside - // a single-quoted bash string in the AWF command. - if cmd.contains('\'') { - anyhow::bail!( - "Bash command '{}' contains a single quote, which is not allowed \ - (would break AWF shell quoting).", - cmd - ); - } - allowed_tools.push(format!("shell({})", cmd)); - } - - Ok(allowed_tools) + bash_commands } /// Validates a single `engine.args` entry.