/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 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 · ◷

✅ Rust PR Reviewer completed successfully!

🔍 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 · ◷

@copilot address suggestion above.

@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.