refactor: redesign CompileContext with rich metadata and async constructor

[jamesadevine]

Summary

Redesigns CompileContext to follow the ExecutionContext pattern — a single struct built eagerly with all resolved metadata, passed uniformly to every extension method.

Problem

CompileContext.inferred_org was:

• None at validate() time (not yet resolved)
• Only populated later when generate_mcpg_config() was called
• Resolved inline in standalone.rs::compile() as a 30-line block that was tightly coupled to the ADO extension

Solution

pub struct CompileContext<'a> {
    pub agent_name: &'a str,
    pub front_matter: &'a FrontMatter,
    pub ado_context: Option<AdoContext>,  // was: inferred_org: Option<&str>
    pub compile_dir: &'a Path,         // new: for future path resolution
}

• CompileContext::new().await — async constructor that infers ADO context from git remote
• ctx.ado_org() — convenience accessor for the org name
• Built once, early in both standalone.rs and onees.rs
• All extension methods (validate, mcpg_servers) receive the same fully-populated context

Changes

File	Change
extensions.rs	Expanded CompileContext with ado_context, compile_dir, async fn new(), test helpers
standalone.rs	Removed 30-line inline org inference block, builds context once early
onees.rs	Builds CompileContext::new().await (same as standalone)

All 842 tests pass.

[github-actions]

🔍 Rust PR Review

Summary: Clean refactor with one notable behavioral change and a minor dead-field concern — otherwise looks good.

Findings

⚠️ Suggestions

• src/compile/extensions.rs:112 — Unconditional ADO context inference: CompileContext::new() always calls infer_ado_context() (which shells out to git remote get-url origin) even when tools.azure-devops is not configured. The old code in standalone.rs was intentionally conditional — it only ran the git inference when azure_devops.is_enabled() && org().is_none(). Now every compile pays this async overhead regardless. For pipelines without ADO tool enabled, this is wasteful. Consider either (a) lazy evaluation — infer only when ado_org() is first called, or (b) keeping the conditional check in new():

  let ado_context = if front_matter.tools.as_ref()
      .and_then(|t| t.azure_devops.as_ref())
      .is_some_and(|ado| ado.is_enabled() && ado.org().is_none())
  {
      Self::infer_ado_context(compile_dir).await
  } else {
      None
  };

• src/compile/extensions.rs:103 — compile_dir is stored but never read: The field is added for "future path resolution" but ctx.compile_dir is never accessed by any trait implementation or caller in this PR. It's pub so it won't warn at the call sites, but it's dead weight in the current state. Either use it now or remove it until it's needed — otherwise it creates a false impression of available context.

• src/compile/extensions.rs:129 — unwrap_or("") in ado_org() is unreachable: rsplit('/').next() on any &str (even "") always returns Some(...) — rsplit always yields at least one item. The .filter(|org| !org.is_empty()) correctly handles the empty-URL case, so correctness is preserved, but unwrap_or("") can never fire. Use .unwrap_or("") if you want to keep the defensive coding style, but it's worth a comment, or simply chain with ? in a helper method to make the intent clearer.

✅ What Looks Good

• Removing the validate()-vs-mcpg_servers() split (where the doc comment used to warn that org wasn't available at validate time) is the right fix — the API is now coherent.
• for_test() / for_test_with_org() helpers are a clean pattern that avoid scattered struct literal construction across 10+ test callsites.
• infer_ado_context() correctly silences errors as None with log::debug! — the right behavior since ADO is optional.
• The org validation (!org.chars().all(|c| c.is_ascii_alphanumeric() || c == '-')) is preserved unchanged after the refactor.

Generated by Rust PR Reviewer for issue #212 · ● 895.5K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Clean refactoring — looks good with a couple of minor observations worth noting for future work.

Findings

✅ What Looks Good

• Latent bug fixed in onees.rs: The old compiler constructed CompileContext { inferred_org: None } for validation in both compilers. The 1ES compiler never had org context during validate(). Now that CompileContext::new().await is called before validation in both paths, any org-dependent validation will work correctly in both targets.
• Clean error chaining: Replacing the manual if/else if/else with .or_else(|| ...).ok_or_else(|| anyhow::anyhow!(...))? at extensions.rs:446–461 is idiomatic and reads more clearly.
• for_test / for_test_with_org helpers: Good ergonomics. Eliminates repetitive struct-literal setup across ~15 test sites.
• 30-line org-inference block removed from standalone.rs: The logic now lives in one place (CompileContext::infer_ado_context) rather than being duplicated per-compiler.

---

⚠️ Suggestions

• extensions.rs:121 — infallible ?: str::rsplit('/').next() is infallible: a rsplit iterator always yields at least one element (even for an empty string, it yields ""). The ? operator is dead code here. The empty-string case is already handled by the subsequent if org.is_empty() { None }. Not a bug, but the ? implies a failure mode that can't occur, which can mislead future readers:

  // current
  let org = ctx.org_url.trim_end_matches('/').rsplit('/').next()?;
  // clearer intent
  let org = ctx.org_url.trim_end_matches('/').rsplit('/').next().unwrap_or("");
  if org.is_empty() { return None; }
  Some(org)

• Unconditional git subprocess on every compile: Previously in standalone.rs, get_git_remote_url was only called when tools.azure-devops was enabled and no explicit org: was set. Now CompileContext::new() always shells out to git remote get-url origin on every compile, even for pipelines that don't use azure-devops. The extra subprocess is fast and the error is swallowed gracefully, but it's a behavioural change worth documenting. Consider a note in the new() doc comment.

• PR description / code mismatch: The PR description lists compile_dir: &'a Path as a new struct field ("new: for future path resolution"), but it was dropped from the final CompileContext struct (correctly — it's only needed by the constructor). The description is misleading if anyone references it later.

• for_test_with_org hardcodes project/repo: project: "test-project" and repo_name: "test-repo" are fine for current consumers (all callers only inspect ado_org() → org_url). If a future extension accesses ctx.ado_context.project or .repo_name in mcpg_servers(), tests using this helper would silently pass with wrong data. Consider adding a for_test_with_context(fm, org, project, repo) overload or a builder when that becomes necessary.

Generated by Rust PR Reviewer for issue #212 · ● 871K · ◷