feat(runtimes): add unified Node.js and Python runtime extensions

[jamesadevine]

Summary

Adds unified Python and Node.js runtime extensions, replacing the divergent approaches in PRs #398 and #399 with a consistent architecture matching the existing Lean runtime pattern.

Python runtime (runtimes: python:):

• UsePythonVersion@0 for version pinning
• PipAuthenticate@1 emitted when feed-url: is set
• PIP_INDEX_URL + UV_DEFAULT_INDEX env var injection via feed-url: field
• Bash commands: python, python3, pip, pip3, uv

Node.js runtime (runtimes: node:):

• Inline NodeTool@0 generation (decoupled from ado-script's node_tool_step())
• npmAuthenticate@0 (+ ensure-.npmrc step) emitted when feed-url: or config: is set
• NPM_CONFIG_REGISTRY env var injection via feed-url: field
• Bash commands: node, npm, npx

Shared infrastructure:

• agent_env_vars() trait method on CompilerExtension with extension_enum! macro dispatch
• collect_agent_env_vars() in common.rs — collects, deduplicates (bails on collision), validates against BLOCKED_ENV_KEYS and reject_pipeline_injection, formats as YAML, wired into compile_shared via {{ engine_env }}
• validate_feed_url() in validate.rs — rejects ADO expressions, pipeline commands, template markers, newlines, and quote characters
• config: field on both runtimes — accepted with a compile-time warning that the config file will not be available inside the AWF agent environment yet (pending gh-aw-firewall#2547)

Key design decisions:

• No AWF mounts or PATH prepends needed — UsePythonVersion@0/NodeTool@0 install to /opt/hostedtoolcache (auto-mounted read-only by AWF) and publish ##vso[task.prependpath] entries that AWF merges via $GITHUB_PATH
• Feed configuration uses env vars only (PIP_INDEX_URL, NPM_CONFIG_REGISTRY) — no config files generated, avoiding conflicts with AWF's ~/.npmrc to /dev/null credential overlay
• Auth tasks are conditional: PipAuthenticate@1 requires feed-url:, npmAuthenticate@0 requires feed-url: or config:
• config: accepted with warning (not error) on both runtimes — mutual exclusivity with feed-url: enforced

Supersedes #398 and #399.

Test plan

• cargo build — compiles cleanly (only pre-existing warnings)
• cargo test — 1224 tests pass (24+ new), 0 failures
• cargo clippy --all-targets --all-features — no new warnings
• New tests cover: extension collection (enable/disable/with-options), YAML front-matter parsing, agent_env_vars() with and without feed-url:, config: warning behavior, config+feed-url mutual exclusivity (both runtimes), validate_feed_url() (valid URLs, missing scheme, injection attempts, quote rejection), prepare_steps with/without feed-url, phase ordering with Python+Node+tools, multiple runtimes simultaneously

[github-actions]

🔍 Rust PR Review

Summary: Solid architecture with two real bugs that need fixing before merge — one is dead code indicating a logic ordering error, and one is a pipeline reliability issue for the Node.js runtime.

Findings

🐛 Bugs / Logic Issues

• src/runtimes/node/extension.rs:79-85 and src/runtimes/python/extension.rs:79-85 — Dead code / unreachable mutual-exclusivity check

  Both validate() implementations have this pattern:

  // Error if config: is set (not yet supported)
  if self.config.config().is_some() {
      anyhow::bail!("...not yet supported...");  // ← always returns here
  }
  
  // Mutual exclusivity: config + feed-url  ← DEAD CODE, never reached
  if self.config.config().is_some() && self.config.feed_url().is_some() {
      anyhow::bail!("...'config' and 'feed-url' are mutually exclusive...");
  }

  The "not yet supported" bail means the mutual-exclusivity check can never execute. When config support lands (gh-aw-firewall#2547), this silent dead code will likely cause a confusing bug where the "mutually exclusive" error is never surfaced. Either swap the order (check mutual exclusivity first, then unsupported) or delete the second check until config is real.

• src/runtimes/node/mod.rs:128-133 — npmAuthenticate@0 hardcodes workingFile: .npmrc, but the file may not exist

  - task: npmAuthenticate@0
    inputs:
      workingFile: .npmrc

  npmAuthenticate@0 fails with a "file not found" error if .npmrc doesn't exist in the working directory. This task is emitted unconditionally (even when no feed-url: is set), which means any user who enables runtimes: node: true without an .npmrc in their repo gets a pipeline failure. Compare with PipAuthenticate@1 which accepts artifactFeeds: '' without needing a config file on disk.

  Fix options:

  1. Only emit npmAuthenticate@0 when feed-url: is set (consistent with the env-var-only philosophy)
  2. Emit it unconditionally but have the compiler also generate a minimal .npmrc with the appropriate registry line
  3. Use a condition: expression and a continueOnError: true if truly unconditional auth is desired

⚠️ Suggestions

• src/compile/common.rs:1942 — YAML value escaping for double-quotes in collect_agent_env_vars

  lines.push(format!("{key}: \"{value}\""));

  reject_pipeline_injection() / validate_feed_url() don't reject " characters. A feed URL containing a literal " (unusual but not blocked) would produce malformed YAML. A simple value.replace('"', "\\\"") before the format call would close this, or add " to the rejection list in validate_feed_url.

✅ What Looks Good

• The agent_env_vars() default-empty trait method + collect_agent_env_vars() with BLOCKED_ENV_KEYS enforcement is a clean, safe extension point.
• validate_feed_url() correctly delegates to reject_pipeline_injection() for the injection checks — no duplicated logic.
• The config: field as a reserved-but-erroring placeholder is a good forward-compatibility pattern.
• Test coverage is thorough — enabled/disabled, boolean/object forms, with/without feed URL, and multi-runtime ordering are all covered.
• SanitizeConfig derive + wiring through RuntimesConfig::sanitize_config_fields() is consistent with the rest of the codebase.

Generated by Rust PR Reviewer for issue #400 · ● 438.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean architecture, solid test coverage, and good injection defenses. One security bug needs fixing before merge; one behavioral inconsistency needs resolution.

Findings

🐛 Bugs / Logic Issues

• config: behavior differs between runtimes — runtimes.python.config is a compile error (via anyhow::bail!), while runtimes.node.config is only a warning (test_node_config_warns_not_functional asserts result.is_ok()). The PR description says "errors at compile time" for both. One of the two is wrong; recommend making both error consistently so users aren't surprised by silently non-functional config.

🔒 Security Concerns

• src/runtimes/node/mod.rs:162 — single-quote injection in generated bash

  echo 'registry={registry}' > .npmrc

  {registry} is the user-supplied feed_url, embedded verbatim inside single quotes. validate_feed_url blocks $(, ##vso[, {{, newlines, and " — but not '. A URL like (example.com/redacted) malicious_cmd; echo ' would close the single-quoted string and inject arbitrary commands into the prepare step, which runs outside the AWF sandbox with full agent privileges.

  Fix: add a single-quote check to validate_feed_url:

  if url.contains('\'') {
      anyhow::bail!(
          "Front matter '{}' contains a single-quote character which would break \
           shell quoting in generated bash steps. Found: '{}'",
          field_name, url,
      );
  }

  And add a test in test_validate_feed_url_injection.

⚠️ Suggestions

• src/runtimes/python/mod.rs — PipAuthenticate@1 with artifactFeeds: '': When artifactFeeds is an empty string, the ADO task's behavior is potentially undefined (it may authenticate against no feeds, or all org feeds depending on agent version). A brief comment explaining the intent (e.g., "empty artifactFeeds uses project-level feeds via service identity") would prevent future confusion. This is especially relevant since the task is emitted unconditionally.

✅ What Looks Good

• The trait-level agent_env_vars() + collect_agent_env_vars() plumbing is clean and correctly blocks BLOCKED_ENV_KEYS and validates key format.
• Using env vars (NPM_CONFIG_REGISTRY, PIP_INDEX_URL) instead of config files to avoid conflicting with AWF's ~/.npmrc → /dev/null overlay is exactly the right approach.
• validate_feed_url composition (reject_pipeline_injection + double-quote + scheme checks) is well-structured.
• Phase ordering (Runtime → Tool) is enforced by ordering in collect_extensions and verified by tests.
• 21 new tests cover the happy paths, edge cases, and validation errors thoroughly.

Generated by Rust PR Reviewer for issue #400 · ● 471.6K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks mostly solid — good architecture, thorough validation, and strong test coverage. Three issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/runtimes/node/mod.rs — generate_ensure_npmrc vulnerable to single-quote in URL

  The registry URL is embedded into a bash single-quoted string:

  echo 'registry={registry}' > .npmrc

  validate_feed_url rejects " but not '. A feed URL containing a single quote (e.g. (example.com/redacted) would produce invalid bash syntax in the generated pipeline step, causing a runtime failure. Fix: add url.contains(''')check tovalidate_feed_url, or switch the generated script to use printf '%s\n' "registry=$REGISTRY"` with the value passed via env.

• Misleading doc comments on generate_npm_authenticate and generate_pip_authenticate

  Both function docs say "Emitted unconditionally when the [runtime] is enabled", but prepare_steps() in both extensions only pushes them when feed_url.is_some() || config.is_some(). The tests confirm the conditional behavior. The doc comments need correcting — or, if unconditional emission was the intended design (so the pipeline identity always sets up credential helpers), the prepare_steps implementation is wrong.

⚠️ Suggestions

• src/runtimes/python/extension.rs — uv promised as pre-installed, but no install step

  required_bash_commands() includes "uv" and the prompt supplement states "uv is pre-installed and recommended for speed". However, there is no pip install uv or equivalent in prepare_steps(). Azure DevOps hosted runners do not pre-install uv the way GitHub Actions runners do. Agents guided to use uv will get command not found. Either add an install step (e.g. a pip install uv bash step after UsePythonVersion@0) or soften the prompt supplement to say uv may need to be installed first.

✅ What Looks Good

• collect_agent_env_vars validates keys against BLOCKED_ENV_KEYS, checks format, and guards against ##vso[ injection — exactly right.
• validate_feed_url catches ADO expressions, pipeline commands, template markers, newlines, and missing scheme — solid defence-in-depth.
• config/feed-url mutual exclusivity check is clean and well-tested.
• Phase ordering (Runtime before Tool) is verified by an explicit test — good regression guard.
• Consistent use of SanitizeConfig derive on new config types and #[serde(default)] on all optional fields.

Generated by Rust PR Reviewer for issue #400 · ● 740.4K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid architecture, well-tested, a few concrete issues to address before merge.

Findings

🐛 Bugs / Logic Issues

• docs/runtimes.md — incorrect "always injected" claim (docs/runtimes.md:67, docs/runtimes.md:99):
  The docs list "PipAuthenticate@1" / "npmAuthenticate@0" as unconditionally injected in the "When enabled, the compiler:" bullets, but both prepare_steps() implementations gate them behind feed_url().is_some() || config().is_some(). A user who enables runtimes: python: true without a feed-url will get no auth step, contrary to what the docs say. Worth correcting both bullet points.

• collect_agent_env_vars does not deduplicate (src/compile/common.rs:134–184):
  The PR description says this function "collects and deduplicates" env vars, but the implementation pushes every (key, value) pair directly with no dedup pass. If two extensions declare the same key (or one is enabled twice through some future path), the rendered YAML would contain duplicate keys — technically valid YAML (last-write wins) but silently surprising. A simple seen HashSet<&str> guard, or bail! on collision, would match the stated contract.

🔒 Security Concerns

• collect_agent_env_vars validates values with contains_pipeline_command only (src/compile/common.rs:163–169):
  This checks for ##vso[ / ##[ but does not call reject_pipeline_injection, which additionally catches ADO expression syntax ($(var), ${{ }}, $[expr]). The feed-url is already gated by validate_feed_url → reject_pipeline_injection before reaching this point, so the risk is low today — but the defence-in-depth at collection time is incomplete. Should call validate::reject_pipeline_injection(&value, &key) here for consistency with how other user-controlled values are handled.

• ${VAR} patterns not rejected in feed URL → bash double-quoted echo (src/runtimes/node/mod.rs:419–435):
  validate_feed_url blocks $(, ${{, and $[ but not ${VAR}. The generated generate_ensure_npmrc bash step embeds the registry URL in a double-quoted echo (echo "Created .npmrc with registry={registry}"), where ${FOO} would be expanded by the shell. This is a diagnostic-only line (the actual .npmrc write uses single-quotes and is safe), so severity is low, but adding ${ to reject_pipeline_injection's pattern list — or using single quotes for both echo statements — would close the gap.

⚠️ Suggestions

• PipAuthenticate@1 with artifactFeeds: '' (src/runtimes/python/mod.rs:203–209):
  When feed-url: is set, the intent is to authenticate to an internal Azure Artifacts feed. However PipAuthenticate@1 with an empty artifactFeeds string does not configure credentials for any specific feed — it processes zero feeds. The env var PIP_INDEX_URL correctly points pip at the right URL, but ADO's Azure Artifacts feeds require an authenticated index URL (the token-bearing form returned by the authenticate task). If the internal feed requires auth, an empty artifactFeeds means pip will receive a 401 at runtime. Consider accepting an optional feed-name: field (e.g. "myOrg/myFeed") and passing it to artifactFeeds when set, or at minimum documenting that unauthenticated feeds are the only supported case today.

✅ What Looks Good

• The extension_enum! macro dispatch for agent_env_vars() is clean and consistent with the rest of the trait surface.
• validate_feed_url correctly rejects quote characters before they can break YAML or bash quoting, and the quote-rejection tests are thorough.
• Using env vars (PIP_INDEX_URL, NPM_CONFIG_REGISTRY) instead of config files to avoid conflicting with AWF's ~/.npmrc → /dev/null overlay is the right call.
• The config: reserved-but-not-functional pattern with a clear compile warning is good forward compatibility.
• Test coverage is solid: enable/disable, with-options, agent_env_vars with/without feed, mutual exclusivity, phase ordering.

Generated by Rust PR Reviewer for issue #400 · ● 437.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good overall structure and security posture — a few correctness issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/runtimes/python/extension.rs:62-68 and src/runtimes/node/extension.rs:62-71 — auth steps are conditional, not unconditional
  The PR description and "Key design decisions" section both state: "Authenticate tasks emitted unconditionally (pipeline identity handles auth regardless of feed-url:)." The code contradicts this: both PipAuthenticate@1 and npmAuthenticate@0 are only emitted when feed_url().is_some() || config().is_some(). If the design intent is always-on auth (so the pipeline identity is pre-configured even for public feeds), the conditional guards are a bug. If conditional is correct, the description is wrong and should be updated — but the description was clearly intentional.

• src/runtimes/python/mod.rs:130-135 — PipAuthenticate@1 with artifactFeeds: '' may be a no-op

  pub fn generate_pip_authenticate() -> String {
      "\
  - task: PipAuthenticate@1
    inputs:
      artifactFeeds: ''

  The PipAuthenticate@1 task expects either a non-empty artifactFeeds (comma-separated Azure Artifacts feed names) or pythonDownloadServiceConnections. Passing an empty string may cause the task to authenticate against nothing, silently succeeding but providing no credentials. This would make the "unconditional auth" design meaningless. If the intent is to authenticate all org feeds, the correct approach is to omit artifactFeeds entirely or use *, not ''.

• src/runtimes/python/extension.rs and src/runtimes/node/extension.rs — config: warns, not errors
  The PR description says config: "errors at compile time ('not yet supported; use feed-url instead')". The code only pushes a warning — it only errors when config and feed-url are both set simultaneously. If the intent is to error on config: alone (to prevent users from unknowingly relying on a non-functional field), the validate method needs anyhow::bail! instead of warnings.push(...) when only config is set.

⚠️ Suggestions

• src/runtimes/node/mod.rs:155 — hardcoded "22.x" default will become stale

  let version = config.version().unwrap_or("22.x");

  Node 22 LTS has a fixed EOL. Consider defaulting to "lts" (which ADO's NodeTool@0 supports) so pipelines automatically track the latest LTS without requiring front-matter edits. Python's "3.x" default is already version-flexible by comparison.

• src/compile/common.rs:1932-1936 — deduplication precedes blocked-key check
  A duplicate of a blocked key (e.g., two extensions both emitting GITHUB_TOKEN) produces "already declared" rather than "conflicts with compiler-controlled variable". The order of checks (dedup → blocked → format → inject) means the error message is misleading in that edge case. Reordering to check blocked keys first would produce clearer diagnostics.

✅ What Looks Good

• validate_feed_url is thorough: covers ADO expressions, ##vso[ commands, template markers, newlines, quotes, and scheme checks — defence-in-depth before any URL lands in YAML or bash.
• generate_ensure_npmrc preserves existing .npmrc: the conditional if [ ! -f .npmrc ] guard correctly avoids clobbering repo-checked-in files.
• extension_enum! macro dispatch for agent_env_vars() is clean; the new method follows existing trait conventions exactly.
• Test coverage is solid: 21 new tests covering enable/disable, feed-url, config warnings, mutual exclusivity, injection attempts, and env var cardinality.

Generated by Rust PR Reviewer for issue #400 · ● 472.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good structure and security-conscious design — two issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/runtimes/node/mod.rs:108 and src/runtimes/python/mod.rs (analogous) — The config field doc comment says "specifying this field produces a compile error", but the implementation emits a warning and continues. The compile error description is incorrect and will mislead users.

• src/runtimes/node/mod.rs:generate_node_install / src/runtimes/python/mod.rs:generate_python_install — The version string is embedded into a YAML single-quoted scalar (versionSpec: '{version}'), but reject_pipeline_injection (the only validation called on version) does not reject ' characters. A version like 3.12' produces versionSpec: '3.12'', which is a YAML parse error (unterminated scalar spans into subsequent lines). validate_feed_url correctly rejects quotes for URLs — the same treatment is needed for version fields. Fix: add if version.contains('\'') { bail!(...) } in the version validation path, or extend reject_pipeline_injection / add a dedicated validate_version_spec helper.

⚠️ Suggestions

• src/runtimes/node/mod.rs:generate_ensure_npmrc — The feed_url value is interpolated into bash single-quoted strings (echo 'registry={registry}') without any local validation. This is safe today because compile_shared always calls ext.validate() (step 3) before generate_prepare_steps (step 4), and validate_feed_url already rejects '. But the function has no guard or doc comment expressing this precondition — a future caller could generate malformed bash. Consider adding a precondition comment or an assert!/debug_assert! that the feed URL has been validated.

• src/compile/common.rs:collect_agent_env_vars — The quote-rejection guard (value.contains('"') || value.contains('\'')) is good defense-in-depth, but it fires after reject_pipeline_injection. Since validate_feed_url (called in validate()) already rejects quotes upstream, this is more of a belt-and-suspenders check, which is fine. Just noting it's intentional, not redundant.

✅ What Looks Good

• validate_feed_url correctly rejects both quote types, injection patterns, newlines, and non-http(s) schemes — solid security layer.
• collect_agent_env_vars validates key format, checks BLOCKED_ENV_KEYS, deduplicates with collision detection, and sanitizes values before YAML emission. Well done.
• Mutual exclusivity between config and feed-url is validated and tested.
• compile_shared ordering (validate → prepare_steps) is correct; the runtime extensions are safe within the current pipeline.
• Test coverage is comprehensive — happy path, disabled case, with/without feed URL, config-warns, mutual exclusivity, and multi-runtime ordering.

Generated by Rust PR Reviewer for issue #400 · ● 930.8K · ◷