From 5042d6e81c4bf28df9d722bbc4dacbcc91046e24 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 10 Jul 2026 10:05:06 +0000 Subject: [PATCH] refactor(engine): reduce complexity of collect_allowed_tools Extract collect_extension_tools, mcp_server_has_backing_server, add_mcp_server_tools, and build_bash_command_list from the monolithic collect_allowed_tools function. Before: single ~90-line function with three nested loops and two in-line match arms (estimated cognitive complexity ~20). After: collect_allowed_tools is a 25-line coordinator; each concern is named and independently testable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/engine.rs | 122 +++++++++++++++++++++++++++----------------------- 1 file changed, 66 insertions(+), 56 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index b675a4ad..324abc7c 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -449,21 +449,63 @@ fn collect_allowed_tools( extension_declarations: &[Declarations], edit_enabled: bool, ) -> Result> { - let mut allowed_tools: Vec = Vec::new(); + let mut allowed_tools = collect_extension_tools(extension_declarations); + add_mcp_server_tools(front_matter, &mut allowed_tools); - // Tools from compiler extensions (github, safeoutputs, azure-devops, etc.) + // 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) +} + +/// Collects deduplicated tool identifiers declared by compiler extensions +/// (github, safeoutputs, azure-devops, etc.). +fn collect_extension_tools(extension_declarations: &[Declarations]) -> Vec { + let mut tools: Vec = Vec::new(); for decl in extension_declarations { for tool in &decl.copilot_allow_tools { - if !allowed_tools.contains(tool) { - allowed_tools.push(tool.clone()); + if !tools.contains(tool) { + tools.push(tool.clone()); } } } + 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()) + } + } +} - // 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. +/// 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 { @@ -472,46 +514,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) { @@ -519,21 +543,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.