Skip to content

Commit 56c38b1

Browse files
jamesadevineCopilot
andcommitted
feat(compile): introduce System phase so ado-script installs before user runtimes
ADO's NodeTool@0 prepends to PATH, so when both AdoScriptExtension (gate+import bundle, requires Node 20.x) and NodeExtension (user-pinned version) emit NodeTool@0 into the same Agent job, the LAST install wins on PATH. Previously AdoScript ran in the Tool phase, AFTER Runtime — so its hardcoded 20.x silently overrode the user's pinned version. Introduces a new ExtensionPhase::System variant that sorts before Runtime. AdoScript moves to System: its NodeTool@0 install runs first, the resolver step uses it during a brief 20.x window, then the user's NodeExtension's NodeTool@0 runs and the user's version wins on PATH for everything after. Adds test_node_runtime_install_orders_after_ado_script_so_user_version_wins pinning this ordering. Updates extension-count tests for the new phase. Refreshes ExtensionPhase docs and the collect_extensions ordering policy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 44f4c85 commit 56c38b1

5 files changed

Lines changed: 121 additions & 22 deletions

File tree

src/compile/extensions/ado_script.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,15 @@ impl CompilerExtension for AdoScriptExtension {
122122
}
123123

124124
fn phase(&self) -> ExtensionPhase {
125-
ExtensionPhase::Tool
125+
// System phase: ado-script's NodeTool@0 install + bundle download +
126+
// resolver step must complete BEFORE any user-facing Runtime
127+
// extension (e.g. NodeExtension) runs. Otherwise our Node 20
128+
// install would prepend onto PATH after the user's pinned Node,
129+
// silently overriding the user's choice for the rest of the
130+
// Agent job. By running first, our install lives only during the
131+
// brief window before the user's Runtime install, and the
132+
// resolver step inside that window picks up our Node 20.
133+
ExtensionPhase::System
126134
}
127135

128136
fn setup_steps(&self, _ctx: &CompileContext) -> Result<Vec<String>> {
@@ -294,7 +302,11 @@ mod tests {
294302
fn name_and_phase() {
295303
let ext = ext_with(None, None, true);
296304
assert_eq!(ext.name(), "ado-script");
297-
assert_eq!(ext.phase(), ExtensionPhase::Tool);
305+
// System phase ensures NodeTool@0 install + bundle download +
306+
// resolver run BEFORE user-facing Runtime extensions (e.g. the
307+
// Node runtime), so the user's pinned Node version wins on PATH
308+
// for the rest of the Agent job.
309+
assert_eq!(ext.phase(), ExtensionPhase::System);
298310
}
299311

300312
#[test]

src/compile/extensions/mod.rs

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -213,16 +213,32 @@ impl<'a> CompileContext<'a> {
213213

214214
/// Execution phase for extension ordering.
215215
///
216-
/// Extensions are collected and processed in phase order. Runtimes run
217-
/// before tools because tools may depend on runtimes (e.g., `uv` requires
218-
/// a Python runtime to already be installed).
216+
/// Extensions are collected and processed in phase order. The compiler
217+
/// emits steps in this order: **System → Runtime → Tool**.
218+
///
219+
/// - **System** is reserved for compiler-internal infrastructure that
220+
/// downstream phases assume is already in place (e.g.
221+
/// `AdoScriptExtension`'s prompt-file resolver). System steps emit
222+
/// their own self-contained tool installs and **must finish before
223+
/// any other phase runs**, so that later phases can override shared
224+
/// tool versions (notably the `node` on PATH).
225+
/// - **Runtime** installs language toolchains (Lean, Python, Node, etc.)
226+
/// for the user agent. A `NodeTool@0` here will land on top of any
227+
/// System-phase Node install, so the user's pinned version wins on
228+
/// PATH for everything after Runtime.
229+
/// - **Tool** is first-party tooling (azure-devops, cache-memory, …)
230+
/// that may depend on runtimes.
219231
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
220232
pub enum ExtensionPhase {
221-
/// Language runtimes (Lean, Python, Node, etc.) — installed first.
222-
Runtime = 0,
223-
/// First-party tools (azure-devops, cache-memory, etc.) — may depend
224-
/// on runtimes being available.
225-
Tool = 1,
233+
/// Compiler-internal infrastructure that everything else depends on.
234+
/// Reserved for ado-aw's own extensions (e.g. ado-script). Not for
235+
/// user-facing extension authors.
236+
System = 0,
237+
/// Language runtimes (Lean, Python, Node, etc.).
238+
Runtime = 1,
239+
/// First-party tools (azure-devops, cache-memory, etc.) — may
240+
/// depend on runtimes being available.
241+
Tool = 2,
226242
}
227243

228244
/// Unified interface for runtimes and first-party tools to declare
@@ -624,9 +640,11 @@ extension_enum! {
624640
/// ## Ordering policy
625641
///
626642
/// Extensions are sorted by [`ExtensionPhase`] before being returned:
627-
/// runtimes run before tools. This guarantees that runtime install steps
628-
/// execute before tool steps — critical when a tool depends on a runtime
629-
/// (e.g., a Python-based tool like `uv` needs the Python runtime first).
643+
/// **System → Runtime → Tool**. System owns compiler-internal
644+
/// infrastructure (ado-script bundle download + prompt resolver) that
645+
/// must complete before user-facing toolchains land — notably so that a
646+
/// later `NodeTool@0` from `NodeExtension` wins on PATH instead of
647+
/// being silently overridden by the System-phase Node install.
630648
///
631649
/// Within the same phase, extensions preserve definition order
632650
/// (runtimes in `RuntimesConfig` field order, tools in `ToolsConfig`
@@ -641,6 +659,11 @@ pub fn collect_extensions(front_matter: &FrontMatter) -> Vec<Extension> {
641659
// (Setup job) and the runtime-import resolver (Agent job). Internal
642660
// gating on `filters:` and `inlined-imports` means the extension
643661
// emits no steps when neither feature is needed.
662+
//
663+
// Phase: `System` — so its `NodeTool@0` install + bundle download +
664+
// resolver step run BEFORE any user-facing Runtime extension (e.g.
665+
// `NodeExtension`). The user's pinned Node version then "wins last"
666+
// on PATH for the rest of the Agent job.
644667
extensions.push(Extension::AdoScript(AdoScriptExtension {
645668
pr_filters: front_matter.pr_filters().cloned(),
646669
pipeline_filters: front_matter.pipeline_filters().cloned(),
@@ -696,7 +719,7 @@ pub fn collect_extensions(front_matter: &FrontMatter) -> Vec<Extension> {
696719
// ── Trigger filters + runtime imports are owned by AdoScriptExtension
697720
// pushed above; no separate trigger-filters extension push is needed.
698721

699-
// Enforce phase ordering: runtimes before tools.
722+
// Enforce phase ordering: System → Runtime → Tool.
700723
// sort_by_key is stable, preserving definition order within the same phase.
701724
extensions.sort_by_key(|ext| ext.phase());
702725

src/compile/extensions/tests.rs

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ fn test_collect_extensions_lean_enabled() {
101101
.unwrap();
102102
let exts = collect_extensions(&fm);
103103
assert_eq!(exts.len(), 4); // GitHub + SafeOutputs + ado-script + Lean
104-
assert_eq!(exts[0].name(), "Lean 4"); // Runtime phase sorts first
104+
assert_eq!(exts[0].name(), "ado-script"); // System phase sorts first
105+
assert_eq!(exts[1].name(), "Lean 4"); // Runtime phase follows System
105106
}
106107

107108
#[test]
@@ -141,24 +142,28 @@ fn test_collect_extensions_all_enabled() {
141142
.unwrap();
142143
let exts = collect_extensions(&fm);
143144
assert_eq!(exts.len(), 6); // GitHub + SafeOutputs + ado-script + Lean + AzureDevOps + CacheMemory
144-
assert_eq!(exts[0].name(), "Lean 4"); // Runtime phase first
145-
// All tool-phase extensions follow
146-
assert!(exts[1..].iter().all(|e| e.phase() == ExtensionPhase::Tool));
145+
assert_eq!(exts[0].name(), "ado-script"); // System phase first
146+
assert_eq!(exts[1].name(), "Lean 4"); // Runtime phase next
147+
// All trailing extensions are Tool phase
148+
assert!(exts[2..].iter().all(|e| e.phase() == ExtensionPhase::Tool));
147149
}
148150

149151
#[test]
150152
fn test_collect_extensions_runtimes_always_before_tools() {
151-
// Verify the phase ordering policy: all Runtime-phase extensions
152-
// must appear before any Tool-phase extensions, regardless of
153-
// front matter field order.
153+
// Verify the phase ordering policy: System → Runtime → Tool. All
154+
// System-phase extensions appear first, then Runtime, then Tool —
155+
// regardless of front matter field order.
154156
let (fm, _) = parse_markdown(
155157
"---\nname: test\ndescription: test\ntools:\n azure-devops: true\n cache-memory: true\nruntimes:\n lean: true\n---\n",
156158
)
157159
.unwrap();
158160
let exts = collect_extensions(&fm);
159161
assert_eq!(exts.len(), 6); // GitHub + SafeOutputs + ado-script + Lean + AzureDevOps + CacheMemory
160162

161-
// Find the boundary: last Runtime and first Tool
163+
// System sorts first
164+
assert_eq!(exts[0].phase(), ExtensionPhase::System);
165+
166+
// Runtime extensions all sit between System and Tool
162167
let last_runtime_idx = exts
163168
.iter()
164169
.rposition(|e| e.phase() == ExtensionPhase::Runtime)
@@ -173,6 +178,17 @@ fn test_collect_extensions_runtimes_always_before_tools() {
173178
"Runtime extensions must come before Tool extensions. \
174179
Last runtime at index {last_runtime_idx}, first tool at index {first_tool_idx}"
175180
);
181+
182+
// System must come strictly before Runtime
183+
let first_runtime_idx = exts
184+
.iter()
185+
.position(|e| e.phase() == ExtensionPhase::Runtime)
186+
.expect("expected at least one Runtime extension");
187+
assert!(
188+
0 < first_runtime_idx,
189+
"System extensions must come before any Runtime extension. \
190+
First runtime at index {first_runtime_idx}"
191+
);
176192
}
177193

178194
// ── LeanExtension ──────────────────────────────────────────────

tests/compiler_tests.rs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4347,6 +4347,43 @@ fn test_neither_feature_active_emits_no_node_or_download_anywhere() {
43474347
);
43484348
}
43494349

4350+
/// When a user pins a Node version via `runtimes.node:` AND runtime imports
4351+
/// are active, both extensions emit `NodeTool@0` into the Agent job. ADO's
4352+
/// `NodeTool@0` prepends to PATH, so the LAST install wins. The ado-script
4353+
/// extension must run in the `System` phase so its Node 20.x install lands
4354+
/// FIRST, and the user's Runtime-phase `NodeTool@0 22.x` lands second —
4355+
/// the user's pinned version then wins on PATH for the rest of the job.
4356+
#[test]
4357+
fn test_node_runtime_install_orders_after_ado_script_so_user_version_wins() {
4358+
let yaml = compile_fixture("dedupe_node_runtime_and_imports.md");
4359+
let agent = extract_job_block(&yaml, "Agent").expect("Agent job should exist");
4360+
4361+
// Find offsets within the Agent block. The ado-script Node install
4362+
// is identifiable by its displayName; the user's runtime install
4363+
// carries the explicit user-pinned versionSpec.
4364+
let ado_script_install_idx = agent
4365+
.find("displayName: \"Install Node.js 20.x\"")
4366+
.expect("ado-script Node 20.x install step missing from Agent job");
4367+
let user_runtime_install_idx = agent
4368+
.find("'Install Node.js 22.x'")
4369+
.expect("user runtime Node 22.x install step missing from Agent job");
4370+
4371+
assert!(
4372+
ado_script_install_idx < user_runtime_install_idx,
4373+
"ado-script NodeTool@0 must precede user NodeTool@0 in the Agent job so the \
4374+
user's pinned Node version wins on PATH after both run. \
4375+
ado-script idx = {ado_script_install_idx}, user idx = {user_runtime_install_idx}"
4376+
);
4377+
4378+
// Both downloads of ado-script.zip remain unaffected (still exactly one
4379+
// in the Agent job in this fixture — no filters, so no Setup-side download).
4380+
assert_eq!(
4381+
yaml.matches("Download ado-aw scripts").count(),
4382+
1,
4383+
"Expected exactly one ado-script.zip download (Agent job only; no gate active)"
4384+
);
4385+
}
4386+
43504387
/// Tier 2 PR filter fixture produces valid YAML.
43514388
#[test]
43524389
fn test_pr_filter_tier2_compiled_output_is_valid_yaml() {
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
name: "Dedupe Node Runtime And Imports"
3+
description: "User pins Node 22 via runtime; ado-script must NOT override on PATH"
4+
runtimes:
5+
node:
6+
version: "22.x"
7+
---
8+
9+
## Dedupe Node Runtime And Imports
10+
11+
Used by `test_node_runtime_install_orders_after_ado_script_so_user_version_wins`.

0 commit comments

Comments
 (0)