From 8fce7eb7e160cc89773b78def37fda0999144eb6 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 09:32:57 +0100 Subject: [PATCH 1/9] refactor: remove dead $HOME/.copilot mcp-config copy from templates The AWF invocation explicitly passes --additional-mcp-config @/tmp/awf-tools/mcp-config.json, so the copy to $HOME/.copilot/ mcp-config.json was never read by copilot inside the AWF container. Removes from both base.yml and 1es-base.yml: - mkdir -p "$HOME/.copilot" - cp/chmod of mcp-config.json to $HOME/.copilot/ Part of #287 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .vscode/mcp.json | 11 +++++++++++ .vscode/settings.json | 5 +++++ src/data/1es-base.yml | 5 +---- src/data/base.yml | 5 +---- 4 files changed, 18 insertions(+), 8 deletions(-) create mode 100644 .vscode/mcp.json create mode 100644 .vscode/settings.json diff --git a/.vscode/mcp.json b/.vscode/mcp.json new file mode 100644 index 0000000..96e7285 --- /dev/null +++ b/.vscode/mcp.json @@ -0,0 +1,11 @@ +{ + "servers": { + "github-agentic-workflows": { + "command": "gh", + "args": [ + "aw", + "mcp-server" + ] + } + } +} \ No newline at end of file diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..dbd4bd7 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,5 @@ +{ + "github.copilot.enable": { + "markdown": true + } +} \ No newline at end of file diff --git a/src/data/1es-base.yml b/src/data/1es-base.yml index ab28da2..71ea796 100644 --- a/src/data/1es-base.yml +++ b/src/data/1es-base.yml @@ -129,7 +129,6 @@ extends: displayName: "Prepare MCPG config" - bash: | - mkdir -p "$HOME/.copilot" mkdir -p /tmp/awf-tools/staging echo "HOME: $HOME" @@ -330,9 +329,7 @@ extends: '.mcpServers |= (to_entries | sort_by(.key) | map(.value.url |= sub("^http://[^/]+/"; "\($prefix)/") | .value.tools = ["*"]) | from_entries)' \ "$GATEWAY_OUTPUT" > /tmp/awf-tools/mcp-config.json - # Also write to $HOME/.copilot for host-side use - cp /tmp/awf-tools/mcp-config.json "$HOME/.copilot/mcp-config.json" - chmod 600 /tmp/awf-tools/mcp-config.json "$HOME/.copilot/mcp-config.json" + chmod 600 /tmp/awf-tools/mcp-config.json echo "Generated MCP config at: /tmp/awf-tools/mcp-config.json" cat /tmp/awf-tools/mcp-config.json diff --git a/src/data/base.yml b/src/data/base.yml index e101836..3b4844a 100644 --- a/src/data/base.yml +++ b/src/data/base.yml @@ -100,7 +100,6 @@ jobs: displayName: "Prepare MCPG config" - bash: | - mkdir -p "$HOME/.copilot" mkdir -p /tmp/awf-tools/staging echo "HOME: $HOME" @@ -301,9 +300,7 @@ jobs: '.mcpServers |= (to_entries | sort_by(.key) | map(.value.url |= sub("^http://[^/]+/"; "\($prefix)/") | .value.tools = ["*"]) | from_entries)' \ "$GATEWAY_OUTPUT" > /tmp/awf-tools/mcp-config.json - # Also write to $HOME/.copilot for host-side use - cp /tmp/awf-tools/mcp-config.json "$HOME/.copilot/mcp-config.json" - chmod 600 /tmp/awf-tools/mcp-config.json "$HOME/.copilot/mcp-config.json" + chmod 600 /tmp/awf-tools/mcp-config.json echo "Generated MCP config at: /tmp/awf-tools/mcp-config.json" cat /tmp/awf-tools/mcp-config.json From 92fc8490713e2ac4fd9cdfd195f1a68a6e113098 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 09:34:02 +0100 Subject: [PATCH 2/9] refactor: move engine log paths behind Engine::log_dir() Add Engine::log_dir() method returning the engine-specific log directory (~/.copilot/logs for Copilot). Replace 6 hardcoded ~/.copilot/logs paths in both templates with {{ engine_log_dir }} marker, substituted at compile time. Part of #287 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 2 ++ src/data/1es-base.yml | 12 ++++++------ src/data/base.yml | 12 ++++++------ src/engine.rs | 9 +++++++++ 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 5d4996d..f3236e3 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1965,6 +1965,7 @@ pub async fn compile_shared( "SC_READ_TOKEN", ); let engine_env = ctx.engine.env(); + let engine_log_dir = ctx.engine.log_dir(); let acquire_write_token = generate_acquire_ado_token( front_matter .permissions @@ -2046,6 +2047,7 @@ pub async fn compile_shared( ("{{ agent_content }}", markdown_body), ("{{ acquire_ado_token }}", &acquire_read_token), ("{{ engine_env }}", &engine_env), + ("{{ engine_log_dir }}", engine_log_dir), ("{{ acquire_write_token }}", &acquire_write_token), ("{{ executor_ado_env }}", &executor_ado_env), ]; diff --git a/src/data/1es-base.yml b/src/data/1es-base.yml index 71ea796..cb0851a 100644 --- a/src/data/1es-base.yml +++ b/src/data/1es-base.yml @@ -412,8 +412,8 @@ extends: - bash: | # Copy all logs to output directory for artifact upload mkdir -p "$(Agent.TempDirectory)/staging/logs" - if [ -d ~/.copilot/logs ]; then - cp -r ~/.copilot/logs/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true + if [ -d {{ engine_log_dir }} ]; then + cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then cp -r ~/.ado-aw/logs/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true @@ -631,9 +631,9 @@ extends: - bash: | # Copy all logs to analyzed outputs for artifact upload mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs" - if [ -d ~/.copilot/logs ]; then + if [ -d {{ engine_log_dir }} ]; then mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot" - cp -r ~/.copilot/logs/* "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot/" 2>/dev/null || true + cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs/ado-aw" @@ -713,9 +713,9 @@ extends: # Copy agent output log from analyzed_outputs for optimisation use cp "$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)/logs/agent-output.txt" \ "$(Agent.TempDirectory)/staging/logs/agent-output.txt" 2>/dev/null || true - if [ -d ~/.copilot/logs ]; then + if [ -d {{ engine_log_dir }} ]; then mkdir -p "$(Agent.TempDirectory)/staging/logs/copilot" - cp -r ~/.copilot/logs/* "$(Agent.TempDirectory)/staging/logs/copilot/" 2>/dev/null || true + cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/staging/logs/copilot/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then mkdir -p "$(Agent.TempDirectory)/staging/logs/ado-aw" diff --git a/src/data/base.yml b/src/data/base.yml index 3b4844a..a9027cf 100644 --- a/src/data/base.yml +++ b/src/data/base.yml @@ -383,8 +383,8 @@ jobs: - bash: | # Copy all logs to output directory for artifact upload mkdir -p "$(Agent.TempDirectory)/staging/logs" - if [ -d ~/.copilot/logs ]; then - cp -r ~/.copilot/logs/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true + if [ -d {{ engine_log_dir }} ]; then + cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then cp -r ~/.ado-aw/logs/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true @@ -600,9 +600,9 @@ jobs: - bash: | # Copy all logs to analyzed outputs for artifact upload mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs" - if [ -d ~/.copilot/logs ]; then + if [ -d {{ engine_log_dir }} ]; then mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot" - cp -r ~/.copilot/logs/* "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot/" 2>/dev/null || true + cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs/ado-aw" @@ -681,9 +681,9 @@ jobs: # Copy agent output log from analyzed_outputs for optimisation use cp "$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)/logs/agent-output.txt" \ "$(Agent.TempDirectory)/staging/logs/agent-output.txt" 2>/dev/null || true - if [ -d ~/.copilot/logs ]; then + if [ -d {{ engine_log_dir }} ]; then mkdir -p "$(Agent.TempDirectory)/staging/logs/copilot" - cp -r ~/.copilot/logs/* "$(Agent.TempDirectory)/staging/logs/copilot/" 2>/dev/null || true + cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/staging/logs/copilot/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then mkdir -p "$(Agent.TempDirectory)/staging/logs/ado-aw" diff --git a/src/engine.rs b/src/engine.rs index 81234ae..e3ed153 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -61,6 +61,15 @@ impl Engine { Engine::Copilot => copilot_env(), } } + + /// Return the engine's log directory path. + /// + /// Used by log collection steps to copy engine logs to pipeline artifacts. + pub fn log_dir(&self) -> &str { + match self { + Engine::Copilot => "~/.copilot/logs", + } + } } fn copilot_args( From 997d1b70d2257422e107f4a04c1972f0f8914345 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 09:38:44 +0100 Subject: [PATCH 3/9] refactor: move engine install steps behind Engine::install_steps() Add Engine::install_steps(engine_config) that generates the full NuGet install YAML for the Copilot CLI. Replaces 4 hardcoded install blocks (Agent + Detection jobs in both templates) with a single {{ engine_install_steps }} marker. - Move COPILOT_CLI_VERSION constant from common.rs to engine.rs - Wire engine.version front matter field: uses custom version if set, falls back to COPILOT_CLI_VERSION - Return empty string when engine.command is set (skip NuGet install) - Remove {{ copilot_version }} marker (absorbed into install_steps) - Remove engine.version "not yet wired" warning Part of #287 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 8 ++--- src/data/1es-base.yml | 49 ++----------------------------- src/data/base.yml | 49 ++----------------------------- src/engine.rs | 68 ++++++++++++++++++++++++++++++++++++++----- 4 files changed, 66 insertions(+), 108 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index f3236e3..7dda129 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -545,11 +545,6 @@ pub const DEFAULT_POOL: &str = "AZS-1ES-L-MMS-ubuntu-22.04"; /// See: https://github.com/github/gh-aw-firewall/releases pub const AWF_VERSION: &str = "0.25.26"; -/// Version of the GitHub Copilot CLI (Microsoft.Copilot.CLI.linux-x64) NuGet package to install. -/// Update this when upgrading to a new Copilot CLI release. -/// See: https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json -pub const COPILOT_CLI_VERSION: &str = "1.0.34"; - /// Prefix used to identify agentic pipeline YAML files generated by ado-aw. pub const HEADER_MARKER: &str = "# @ado-aw"; @@ -1917,6 +1912,7 @@ pub async fn compile_shared( // 4. Generate copilot params let copilot_params = ctx.engine.args(ctx.front_matter, extensions)?; + let engine_install_steps = ctx.engine.install_steps(&front_matter.engine); // 5. Compute workspace, working directory, triggers let effective_workspace = compute_effective_workspace( @@ -2018,7 +2014,7 @@ pub async fn compile_shared( let replacements: Vec<(&str, &str)> = vec![ ("{{ parameters }}", ¶meters_yaml), ("{{ compiler_version }}", compiler_version), - ("{{ copilot_version }}", COPILOT_CLI_VERSION), + ("{{ engine_install_steps }}", &engine_install_steps), ("{{ pool }}", &pool), ("{{ setup_job }}", &setup_job), ("{{ teardown_job }}", &teardown_job), diff --git a/src/data/1es-base.yml b/src/data/1es-base.yml index cb0851a..327f3c2 100644 --- a/src/data/1es-base.yml +++ b/src/data/1es-base.yml @@ -56,30 +56,7 @@ extends: {{ acquire_ado_token }} - - task: NuGetAuthenticate@1 - displayName: "Authenticate NuGet Feed" - - - task: NuGetCommand@2 - displayName: "Install Copilot CLI" - inputs: - command: 'custom' - arguments: 'install Microsoft.Copilot.CLI.linux-x64 -Source "https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json" -Version {{ copilot_version }} -OutputDirectory $(Agent.TempDirectory)/tools -ExcludeVersion -NonInteractive' - - - bash: | - ls -la "$(Agent.TempDirectory)/tools" - echo "##vso[task.prependpath]$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64" - - # Copy copilot binary to /tmp so it's accessible inside AWF container - # (AWF auto-mounts /tmp:/tmp:rw but not Agent.TempDirectory) - mkdir -p /tmp/awf-tools - cp "$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64/copilot" /tmp/awf-tools/copilot - chmod +x /tmp/awf-tools/copilot - displayName: "Add copilot to PATH" - - - bash: | - copilot --version - copilot -h - displayName: "Output copilot version" + {{ engine_install_steps }} - bash: | COMPILER_VERSION="{{ compiler_version }}" @@ -445,29 +422,7 @@ extends: - download: current artifact: agent_outputs_$(Build.BuildId) - - task: NuGetAuthenticate@1 - displayName: "Authenticate NuGet Feed" - - - task: NuGetCommand@2 - displayName: "Install Copilot CLI" - inputs: - command: 'custom' - arguments: 'install Microsoft.Copilot.CLI.linux-x64 -Source "https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json" -Version {{ copilot_version }} -OutputDirectory $(Agent.TempDirectory)/tools -ExcludeVersion -NonInteractive' - - - bash: | - ls -la "$(Agent.TempDirectory)/tools" - echo "##vso[task.prependpath]$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64" - - # Copy copilot binary to /tmp so it's accessible inside AWF container - mkdir -p /tmp/awf-tools - cp "$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64/copilot" /tmp/awf-tools/copilot - chmod +x /tmp/awf-tools/copilot - displayName: "Add copilot to PATH" - - - bash: | - copilot --version - copilot -h - displayName: "Output copilot version" + {{ engine_install_steps }} - bash: | COMPILER_VERSION="{{ compiler_version }}" diff --git a/src/data/base.yml b/src/data/base.yml index a9027cf..386dcd7 100644 --- a/src/data/base.yml +++ b/src/data/base.yml @@ -27,30 +27,7 @@ jobs: {{ acquire_ado_token }} - - task: NuGetAuthenticate@1 - displayName: "Authenticate NuGet Feed" - - - task: NuGetCommand@2 - displayName: "Install Copilot CLI" - inputs: - command: 'custom' - arguments: 'install Microsoft.Copilot.CLI.linux-x64 -Source "https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json" -Version {{ copilot_version }} -OutputDirectory $(Agent.TempDirectory)/tools -ExcludeVersion -NonInteractive' - - - bash: | - ls -la "$(Agent.TempDirectory)/tools" - echo "##vso[task.prependpath]$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64" - - # Copy copilot binary to /tmp so it's accessible inside AWF container - # (AWF auto-mounts /tmp:/tmp:rw but not Agent.TempDirectory) - mkdir -p /tmp/awf-tools - cp "$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64/copilot" /tmp/awf-tools/copilot - chmod +x /tmp/awf-tools/copilot - displayName: "Add copilot to PATH" - - - bash: | - copilot --version - copilot -h - displayName: "Output copilot version" + {{ engine_install_steps }} - bash: | COMPILER_VERSION="{{ compiler_version }}" @@ -414,29 +391,7 @@ jobs: - download: current artifact: agent_outputs_$(Build.BuildId) - - task: NuGetAuthenticate@1 - displayName: "Authenticate NuGet Feed" - - - task: NuGetCommand@2 - displayName: "Install Copilot CLI" - inputs: - command: 'custom' - arguments: 'install Microsoft.Copilot.CLI.linux-x64 -Source "https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json" -Version {{ copilot_version }} -OutputDirectory $(Agent.TempDirectory)/tools -ExcludeVersion -NonInteractive' - - - bash: | - ls -la "$(Agent.TempDirectory)/tools" - echo "##vso[task.prependpath]$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64" - - # Copy copilot binary to /tmp so it's accessible inside AWF container - mkdir -p /tmp/awf-tools - cp "$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64/copilot" /tmp/awf-tools/copilot - chmod +x /tmp/awf-tools/copilot - displayName: "Add copilot to PATH" - - - bash: | - copilot --version - copilot -h - displayName: "Output copilot version" + {{ engine_install_steps }} - bash: | COMPILER_VERSION="{{ compiler_version }}" diff --git a/src/engine.rs b/src/engine.rs index e3ed153..6bcf590 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1,11 +1,15 @@ use anyhow::Result; use crate::compile::extensions::{CompilerExtension, Extension}; -use crate::compile::types::{FrontMatter, McpConfig}; +use crate::compile::types::{EngineConfig, FrontMatter, McpConfig}; /// Default model used by the Copilot engine when no model is specified in front matter. pub const DEFAULT_COPILOT_MODEL: &str = "claude-opus-4.5"; +/// Default pinned version of the Copilot CLI NuGet package. +/// Override per-agent via `engine: { id: copilot, version: "1.0.35" }` in front matter. +pub const COPILOT_CLI_VERSION: &str = "1.0.34"; + /// Resolved engine — enum dispatch over supported engine identifiers. /// /// Currently only `Copilot` (GitHub Copilot CLI) is supported. New engines @@ -70,6 +74,17 @@ impl Engine { Engine::Copilot => "~/.copilot/logs", } } + + /// Generate pipeline YAML steps to install the engine binary. + /// + /// Uses `engine_config.version()` if set in front matter, otherwise falls back + /// to the pinned `COPILOT_CLI_VERSION` constant. Returns an empty string when + /// `engine.command` is set (the user provides their own binary). + pub fn install_steps(&self, engine_config: &EngineConfig) -> String { + match self { + Engine::Copilot => copilot_install_steps(engine_config), + } + } } fn copilot_args( @@ -233,13 +248,6 @@ fn copilot_args( front_matter.name ); } - if front_matter.engine.version().is_some() { - eprintln!( - "Warning: Agent '{}' has engine.version set, but custom engine versioning is not yet \ - wired into the pipeline and will be ignored.", - front_matter.name - ); - } if front_matter.engine.agent().is_some() { eprintln!( "Warning: Agent '{}' has engine.agent set, but custom agent file selection is not yet \ @@ -306,6 +314,50 @@ fn copilot_env() -> String { lines.join("\n") } +/// Generate Copilot CLI install steps for Azure DevOps pipelines. +/// +/// Produces the YAML block that authenticates with NuGet, installs the +/// `Microsoft.Copilot.CLI.linux-x64` package, copies the binary to +/// `/tmp/awf-tools/copilot`, and verifies the install. +fn copilot_install_steps(engine_config: &EngineConfig) -> String { + // Custom binary path → skip NuGet install entirely + if engine_config.command().is_some() { + return String::new(); + } + + let version = engine_config + .version() + .unwrap_or(COPILOT_CLI_VERSION); + + format!( + "\ +- task: NuGetAuthenticate@1 + displayName: \"Authenticate NuGet Feed\" + +- task: NuGetCommand@2 + displayName: \"Install Copilot CLI\" + inputs: + command: 'custom' + arguments: 'install Microsoft.Copilot.CLI.linux-x64 -Source \"https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json\" -Version {version} -OutputDirectory $(Agent.TempDirectory)/tools -ExcludeVersion -NonInteractive' + +- bash: | + ls -la \"$(Agent.TempDirectory)/tools\" + echo \"##vso[task.prependpath]$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64\" + + # Copy copilot binary to /tmp so it's accessible inside AWF container + # (AWF auto-mounts /tmp:/tmp:rw but not Agent.TempDirectory) + mkdir -p /tmp/awf-tools + cp \"$(Agent.TempDirectory)/tools/Microsoft.Copilot.CLI.linux-x64/copilot\" /tmp/awf-tools/copilot + chmod +x /tmp/awf-tools/copilot + displayName: \"Add copilot to PATH\" + +- bash: | + copilot --version + copilot -h + displayName: \"Output copilot version\"" + ) +} + #[cfg(test)] mod tests { use super::{get_engine, Engine}; From 4bd2a8799b2e048957497779ef1f2ff351ba2d3b Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 09:42:14 +0100 Subject: [PATCH 4/9] refactor: move engine invocation behind Engine::invocation() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add Engine::invocation() that generates the complete AWF command string including binary path, prompt delivery flag, and MCP config flag. The engine now controls how prompts are provided (e.g. --prompt "$(cat ...)") and how MCP config is referenced. Template changes: - Replace hardcoded AWF commands with {{ engine_run }} (Agent job) and {{ engine_run_detection }} (Detection job) - Remove {{ copilot_params }} marker (absorbed into invocation) Code changes: - Add copilot_invocation() helper in engine.rs - Replace copilot_params variable with engine_run/engine_run_detection - Rename 20 test functions: test_copilot_params_* → test_engine_args_* - Update compiler_tests.rs marker assertion Part of #287 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 58 +++++++++++++++++++++++++---------------- src/data/1es-base.yml | 4 +-- src/data/base.yml | 4 +-- src/engine.rs | 47 +++++++++++++++++++++++++++++++++ tests/compiler_tests.rs | 4 +-- 5 files changed, 88 insertions(+), 29 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 7dda129..2645faa 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1910,8 +1910,19 @@ pub async fn compile_shared( } } - // 4. Generate copilot params - let copilot_params = ctx.engine.args(ctx.front_matter, extensions)?; + // 4. Generate engine invocations and install steps + let engine_run = ctx.engine.invocation( + ctx.front_matter, + extensions, + "/tmp/awf-tools/agent-prompt.md", + Some("/tmp/awf-tools/mcp-config.json"), + )?; + let engine_run_detection = ctx.engine.invocation( + ctx.front_matter, + extensions, + "/tmp/awf-tools/threat-analysis-prompt.md", + None, + )?; let engine_install_steps = ctx.engine.install_steps(&front_matter.engine); // 5. Compute workspace, working directory, triggers @@ -2032,7 +2043,8 @@ pub async fn compile_shared( ("{{ agent }}", &agent_name), ("{{ agent_name }}", &front_matter.name), ("{{ agent_description }}", &front_matter.description), - ("{{ copilot_params }}", &copilot_params), + ("{{ engine_run }}", &engine_run), + ("{{ engine_run_detection }}", &engine_run_detection), ("{{ source_path }}", &source_path), // integrity_check must come before pipeline_path because the // integrity step content itself contains {{ pipeline_path }}. @@ -2157,7 +2169,7 @@ mod tests { // ─── Engine::args (copilot params) ────────────────────────────────────── #[test] - fn test_copilot_params_bash_wildcard() { + fn test_engine_args_bash_wildcard() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec![":*".to_string()]), @@ -2171,7 +2183,7 @@ mod tests { } #[test] - fn test_copilot_params_bash_star_wildcard() { + fn test_engine_args_bash_star_wildcard() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec!["*".to_string()]), @@ -2185,7 +2197,7 @@ mod tests { } #[test] - fn test_copilot_params_bash_disabled() { + fn test_engine_args_bash_disabled() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec![]), @@ -2198,7 +2210,7 @@ mod tests { } #[test] - fn test_copilot_params_allow_all_paths_when_edit_enabled() { + fn test_engine_args_allow_all_paths_when_edit_enabled() { let fm = minimal_front_matter(); // edit defaults to true, bash defaults to wildcard let params = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)).unwrap(); assert!(params.contains("--allow-all-paths"), "edit enabled (default) should emit --allow-all-paths"); @@ -2207,7 +2219,7 @@ mod tests { } #[test] - fn test_copilot_params_no_allow_all_paths_when_edit_disabled() { + fn test_engine_args_no_allow_all_paths_when_edit_disabled() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: None, @@ -2221,7 +2233,7 @@ mod tests { } #[test] - fn test_copilot_params_allow_all_tools_with_allow_all_paths() { + fn test_engine_args_allow_all_tools_with_allow_all_paths() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec![":*".to_string()]), @@ -2236,7 +2248,7 @@ mod tests { } #[test] - fn test_copilot_params_lean_adds_bash_commands() { + fn test_engine_args_lean_adds_bash_commands() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec!["cat".to_string()]), @@ -2256,7 +2268,7 @@ mod tests { } #[test] - fn test_copilot_params_lean_with_unrestricted_bash() { + fn test_engine_args_lean_with_unrestricted_bash() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec![":*".to_string()]), @@ -2274,7 +2286,7 @@ mod tests { } #[test] - fn test_copilot_params_custom_mcp_no_mcp_flag() { + fn test_engine_args_custom_mcp_no_mcp_flag() { let mut fm = minimal_front_matter(); fm.mcp_servers.insert( "my-tool".to_string(), @@ -2291,7 +2303,7 @@ mod tests { } #[test] - fn test_copilot_params_allow_tool_for_container_mcp() { + fn test_engine_args_allow_tool_for_container_mcp() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec!["cat".to_string()]), @@ -2311,7 +2323,7 @@ mod tests { } #[test] - fn test_copilot_params_allow_tool_for_url_mcp() { + fn test_engine_args_allow_tool_for_url_mcp() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec!["cat".to_string()]), @@ -2331,7 +2343,7 @@ mod tests { } #[test] - fn test_copilot_params_no_allow_tool_for_enabled_only_mcp() { + fn test_engine_args_no_allow_tool_for_enabled_only_mcp() { let mut fm = minimal_front_matter(); fm.mcp_servers.insert( "my-tool".to_string(), @@ -2342,7 +2354,7 @@ mod tests { } #[test] - fn test_copilot_params_allow_tool_mcps_sorted() { + fn test_engine_args_allow_tool_mcps_sorted() { let mut fm = minimal_front_matter(); fm.tools = Some(crate::compile::types::ToolsConfig { bash: Some(vec!["cat".to_string()]), @@ -2371,7 +2383,7 @@ mod tests { } #[test] - fn test_copilot_params_builtin_mcp_no_mcp_flag() { + fn test_engine_args_builtin_mcp_no_mcp_flag() { let mut fm = minimal_front_matter(); fm.mcp_servers .insert("ado".to_string(), McpConfig::Enabled(true)); @@ -2381,7 +2393,7 @@ mod tests { } #[test] - fn test_copilot_params_max_turns_ignored() { + fn test_engine_args_max_turns_ignored() { let (fm, _) = parse_markdown( "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n max-turns: 50\n---\n", ) @@ -2391,14 +2403,14 @@ mod tests { } #[test] - fn test_copilot_params_no_max_turns_when_simple_engine() { + fn test_engine_args_no_max_turns_when_simple_engine() { let fm = minimal_front_matter(); let params = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)).unwrap(); assert!(!params.contains("--max-turns")); } #[test] - fn test_copilot_params_no_max_timeout() { + fn test_engine_args_no_max_timeout() { let (fm, _) = parse_markdown( "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n timeout-minutes: 30\n---\n", ) @@ -2408,14 +2420,14 @@ mod tests { } #[test] - fn test_copilot_params_no_max_timeout_when_simple_engine() { + fn test_engine_args_no_max_timeout_when_simple_engine() { let fm = minimal_front_matter(); let params = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)).unwrap(); assert!(!params.contains("--max-timeout")); } #[test] - fn test_copilot_params_max_turns_zero_not_emitted() { + fn test_engine_args_max_turns_zero_not_emitted() { let (fm, _) = parse_markdown( "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n max-turns: 0\n---\n", ) @@ -2425,7 +2437,7 @@ mod tests { } #[test] - fn test_copilot_params_max_timeout_zero_not_emitted() { + fn test_engine_args_max_timeout_zero_not_emitted() { let (fm, _) = parse_markdown( "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n timeout-minutes: 0\n---\n", ) diff --git a/src/data/1es-base.yml b/src/data/1es-base.yml index 327f3c2..fc316aa 100644 --- a/src/data/1es-base.yml +++ b/src/data/1es-base.yml @@ -342,7 +342,7 @@ extends: --container-workdir "{{ working_directory }}" \ --log-level info \ --proxy-logs-dir "$(Agent.TempDirectory)/staging/logs/firewall" \ - -- '/tmp/awf-tools/copilot --prompt "$(cat /tmp/awf-tools/agent-prompt.md)" --additional-mcp-config @/tmp/awf-tools/mcp-config.json {{ copilot_params }}' \ + -- '{{ engine_run }}' \ 2>&1 \ | sed -u 's/##vso\[/[VSO-FILTERED] vso[/g; s/##\[/[VSO-FILTERED] [/g' \ | tee "$AGENT_OUTPUT_FILE" \ @@ -508,7 +508,7 @@ extends: --container-workdir "{{ working_directory }}" \ --log-level info \ --proxy-logs-dir "$(Agent.TempDirectory)/threat-analysis-logs/firewall" \ - -- '/tmp/awf-tools/copilot --prompt "$(cat /tmp/awf-tools/threat-analysis-prompt.md)" {{ copilot_params }}' \ + -- '{{ engine_run_detection }}' \ 2>&1 \ | sed -u 's/##vso\[/[VSO-FILTERED] vso[/g; s/##\[/[VSO-FILTERED] [/g' \ | tee "$THREAT_OUTPUT_FILE" \ diff --git a/src/data/base.yml b/src/data/base.yml index 386dcd7..4cf754c 100644 --- a/src/data/base.yml +++ b/src/data/base.yml @@ -313,7 +313,7 @@ jobs: --container-workdir "{{ working_directory }}" \ --log-level info \ --proxy-logs-dir "$(Agent.TempDirectory)/staging/logs/firewall" \ - -- '/tmp/awf-tools/copilot --prompt "$(cat /tmp/awf-tools/agent-prompt.md)" --additional-mcp-config @/tmp/awf-tools/mcp-config.json {{ copilot_params }}' \ + -- '{{ engine_run }}' \ 2>&1 \ | sed -u 's/##vso\[/[VSO-FILTERED] vso[/g; s/##\[/[VSO-FILTERED] [/g' \ | tee "$AGENT_OUTPUT_FILE" \ @@ -477,7 +477,7 @@ jobs: --container-workdir "{{ working_directory }}" \ --log-level info \ --proxy-logs-dir "$(Agent.TempDirectory)/threat-analysis-logs/firewall" \ - -- '/tmp/awf-tools/copilot --prompt "$(cat /tmp/awf-tools/threat-analysis-prompt.md)" {{ copilot_params }}' \ + -- '{{ engine_run_detection }}' \ 2>&1 \ | sed -u 's/##vso\[/[VSO-FILTERED] vso[/g; s/##\[/[VSO-FILTERED] [/g' \ | tee "$THREAT_OUTPUT_FILE" \ diff --git a/src/engine.rs b/src/engine.rs index 6bcf590..03c8b2f 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -85,6 +85,29 @@ impl Engine { Engine::Copilot => copilot_install_steps(engine_config), } } + + /// Generate the full AWF `--` command string for running the engine. + /// + /// Returns the content for the AWF `-- ''` argument, including the + /// binary path, prompt delivery flag, MCP config flag, and all CLI arguments. + /// The engine controls how the prompt is provided (e.g., `--prompt "$(cat ...)"` + /// for Copilot) and how MCP config is referenced. + /// + /// `prompt_path` is the path to the prompt file inside the AWF container. + /// `mcp_config_path` is optionally the path to the MCP config file + /// (Some for Agent job, None for Detection job which has no MCP). + pub fn invocation( + &self, + front_matter: &FrontMatter, + extensions: &[Extension], + prompt_path: &str, + mcp_config_path: Option<&str>, + ) -> Result { + let args = self.args(front_matter, extensions)?; + match self { + Engine::Copilot => Ok(copilot_invocation(prompt_path, mcp_config_path, &args)), + } + } } fn copilot_args( @@ -358,6 +381,30 @@ fn copilot_install_steps(engine_config: &EngineConfig) -> String { ) } +/// Build the full AWF `--` command string for the Copilot CLI. +/// +/// The returned string goes inside `-- '...'` in the pipeline YAML. +fn copilot_invocation( + prompt_path: &str, + mcp_config_path: Option<&str>, + args: &str, +) -> String { + let mut parts = vec![ + "/tmp/awf-tools/copilot".to_string(), + format!("--prompt \"$(cat {prompt_path})\""), + ]; + + if let Some(mcp_path) = mcp_config_path { + parts.push(format!("--additional-mcp-config @{mcp_path}")); + } + + if !args.is_empty() { + parts.push(args.to_string()); + } + + parts.join(" ") +} + #[cfg(test)] mod tests { use super::{get_engine, Engine}; diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index e1120cc..c0d74a9 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -105,8 +105,8 @@ fn test_compiled_yaml_structure() { "Template should contain agent_name marker" ); assert!( - template_content.contains("{{ copilot_params }}"), - "Template should contain copilot_params marker" + template_content.contains("{{ engine_run }}"), + "Template should contain engine_run marker" ); assert!( template_content.contains("{{ compiler_version }}"), From 7f3320ad098448bc42c40497ddb965282f10d25b Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 09:44:33 +0100 Subject: [PATCH 5/9] docs: update AGENTS.md, test docs, and version updater for Engine refactor - AGENTS.md: document new markers (engine_install_steps, engine_run, engine_run_detection, engine_env, engine_log_dir), mark copilot_version as removed, remove duplicate engine_env section - Version updater: point COPILOT_CLI_VERSION at src/engine.rs - Test docs: rename copilot_params references to engine_args/engine_run - compiler_tests.rs: update assertion messages Part of #287 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/update-awf-version.md | 6 +-- AGENTS.md | 58 +++++++++++++++++-------- tests/EXAMPLES.md | 4 +- tests/README.md | 4 +- tests/SUMMARY.md | 6 +-- tests/VERIFICATION.md | 2 +- tests/compiler_tests.rs | 2 +- 7 files changed, 51 insertions(+), 31 deletions(-) diff --git a/.github/workflows/update-awf-version.md b/.github/workflows/update-awf-version.md index 6a68205..335160d 100644 --- a/.github/workflows/update-awf-version.md +++ b/.github/workflows/update-awf-version.md @@ -29,7 +29,7 @@ There are four items to check: | Item | Upstream Source | Local Path | |------|---------------|------------| | `AWF_VERSION` | [github/gh-aw-firewall](https://github.com/github/gh-aw-firewall) latest release | `src/compile/common.rs` | -| `COPILOT_CLI_VERSION` | [github/copilot-cli](https://github.com/github/copilot-cli) latest release | `src/compile/common.rs` | +| `COPILOT_CLI_VERSION` | [github/copilot-cli](https://github.com/github/copilot-cli) latest release | `src/engine.rs` | | `MCPG_VERSION` | [github/gh-aw-mcpg](https://github.com/github/gh-aw-mcpg) latest release | `src/compile/common.rs` | | `ecosystem_domains.json` | [github/gh-aw](https://github.com/github/gh-aw) `pkg/workflow/data/ecosystem_domains.json` on `main` | `src/data/ecosystem_domains.json` | @@ -45,7 +45,7 @@ Fetch the latest release of the upstream repository. Record the tag name, stripp ### Step 2: Read the Current Version -Read the file `src/compile/common.rs` in this repository and find the corresponding constant: +Read the file `src/compile/common.rs` (for `AWF_VERSION`, `MCPG_VERSION`) or `src/engine.rs` (for `COPILOT_CLI_VERSION`) in this repository and find the corresponding constant: - `pub const AWF_VERSION: &str = "...";` - `pub const COPILOT_CLI_VERSION: &str = "...";` @@ -89,7 +89,7 @@ If the latest version is newer than the current constant: ```markdown ## Dependency Update - Updates the pinned `COPILOT_CLI_VERSION` constant in `src/compile/common.rs` from `` to ``. + Updates the pinned `COPILOT_CLI_VERSION` constant in `src/engine.rs` from `` to ``. ### Release diff --git a/AGENTS.md b/AGENTS.md index 7150f5a..1e3aa89 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -562,7 +562,7 @@ The compiler transforms the input into valid Azure DevOps pipeline YAML based on - **Standalone**: Uses `src/data/base.yml` - **1ES**: Uses `src/data/1es-base.yml` -Explicit markings are embedded in these templates that the compiler is allowed to replace e.g. `{{ copilot_params }}` denotes parameters which are passed to the copilot command line tool. The compiler should not replace sections denoted by `${{ some content }}`. What follows is a mapping of markings to responsibilities (primarily for the standalone template). +Explicit markings are embedded in these templates that the compiler is allowed to replace e.g. `{{ engine_run }}` denotes the full engine invocation command. The compiler should not replace sections denoted by `${{ some content }}`. What follows is a mapping of markings to responsibilities (primarily for the standalone template). ## {{ parameters }} @@ -650,9 +650,26 @@ This distinction allows resources (like templates) to be available as pipeline r Should be replaced with the human-readable name from the front matter (e.g., "Daily Code Review"). This is used for display purposes like stage names. -## {{ copilot_params }} +## {{ engine_install_steps }} -Additional params provided to copilot CLI. The compiler generates: +Should be replaced with engine-specific pipeline steps to install the engine binary. Generated by `Engine::install_steps()`. For Copilot, this produces: +- `NuGetAuthenticate@1` task +- `NuGetCommand@2` task to install `Microsoft.Copilot.CLI.linux-x64` (uses `engine.version` if set, otherwise `COPILOT_CLI_VERSION` constant) +- Bash step to copy binary to `/tmp/awf-tools/copilot` +- Bash step to verify installation + +Returns empty when `engine.command` is set (user provides own binary). + +## {{ engine_run }} + +Should be replaced with the full AWF `--` command string for the Agent job. Generated by `Engine::invocation()`. For Copilot, this produces: +``` +/tmp/awf-tools/copilot --prompt "$(cat /tmp/awf-tools/agent-prompt.md)" --additional-mcp-config @/tmp/awf-tools/mcp-config.json +``` + +The engine controls the binary path, how the prompt is delivered (`--prompt "$(cat ...)"`), and how MCP config is referenced (`--additional-mcp-config @...`). + +Engine args include: - `--model ` - AI model from `engine` front matter field (default: claude-opus-4.5) - `--no-ask-user` - Prevents interactive prompts - `--disable-builtin-mcps` - Disables all built-in Copilot CLI MCPs (single flag, no argument) @@ -662,6 +679,24 @@ Additional params provided to copilot CLI. The compiler generates: MCP servers are handled entirely by the MCP Gateway (MCPG) and are not passed as copilot CLI params. +## {{ engine_run_detection }} + +Same as `{{ engine_run }}` but for the Detection (threat analysis) job. Uses a different prompt path (`/tmp/awf-tools/threat-analysis-prompt.md`) and no MCP config. + +## {{ engine_env }} + +Generates engine-specific environment variable entries for the AWF sandbox step via `Engine::env()`. For the Copilot engine, this produces: + +- `GITHUB_TOKEN: $(GITHUB_TOKEN)` — GitHub authentication +- `GITHUB_READ_ONLY: 1` — Restricts GitHub API to read-only access +- `COPILOT_OTEL_ENABLED`, `COPILOT_OTEL_EXPORTER_TYPE`, `COPILOT_OTEL_FILE_EXPORTER_PATH` — OpenTelemetry file-based tracing for agent statistics + +ADO access tokens (`AZURE_DEVOPS_EXT_PAT`, `SYSTEM_ACCESSTOKEN`) are not part of this marker — they are injected separately by `{{ acquire_ado_token }}` and extension pipeline variable mappings when `permissions.read` is configured. + +## {{ engine_log_dir }} + +Should be replaced with the engine's log directory path, generated by `Engine::log_dir()`. For Copilot: `~/.copilot/logs`. Used by log collection steps to copy engine logs to pipeline artifacts. + ## {{ pool }} Should be replaced with the agent pool name from the `pool` front matter field. Defaults to `AZS-1ES-L-MMS-ubuntu-22.04` if not specified. @@ -884,16 +919,6 @@ The step: If `permissions.read` is not configured, this marker is replaced with an empty string. -## {{ engine_env }} - -Generates engine-specific environment variable entries for the AWF sandbox step via `Engine::env()`. For the Copilot engine, this produces: - -- `GITHUB_TOKEN: $(GITHUB_TOKEN)` — GitHub authentication -- `GITHUB_READ_ONLY: 1` — Restricts GitHub API to read-only access -- `COPILOT_OTEL_ENABLED`, `COPILOT_OTEL_EXPORTER_TYPE`, `COPILOT_OTEL_FILE_EXPORTER_PATH` — OpenTelemetry file-based tracing for agent statistics - -ADO access tokens (`AZURE_DEVOPS_EXT_PAT`, `SYSTEM_ACCESSTOKEN`) are not part of this marker — they are injected separately by `{{ acquire_ado_token }}` and extension pipeline variable mappings when `permissions.read` is configured. - ## {{ acquire_write_token }} Generates an `AzureCLI@2` step that acquires a write-capable ADO-scoped access token from the ARM service connection specified in `permissions.write`. This token is used only by the executor in Stage 3 (`Execution` job) and is never exposed to the agent. @@ -951,12 +976,7 @@ Should be replaced with the domain the AWF-sandboxed agent uses to reach MCPG on ## {{ copilot_version }} -Should be replaced with the pinned version of the `Microsoft.Copilot.CLI.linux-x64` NuGet package (defined as `COPILOT_CLI_VERSION` constant in `src/compile/common.rs`). This version is used in the pipeline step that installs the Copilot CLI tool from Azure Artifacts. - -The generated pipelines install the package from: -``` -https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json -``` +**Removed.** This marker has been absorbed into `{{ engine_install_steps }}`. The `COPILOT_CLI_VERSION` constant now lives in `src/engine.rs` and is used internally by `Engine::install_steps()`. The version can be overridden per-agent via `engine: { id: copilot, version: "..." }` in front matter. ### 1ES-Specific Template Markers diff --git a/tests/EXAMPLES.md b/tests/EXAMPLES.md index 8fb0e03..459ef51 100644 --- a/tests/EXAMPLES.md +++ b/tests/EXAMPLES.md @@ -78,7 +78,7 @@ fn test_with_hashmap() { mcps.insert("ado".to_string(), McpConfig::Enabled(true)); mcps.insert("es-chat".to_string(), McpConfig::Enabled(true)); - let result = generate_copilot_params(&mcps); + let result = generate_engine_args(&mcps); assert!(result.contains("--prompt")); // MCPs are handled via the MCP firewall, not --mcp flags @@ -103,7 +103,7 @@ fn test_with_options() { }), ); - let result = generate_copilot_params(&mcps); + let result = generate_engine_args(&mcps); assert!(!result.contains("--mcp custom-tool")); } diff --git a/tests/README.md b/tests/README.md index ac53fc6..60517fa 100644 --- a/tests/README.md +++ b/tests/README.md @@ -68,7 +68,7 @@ Tests checkout step generation for: - Empty repository list - Multiple repositories -### `test_generate_copilot_params_*` (3 tests) +### `test_generate_engine_args_*` (3 tests) Tests copilot parameter generation for: - Built-in MCPs (enabled) - Built-in MCPs (disabled) @@ -101,7 +101,7 @@ Verifies that the base template contains all required markers: - `{{ checkout_repositories }}` - `{{ agent }}` - `{{ agent_name }}` -- `{{ copilot_params }}` +- `{{ engine_run }}` ### `test_example_file_structure` Validates the example file (`examples/sample-agent.md`) to ensure: diff --git a/tests/SUMMARY.md b/tests/SUMMARY.md index 13e775b..43a8c60 100644 --- a/tests/SUMMARY.md +++ b/tests/SUMMARY.md @@ -28,9 +28,9 @@ Added 18 comprehensive unit tests in the `tests` module at the end of `main.rs`: - `test_generate_checkout_steps_multiple` - Tests multiple checkout steps #### Copilot Parameters (3 tests) -- `test_copilot_params_custom_mcp_no_mcp_flag` - Verifies custom MCPs don't generate --mcp flags -- `test_copilot_params_builtin_mcp_no_mcp_flag` - Verifies built-in MCPs don't generate --mcp flags (all MCPs handled via firewall) -- `test_generate_copilot_params_custom_mcp_skipped` - Verifies custom MCPs are skipped +- `test_engine_args_custom_mcp_no_mcp_flag` - Verifies custom MCPs don't generate --mcp flags +- `test_engine_args_builtin_mcp_no_mcp_flag` - Verifies built-in MCPs don't generate --mcp flags (all MCPs handled via firewall) +- `test_generate_engine_args_custom_mcp_skipped` - Verifies custom MCPs are skipped #### Markdown Parsing (4 tests) - `test_parse_markdown_valid` - Tests valid markdown with front matter diff --git a/tests/VERIFICATION.md b/tests/VERIFICATION.md index 6b8473b..ac0eb5e 100644 --- a/tests/VERIFICATION.md +++ b/tests/VERIFICATION.md @@ -136,7 +136,7 @@ fn test_compile_pipeline_basic() { - `test_generate_schedule_*` - verifies schedule generation - `test_generate_repositories_*` - verifies repository YAML generation - `test_generate_checkout_steps_*` - verifies checkout step generation - - `test_generate_copilot_params_*` - verifies parameter generation + - `test_generate_engine_args_*` - verifies parameter generation - `test_parse_markdown_*` - verifies markdown parsing including error cases - Template and structure validation tests diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index c0d74a9..45fe0f7 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -3039,7 +3039,7 @@ fn test_1es_compiled_output_is_valid_yaml() { ); assert!( compiled.contains("copilot --prompt"), - "1ES output should contain copilot invocation (copilot_params substituted)" + "1ES output should contain copilot invocation (engine_run substituted)" ); assert!( compiled.contains("threat-analysis"), From b23cf2b3af97cf580eec6b4e663426cc2a07615f Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 10:09:34 +0100 Subject: [PATCH 6/9] remove max-turns config... specific to claude code --- AGENTS.md | 2 -- src/compile/common.rs | 33 +++------------------------------ src/compile/types.rs | 25 ++++--------------------- src/engine.rs | 7 ------- 4 files changed, 7 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1e3aa89..2d81e09 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -353,8 +353,6 @@ engine: | `env` | map | *(none)* | Engine-specific environment variables. **Not yet wired** — parsed but ignored with a warning. | | `command` | string | *(none)* | Custom engine executable path (skips default installation). **Not yet wired** — parsed but ignored with a warning. | -> **Deprecated:** `max-turns` is still accepted in front matter for backwards compatibility but is ignored at compile time (a warning is emitted). It was specific to Claude Code and is not supported by Copilot CLI. - > **Note:** Fields marked "not yet wired" are accepted in the schema for forward compatibility with gh-aw but produce a compile-time warning. Pipeline wiring for these fields is incremental. #### `timeout-minutes` diff --git a/src/compile/common.rs b/src/compile/common.rs index 2645faa..4f088b0 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2392,23 +2392,6 @@ mod tests { assert!(!params.contains("--mcp ado")); } - #[test] - fn test_engine_args_max_turns_ignored() { - let (fm, _) = parse_markdown( - "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n max-turns: 50\n---\n", - ) - .unwrap(); - let params = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)).unwrap(); - assert!(!params.contains("--max-turns"), "max-turns should not be emitted as a CLI arg"); - } - - #[test] - fn test_engine_args_no_max_turns_when_simple_engine() { - let fm = minimal_front_matter(); - let params = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)).unwrap(); - assert!(!params.contains("--max-turns")); - } - #[test] fn test_engine_args_no_max_timeout() { let (fm, _) = parse_markdown( @@ -2426,16 +2409,6 @@ mod tests { assert!(!params.contains("--max-timeout")); } - #[test] - fn test_engine_args_max_turns_zero_not_emitted() { - let (fm, _) = parse_markdown( - "---\nname: test\ndescription: test\nengine:\n model: claude-opus-4.5\n max-turns: 0\n---\n", - ) - .unwrap(); - let params = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)).unwrap(); - assert!(!params.contains("--max-turns"), "max-turns should not be emitted as a CLI arg"); - } - #[test] fn test_engine_args_max_timeout_zero_not_emitted() { let (fm, _) = parse_markdown( @@ -3392,7 +3365,7 @@ mod tests { model: Some("model' && echo pwned".to_string()), version: None, agent: None, api_target: None, args: vec![], env: None, command: None, - max_turns: None, timeout_minutes: None, + timeout_minutes: None, }); let result = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)); assert!(result.is_err()); @@ -3407,7 +3380,7 @@ mod tests { model: Some("model && curl evil.com".to_string()), version: None, agent: None, api_target: None, args: vec![], env: None, command: None, - max_turns: None, timeout_minutes: None, + timeout_minutes: None, }); let result = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)); assert!(result.is_err()); @@ -3422,7 +3395,7 @@ mod tests { model: Some(name.to_string()), version: None, agent: None, api_target: None, args: vec![], env: None, command: None, - max_turns: None, timeout_minutes: None, + timeout_minutes: None, }); let result = CompileContext::for_test(&fm).engine.args(&fm, &crate::compile::extensions::collect_extensions(&fm)); assert!(result.is_ok(), "Model name '{}' should be valid", name); diff --git a/src/compile/types.rs b/src/compile/types.rs index da679ca..5861199 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -199,14 +199,6 @@ impl EngineConfig { } } - /// Get the max turns setting (deprecated — ignored at compile time) - pub fn max_turns(&self) -> Option { - match self { - EngineConfig::Simple(_) => None, - EngineConfig::Full(opts) => opts.max_turns, - } - } - /// Get the timeout in minutes pub fn timeout_minutes(&self) -> Option { match self { @@ -299,9 +291,6 @@ pub struct EngineOptions { /// Custom engine executable path (skips default installation) #[serde(default)] pub command: Option, - /// Maximum number of chat iterations per run (deprecated — not supported by Copilot CLI) - #[serde(default, rename = "max-turns")] - pub max_turns: Option, /// Workflow timeout in minutes #[serde(default, rename = "timeout-minutes")] pub timeout_minutes: Option, @@ -936,32 +925,29 @@ mod tests { let ec = EngineConfig::Simple("copilot".to_string()); assert_eq!(ec.engine_id(), "copilot"); assert_eq!(ec.model(), None); - assert_eq!(ec.max_turns(), None); assert_eq!(ec.timeout_minutes(), None); } #[test] fn test_engine_config_full_object() { - let yaml = "id: copilot\nmodel: claude-sonnet-4.5\nmax-turns: 50\ntimeout-minutes: 30"; + let yaml = "id: copilot\nmodel: claude-sonnet-4.5\ntimeout-minutes: 30"; let opts: EngineOptions = serde_yaml::from_str(yaml).unwrap(); let ec = EngineConfig::Full(opts); assert_eq!(ec.engine_id(), "copilot"); assert_eq!(ec.model(), Some("claude-sonnet-4.5")); - assert_eq!(ec.max_turns(), Some(50)); assert_eq!(ec.timeout_minutes(), Some(30)); } #[test] fn test_engine_config_full_object_partial_fields() { - let yaml = "max-turns: 10"; + let yaml = "timeout-minutes: 10"; let opts: EngineOptions = serde_yaml::from_str(yaml).unwrap(); let ec = EngineConfig::Full(opts); // id defaults to "copilot" when not specified assert_eq!(ec.engine_id(), "copilot"); // model is None when not specified (engine impl decides default) assert_eq!(ec.model(), None); - assert_eq!(ec.max_turns(), Some(10)); - assert_eq!(ec.timeout_minutes(), None); + assert_eq!(ec.timeout_minutes(), Some(10)); } #[test] @@ -969,7 +955,6 @@ mod tests { let ec = EngineConfig::default(); assert_eq!(ec.engine_id(), "copilot"); assert_eq!(ec.model(), None); - assert_eq!(ec.max_turns(), None); assert_eq!(ec.timeout_minutes(), None); } @@ -980,19 +965,17 @@ mod tests { let ec: EngineConfig = serde_yaml::from_value(fm["engine"].clone()).unwrap(); assert_eq!(ec.engine_id(), "copilot"); assert_eq!(ec.model(), None); - assert_eq!(ec.max_turns(), None); assert_eq!(ec.timeout_minutes(), None); } #[test] fn test_engine_config_deserialized_as_object() { let yaml = - "engine:\n id: copilot\n model: claude-opus-4.5\n max-turns: 50\n timeout-minutes: 30"; + "engine:\n id: copilot\n model: claude-opus-4.5\n timeout-minutes: 30"; let fm: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); let ec: EngineConfig = serde_yaml::from_value(fm["engine"].clone()).unwrap(); assert_eq!(ec.engine_id(), "copilot"); assert_eq!(ec.model(), Some("claude-opus-4.5")); - assert_eq!(ec.max_turns(), Some(50)); assert_eq!(ec.timeout_minutes(), Some(30)); } diff --git a/src/engine.rs b/src/engine.rs index 03c8b2f..e1a88bc 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -243,13 +243,6 @@ fn copilot_args( ); } params.push(format!("--model {}", model)); - if front_matter.engine.max_turns().is_some() { - eprintln!( - "Warning: Agent '{}' has max-turns set, but max-turns is not supported by Copilot CLI \ - and will be ignored. Consider removing it from the engine configuration.", - front_matter.name - ); - } if let Some(timeout_minutes) = front_matter.engine.timeout_minutes() { if timeout_minutes == 0 { eprintln!( From ab6f2db1a06746d72e9b5c026acfb451ce9fcac2 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 10:25:12 +0100 Subject: [PATCH 7/9] feat: wire engine.command, agent, api-target, args, and env into pipeline Wire the five remaining engine front matter fields that were previously parsed but emitted 'not yet wired' warnings: - engine.command: overrides the engine binary path in the AWF invocation; skips NuGet install (already handled). Validated for shell-safe chars. - engine.agent: adds --agent to the Copilot CLI invocation for custom agent file selection from .github/agents/. - engine.api-target: adds --api-target for GHES/GHEC endpoints and adds the hostname to the AWF network allowlist. - engine.args: appends user-provided CLI arguments after compiler-generated args. Subject to strict char validation and blocked from overriding compiler-controlled flags (--prompt, --allow-tool, --disable-builtin-mcps, --no-ask-user, --ask-user, --additional-mcp-config, --allow-all-tools, --allow-all-paths). - engine.env: merges user environment variables into the sandbox step env block. Validates key names, blocks compiler-controlled keys (GITHUB_TOKEN, PATH, BASH_ENV, LD_PRELOAD, etc.), and rejects ADO expression/command injection in values. All five warnings are removed. AGENTS.md updated to reflect wired status. 33 new tests added covering happy paths, validation, and security edge cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 26 +- src/compile/common.rs | 11 +- src/compile/standalone.rs | 13 + src/engine.rs | 535 ++++++++++++++++++++++++++++++++++---- 4 files changed, 516 insertions(+), 69 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2d81e09..b2f1479 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -346,14 +346,13 @@ engine: | `id` | string | `copilot` | Engine identifier. Currently only `copilot` (GitHub Copilot CLI) is supported. | | `model` | string | `claude-opus-4.5` | AI model to use. Options include `claude-sonnet-4.5`, `gpt-5.2-codex`, `gemini-3-pro-preview`, etc. | | `timeout-minutes` | integer | *(none)* | Maximum time in minutes the agent job is allowed to run. Sets `timeoutInMinutes` on the `Agent` job in the generated pipeline. | -| `version` | string | *(none)* | Engine CLI version to install (e.g., `"0.0.422"`, `"latest"`). **Not yet wired** — parsed but ignored with a warning. | -| `agent` | string | *(none)* | Custom agent file identifier (Copilot only). **Not yet wired** — parsed but ignored with a warning. | -| `api-target` | string | *(none)* | Custom API endpoint hostname for GHES/GHEC (e.g., `"api.acme.ghe.com"`). **Not yet wired** — parsed but ignored with a warning. | -| `args` | list | `[]` | Custom CLI arguments injected before the prompt. **Not yet wired** — parsed but ignored with a warning. | -| `env` | map | *(none)* | Engine-specific environment variables. **Not yet wired** — parsed but ignored with a warning. | -| `command` | string | *(none)* | Custom engine executable path (skips default installation). **Not yet wired** — parsed but ignored with a warning. | +| `version` | string | *(none)* | Engine CLI version to install (e.g., `"0.0.422"`, `"latest"`). Overrides the pinned `COPILOT_CLI_VERSION`. Set to `"latest"` to use the newest available version. | +| `agent` | string | *(none)* | Custom agent file identifier (Copilot only). Adds `--agent ` to the CLI invocation, selecting a custom agent from `.github/agents/`. | +| `api-target` | string | *(none)* | Custom API endpoint hostname for GHES/GHEC (e.g., `"api.acme.ghe.com"`). Adds `--api-target ` to the CLI invocation and adds the hostname to the AWF network allowlist. | +| `args` | list | `[]` | Custom CLI arguments appended after compiler-generated args. Subject to shell-safety validation and blocked from overriding compiler-controlled flags (`--prompt`, `--allow-tool`, `--disable-builtin-mcps`, etc.). | +| `env` | map | *(none)* | Engine-specific environment variables merged into the sandbox step's `env:` block. Keys must be valid env var names; values must not contain ADO expressions (`$(`, `${{`) or pipeline command injection (`##vso[`). Compiler-controlled keys (`GITHUB_TOKEN`, `PATH`, `BASH_ENV`, etc.) are blocked. | +| `command` | string | *(none)* | Custom engine executable path (skips default NuGet installation). The path must be accessible inside the AWF container (e.g., `/tmp/...` or workspace-mounted paths). | -> **Note:** Fields marked "not yet wired" are accepted in the schema for forward compatibility with gh-aw but produce a compile-time warning. Pipeline wiring for these fields is incremental. #### `timeout-minutes` @@ -662,18 +661,21 @@ Returns empty when `engine.command` is set (user provides own binary). Should be replaced with the full AWF `--` command string for the Agent job. Generated by `Engine::invocation()`. For Copilot, this produces: ``` -/tmp/awf-tools/copilot --prompt "$(cat /tmp/awf-tools/agent-prompt.md)" --additional-mcp-config @/tmp/awf-tools/mcp-config.json + --prompt "$(cat /tmp/awf-tools/agent-prompt.md)" --additional-mcp-config @/tmp/awf-tools/mcp-config.json ``` -The engine controls the binary path, how the prompt is delivered (`--prompt "$(cat ...)"`), and how MCP config is referenced (`--additional-mcp-config @...`). +The binary path defaults to `/tmp/awf-tools/copilot` but can be overridden via `engine.command`. The engine controls how the prompt is delivered (`--prompt "$(cat ...)"`), and how MCP config is referenced (`--additional-mcp-config @...`). Engine args include: - `--model ` - AI model from `engine` front matter field (default: claude-opus-4.5) +- `--agent ` - Custom agent file from `engine.agent` (selects from `.github/agents/`) +- `--api-target ` - Custom API endpoint from `engine.api-target` (GHES/GHEC) - `--no-ask-user` - Prevents interactive prompts - `--disable-builtin-mcps` - Disables all built-in Copilot CLI MCPs (single flag, no argument) - `--allow-all-tools` - When bash is omitted (default) or has a wildcard (`":*"` or `"*"`), allows all tools instead of individual `--allow-tool` flags - `--allow-tool ` - When bash is NOT wildcard, explicitly allows configured tools (github, safeoutputs, write, and shell commands from the `bash:` field plus any runtime-required commands) - `--allow-all-paths` - When `edit` tool is enabled (default), allows the agent to write to any file path +- Custom args from `engine.args` — appended after compiler-generated args (subject to shell-safety validation and blocked flag checks) MCP servers are handled entirely by the MCP Gateway (MCPG) and are not passed as copilot CLI params. @@ -688,6 +690,7 @@ Generates engine-specific environment variable entries for the AWF sandbox step - `GITHUB_TOKEN: $(GITHUB_TOKEN)` — GitHub authentication - `GITHUB_READ_ONLY: 1` — Restricts GitHub API to read-only access - `COPILOT_OTEL_ENABLED`, `COPILOT_OTEL_EXPORTER_TYPE`, `COPILOT_OTEL_FILE_EXPORTER_PATH` — OpenTelemetry file-based tracing for agent statistics +- Custom env vars from `engine.env` — merged after compiler-controlled vars (YAML-quoted, validated for safety) ADO access tokens (`AZURE_DEVOPS_EXT_PAT`, `SYSTEM_ACCESSTOKEN`) are not part of this marker — they are injected separately by `{{ acquire_ado_token }}` and extension pipeline variable mappings when `permissions.read` is configured. @@ -878,8 +881,9 @@ This ensures the Copilot CLI config reflects MCPG's actual runtime state rather Should be replaced with the comma-separated domain list for AWF's `--allow-domains` flag. The list includes: 1. Core Azure DevOps/GitHub endpoints (from `allowed_hosts.rs`) 2. MCP-specific endpoints for each enabled MCP -3. Ecosystem identifier expansions from `network.allowed:` (e.g., `python` → PyPI/pip domains) -4. User-specified additional hosts from `network.allowed:` front matter +3. Engine-required hosts (e.g., `engine.api-target` hostname for GHES/GHEC) +4. Ecosystem identifier expansions from `network.allowed:` (e.g., `python` → PyPI/pip domains) +5. User-specified additional hosts from `network.allowed:` front matter The output is formatted as a comma-separated string (e.g., `github.com,*.dev.azure.com,api.github.com`). diff --git a/src/compile/common.rs b/src/compile/common.rs index 4f088b0..ca92a31 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1782,6 +1782,13 @@ pub fn generate_allowed_domains( } } + // Add engine-required hosts (e.g., GHES/GHEC api-target hostname). + // The engine resolves its config and returns additional hosts that AWF must allow. + let engine = crate::engine::get_engine(front_matter.engine.engine_id())?; + for host in engine.required_hosts(&front_matter.engine) { + hosts.insert(host); + } + // Add user-specified hosts (validated against DNS-safe characters) // Entries may be ecosystem identifiers (e.g., "python", "rust") which // expand to their domain lists, or raw domain names. @@ -1971,7 +1978,7 @@ pub async fn compile_shared( .and_then(|p| p.read.as_deref()), "SC_READ_TOKEN", ); - let engine_env = ctx.engine.env(); + let engine_env = ctx.engine.env(&front_matter.engine)?; let engine_log_dir = ctx.engine.log_dir(); let acquire_write_token = generate_acquire_ado_token( front_matter @@ -3322,7 +3329,7 @@ mod tests { fn test_engine_env() { let fm = minimal_front_matter(); let ctx = CompileContext::for_test(&fm); - let result = ctx.engine.env(); + let result = ctx.engine.env(&fm.engine).unwrap(); assert!( result.contains("GITHUB_TOKEN: $(GITHUB_TOKEN)"), "Should include GITHUB_TOKEN" diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 2e4c5e8..bc941fd 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -256,4 +256,17 @@ mod tests { assert!(domains.contains("crates.io"), "rust domains present"); } + #[test] + fn test_generate_allowed_domains_api_target_included() { + let (mut fm, _) = parse_markdown( + "---\nname: test-agent\ndescription: test\nengine:\n id: copilot\n api-target: api.acme.ghe.com\n---\n", + ).unwrap(); + fm.network = None; + let exts = super::super::extensions::collect_extensions(&fm); + let domains = generate_allowed_domains(&fm, &exts).unwrap(); + assert!( + domains.contains("api.acme.ghe.com"), + "api-target hostname must be in the allowlist" + ); + } } diff --git a/src/engine.rs b/src/engine.rs index e1a88bc..0388f1b 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -3,6 +3,65 @@ use anyhow::Result; use crate::compile::extensions::{CompilerExtension, Extension}; use crate::compile::types::{EngineConfig, FrontMatter, McpConfig}; +/// Characters allowed in engine.command paths (absolute path chars only). +/// Prevents shell injection when the path is embedded in AWF single-quoted commands. +fn is_valid_command_path(s: &str) -> bool { + !s.is_empty() + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '/' | '-')) +} + +/// Characters allowed in engine.agent and engine.model identifiers. +fn is_valid_identifier(s: &str) -> bool { + !s.is_empty() + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | ':' | '-')) +} + +/// Characters allowed in engine.api-target hostnames. +fn is_valid_hostname(s: &str) -> bool { + !s.is_empty() + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-')) +} + +/// Characters allowed in individual engine.args entries. +/// Strict allowlist to prevent shell injection inside AWF single-quoted commands. +fn is_valid_arg(s: &str) -> bool { + !s.is_empty() + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | ':' | '-' | '=' | '/' | '@')) +} + +/// Flags that the compiler controls — user args must not attempt to override these. +const BLOCKED_ARG_PREFIXES: &[&str] = &[ + "--prompt", + "--additional-mcp-config", + "--allow-tool", + "--allow-all-tools", + "--allow-all-paths", + "--disable-builtin-mcps", + "--no-ask-user", + "--ask-user", +]; + +/// Environment variable keys that the compiler controls — users must not override these. +const BLOCKED_ENV_KEYS: &[&str] = &[ + "GITHUB_TOKEN", + "GITHUB_READ_ONLY", + "COPILOT_OTEL_ENABLED", + "COPILOT_OTEL_EXPORTER_TYPE", + "COPILOT_OTEL_FILE_EXPORTER_PATH", + // Shell/system vars that could affect AWF or pipeline behavior + "PATH", + "HOME", + "BASH_ENV", + "ENV", + "IFS", + "LD_PRELOAD", + "LD_LIBRARY_PATH", +]; + /// Default model used by the Copilot engine when no model is specified in front matter. pub const DEFAULT_COPILOT_MODEL: &str = "claude-opus-4.5"; @@ -60,9 +119,9 @@ impl Engine { } /// Generate the env block entries for the engine's sandbox step. - pub fn env(&self) -> String { + pub fn env(&self, engine_config: &EngineConfig) -> Result { match self { - Engine::Copilot => copilot_env(), + Engine::Copilot => copilot_env(engine_config), } } @@ -75,6 +134,22 @@ impl Engine { } } + /// Return additional hosts the engine needs based on its configuration. + /// + /// Used by the domain allowlist generator to ensure engine-specific endpoints + /// (e.g., GHES/GHEC API targets) are reachable through AWF. + pub fn required_hosts(&self, engine_config: &EngineConfig) -> Vec { + match self { + Engine::Copilot => { + let mut hosts = Vec::new(); + if let Some(api_target) = engine_config.api_target() { + hosts.push(api_target.to_string()); + } + hosts + } + } + } + /// Generate pipeline YAML steps to install the engine binary. /// /// Uses `engine_config.version()` if set in front matter, otherwise falls back @@ -105,7 +180,22 @@ impl Engine { ) -> Result { let args = self.args(front_matter, extensions)?; match self { - Engine::Copilot => Ok(copilot_invocation(prompt_path, mcp_config_path, &args)), + Engine::Copilot => { + let command_path = match front_matter.engine.command() { + Some(cmd) => { + if !is_valid_command_path(cmd) { + anyhow::bail!( + "engine.command '{}' contains invalid characters. \ + Only ASCII alphanumerics, '.', '_', '/', and '-' are allowed.", + cmd + ); + } + cmd.to_string() + } + None => "/tmp/awf-tools/copilot".to_string(), + }; + Ok(copilot_invocation(&command_path, prompt_path, mcp_config_path, &args)) + } } } } @@ -243,54 +333,37 @@ fn copilot_args( ); } params.push(format!("--model {}", model)); - if let Some(timeout_minutes) = front_matter.engine.timeout_minutes() { - if timeout_minutes == 0 { - eprintln!( - "Warning: Agent '{}' has timeout-minutes: 0, which means no time is allowed. \ - The agent job will time out immediately. \ - Consider setting timeout-minutes to at least 1.", - front_matter.name - ); - } - } - - // Warn about engine options that are parsed but not yet wired into the pipeline. - // These fields are scaffolding for future engines/features — users should know - // they have no effect today so they aren't confused by silent no-ops. - if !front_matter.engine.args().is_empty() { + if let Some(0) = front_matter.engine.timeout_minutes() { eprintln!( - "Warning: Agent '{}' has engine.args set, but custom CLI arguments are not yet \ - wired into the pipeline and will be ignored.", + "Warning: Agent '{}' has timeout-minutes: 0, which means no time is allowed. \ + The agent job will time out immediately. \ + Consider setting timeout-minutes to at least 1.", front_matter.name ); } - if front_matter.engine.agent().is_some() { - eprintln!( - "Warning: Agent '{}' has engine.agent set, but custom agent file selection is not yet \ - wired into the pipeline and will be ignored.", - front_matter.name - ); - } - if front_matter.engine.api_target().is_some() { - eprintln!( - "Warning: Agent '{}' has engine.api-target set, but custom API target (GHES/GHEC) is \ - not yet wired into the pipeline and will be ignored.", - front_matter.name - ); - } - if front_matter.engine.command().is_some() { - eprintln!( - "Warning: Agent '{}' has engine.command set, but custom engine command paths are not \ - yet wired into the pipeline and will be ignored.", - front_matter.name - ); + + // Wire engine.agent — selects a custom agent from .github/agents/ + if let Some(agent) = front_matter.engine.agent() { + if !is_valid_identifier(agent) { + anyhow::bail!( + "engine.agent '{}' contains invalid characters. \ + Only ASCII alphanumerics, '.', '_', ':', and '-' are allowed.", + agent + ); + } + params.push(format!("--agent {}", agent)); } - if front_matter.engine.env().is_some() { - eprintln!( - "Warning: Agent '{}' has engine.env set, but custom engine environment variables are \ - not yet wired into the pipeline and will be ignored.", - front_matter.name - ); + + // Wire engine.api-target — sets the GHES/GHEC API endpoint hostname + if let Some(api_target) = front_matter.engine.api_target() { + if !is_valid_hostname(api_target) { + anyhow::bail!( + "engine.api-target '{}' contains invalid characters. \ + Only ASCII alphanumerics, '.', and '-' are allowed.", + api_target + ); + } + params.push(format!("--api-target {}", api_target)); } params.push("--disable-builtin-mcps".to_string()); @@ -316,18 +389,102 @@ fn copilot_args( params.push("--allow-all-paths".to_string()); } + // Wire engine.args — append user-provided CLI arguments after compiler-generated args. + // User args are additive; they cannot remove compiler security flags but may override + // non-security defaults via last-wins semantics (e.g., --model). + for arg in front_matter.engine.args() { + if !is_valid_arg(arg) { + anyhow::bail!( + "engine.args entry '{}' contains invalid characters. \ + Only ASCII alphanumerics and '.', '_', ':', '-', '=', '/', '@' are allowed.", + arg + ); + } + // Reject args that attempt to override compiler-controlled flags + for blocked in BLOCKED_ARG_PREFIXES { + if arg.starts_with(blocked) { + anyhow::bail!( + "engine.args entry '{}' conflicts with compiler-controlled flag '{}'. \ + These flags are managed by the compiler and cannot be overridden.", + arg, + blocked + ); + } + } + params.push(arg.to_string()); + } + Ok(params.join(" ")) } -fn copilot_env() -> String { - let lines = [ - "GITHUB_TOKEN: $(GITHUB_TOKEN)", - "GITHUB_READ_ONLY: 1", - "COPILOT_OTEL_ENABLED: \"true\"", - "COPILOT_OTEL_EXPORTER_TYPE: \"file\"", - "COPILOT_OTEL_FILE_EXPORTER_PATH: \"/tmp/awf-tools/staging/otel.jsonl\"", +fn copilot_env(engine_config: &EngineConfig) -> Result { + let mut lines: Vec = vec![ + "GITHUB_TOKEN: $(GITHUB_TOKEN)".to_string(), + "GITHUB_READ_ONLY: 1".to_string(), + "COPILOT_OTEL_ENABLED: \"true\"".to_string(), + "COPILOT_OTEL_EXPORTER_TYPE: \"file\"".to_string(), + "COPILOT_OTEL_FILE_EXPORTER_PATH: \"/tmp/awf-tools/staging/otel.jsonl\"".to_string(), ]; - lines.join("\n") + + // Wire engine.env — merge user-provided environment variables + if let Some(env_map) = engine_config.env() { + let mut sorted_keys: Vec<&String> = env_map.keys().collect(); + sorted_keys.sort(); + + for key in sorted_keys { + let value = &env_map[key]; + + // Validate key: must be a valid env var name + let first_char = key.chars().next().unwrap(); + if key.is_empty() + || !(first_char.is_ascii_alphabetic() || first_char == '_') + || !key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') + { + anyhow::bail!( + "engine.env key '{}' is not a valid environment variable name. \ + Must match [A-Za-z_][A-Za-z0-9_]*.", + key + ); + } + + // Block compiler-controlled env vars + if BLOCKED_ENV_KEYS.iter().any(|blocked| key.eq_ignore_ascii_case(blocked)) { + anyhow::bail!( + "engine.env key '{}' conflicts with a compiler-controlled environment variable. \ + These variables are managed by the compiler and cannot be overridden.", + key + ); + } + + // Validate value: reject ADO command injection and YAML-breaking content + if value.contains("##vso[") || value.contains("##[") { + anyhow::bail!( + "engine.env value for '{}' contains ADO pipeline command injection ('##vso[' or '##['). \ + This is not allowed.", + key + ); + } + if value.contains("$(") || value.contains("${{") { + anyhow::bail!( + "engine.env value for '{}' contains ADO expression syntax ('$(' or '${{{{}}}}')). \ + Use literal values only — ADO macro/expression expansion is not allowed.", + key + ); + } + if value.contains('\n') || value.contains('\r') { + anyhow::bail!( + "engine.env value for '{}' contains newline characters, \ + which would break YAML formatting.", + key + ); + } + + // YAML-quote the value to prevent injection + lines.push(format!("{}: \"{}\"", key, value.replace('\\', "\\\\").replace('"', "\\\""))); + } + } + + Ok(lines.join("\n")) } /// Generate Copilot CLI install steps for Azure DevOps pipelines. @@ -378,12 +535,13 @@ fn copilot_install_steps(engine_config: &EngineConfig) -> String { /// /// The returned string goes inside `-- '...'` in the pipeline YAML. fn copilot_invocation( + command_path: &str, prompt_path: &str, mcp_config_path: Option<&str>, args: &str, ) -> String { let mut parts = vec![ - "/tmp/awf-tools/copilot".to_string(), + command_path.to_string(), format!("--prompt \"$(cat {prompt_path})\""), ]; @@ -433,7 +591,8 @@ mod tests { #[test] fn copilot_engine_env() { - let env = Engine::Copilot.env(); + let (front_matter, _) = parse_markdown("---\nname: test\ndescription: test\n---\n").unwrap(); + let env = Engine::Copilot.env(&front_matter.engine).unwrap(); assert!(env.contains("GITHUB_TOKEN: $(GITHUB_TOKEN)")); assert!(env.contains("GITHUB_READ_ONLY: 1")); assert!(env.contains("COPILOT_OTEL_ENABLED")); @@ -464,4 +623,268 @@ mod tests { fn get_engine_rejects_codex() { assert!(get_engine("codex").is_err()); } + + // ─── engine.command tests ───────────────────────────────────────────── + + #[test] + fn engine_command_overrides_binary_path() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n command: /usr/local/bin/my-copilot\n---\n", + ).unwrap(); + let result = Engine::Copilot + .invocation(&fm, &collect_extensions(&fm), "/tmp/prompt.md", Some("/tmp/mcp.json")) + .unwrap(); + assert!(result.starts_with("/usr/local/bin/my-copilot ")); + assert!(!result.contains("/tmp/awf-tools/copilot")); + } + + #[test] + fn engine_command_default_uses_awf_path() { + let (fm, _) = parse_markdown("---\nname: test\ndescription: test\n---\n").unwrap(); + let result = Engine::Copilot + .invocation(&fm, &collect_extensions(&fm), "/tmp/prompt.md", Some("/tmp/mcp.json")) + .unwrap(); + assert!(result.starts_with("/tmp/awf-tools/copilot ")); + } + + #[test] + fn engine_command_rejects_shell_metacharacters() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n command: \"/tmp/copilot; rm -rf /\"\n---\n", + ).unwrap(); + let result = Engine::Copilot.invocation(&fm, &collect_extensions(&fm), "/tmp/prompt.md", None); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("invalid characters")); + } + + #[test] + fn engine_command_rejects_single_quotes() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n command: \"/tmp/co'pilot\"\n---\n", + ).unwrap(); + let result = Engine::Copilot.invocation(&fm, &collect_extensions(&fm), "/tmp/prompt.md", None); + assert!(result.is_err()); + } + + // ─── engine.agent tests ─────────────────────────────────────────────── + + #[test] + fn engine_agent_adds_flag() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n agent: my-custom-agent\n---\n", + ).unwrap(); + let params = Engine::Copilot.args(&fm, &collect_extensions(&fm)).unwrap(); + assert!(params.contains("--agent my-custom-agent")); + } + + #[test] + fn engine_agent_validates_identifier() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n agent: \"bad agent!\"\n---\n", + ).unwrap(); + let result = Engine::Copilot.args(&fm, &collect_extensions(&fm)); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("invalid characters")); + } + + // ─── engine.api-target tests ────────────────────────────────────────── + + #[test] + fn engine_api_target_adds_flag() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n api-target: api.acme.ghe.com\n---\n", + ).unwrap(); + let params = Engine::Copilot.args(&fm, &collect_extensions(&fm)).unwrap(); + assert!(params.contains("--api-target api.acme.ghe.com")); + } + + #[test] + fn engine_api_target_validates_hostname() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n api-target: \"bad host/path\"\n---\n", + ).unwrap(); + let result = Engine::Copilot.args(&fm, &collect_extensions(&fm)); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("invalid characters")); + } + + #[test] + fn engine_api_target_adds_to_required_hosts() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n api-target: api.acme.ghe.com\n---\n", + ).unwrap(); + let hosts = Engine::Copilot.required_hosts(&fm.engine); + assert_eq!(hosts, vec!["api.acme.ghe.com"]); + } + + #[test] + fn engine_no_api_target_no_required_hosts() { + let (fm, _) = parse_markdown("---\nname: test\ndescription: test\n---\n").unwrap(); + let hosts = Engine::Copilot.required_hosts(&fm.engine); + assert!(hosts.is_empty()); + } + + // ─── engine.args tests ──────────────────────────────────────────────── + + #[test] + fn engine_args_appended_after_compiler_args() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n args:\n - --verbose\n - --debug\n---\n", + ).unwrap(); + let params = Engine::Copilot.args(&fm, &collect_extensions(&fm)).unwrap(); + // Compiler args come first + assert!(params.contains("--disable-builtin-mcps")); + assert!(params.contains("--no-ask-user")); + // User args come after + let disable_pos = params.find("--disable-builtin-mcps").unwrap(); + let verbose_pos = params.find("--verbose").unwrap(); + assert!(verbose_pos > disable_pos, "User args must come after compiler args"); + assert!(params.contains("--debug")); + } + + #[test] + fn engine_args_rejects_shell_injection() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n args:\n - \"--flag; rm -rf /\"\n---\n", + ).unwrap(); + let result = Engine::Copilot.args(&fm, &collect_extensions(&fm)); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("invalid characters")); + } + + #[test] + fn engine_args_blocks_prompt_override() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n args:\n - --prompt=evil\n---\n", + ).unwrap(); + let result = Engine::Copilot.args(&fm, &collect_extensions(&fm)); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("compiler-controlled")); + } + + #[test] + fn engine_args_blocks_allow_tool() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n args:\n - --allow-tool=evil\n---\n", + ).unwrap(); + let result = Engine::Copilot.args(&fm, &collect_extensions(&fm)); + assert!(result.is_err()); + } + + #[test] + fn engine_args_blocks_ask_user() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n args:\n - --ask-user\n---\n", + ).unwrap(); + let result = Engine::Copilot.args(&fm, &collect_extensions(&fm)); + assert!(result.is_err()); + } + + #[test] + fn engine_args_blocks_additional_mcp_config() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n args:\n - --additional-mcp-config=@evil.json\n---\n", + ).unwrap(); + let result = Engine::Copilot.args(&fm, &collect_extensions(&fm)); + assert!(result.is_err()); + } + + // ─── engine.env tests ───────────────────────────────────────────────── + + #[test] + fn engine_env_merges_user_vars() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n MY_VAR: hello\n---\n", + ).unwrap(); + let env = Engine::Copilot.env(&fm.engine).unwrap(); + assert!(env.contains("GITHUB_TOKEN: $(GITHUB_TOKEN)"), "compiler vars preserved"); + assert!(env.contains("MY_VAR: \"hello\""), "user var included"); + } + + #[test] + fn engine_env_blocks_github_token() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n GITHUB_TOKEN: evil\n---\n", + ).unwrap(); + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("compiler-controlled")); + } + + #[test] + fn engine_env_blocks_path() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n PATH: /evil\n---\n", + ).unwrap(); + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + } + + #[test] + fn engine_env_blocks_bash_env() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n BASH_ENV: /evil\n---\n", + ).unwrap(); + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + } + + #[test] + fn engine_env_blocks_ld_preload() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n LD_PRELOAD: /evil.so\n---\n", + ).unwrap(); + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + } + + #[test] + fn engine_env_rejects_vso_injection() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n MY_VAR: \"##vso[task.setvariable]evil\"\n---\n", + ).unwrap(); + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("ADO pipeline command injection")); + } + + #[test] + fn engine_env_rejects_ado_expressions() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n MY_VAR: \"$(SYSTEM_ACCESSTOKEN)\"\n---\n", + ).unwrap(); + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("ADO expression syntax")); + } + + #[test] + fn engine_env_rejects_newlines() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n MY_VAR: \"line1\\nline2\"\n---\n", + ).unwrap(); + // YAML double-quoted strings interpret \n as an actual newline + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("newline characters")); + } + + #[test] + fn engine_env_rejects_invalid_key() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n \"123bad\": value\n---\n", + ).unwrap(); + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("not a valid environment variable name")); + } + + #[test] + fn engine_env_escapes_quotes_in_values() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n MY_VAR: 'has \"quotes\"'\n---\n", + ).unwrap(); + let env = Engine::Copilot.env(&fm.engine).unwrap(); + assert!(env.contains(r#"MY_VAR: "has \"quotes\"""#)); + } } From 56ebd14a0ebd7698ce274138e51da5d92de5e497 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 10:36:52 +0100 Subject: [PATCH 8/9] fix: prevent panic on empty env key and validate engine.version - Fix empty env key panic: key.chars().next().unwrap() was called before the emptiness check, causing a panic on empty YAML keys ("": value). Now uses let-else to bail with a clear error. - Validate engine.version: the version string was embedded unvalidated into NuGet command arguments, allowing injection (e.g., spaces, quotes, additional -Source flags). Now validated with is_valid_version allowlist [A-Za-z0-9._-] before use. install_steps() returns Result to propagate the validation error. - 5 new tests for both fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 2 +- src/engine.rs | 88 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 81 insertions(+), 9 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index ca92a31..8676f65 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1930,7 +1930,7 @@ pub async fn compile_shared( "/tmp/awf-tools/threat-analysis-prompt.md", None, )?; - let engine_install_steps = ctx.engine.install_steps(&front_matter.engine); + let engine_install_steps = ctx.engine.install_steps(&front_matter.engine)?; // 5. Compute workspace, working directory, triggers let effective_workspace = compute_effective_workspace( diff --git a/src/engine.rs b/src/engine.rs index 0388f1b..ac4f251 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -25,6 +25,13 @@ fn is_valid_hostname(s: &str) -> bool { .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-')) } +/// Characters allowed in engine.version strings (e.g., "1.0.34", "latest"). +fn is_valid_version(s: &str) -> bool { + !s.is_empty() + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-')) +} + /// Characters allowed in individual engine.args entries. /// Strict allowlist to prevent shell injection inside AWF single-quoted commands. fn is_valid_arg(s: &str) -> bool { @@ -155,7 +162,7 @@ impl Engine { /// Uses `engine_config.version()` if set in front matter, otherwise falls back /// to the pinned `COPILOT_CLI_VERSION` constant. Returns an empty string when /// `engine.command` is set (the user provides their own binary). - pub fn install_steps(&self, engine_config: &EngineConfig) -> String { + pub fn install_steps(&self, engine_config: &EngineConfig) -> Result { match self { Engine::Copilot => copilot_install_steps(engine_config), } @@ -435,9 +442,13 @@ fn copilot_env(engine_config: &EngineConfig) -> Result { let value = &env_map[key]; // Validate key: must be a valid env var name - let first_char = key.chars().next().unwrap(); - if key.is_empty() - || !(first_char.is_ascii_alphabetic() || first_char == '_') + let Some(first_char) = key.chars().next() else { + anyhow::bail!( + "engine.env contains an empty key. \ + Keys must match [A-Za-z_][A-Za-z0-9_]*." + ); + }; + if !(first_char.is_ascii_alphabetic() || first_char == '_') || !key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { anyhow::bail!( @@ -492,17 +503,27 @@ fn copilot_env(engine_config: &EngineConfig) -> Result { /// Produces the YAML block that authenticates with NuGet, installs the /// `Microsoft.Copilot.CLI.linux-x64` package, copies the binary to /// `/tmp/awf-tools/copilot`, and verifies the install. -fn copilot_install_steps(engine_config: &EngineConfig) -> String { +fn copilot_install_steps(engine_config: &EngineConfig) -> Result { // Custom binary path → skip NuGet install entirely if engine_config.command().is_some() { - return String::new(); + return Ok(String::new()); } let version = engine_config .version() .unwrap_or(COPILOT_CLI_VERSION); - format!( + // Validate version to prevent NuGet argument injection — the version string + // is embedded directly into NuGet command arguments. + if !is_valid_version(version) { + anyhow::bail!( + "engine.version '{}' contains invalid characters. \ + Only ASCII alphanumerics, '.', '_', and '-' are allowed.", + version + ); + } + + Ok(format!( "\ - task: NuGetAuthenticate@1 displayName: \"Authenticate NuGet Feed\" @@ -528,7 +549,7 @@ fn copilot_install_steps(engine_config: &EngineConfig) -> String { copilot --version copilot -h displayName: \"Output copilot version\"" - ) + )) } /// Build the full AWF `--` command string for the Copilot CLI. @@ -887,4 +908,55 @@ mod tests { let env = Engine::Copilot.env(&fm.engine).unwrap(); assert!(env.contains(r#"MY_VAR: "has \"quotes\"""#)); } + + // ─── engine.version validation tests ────────────────────────────────── + + #[test] + fn engine_version_rejects_injection() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n version: '1.0.0 -Source https://evil.com'\n---\n", + ).unwrap(); + let result = Engine::Copilot.install_steps(&fm.engine); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("invalid characters")); + } + + #[test] + fn engine_version_rejects_single_quotes() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n version: \"1.0.0'\"\n---\n", + ).unwrap(); + let result = Engine::Copilot.install_steps(&fm.engine); + assert!(result.is_err()); + } + + #[test] + fn engine_version_accepts_valid() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n version: '1.0.34'\n---\n", + ).unwrap(); + let result = Engine::Copilot.install_steps(&fm.engine).unwrap(); + assert!(result.contains("-Version 1.0.34")); + } + + #[test] + fn engine_version_accepts_latest() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n version: latest\n---\n", + ).unwrap(); + let result = Engine::Copilot.install_steps(&fm.engine).unwrap(); + assert!(result.contains("-Version latest")); + } + + // ─── engine.env empty key test ──────────────────────────────────────── + + #[test] + fn engine_env_rejects_empty_key() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n env:\n \"\": value\n---\n", + ).unwrap(); + let result = Engine::Copilot.env(&fm.engine); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("empty key")); + } } From 500a1c6a856840133e13c3610d9e3f8376fb812b Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 22 Apr 2026 10:46:00 +0100 Subject: [PATCH 9/9] fix: handle engine.version "latest" and quote engine_log_dir in templates - engine.version: "latest" now omits the -Version flag entirely so NuGet installs the newest available version, instead of passing "-Version latest" which NuGet rejects at runtime. - Quote {{ engine_log_dir }} in base.yml and 1es-base.yml shell scripts to future-proof against paths with spaces. - Add explanatory comment on the intentionally case-insensitive BLOCKED_ENV_KEYS comparison (prevents accidental shadowing). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/data/1es-base.yml | 12 ++++++------ src/data/base.yml | 12 ++++++------ src/engine.rs | 20 +++++++++++++++++--- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/data/1es-base.yml b/src/data/1es-base.yml index fc316aa..3545c5f 100644 --- a/src/data/1es-base.yml +++ b/src/data/1es-base.yml @@ -389,8 +389,8 @@ extends: - bash: | # Copy all logs to output directory for artifact upload mkdir -p "$(Agent.TempDirectory)/staging/logs" - if [ -d {{ engine_log_dir }} ]; then - cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true + if [ -d "{{ engine_log_dir }}" ]; then + cp -r "{{ engine_log_dir }}"/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then cp -r ~/.ado-aw/logs/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true @@ -586,9 +586,9 @@ extends: - bash: | # Copy all logs to analyzed outputs for artifact upload mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs" - if [ -d {{ engine_log_dir }} ]; then + if [ -d "{{ engine_log_dir }}" ]; then mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot" - cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot/" 2>/dev/null || true + cp -r "{{ engine_log_dir }}"/* "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs/ado-aw" @@ -668,9 +668,9 @@ extends: # Copy agent output log from analyzed_outputs for optimisation use cp "$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)/logs/agent-output.txt" \ "$(Agent.TempDirectory)/staging/logs/agent-output.txt" 2>/dev/null || true - if [ -d {{ engine_log_dir }} ]; then + if [ -d "{{ engine_log_dir }}" ]; then mkdir -p "$(Agent.TempDirectory)/staging/logs/copilot" - cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/staging/logs/copilot/" 2>/dev/null || true + cp -r "{{ engine_log_dir }}"/* "$(Agent.TempDirectory)/staging/logs/copilot/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then mkdir -p "$(Agent.TempDirectory)/staging/logs/ado-aw" diff --git a/src/data/base.yml b/src/data/base.yml index 4cf754c..fa6d084 100644 --- a/src/data/base.yml +++ b/src/data/base.yml @@ -360,8 +360,8 @@ jobs: - bash: | # Copy all logs to output directory for artifact upload mkdir -p "$(Agent.TempDirectory)/staging/logs" - if [ -d {{ engine_log_dir }} ]; then - cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true + if [ -d "{{ engine_log_dir }}" ]; then + cp -r "{{ engine_log_dir }}"/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then cp -r ~/.ado-aw/logs/* "$(Agent.TempDirectory)/staging/logs/" 2>/dev/null || true @@ -555,9 +555,9 @@ jobs: - bash: | # Copy all logs to analyzed outputs for artifact upload mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs" - if [ -d {{ engine_log_dir }} ]; then + if [ -d "{{ engine_log_dir }}" ]; then mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot" - cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot/" 2>/dev/null || true + cp -r "{{ engine_log_dir }}"/* "$(Agent.TempDirectory)/analyzed_outputs/logs/copilot/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then mkdir -p "$(Agent.TempDirectory)/analyzed_outputs/logs/ado-aw" @@ -636,9 +636,9 @@ jobs: # Copy agent output log from analyzed_outputs for optimisation use cp "$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)/logs/agent-output.txt" \ "$(Agent.TempDirectory)/staging/logs/agent-output.txt" 2>/dev/null || true - if [ -d {{ engine_log_dir }} ]; then + if [ -d "{{ engine_log_dir }}" ]; then mkdir -p "$(Agent.TempDirectory)/staging/logs/copilot" - cp -r {{ engine_log_dir }}/* "$(Agent.TempDirectory)/staging/logs/copilot/" 2>/dev/null || true + cp -r "{{ engine_log_dir }}"/* "$(Agent.TempDirectory)/staging/logs/copilot/" 2>/dev/null || true fi if [ -d ~/.ado-aw/logs ]; then mkdir -p "$(Agent.TempDirectory)/staging/logs/ado-aw" diff --git a/src/engine.rs b/src/engine.rs index ac4f251..51f0e58 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -458,7 +458,11 @@ fn copilot_env(engine_config: &EngineConfig) -> Result { ); } - // Block compiler-controlled env vars + // Block compiler-controlled env vars. + // Intentionally case-insensitive: while Linux env vars are case-sensitive, + // blocking both "GITHUB_TOKEN" and "github_token" prevents accidental + // shadowing and confusion. The trade-off is that a legitimate custom var + // whose name collides case-insensitively with a blocked key is rejected. if BLOCKED_ENV_KEYS.iter().any(|blocked| key.eq_ignore_ascii_case(blocked)) { anyhow::bail!( "engine.env key '{}' conflicts with a compiler-controlled environment variable. \ @@ -523,6 +527,14 @@ fn copilot_install_steps(engine_config: &EngineConfig) -> Result { ); } + // "latest" means "install the newest available version" — NuGet doesn't + // recognise "latest" as a version string; omitting -Version installs the newest. + let version_arg = if version == "latest" { + String::new() + } else { + format!("-Version {version} ") + }; + Ok(format!( "\ - task: NuGetAuthenticate@1 @@ -532,7 +544,7 @@ fn copilot_install_steps(engine_config: &EngineConfig) -> Result { displayName: \"Install Copilot CLI\" inputs: command: 'custom' - arguments: 'install Microsoft.Copilot.CLI.linux-x64 -Source \"https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json\" -Version {version} -OutputDirectory $(Agent.TempDirectory)/tools -ExcludeVersion -NonInteractive' + arguments: 'install Microsoft.Copilot.CLI.linux-x64 -Source \"https://pkgs.dev.azure.com/msazuresphere/_packaging/Guardian1ESPTUpstreamOrgFeed/nuget/v3/index.json\" {version_arg}-OutputDirectory $(Agent.TempDirectory)/tools -ExcludeVersion -NonInteractive' - bash: | ls -la \"$(Agent.TempDirectory)/tools\" @@ -945,7 +957,9 @@ mod tests { "---\nname: test\ndescription: test\nengine:\n id: copilot\n version: latest\n---\n", ).unwrap(); let result = Engine::Copilot.install_steps(&fm.engine).unwrap(); - assert!(result.contains("-Version latest")); + // "latest" omits -Version entirely so NuGet installs the newest available + assert!(!result.contains("-Version"), "should not contain -Version flag for 'latest'"); + assert!(result.contains("-OutputDirectory"), "should still contain other NuGet args"); } // ─── engine.env empty key test ────────────────────────────────────────