feat(compile): reusable cross-repository custom safe-output components

[jamesadevine]

Summary

Implements #1473: reusable, cross-repository custom safe-output components. Workflow authors can publish typed, secret-bearing safe-output integrations once in a shared (optionally cross-repo, SHA-pinned) component and consume them from many workflows — flowing through the existing Agent → Detection → (approval) → privileged executor trust boundary.

Two mechanisms (gh-aw parity), one dedicated Detection-gated job per definition, both with scoped secrets:

• safe-outputs.scripts.<name> — entrypoint / native dispatch: the job runs a single ado-aw execute --custom-config (the executor owns the loop).
• safe-outputs.jobs.<name> — arbitrary ADO steps:: the job runs execute --custom-phase pre → author steps → execute --custom-phase post.

What's included

• Imports — imports: front matter (bare spec or { uses, with, endpoint }), owner/repo/path@<40-char-sha> resolver with a committed, markdown-only SHA-keyed manifest cache under .ado-aw/imports/<owner>/<repo>/<sha>/<path> (directory structure preserved — no lossy flattening), import-schema validation, and a consumer-wins front-matter merge pass (gated on non-empty imports:).
• Azure Repos is the PRIMARY manifest fetcher — the compile-time fetcher is async and routes by a typed endpoint: absent ⇒ same-org Azure Repos (ADO Git Items API via SYSTEM_ACCESSTOKEN → AZURE_DEVOPS_EXT_PAT → az), type: azure-repos ⇒ cross-org Azure Repos, type: github / type: ghe ⇒ GitHub / GitHub Enterprise via gh. Routing is fail-closed (an Azure-Repos import can never silently fall back to GitHub, and vice-versa) and the compile-time fetch source always matches the runtime checkout source. Auth/org resolve lazily on first fetch, so a fully-vendored committed cache (as used by check/inspect) does zero git/az work.
• Input substitution — import-schema inputs are substituted at compile time as {{ inputs.<key> }} — a compile-time {{ }}-family marker, deliberately not the ADO ${{ }} template delimiter (whose output would be re-processed by ADO once embedded in pipeline YAML). A $-preceded {{ is left verbatim, and any unresolved {{ inputs.* }} is a hard compile error.
• Prompt-body delivery — imported component bodies are substituted and inlined into the agent prompt at compile time (imports-first), while the consumer's own body stays a {{#runtime-import}} marker in the default mode (gh-aw parity).
• Runtime component checkout — remote components get compiler-owned, non-forgeable provenance (component-source/sha/manifest-digest + resolved repo-type/service-connection) stamped onto their tools during merge; the executor job checks the component out with the correct repository-resource type/endpoint and pins the exact commit via the fail-closed checkout-component bundle (no hardcoded refs/heads/main — ADO uses the repo default branch).
• Dynamic MCP tools — config-driven registration in the SafeOutputs MCP server (rmcp ToolRoute::new_dyn) from compile-time-generated closed, scalar-only JSON schemas (additionalProperties: false).
• Executor — one Custom_<tool> job per definition; require-approval custom tools route through ManualReview; versioned CustomExecutionRecord with compiler-owned provenance.
• compile, check, and inspect/graph all resolve imports via one shared merge helper, so the drift check and the read-only IR queries reason about the same fully-merged pipeline compile produces.
• Audit — component provenance surfaced in ado-aw audit with a local # ado-aw-metadata marker cross-check finding.
• Docs — new docs/imports.md plus updates to docs/safe-outputs.md, docs/front-matter.md, docs/network.md, and AGENTS.md.

Security / hardening (from in-branch review)

• Compiler fully owns the component-* provenance keys — author-provided values on an imported component are stripped before stamping, so a component cannot spoof its own checkout (inject a service connection or redirect the source).
• Import cache uses an injective path layout (no a/b.md vs a_b.md collision) so one component's manifest can't be silently served for another.
• Manifest cache verified against a .sha256 digest sidecar on read; nested/transitive imports rejected; path-traversal guarded on both local and remote import paths.

Intentionally deferred (documented)

Transitive/nested imports (depth ≤ 3); array/object agent inputs; Bitbucket sources.

Test plan

• cargo test --bin ado-aw — 2681 unit/bin tests green (incl. new imports, custom_tools, mcp_custom_tools, custom-job, audit-provenance, endpoint-routing, delimiter/leftover-guard, body-delivery, provenance-stamping/spoofing-guard, and the four-target acceptance matrix in compiler_tests).
• cargo clippy --all-targets — clean.
• Integration suites (compiler_tests, codemod_tests, inspect_integration, bash_lint_tests) — green.
• Rebased onto latest main; one incidental test conflict resolved in favour of main's stronger assertion (test: correct mislabelled vacuous test in src/compile/types.rs #1518).

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid architecture and good test coverage. Two issues worth addressing before merge.

Findings

🔒 Security Concerns

• src/compile/imports/mod.rs — resolve_local_path missing .. traversal guard

  The local-import path resolver only blocks absolute paths but does not reject .. components:

  fn resolve_local_path(base_dir: &Path, import_path: &str) -> Result<PathBuf> {
      let path = Path::new(import_path);
      if path.is_absolute() {
          anyhow::bail!(...);
      }
      Ok(base_dir.join(path)) // <- ../../sensitive.md escapes base_dir
  }

  A workflow using imports: ../../.github/some-secret-config.md (or any ..-relative path) would silently read files outside the workflow directory at compile time. This is exactly the check that flatten_import_path already applies to remote import paths:

  path.split('/').any(|segment| segment.is_empty() || segment == "." || segment == "..")

  Apply the same guard in resolve_local_path, bailing if any segment is .. or . (or the string contains \\).

⚠️ Suggestions

• src/compile/imports/mod.rs:389 — non-test .expect() in production code

  let start_level = markdown_heading(lines[start])
      .map(|(level, _)| level)
      .expect("line matched heading above"); // production path

  The invariant holds as written (the ? on position() above guarantees lines[start] is a heading), but a future refactor could silently break it and cause a panic. Prefer ok_or_else(|| anyhow::anyhow!(...)) + ? to stay consistent with the rest of the function.

• src/compile/imports/schema.rs — render_json_value does not sanitize import inputs

  String inputs substituted into the markdown body (agent system prompt) via ${{ ado.aw.import-inputs.key }} are rendered verbatim. A with: value containing ##vso[task.setvariable ...] won't inject ADO pipeline commands (the body goes to the LLM, not a bash: step), but could be used for prompt injection if a component author's published defaults or a consumer's with: values are attacker-controlled. Consider escaping or rejecting ##vso[ sequences in string inputs, or explicitly document the trust boundary (compile-time author choice only).

✅ What Looks Good

• Shell quoting is consistent: every dynamic value in generated bash steps passes through shell_quote() — no command injection surfaces found.
• Closed JSON schemas: additionalProperties: false enforced at schema generation time (custom_tools.rs) and MCP registration (mcp_custom_tools.rs). Built-in tool name collision check is good defence-in-depth.
• CustomExecutionRecord.schema_version: versioning the record format at 1 from day one is the right practice.
• Heredoc safety: the 'CUSTOM_TOOLS_JSON_EOF' and 'ADO_AW_CUSTOM_CONFIG_JSON' delimiters are single-quoted (no variable expansion) and serde_json::to_string escapes all control characters, so JSON content cannot prematurely terminate either heredoc.
• Budget enforcement: per-tool max budget enforced in both scripts and jobs paths; check_budget keyed on context_id is a clean approach.
• Provenance chain: schema_digest + manifest_digest + component_sha threaded from compile time through the executor and surfaced in ado-aw audit is a well-designed trust anchor.

Generated by Rust PR Reviewer for #1508 · 123.6 AIC · ⌖ 5.83 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid implementation with excellent test coverage and defense-in-depth. One genuine security gap found in the import-input substitution path.

Findings

🔒 Security Concerns

• src/compile/imports/schema.rs — render_json_value does not sanitize Array/Object nested strings

  substitute_inputs correctly calls sanitize_config for JsonValue::String values to neutralize ##vso[...] pipeline-command injection, but the Array and Object arms serialize the value with raw serde_json::to_string and no sanitization:

  JsonValue::Array(_) | JsonValue::Object(_) => {
      serde_json::to_string(value).unwrap_or_else(|_| value.to_string())
  }

  import-schema: does support array and object field types (SchemaType::Array and SchemaType::Object both exist and are reachable from parse_schema_type). A consumer that passes a with: value such as:

  with:
    tags: ["##vso[task.setvariable variable=SECRET]leak"]

  ...into a string-scalar front-matter field (e.g. bash: echo ${{ ado.aw.import-inputs.tags }}) would produce output that ADO executes as a pipeline command, bypassing the backtick-wrap neutralization. The existing test substitute_inputs_neutralizes_pipeline_commands_in_string_values only covers the direct-string case. The fix is to recursively sanitize string leaves when serializing arrays/objects, or to prohibit array/object import-schema types from being substituted into string-scalar positions.

⚠️ Suggestions

• src/compile/imports/schema.rs ~line 280 — .expect("checked is_some") in production code

  None if field.default.is_some() => {
      let default = field.default.as_ref().expect("checked is_some");

  Safe today, but brittle if surrounding match arms are ever reordered. Prefer collapsing into if let Some(default) = &field.default { ... } to remove the double-lookup entirely.

• src/execute.rs — parse_script_result_stdout requires exactly one non-empty line

  Authors who accidentally write debug output to stdout (e.g. console.log(...) before the JSON line) will see the opaque error "Custom tool 'X' must print exactly one JSON line, got N". A two-level message ("got 0 lines" vs. "got N lines — is debug output going to stdout?") would improve the authoring experience.

✅ What Looks Good

• Path-traversal guards in both local (resolve_local_path) and remote (flatten_import_path) paths are thorough and well-tested, including the ./x.md dot-segment case.
• ##vso[ injection defense for direct string imports is correctly implemented and tested.
• additionalProperties: false on generated custom-tool JSON schemas is the right closed-schema choice.
• Provenance fields (component_sha, manifest_digest, schema_digest) are compiler-owned and non-forgeable by the executor — correct trust-boundary placement.
• Tool-name collision detection against ALL_KNOWN_SAFE_OUTPUTS is a good safety net.
• Test coverage across the new modules is comprehensive.

Generated by Rust PR Reviewer for #1508 · 81 AIC · ⌖ 5.87 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security discipline. A few items worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/execute.rs — run_custom_entrypoint has no timeout. child.wait_with_output().await blocks until the child exits. A misbehaving or malicious component script (e.g., intentional sleep inf) will hang the entire Stage-3 executor until the ADO job-level timeout fires. Consider wrapping with tokio::time::timeout(Duration::from_secs(...), ...) and failing the proposal with a clear message. The job timeout is an acceptable backstop, but an explicit timeout gives a better error message and faster recovery.

• src/execute.rs — read_component_results hard-fails on schema_version != Some(1). This is correct today, but the check is a hard error with no forward-compatibility path:

  if schema_version != Some(CUSTOM_SCHEMA_VERSION as u64) {
      anyhow::bail!(...);
  }

  If CUSTOM_SCHEMA_VERSION is ever bumped, any in-flight post-phase job that receives a v1 result record (produced by an older component) will fail. Consider accepting >= CUSTOM_SCHEMA_VERSION (readers should tolerate older schema versions) or treating unknown versions as a warning.

• src/execute.rs — parse_script_result_stdout silently discards stderr output on success. When a script exits 0 but emits more than one JSON line (e.g., because it also writes a debug log line to stdout), the executor fails the proposal with "Custom tool '...' must print exactly one JSON line, got N". This is an explicit contract, but it's easy for component authors to trip over. The error message should indicate how many lines were received and show the first unexpected line (truncated + sanitized) to help authors debug.

🔒 Security Concerns

• src/compile/mod.rs — import content is merged before sanitize_config_fields runs. The merged front_matter (which now includes component front matter from SHA-pinned or local imports) is passed to sanitize_config_fields() after the merge. This ordering is correct — sanitization covers the merged result. However, the component body (markdown text) that flows into markdown_body via merge_imports is NOT covered by sanitize_config_fields (which only covers front-matter fields). The body becomes the agent prompt, not a bash step, so ##vso[ in a body string wouldn't be executed — but this asymmetry is worth an explicit comment in compile_pipeline_inner to protect future maintainers from reversing the order.

• src/compile/imports/mod.rs — local import path guard uses a string-split heuristic. The check in resolve_local_path:

  import_path.split('/').any(|segment| segment.is_empty() || segment == "." || segment == "..")

  rejects traversal via path segments, but on Windows the Path::new(import_path) join in base_dir.join(path) could normalize a path differently than the string check expects. Since the project targets ADO (Linux agents) this is low risk, but a path.components().any(|c| matches!(c, Component::ParentDir | Component::RootDir)) check after Path::new() would be platform-safe and more idiomatic.

⚠️ Suggestions

• src/compile/types.rs — FrontMatter.imports carries #[allow(dead_code)] but IS accessed (!front_matter.imports.is_empty() in compile/mod.rs). The attribute is unnecessary and misleading; removing it will help the compiler catch genuine dead code in future. The module-level #![allow(dead_code)] on imports/mod.rs and imports/schema.rs is a broader suppression — consider removing it or scoping it to specific functions once integration is confirmed complete.

• src/compile/agentic_pipeline.rs — detect_custom_proposals_step raw-scan fallback. The RAW_SCAN grep path matches "name"[[:space:]]*:[[:space:]]*"{tool}" as a regex. Since tool names are validated to ASCII alphanumeric/hyphens, there is no regex injection risk — but the fallback grep could match a tool name appearing in a proposal value (e.g., a context field whose value mentions the tool name as a string). The jq path (jq -r 'select(type=="object") | .name // empty') is correct; consider making the raw fallback match "name":"{tool}" as a whole-word pattern or documenting the known false-positive risk.

• src/compile/imports/merge.rs — COLLECTION_MAP_KEYS does not include parameters / repos / variable-groups (those are in SEQUENCE_KEYS). The constant is well-named and the distinction is clear, but safe-outputs is in COLLECTION_MAP_KEYS while the additive sub-key logic also contains a special safe-outputs guard for executor fields. A comment referencing the "consumer-wins" executor-protection rule inline in that guard would help reviewers verify the invariant at a glance.

✅ What Looks Good

• SHA-pinned imports with digest-sidecar verification (read_remote_manifest): fetching, caching, and re-verifying the SHA-256 sidecar on subsequent reads is a solid defense-in-depth against committed-cache tampering.
• Compiler-owned name tag in append_custom_proposal: agent-supplied name is silently overridden, preventing a prompt-injection path where an agent calls a custom tool with a spoofed name to hijack execution routing.
• Closed additionalProperties: false JSON schemas for custom MCP tools: correctly constrains the agent to declared inputs only.
• shell_quote usage throughout the generated bash steps (entrypoint, config path, tool names, provenance args) is consistent and correct.
• read_component_results attempted_ids guard: correctly prevents a component from injecting result records for proposals it never received.
• Error handling is consistently idiomatic (anyhow::bail!, .context(), no silent unwrap on user-facing paths).

Generated by Rust PR Reviewer for #1508 · 96 AIC · ⌖ 6.05 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Large, well-structured PR — the trust-boundary design is sound and error handling is thorough. Two concrete bugs worth fixing before merge, one in the cache layer and one in the repository-resource generator.

---

Findings

🐛 Bugs / Logic Issues

1. flatten_import_path cache-key collisions (src/compile/imports/mod.rs)

fn flatten_import_path(path: &str) -> Result<String> {
    Ok(path.replace('/', "_"))   // "a/b.md" → "a_b.md"
}

A remote import at path a/b.md and a flat file at path a_b.md (same owner/repo) collapse to the same .ado-aw/imports/<owner>/<repo>/<sha>/a_b.md cache entry. The second writer silently overwrites — or, if cached first, serves stale content with no error. A separator that can't appear in a path segment (e.g. %2F, or just including the SHA-scoped directory differently) would fix this. Alternatively, keep the full relative path as a sub-tree under the SHA directory (join each segment) instead of flattening.

---

2. custom_repository_resources drops the endpoint (src/compile/agentic_pipeline.rs, lines ~461-466)

resources.push(RepositoryResource::Named {
    identifier: component.alias,
    kind: "git".to_string(),
    name: format!("{owner}/{repo}"),
    r#ref: Some("refs/heads/main".to_string()),
    endpoint: None,   // ← always None
});

CustomComponentRuntime has no endpoint field, so the service-connection name from the import's endpoint: block is silently dropped. At runtime, ADO needs the service connection to authenticate the checkout of a GitHub/GHE-sourced component — without it, the executor job will fail with a 401/checkout error. The fix is to carry the endpoint through parse_custom_component_runtime → CustomComponentRuntime → custom_repository_resources and pass it into the Named variant, mirroring what build_resources does for regular repos: entries.

---

⚠️ Suggestions

3. #![allow(dead_code)] in four production modules

src/compile/imports/{mod,alias,merge,schema}.rs all carry #![allow(dead_code)], indicating the imports integration isn't fully wired into the main compile path yet. This is a maintenance trap — future refactors of these modules won't get dead-code warnings, and the suppressions may outlive their intent. Worth either removing them when the wiring lands (presumably in a follow-up) or replacing them with targeted #[allow(dead_code)] on the specific items that are intentionally deferred.

4. Merged front matter is never re-validated through FrontMatter serde (src/compile/imports/merge.rs)

After merge_resolved builds the combined Mapping and replaces consumer_fm, the result is a raw serde_yaml::Value. If a component contributes a field with the wrong type (e.g., engine: as a bare string when the consumer expects an object), the merge succeeds silently and the error surfaces much later with a confusing message that doesn't mention the import. A round-trip through serde_yaml::from_value::<FrontMatter>(merged) after the merge — or at least a note in the doc comment about the deferred validation — would produce clearer diagnostics.

---

✅ What Looks Good

• The fail-closed RoutingFetcher (AzureRepos vs GitHub, never silently cross-routed) is exactly the right design.
• validate_cache_segment + flatten_import_path together prevent path traversal in remote cache paths; the local-import guard (.., ., empty segment, backslash) is thorough.
• additionalProperties: false on generated custom-tool schemas prevents agents from submitting undeclared keys, closing a common injection surface.
• with_excluded_tools on SafeOutputsVariant cleanly prevents the standard executor from double-executing custom tool proposals.
• The digest sidecar for cached manifests (detect tampering of committed cache files) is a solid defense-in-depth measure.

Generated by Rust PR Reviewer for #1508 · 73.7 AIC · ⌖ 5.91 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid, well-architected feature with good security hardening. One logic bug in the merge layer and a few suggestions.

Findings

🐛 Bugs / Logic Issues

• src/compile/imports/merge.rs — configure_safe_output non-mapping arm bypasses executor protection

  When a consumer provides a non-mapping value for an imported safe-outputs tool (e.g. safe-outputs: scripts: notify: true or null), the (Some(slot), _) arm silently replaces the entire imported tool definition wholesale — including its executor keys (run, entrypoint, steps). The executor-redefinition guard only fires in the (Some(Mapping), Mapping) arm. This is inconsistent with the stated invariant: a consumer "may configure an imported tool but not redefine its executor."

  Concretely, safe-outputs: scripts: notify: null from a consumer would wipe the component's run: key, causing the executor job to fail at compile time with an obscure error rather than a clear conflict message. The fix is to reject non-mapping consumer values for pre-existing imported safe-output tools:

  (Some(slot), _) => {
      // Consumer attempted to replace an imported safe-output tool
      // with a non-mapping value — reject it.
      anyhow::bail!(
          "import conflict: the consumer must provide a mapping to configure \
           the imported safe-output '{sub_name}' (got a non-mapping value)."
      );
  }

  There is no test covering this case (the test suite covers the mapping-vs-mapping configure path and the executor-key rejection, but not the non-mapping consumer value case).

⚠️ Suggestions

• src/compile/agentic_pipeline.rs — heredoc terminator could theoretically be tripped by pretty-printed JSON

  write_custom_scripts_config_step writes the config JSON into the pipeline via a bash heredoc with a hardcoded terminator ADO_AW_CUSTOM_CONFIG_JSON. The JSON body comes from serde_json::to_string_pretty, and while serde_json escapes embedded newlines in string values (so the entrypoint path can't inject a real newline), a multi-key JSON object rendered by to_string_pretty could in principle coincidentally contain a line that exactly matches the indented terminator. The risk is low (the tool name is [A-Za-z0-9-]-constrained and the indentation is unusual), but using printf '%s' '...' instead of a heredoc would eliminate the class entirely.

• src/compile/imports/mod.rs / merge.rs / schema.rs — #![allow(dead_code)]

  All three new import modules carry #![allow(dead_code)] file-level attributes. If this suppresses warnings on entry points not yet wired to compile, check, or inspect, it should either be removed once wiring is confirmed complete, or replaced with targeted #[allow(dead_code)] annotations on the specific items. A blanket module-level allow can mask accidentally-unreachable code in future additions.

• src/compile/imports/mod.rs:3357 — markdown_heading only recognises # and ##

  The section extractor silently ignores ###+ headings, meaning component.md#SubDetail would error "section not found" even if ### SubDetail exists. If the restriction is intentional (only top-level and second-level sections are addressable), a comment or a better error message ("sections must be # or ## headings") would help authors diagnose the failure.

✅ What Looks Good

• Provenance stamping: Strip-then-stamp in stamp_component_provenance is the right pattern — author-provided component-* keys are cleared unconditionally before compiler-resolved values are written, so a component cannot spoof its own checkout source or service connection.
• Fail-closed routing: The AdoRepoFetcher rejects GitHub-typed specs up front (before any lazy org/auth resolution), and the RoutingFetcher is a single chokepoint. Tests confirm both sides of the guard.
• Cache path layout: Injective (a/b.md ≠ a_b.md), traversal-guarded, and verified against a .sha256 sidecar on read. The regression test for path collisions is particularly well done.
• Schema generation: additionalProperties: false on generated schemas, CUSTOM_TOOL_LIMIT = 10, and HARD_STRING_MAX_LENGTH = 8000 are solid defaults that limit agent-facing attack surface.
• shell_quote: The '...'-with-internal-'\''-escaping implementation is correct for POSIX shells and is applied consistently to all user-controlled values injected into generated bash.
• Custom tool name collision guard: Built-in tool names are blocked at schema generation time before they can shadow a standard safe-output.
• Test coverage: The four-target acceptance matrix and provenance-spoofing unit tests are thorough.

Generated by Rust PR Reviewer for #1508 · 115.7 AIC · ⌖ 5.93 AIC · ⊞ 6.2K · ◷