feat(compile): native ADO pipeline IR

[jamesadevine]

Replaces the YAML-string composition pipeline compiler with a typed Rust IR. Every production compile target (standalone, 1ES, job, stage) now builds a Pipeline value and lowers it through a single serde_yaml emit. All four src/data/*-base.yml templates are deleted. The CompilerExtension trait surface is reduced to name / phase / declarations, with every signal (steps, hosts, MCPG entries, prompt fragments, AWF mounts, env vars, warnings) returned as a typed Declarations aggregate.

Companion to merged prep PR #957 (canonical serde_yaml normalisation across all lock files), which makes this PR''s diff purely structural.

What landed

Foundation (src/compile/ir/)

• 080bf10d typed pipeline IR (ids, step, job, stage, env, condition, output)
• f2b76455 lower.rs + emit.rs with round-trip tests
• cd3af4d3 graph.rs — Kahn cycle detection, per-stage dependsOn derivation from OutputRef graph
• ec50b1fa per-location OutputRef lowering — same-job macro / cross-job dependencies.<job>.outputs[...] / cross-stage stageDependencies + auto-isOutput=true
• 87759d2e Condition AST + codegen with Custom-injection rejection
• 1253187f top-level parameters / resources / triggers / variables lowering

Extension ports (one commit each)

All extensions now return typed Declarations with Step::Bash / Step::Task / Step::Download / Step::Publish values: AdoAwMarker, GitHub, SafeOutputs, AzureCli, Lean/Python/Node/Dotnet, AzureDevOps/CacheMemory, AdoScript (the marquee — typed synthPr step with OutputDecls, prGate consumes via OutputRef, unlocks declarative cross-stage synth-PR propagation), ExecContext (PR contributor uses EnvValue::Coalesce(vec![Macro(SYS_PR_*), StepOutput(synthPr.*)]) instead of hand-written $[ coalesce(...) ] strings).

Compile-target migrations (every *-base.yml deleted)

• dfba833c standalone → src/compile/standalone_ir.rs; delete base.yml
• 468359f6 extract canonical-jobs builder + extend IR for template targets (StageExternalParamsWrap, TemplateDependsOnWrap)
• 9f400732 stage → src/compile/stage_ir.rs; delete stage-base.yml
• 63b489ee job → src/compile/job_ir.rs; delete job-base.yml
• 770e99df 1es → src/compile/onees_ir.rs; delete 1es-base.yml (−647 lines net); PipelineShape::OneEs lowering with templateContext.outputs[] for Step::Publish lifting

Cleanup (IR_PLAN.md)

• 7c41c6a9 retire-agentic-depends-on — delete every generate_* / format_* / compile_shared / compile_template_target helper from common.rs (−3 234 lines)
• 63f3af18 lockfile-rebaseline — recompile every tests/safe-outputs/*.lock.yml at v0.35.3 (diff is purely version-bump churn → byte-confirms the IR path is semantically identical)
• 5199c425 Wave A — delete legacy prepare_steps / setup_steps trait methods + enum delegations
• 385cba75 Wave B — fold the other 10 accessors (required_hosts, required_bash_commands, prompt_supplement, mcpg_servers, allowed_copilot_tools, validate, required_pipeline_vars, required_awf_mounts, awf_path_prepends, agent_env_vars) into declarations(). Trait surface is now exactly name / phase / declarations.
• 5796a72d rewrite docs/extending.md, replace docs/template-markers.md with docs/ir.md, refresh AGENTS.md and site/src/content/docs/ for the typed IR

Pragmatic deviations from IR_PLAN.md

1. declarations() started as a default trait impl that wrapped Step::RawYaml around legacy Vec<String> outputs. This kept all ~150 existing call sites compiling during the per-extension rollout. The default impl is now gone (Wave B); every extension provides typed values directly.
2. Step::RawYaml survives — standalone_ir::build_setup_job / build_teardown_job use it for user-authored setup/teardown YAML (the IR does not model arbitrary user-authored ADO step shapes). Audit grep for RawYaml only matches those legitimate sites + IR type/lowering.
3. Setup / Teardown stay unprefixed even under target: job|stage|1es — JobPrefix::id returns the unprefixed base for Setup / Teardown to match the legacy *-base.yml literal emission.
4. 1ES jobs use templateContext: instead of per-job pool: — Job::template_context: Option<JobTemplateContext> causes the lowering pass to suppress pool: and lift Step::Publish entries into templateContext.outputs[].

Validation

Run on every commit:

• cargo build — clean
• cargo test — 1813+ unit + integration tests passing
• cargo clippy --all-targets --all-features — clean
• cargo test --test bash_lint_tests — clean (shellcheck against every literal bash: body in emitted YAML)

Final spot-check: find src/data -name "*-base.yml" returns nothing — no YAML survives in source. grep -r "##vso\[task.setvariable" src/ and grep -r "dependencies\." src/ only match the IR lowering pass — no hand-built reference strings remain in extensions or common.rs.

Stats

27 commits. Net: ~−5 000 lines (deleted four *-base.yml templates totalling ~131 KB and the entire YAML-string composition layer; added typed IR + per-target builders + docs/ir.md).

[github-actions]

🔍 Rust PR Review

Summary: Solid IR foundation — well-structured, well-tested, and good error handling throughout. Two issues worth addressing before real callers are wired up.

Findings

🐛 Bugs / Logic Issues

• src/compile/ir/graph.rs:184–202 — cross-job duplicate StepIds silently overwrite in step_locations

  The duplicate-ID guard only fires when prev.stage == stage && prev.job == job.id (same job). For two different jobs that both declare a step named e.g. synthPr, the second g.step_locations.insert(...) silently overwrites the first. Any OutputRef to synthPr then resolves to whichever job was indexed last — wrong dependsOn derivations and wrong reference syntax. Currently safe only by convention (ado-aw generates unique IDs), but the invariant isn't enforced by the type system. Suggest either (a) making the map key (Option<StageId>, JobId, StepId) or (b) erroring on any cross-job duplicate.

• src/compile/ir/graph.rs:380–383 — misleading comment contradicts the code and the test

  The comment says "add stage edge AND surface a cross-job edge", but the code only inserts into g.stage_edges (no g.job_edges.insert in the cross-stage branch). The test at line 694 explicitly asserts stage_b.jobs[0].depends_on.is_empty(), confirming the correct behavior. The comment should read something like: "Only add a stage-level edge; ADO infers per-job ordering from the stage-level dependsOn — no cross-job edge is needed here."

⚠️ Suggestions

• src/compile/ir/lower.rs:448–462 — lower_outputref_for_expr silently emits semantically broken ADO for same-job step outputs

  When producer and consumer are in the same job, lower_outputref returns $(step.name). The function strips the $() and emits variables['step.name'] — which ADO's runtime expression engine also cannot resolve for step outputs (the comment acknowledges this). Instead of letting callers produce syntactically valid but semantically dead YAML, consider bail!-ing: same-job StepOutput inside a Coalesce is a programming error the IR should reject rather than silently mangle.

• src/compile/ir/lower.rs:229–249 — lower_raw_yaml de-indent uses unwrap_or(line) as a silent fallback

  For a - -prefixed body, continuation lines with fewer than 2 leading spaces are passed through unchanged rather than failing. serde_yaml will catch truly broken output, but an explicit error when a continuation line has unexpected indentation would make future debugging much faster when a legacy emitter changes its formatting.

✅ What Looks Good

• IR validation coverage is thorough: unknown producers, anonymous producers, unknown outputs, duplicate IDs, and cycles all have dedicated error paths and unit tests. The five-stage chain test is a nice stress test of the full transitive-edge derivation.
• Condition::Custom injection check correctly blocks ##vso[/##[ and embedded newlines while explicitly allowlisting ADO runtime expressions — the separation of compile-time safety from runtime expressiveness is sound.
• EnvValue::AdoMacro allowlist with anyhow::Result construction is a great pattern — typos in ADO predefined-variable names are caught at compile time rather than silently producing $(Bad.Var) at pipeline runtime.
• detect_cycles_in Kahn's algorithm is correctly implemented; the .expect("node must be in in_degree") is load-bearing and the invariant holds (every consumer and producer node is inserted into in_degree during edge enumeration).
• Step::RawYaml migration bridge design is clean — the per-extension port-* commit strategy, #[allow(dead_code)] scoping, and explicit removal commit in the plan are all well-considered.

Generated by Rust PR Reviewer for issue #960 · sonnet46 3.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid foundation — the IR design is clean and well-motivated. One correctness bug to address before the compile-target-* commits land; a few smaller items noted below.

Findings

🐛 Bugs / Logic Issues

• step_locations keyed only on StepId → cross-job collision (src/compile/ir/graph.rs:172-206)

  step_locations: BTreeMap<StepId, StepLocation> uses StepId as the sole key. In ADO, step names are only unique within a job — two different jobs may legally have a step named synthPr. When index_job_steps encounters a second job with a conflicting step ID, line g.step_locations.insert(id.clone(), StepLocation { ... }) silently overwrites the first entry. The inline comment acknowledges cross-job duplicates "are technically allowed", but the fix (rejecting the overwrite) is never done.

  Downstream consequences:

  1. add_edge_for_ref resolves the wrong job as the producer, so dependencies.WrongJob.outputs['step.X'] would be emitted.
  2. apply_auto_is_output walks all steps and marks any step sharing that StepId (in any job) with auto_is_output = true, even unrelated ones.

  The current extension layout (globally-unique IDs: synthPr, prGate, ...) means this won't bite yet, but it must be fixed before the compile-target-* commits wire the IR to production. Suggested fix: either enforce globally-unique StepIds in build_graph validation, or use a compound key (Option<StageId>, JobId, StepId) and require OutputRef to carry the job context too.

• unimplemented!() panic instead of structured error (src/compile/ir/lower.rs:96-103)

  PipelineShape::OneEs { .. } | PipelineShape::JobTemplate { .. } | ... => {
      unimplemented!("PipelineShape wrapping is introduced by the compile-target-* commits");
  }

  unimplemented!() is a hard panic (no Result). If any test or future caller inadvertently passes a non-Standalone pipeline to lower() during the migration window, the error message is unhelpful and the callsite gets no anyhow context chain. Prefer anyhow::bail! which returns Err and lets the caller surface it gracefully.

⚠️ Suggestions

• Condition::And(vec![]) / Or(vec![]) produces invalid ADO syntax (src/compile/ir/condition.rs)

  lower_condition for Condition::And(parts) emits and() when parts is empty. and() / or() with zero arguments is not valid ADO condition syntax (runtime error). The Condition::and() / Condition::or() constructors accept empty iterators without complaint. A guard in lower_condition (bail if flat.is_empty()) would catch authoring errors early with a clear message rather than letting them surface as cryptic ADO runtime failures.

• expect() in lower_outputref is load-bearing on graph-validation ordering (src/compile/ir/output.rs:141)

  let prod_stage = producer.stage.expect("cross-stage ref must have producer stage (graph validation enforces)");

  The expect fires if lower_outputref is ever called without a prior build_graph pass (e.g., in a future direct test). The contract is documented, but replacing with ok_or_else(|| anyhow::anyhow!("...")) propagates cleanly through lower_outputref's caller chain (changing the return type from String to Result<String>) rather than panicking. Given that lower_outputref is pub, this is worth tidying.

• Minor doc/impl mismatch in EnvValue::Coalesce (src/compile/ir/env.rs + lower.rs)

  The doc-comment says "Nested Coalesce is flattened during lowering" but lower_env_value_as_expr_atom actually emits coalesce(coalesce(a, b), c) for nested Coalesce — structurally nested, not flat. The "flattening" intended is just that the outer $[ ... ] wrapper doesn't repeat, which is correct. Amending the doc to "The outer $[ ... ] wrapper is not repeated for nested Coalesce" avoids confusion for future contributors reading the IR.

✅ What Looks Good

• Kahn cycle detection (graph.rs:417-467): correct BFS-over-in-degree-0 implementation; the adjacency map is oriented producer → [consumer] (right direction for topological order); visited != in_degree.len() cycle test is sound.

• ADO reference-syntax table (output.rs): the three-form dispatch (same-job $(step.X) / cross-job dependencies.J.outputs[...] / cross-stage stageDependencies.S.J.outputs[...]) is correctly implemented and the four unit-test cases in output::tests lock every branch.

• Condition::Custom injection guard (condition.rs:validate_custom_condition): rejects ##vso[ and ##[ pipeline-command prefixes and embedded newlines while deliberately allowing ADO expressions ($(...), $[...], ${{...}}). The two rejection tests are specific and clear.

• Typed-ID newtype via define_id! (ids.rs): the macro-generated StageId / JobId / StepId enforce the ADO identifier grammar (^[A-Za-z_][A-Za-z0-9_]*$) at construction with anyhow::Result, making misuse a compile-time / immediate-runtime error instead of a silent wrong-YAML field.

• Default declarations() migration bridge: wrapping prepare_steps / setup_steps results as Step::RawYaml in the trait default preserves all ~150 legacy call sites without change and makes the per-extension port-* commits independently reviewable — clean incremental strategy.

• Marquee tests (ado_script.rs::typed_gate_pr_id_lowers_to_macro_concat_in_same_job, exec_context/mod.rs::exec_context_pr_step_lowers_to_cross_job_dep_form_in_agent_job): these build real Pipeline structs, run the full lowering pass, and assert the exact env-var values in the emitted YAML — exactly the right level of coverage for the ADO macro/expression duality that caused bugs in the past.

• EnvValue::AdoMacro(&'static str) allowlist (env.rs): requiring construction through the ALLOWED_ADO_MACROS gate catches predefined-variable typos at Rust compile time / declarations() call time instead of silently producing $(Bad.Var) at pipeline runtime. The env_value_from_ado_macro fallback in filter_ir.rs preserves backward compat for not-yet-listed variables while still emitting a functionally equivalent scalar.

Generated by Rust PR Reviewer for issue #960 · sonnet46 3.9M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid IR foundation with 97 new tests and clean design — two latent bugs worth fixing before this path handles staged pipelines.

Findings

🐛 Bugs / Logic Issues

• src/compile/ir/graph.rs — build_graph skips stage-level conditions: Pass 2 calls add_edges_from_job for every job in every stage, but never walks Stage::condition for OutputRefs. Consequence: a stage condition containing Expr::StepOutput(...) (1) won't validate the producer reference, (2) won't add the cross-stage edge to g.stage_edges, and (3) won't populate Stage::depends_on via apply_edges. The pipeline would emit a condition: stageDependencies.X... with no dependsOn: — which ADO may silently accept or reject. Today this is unreachable (standalone uses flat Jobs), but it's the stated goal of the staged-pipeline path. Fix: add a add_edges_from_stage_condition sweep alongside the job loop in pass 2.

• src/compile/ir/lower.rs:108 — unimplemented!() panic in lower_with_graph: The match arm for OneEs | JobTemplate | StageTemplate calls unimplemented!() rather than returning a proper Err(anyhow::bail!(...)). Currently safe because standalone_ir.rs always produces PipelineShape::Standalone, but lower_with_graph is pub and the contract is only enforced by convention. Should return an error variant rather than panic.

⚠️ Suggestions

• src/compile/ir/output.rs:142 — .expect() in pub lower_outputref: The cross-stage branch asserts producer.stage.expect("cross-stage ref must have producer stage (graph validation enforces)"). The function signature is pub fn ... -> String, not Result<String>, so it relies entirely on callers having run build_graph first. The two private wrappers (lower_outputref_for, lower_expr) respect this contract, but the public surface doesn't enforce it. Suggest either making the function pub(crate) or changing the return to Result<String>.

• src/compile/ir/lower.rs — lower_raw_yaml 2-space de-indentation assumption: The strip_prefix(" ") stripping assumes the sequence marker - is immediately followed by the mapping key and all continuation lines have exactly 2 spaces of sequence indent. This holds for all current extension YAML shapes, but would silently corrupt content from any future extension that uses 4-space or tab continuation. Adding a brief doc-comment specifying the expected format (and ideally a test covering the de-indented path directly) would make the invariant explicit.

✅ What Looks Good

• Typed IDs with ADO grammar validation — StageId / JobId / StepId construction validates the ^[A-Za-z_][A-Za-z0-9_]*$ pattern at the callsite; this is the right place for it.
• ALLOWED_ADO_MACROS allowlist — rejecting unknown EnvValue::AdoMacro names at construction time is excellent; a typo that would silently produce $(Bad.Var) at runtime is now a build error.
• Condition::Custom injection checks — validate_custom_condition correctly rejects ##vso[ / ##[ markers and embedded newlines while permitting $() / $[] / ${{}} ADO expressions (which is exactly the right trade-off for an escape hatch).
• Two-pass graph build — index-then-resolve cleanly prevents forward-reference failures and separates duplicate-detection from edge-derivation.
• Kahn cycle detection is correctly implemented with full node tracking; the expect("node must be in in_degree") in the inner loop is justified by the construction invariant a few lines above.
• RawYaml migration bridge with a documented exit path is a pragmatic and transparent choice for the ~150 legacy call sites.

Generated by Rust PR Reviewer for issue #960 · sonnet46 4.7M · ◷

[jamesadevine]

@copilot resolve merge conflicts

[Copilot]

@copilot resolve merge conflicts

Merge conflicts resolved (commit 4cd0d71):

• tests/compiler_tests.rs: Updated assertions to use the new $(AW_PR_ID) / $(AW_PR_TARGETBRANCH) macro form from origin/main (the new unified variable namespace from fix(compile): unify synthetic-PR variable namespace via ado-script #972/fix(ado-script): treat unsubstituted ADO macros as empty in synthPr #975).
• src/data/base.yml: Kept the intentional deletion from this branch — the standalone target now builds from the Pipeline IR directly, so the template file is no longer needed.

Build and tests green.

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Strong progress — standalone_ir.rs cleanly replaces base.yml with typed IR, and the parameter/resource/trigger lowering in lower.rs is correct and well-tested. One new silent-failure bug in the latest commits; four previously-flagged items remain open.

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone_ir.rs — parse_env_block silently drops all env vars on YAML parse failure (NEW)

  let parsed: serde_yaml::Value = match serde_yaml::from_str(yaml_block) {
      Ok(v) => v,
      Err(_) => return Vec::new(),  // ← silent drop
  };

  Called in run_copilot_step with a synthetic_block constructed by indenting engine_env lines under an env: key. If any engine-env value (including extension-contributed agent_env_vars) contains YAML-special characters (:, #, multi-line text), serde_yaml::from_str returns Err and all env vars are silently dropped — no compile error, no warning. The agent step would then run without its required env vars. Suggest changing the return type to Result<Vec<_>> and propagating with .context("parse_env_block: malformed engine env block").

• src/compile/ir/lower.rs:96–103 — unimplemented!() panic instead of anyhow::bail! (PERSISTENT — 2nd time flagged)

  PipelineShape::OneEs | JobTemplate | StageTemplate hits unimplemented!(). lower_with_graph is pub; any test that accidentally constructs a non-Standalone pipeline and calls emit will panic with no anyhow context chain. Replace with anyhow::bail!.

• src/compile/ir/graph.rs:185–206 — step_locations keyed only on StepId, cross-job collision (PERSISTENT — 3rd time flagged)

  Still BTreeMap<StepId, StepLocation>. Two jobs sharing a StepId silently overwrite. OutputRefs pointing to that ID resolve to whichever job was indexed last — wrong dependencies.* syntax at emit time. Fix: use a compound key or enforce global uniqueness in validation.

• src/compile/ir/graph.rs — Stage::condition not walked in Pass 2 (PERSISTENT — 2nd time flagged)

  add_edges_from_job is called for every job in every stage, but the stage-level condition field is never scanned for Expr::StepOutput refs. A stage condition with a cross-stage StepOutput reference would silently miss its dependsOn edge and fail at ADO runtime.

⚠️ Suggestions

• src/compile/ir/condition.rs — Condition::And([])/Or([]) emits and()/or() — invalid ADO syntax (PERSISTENT — 2nd time flagged)

  lower_condition for And(parts) calls flatten_and(parts) which can return an empty slice, emitting and(). ADO's runtime condition evaluator errors on zero-arg and()/or(). Add a guard: if flat.is_empty() { bail!("Condition::And requires at least one operand") }.

✅ What Looks Good

• standalone_ir.rs end-to-end replacement of base.yml is clean: build_standalone_pipeline composes typed jobs, wire_explicit_dependencies sets explicit dependsOn edges, and emit::emit round-trips correctly through the lock-file test suite. Deleting base.yml is the right call.
• lower_parameters / lower_resources / lower_pr_trigger / lower_ci_trigger all correctly handle empty / None / disabled cases and elide empty keys — output matches the canonical lock-file format.
• parse_env_block heuristic ($(X) → PipelineVar, other strings → Literal, non-string scalars → RawYamlScalar) correctly threads all three ADO env-value shapes through the IR without losing type information.
• Condition::Eq(Expr::StepOutput(...)) on SafeOutputs job condition is the first production use of typed cross-job OutputRef in a condition — the lowering correctly selects dependencies.Detection.outputs['threatAnalysis.SafeToProcess'] and the lock-file tests confirm it.

---

Generated by Rust PR Reviewer for issue #960 · sonnet4-6 · ◷

Generated by Rust PR Reviewer for issue #960 · sonnet46 3M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid IR foundation — typed identifiers, well-tested graph pass, good injection checks. A few minor issues to address before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/ir/lower.rs:108 — unimplemented!() panic instead of bail!()
lower_with_graph (which backs the public emit() entrypoint) panics via unimplemented!() for PipelineShape::OneEs. The function signature is Result<Value>, and the project convention uses bail!() or .context() — never unimplemented!() / panic!() in production return paths. Today this arm is unreachable (no call site constructs OneEs yet), but the 1ES port commit will do exactly that, and a panic will silently bypass the anyhow error chain. Change to:

  PipelineShape::OneEs { .. } => bail!(
      "PipelineShape::OneEs lowering is not yet implemented \
       (introduced by compile-target-1es)"
  ),

• src/compile/standalone_ir.rs:1846 — .unwrap() in non-test production path
  threat_analysis_step() calls StepId::new("threatAnalysis").unwrap() inside a plain fn that returns BashStep, not Result<BashStep>. "threatAnalysis" always passes validation, so this won't panic, but:
  (a) it violates the no-unwrap-on-user-facing-paths convention, and
  (b) it silently blocks the function from being upgraded to return Result<BashStep> if the script body ever needs to do fallible work.
  Either promote the return type to Result<BashStep> and use ?, or hoist the StepId to a static / const outside the function.

• src/compile/standalone_ir.rs:1149–1153 — .expect() in wire_explicit_dependencies
wire_explicit_dependencies returns () but calls .expect("Setup ID") etc. on four JobId::new(...) calls. All four are compile-time string constants that pass JobId validation, so they cannot panic today. However, the function signature cannot propagate errors to its caller — if someone adds a dynamic ID here the compiler forces them to .expect() or .unwrap(). Change the signature to fn wire_explicit_dependencies(jobs: &mut [Job], prefix: &JobPrefix<'_>) -> Result<()> and use ?.

⚠️ Suggestions

• src/compile/standalone_ir.rs:72–83 — duplicate doc comment
  The entire doc paragraph for build_standalone_pipeline appears verbatim twice (lines 72–77 then 78–83). Remove the first copy; the second one immediately precedes the #[allow(clippy::too_many_arguments)] line and is the correct placement.

• src/compile/standalone_ir.rs — Condition::Custom for synth-PR / gate conditions bypasses graph validation
  build_agentic_condition encodes ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true') etc. as Condition::Custom strings. This means the graph pass cannot validate those step IDs / output names, and the validate_custom_condition injection check only runs at codegen time (not compile-validation time). The Setup → Agent edges are covered by wire_explicit_dependencies, so generated YAML is correct. The PR description acknowledges these will be migrated to typed Condition::Ne(Expr::StepOutput(...)) by follow-on port commits — just flagging it here so the migration doesn't get left behind indefinitely.

• src/compile/ir/lower.rs:900 — silent drop on serialization error
  yaml_value_to_scalar_string ends with serde_yaml::to_string(other).unwrap_or_default().trim(). If a Sequence or Mapping YAML value somehow appears in the RawYamlScalar escape hatch and fails to serialise, the error is silently swallowed and an empty string is emitted. Given this is already a best-effort escape hatch this may be acceptable, but unwrap_or_else(|_| String::new()) with a log::warn! would be safer.

✅ What Looks Good

• Graph validation (ir/graph.rs) — the two-pass design (index → walk) is clean. Kahn's algorithm for cycle detection is correct, and the error messages include the offending node set. The mixed staged/un-staged rejection is correct and well-tested.

• lower_outputref + ConsumerLocation/ProducerLocation newtype split — using distinct types prevents consumer/producer mixups at the call site, and the three-syntax dispatch table is easy to reason about.

• validate_custom_condition injection check (ir/condition.rs) — correctly rejects ##vso[ / ##[ pipeline-command markers and embedded newlines while explicitly allowing ADO expression syntax ($(...), $[...], ${{ }}). Tests cover both rejection paths.

• StageId/JobId/StepId newtype IDs with validated construction — returning Result from ::new() and using them as typed map keys throughout the graph pass is exactly the right model; the Display + AsRef<str> impls slot into format strings cheaply.

• Declarations default impl bridging legacy prepare_steps/setup_steps — the Step::RawYaml migration bridge is well-documented and the per-extension port pattern is sound. Using #[allow(dead_code)] at the struct level (rather than scattering it on fields) keeps the intent visible.

• Safety of the marquee SafeOutputs job condition — the typed OutputRef for threatAnalysis.SafeToProcess correctly picks up the prefixed {prefix}_Detection job at lowering time via the graph's step_locations, so the cross-job dependency reference auto-adjusts for target: job/stage without hard-coding the job name. This is the right design.

Generated by Rust PR Reviewer for issue #960 · sonnet46 4.8M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good progress — the unimplemented!() panic in lower_with_graph is fixed and the three new compile-target modules (job_ir, stage_ir, onees_ir) are clean. Four issues from prior rounds remain open; two are notable enough to address before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/ir/graph.rs:74,185–202 — step_locations still uses BTreeMap<StepId, StepLocation> (cross-job collision) (flagged 3 prior rounds)

  The duplicate-ID guard (lines 185–192) only fires when prev.stage == stage && prev.job == job.id — i.e. the exact same job. When two different jobs each declare a step named e.g. synthPr, the second g.step_locations.insert(...) silently overwrites the first. Every subsequent add_edge_for_ref call resolves that StepId to whichever job was indexed last, producing wrong dependencies.<WrongJob>.outputs[...] syntax at emit time. apply_auto_is_output similarly marks the wrong job's step.

  Today ado-aw generates globally-unique step IDs, so this won't fire in production yet. But the comment on line 181 acknowledges the ambiguity ("technically allowed if both jobs are referenced through the qualifying job name") without actually fixing it, and the compile-target-* commits that wire user-authored step IDs are exactly the callsite that could break this.

  Suggested fix: either (a) use a compound key (Option<StageId>, JobId, StepId) throughout the graph and OutputRef, or (b) add a global-uniqueness validation pass in build_graph that errors when the same StepId appears in two different jobs.

• src/compile/standalone_ir.rs:1984–1986 — parse_env_block silently drops all env vars on YAML parse failure (flagged 2 prior rounds)

  Err(_) => return Vec::new(),  // ← silent drop

  The synthetic_block passed to parse_env_block at call-sites (lines 1529, 1602, 1624) is constructed by indenting engine_env values under an env: key. If any extension-contributed env value contains YAML-special characters (unquoted :, #, multi-line text), serde_yaml::from_str returns Err and all env vars for that step are silently dropped — no compile error, no warning. The agent step would run without its required env vars.

  Change parse_env_block to return Result<Vec<_>> and propagate with .context("parse_env_block: malformed engine env block — check for YAML-special characters in extension-contributed env values"). The three call-sites should ?-propagate into their Result<_> functions.

⚠️ Suggestions

• src/compile/standalone_ir.rs:72–83 — duplicated doc comment (flagged in prior round as merge artifact)

  Lines 72–77 and 78–83 contain an identical /// Build the typed [Pipeline]... doc comment. One copy should be removed.

• src/compile/standalone_ir.rs:1150–1153 and :1846 — .expect() / .unwrap() in non-test production code

  wire_explicit_dependencies calls .expect("Setup ID") etc. on four JobId::new(...) calls, and threat_analysis_step calls StepId::new("threatAnalysis").unwrap(). All six string literals are compile-time constants that always pass validation, so there's no panic risk today. But the functions can't propagate errors if a dynamic ID is ever threaded through, and they violate the project convention of no unwrap/expect outside tests. Promoting both to Result<_> return types (or hoisting to static/const for the StepId) would close the gap at the cost of ~10 lines.

✅ What Looks Good

• unimplemented!() eliminated from lower_with_graph — the OneEs / JobTemplate / StageTemplate arms all handle properly now; no more bare panics in the pub emit() path.
• job_ir.rs, stage_ir.rs, onees_ir.rs — the three new compile-target builders are clean: no panics, proper Result propagation, well-scoped responsibilities.
• Condition::Custom injection check runs at codegen time (lower_condition → validate_custom_condition) and correctly rejects ##vso[ and embedded newlines while intentionally permitting ADO expression syntax.
• Kahn cycle detection in graph.rs is correct and well-tested.

Generated by Rust PR Reviewer for issue #960 · sonnet46 2.3M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — this is a well-engineered structural rewrite. Two concrete concerns worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/ir/output.rs + src/compile/ir/graph.rs — auto_is_output is set but never read at emit time.

  output.rs's doc-comment promises:

  "The compiler auto-emits isOutput=true ... the actual bash rewrite is performed at emit time by the producer's extension."

  graph.rs::apply_auto_is_output correctly sets the flag. But lower.rs, standalone_ir.rs, and all extensions have zero occurrences of auto_is_output. No extension performs the promised bash-body rewrite.

  This is not a current runtime bug — every OutputDecl producer either (a) runs a JS bundle that calls setOutput directly (synthPr), or (b) has isOutput=true hardcoded in the bash body (threatAnalysis). But the mechanism is unimplemented and the doc-comment advertises it as the contract. Any future contributor who adds an OutputDecl to a typed BashStep without hardcoding isOutput=true in the script body will silently produce empty values for all cross-step consumers — the graph pass will faithfully set auto_is_output = true and nothing will act on it.

  Recommend either: (a) implement the bash-body rewrite in lower_bash using auto_is_output, or (b) update output.rs's doc-comment to say "extension authors are responsible for ensuring isOutput=true is present in the bash body".

• src/compile/extensions/ado_script.rs (SYNTH_PR_OUTPUT_NAMES comment) — incorrect claim about OutputRef usage:

  // Legacy `AW_SYNTHETIC_PR_*` identifier names. The runtime no
  // longer emits these (see PR #972) but the typed gate-step
  // emitter in `filter_ir.rs::build_gate_step_typed` still
  // references them via OutputRef.

  filter_ir.rs has zero OutputRef usages. build_gate_step_typed uses EnvValue::pipeline_var("AW_PR_ID") etc. — no OutputRef involved. The legacy names are dead declarations. The comment incorrectly identifies the consumer and misleads future maintainers about why these stubs exist.

⚠️ Suggestions

• src/compile/extensions/ado_script.rs (~line 217–228) — duplicated doc-comment. The "Outputs declared by the synthPr step" paragraph appears twice back-to-back (copy-paste artifact).

• src/compile/ir/lower.rs (lower_env_value_as_expr_atom for Coalesce) — comment says "Flatten nested Coalesce: their children appear inline in the enclosing one's argument list" but the code actually produces a nested coalesce(...) call string, not a flattened list. Functionally correct since ADO handles nested coalesce, but the comment contradicts the implementation.

✅ What Looks Good

• Condition::Custom injection check correctly rejects ##vso[ / ##[ pipeline commands and embedded newlines while intentionally passing ${{...}} ADO template expressions.
• lower_outputref's expect("cross-stage ref must have producer stage") is load-bearing and protected: add_edge_for_ref returns bail! for mixed staged/un-staged references before this path is reachable.
• Kahn-based detect_cycles_in is correct; the expect("node must be in in_degree") is sound because adjacency-map consumers are unconditionally inserted into in_degree in the same loop.
• Single-item Coalesce in agent_job_variables_hoist correctly appends '' sentinel, producing $[ coalesce(dependencies.Setup.outputs['synthPr.AW_PR_ID'], '') ].
• ALLOWED_ADO_MACROS allowlist in env.rs is a clean compile-time guard against silent $(BadVar) literal passthrough.

Generated by Rust PR Reviewer for issue #960 · sonnet46 4.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured IR with thorough validation and test coverage. Two minor concerns around expect() usage and one style note.

Findings

⚠️ Suggestions

• src/compile/ir/output.rs:156 — lower_outputref returns String (not Result<String>) but contains an .expect() for the cross-stage/no-producer-stage case. The graph validation in add_edge_for_ref does bail on mixed staged/un-staged references before lowering, so this is safe along the normal resolve() → lower() path. But it's a silent panic if lower_outputref is ever called outside that flow. The comment says "graph validation enforces" but the function signature doesn't type-encode that precondition. Consider returning Result<String> for defensive consistency; alternatively document the precondition as a # Safety-style doc comment to make the invariant explicit.

• src/compile/standalone_ir.rs:1171–1175 — wire_explicit_dependencies uses four consecutive .expect() calls on prefix.id(...). The invariant (prefix is valid because the jobs were built successfully with the same prefix) holds in the current call graph, but the function signature fn wire_explicit_dependencies(jobs: &mut [Job], prefix: &JobPrefix<'_>) doesn't capture it — it could be called from a context where that invariant doesn't hold. Changing to -> Result<()> and using ? would eliminate the latent panic risk with minimal noise.

• src/compile/extensions/mod.rs:157 — #[allow(dead_code)] is placed on the entire Declarations struct. Several fields (detection_prepare_steps, safe_outputs_steps, agent_finalize_steps, agent_finalize_condition) are declared but never read in standalone_ir.rs. A struct-level suppression means any future unused field will silently escape the lint. Prefer per-field annotations on the specific unimplemented fields, or add // TODO(#NNN) comments so they're tracked rather than silently dead.

✅ What Looks Good

• Injection defences are correct: Condition::Custom validation rejects both ##vso[/##[ pipeline commands and embedded newlines before codegen. ALLOWED_ADO_MACROS allowlist prevents silent typo-expansion at pipeline runtime. lower_raw_yaml round-trips user-authored YAML through serde_yaml::from_str before emitting, so malformed YAML surfaces as a compile error rather than a corrupt pipeline file.
• Graph validation is thorough: add_edge_for_ref explicitly bails on mixed staged/un-staged OutputRefs; detect_cycles_in (Kahn's algorithm) uses BTreeSet edges so there's no duplicate-edge double-counting, and the .expect("node must be in in_degree") invariant is provably safe because every successor in the adjacency map is also a consumer in the in-degree map.
• EnvValue::Concat inside Coalesce returns a runtime error, not a panic — the bail path in lower_env_value_as_expr_atom correctly rejects this at IR-lower time with an actionable message.
• apply_auto_is_output only touches Step::Bash — TaskStep has no outputs field so the pattern match is exhaustive by design, and collect_step_outputs documents the expected extension point for when TaskStep gains outputs.
• Test coverage throughout — condition.rs, env.rs, ids.rs, output.rs, graph.rs, lower.rs, and emit.rs all have #[cfg(test)] suites covering happy paths and every error variant that matters.

Generated by Rust PR Reviewer for issue #960 · sonnet46 4.4M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Strong architectural improvement overall — the typed IR eliminates an entire class of template-composition bugs. Three issues need attention before merge: one is a bash injection vector, one silently emits broken ADO YAML, and one is a production panic site.

---

Findings

🔒 Security Concerns

• standalone_ir.rs ~L1309, L1762 — Heredoc sentinel collision (bash injection). prepare_agent_prompt_step writes the resolved agent markdown into a bash heredoc with sentinel AGENT_PROMPT_EOF. If inlined_imports: true, agent_content is the fully-resolved user-authored markdown body. Any line containing only AGENT_PROMPT_EOF terminates the heredoc early, and everything after it executes as bash in the Detection job. Same issue in prepare_threat_analysis_prompt_step (sentinel THREAT_ANALYSIS_EOF) where front_matter.description is interpolated verbatim — a malicious description: field can break the Detection stage. Fix: add anyhow::ensure!(!content.contains("AGENT_PROMPT_EOF\n"), ...) (and equivalents) before constructing the script, mirroring the existing ensure!(!path_str.contains('}'), ...) pattern used for import paths.

🐛 Bugs / Logic Issues

• lower.rs ~L1154–1166 — Same-job Coalesce silently emits broken ADO YAML. When a same-job OutputRef appears inside EnvValue::Coalesce, lower_outputref returns the $(step.name) macro form. The code converts it to variables['step.name'], but the inline comment admits this doesn't work at ADO runtime (step outputs from the producing job aren't visible via variables[...]). The function returns Ok with invalid output rather than bail!ing. This will produce pipelines that compile cleanly but silently use empty values at runtime. Should bail! here rather than emit known-broken syntax.

• graph.rs ~L445 — Production expect() in detect_cycles_in:

  let entry = in_degree.get_mut(s).expect("node must be in in_degree");

  The invariant is currently sound, but any future edit to the edge-construction loop breaks it and panics in production instead of returning a typed anyhow error. Should use .ok_or_else(|| anyhow::anyhow!("ir::graph: BUG: node missing from in_degree"))?.

• graph.rs ~L40–43 — Cycle detection misses manually-set depends_on. detect_cycles_in only walks g.job_edges/g.stage_edges (derived from OutputRef walks). Manual Job::depends_on / Stage::depends_on entries set by callers before resolve() are not included — user-authored cycles silently pass detection and only fail at ADO pipeline-load time.

⚠️ Suggestions

• lower.rs ~L1065 — yaml_value_to_scalar_string uses .unwrap_or_default() on serialization failure, silently emitting an empty env-var value. Should propagate as Err so callers know the emission was lossy.

• lower.rs ~L693, L555 — TemplateDependsOnWrap::depends_on_param / condition_param are plain Strings embedded into ADO template-expression YAML keys via format!("${{{{ if eq(length(parameters.{}), 0) }}}}", wrap.depends_on_param). Unlike JobId/StageId/StepId these are not validated against any identifier grammar. Doc-comments say these are always 'dependsOn'/'condition' today, but the type does not enforce that. A caller passing }} or whitespace would produce malformed YAML keys.

• standalone_ir.rs ~L1879 — Residual .unwrap() in evaluate_threat_analysis_step on StepId::new("threatAnalysis"). The literal is always valid, but the PR's stated goal was to replace latent panics with typed errors — the parallel .unwrap() in wire_explicit_dependencies was already fixed; this sibling was missed. Use a LazyLock<StepId> or const.

• lower.rs ~L519–529 — Stage-condition consumer-job placeholder borrows stage.jobs.first(). Correct for cross-stage refs, but if a stage condition ever references a same-stage sibling job output, the first-job placeholder will produce the wrong reference form ($(step.name) instead of dependencies.<job>.outputs[...]). Low-risk today but fragile for future callers.

• graph.rs ~L384–386 — Misleading comment says "add stage edge AND surface a cross-job edge" but only the stage edge is inserted. The code is correct; the comment is wrong.

• exec_context/pr.rs ~L217–222 — Test function name carries_typed_coalesce_envs and its doc-comment describe Coalesce(AdoMacro, StepOutput) but the assertions check PipelineVar. Stale from an earlier implementation; rename/update the test.

✅ What Looks Good

• Injection handling in condition.rs is correct: ##vso[ / ##[ markers and embedded newlines are rejected; Expr::Literal values are single-quote–escaped (v.replace('\'', "''")).
• output.rs OutputRef lowering (same-job macro / cross-job dependencies. / cross-stage stageDependencies.) is correct and all identifier values injected into format strings are typed StepId/JobId/StageId instances validated to ^[A-Za-z_][A-Za-z0-9_]*$ at construction — no injection surface.
• env.rs EnvValue::AdoMacro allowlist prevents typos from emitting unexpanded $(Bad.Var) literals at runtime.
• CompilerExtension trait collapse (declarations() single entry-point replacing 10 individual accessors) is clean and the reserved agent_finalize_steps / detection_prepare_steps / safe_outputs_steps fields are correctly annotated #[allow(dead_code)].
• Deleting the four *-base.yml YAML string templates is the right move — the typed IR makes this entire class of template-marker bugs structurally impossible.

Generated by Rust PR Reviewer for issue #960 · sonnet46 6.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Major architectural improvement that looks well-considered and well-tested; a few stale artefacts and one silent-failure footgun worth addressing before merge.

Findings

⚠️ Suggestions

• src/compile/ir/mod.rs:34 — stale #![allow(dead_code)]
  The suppressor's own comment says "The allow is removed atomically with the trait port" — the trait port landed in Wave B, but the module-level #![allow(dead_code)] is still present. A module-level suppress absorbs any future unreachable item silently; the fix(ir) commit rightly narrowed the analogous suppress in extensions/mod.rs to per-field granularity. Same treatment needed here: remove the module-level suppress and add per-item #[allow(dead_code)] where genuinely needed (e.g. TemplateParams::reserved).

• src/compile/standalone_ir.rs:70-81 — duplicate doc comment
  build_standalone_pipeline has its doc block verbatim twice (lines 70–75 and 76–81). The first copy should be dropped.

• src/compile/ir/lower.rs:1140-1166 — silent Coalesce(StepOutput) footgun for same-job producers
  lower_outputref_for_expr strips the $() macro and rewrites to variables['stepName.X'], but the inline comment admits: "ADO runtime expressions cannot see step outputs from the producing job via variables[...] either." A caller who puts a same-job OutputRef inside Coalesce will produce an expression that silently returns empty at runtime. There's no compile-time guard. The doc says "don't do this", but adding an explicit bail! here (checking consumer.job == producer.job && stages equal) would convert a latent footgun into an actionable error.

• src/compile/standalone_ir.rs:66-68 — stale #[allow(unused_imports)] comment
  The comment says "Suppress unused; this module is wired up in a sibling commit" — that commit already landed. The functions are reached via common:: qualified paths anyway, making the bare use aliases dead. Either remove the aliases (and the allow), or explain why both forms are needed.

• src/compile/ir/step.rs:13-14 — stale commit-name references
  ir-graph, ir-output-lowering, ir-yaml-emit refer to internal planning commit names. These are all realised; the sentence should be updated or dropped.

🔒 Security Concerns

None found. The heredoc_sentinel fix is correct and well-designed: SHA-256 prefix makes a collision practically infeasible, the defense-in-depth sentinel-in-content check converts a worst-case scenario into a typed compile error, and the single-quoted '<sentinel>' heredoc form correctly suppresses variable expansion so agent markdown with $ signs can't escape.

✅ What Looks Good

• Condition::Custom injection check — rejects ##vso[ / ##[ markers and embedded newlines before they reach the YAML emitter. Well-placed.
• EnvValue::ado_macro() allowlist — closed set of ADO predefined variables enforced at construction; a typo returns Err rather than silently producing $(Bad.Var) at runtime.
• Kahn cycle detection — correct Kahn's algorithm on both job-edge and stage-edge sets; the expect("node must be in in_degree") at graph.rs:445 is provably safe by construction (every edge adds both endpoints to in_degree).
• OutputRef graph validation — unknown producer, anonymous producer, unknown output, duplicate IDs all surface as typed errors before lowering, eliminating an entire class of silent YAML mismatches.
• anyhow::Result throughout — no unwrap()/expect() on user-facing paths in the new IR code outside tests.
• Round-trip tests — the lock-file rebaseline gives strong confidence the semantic equivalence claim holds.

Generated by Rust PR Reviewer for issue #960 · sonnet46 5.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Impressive architectural migration — the typed IR foundation is sound and the extension port is thorough. Three concrete findings below, one of which will silently regress a future consumer.

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone_ir.rs (agent_job_variables_hoist) — AW_PR_IS_DRAFT is declared in SYNTH_PR_OUTPUT_NAMES (in ado_script.rs) but is not hoisted to the Agent-job variables: block. The hoisted set is [AW_PR_ID, AW_PR_TARGETBRANCH, AW_PR_SOURCEBRANCH, AW_SYNTHETIC_PR]. Any step on the synth-from-CI path that reads $(AW_PR_IS_DRAFT) via EnvValue::PipelineVar will silently get empty string. AW_SYNTHETIC_PR_SKIP is intentionally absent (consumed only as a typed OutputRef in the Agent-job condition, not as a job-level variable), but AW_PR_IS_DRAFT has no such justification. Suggests the hoist list and SYNTH_PR_OUTPUT_NAMES should be kept in sync.

• src/compile/standalone_ir.rs (build_standalone_pipeline) — The doc-comment is duplicated: the /// Build the typed [Pipeline] for the standalone target... block appears twice in a row immediately before the #[allow(clippy::too_many_arguments)] attribute. Leftover merge artifact — harmless but noisy.

⚠️ Suggestions

• src/compile/standalone_ir.rs (build_agentic_condition) — The Agent-job condition for the synthPr skip signal, prGate, and pipelineGate uses Condition::Custom(hardcoded_string), while the SafeOutputs job correctly uses Condition::Eq(Expr::StepOutput(OutputRef::new(...))). Concretely:

  parts.push(Condition::Custom(
      "ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true')".to_string(),
  ));

  This bypasses the graph validation (build_graph would reject a misspelled step ID or unknown output name) and the auto_is_output promotion path. The explicit wire_canonical_job_dependencies call keeps the dependsOn edges correct, so this is functionally safe today. But it is inconsistent with the stated IR migration goal and creates a maintenance trap: a future rename of prGate → prGateStep in filter_ir.rs would compile cleanly but silently break the Agent condition at ADO runtime. Porting these three custom conditions to typed Condition::Ne/Eq(Expr::StepOutput(...)) would give the same compile-time safety as build_safe_outputs_job now has.

• src/compile/ir/mod.rs — #![allow(dead_code)] is still present. The inline comment explicitly says "The allow is removed atomically with the trait port." Wave B is merged and every extension now provides typed declarations(), so the suppress-attribute is stale and may be masking real dead-code warnings on otherwise-unreachable IR helpers.

✅ What Looks Good

• The Kahn cycle-detection (graph::detect_cycles_in) is correct: edges are oriented producer → consumer in the adjacency list, in-degree counts consumer dependencies, and the visited != in_degree.len() check reliably identifies back-edges.
• lower_outputref_for_expr correctly rejects same-job step outputs inside Coalesce with a clear error referencing the regression history (PR fix(compile): repair synthetic-PR gate + Stage step env var plumbing #956/975) — good defensive check.
• validate_custom_condition catches both ##vso[ and ##[ (the ADO group-command form), which is the complete injection surface.
• The step_to_raw_yaml_string ↔ lower_raw_yaml encode/decode pair is balanced and round-trips correctly through serde_yaml.
• All four compile targets (standalone / 1ES / job / stage) sharing build_pipeline_context is a clean elimination of the 3 234-line duplication.

Generated by Rust PR Reviewer for issue #960 · sonnet46 5.3M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid architecture — the typed IR replaces a large body of string-template code with principled, validated types. A few latent issues worth addressing before a future engine change silently breaks a pipeline.

---

Findings

⚠️ Suggestions

1. split_yaml_step_sequence is fragile (standalone_ir.rs:2165–2197)

The splitter uses blank-line presence to detect step boundaries. Two adjacent steps without a blank line between them are merged into one RawYaml chunk that lower_raw_yaml expects to be a single step — it strips the - prefix and de-indents, and the resulting YAML would parse as one step with garbage structure. Current callers always emit blank-line separators, so there is no regression, but the invariant is implicit.

The doc comment mentions engine_install_steps_yaml is two steps: install + version output, making this the only multi-step case. A brief assertion in the callers (or a YAML-level split using serde_yaml::from_str::<serde_yaml::Sequence>) would make the invariant explicit and catch a future regression immediately.

2. parse_env_block silently discards YAML parse failures (standalone_ir.rs:2068–2070)

Err(_) => return Vec::new(),

If the concatenated engine_env string is malformed YAML, the AWF step silently gets zero env vars (GITHUB_TOKEN missing, model config absent), and the pipeline fails at runtime with a non-obvious error. The string is compiler-generated from validated inputs, so a parse failure here would indicate a compiler bug — returning Vec::new() buries it. A bail! or at minimum a tracing::warn! would surface the issue at compile time.

3. env_value_from_ado_macro fallback bypasses the allowlist (filter_ir.rs:1349–1352)

// Fallback: keep the raw scalar verbatim
Ok(EnvValue::literal(ado_macro))

When an ADO macro isn't in ALLOWED_ADO_MACROS, it falls through to EnvValue::Literal with the raw $(X.Y) string. The literal form is correct — it round-trips through YAML fine — but it silently short-circuits the allowlist validation that EnvValue::AdoMacro would enforce. The comment documents this as intentional ("not-yet-allowlisted predefined var"), but over time this becomes a standing bypass. Consider logging a compile-time diagnostic when a macro falls through, or expanding the allowlist instead.

---

✅ What Looks Good

• Typed injection guards — Condition::Custom rejects ##vso[ and newlines at construction; EnvValue::AdoMacro validates against a closed allowlist. No hand-crafted ADO expression strings in new code.

• Shell injection prevention — is_valid_command_path, is_valid_arg, is_valid_identifier enforce strict character allowlists on everything embedded in the single-quoted '{engine_run}' AWF command (line 1651). The comment at line 411 explicitly calls out the single-quote constraint and routes tool args with parens/spaces through double-quote form.

• Cycle detection — Kahn's BFS in detect_cycles_in is correct and produces human-readable sorted error messages. Per-stage job-ID uniqueness enforcement matches ADO semantics.

• lower_outputref_for_expr regression fix — Same-job StepOutput inside Coalesce is explicitly rejected with a clear error, closing the silent-reuse footgun from fix(compile): repair synthetic-PR gate + Stage step env var plumbing #956.

• build_gate_step_typed — Clean integration: typed OutputDecl::new("SHOULD_RUN") for graph validation, EnvValue::ado_macro("System.AccessToken") through the allowlist, base64 spec in EnvValue::Literal. The pr_synth_active path clearly differentiates the three AW_PR_* variable substitutions.

• Identity validation chain — validate_front_matter_identity → reject_pipeline_injection covers name and description before they reach YAML output and threat-analysis prompt substitution.

• Test density — Inline #[cfg(test)] modules co-located with each IR file are comprehensive. The regression test pinning the PR filter synth-mode gate step behaviour (test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref) is exactly the kind of test that pays off long-term.

Generated by Rust PR Reviewer for issue #960 · sonnet46 9.8M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Large structural refactor looks solid — typed IR is well-designed and the security-relevant injection paths are correctly guarded. One latent correctness bug worth addressing before the trait surface grows further.

Findings

🐛 Bugs / Logic Issues

• src/compile/ir/graph.rs — index_job_steps: cross-job duplicate StepIds silently overwrite step_locations.

  The uniqueness check only rejects duplicates within the same job:

  if let Some(prev) = g.step_locations.get(id)
      && prev.stage == stage
      && prev.job == job.id   // ← cross-job duplicate is not caught
  {
      bail!("ir::graph: duplicate StepId '{}' inside job '{}'", id, job.id);
  }
  g.step_locations.insert(id.clone(), StepLocation { ... });

  If two jobs each carry a step with the same StepId (e.g. a future extension author accidentally reuses "synthPr" or "marker"), the second insert silently overwrites the first entry in step_locations. Any OutputRef pointing to that StepId then resolves to the second job's location, not the first.

  The comment explains the intent — "ADO technically allows it if both jobs are referenced through the qualifying job name" — but this is only true for hand-written YAML. The IR's OutputRef carries only StepId (no job qualifier), so it cannot correctly disambiguate. The implicit contract is that StepIds must be pipeline-wide unique for correct producer resolution, but that constraint is neither enforced nor documented on StepId or OutputRef. Current production code is safe because all extension step IDs happen to be globally unique (synthPr, threatAnalysis, marker, prGate, ...), but this is a latent maintenance hazard.

  Suggested fix: either reject cross-job duplicate step IDs outright (simplest), or add a clear doc comment on OutputRef that callers must ensure pipeline-wide StepId uniqueness and add an assertion in index_job_steps.

⚠️ Suggestions

• src/compile/ir/lower.rs — lower_raw_yaml: the 2-space de-indent uses unwrap_or(line) as fallback.

  out.push_str(line.strip_prefix("  ").unwrap_or(line));

  For strings that arrive via step_to_raw_yaml_string (which always adds exactly 2 spaces), this fallback is dead code and the behaviour is correct. For any future caller that passes a string with a different indentation assumption, the fallback silently produces misaligned YAML (instead of an error). A bail! or a debug assertion here would surface unexpected callers more loudly than a serde_yaml parse failure several stack frames later.

• src/compile/ir/env.rs — EnvValue::Literal has no injection guardrails and no compiler-internal-only marker.

  lower_env_value emits Literal(s) as s.clone() with no sanitization. All current construction sites use hardcoded strings, but the type accepts any impl Into<String>. As new IR callers appear, a future EnvValue::literal(user_supplied_string) would silently embed the raw value in the env: block. A doc comment on EnvValue::Literal (and/or a #[non_exhaustive] hint) making the "compiler-internal only" contract explicit would reduce the chance of accidental misuse.

✅ What Looks Good

• Condition::Custom injection guard — validate_custom_condition correctly rejects ##vso[ / ##[ markers and embedded newlines while still allowing ADO expressions ($(...), $[...], ${{...}}). The test coverage is thorough.
• EnvValue::AdoMacro closed allowlist — construction validates against ALLOWED_ADO_MACROS at build time, so a future typo like "Build.Resoon" produces a compile-time error rather than a silent empty macro expansion at pipeline runtime.
• lower_outputref_for_expr same-job Coalesce guard — the PR fix(compile): repair synthetic-PR gate + Stage step env var plumbing #956 regression (cross-job $[ dependencies.* ] refs don't resolve in step env:) is now a hard build-time error rather than a silent wrong-value bug. The error message cites the regression history, which is exactly the right level of context.
• split_yaml_step_sequence + push_raw_yaml_if_nonempty — multi-step engine install YAML is properly split through serde_yaml before wrapping each item in Step::RawYaml, rather than trying to split on blank lines. Correct and robust.
• Overall test discipline — 1813+ tests, per-commit cargo clippy / bash_lint_tests, and locked fixture recompile to verify byte-level IR semantic equivalence. The Wave B cleanup of the last 10 legacy accessors is a particularly clean finish.

Generated by Rust PR Reviewer for issue #960 · sonnet46 4M · ◷