diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 8879a08e..ca259a50 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -2674,9 +2674,10 @@ fn start_mcpg_step( # - Rewrite URLs from 127.0.0.1 to host.docker.internal (AWF container needs\n\ # host.docker.internal to reach MCPG on the host; 127.0.0.1 is container loopback)\n\ # - Ensure tools: [\"*\"] on each server entry (Copilot CLI requirement)\n\ + # - Mark generated MCPG entries as default/trusted servers for Copilot CLI\n\ # - Preserve all other fields (headers, type, etc.)\n\ jq --arg prefix \"http://$(MCP_GATEWAY_DOMAIN):$(MCP_GATEWAY_PORT)\" \\\n \ - '.mcpServers |= (to_entries | sort_by(.key) | map(.value.url |= sub(\"^http://[^/]+/\"; \"\\($prefix)/\") | .value.tools = [\"*\"]) | from_entries)' \\\n \ + '.mcpServers |= (to_entries | sort_by(.key) | map(.value.url |= sub(\"^http://[^/]+/\"; \"\\($prefix)/\") | .value.tools = [\"*\"] | .value.isDefaultServer = true) | from_entries)' \\\n \ \"$GATEWAY_OUTPUT\" > /tmp/awf-tools/mcp-config.json\n\ \n\ chmod 600 /tmp/awf-tools/mcp-config.json\n\ @@ -3679,6 +3680,24 @@ mod tests { assert!(msg.contains("missing `env:` key"), "got: {msg}"); } + // ── start_mcpg_step ───────────────────────────────────────────────────── + + #[test] + fn start_mcpg_step_marks_copilot_mcp_servers_as_default() { + let step = start_mcpg_step("", "", false, None).unwrap(); + + assert!( + step.script.contains(".value.tools = [\"*\"]"), + "Copilot mcp-config conversion should preserve wildcard tools: {}", + step.script + ); + assert!( + step.script.contains(".value.isDefaultServer = true"), + "Copilot mcp-config conversion should mark generated MCP servers as default/trusted: {}", + step.script + ); + } + // ── split_yaml_step_sequence ─────────────────────────────────────────── #[test] diff --git a/src/compile/common.rs b/src/compile/common.rs index 1b4428c8..bdb715b7 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -3445,8 +3445,8 @@ mod tests { "wildcard bash should emit --allow-all-tools" ); assert!( - !params.contains("--allow-tool"), - "no individual --allow-tool flags with --allow-all-tools" + params.contains("--allow-tool safeoutputs"), + "safeoutputs should be explicitly allowed even with --allow-all-tools" ); } @@ -3465,8 +3465,8 @@ mod tests { "\"*\" should behave same as \":*\"" ); assert!( - !params.contains("--allow-tool"), - "no individual --allow-tool flags with --allow-all-tools" + params.contains("--allow-tool safeoutputs"), + "safeoutputs should be explicitly allowed even with --allow-all-tools" ); } @@ -3511,8 +3511,8 @@ mod tests { "default (no bash) should emit --allow-all-tools" ); assert!( - !params.contains("--allow-tool"), - "no individual --allow-tool flags with --allow-all-tools" + params.contains("--allow-tool safeoutputs"), + "safeoutputs should be explicitly allowed even with --allow-all-tools" ); } @@ -3555,8 +3555,8 @@ mod tests { "edit enabled should still emit --allow-all-paths" ); assert!( - !params.contains("--allow-tool"), - "no individual --allow-tool flags" + params.contains("--allow-tool safeoutputs"), + "safeoutputs should be explicitly allowed even with --allow-all-tools" ); } @@ -3615,10 +3615,13 @@ mod tests { params.contains("--allow-all-tools"), "wildcard should use --allow-all-tools" ); - // Should NOT add individual tool flags when --allow-all-tools is active assert!( - !params.contains("--allow-tool"), - "no individual tool flags with --allow-all-tools" + params.contains("--allow-tool safeoutputs"), + "safeoutputs should be explicitly allowed even with --allow-all-tools" + ); + assert!( + !params.contains("shell(lean)"), + "runtime shell tools should still be covered by --allow-all-tools" ); } @@ -3637,6 +3640,10 @@ mod tests { !params.contains("--allow-tool my-tool"), "default (all-tools) mode should not emit individual --allow-tool for MCPs" ); + assert!( + params.contains("--allow-tool safeoutputs"), + "compiler-owned safeoutputs should still get an explicit grant" + ); } #[test] diff --git a/src/engine.rs b/src/engine.rs index b675a4ad..02a06fe4 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -439,6 +439,25 @@ impl Engine { } } +fn push_unique_tool(allowed_tools: &mut Vec, tool: &str) { + if !allowed_tools.iter().any(|existing| existing == tool) { + allowed_tools.push(tool.to_string()); + } +} + +fn collect_extension_allowed_tools(extension_declarations: &[Declarations]) -> Vec { + let mut allowed_tools: Vec = Vec::new(); + + // Tools from compiler extensions (github, safeoutputs, azure-devops, etc.) + for decl in extension_declarations { + for tool in &decl.copilot_allow_tools { + push_unique_tool(&mut allowed_tools, tool); + } + } + + allowed_tools +} + /// Collects the list of allowed tool identifiers when bash is not in wildcard mode. /// /// Returns a flat `Vec` of fully-qualified tool identifiers ready to be @@ -449,16 +468,7 @@ fn collect_allowed_tools( extension_declarations: &[Declarations], edit_enabled: bool, ) -> Result> { - let mut allowed_tools: Vec = Vec::new(); - - // Tools from compiler extensions (github, safeoutputs, azure-devops, etc.) - for decl in extension_declarations { - for tool in &decl.copilot_allow_tools { - if !allowed_tools.contains(tool) { - allowed_tools.push(tool.clone()); - } - } - } + let mut allowed_tools = collect_extension_allowed_tools(extension_declarations); // 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., @@ -588,10 +598,12 @@ fn copilot_args( .and_then(|t| t.edit) .unwrap_or(true); - // When --allow-all-tools is active, skip individual tool collection entirely. - // --allow-all-tools is a superset that permits all tool calls regardless. + // When --allow-all-tools is active, skip user/tool collection but keep + // compiler-owned extension grants. Copilot's MCP policy still requires + // first-party generated MCP servers (notably safeoutputs) to be explicitly + // permitted even when all tool calls are otherwise allowed. let allowed_tools: Vec = if use_allow_all_tools { - Vec::new() + collect_extension_allowed_tools(extension_declarations) } else { collect_allowed_tools(front_matter, extension_declarations, edit_enabled)? }; @@ -651,15 +663,15 @@ fn copilot_args( if use_allow_all_tools { params.push("--allow-all-tools".to_string()); - } else { - for tool in allowed_tools { - if tool.contains('(') || tool.contains(')') || tool.contains(' ') { - // Use double quotes - the copilot_params are embedded inside a single-quoted - // bash string in the AWF command, so single quotes would break quoting. - params.push(format!("--allow-tool \"{}\"", tool)); - } else { - params.push(format!("--allow-tool {}", tool)); - } + } + + for tool in allowed_tools { + if tool.contains('(') || tool.contains(')') || tool.contains(' ') { + // Use double quotes - the copilot_params are embedded inside a single-quoted + // bash string in the AWF command, so single quotes would break quoting. + params.push(format!("--allow-tool \"{}\"", tool)); + } else { + params.push(format!("--allow-tool {}", tool)); } } diff --git a/tests/copilot_cli_safeoutputs_tests.rs b/tests/copilot_cli_safeoutputs_tests.rs index 78dc374b..bb9b0588 100644 --- a/tests/copilot_cli_safeoutputs_tests.rs +++ b/tests/copilot_cli_safeoutputs_tests.rs @@ -182,6 +182,44 @@ fn extract_agent_invocation(compiled: &str) -> String { line[start + 1..end].to_string() } +/// Splits the compiler-emitted Copilot command line enough for this contract +/// test: whitespace separates words, single/double quotes group words, and +/// backslash escapes the next character inside double quotes. It intentionally +/// does not perform shell expansion or model every shell escape form. +fn split_shell_words(input: &str) -> Vec { + let mut words = Vec::new(); + let mut current = String::new(); + let mut chars = input.chars().peekable(); + let mut quote = None; + + while let Some(ch) = chars.next() { + match (quote, ch) { + (None, '\'') => quote = Some('\''), + (None, '"') => quote = Some('"'), + (None, ch) if ch.is_whitespace() => { + if !current.is_empty() { + words.push(std::mem::take(&mut current)); + } + } + (Some(q), ch) if ch == q => quote = None, + (Some('"'), '\\') => { + if let Some(next) = chars.next() { + current.push(next); + } + } + (_, ch) => current.push(ch), + } + } + + if let Some(quote) = quote { + panic!("unterminated {quote:?} shell quote in invocation: {input}"); + } + if !current.is_empty() { + words.push(current); + } + words +} + fn build_local_copilot_mcp_config( mcpg_template: &str, port: u16, @@ -327,21 +365,37 @@ fn real_copilot_cli_noop_contract() { ); fs::write(&prompt_path, &prompt).expect("write prompt"); - let prefix = "/tmp/awf-tools/copilot --prompt \"$(cat /tmp/awf-tools/agent-prompt.md)\" --additional-mcp-config @/tmp/awf-tools/mcp-config.json "; - let suffix = invocation - .strip_prefix(prefix) - .unwrap_or_else(|| panic!("unexpected compiler-emitted invocation prefix: {invocation}")); - let copilot_bin = std::env::var("ADO_AW_COPILOT_CLI_PATH").unwrap_or_else(|_| "copilot".to_string()); + let invocation_args = split_shell_words(&invocation); + assert_eq!( + invocation_args.first().map(String::as_str), + Some("/tmp/awf-tools/copilot"), + "unexpected compiler-emitted Copilot binary in invocation: {invocation}" + ); + let mut command = Command::new(&copilot_bin); - command - .arg("--prompt") - .arg(&prompt) - .arg("--additional-mcp-config") - .arg(format!("@{}", copilot_mcp_config_path.display())); - for arg in suffix.split_whitespace() { - command.arg(arg); + let mut args = invocation_args.iter().skip(1); + while let Some(arg) = args.next() { + match arg.as_str() { + "--prompt" => { + let _ = args.next().unwrap_or_else(|| { + panic!("missing --prompt value in invocation: {invocation}") + }); + command.arg("--prompt").arg(&prompt); + } + "--additional-mcp-config" => { + let _ = args.next().unwrap_or_else(|| { + panic!("missing --additional-mcp-config value in invocation: {invocation}") + }); + command + .arg("--additional-mcp-config") + .arg(format!("@{}", copilot_mcp_config_path.display())); + } + _ => { + command.arg(arg); + } + } } command.current_dir(server.output_dir.path()); command.env("XDG_CONFIG_HOME", artifact_dir.join("xdg"));