feat(compile): IR-validated step authoring & runtime self-optimization

[jamesadevine]

feat(compile): IR-validated step authoring & runtime self-optimization

Summary

Two new agent-driven capabilities that don't exist in ado-aw (or upstream gh-aw) today:

Flow A — Authoring-time step hoisting: When an authoring agent creates or updates a pipeline, it proactively scaffolds concrete steps: / post-steps: entries for deterministic, non-LLM work (clone, cache restore, CLI install). A new validate_steps MCP tool on the author-facing server gives the agent IR-level feedback before committing.

Flow B — Runtime self-optimization: Opt-in via self-optimization: enabled: true. The Stage-1 runtime agent recognises bash it ran successfully and proposes lifting it into front-matter steps via a new structured safe-output. Stage 2 cross-checks proposals against command history (anti-injection). Stage 3 IR-validates (Curated allow-list: bash + typed-factory tasks only) and either renders a 🎭 build-log preview (staged mode, the default) or opens a PR against the source .md (live mode, with idempotency dedup).

Self-audit prompt: New prompts/audit-ado-agentic-workflow.md teaches a Copilot CLI agent to systematically audit pipeline runs across 5 dimensions (cost, hoist candidates, reliability, safe-output quality, security) and produce actionable reports with concrete front-matter patches.

What shipped (13 commits)

Layer	What
Shared core	compile::ir::validate_step_block structural validator + CURATED_TASK_IDS allow-list
Flow A	validate_steps MCP tool + 3 authoring prompt updates (hoist heuristic)
Front matter	self-optimization: section (enabled, staged, max-proposals-per-run, allowed-sections)
Flow B Stage 1	propose-step-optimization safe-output + OPT_IN_GATED_TOOLS gating
Flow B Stage 2	Detection prompt extension (command-history cross-check)
Flow B Stage 3	Staged preview renderer + live-PR path (with idempotency dedup)
Telemetry	AgentStats.propose_step_optimization_calls OTel counter
Self-audit	prompts/audit-ado-agentic-workflow.md + dispatcher routing
Docs	8 doc files updated/created (front-matter, mcp-author, extending, safe-outputs, self-optimization, prompts×4, AGENTS.md)
Tests	2046 total tests pass; ~50 new tests across unit, integration, and fixture coverage

Configuration

self-optimization:
  enabled: true              # opt-in (default: false)
  staged: true               # preview-only (default: true)
  max-proposals-per-run: 3   # cap (default: 3, max: 50)
  allowed-sections:          # which sections proposals may target
    - steps                  # (default: [steps, post-steps])
    - post-steps

Security model

• Curated allow-list: Only Bash steps and tasks with a typed factory in tasks.rs are accepted (today: ArchiveFiles@2, CopyFiles@2, DockerInstaller@0, DotNetCoreCLI@2, PublishTestResults@2).
• Stage 2 cross-check: Detection agent verifies proposed bash against source_command_evidence — ungrounded proposals are flagged as prompt injection.
• Staged-by-default: First runs only preview; authors flip staged: false once they trust the agent's judgement.
• Section scoping: setup/teardown require explicit opt-in (different job identities).
• OPT_IN_GATED_TOOLS: MCP layer strips the tool unless the compiler explicitly enables it.
• Validator rejects safe-outputs.propose-step-optimization: — the tool is ONLY activated via the self-optimization: section.

Testing

cargo build        # 1 warning (allows_section unused — trivial)
cargo test         # 2046 passed, 0 failed, 1 ignored
cargo clippy       # clean

[github-actions]

🔍 Rust PR Review

Summary: Has a few real bugs and one security gap worth fixing before merge.

---

Findings

🐛 Bugs / Logic Issues

1. checkout, download, publish accepted in Curated mode — contradicts stated security model

src/compile/ir/step_validation.rs — classify_and_validate_step

The PR description and docs/self-optimization.md explicitly state the curated allow-list is "bash + typed-factory tasks only", but checkout, download, and publish steps pass StepKindAllow::Curated validation without restriction. The test accepts_multi_entry_mixed_step_block confirms this by calling validate_step_block(..., StepKindAllow::Curated) with a publish: entry and asserting success.

checkout: self with persistCredentials: true is particularly concerning in this mode — an agent could propose it to leave credentials available to subsequent steps. publish: could exfiltrate data as a build artifact.

Fix: add a StepKindAllow::Curated arm to classify_and_validate_step that rejects checkout, download, and publish with a clear error message pointing authors to StepKindAllow::Full (the authoring-time tool).

---

2. max_proposals_per_run is configured but never enforced at Stage 3

src/safeoutputs/propose_step_optimization.rs — execute_impl

SelfOptimizationConfig::max_proposals_per_run is validated, clamped at 50 in sanitize_config_fields, and documented in the front-matter grammar. But execute_impl never reads this field. Stage 3 processes every NDJSON entry independently, so a misbehaving agent could emit 100+ proposals regardless of the configured cap.

Stage 3 needs to count how many times this tool has already executed in the current run (e.g., by recording processed proposals in a sidecar file or checking the NDJSON archive) and return ExecutionResult::failure when the cap is exceeded.

---

3. patch_front_matter can split incorrectly on multiline YAML strings

src/safeoutputs/propose_step_optimization.rs:644

let Some(fence_end) = after_first_fence.find("\n---") else { ... };

find("\n---") returns the first occurrence of \n--- in the post-opening-fence content. If the front matter contains a multiline string value that includes --- on its own line — e.g.:

description: |
  ---
  some details here

...the split will land inside the YAML block, producing a truncated yaml_str and a corrupted body_with_fence. The resulting re-serialized file will be silently malformed. Change the search to "\n---\n" (or "\n---\r\n" for CRLF), which matches only a bare fence separator and won't fire on inline --- content.

---

🔒 Security Concerns

4. env: values in proposed steps not checked for ADO macro expansion syntax

src/compile/ir/step_validation.rs — check_env_string_values

The validator correctly rejects non-string env values, but it does not inspect whether string values contain ADO macro syntax ($(...)) or template expressions ($[...]). Stage 2 cross-checks bash bodies against source_command_evidence, but env values are outside that check. A proposal like:

- bash: ls -la   # agent did run this → evidence matches
  env:
    ADO_TOKEN: $(System.AccessToken)

would pass the IR validator, pass Stage 2 (bash body is in evidence), and — if merged — would expand $(System.AccessToken) in a future build and make the token available inside $ADO_TOKEN during the step. Consider rejecting values that match \$\( or \$\[ at the validator level as a defence-in-depth measure, consistent with the stated security model.

---

⚠️ Suggestions

5. Section comparison uses serde_yaml::to_string(s).trim() — fragile round-trip

src/safeoutputs/propose_step_optimization.rs:243–248

let section_allowed = config.allowed_sections.iter().any(|s| {
    serde_yaml::to_string(s)
        .unwrap_or_default()
        .trim()
        == section_wire
});

Comparing via a YAML serialization round-trip when a direct string conversion is already available elsewhere in this file (ProposalSection::as_wire_str) is unnecessarily fragile. Add as_wire_str() -> &'static str to StepSection (same approach as ProposalSection) and use that for the comparison. This eliminates the allocation and the implicit dependency on serde_yaml's enum formatting.

---

6. patch_front_matter silently strips YAML comments on re-serialisation

src/safeoutputs/propose_step_optimization.rs:651–679

Parsing the front matter into a serde_yaml::Value and re-serializing with serde_yaml::to_string will silently discard any # comments in the YAML block and may reorder keys. The live-mode PR will therefore contain spurious formatting changes beyond the intended step additions, making review noisier and potentially surprising authors who maintain documentation comments in their front matter. Worth calling out in the live-mode PR body ("Note: YAML comments in the front-matter block are not preserved across this rewrite") or in the docs.

---

✅ What Looks Good

• The OPT_IN_GATED_TOOLS gating mechanism + validate_self_optimization_config rejecting safe-outputs.propose-step-optimization is a clean defence-in-depth layer.
• Staged-by-default is the right choice — authors opt into real mutations explicitly.
• validate_step_block collects all errors rather than short-circuiting; the comprehensive error message UX is good.
• is_valid_task_identifier is simple, regex-free, and tests the edge cases well.
• The source_command_evidence cross-check design for Stage 2 is sound.
• SelfOptimizationConfig::sanitize_config_fields clamping max_proposals_per_run at 50 is the right call.

Generated by Rust PR Reviewer for issue #1061 · 367.8 AIC · ⌖ 12.7 AIC · ⊞ 33.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Two findings need attention before merge — one security bug and one enforcement gap. The overall architecture and test coverage are strong.

Findings

🔒 Security Concerns

• propose_step_optimization.rs:990-1011 — ##vso[ pipeline-command injection via staged preview stdout

  In staged mode, proposed_yaml is printed directly to stdout without calling neutralize_pipeline_commands:

  // propose_step_optimization.rs ~line 990
  let proposed_yaml = serde_yaml::to_string(&yaml_value)
      .unwrap_or_else(|_| "<failed to render YAML>".to_string());
  // ...
  println!("{preview}");   // ← preview embeds proposed_yaml verbatim

  The SanitizeContent impl explicitly skips steps (justified for Stage 2 cross-check integrity), and validate_step_block checks structural shape and byte limits but does not strip ##vso[task.setvariable variable=X]Y sequences. Since Stage 3 runs as a regular ADO task, the ADO build agent processes ##vso[...] written to stdout as pipeline commands. A compromised agent could embed ##vso[task.setvariable variable=EXPLOIT]$(System.AccessToken) in a bash body (valid string ≤ 10KB), pass IR validation, and use Stage 3's staged preview to inject pipeline commands.

  The other println!/stdout paths in execute.rs correctly route through log_and_print_entry_result which calls neutralize_pipeline_commands(&result.message), but this println!("{preview}") call inside execute_impl bypasses that wrapper entirely.

  Fix: apply neutralize_pipeline_commands to proposed_yaml before embedding it in the preview string:

  use crate::sanitize::neutralize_pipeline_commands;
  let proposed_yaml_safe = neutralize_pipeline_commands(&proposed_yaml);
  // use proposed_yaml_safe in preview format string

🐛 Bugs / Logic Issues

• propose_step_optimization.rs:920-1027 — max_proposals_per_run declared but never enforced in Stage 3

  execute_impl reads config.max_proposals_per_run from the tool config but never uses it:

  let config: crate::compile::types::SelfOptimizationConfig =
      ctx.get_tool_config("propose-step-optimization");
  
  // config.staged is used; config.allowed_sections is used;
  // config.max_proposals_per_run is ... never checked.

  execute_safe_outputs in execute.rs processes every NDJSON entry sequentially, dispatching each propose-step-optimization entry into a fresh execute_impl call with no shared counter. In staged mode this means unlimited previews. In live mode, the per-section idempotency check caps to one PR per section (max 4), but that bound is an incidental side-effect of idempotency, not enforcement of the documented max_proposals_per_run cap.

  The config promise in the docs and front-matter type should be enforced. One pattern used elsewhere: track counts in ExecutionContext via an Arc<Mutex<u32>> counter keyed on the tool name (similar to uploaded_pipeline_artifact_keys).

⚠️ Suggestions

• propose_step_optimization.rs:925-930 — allows_section comparison via serde_yaml::to_string roundtrip

  let section_allowed = config.allowed_sections.iter().any(|s| {
      serde_yaml::to_string(s)
          .unwrap_or_default()
          .trim()
          == section_wire
  });

  This serializes each StepSection enum variant to YAML and string-compares. It works correctly today, but it's fragile — it relies on serde_yaml's enum-unit-variant format staying stable and matching the kebab-case wire strings that ProposalSection::as_wire_str() returns. StepSection already has #[serde(rename_all = "kebab-case")] so the values align, but the indirection is unnecessary. Adding fn as_wire_str(self) -> &'static str to StepSection (mirroring the identical method on ProposalSection) would let this be a direct == comparison and remove any serialization dependency from a security-critical allow-list check.

• ir/mod.rs — stale #[allow(unused_imports)] comment

  The #[allow(unused_imports)] block says the downstream consumers "land in subsequent commits," but both consumers (validate_steps in mcp_author/mod.rs and propose-step-optimization Stage 3 in safeoutputs/propose_step_optimization.rs) are in this PR. The attribute is harmless since the items are used, but the comment is misleading. The #[allow] can be removed.

✅ What Looks Good

• The curated task allow-list design (CURATED_TASK_IDS + is_curated_task + the lockstep test curated_task_ids_match_factories) is exactly the right pattern — the allow-list is the source of truth and the test enforces it stays in sync with the factories.
• validate_step_block collecting all errors rather than short-circuiting is the right UX call for a validator that surfaces feedback to agents.
• The OPT_IN_GATED_TOOLS / DEBUG_ONLY_TOOLS distinction in the MCP filter is a clean security boundary that makes the gating explicit and extensible.
• Per-section idempotency check in live mode (prefix branch search before creating a new branch) is a thoughtful operator-friendliness touch.
• Test coverage is comprehensive — the structural validator tests, section roundtrip tests, and patch_front_matter tests all cover the key failure modes.

Generated by Rust PR Reviewer for issue #1061 · 447.2 AIC · ⌖ 12.6 AIC · ⊞ 33.8K · ◷