feat(compile): runtime prompt loading via {{#runtime-import}} markers

[jamesadevine]

Summary

Adopts gh-aw's {{#runtime-import path}} marker model and the inlined-imports front-matter toggle. Agent prompt bodies are now loaded at pipeline runtime by default, so edits to the markdown body no longer require ado-aw compile.

Behaviour

• inlined-imports: false (new default) — the agent body is loaded at pipeline runtime via a {{#runtime-import …}} marker resolved by a new import.js ado-script bundle. The compiled YAML contains the marker (and the resolver step), not the body text.
• inlined-imports: true — legacy behaviour preserved bytes-for-bytes for the simple case; author-written {{#runtime-import shared/snippet.md}} markers are resolved at compile time so the body is fully self-contained.
• Author UX bonus — {{#runtime-import path}} (required) and {{#runtime-import? path}} (optional, skip-if-missing) work inside any agent's markdown body, with the same semantics as gh-aw.

The Stage-2 threat-analysis prompt is not runtime-imported. It's a tooling-shipped template that's include_str!'d into the ado-aw binary and inlined into the emitted YAML at compile time, matching gh-aw's pattern (their threat_detection.md ships with the setup action and is read directly from disk — no marker, no resolver).

Consolidated single extension (addresses reviewer feedback)

One always-on AdoScriptExtension owns all ado-script wiring. It exposes two features through the existing trait hooks — no new template markers, no ScriptAssets registry:

Feature	Hook	Job that consumes the bundle
Gate evaluator (gate.js)	setup_steps()	Setup
Runtime-import resolver	prepare_steps()	Agent

ADO jobs use isolated VMs — /tmp is not shared. The bundle is therefore downloaded once per consuming job. When both features are active, install + download steps appear in both Setup and Agent. That's correct architecture given ADO's topology, not waste.

filters:	inlined-imports	Setup-job steps	Agent-job extra steps
inactive	true	(none)	(none)
inactive	false	(no Setup job)	install + download + resolver
active	true	install + download + gate	(none)
active	false	install + download + gate	install + download + resolver

Implementation

• New consolidated extension src/compile/extensions/ado_script.rs (~470 lines). One internal helper install_and_download_steps() produces the install+download YAML; both setup_steps() and prepare_steps() call it. The Rust source has one place for the install/download YAML; the emitted YAML carries it once per consuming job.
• resolve_imports_inline() (compile-time resolver for inlined-imports: true) lives in the same module.
• Deleted: src/compile/extensions/trigger_filters.rs, src/compile/extensions/runtime_prompt.rs, src/compile/script_assets.rs, the script_assets: ScriptAssets field on CompileContext and its four constructor sites, the prepend block at common.rs:2123-2131, and the {{ agent_prompt_resolver_steps }} template marker (all four base templates).
• 1ES double-call eliminated: compile_shared now detects when extra_replacements already binds {{ setup_job }} (the 1ES path) and skips its own redundant generate_setup_job invocation. Each extension's setup_steps() is now invoked exactly once per pipeline.
• NodeTool@0 displayName updated from "Install Node.js 20.x for gate evaluator" to "Install Node.js 20.x" (the bundle now serves both gate.js and import.js).
• Docs refreshed: docs/ado-script.md (per-job download model), docs/runtime-imports.md, docs/filter-ir.md, docs/template-markers.md, AGENTS.md.

Test plan

Per-job placement tests (pin the reviewer-found bug)

4 new tests in tests/compiler_tests.rs that split the emitted YAML by job block and assert exactly which job(s) contain Download ado-aw scripts:

Test	Expectation
test_gate_only_pipeline_downloads_bundle_in_setup_job_not_agent	Download in Setup only
test_imports_only_pipeline_downloads_bundle_in_agent_job_not_setup	Download in Agent only (no Setup job)
test_both_features_active_downloads_bundle_in_both_jobs	Download in BOTH (exactly 2 occurrences)
test_neither_feature_active_emits_no_node_or_download_anywhere	No NodeTool@0 and no Download anywhere

Without the consolidation, tests (2) and (3) fail with "Agent job is missing the script bundle download" — exactly the cross-job VM isolation bug the reviewer caught.

Automated

• cargo build clean.
• cargo test --bin ado-aw: 1595 unit tests pass.
• cargo test --tests: 108 compiler_tests + every integration suite green.
• cargo clippy --all-targets --all-features clean (only pre-existing warnings).
• cd scripts/ado-script && npm test: 199 tests across 26 files.
• cd scripts/ado-script && npm run test:smoke: 3/3 pass.
• cd scripts/ado-script && npm run typecheck clean.

Manual

Hand-inspected emitted YAML for the four (gate × imports) combinations:

Combo	NodeTool@0	Download	Resolver	Setup job present	Agent job present
gate_only	1	1	0	Yes	Yes
imports_only	1	1	1	No	Yes
both	2	2	1	Yes	Yes
neither	0	0	0	No	Yes

All four match the design.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[github-actions]

🔍 Rust PR Review

Summary: Has a critical cross-job VM isolation bug that will cause inlined-imports: false (the new default) pipelines to fail at runtime, plus a minor issue in the TypeScript error path.

---

Findings

🐛 Bugs / Logic Issues

[CRITICAL] src/compile/script_assets.rs + src/compile/common.rs:3197-3215 — ScriptAssets download runs in the wrong ADO job

RuntimePromptExtension::setup_steps() calls ctx.script_assets.request(), which causes ScriptAssets::emit_steps() to include the NodeTool@0 + Download ado-aw scripts steps in generate_setup_job. That function produces a separate top-level - job: Setup block (look at common.rs:2168). The node import.js resolver steps, however, are placed inside the - job: Agent block via {{ agent_prompt_resolver_steps }} (base.yml line 111).

Azure DevOps jobs run on separate, isolated VMs — /tmp is not shared between jobs. So when the Agent job tries to execute:

- bash: |
    set -eo pipefail
    node '/tmp/ado-aw-scripts/ado-script/dist/import/index.js' /tmp/awf-tools/agent-prompt.md

.../tmp/ado-aw-scripts/ won't exist (it was unzipped in the Setup job on a different VM). Every pipeline with inlined-imports: false (the new default) will fail at this step at runtime.

The existing TriggerFiltersExtension doesn't have this problem because its gate.js steps are also returned from setup_steps() and placed in the Setup job — they run on the same VM as the download. The import resolver needs to follow the same pattern: either emit the download steps as part of agent_resolver_steps inline in the Agent job, or add a per-job download mechanism.

The automated tests don't catch this because they only verify that the compiled YAML contains the string "Download ado-aw scripts", not which job it appears in.

---

[Minor] scripts/ado-script/src/import/index.ts:42-46 — Writes partially-expanded file before exiting on error

const expanded = original.replace(MARKER, (_whole, optional, rawPath) => {
    if (!existsSync(absPath)) {
        hadError ??= `file not found: ${rawPath}`;
        return "";   // ← replaces required marker with empty string
    }
    // ...
});
// ...
if (hadError) { fail(hadError); }
writeFileSync(target, expanded, "utf8");  // ← already wrote above ← wait, no:

Actually the writeFileSync only runs if there's no hadError check issue — looking again: fail() calls process.exit(1) so writeFileSync is NOT reached on error. ✓ (I withdraw this finding — the control flow is correct since fail() exits.)

---

⚠️ Suggestions

• src/compile/script_assets.rs:40 — The NodeTool@0 display name says "Install Node.js 20.x for gate evaluator" but this step now also serves the import resolver. Minor: consider updating to "Install Node.js 20.x".

• src/compile/onees.rs:131-143 — onees.rs::generate_setup_job calls ext.setup_steps(ctx) for all extensions, and then compile_shared calls common.rs::generate_setup_job which does the same. For 1ES, the result of the second call is never used (the {{ setup_job }} marker is already replaced by extra_replacements). This is harmless due to ScriptAssets::request() being idempotent, but the double-call is wasteful and could surprise future extension authors whose setup_steps() have side effects.

---

✅ What Looks Good

• resolve_imports_inline in runtime_prompt.rs is clean and well-tested: path traversal is impossible (the check rejects whitespace, absolute paths are used as-is, relative paths join against the compile-time base dir), error messages are actionable, the five unit tests cover the important cases.
• ScriptAssets dedup pattern is elegant — single AtomicBool + idempotent request() correctly collapses N extension calls to exactly one NodeTool + download pair. The existing gate.js refactor (removing duplicate steps from TriggerFiltersExtension) is a clean win.
• inlined-imports: bool field has proper #[serde(rename = "inlined-imports", default)] — defaults to false without requiring the field to be Option<bool>.
• The import.js single-pass design (no recursive expansion of imported snippets) is correct and intentional; the inline comment explains why.
• Integrity-check tests in codemod_tests.rs (body-only edits pass under default, fail under inlined-imports: true) directly validate the stated UX goal.

Generated by Rust PR Reviewer for issue #625 · ● 2.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-designed feature with strong test coverage. Three minor findings, none blocking.

---

Findings

⚠️ Suggestions

• src/compile/script_assets.rs:37 — Stale display name in shared NodeTool step
  The NodeTool@0 step still displays "Install Node.js 20.x for gate evaluator" but is now also used for the import resolver. Since ScriptAssets is shared by both TriggerFiltersExtension and RuntimePromptExtension, the display name should be something like "Install Node.js 20.x for ado-aw scripts" to avoid confusion in pipeline run logs.

• src/compile/common.rs:3195 — Resolve runtime imports step missing timeoutInMinutes
  The NodeTool and Download steps both have timeoutInMinutes: 5, but the resolver bash step (node '{import_eval_path}' /tmp/awf-tools/agent-prompt.md) omits it. The script is fast in practice, but consistency with the surrounding steps would prevent a stalled Node process from tying up the pipeline until the job-level timeout fires.

• src/compile/onees.rs:65 / src/compile/common.rs:3057 — ext.setup_steps() called twice for 1ES
  For the 1ES compile path, each extension's setup_steps() (and its ctx.script_assets.request() side-effect) is called once inside onees::generate_setup_job and then again inside compile_shared → common::generate_setup_job. The second call's result is silently discarded because the extra_replacements entry for {{ setup_job }} wins before the shared replacement runs. This is currently harmless — all side-effects are idempotent and AtomicBool is fine — but a future extension with a non-idempotent setup_steps would quietly drop steps on the 1ES target. Worth a comment, or a short-circuit in compile_shared when extra_replacements already covers {{ setup_job }}.

---

✅ What Looks Good

• ScriptAssets deduplication is clean: request() + emit_steps() pattern correctly emits NodeTool + curl/sha256/unzip exactly once regardless of how many extensions request it, and the unit tests pin both the no-request and multi-request cases.
• resolve_imports_inline error handling is solid: unterminated markers, empty paths, whitespace-in-paths, missing required files, and missing optional files all handled with clear anyhow errors or silent drops respectively.
• Integrity check semantics are correct for the new default: body-only edits don't trip the check (inlined-imports: false), front-matter edits still do, and the three new codemod_tests.rs tests cover all three cases end-to-end.
• ##vso[...] injection in import.js is not possible — the [^\s}]+ capture group prevents any whitespace (including newlines) in the imported path, so the fail() output can't be split across lines to inject additional VSO commands.
• Test coverage is comprehensive: 12 compile-output tests across 3 modes × 4 targets, 5 unit tests for inline resolution, 7 vitest specs for the import bundle, 3 smoke tests, and the 1ES setup-job regression test.

Generated by Rust PR Reviewer for issue #625 · ● 3.6M · ◷

[jamesadevine]

Thanks for the careful review — the critical cross-job VM isolation finding was spot on. Force-pushed the fix.

What changed in this revision

• Critical bug fixed (per-job download). The ado-script.zip install + download now lands in the same job as each consumer: Setup job for the gate evaluator, Agent job for the runtime-import resolver. When both features are active the bundle is downloaded in both jobs — correct architecture given ADO's VM isolation, not duplication waste.

• Consolidated to a single always-on AdoScriptExtension at src/compile/extensions/ado_script.rs. It exposes the gate via setup_steps() and the runtime-import resolver via prepare_steps() — both hooks already exist on the CompilerExtension trait, so no new template markers were needed. The previous {{ agent_prompt_resolver_steps }} marker is gone (resolver flows through the existing {{ prepare_steps }} block instead).

• ScriptAssets registry removed along with the script_assets: ScriptAssets field on CompileContext. The registry's "one shared download serves all consumers" premise was the bug; per-job emission replaces it.

• {{ setup_job }} double-binding fixed. compile_shared now detects when extra_replacements has already bound {{ setup_job }} (the 1ES override path) and skips its own redundant generate_setup_job invocation. Each extension's setup_steps() is invoked exactly once per pipeline.

• NodeTool@0 displayName updated from "Install Node.js 20.x for gate evaluator" to "Install Node.js 20.x".

Test pinning

4 new placement tests in tests/compiler_tests.rs parse the emitted YAML by job block (extract_job_block helper) and assert exactly which job(s) contain Download ado-aw scripts:

• gate-only → Setup only
• imports-only → Agent only (no Setup job)
• both → both jobs (exactly 2 downloads)
• neither → no NodeTool@0, no download anywhere

On main before this revision, the imports-only and both-active tests fail with "Agent job is missing the script bundle download" — your critical finding, codified.

Net file shape

• New: src/compile/extensions/ado_script.rs.
• Deleted: src/compile/extensions/trigger_filters.rs, src/compile/extensions/runtime_prompt.rs, src/compile/script_assets.rs, the {{ agent_prompt_resolver_steps }} template marker.
• 24 files changed, ~+5 net lines.

[jamesadevine]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into this branch and fixing the conflict in docs/filter-ir.md. Commit: 44f4c85.

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean consolidation, correct per-job placement architecture, and solid test coverage. One functional bug to address, one minor observation.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs:3202-3204 — runtime-import marker silently breaks for agent file paths with spaces

  The generated marker is:

  format!("{{{{#runtime-import {}}}}}", agent_marker_path)

  agent_marker_path is built from generate_source_path(input_path), which embeds the repo-relative file path verbatim. If the path contains spaces (e.g. my agents/pipeline.md → $(Build.SourcesDirectory)/my agents/pipeline.md), import.js's regex [^\s}]+ truncates at the first space. The marker then doesn't match, is left unexpanded in the prompt, and the LLM sees a raw {{#runtime-import ...}} string — no error is surfaced.

  The compile-time resolver (resolve_imports_inline) already guards against this: it ensure!s that path strings contain no whitespace. The same validation is missing on the runtime-import path construction path. The fix is either to reject space-containing source paths at compile time (when inlined-imports: false) or to percent-encode the path in the marker, with a matching decode in import.js.

  Paths without spaces are the common case in CI repos, but it's a silent data-integrity failure.

---

⚠️ Suggestions

• scripts/ado-script/src/import/index.ts:22-35 — only the first error is reported when multiple required markers fail

  hadError ??= ... captures only the first failure; subsequent missing files are also silently replaced with "". The file is correctly not written (fail() comes before writeFileSync), but the error message names only one file. Consider replacing ??= with += (or an array) and reporting all failures so debugging multi-import bodies isn't a guessing game.

• scripts/ado-script/src/import/index.ts:7-9 — ##vso[task.logissue] output contains unsanitized rawPath

  rawPath is matched by [^\s}]+, which permits ] characters. A path like ../../tricky]type=error]secret would produce a malformed ##vso command. The risk is low (all marker paths originate from compile-time generation and paths containing ] are unusual), but worth sanitizing: replace or strip ] in the error message. Compare how gate.js handles its equivalent diagnostic output.

---

✅ What Looks Good

• Per-job VM isolation is correctly modelled: install_and_download_steps() is shared between setup_steps() and prepare_steps(), and the four placement tests (gate_only, imports_only, both, neither) pin the exact intent. Clean fix for the reviewer-caught cross-job bug.
• Double-call elimination in 1ES path: detecting {{ setup_job }} already bound in extra_replacements is the right hook; the comment explains the invariant clearly.
• Integrity check semantics are correct and tested: body edits when inlined-imports: false should not invalidate the lock (body isn't compiled in), and the three new codemod_tests cases cover all branches.
• ExtensionPhase::System ordering and rationale: the Node 20 install lands before user runtimes, so NodeExtension wins on PATH. The comment in phase() and the name_and_phase test both document the invariant.
• resolve_imports_inline is single-pass by design (matching gh-aw semantics), clearly documented, and well unit-tested including the optional marker and absolute path cases.

Generated by Rust PR Reviewer for issue #625 · ● 1.9M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation — the consolidation into a single AdoScriptExtension is clean, the per-job VM-isolation architecture is correct, and test coverage for the four feature combinations is thorough. Two items worth addressing before merge.

---

Findings

🔒 Security Concerns

• resolve_imports_inline / import.js: no path traversal guard (src/compile/extensions/ado_script.rs:261–277, scripts/ado-script/src/import/index.ts:41–43)

  Both the compile-time resolver (inlined-imports: true) and the runtime resolver accept ../-style relative paths without restriction. For the compile-time path, if CI runs ado-aw compile on an untrusted PR branch, a malicious markdown body containing {{#runtime-import ../../../../etc/secret}} would silently embed the host file into the compiled YAML output. Consider rejecting paths that escape the base_dir via .. components, or at minimum documenting that ado-aw compile must not be run on untrusted input without a sandbox. The runtime case is less critical (the import runs in an ephemeral ADO VM, and the markers come from source-controlled files), but the same guard there would be good defence-in-depth.

⚠️ Suggestions

• extract_job_block is fragile for indented job lists (tests/compiler_tests.rs, new extract_job_block fn)

  The function uses "\n- job: " (zero-indent) as the delimiter between jobs. That works for the standalone fixtures used here, but for stage-nested pipelines (stages: > jobs:) the next job would appear as " - job: " (indented), causing the function to silently include the next job's content in the block, potentially making the per-job placement assertions pass incorrectly. Consider using a regex or stripping leading whitespace in the search, or add a comment that this helper is only valid for flat (non-staged) jobs: lists.

• inlined_imports: false is a silent behaviour change on recompile (src/compile/types.rs:688)

  Existing agent files without inlined-imports: in their front matter currently inline the body. After this PR, recompiling them will emit a {{#runtime-import ...}} marker and require ado-aw-scripts at runtime. This is intentional, but a first-run pipeline after recompile will fail if the release artifact it tries to download (v{version}/ado-script.zip) does not yet include import.js. Worth verifying the release packaging explicitly includes the new bundle, and consider emitting a compile-time notice when the default mode is in effect so authors know what changed.

• compile_fixture_with_inlined_imports duplicates inlined-imports: when the fixture already has it (tests/compiler_tests.rs:~170)

  The helper injects inlined-imports: true by string substitution before ---. If a future fixture already declares inlined-imports: false, the resulting front matter has two inlined-imports: keys. serde_yaml silently uses the last one, so the test still passes, but the fixture file round-trips incorrectly and the double-key could confuse readers. A small guard (if !contents.contains("inlined-imports:")) would be safer.

✅ What Looks Good

• Architecture: consolidating gate + import-resolver into one always-on AdoScriptExtension with ExtensionPhase::System is exactly right. Phase ordering ensures the user's pinned Node version wins, per-job downloads are correct given ADO VM isolation, and the has_gate() / runtime_imports_active() guards mean zero overhead for pipelines that need neither feature.
• Double-call fix: the setup_job_already_bound guard in compile_shared cleanly prevents the 1ES double-invocation without requiring callers to coordinate.
• Security in import.js: sanitizeForVsoMessage stripping ], \r, \n correctly prevents VSO command injection in error output, and the whitespace-in-source-path compile-time rejection is a well-considered defence against silent runtime truncation.
• Test suite: the four per-job placement tests (gate-only / imports-only / both / neither) directly pin the VM-isolation invariant, and the test_node_runtime_install_orders_after_ado_script_so_user_version_wins test catches the ordering regression that motivated the System phase.
• SHA-256 download verification: grep "ado-script.zip" checksums.txt | sha256sum -c - with unzip -d is correct belt-and-suspenders hardening for the release artifact.

Generated by Rust PR Reviewer for issue #625 · ● 1.9M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Strong implementation with good security coverage — one stale doc addition that should be removed, one narrow missing validation worth noting.

Findings

🐛 Bugs / Logic Issues

• docs/template-markers.md — stale doc addition contradicts the code (docs/template-markers.md, line ~491)

  The added paragraph under {{ threat_analysis_prompt }} says:

  "When inlined-imports: false, the compiler emits a top-level {{#runtime-import ...}} marker pointing at the bundled threat-analysis prompt that ships in ado-script.zip; when inlined-imports: true, the expanded prompt body is embedded directly into the YAML."

  But compile_shared always inlines the threat-analysis prompt via include_str! regardless of inlined-imports:

  let threat_analysis_prompt = include_str!("../data/threat-analysis.md");
  template = replace_with_indent(&template, "{{ threat_analysis_prompt }}", threat_analysis_prompt);

  The PR description itself says explicitly: "The Stage-2 threat-analysis prompt is not runtime-imported." This paragraph should be removed — it will mislead maintainers.

⚠️ Suggestions

• } in source path not validated (src/compile/common.rs, around the agent_marker_path block)

  The code explicitly documents the runtime resolver's regex as [^\s}]+ (stops at }), and validates whitespace — but not }. A source path like my-pipeline}-v2.md would produce a malformed marker ({{#runtime-import my-pipeline}) that the resolver silently truncates, causing a confusing "file not found" at pipeline runtime. Trivially fixable alongside the existing whitespace check:

  anyhow::ensure!(
      !agent_marker_path.contains('}'),
      "runtime-import: agent source path '{}' contains '}}' which would terminate the \
       runtime-import marker early (rename the path or set `inlined-imports: true`)",
      agent_marker_path
  );

  } in filenames is uncommon, but the existing comment calls out the regex boundary — the guard should follow through.

• required_hosts is unconditionally permissive (src/compile/extensions/ado_script.rs, required_hosts())

  The extension always returns ["github.com"] even when inlined_imports == true AND no filters are active (i.e. no steps are emitted at all). This is a no-op in practice since github.com is already in CORE_ALLOWED_HOSTS, but it's a semantic mismatch with the extension's own no-op behavior. Consider returning an empty vec when !self.has_gate() && !self.runtime_imports_active().

✅ What Looks Good

• Path traversal protection is thorough: .. segments blocked in both resolve_imports_inline (Rust) and import.js (TypeScript), with matching test coverage in both layers. The allows_literal_double_dot_in_filename test is a nice edge-case catch.
• VSO command injection hardening in sanitizeForVsoMessage: ], \r, \n removal prevents premature bracket closure and log-splitting attacks in ##vso[task.logissue] diagnostics.
• Per-job download placement is architecturally correct and the four extract_job_block tests are exactly the right regression guards for the VM-isolation bug.
• setup_job_already_bound dedup cleanly fixes the 1ES double-call without adding any new state to the extension trait.
• ExtensionPhase::System ordering rationale (ado-script Node 20.x → user Runtime Node N.x wins PATH) is well-reasoned and the test_node_runtime_install_orders_after_ado_script_so_user_version_wins test pins it.
• inlined_imports: bool with #[serde(default)] is the right type for an off-by-default feature; the false default activates the new behaviour without requiring any front-matter change from existing users.

Generated by Rust PR Reviewer for issue #625 · ● 1.9M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid implementation with thorough test coverage. Two findings worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

docs/runtime-imports.md:43 — ADO_AW_IMPORT_BASE documentation doesn't match the code

The docs say:

For user-facing imports, the compiler sets this to {{ trigger_repo_directory }}.

But resolver_step() in ado_script.rs hardcodes:

ADO_AW_IMPORT_BASE: $(Build.SourcesDirectory)

For single-checkout (the common case), these are the same. For multi-checkout pipelines (where trigger_repo_directory expands to $(Build.SourcesDirectory)/$(Build.Repository.Name)), the base for relative author-written imports like {{#runtime-import shared/snippet.md}} will resolve against the workspace root rather than the trigger repo's subdirectory. The implicit body marker uses the full absolute ADO-expanded path so it's unaffected — only author-written relative imports are concerned.

This could silently produce the wrong resolution for users with custom checkout: entries who write relative markers. Either the docs need to say $(Build.SourcesDirectory) (workspace root) or the resolver step should pass {{ trigger_repo_directory }} — noting that {{ prepare_steps }} is replaced before {{ trigger_repo_directory }} in the replacements loop, so the expansion would propagate correctly if used.

---

scripts/ado-script/src/import/index.ts:4 — runtime MARKER regex silently leaves markers unexpanded for paths containing }

The capture group [^\s}]+ stops at }. If a path contains } (unusual but filesystem-legal), the MARKER regex fails to match the whole marker and the replace callback is never called — leaving the literal {{#runtime-import ...}} text in the agent prompt with no error.

The compile-time resolve_imports_inline uses }} as the end-of-marker terminator and handles } in paths correctly. The anyhow::ensure! whitespace guard in compile_shared doesn't cover }. Paths with } are unusual, but the silent-failure mode (no exit(1), unexpanded marker in prompt) is worse than a loud failure. A matching compile-time validation (!path_str.contains('}')) would keep the behaviour consistent and surface the problem at compile time.

---

⚠️ Suggestions

• ado_script.rs:198-203 — required_hosts() returns ["github.com"] unconditionally even when inlined_imports: true with no filters (i.e., neither feature is active and the extension emits no steps). This is benign because github.com is already in CORE_ALLOWED_HOSTS. The comment acknowledges this intent ("conservatively always requested"). No action required, but conditionalizing on has_gate() || runtime_imports_active() would make the contract precise.

• ado_script.rs:136-161 — generate_setup_job in common.rs re-derives has_pr_gate/has_pipeline_gate independently of AdoScriptExtension::has_gate(), calling lower_pr_filters / lower_pipeline_filters a second time for the same data. The gate check is also inside AdoScriptExtension::setup_steps(). The redundancy is harmless but could cause confusion if the lowering logic drifts.

✅ What Looks Good

• Path traversal protection is consistent between compile-time Rust (resolve_imports_inline) and runtime TypeScript (import.js): .. segments are correctly rejected in both, with matching test coverage on both sides.
• sanitizeForVsoMessage correctly strips ], \r, \n to prevent VSO command injection in error log messages — good defence-in-depth given the marker paths can contain user-controlled content from the trigger repo.
• SHA-256 checksum verification on the downloaded ado-script.zip bundle is a solid supply-chain safeguard (grep ... | sha256sum -c -).
• The four per-job placement tests (test_gate_only_pipeline_downloads_bundle_in_setup_job_not_agent, etc.) cleanly pin the ADO VM isolation architecture and would have caught the reviewer-identified double-call bug before it regressed.
• Phase ordering (System → Runtime → Tool) + test_node_runtime_install_orders_after_ado_script_so_user_version_wins together guarantee the user's pinned Node version wins on PATH, which is easy to get wrong and harder to notice.
• 1ES double-call elimination via the {{ setup_job }} pre-bound detection in compile_shared is clean and correct.
• Single-pass import semantics (no nested expansion) matching gh-aw is well-documented and intentional.
• Compile-time fail-fast for paths with whitespace (anyhow::ensure!(!)) is a good UX choice over a confusing runtime failure.

Generated by Rust PR Reviewer for issue #625 · ● 5M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the consolidation into a single AdoScriptExtension, the per-job VM isolation architecture, and the test coverage are all solid. One documentation inaccuracy needs fixing before merge; one minor observation below.

---

Findings

🐛 Bugs / Logic Issues

• docs/template-markers.md line 492 — incorrect claim about {{ threat_analysis_prompt }}
  The added line says:

  "When inlined-imports: false, the compiler emits a top-level {{#runtime-import ...}} marker pointing at the bundled threat-analysis prompt that ships in ado-script.zip"

  This is false. The threat-analysis prompt is unconditionally include_str!'d and inlined via replace_with_indent at step 11.5 in compile_shared, with an explicit comment saying "always inlined into the emitted YAML regardless of inlined-imports". The PR description also calls this out explicitly. The doc should either be removed or replaced with something like: "The threat-analysis prompt is always inlined at compile time (via include_str!) regardless of inlined-imports."

⚠️ Suggestions

• ado_script.rs — required_hosts() is always ["github.com"] even when no download occurs
  When inlined_imports: true and no filters are configured, neither setup_steps() nor prepare_steps() emits any steps, so the bundle is never downloaded. Yet required_hosts() still adds github.com to the network allowlist. The inline comment acknowledges this as "conservatively always requested" and it's additive/benign, but for a security-sensitive project it's worth tightening to if self.has_gate() || self.runtime_imports_active() { vec!["github.com"...] } else { vec![] }.

• ado_script.rs — double lowering of filters in setup_steps()
has_gate() calls lower_pr_filters() / lower_pipeline_filters(), and then setup_steps() calls them again after the guard to generate actual steps. Both functions are pure so this is just redundant work, but it could be cleaned up by caching the results locally in setup_steps() instead of delegating to has_gate().

✅ What Looks Good

• Path traversal protection is thorough — .. component check in both resolve_imports_inline (Rust) and import.ts (TypeScript), plus the whitespace guard on agent_marker_path to prevent silent marker truncation at runtime. Good defence-in-depth.
• VSO command injection hardening in sanitizeForVsoMessage covers ], \r, and \n — exactly the characters that would break the ##vso[task.logissue...] framing.
• 1ES double-call elimination via setup_job_already_bound detection is correct and well-explained.
• Integrity check semantics are right: body edits don't trip the hash when inlined-imports: false (the YAML only contains a marker, not the body), but do fail when inlined-imports: true (body is embedded). The three new integrity tests pin this behaviour precisely.
• Test coverage is comprehensive across all four (gate × imports) combinations for all four targets, plus per-job placement assertions, whitespace-path rejection, and author-written marker resolution.

Generated by Rust PR Reviewer for issue #625 · ● 2.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security coverage. One logic-ordering concern in the 1ES path and two documented limitations worth making explicit.

---

Findings

⚠️ Suggestions

• src/compile/onees.rs:generate_setup_job — extension steps now precede user setup steps

  Before this PR, the 1ES Setup job only contained user-defined setup_steps. Now ext_setup_steps (NodeTool@0 + bundle download + gate step) are prepended before user steps:

  if !ext_steps_indented.is_empty() {
      body.push_str(&ext_steps_indented);  // extensions first
      body.push('\n');
  }
  if !user_steps_yaml.is_empty() {
      body.push_str(&user_steps_yaml);     // user steps second

  This is almost certainly the right order (Node must be installed before the gate step runs). However, the only tests added check the empty-user-steps cases or gate-placement in standalone. There's no test that exercises the 1ES path with both filters: active (ext steps present) and user-authored setup: steps — which is the combination that would expose incorrect ordering or double-indentation. Worth adding a test_generate_setup_job_with_steps_and_ext_steps unit test.

• src/compile/common.rs — runtime $(Build.SourcesDirectory) whitespace not detectable at compile time

  The whitespace guard:

  anyhow::ensure!(
      !agent_marker_path.chars().any(char::is_whitespace),
      "runtime-import: agent source path '{}' contains whitespace...",
      agent_marker_path
  );

  correctly rejects a space in the alias or relative path portion of the marker. But agent_marker_path contains the unexpanded ADO variable $(Build.SourcesDirectory)/alias/.... If the agent VM's actual build directory expands to a path with spaces (e.g., a non-standard Windows runner at D:\Build Output\1\s\...), the runtime resolver's [^\s}]+ regex would silently truncate the path. This can't be caught at compile time. A comment in resolver_step() or docs/runtime-imports.md flagging this as a known constraint would help future debuggers.

• inlined-imports: false is a silent behavior change on recompile

  This is the intended design, but: any existing pipeline that omits inlined-imports: will change behavior the next time it's compiled — going from an inlined body to a live {{#runtime-import ...}} marker in the emitted YAML. The pipeline will still work as long as the source .md file is reachable on the agent VM, but the failure mode shifts from a compile-time error to a runtime error (resolver exits nonzero → pipeline fails). Scenarios like fork PRs, offline agents, or pipelines triggered by commits that predate the .md file could surface unexpected runtime failures that were previously safe compile-time artifacts. A brief migration note in CHANGELOG.md or docs/runtime-imports.md (e.g. "existing pipelines default to inlined-imports: false on recompile; set inlined-imports: true to preserve the old behaviour") would reduce upgrade surprises.

---

🔒 Security

• Path traversal: both resolve_imports_inline (Rust) and import.js (TypeScript) apply identical ..-segment rejection logic — split on / and \, compare each segment to ".." — before the optional-flag check. {{#runtime-import? ../escape}} is therefore rejected with a hard error, not silently skipped. Tests in path-traversal.test.ts cover the required, optional, and Windows-path cases. ✅

• ##vso injection in error messages: sanitizeForVsoMessage strips ], \r, and \n before embedding rawPath into ##vso[task.logissue type=error].... The fail() function prefixes the message with runtime-import: , making ##vso prefix injection impossible from the user-controlled portion. ✅

---

✅ What Looks Good

• ExtensionPhase::System ordering — ensuring ado-script's NodeTool@0 installs first so the user's Runtime-phase Node install "wins last" on PATH is architecturally correct and verified by test_node_runtime_install_orders_after_ado_script_so_user_version_wins.
• Per-job download design — the four placement tests (gate_only, imports_only, both, neither) are exactly the right regression suite for the VM-isolation bug identified in review. Clean coverage.
• 1ES double-call elimination — detecting a pre-bound {{ setup_job }} in extra_replacements to skip compile_shared's own aggregation is a workable protocol. The comment in onees.rs explaining the contract is sufficient.
• import.js atomic write — writeFileSync is only called after the full errors.length === 0 check, so the prompt file is never left in a partially-expanded state on error.
• resolve_imports_inline is well-structured — byte-offset slicing on ASCII marker delimiters is safe, the cursor bookkeeping is correct, and early-exit on the first required-missing error is the right fail-fast behaviour.

Generated by Rust PR Reviewer for issue #625 · ● 1.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — solid architecture with one security gap worth addressing and a few minor observations.

Findings

🔒 Security Concerns

• src/compile/extensions/ado_script.rs (resolve_imports_inline) — The ..-segment guard correctly blocks ../../../../etc/passwd-style traversal, but absolute paths are allowed without restriction. When inlined-imports: true, {{#runtime-import /home/runner/.ssh/id_rsa}} (or any arbitrary absolute path) in an untrusted branch's markdown body would embed the file contents into the compiled YAML. The PR description explicitly calls out the .. traversal threat for "untrusted PR branches" — the same threat applies to absolute paths. The fix is a simple !Path::new(path_str).is_absolute() guard (or documented acknowledgement that compile-time resolution of absolute paths is intentional even in adversarial scenarios).

  Note: import.js also allows absolute paths at runtime, but the agent VM threat model is lower-risk than the compile-machine model.

⚠️ Suggestions

• src/compile/extensions/ado_script.rs (has_gate()) — lower_pr_filters() and lower_pipeline_filters() are called inside has_gate(), then called a second time inside setup_steps() to produce the actual checks. Each call is cheap, but a let checks = lower_*_filters(filters); if !checks.is_empty() pattern in setup_steps() that reuses the has_gate() result would be clearer and avoid the redundant work. Not a bug.

• scripts/ado-script/src/import/index.ts (sanitizeForVsoMessage) — Strips ], \r, \n to prevent VSO command injection. Worth a brief comment noting that [ is intentionally not stripped: without a ] closer, an attacker-controlled [ in a path can't open a new ##vso[...] bracket, so the omission is correct but non-obvious.

✅ What Looks Good

• Per-job VM isolation architecture is sound and well-justified. The design table in the PR description and the four placement tests (test_gate_only_..., test_imports_only_..., test_both_features_..., test_neither_...) pin the invariant cleanly.

• ExtensionPhase::System ordering ensures the ado-script NodeTool@0 install lands first, so the user's runtimes.node: NodeTool@0 wins on PATH. The test_node_runtime_install_orders_after_ado_script_so_user_version_wins test covers the exact failure mode — excellent signal-to-noise ratio.

• source_path always contains {{ trigger_repo_directory }} (confirmed via generate_source_path always returning "{{ trigger_repo_directory }}/..." at line 1380). The .replace() call is always a valid substitution; no silent no-op.

• inlined_imports: bool field has #[serde(rename = "inlined-imports", default)] — correct field name, correct serde default (false), correct bool type with three deserialization tests.

• ..hidden.md/name..md false-positive test — the segment-aware .. check is correctly scoped to path component equality rather than substring matching. Good defensive test.

• Single-pass expansion is consistent between Rust (resolve_imports_inline iterates the original string) and TypeScript (.replace() is inherently single-pass). This prevents recursive marker injection in imported snippets.

• Compile-time whitespace guard on agent_marker_path — correctly catches paths that would cause silent truncation by the TypeScript regex [^\s}]+. Clear error message pointing the user to the right fix.

• Extension consolidation (TriggerFiltersExtension → AdoScriptExtension) is clean. One source of truth for install/download YAML, one place to update the version string.

Generated by Rust PR Reviewer for issue #625 · ● 2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured feature with good security analysis throughout — one functional gap worth clarifying and a minor regex asymmetry to note.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/extensions/ado_script.rs / import.js (single-pass gap): The PR description says "Author UX bonus — {{#runtime-import path}} work inside any agent's markdown body", but this is only true for inlined-imports: true. With inlined-imports: false (the new default), the entire agent body is loaded as the replacement text for the compiler-generated top-level import. Since import.js is single-pass, any author-written {{#runtime-import shared/snippet.md}} markers inside the body are silently left unexpanded — they appear as literal text in the agent prompt. The docs do mention single-pass and the inlined-imports: true clause covers author markers explicitly, but there's no negative-path test covering this case, and no compile-time error or warning when an author marker is present in a inlined-imports: false pipeline. A user who adds snippet imports and later compiles with the new default will see broken behaviour at runtime with no diagnostic. Consider either: (a) a compile-time warning when the body contains {{#runtime-import and inlined_imports is false, or (b) a test that documents/asserts the current silent-expansion-skipped behaviour so it's deliberate and visible.

• scripts/ado-script/src/import/index.ts — MARKER regex vs. compile-time resolver asymmetry ([^\s}]+ vs. find("}}")): The runtime regex [^\s}]+ stops at the first lone } in a path, while the Rust resolve_imports_inline uses body.find("}}") and correctly handles } in a path segment (it only terminates at }}). A file named foo}bar.md would: compile-time → resolved correctly; runtime → path truncated to foo and then likely "file not found". Extremely unlikely in practice but the asymmetry is worth a comment or making the Rust side equally strict (reject } in paths) for parity.

⚠️ Suggestions

• src/compile/extensions/ado_script.rs:77-79 — has_gate() called by required_hosts() and independently by setup_steps(): Both call lowered_checks(), which lowers the filter IR twice per compilation. The struct-level comment correctly notes this is cheap; a small improvement would be making required_hosts() use the stored result from a prior setup_steps() call, or just computing it once in the caller. Not blocking.

• docs/runtime-imports.md — opening sentence could mislead: "Authors can place runtime-import markers anywhere in the agent markdown body" without immediately qualifying that they are only processed at compile time (i.e., with inlined-imports: true). The subsequent paragraph does clarify, but the lead sentence sets the wrong expectation for the common case (inlined-imports: false). A one-sentence qualification upfront would help.

🔒 Security Concerns

• import.js allows absolute paths at runtime (intentional by design, but worth noting): Compile-time resolve_imports_inline explicitly rejects absolute paths. At runtime, import.js accepts them (needed because the compiler-generated top-level marker uses $(Build.SourcesDirectory)/... which ADO expands to an absolute path before bash runs). The comment in import.js notes import.js is also invoked against arbitrary author-written markers. An author-written {{#runtime-import /etc/os-release}} in their body would be silently resolved at runtime on the agent VM. The agent VM is sandboxed and /etc/os-release is non-sensitive, so this is low-risk, but an explicit comment explaining why absolute-path rejection is intentionally absent from the runtime path (contrast with compile-time) would close the reasoning gap for future reviewers.

✅ What Looks Good

• Per-job download placement: The setup_job_already_bound detection in compile_shared elegantly prevents the 1ES double-invocation of setup_steps() — the old bug (reviewer-caught) is pinned by four crisp placement tests that split the YAML by job block. Good regression coverage.
• Path traversal hardening: Both resolve_imports_inline (Rust) and import.js (TS) independently block .. path segments, with matching test suites. The UNC-path, Windows drive-letter, and dotdot-in-filename edge cases are all covered.
• ExtensionPhase::System ordering: The phase ensures AdoScriptExtension's NodeTool@0 install runs before user-facing NodeExtension, so the user's pinned Node version wins on PATH. The test_node_runtime_install_orders_after_ado_script_so_user_version_wins test pins this ordering precisely.
• inlined_imports: bool field: Correct use of #[serde(rename = "inlined-imports", default)] — deserialises from the hyphenated YAML key, defaults to false, and the three deserialization round-trip tests cover default/true/explicit-false.
• VSO injection hardening in sanitizeForVsoMessage: The ]/\r/\n stripping in diagnostic messages is well-commented and the [ justification is thorough.

Generated by Rust PR Reviewer for issue #625 · ● 3.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the architecture is clean, the security thinking is thorough, and the tests are excellent. One genuine security concern in the TypeScript runtime resolver deserves attention before merge.

---

Findings

🔒 Security Concerns

import.js allows absolute paths at runtime (scripts/ado-script/src/import/index.ts:74)

const absPath = isAbsolute(rawPath) ? rawPath : resolve(base, rawPath);

The Rust compile-time resolver (resolve_imports_inline) explicitly rejects absolute paths with detailed justification — prevent host secrets from being embedded into the compiled YAML. The runtime resolver does not apply the same policy. The code comment says "Mirrors resolve_imports_inline" but it doesn't, and this matters:

Author-written {{#runtime-import /tmp/awf-tools/staging/mcpg-config.json}} in an agent body would succeed at runtime and inline the MCP server configuration (potentially containing credentials) directly into the agent's system prompt. The staging directory and token files are on the same agent VM. The code itself even notes that import.js processes "arbitrary author-written markers in the agent body."

The ..-segment guard is correctly mirrored (good!), but the absolute-path check is the more impactful protection here. Suggest adding:

if (isAbsolute(rawPath)) {
  errors.push(`invalid path '${sanitizeForVsoMessage(rawPath)}': absolute paths are not allowed`);
  return "";
}

---

⚠️ Suggestions

• lowered_checks() called twice per compilation when both has_gate() and setup_steps()/required_hosts() are invoked (src/compile/extensions/ado_script.rs:77-82). The inline comment says "lowering is cheap" which is true, but the doc-comment on lowered_checks itself says it "folds both passes into a single computation that callers reuse" — the "reuse" part isn't happening since each call site recomputes independently. Worth either caching or removing the doc-comment claim about reuse.

• base_dir fallback to . when input_path has no parent (src/compile/common.rs — the inlined-imports: true path): input_path.parent().unwrap_or_else(|| Path::new(".")) silently uses CWD if input_path is a bare filename like "pipeline.md". This is probably fine in practice (callers always pass a real path), but a comment noting the assumption would be helpful.

---

✅ What Looks Good

• Path traversal (..) protection is correctly mirrored in both the Rust and TypeScript resolvers, including the \\-separator case and the false-positive guard for ..filename.md. The tests are excellent and cover the edge cases properly.
• writeFileSync is only called on success — the original agent-prompt file is preserved on any error, preventing partial-expansion corruption.
• sanitizeForVsoMessage correctly strips ] and newlines to prevent VSO logging command injection; the comment explaining why [ is intentionally kept is thorough.
• ExtensionPhase::System ordering ensures the Node.js install + resolver runs before user Runtime extensions, so a user-pinned NodeTool@0 from NodeExtension correctly wins on PATH.
• 1ES double-call elimination via setup_job_already_bound is clean — it avoids running all extensions' setup_steps() twice when the 1ES path has already bound {{ setup_job }}.
• inlined_imports defaults to false correctly via #[serde(default)] on a bool field, and the round-trip tests in types.rs verify all three cases (absent, true, false).
• The consolidated single-extension model is architecturally clean compared to the previous split.

Generated by Rust PR Reviewer for issue #625 · ● 1.9M · ◷