🔍 Rust PR Review

Summary: Good feature addition overall — clean architecture, proper trait implementation, and solid test coverage. Three issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/job.rs:147 and src/compile/stage.rs:124 — repos: entries silently dropped from header docs

  The condition correctly checks both front_matter.repos and front_matter.repositories, but the documentation loop only iterates front_matter.repositories (legacy field). When a user uses the modern repos: field, the header emits the scaffolding comment block with no actual repo entries:

  // Condition fires on new-style repos:
  if !front_matter.repos.is_empty() || !front_matter.repositories.is_empty() {
      ...
      for repo in &front_matter.repositories { // ← never iterates repos entries

  The repos field uses ReposItem (string shorthand or object). After lower_repos() is called, the lowered form is in front_matter.repositories, but at header-generation time that lowering may not have run yet on the original FrontMatter. Worth either lowering first or generating the docs from the repos field directly.

• src/compile/job.rs:136 and src/compile/stage.rs:113 — .replace(".md", ".lock.yml") corrupts directory names

  source_path.replace(".md", ".lock.yml")

  This replaces all occurrences of .md in the path string. A path like markdown-docs/my-agent.md becomes lock.yml-docs/my-agent.lock.yml. Should use Path-aware extension replacement:

  input_path.with_extension("lock.yml").to_string_lossy().replace('\\', "/")

⚠️ Suggestions

• src/compile/common.rs:1009 — generate_stage_prefix preserves Unicode alphanumerics

  The function uses Rust's Unicode-aware char::is_alphanumeric(), so agent names with non-ASCII letters (e.g. "über-agent" → "ÜberAgent") pass through and produce ADO job names that fail ADO's [A-Za-z0-9_] identifier requirement at pipeline queue time. Consider filtering to ASCII only:

  .split(|c: char| !c.is_ascii_alphanumeric())

  And in the leading-digit guard, starts_with(|c: char| c.is_ascii_digit()) is already correct — just the split needs the is_ascii_ variant.

✅ What Looks Good

• Clean factoring of shared compilation logic via skip_header: bool in CompileConfig — minimal diff on existing compilers.
• The extra_replacements mechanism correctly threads {{ stage_prefix }} and {{ template_parameters }} through compile_shared without needing new trait methods.
• Job/stage dependency chain in both templates correctly gates _Execution on both _Agent and _Detection outputs.
• Bash lint tests properly extended to cover both new targets in REQUIRED_TARGETS.
• The generate_stage_prefix fallback to "Agent" and leading-digit prefix guard are sensible ADO safety rails.

Generated by Rust PR Reviewer for issue #519 · ● 921.3K · ◷

🔍 Rust PR Review

Summary: Solid implementation with good marker coverage and test infrastructure — two things worth addressing before merge.

Findings

⚠️ Suggestions

• docs/template-markers.md not updated (src/compile/common.rs, src/data/job-base.yml, src/data/stage-base.yml): Two new template markers are introduced — {{ stage_prefix }} and {{ template_parameters }} — that are exclusive to the new template targets. Per project convention in AGENTS.md: "New template markers are documented in docs/template-markers.md". Both markers need entries describing their source and semantics.

• Significant code duplication (src/compile/job.rs, src/compile/stage.rs): The compile() implementations are essentially identical — same extensions collection, same MCPG config generation, same CompileConfig construction, and identical extra_replacements list. Only the template file and the header generation function differ. A shared private helper (e.g., compile_template_target(template: &str, header_fn, ...)) would halve the maintenance surface and reduce the risk of the two compilers diverging silently. This is particularly notable because stage.rs already has the same generate_stage_header name pattern but independently reimplements everything.

• info!("Using {} compiler", ...) removed without replacement (src/compile/mod.rs): The removed log applied to all targets. The two new compilers add their own info!() in compile(), but StandaloneCompiler and OneESCompiler now log nothing about which compiler path was selected. Easy fix: either keep the mod.rs log or add matching info! calls to the existing compilers.

• Silent non-ASCII truncation in generate_stage_prefix (src/compile/common.rs:1007): "über-agent" silently produces "BerAgent", dropping the initial character. This is documented in the function-level doc comment and tested, but a user compiling an agent with a non-ASCII name gets no warning. A warn!() when the resulting prefix differs from what a naive strip would suggest would help — though this is low-priority given the ADO identifier constraint is real.

✅ What Looks Good

• Template marker coverage is complete: every {{ marker }} in both job-base.yml and stage-base.yml is accounted for — either in extra_replacements or handled by compile_shared's standard replacement table. No leftover markers to worry about.
• serde deserialization is correct: #[serde(rename_all = "lowercase")] on CompileTarget means target: job and target: stage deserialize correctly without explicit #[serde(rename)] attributes.
• generate_stage_prefix output is safe: the split-on-non-alphanumeric logic guarantees the output only ever contains [A-Za-z0-9_], making it injection-safe for both YAML job names and ADO condition expressions.
• Test coverage is thorough: seven unit tests for generate_stage_prefix covering edge cases (empty, leading digit, unicode), fixture files for both targets, and both targets added to REQUIRED_TARGETS in bash_lint_tests.rs.
• Error handling follows project conventions: anyhow::Result throughout, .context() on fallible calls, no silent unwrap() on user-facing paths.

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

@copilot implement suggestions

@copilot implement suggestions

Implemented all suggestions from both review passes in commit 5f9e197:

• Code duplication removed: Extracted compile_template_target() shared helper and TemplateTargetConfig struct into common.rs. Both JobCompiler and StageCompiler now delegate to it — each reduced from ~115 lines to ~45 lines.
• Non-ASCII warning: generate_stage_prefix() now emits a warn!() when Unicode alphanumeric characters are silently stripped from the ADO job name prefix.
• Compiler log restored: info!("Using {} compiler", ...) is back in mod.rs so all four targets log consistently.
• Docs updated: docs/template-markers.md now documents {{ stage_prefix }} and {{ template_parameters }} with derivation rules and example output, and the Output Format section lists all four compile targets.

🔍 Rust PR Review

Summary: Clean pass — the formatting-only changes in this diff are correct and the prior review concerns are fully resolved.

Findings

✅ What Looks Good

• All prior issues addressed: Both earlier review passes flagged code duplication, missing docs/template-markers.md entries, the Unicode stripping in generate_stage_prefix, and the compiler log regression — all confirmed fixed:

  • compile_template_target + TemplateTargetConfig extracted to common.rs; both JobCompiler and StageCompiler delegate to it.
  • generate_stage_prefix now uses is_ascii_alphanumeric() as the split predicate and emits a warn!() when non-ASCII chars are present.
  • front_matter.repositories is correctly populated by mod.rs:160 (resolve_repos) before any compiler runs, so the header repo-documentation loop is safe.
  • No leftover {{ marker }} in either template: every token in job-base.yml / stage-base.yml is covered by compile_template_target's extra_replacements or compile_shared's standard table.

• This diff is exclusively formatting (doc-comment whitespace, one-arg-per-line call sites, alphabetical imports, .await newline placement, removal of unused use super::*) — no logic changes, nothing to flag.

Generated by Rust PR Reviewer for PR #519 ·

Generated by Rust PR Reviewer for issue #519 · ● 829.8K · ◷