refactor: complete Engine abstraction — move install, invocation, and paths behind Engine enum

[jamesadevine]

Summary

Completes the Engine abstraction by moving all Copilot-specific artifacts out of pipeline templates and behind Engine enum methods, then wires all remaining engine.* front matter fields into the pipeline. Adding a new engine now requires only implementing the enum variant — no template changes needed.

Closes #287

Changes

New Engine methods

Method	Template Marker	Purpose
install_steps(engine_config)	{{ engine_install_steps }}	NuGet install + binary copy + verification
invocation(fm, exts, prompt, mcp)	{{ engine_run }}, {{ engine_run_detection }}	Full AWF -- command string
log_dir()	{{ engine_log_dir }}	Engine log directory for collection
env(engine_config)	{{ engine_env }}	Sandbox step environment variables
required_hosts(engine_config)	(domain list)	Engine-required AWF network allowlist entries

Removed template markers

• {{ copilot_version }} — absorbed into {{ engine_install_steps }}
• {{ copilot_params }} — absorbed into {{ engine_run }} / {{ engine_run_detection }}

Engine front matter fields — now fully wired

All five previously "not yet wired" fields are now functional:

Field	What it does	Security
command	Overrides the engine binary path in AWF invocation; skips NuGet install	Strict char allowlist [A-Za-z0-9_./-]
agent	Adds --agent <name> to Copilot CLI for custom agent selection	Identifier validation
api-target	Adds --api-target <host> for GHES/GHEC + adds hostname to AWF allowlist	Hostname validation
args	Appends user CLI args after compiler-generated args	Char allowlist + 10 blocked flag prefixes
env	Merges user env vars into sandbox step env: block	12 blocked keys, ADO injection checks, YAML quoting

max-turns removed

The max-turns field was specific to Claude Code and not supported by Copilot CLI. Removed from EngineOptions, EngineConfig accessors, and all tests.

Key design decisions

• Engine owns prompt delivery: invocation() generates the complete command including --prompt "$(cat ...)" and --additional-mcp-config @.... Different engines can use different prompt delivery mechanisms.
• engine.version wired: install_steps() uses engine_config.version() if set, falling back to COPILOT_CLI_VERSION. Returns empty when engine.command is set.
• engine.args merge strategy: User args are appended after compiler args (last-wins for duplicate flags). Compiler security flags always appear first and cannot be removed. Blocked prefixes: --prompt, --additional-mcp-config, --allow-tool, --allow-all-tools, --allow-all-paths, --disable-builtin-mcps, --no-ask-user, --ask-user.
• engine.env security: Blocked keys include compiler-controlled (GITHUB_TOKEN, COPILOT_OTEL_*) and dangerous shell/system vars (PATH, BASH_ENV, LD_PRELOAD, etc.). Values reject ADO expressions ($(, ${{), pipeline commands (##vso[), and newlines. Values are YAML-quoted with proper escaping.
• Dead code removed: $HOME/.copilot/mcp-config.json copy was redundant — AWF invocation explicitly passes --additional-mcp-config @/tmp/awf-tools/mcp-config.json.
• COPILOT_CLI_VERSION moved from compile/common.rs to engine.rs (engine-owned constant).

Files changed

• src/engine.rs — all 5 field wirings, validation helpers, blocked lists, required_hosts(), 33 new tests
• src/compile/common.rs — env() signature update, engine hosts in domain list
• src/compile/standalone.rs — api-target domain allowlist test
• src/compile/types.rs — max_turns removal
• src/data/base.yml + src/data/1es-base.yml — template marker replacements
• AGENTS.md — updated engine config table (removed "not yet wired"), updated marker docs

Out of scope

• run.rs (debug-only local dev mode)
• Implementing a second engine

Testing

All 954 tests pass (33 new). Clippy clean on changed files.

[github-actions]

🔍 Rust PR Review

Summary: Mostly well-executed refactor — the abstraction is clean and the template changes are complete, but there's one security gap and one logic regression worth addressing before merge.

Findings

🔒 Security Concerns

• src/engine.rs:362 — engine.version interpolated without sanitization

  The version string from front matter is directly embedded inside a single-quoted YAML argument:

  arguments: 'install Microsoft.Copilot.CLI.linux-x64 ... -Version {version} ...'

  A version string containing ' (e.g. engine: { version: "1.0' -ExtraArg value" }) would break the YAML string quoting and corrupt the generated pipeline. Worse, a \n in the version string would inject new YAML keys.

  The rest of the codebase validates user-controlled strings that get embedded in YAML/shell (e.g. is_safe_tool_name, is_valid_parameter_name, single-quote escaping for service connection names in generate_acquire_ado_token). The same care should apply here. A NuGet version should only contain alphanumerics, dots, and hyphens — a simple validation at the top of copilot_install_steps (or in EngineConfig::version()) would close this:

  if !version.chars().all(|c| c.is_alphanumeric() || c == '.' || c == '-') {
      // bail! or emit a warning and fall back to COPILOT_CLI_VERSION
  }

🐛 Bugs / Logic Issues

• src/engine.rs:348,388 — engine.command skips install but invocation still hardcodes /tmp/awf-tools/copilot

  copilot_install_steps returns String::new() when engine_config.command().is_some(), but copilot_invocation unconditionally starts with /tmp/awf-tools/copilot. Before this PR, users who set engine.command (despite the "not yet wired" warning) would still get the NuGet install, leaving a working binary at /tmp/awf-tools/copilot. Now they get no install and an invocation pointing at a non-existent path — a silent runtime failure.

  Since engine.command isn't fully wired into invocation() (per the PR description, out of scope), the safest fix is to remove the engine.command early-return from install_steps() until the invocation also uses the custom path. Partial wiring is worse than no wiring here.

⚠️ Suggestions

• src/data/base.yml:363, src/data/1es-base.yml:392 (and 4 other occurrences) — {{ engine_log_dir }} unquoted in bash

  if [ -d {{ engine_log_dir }} ]; then
    cp -r {{ engine_log_dir }}/* ...

  Today this expands to ~/.copilot/logs which is fine. But log_dir() is a trait-dispatch point for future engines — if one returns a path with spaces, the generated bash would silently break. Worth quoting pre-emptively: if [ -d "{{ engine_log_dir }}" ] and cp -r "{{ engine_log_dir }}"/*.

• Missing unit tests for the three new Engine methods

  install_steps(), invocation(), and log_dir() have no unit tests in src/engine.rs. The existing test suite covers args(), env(), and command(). At minimum it would help to add tests that verify:

  • install_steps with default version contains COPILOT_CLI_VERSION
  • install_steps with a version override uses that version
  • install_steps returns empty string when engine.command is set (once the logic regression above is resolved)
  • copilot_invocation with None mcp path omits --additional-mcp-config

✅ What Looks Good

• replace_with_indent correctly re-indents multi-line YAML blocks — the {{ engine_install_steps }} substitution at 6-space (standalone) and 16-space (1ES) indentation levels will both render valid YAML.
• No stale {{ copilot_params }} or {{ copilot_version }} markers remain in either template.
• The warning for engine.version is correctly removed from copilot_args() since it's now wired. The symmetry (warn for unwired fields, don't warn for wired ones) is clean.
• AGENTS.md and the version-updater workflow doc are properly updated to reflect the COPILOT_CLI_VERSION move to src/engine.rs.
• Removal of the redundant $HOME/.copilot/mcp-config.json copy is a clean cleanup.

Generated by Rust PR Reviewer for issue #301 · ● 2.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Mostly clean refactor with one functional bug and two stale documentation entries that need fixing before merge.

---

Findings

🐛 Bugs / Logic Issues

• src/engine.rs:344–357 — engine.version: "latest" causes NuGet install failure.
  copilot_install_steps() interpolates the version string directly into -Version {version}. The AGENTS.md docs (and prior test) explicitly mention "latest" as a valid value, but NuGet's install command doesn't accept latest as a version — it needs -Version omitted entirely to resolve the newest package. The pre-PR warning code was removed (correct), but the "latest" special case was never implemented. Suggested fix:

  let version_flag = match engine_config.version() {
      Some("latest") | None => String::new(),
      Some(v) => format!("-Version {v} "),
  };
  // use version_flag in the arguments string

🔒 Security Concerns

• src/engine.rs:357 — No validation on engine.version before embedding in NuGet arguments string.
  The version is interpolated into a single-quoted YAML string: arguments: 'install ... -Version {version} ...'. A value containing ' would break out of the YAML literal. While this field is author-controlled, a quick regex guard (alphanumerics + ., -) would keep the generated YAML well-formed and consistent with the model-name validation already present in copilot_args().

⚠️ Suggestions

• AGENTS.md:349,354 — version and command still marked "Not yet wired".
  Both fields are wired in this PR (install_steps() respects both), but the docs still say "Not yet wired — parsed but ignored with a warning." This will actively mislead users. The {{ engine_install_steps }} section added to AGENTS.md calls out version support, but the field reference table above it contradicts it.

• src/engine.rs — No unit tests for the three new public methods.
  install_steps(), invocation(), and log_dir() have no #[cfg(test)] coverage. A few targeted tests would be valuable given that correctness here directly affects every generated pipeline:

  • install_steps with version set vs. default vs. command set (should return empty)
  • invocation with and without mcp_config_path
  • Snapshot/contains check that the generated YAML block has the right task names

• src/compile/types.rs — Silent removal of max-turns deprecation warning.
  max-turns was previously emitting a named deprecation warning. Now it's silently unknown (serde ignores it). Users with max-turns: in existing front matter get no signal. Not a blocker, but worth a CHANGELOG note or a soft migration path.

---

✅ What Looks Good

• replace_with_indent used throughout — all multi-line block replacements (including {{ engine_install_steps }} at 6/16-space indent in base.yml/1es-base.yml) correctly propagate indentation. No YAML structure corruption.
• Detection job correctly omits --additional-mcp-config — invocation(..., None) properly drops the MCP flag for the threat-analysis job.
• engine.command short-circuits install — copilot_install_steps() returns String::new() cleanly when the user supplies their own binary.
• $HOME/.copilot/mcp-config.json copy removal is safe — the AWF invocation explicitly passes --additional-mcp-config @/tmp/awf-tools/mcp-config.json, so the home-dir copy was genuinely redundant.
• Rename of 20 test functions is thorough and consistent.

Generated by Rust PR Reviewer for issue #301 · ● 2.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid abstraction with good security rigor — one bug to fix before merging, one missing validation worth adding for consistency.

---

Findings

🐛 Bugs / Logic Issues

• src/engine.rs:438 — Panic on empty env key. key.chars().next().unwrap() is evaluated before the key.is_empty() guard on line 439, so an empty YAML key ("": value) causes an unwrap-on-None panic instead of the intended bail!. Since engine.env is deserialized into HashMap<String, String>, YAML allows empty string keys. Fix by checking emptiness first:

  // Instead of:
  let first_char = key.chars().next().unwrap();
  if key.is_empty() || ...
  
  // Use:
  let Some(first_char) = key.chars().next() else {
      anyhow::bail!("engine.env key is empty ...");
  };
  if !(first_char.is_ascii_alphabetic() || first_char == '_') || ...

🔒 Security Concerns

• src/engine.rs:501–514 — engine.version is embedded unvalidated directly into the NuGet arguments: YAML single-quoted string. Every other engine field (command, agent, api-target, args, env keys/values) has an explicit character allowlist; version has none. A value like "1.0.0 -Source (evil.com/redacted)" would inject additional NuGet command args; a value with ' would break the generated YAML. The fix is a short is_valid_version predicate (e.g., [A-Za-z0-9._-]+) mirroring is_valid_identifier, called before the format!.

✅ What Looks Good

• replace_with_indent correctly reindents the multi-line {{ engine_install_steps }} block for both the 2-space standalone template and the 16-space 1ES template — no YAML formatting issue there.
• The BLOCKED_ARG_PREFIXES list is thorough and the --allow-all-tools/--allow-all-paths split is preserved correctly in is_valid_arg's character allowlist (no !, ;, |, `).
• engine.env YAML quoting (\\ → \\\\, " → \") is correct; newline and ADO expression checks are in the right order (value validation before insertion).
• Case-insensitive matching in BLOCKED_ENV_KEYS (eq_ignore_ascii_case) prevents case-variation bypass.
• Removing the redundant $HOME/.copilot/mcp-config.json copy is correct — the AWF invocation already passes --additional-mcp-config @/tmp/awf-tools/mcp-config.json explicitly.
• 33 targeted unit tests provide good coverage of the new validation paths.

Generated by Rust PR Reviewer for issue #301 · ● 1.8M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid refactor with good security hygiene — one runtime bug with engine.version: "latest" needs a fix before this ships.

---

Findings

🐛 Bugs / Logic Issues

• src/engine.rs:535 — engine.version: "latest" will fail at pipeline runtime

  When engine.version is set to "latest", copilot_install_steps generates:

  -Version latest
  

  but nuget install doesn't recognise "latest" as a version string — to install the newest version you simply omit -Version. The pipeline step will fail with a version-not-found error.

  The fix is a one-liner before the format!:

  let version_arg = if version == "latest" {
      String::new()
  } else {
      format!("-Version {version} ")
  };
  // then use `{version_arg}` in the arguments string (minus the hardcoded `-Version`)

  The companion test engine_version_accepts_latest (line 943) asserts result.contains("-Version latest"), which means it's asserting the broken behaviour rather than the correct one. It should assert the flag is absent.

---

⚠️ Suggestions

• src/engine.rs / template — engine_log_dir inserted unquoted in shell scripts

  log_dir() returns ~/.copilot/logs, which is inserted literally into:

  if [ -d {{ engine_log_dir }} ]; then
    cp -r {{ engine_log_dir }}/* ...

  Bash tilde-expansion works fine here today, but the substitution is unquoted. If a future engine returns a path with spaces the script would silently break. Wrapping with double-quotes in the templates now ("{{ engine_log_dir }}") would future-proof this at zero cost.

• src/engine.rs:462 — BLOCKED_ENV_KEYS comparison is case-insensitive

  Using eq_ignore_ascii_case means github_token is blocked even though Linux env vars are case-sensitive and github_token ≠ GITHUB_TOKEN. This is intentionally conservative (fine), but it means someone can't legitimately pass MY_CUSTOM_github_data without hitting a false-positive if they're unlucky. Worth a brief comment in the code explaining the intentional conservatism.

---

✅ What Looks Good

• The validation allowlists (is_valid_command_path, is_valid_identifier, is_valid_hostname, is_valid_arg) are tight and well-separated. Excluding ' from all of them is exactly right given the AWF single-quote wrapping.
• engine.env YAML quoting (escaping \ then ") is correct — handles the full set of YAML-double-quote special chars.
• BLOCKED_ARG_PREFIXES uses prefix matching so --prompt=anything is caught the same way as --prompt anything. Clean.
• required_hosts returning the api-target hostname is the right place to do it — the domain allowlist wiring in generate_allowed_domains picks it up automatically.
• 33 new tests covering all the new validation paths; coverage looks exhaustive for the happy paths and adversarial inputs.
• Removing the redundant $HOME/.copilot/mcp-config.json copy is a correct and safe dead-code removal.

Generated by Rust PR Reviewer for issue #301 · ● 1.5M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid refactor with good security validation throughout — one regression introduced in the template migration that silently breaks log collection.

---

Findings

🐛 Bugs / Logic Issues

• src/engine.rs:140 + src/data/base.yml:363 (and 1es-base.yml) — Log collection is silently broken. log_dir() returns "~/.copilot/logs", but every usage in the templates is now inside double-quoted strings:

  if [ -d "{{ engine_log_dir }}" ]; then
    cp -r "{{ engine_log_dir }}"/* ...

  In bash, tilde expansion does not occur inside double quotes — "~/.copilot/logs" is treated as a literal path starting with ~, so the [ -d ... ] check always returns false and no logs are collected. The old template used unquoted ~/.copilot/logs directly, where tilde expansion works.

  The fix is to return $HOME/.copilot/logs from log_dir() (shell variable expansion works inside double quotes) rather than the tilde form:

  Engine::Copilot => "$HOME/.copilot/logs",

  This appears across 6 locations in base.yml and 6 in 1es-base.yml.

---

⚠️ Suggestions

• src/engine.rs:44 BLOCKED_ARG_PREFIXES — --model is intentionally not blocked (last-wins is the documented design), but it's worth a brief comment near the list to explain why, since a future reviewer might add it thinking it's an oversight.

• src/engine.rs:37 is_valid_arg — Spaces are already excluded by the allowlist, but the error message could be clearer that values with spaces should be split into separate list items. This would save users debugging time when they write args: ["--flag value"] instead of args: ["--flag", "value"].

---

✅ What Looks Good

• The security validation is comprehensive and well-layered: is_valid_command_path, is_valid_hostname, is_valid_arg, is_valid_version, plus blocked-prefix checks for args and blocked-key checks for env — all with clear, actionable error messages.
• engine.env YAML quoting (replace('\\', "\\\\").replace('"', "\\\"")) is correct for YAML double-quoted strings, and the ADO injection checks (##vso[, $(, ${{, newlines) are thorough.
• copilot_install_steps returning empty string when engine.command is set cleanly handles the custom binary case.
• Sorting env keys for deterministic output is a good call — avoids spurious diffs in check runs.
• 33 targeted unit tests covering both happy-path and injection/validation edge cases for all 5 newly-wired fields.
• engine.api-target being added to required_hosts() and then fed into generate_allowed_domains is the right plumbing — it ensures GHES/GHEC hostnames actually reach the AWF allowlist.

Generated by Rust PR Reviewer for issue #301 · ● 1.4M · ◷