Refactor compile extensions into module-based layout with per-extension impl files

[Copilot]

src/compile/extensions.rs had grown into a large multi-purpose file, making extension-specific changes harder to review and maintain. This PR reorganizes the extensions code into a module tree while preserving the same external API and behavior.

• Module entrypoint and exports

  • Replaced the monolithic file with src/compile/extensions/mod.rs.
  • Kept shared types/traits/collection logic centralized (CompilerExtension, Extension, CompileContext, MCPG config types, collect_extensions*, wrap_prompt_append).
  • Re-exported extension types from mod.rs to preserve call sites.

• Per-extension implementation split

  • Moved each extension implementation into its own file:
    • lean.rs
    • azure_devops.rs
    • cache_memory.rs
    • github.rs
    • safe_outputs.rs
  • No functional behavior changes were introduced; this is an organizational refactor.

• Test relocation

  • Moved extension-focused unit tests to src/compile/extensions/tests.rs under #[cfg(test)] mod tests; in mod.rs.
  • Coverage and assertions remain equivalent, now aligned with the new module structure.

// src/compile/extensions/mod.rs
mod azure_devops;
mod cache_memory;
mod github;
mod lean;
mod safe_outputs;

pub use azure_devops::{AdoAuthMode, AzureDevOpsExtension};
pub use cache_memory::CacheMemoryExtension;
pub use github::GitHubExtension;
pub use lean::LeanExtension;
pub use safe_outputs::SafeOutputsExtension;

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• dev.azure.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-c05a54f0ee13c0b7 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-c05a54f0ee13c0b7 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/async_trait-3589f01be3885149.a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-3bcc4ddce1e312d9.2ukf37jb0777k2y8ri9exqh0h.12vvtj1.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/async_trait-3589f01be3885149.a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-3bcc4ddce1e312d9.2ynn8gvql2fyvn7igwbexxwwe.12vvtj1.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/async_trait-3589f01be3885149.a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-3bcc4ddce1e312d9.32e9rgsalv16jx9z0iqp0mleg.12vvtj1.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/async_trait-3589f01be3885149.a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-3bcc4ddce1e312d9.374c5aulltrl0bl9xey67nr26.12vvtj1.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/async_trait-3589f01be3885149.a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-3bcc4ddce1e312d9.3adno12ab0g5nlfulsvspfcq8.12vvtj1.rcgu.o 0.03l0815d29lj002ae3mmxfl3m.09q1zyi.rcgu.o 0.0tn8ejcnvgjygdyylhtak9lpf.09q1zyi.rcgu.o 0.0xx92o0xvmao3s3mmpaude4gm.09q1zyi.rcgu.o 0.11wu39pl8x5unsfa0pm0sx7i8.09q1zyi.rcgu.o 0.15ekd7s10qhmgvd15js1u58j5.09q1zyi.rcgu.o 0.20695tlwm01eu06f6nytfaj15.09q1zyi.rcgu.o 0.25dmyxexlbrf6s096aq9lq99g.09q1zyi.rcgu.o 0.26zkdc197kx7li1nwob9t5xj9.09q1zyi.rcgu.o 0.2cvf4nkhwgdzrg6bb0j1g45xz.09q1zyi.rcgu.o 0.2figln0c2pfqzi2e08i006o8u.09q1zyi.rcgu.o 0.2fla2kplyo8fcs0of160c962a.09q1zyi.rcgu.o 0.2ikaohql56ne0w9lfnxyhwv2k.09q1zyi.rcgu.o 0.2oqlsydd3dffl94jckwj4qblh.09q1zyi.rcgu.o 0.2pfydlp0bjr74wlj1t3r6ms18.09q1zyi.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-c05a54f0ee13c0b7 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-c05a54f0ee13c0b7 --check-cfg cfg(docsrs,test) --check-cfg th.r�� th.rcgu.o 0.03l0815d29lj002ae3mmxfl3m.06lqajb.rcgu.o 0.0tn8ejcnvgjygdyylhtak9lpf.06lqajb.rcgu.o 0.0xx92o0xvmao3s3mmpaude4gm.06lqajb.rcgu.o 0.11wu39pl8x5unsfa0pm0sx7i8.06lqajb.rcgu.o 0.15ekd7s10qhmgvd15js1u58j5.06lqajb.rcgu.o 0.20695tlwm01eu06f6nytfaj15.06lqajb.rcgu.o 0.25dmyxexlbrf6s096aq9lq99g.06lqajb.rcgu.o 0.26zkdc197kx7li1nwob9t5xj9.06lqajb.rcgu.o 0.2cvf4nkhwgdzrg6bb0j1g45xz.06lqajb.rcgu.o 0.2figln0c2pfqzi2e08i006o8u.06lqajb.rcgu.o 0.2fla2kplyo8fcs0of160c962a.06lqajb.rcgu.o 0.2ikaohql56ne0w9lfnxyhwv2k.06lqajb.rcgu.o 0.2oqlsydd3dffl94jckwj4qblh.06lqajb.rcgu.o 0.2pfydlp0bjr74wlj1t3r6ms18.06lqajb.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-c05a54f0ee13c0b7 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-c05a54f0ee13c0b7 artifacts,futureremote 5.17unxb5laoekqiget-url stable-x86_64-unorigin remo�� get-url origin stable-x86_64-REDACTED-linux-gnu/bin/rustc 5.2paser1vd5pxuzgit artifacts,futureremote f/inquire-0.9.2/get-url stable-x86_64-unorigin 7cc2�� 7cc23-cgu.2.rcgu.o 7cc23-cgu.3.rcgu.o 7cc23-cgu.4.rcgu.o 7cc23-cgu.5.rcgubash -a8a48394b293b05--norc gu.o stup/toolchains/stable-x86_64-un-o (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[jamesadevine]

/rust-review

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean, well-structured refactoring with no functional regressions.

Findings

✅ What Looks Good

• All 10 CompilerExtension trait methods are delegated by extension_enum! — no accidental omissions (verified by inspection and test run).
• No unwrap()/expect() on production paths — confirmed by grep across all six new files.
• Stable sort preserves intra-phase ordering — sort_by_key(|e| e.phase()) is stable, so the "Runtimes before Tools" guarantee holds without any fragility.
• wrap_prompt_append heredoc injection is safe — all non-empty content lines are indented by 4 spaces; the heredoc terminator (NAME_EOF) requires an unindented match, so accidental early termination is impossible regardless of content.
• for_test / for_test_with_org correctly gated behind #[cfg(test)].
• Test coverage is equivalent — all 79 tests pass; moved tests in tests.rs use use super::*; which correctly imports parent-private items (Rust child-module visibility rules apply).
• AdoAuthMode re-exported from mod.rs — call sites don't need to know it's defined in azure_devops.rs.

⚠️ Suggestions

• src/compile/mod.rs:26 — pre-existing unused import: MCPG_PORT warning (not introduced by this PR, but visible in cargo check output). Worth a follow-up cleanup #[allow(unused_imports)] or removing the re-export.

• azure_devops.rs — required_pipeline_vars for AdoAuthMode::Pat is silent — PAT mode needs PERSONAL_ACCESS_TOKEN to reach the MCP container but returns vec![]. This appears intentional (local-dev path; the PAT is passed differently), but a brief comment explaining how it gets there would help future maintainers avoid incorrectly "fixing" it.

• extension_enum! macro placement — the macro is defined at line 307 but the types it wraps (LeanExtension, etc.) aren't declared until line 354. This works because macro invocation happens after mod declarations are processed, but it's a non-obvious ordering. A brief comment like // NOTE: submodules and pub use re-exports must appear before the macro invocation would clarify intent.

This is an exemplary refactoring PR — behaviour is unchanged, tests are complete, and the module structure makes future extension additions much cleaner.

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

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Clean refactoring — looks good with one minor test robustness observation.

Findings

✅ What Looks Good

• Module structure is idiomatic: mod.rs as the public entry point with re-exports, per-extension files, and a gated #[cfg(test)] mod tests; — textbook module organization.
• Static dispatch via extension_enum! macro: Avoids Box<dyn CompilerExtension> heap allocation with zero boilerplate at the call sites. The macro is defined before mod declarations and invoked after pub use re-exports, which is correct Rust — all types are in scope at the point of invocation (line 366).
• wrap_prompt_append is injection-safe: The heredoc uses a single-quoted delimiter (<< '{delimiter}'), which prevents shell variable expansion of any content passed through the prompt supplement. The display_name validation ([A-Za-z0-9 \-_] only) protects the echo and displayName embedding. ✔
• No panicking calls in non-test code: unwrap() / expect() only appear in tests and in the infer_ado_context log line (unwrap_or("?")), which is benign.
• Error handling follows project conventions: anyhow::bail!, .context(), and ? are used consistently throughout; all fallible paths return anyhow::Result.
• Test coverage: The relocated tests cover all extension variants, phase-ordering semantics, and edge cases (org missing, duplicate MCP entry, bash disabled with Lean, unsafe display names).

⚠️ Suggestions

• tests.rs:97-103 — silent pass risk in phase-ordering test: The assert! lives inside if let (Some(last_rt), Some(first_tool)) = .... Because the test already asserts exts.len() == 5 above it, both positions will always be Some in practice, but the guard makes the critical assertion silently skippable. A reader might not notice the check is conditional. Replacing the if let with direct unwrap()s would be cleaner and would panic loudly if the precondition ever breaks:

  let last_runtime_idx = exts
      .iter()
      .rposition(|e| e.phase() == ExtensionPhase::Runtime)
      .expect("expected at least one Runtime extension");
  let first_tool_idx = exts
      .iter()
      .position(|e| e.phase() == ExtensionPhase::Tool)
      .expect("expected at least one Tool extension");
  assert!(last_runtime_idx < first_tool_idx, ...);

No bugs, security concerns, or correctness issues found. The 1:1 line ratio (1209 removed / 1210 added) confirms this is a pure reorganization.

Generated by Rust PR Reviewer for issue #285 · ● 920.7K · ◷

[jamesadevine]

@copilot address suggestion above.

[Copilot]

@copilot address suggestion above.

Addressed in b95c9f8. I updated the phase-ordering test to remove the conditional if let and use explicit expect(...) checks with an unconditional ordering assertion so the test can’t silently pass.