feat!: align engine front matter with gh-aw and hook up Engine enum

[jamesadevine]

Summary

Aligns the engine: front matter with gh-aw's engine format and hooks up the Engine enum through the compile pipeline.

Breaking Change

The engine: field now takes an engine identifier (copilot), not a model name. Model moves to the object form:

# Before (no longer valid)
engine: claude-opus-4.5

# After — simple form (defaults to copilot)
engine: copilot

# After — object form with model override
engine:
  id: copilot
  model: claude-opus-4.5
  timeout-minutes: 30

Non-copilot engine identifiers (claude, codex, gemini) produce a compile error.

Changes

Engine architecture

• Engine is now an enum (Engine::Copilot) with enum dispatch — no trait objects or &dyn
• get_engine() factory resolves from front matter engine_id(), rejects unsupported engines
• Engine stored in CompileContext — downstream code uses ctx.engine

EngineConfig (types.rs)

• String form = engine identifier, object form = {id, version, model, agent, api-target, args, env, command, timeout-minutes}
• Full gh-aw field alignment (parsed + stored, pipeline wiring for new fields is incremental)
• model() returns Option<&str> — engine impl provides default when None

Engine env consolidation

• GITHUB_TOKEN, GITHUB_READ_ONLY, COPILOT_OTEL_* moved from pipeline templates into Engine::env()
• {{ copilot_ado_env }} renamed to {{ engine_env }}
• AZURE_DEVOPS_EXT_PAT removed from engine env (handled by MCPG)
• SYSTEM_ACCESSTOKEN removed from agent sandbox step

Removed

• cancel_previous_builds — used $(System.AccessToken) directly
• generate_copilot_ado_env wrapper — callers use ctx.engine.env() directly
• default_model() helper — default lives in engine impl (DEFAULT_COPILOT_MODEL)

Updated

• Examples updated to new engine: { id: copilot, model: ... } format
• All tests updated for new format

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the engine enum refactor is clean and the breaking change is well-handled. A few items worth addressing.

Findings

⚠️ Suggestions

• AGENTS.md not updated — The project's own reference doc still describes the old format:

  engine: claude-opus-4.5 # AI engine to use. Defaults to claude-opus-4.5. Other options include claude-sonnet-4.5, gpt-5.2-codex, ...

  and later:

  engine:
    model: claude-opus-4.5

  Both examples now silently break (the first will be rejected at compile time; the second accepts model but id is needed to pick a non-default). The AGENTS.md should be updated to reflect the new engine: copilot / engine: { id: copilot, model: ... } format.

• src/engine.rs:237–244 — copilot_args body retains 8-space indent from its old impl context — The function was extracted from the impl Engine for GitHubCopilotCliEngine block but its body kept the old double-indent. Cosmetic, but makes the file harder to read.

• New EngineOptions fields are parsed but silently ignored (args, version, agent, api-target, command, env in src/compile/types.rs). The PR description acknowledges "pipeline wiring for new fields is incremental", but a user who writes:

  engine:
    id: copilot
    args: ["--add-dir", "/workspace"]
    api-target: api.acme.ghe.com

  will get no error and no effect. Consider emitting eprintln! warnings (matching the pattern already used for max-turns and timeout-minutes: 0) so users aren't confused. Especially important for api-target and command, since those hint at GHES/GHEC and custom binary paths — both plausibly real near-term use cases.

• Engine::command() is never used — Engine::command() (returns "copilot") is defined on the enum but called nowhere in the codebase. Meanwhile, EngineConfig::command() accessor exists for the front-matter value but is also never used. If Engine::command() is meant to be the canonical source for the CLI binary name, it should be wired into the pipeline template (replacing the hard-coded copilot in base.yml). If it's purely scaffolding for future engines, a #[allow(dead_code)] or a doc comment acknowledging this would clarify intent.

• cancel_previous_builds removal — Intentional per the PR description, but worth noting in the commit/changelog that pipeline-triggered pipelines will no longer auto-cancel stale queued runs. Users relying on that behaviour will need another mechanism (e.g. ADO's own concurrency controls). The test update correctly reflects the removal.

✅ What Looks Good

• get_engine() with an explicit match over engine identifiers is the right approach — easy to extend, clear error messages.
• CompileContext::new() correctly becomes Result<Self> and all three callsites (standalone.rs, onees.rs, run.rs) propagate the error with ?.
• Moving GITHUB_TOKEN, GITHUB_READ_ONLY, and COPILOT_OTEL_* into Engine::env() is a clean consolidation — the env block is now the engine's responsibility, not the template's.
• replace_with_indent correctly handles the multi-line {{ engine_env }} replacement at both template indentation levels (10 spaces in base.yml, 20 in 1es-base.yml).
• Model-name injection validation (test_model_name_rejects_single_quote, test_model_name_rejects_space) is preserved and adapted to the new EngineOptions::Full shape.

Generated by Rust PR Reviewer for issue #286 · ● 2.3M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Mostly solid architecture, but two stale prompt files will cause broken pipeline generation by the dispatcher agent.

---

Findings

🐛 Bugs / Logic Issues

• prompts/create-ado-agentic-workflow.md line 430 — The "Complete Example" still uses the old format:

  engine: claude-sonnet-4.5

  With this PR, EngineConfig::Simple("claude-sonnet-4.5") → get_engine("claude-sonnet-4.5") → compile error: "Unsupported engine 'claude-sonnet-4.5'". The dispatcher agent generated by init follows these prompts verbatim to create new pipelines. Any pipeline it generates will fail to compile.

• prompts/create-ado-agentic-workflow.md lines 67–79 — Step 2 guidance still frames engine: as a model selector ("Default is claude-opus-4.5. Only include engine: if the user requests a different model."), and the object example omits id: copilot:

  engine:
    model: claude-sonnet-4.5   # missing id: copilot
    timeout-minutes: 30

  The omission of id: is technically fine (defaults to "copilot"), but the surrounding narrative teaches the wrong mental model — dispatcher will generate engine: claude-opus-4.5 (the model name as an engine identifier) when users ask for the default.

• prompts/debug-ado-agentic-workflow.md line 136 — Still says "Check the engine: field matches a supported model (claude-opus-4.5, claude-sonnet-4.5, gpt-5.2-codex, ...)" — this is now wrong; engine: is an engine identifier, not a model name. A debugging user following this will write engine: claude-opus-4.5 and get a confusing error.

⚠️ Suggestions

• generate_copilot_params signature (src/compile/common.rs:423) — the wrapper now takes both front_matter: &FrontMatter and ctx: &CompileContext<'_>, but ctx.front_matter is the same value. The function body only uses front_matter as a passthrough to ctx.engine.args(). Consider either dropping the redundant front_matter parameter (accessing it via ctx.front_matter) or inlining the wrapper entirely since it's now a trivial one-liner. Low priority, but the dual paths are a bit confusing for readers.

✅ What Looks Good

• Engine enum dispatch is clean — #[derive(Debug, Clone, Copy)] on a C-like enum is the right choice here. No trait objects or Arc<dyn Engine> overhead.
• get_engine() error messages are actionable — users see exactly which identifier failed and are directed to the supported value.
• ADO token removal from copilot_env() is a genuine security improvement — the agent no longer gets AZURE_DEVOPS_EXT_PAT/SYSTEM_ACCESSTOKEN in its env; those now flow only via MCPG to authorized MCP servers.
• replace_with_indent correctly handles the 5-line copilot_env() output inside the env: YAML block — no indentation bug.
• cancel_previous_builds removal is justified — it was the only place using $(System.AccessToken) directly, which violates the explicit-service-connection security model.
• All 68 unit + integration tests pass with cargo test.

Generated by Rust PR Reviewer for issue #286 · ● 2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid architectural refactor, but has a functional regression in permissions.read token forwarding and a documentation gap.

Findings

🐛 Bugs / Logic Issues

• src/engine.rs (copilot_env) / src/data/base.yml + 1es-base.yml: permissions.read is now non-functional for agents that make direct ADO API calls. The old {{ copilot_ado_env }} conditionally injected AZURE_DEVOPS_EXT_PAT: $(SC_READ_TOKEN) and SYSTEM_ACCESSTOKEN: $(SC_READ_TOKEN) into the agent sandbox env when permissions.read was configured. The new {{ engine_env }} / copilot_env() always emits the same hardcoded vars with no ADO tokens. The token is still acquired (SC_READ_TOKEN pipeline var is set) but never forwarded to the agent. Agents relying on permissions.read without tools.azure-devops (e.g. using curl with AZURE_DEVOPS_EXT_PAT directly) will silently have no ADO access. If the intent is "always use the ADO MCP", a compile-time warning or error should be emitted when permissions.read is set without tools.azure-devops.

• AGENTS.md: The {{ copilot_ado_env }} documentation section is never removed or updated. After this PR it still says "Sets both AZURE_DEVOPS_EXT_PAT and SYSTEM_ACCESSTOKEN to $(SC_READ_TOKEN)" but the marker no longer exists in either template (renamed to {{ engine_env }}). Docs for {{ engine_env }} are also missing. This mismatch will confuse anyone trying to understand the template contract.

⚠️ Suggestions

• src/engine.rs:Engine::args() vs EngineConfig::args(): Two different args() methods with very different signatures and semantics — EngineConfig::args() -> &[String] returns raw CLI args from front matter, while Engine::args(...) -> Result<String> generates the full formatted params string. The Engine method would be less surprising as generate_cli_params() or generate_params().

• src/compile/extensions/mod.rs:CompileContext::for_test() hardcodes engine: Engine::Copilot (line 162). This is correct today, but when more engines are added the test context will need to respect front_matter.engine. Worth a // TODO: resolve from front_matter when multiple engines exist comment to flag the tech debt.

✅ What Looks Good

• Enum dispatch over trait objects — clean, zero-cost, and eliminates the GITHUB_COPILOT_CLI_ENGINE static singleton pattern. Correct move for a single-engine compile step.
• CompileContext::new() → Result<Self> — proper error propagation; unsupported engine IDs now fail at context construction rather than potentially deeper.
• run.rs scoping fix — compile_ctx was previously created inside the if use_mcpg block but then needed by copilot_params generation outside it. Lifting it out is the right fix.
• Model name validation preserved — shell-injection guard for model chars is correctly kept in copilot_args().
• Not-yet-wired field warnings — emitting eprintln! warnings for version, agent, api-target, args, env, command is the right pattern for forward-compatible schema adoption without silent no-ops.
• Test coverage — get_engine_rejects_unsupported(), get_engine_rejects_codex(), and the full test_engine_config_full_with_all_gh_aw_fields() test give good coverage of the new surface area.

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