feat: unify standalone and 1ES compilers

[jamesadevine]

Summary

Unifies the standalone and 1ES compilers to share the same execution model (Copilot CLI + AWF + MCPG) and compilation flow. Replaces the legacy Agency job type in 1ES with direct Copilot CLI invocation.

Compiler unification

• Extracted compile_shared() + CompileConfig into common.rs — both compilers are now thin wrappers that provide target-specific values and delegate to the shared function
• CompileConfig.extra_replacements are applied before shared replacements, allowing targets to override shared markers (e.g., 1ES-specific setup/teardown jobs)
• compile_shared() accepts a pre-built &CompileContext to avoid duplicate git I/O
• Moved all shared helpers to common.rs: generate_setup_job, generate_teardown_job, generate_prepare_steps, generate_finalize_steps, generate_agentic_depends_on, generate_mcpg_config, generate_mcpg_docker_env, generate_allowed_domains, and all MCP validation functions

1ES modernisation (breaking change for 1ES users)

• Replaced templateContext.type: agencyJob with buildJob running Copilot CLI directly
• Added AWF network isolation and MCP Gateway to 1ES pipelines
• Dropped Agency-specific concepts: commandOptions, globalOptions, logLevel, mcpConfiguration, agentContextRoot, AgencyArtifact
• 1ES template now has the same 3-job pipeline as standalone (PerformAgenticTask → AnalyzeSafeOutputs → ProcessSafeOutputs) with identical step sequences

Bug fixes

• Fixed generate_repositories: removed hardcoded indentation, now outputs flat YAML for replace_with_indent to handle
• Fixed generate_checkout_steps: same hardcoded indentation fix
• Fixed generate_teardown_job: matched the generate_setup_job pattern (flat output, template provides indentation context)
• Fixed 1ES generate_setup_job/generate_teardown_job: corrected step indentation to match templateContext.steps nesting depth

Tests

• Added YAML validation integration tests for all compiled fixtures (1ES, minimal, complete, pipeline-trigger)
• Strengthened 1ES test with content assertions: verifies Copilot CLI, AWF, MCPG, SafeOutputs, all three job names, and absence of Agency remnants
• Standalone output verified byte-identical to pre-refactor baseline (via temp golden snapshots, not committed)

File sizes (before → after)

File	Before	After
standalone.rs	2100 lines	254 lines
onees.rs	572 lines	188 lines
common.rs	2573 lines	4432 lines
1es-base.yml	348 lines	711 lines

878 tests pass, cargo clippy clean

[github-actions]

🔍 Rust PR Review

Summary: Solid refactor with good test coverage — one minor performance bug, one fragile architecture pattern, and a missing golden test for the 1ES behavioral change.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone.rs:50, src/compile/onees.rs:62 — Double CompileContext::new() per compilation. Both callers build a context before calling compile_shared, and compile_shared builds a second one (common.rs:1952). CompileContext::new() spawns git remote get-url origin (async process), so this doubles the git I/O on every ado-aw compile invocation. Fix: pass the pre-built ctx into compile_shared (or pass mcpg_config in CompileConfig instead of having callers compute it before calling compile_shared).

⚠️ Suggestions

• src/compile/onees.rs:145-158 — The 1ES compiler pre-replaces {{ setup_job }} and {{ teardown_job }} in the template before handing it to compile_shared. compile_shared then generates standalone versions (with pool:) and tries to replace the same markers — silently becoming a no-op since the markers are already consumed. This works today but is fragile: the design intent of CompileConfig.extra_replacements exists precisely for this kind of target-specific override. Consider moving the 1ES-specific setup/teardown YAML into extra_replacements (pre-keyed as {{ setup_job }} / {{ teardown_job }}) and having compile_shared apply extra_replacements before its own shared replacements (or adding a distinct marker pair for 1ES jobs). The current approach creates an implicit ordering dependency that isn't obvious to future contributors.

• tests/ — The PR description says "Standalone output verified byte-identical to pre-refactor baseline". No equivalent golden snapshot exists for the 1ES output. Given that the 1ES compiler went from agencyJob to buildJob (a complete behavioral change), a tests/fixtures/expected/1es-test-agent.yml snapshot would catch regressions the current YAML-validity and no-unreplaced-markers tests won't. The test_1es_compiled_output_is_valid_yaml test only validates structural correctness; it wouldn't catch, e.g., a {{ copilot_params }} regression in the new 1ES template.

✅ What Looks Good

• The compile_shared / CompileConfig extraction is clean. Both target compilers are now correct thin wrappers, and compile_shared centralises all validations (permissions, work-item targets, PR review events, etc.) that were previously duplicated.
• Indentation regressions introduced during the initial extraction (generate_repositories, generate_checkout_steps, teardown_job) are correctly fixed in the final commit 9c84924 — the replace_with_indent approach is strictly cleaner than baking template-specific spaces into generator functions.
• The Docker args/mount security validators (validate_docker_args, validate_mount_source, validate_container_image) with their unit tests are well-written, including edge cases like /etc-configs not matching /etc, and --volume= equals-form detection.
• No unwrap()/expect() on user-facing paths; all fallible paths use ? with .context(). The is_valid_env_var_name guard for Docker -e injection is the right place to enforce this.

Generated by Rust PR Reviewer for issue #226 · ● 7.5M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Excellent refactor overall — clean unification, good test coverage. Two actionable issues found, one minor doc nit.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs:81–83 — Stale doc comment. The lines "Generate a schedule YAML block from a ScheduleConfig. When no explicit schedule branches are configured, defaults to \main`."are orphaned from the adjacentgenerate_schedule()function and were accidentally prepended to thegenerate_parameters()` doc comment during the refactor. They should be removed.

🔒 Security Concerns

• src/compile/common.rs:895 — Service connection name embedded in a single-quoted YAML string without sanitization:

  lines.push(format!("    azureSubscription: '{}'", sc));

  If a service connection name contains a single quote (e.g. my-conn's-sc), the emitted YAML will be malformed and the pipeline will fail at parse time. ADO allows arbitrary display names for service connections. Either validate the name at compile time (reject if it contains ') or escape it:

  lines.push(format!("    azureSubscription: '{}'", sc.replace('\'', "''")));

  Same pattern applies to the generate_acquire_write_token() counterpart.

⚠️ Suggestions

• AGENTS.md (resolve-pr-thread section) — The documentation refers to the tool as resolve-pr-thread, but the actual MCP registration in src/mcp.rs:1036 and the executor dispatch in src/execute.rs:381 both use resolve-pr-review-thread. The validation function validate_resolve_pr_thread_statuses() in common.rs:1186 also correctly uses "resolve-pr-review-thread" as the front matter key. The AGENTS.md tool name and example YAML (resolve-pr-thread:) should be updated to resolve-pr-review-thread to match.

✅ What Looks Good

• The CompileConfig.extra_replacements override-before-shared pattern is an elegant way to let 1ES supply its own {{ setup_job }}/{{ teardown_job }} implementations (using templateContext: type: buildJob) without any conditional branching in compile_shared().
• Template marker exhaustiveness is solid — all {{ marker }} occurrences in both base.yml and 1es-base.yml are handled, either in compile_shared() or in the target-specific extra_replacements.
• The CompilerExtension trait integration is correctly threaded through — extensions contribute required_hosts, required_bash_commands, prepare_steps, and MCPG servers all in one place.
• Error handling is consistently anyhow::Result with actionable .context() messages; no user-facing unwrap() calls.
• The new 1ES test suite (test_1es_compiled_output_no_unreplaced_markers, test_1es_compiled_output_is_valid_yaml) gives solid regression coverage for the rewritten template.

Generated by Rust PR Reviewer for issue #226 · ● 3.7M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Clean refactoring with solid architecture — the shared compilation model works correctly. A few dead-code items were left behind, and the "cargo clippy clean" claim in the PR description doesn't hold.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs:2089 — let mut pipeline_yaml is declared mutable but never reassigned after the fold. Clippy flags this as unused_mut. The mut is harmless but misleading.

• src/compile/common.rs:627,663 — format_step_yaml and format_steps_yaml (without _indented) are now dead code. Both functions were previously called from the old 1ES teardown codepath (removed in this PR) but were left in. Clippy warns dead_code on both.

• src/compile/common.rs:~2074 — ("{{ agent }}", &agent_name) in the shared replacements list never matches anything in either template (both use {{ agent_name }}). This silent no-op was presumably left over from an earlier design. Not a runtime error, but misleading.

• src/compile/mod.rs:21,23 — Two unused re-exports introduced after refactoring: pub use common::sanitize_filename and PermissionsConfig. Clippy warns on both.

⚠️ Suggestions

• src/compile/common.rs (compile_shared, step 8) — For the 1ES target, generate_setup_job and generate_teardown_job from common.rs are computed but their results are silently discarded (1ES overrides them via extra_replacements applied first). The compute is harmless, but a comment noting the intentional override would clarify the design for future readers (as there's already a comment in onees.rs but none in compile_shared itself).

• Chained substitution ordering — generate_source_path and generate_pipeline_path embed a literal {{ workspace }} in their return values (e.g. {{ workspace }}/agents/foo.md). This works because {{ source_path }} is replaced before {{ workspace }} in the shared replacements fold (indices 21 vs 24). This is correct today but silently order-dependent — if those entries are ever reordered, {{ workspace }} would survive into the output. A brief comment explaining this dependency would help.

✅ What Looks Good

• Architecture: The CompileConfig.extra_replacements applied before shared replacements is an elegant mechanism — 1ES can override {{ setup_job }}/{{ teardown_job }} without any if target == 1es conditionals in compile_shared.
• Marker coverage: All 30 markers used in the new 1ES template are handled (verified by cross-referencing extra_replacements + shared replacements list). No dangling {{ marker }} placeholders remain in compiled output.
• Sanitization preserved: front_matter.sanitize_config_fields() is correctly called in mod.rs before dispatch, so both targets inherit the same sanitization guarantees.
• New YAML validation tests: The integration tests that parse compiled output as YAML (rather than just grepping) are a good addition — they would catch indentation regressions that string checks miss.
• Security validations: All six validate_* calls are preserved in compile_shared and apply to both targets equally.

---

Note: cargo clippy currently emits 61 warnings. The PR description's claim of "clippy clean" appears to have been inaccurate — at minimum unused_mut, two dead_code items, and two unused_imports were introduced by this PR.

Generated by Rust PR Reviewer for issue #226 · ● 3.6M · ◷