Refactor Engine dispatch match arms with a single macro

[Copilot]

PR #307 identified repetitive Engine::Copilot match arms in src/engine.rs that were good candidates for further consolidation. This change applies a macro-based dispatch pattern to reduce duplication while keeping behavior and public APIs unchanged.

• Dispatch consolidation

  • Added a local dispatch_engine! macro that centralizes enum-variant dispatch for Engine.
  • Keeps static enum dispatch explicit while removing repeated match self { Engine::Copilot => ... } blocks.

• Methods refactored to macro dispatch

  • Updated these methods to use the macro:
    • command
    • args
    • env
    • log_dir
    • required_hosts
    • install_steps
    • invocation

• Maintenance impact

  • Reduces repeated boilerplate in engine-specific routing.
  • Makes future engine-variant expansion more mechanical and less error-prone by localizing dispatch structure.

macro_rules! dispatch_engine {
    ($engine:expr, $copilot:expr) => {
        match $engine {
            Engine::Copilot => $copilot,
        }
    };
}

// before: match self { Engine::Copilot => copilot_env(engine_config) }
pub fn env(&self, engine_config: &EngineConfig) -> Result<String> {
    dispatch_engine!(self, copilot_env(engine_config))
}

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Functionally correct, but the macro design creates a maintenance trap that makes future engine expansion harder, not easier — contrary to the stated goal.

Findings

⚠️ Suggestions

• src/engine.rs:60-66 — Macro design works against its stated purpose

  The PR description says the macro makes "future engine-variant expansion more mechanical and less error-prone." In practice it does the opposite.

  With the current explicit match arms, adding Engine::Gemini requires adding one arm per method — Rust's exhaustiveness checker enforces this at every call site:

  pub fn command(&self) -> &str {
      match self {
          Engine::Copilot => "copilot",
          Engine::Gemini => "gemini",  // compiler forces you to add this
      }
  }

  With the macro, adding a new variant requires both updating the macro signature and updating every single call site with an additional positional argument:

  // macro must grow a new parameter:
  macro_rules! dispatch_engine {
      ($engine:expr, $copilot:expr, $gemini:expr) => { ... }
  }
  // every call site must be updated:
  dispatch_engine!(self, "copilot", "gemini")
  dispatch_engine!(self, copilot_args(...), gemini_args(...))
  // ...7 call sites total

  That's a more invasive, error-prone change than just appending a match arm. The $copilot-named parameter also locks in the assumption that Copilot is variant Initial compiler PR #1 — a leaky abstraction detail.

  Recommendation: The cleanest path for future extensibility would be a trait (EngineBackend) or leaving the explicit match arms in place. If the goal is simply reducing repetitive match self { Engine::Copilot => ... } boilerplate while keeping enum dispatch, the macro as written trades one boilerplate for another that's harder to extend.

• src/engine.rs:157-172 — bail! inside a macro block (informational)

  anyhow::bail! expands to return Err(...), so it returns from the enclosing function even when nested inside the dispatch_engine! macro block. This is correct and works as intended — just worth noting for reviewers unfamiliar with how bail! interacts with macros.

✅ What Looks Good

• All Result-returning methods continue to use ? and bail! with descriptive error messages — no regressions in error handling quality.
• The macro correctly preserves exhaustiveness checking: adding a new Engine variant without updating the macro's match arm will still produce a compile error (though the error message will point into the macro expansion, making it slightly noisier to diagnose).
• Net change is +38/-40 lines — minimal scope, no unrelated modifications.

Generated by Rust PR Reviewer for issue #309 · ● 375.5K · ◷

[jamesadevine]

Yes agree