refactor(engine): reduce complexity of collect_allowed_tools (#1454)

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: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>

Author: 4 people

src/engine.rs

@@ -469,11 +469,48 @@ fn collect_allowed_tools(
     edit_enabled: bool,
 ) -> Result<Vec<String>> {
     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<String>) {
     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,68 +519,36 @@ 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<String> {
     let mut bash_commands: Vec<String> =
         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) {
                 bash_commands.push(cmd.clone());
             }
         }
     }
-
-    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.