refactor: unify runtime/tool requirements via CompilerExtension trait

[jamesadevine]

Summary

Introduces a CompilerExtension trait that runtimes and first-party tools implement to declare their compilation requirements. Replaces scattered special-case if blocks across the compiler with a single generic collection loop.

Problem

Adding a new runtime or tool required touching 5+ files with bespoke integration code:

• standalone.rs — network hosts, MCPG config, prepare steps, prompt injection
• common.rs — bash command allowlist
• allowed_hosts.rs — domain declarations
• types.rs — config structs

Solution

A unified CompilerExtension trait (src/compile/extensions.rs) with methods:

Method	Purpose
required_hosts()	AWF network allowlist
required_bash_commands()	--allow-tool shell(cmd) flags
prompt_supplement()	Agent prompt markdown sections
prepare_steps()	Pipeline steps (install, download)
mcpg_servers()	MCPG server entries
validate()	Compile-time warnings/errors

Extensions implemented

• LeanExtension — hosts, bash cmds (lean/lake/elan), install steps, prompt, bash-disabled warning
• AzureDevOpsExtension — hosts, MCPG container entry (with org inference), duplicate MCP warning
• CacheMemoryExtension — download/restore steps, prompt supplement

Adding a new runtime or tool

1. Implement CompilerExtension for your config struct
2. Add a collection check in collect_extensions()

No other files need modification.

Regression testing

Built v0.11.0 baseline and compared pipeline YAML output:

Example	Result
sample-agent.md (no extensions)	Byte-identical ✅
lean-verifier.md (Lean + cache-memory)	Functionally identical ✅ (cosmetic: extension ordering, heredoc delimiter names)

Changes

• New: src/compile/extensions.rs (trait, 3 impls, MCPG types, 19 tests)
• Modified: standalone.rs (−180 lines of special-case code → generic loops)
• Modified: common.rs (generate_copilot_params accepts extensions)
• Modified: onees.rs (passes extensions to generate_copilot_params)
• Modified: AGENTS.md (updated developer docs)

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean refactor with solid test coverage. Two issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/onees.rs:65-66 — OneESCompiler calls collect_extensions() (for bash command injection) but never runs the validation loop that StandaloneCompiler runs. Warnings like "Lean requires bash access" and "duplicate azure-devops MCP" are silently dropped for all target: 1es agents. The validation block from standalone.rs (for ext in &extensions { for warning in ext.validate(&ctx)? { ... } }) should be added to onees.rs as well.

⚠️ Suggestions

• src/compile/extensions.rs — wrap_prompt_append — display_name is embedded unsanitized in two places: a bash echo statement and a YAML displayName field. All current extension names are hardcoded alphanumeric strings so there's no bug today, but there's no guard against a future extension whose name() contains ", $(), or :. Adding a debug_assert!(!display_name.contains('"')) or a compile-time validation in validate() (checking ext.name()) would make this explicit. The same fragility applies to the heredoc delimiter — if a future name() produces the same string as the delimiter on a content line, the heredoc silently truncates.

• src/compile/extensions.rs — CacheMemoryExtension.config — #[allow(dead_code)] on the config field means that any CacheMemoryToolConfig options (e.g., allowed-extensions) are ignored at compile time and consumed only in Stage 2. This is fine, but the comment saying "options are applied at execution time" (or similar) would help the next person adding options to the config not expect them to be available in the extension.

✅ What Looks Good

• The CompilerExtension trait design is clean — good defaults on all methods make simple extensions trivial to write, and the validate() / mcpg_servers() split (warnings vs. generation) is a sensible separation.
• The generalization of the skip-if-already-configured logic (if mcp_servers.contains_key(name)) is strictly better than the old hardcoded ADO_MCP_SERVER_NAME check.
• The bash command deduplication after switching from Vec<&str> to Vec<String> (!bash_commands.contains(&cmd)) is correct.
• 19 tests in extensions.rs cover the happy paths and key error cases (no-org failure, duplicate MCP warning, bash-disabled warning) well.

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

[github-actions]

🔍 Rust PR Review

Summary: Clean refactor with one security concern and one behavioral change worth noting.

---

Findings

🔒 Security Concerns

• extensions.rs:~810 — debug_assert! is a no-op in release builds

  debug_assert!(
      display_name.chars().all(|c| c.is_ascii_alphanumeric() || matches!(c, ' ' | '-' | '_')),
      "Extension display_name '{}' contains characters unsafe for bash/YAML embedding",
      display_name
  );

  The generated step includes display_name in an unquoted echo "..." call and in the YAML displayName: field. If a future extension's name() returns something like My "Ext" or ext$(rm -rf), this check is silently skipped in release, producing broken or injectable pipeline YAML. Since wrap_prompt_append is pub, it's callable from new extension implementations outside the current three.

  Suggested fix: replace with a runtime guard:

  anyhow::ensure!(
      display_name.chars().all(|c| c.is_ascii_alphanumeric() || matches!(c, ' ' | '-' | '_')),
      "Extension display_name '{}' contains characters unsafe for bash/YAML embedding",
      display_name
  );

  This makes the function return Result<String> (or alternatively panic! + a doc comment saying callers must use only safe names — but Result is safer and more consistent with the codebase convention).

---

⚠️ Suggestions

• extensions.rs + standalone.rs — prepare step ordering changed (Lean → Memory)

  Old order in generate_prepare_steps:

  1. Memory download/restore
  2. Lean install

  New order via collect_extensions (runtimes before tools):

  1. Lean install
  2. Memory download/restore

  The PR description notes this as "cosmetic," and the change is functionally harmless today (Lean install doesn't need memory). However the reversed order is now silently imposed by the collect_extensions definition order rather than any explicit policy. Consider a brief comment in collect_extensions explaining that runtimes run before tools and the implications for step ordering, to make future extension authors aware.

• standalone.rs:~87 + onees.rs:~74 — two-phase validation pattern

  validate() is called with inferred_org: None (before org inference happens), then mcpg_servers() is called later with the actual org. The comment // Not yet available; ADO org validation happens in mcpg_servers() explains the intent, but it means AzureDevOpsExtension::validate() silently returns no errors even if there's no org available. Someone reading the validate() call at the top of compile() would reasonably expect it to be exhaustive.

  This is not a bug today, but the split between the two phases is non-obvious. A short doc comment on the validate trait method noting it doesn't have access to inferred_org (so org-dependent errors go in mcpg_servers()) would help future maintainers.

---

✅ What Looks Good

• Static dispatch via extension_enum! macro — avoids heap allocation for Box<dyn CompilerExtension> while keeping the dispatch table compact. Nice choice.
• Security validations preserved faithfully — org name and toolset character validation (is_ascii_alphanumeric() || c == '-') migrated correctly from standalone.rs into AzureDevOpsExtension::mcpg_servers().
• Generic MCP skip logic — replacing if name == ADO_MCP_SERVER_NAME && mcp_servers.contains_key(ADO_MCP_SERVER_NAME) with if mcp_servers.contains_key(name) is a clean generalization that correctly handles future extensions that also contribute MCPG entries.
• 19 focused unit tests in extensions.rs with good coverage of the happy path and error cases (org absent, duplicate MCP, bash-disabled warning).
• generate_memory_download content is byte-for-byte identical to the original; the heredoc delimiter name change (CACHE_MEMORY_EOF vs MEMORY_PROMPT_EOF) is the only cosmetic difference.

Generated by Rust PR Reviewer for issue #211 · ● 1.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Clean, well-structured refactor — looks good with a few minor issues worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/extensions.rs — wrap_prompt_append heredoc structural correctness: The function is correct in isolation, but there's no test that verifies the heredoc delimiter (LEAN_4_EOF etc.) actually lands at column 0 after YAML block scalar parsing. The current test only asserts .contains("TEST_FEATURE_EOF"). The logic works because empty lines in indented_content are kept truly empty (not 4 spaces), so YAML's 4-space strip produces a column-0 closing delimiter — but this fragile invariant is unguarded. A test asserting the format string can be parsed as YAML and executed as a valid bash heredoc would lock this in.

⚠️ Suggestions

• src/runtimes/lean.rs:120 — generate_lean_prompt is dead code: LeanExtension::prepare_steps() now calls lean::generate_lean_install but prompt_supplement() returns content directly (wrapped by wrap_prompt_append). generate_lean_prompt() is no longer called anywhere. Because it's pub, neither the compiler nor clippy catches this. Should be removed (or #[allow(dead_code)] if kept for external consumers, though nothing in the codebase uses it).

• src/compile/extensions.rs and src/compile/onees.rs — false-positive duplicate MCP warning in 1ES mode: AzureDevOpsExtension::validate() runs in both standalone and 1ES compilers (via the shared validation loop added in onees.rs). In 1ES mode, mcpg_servers() is never called on extensions, so no MCPG config is generated regardless. If a user has both tools.azure-devops: true and mcp-servers.azure-devops: in a 1ES pipeline, they'll see "tools.azure-devops takes precedence" — but neither entry actually produces anything. Consider gating this warning behind a flag in the CompileContext indicating the target, or scoping it to standalone only.

• AGENTS.md — docs/code mismatch on trait definition: The newly added CompilerExtension trait documentation in AGENTS.md shows pub trait CompilerExtension: Send, but the actual trait in src/compile/extensions.rs:129 is pub trait CompilerExtension { with no Send bound. If Send is intentionally not required (extensions are only used single-threaded during compilation), remove it from the docs.

✅ What Looks Good

• The extension_enum! macro is elegant — eliminates delegation boilerplate without runtime cost (static dispatch preserved).
• ExtensionPhase with Ord + sort_by_key (stable sort) is the right approach for deterministic ordering.
• wrap_prompt_append validates display_name correctly (rejects ", $, `) before embedding in both the bash echo and the YAML displayName.
• Single-quoted heredoc delimiter << 'LEAN_4_EOF' correctly prevents $VAR and ##vso[ expansion in prompt content.
• The CompileContext.inferred_org: None + doc note guiding ADO org validation to mcpg_servers() is a clean solution to the async-timing problem.
• All 49 tests + 8 MCP HTTP tests still pass after the refactor.

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