feat(compile): configure threat detection

[jamesadevine]

Summary

• add gh-aw-compatible safe-outputs.threat-detection boolean and object configuration
• support append-only literal prompts, Detection-specific Copilot overlays, and trusted pre/post steps
• preserve the Detection graph, analyzed-output artifact, and manual-review outputs when AI analysis is disabled
• resolve Detection install, auth, provider, firewall, metadata, lint, audit, and documentation from the effective configuration

Closes #1701

Test plan

• cargo test
• cargo test --test bash_lint_tests
• cargo clippy
• direct IR test with hard-coded expected edges, output locations, and promoted outputs for enabled and disabled Detection
• manual comparison with ado-aw inspect, ado-aw graph dump, and ado-aw whatif --fail Detection

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured feature with solid test coverage; one code-quality concern and a subtle validation footgun worth discussing.

---

Findings

⚠️ Suggestions

src/inspect/lint.rs — lint_front_matter_tasks uses .expect() on a public function

let threat_detection = front_matter
    .threat_detection_config()
    .expect("front matter was validated before lint task checks");

The invariant holds today because the only caller is build_lint in cli.rs, which first calls build_pipeline_ir (which validates). But lint_front_matter_tasks is pub, so direct callers — tests with hand-crafted invalid front matter, the MCP author server, future internal consumers — can hit a panic on malformed threat-detection config rather than receiving an error they can handle or surface as a lint finding. The signature -> Vec<LintFinding> gives no indication it can panic.

Options (in preference order):

1. Change the return type to anyhow::Result<Vec<LintFinding>> and propagate with ?.
2. Convert the error to a LintFinding with LintSeverity::Error instead of panicking — keeps the signature stable and matches how other lint errors are surfaced.

---

src/compile/types.rs — explicitly writing threat-detection: true blocks existing --model/--api-target args

validate_threat_detection fires its --model/--api-target arg guard whenever safe_outputs.contains_key(THREAT_DETECTION_KEY). This means a user who currently has engine.args: [--model=foo] and now explicitly writes threat-detection: true (e.g. to satisfy a policy requirement) gets a compile failure, even though the effective Detection behaviour is unchanged from the implicit default.

The error message is excellent and the test is clear about intent. Worth considering whether the guard should only fire when an engine override is present (threat_detection.engine.is_some()), consistent with the job-timeout logic which already gates on that predicate.

---

src/engine.rs — Engine::args is annotated #[allow(dead_code)] but not removed

#[allow(dead_code)]
pub fn args(...)

If no path needs the original delegation form, remove it. If it's intentionally kept as a stable entry-point, a short doc-comment would prevent it from being pruned in a future cleanup pass.

---

✅ What Looks Good

• Prompt sanitization is thorough: ##vso escaping, ADO expression rejection (${, $(, $[), and a test confirming null-byte reconstruction attempts ($\0(SECRET)) are all caught before the prompt is written to disk.
• deny_unknown_fields on ThreatDetectionConfig catches typos (e.g. promtp) at parse time — the test coverage here is solid.
• Engine overlay merge semantics are well-modelled: args: [] explicitly clears inherited args, env merges by key with Detection values winning, and a typed provider: strips inherited legacy COPILOT_PROVIDER_* env keys before the merge to prevent silent conflicts.
• Detection job timeout is correctly scoped to only apply when an explicit engine overlay is present — consistent with the "explicit override, explicit effect" principle.
• Test coverage is comprehensive: disabled passthrough, custom prompt/engine/steps, cross-target compilation, provider isolation, and the source-path injection guard are all exercised.

Generated by Rust PR Reviewer for #1716 · 74.6 AIC · ⌖ 5.86 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the feature is well-structured, tests are comprehensive, and the security-sensitive paths (prompt sanitization, ADO expression rejection, provider env isolation) are correctly handled. A few findings worth addressing.

---

Findings

⚠️ Suggestions

• src/inspect/lint.rs:349 — expect() creates a hidden precondition contract

  let threat_detection = front_matter
      .threat_detection_config()
      .expect("front matter was validated before lint task checks");

  This is safe today because build_lint → build_pipeline_ir → build_pipeline_context → validate_threat_detection runs first and returns an Err before lint_front_matter_tasks is reached. But the precondition is invisible at the call site and fragile: any future caller of lint_front_matter_tasks that skips the IR build path will panic on a user-authored typo. Consider returning the error instead:

  pub fn lint_front_matter_tasks(front_matter: &FrontMatter) -> anyhow::Result<Vec<LintFinding>> {
      let threat_detection = front_matter.threat_detection_config()?;
      ...
  }

  Or propagate via a LintFinding with severity Error so the function signature stays consistent. Either way, a panic here would surface as an internal server error in lint_workflow MCP instead of a structured lint result.

• src/engine.rs:299 — #[allow(dead_code)] signals an incomplete refactor

  The original Engine::args is now a thin wrapper over args_with_config but is marked dead. Either remove it (all callers have been updated to use args_with_config) or keep it without the suppression if it belongs in the public API. The current state silently hides a code smell.

• src/compile/types.rs — threat_detection_config() is parsed three times per compile

  build_pipeline_context calls validate_threat_detection() at line 141 (which calls threat_detection_config() internally), then calls it again at line 183. That is two full serde_json deserializations + sanitize passes on the same YAML value; lint_front_matter_tasks adds a third. Consider having validate_threat_detection accept a pre-parsed &ThreatDetectionConfig to eliminate the redundant work in the hot compile path.

• src/compile/agentic_pipeline.rs:1316-1319 — Detection timeout is silently suppressed unless an explicit engine overlay is present

  if cfg.threat_detection.is_enabled()
      && cfg.threat_detection.engine.is_some()   // ← guard
      && let Some(minutes) = cfg.detection_engine_config.timeout_minutes()

  When threat-detection: true (no overlay) and the top-level engine.timeout-minutes is set, detection_engine_config is a clone of the agent engine (with the timeout present), but the engine.is_some() guard prevents it from being applied to the Detection job. If intentional (decouple Detection timeout from Agent timeout), please add a doc comment on the guard. If unintentional, removing the engine.is_some() check would let the effective timeout flow through.

• src/engine.rs:922 — intermediate keys.sort() in copilot_detection_env is redundant

  Non-provider keys are sorted before appending, but pairs.sort() at line 930 re-sorts everything together. The line 922 sort is a no-op and can be removed.

---

✅ What Looks Good

• ADO-expression rejection on threat-detection.prompt is solid — sanitize then validate, with tests covering $(...), ${{...}}, $[...], and null-byte reconstruction.
• SAFE_OUTPUT_RESERVED_KEYS updated to include "threat-detection", preventing it from being treated as a tool name in budget or --enabled-tools validation.
• DetectionEngineConfig::resolve() correctly strips inherited COPILOT_PROVIDER_* keys before merging when a typed provider overlay is set — the conflict test case is well-constructed.
• The IR graph test validates both job-edge structure and output declarations for enabled and disabled Detection in a single fixture, which is a good pattern.
• generate_allowed_domains_for_engine refactoring is clean — the original function becomes a one-liner delegation with no API break.

Generated by Rust PR Reviewer for #1716 · 106.8 AIC · ⌖ 5.96 AIC · ⊞ 6.3K · ◷

[jamesadevine]

@copilot based on the changes in this PR give me representative frontmatter configurations for the new threat detection frontmatter. Respond here.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — solid feature with comprehensive tests. One minor step-ordering change and a redundant re-parsing pattern worth noting, but no blocking issues.

Findings

⚠️ Suggestions

• agentic_pipeline.rs — copy_logs_step ordering changed for enabled detection

  In the original code, copy_logs_step was the final step before the artifact publish, running after the reviewedProposals output-variable step. In the new code copy_logs_step moves inside the is_enabled() block and runs before the reviewedProposals detection step (which falls outside the if/else):

  Old: ...evaluate_threat_analysis → [reviewedProposals] → copy_logs → Publish
  New: ...evaluate_threat_analysis → copy_logs → [reviewedProposals] → Publish
  

  Functionally harmless (the two steps are independent), but it is a silent ordering change for existing compiled pipelines. A brief comment noting the intentional move would help future readers.

• types.rs / ado_aw_marker.rs — threat_detection_config() called 3+ times per compile

  threat_detection_config() re-parses, re-sanitizes, and re-validates on every call. It is invoked separately in build_pipeline_context, CompileMeta(redacted) collect_path_layout_warnings(via.ok()), and lint_front_matter_tasks. Idempotent so not a correctness bug, but the StandaloneCtxalready carries the parsedThreatDetectionConfig` — the marker extension could read from the pre-built context instead of re-parsing.

• types.rs — DetectionEngineConfig::resolve() only strips COPILOT_PROVIDER_* from the base env, not from the overlay env

  if overlay.provider.is_some() {
      if let Some(env) = &mut options.env {
          env.retain(|key, _| !key.starts_with("COPILOT_PROVIDER_"));
      }
  }
  if let Some(value) = &overlay.env {
      merged.extend(value.clone()); // overlay can re-introduce COPILOT_PROVIDER_* keys
  }

  If a user sets both provider: and env: { COPILOT_PROVIDER_TYPE: azure } in the Detection overlay the cleanup strips the base keys but the overlay re-adds them. The conflict test confirms validate_threat_detection_config catches this (no silent bypass). A comment here noting that validation is the intended backstop would prevent a future reader from "fixing" the partial strip into a full one and accidentally hiding the validation path.

✅ What Looks Good

• Injection prevention is thorough: ADO expression rejection (${{, $(, $[) in the custom prompt, reject_pipeline_injection added to the source path, #[serde(deny_unknown_fields)] on ThreatDetectionConfig, and sanitize_config applied before the expression check — good layered defence.

• Disabled-detection path preserves the pipeline graph contract: threat_analysis_disabled_step uses the same StepId("threatAnalysis") and OutputDecl("SafeToProcess") as the enabled path, so SafeOutputs/ManualReview dependency expressions work identically in both modes. The IR graph test locks this invariant with hard-coded edge and output assertions.

• args: [] clears inherited args — the distinction between omitted (inherit) vs explicit-empty (clear) is correctly modelled with Option<Vec<String>> and documented clearly in the struct comment.

• lint_front_matter_tasks signature upgrade (-> anyhow::Result<Vec<...>>) is the right call now that the function can fail on malformed config, and the call site propagates the error correctly.

Generated by Rust PR Reviewer for #1716 · 90.2 AIC · ⌖ 8.27 AIC · ⊞ 6.3K · ◷

[Copilot]

@copilot based on the changes in this PR give me representative frontmatter configurations for the new threat detection frontmatter. Respond here.

# 1) Minimal (default behavior: threat detection enabled)
name: example
description: demo
safe-outputs:
  create-pull-request: {}

# 2) Boolean shorthand: explicitly disable AI analysis (pass-through Detection job remains)
name: example
description: demo
safe-outputs:
  create-pull-request: {}
  threat-detection: false

# 3) Object form: custom additional detector instructions
name: example
description: demo
safe-outputs:
  create-pull-request: {}
  threat-detection:
    enabled: true
    prompt: |
      Also check for:
      - authentication bypasses
      - unsafe deserialization
      - hardcoded credentials

# 4) Object form with Detection-specific engine overlay + trusted pre/post steps
name: example
description: demo
engine:
  id: copilot
  model: claude-opus-4.7
  args: [--reasoning-effort=high]
  env:
    SHARED_SETTING: agent

safe-outputs:
  create-pull-request: {}
  threat-detection:
    engine:
      model: gpt-5-mini
      args: []
      env:
        SHARED_SETTING: detection
        DETECTION_ONLY: "true"
    steps:
      - bash: ./scripts/prepare-security-scanner.sh
        displayName: Prepare security scanner
    post-steps:
      - bash: ./scripts/run-security-scanner.sh
        displayName: Run security scanner

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the threat-detection configuration feature is well-structured with solid security practices. One minor logic asymmetry worth noting.

Findings

⚠️ Suggestions

• src/compile/types.rs DetectionEngineConfig::resolve (Simple branch): When threat-detection.engine is a simple string (e.g. gpt-5-mini), it replaces the engine ID but does not strip inherited COPILOT_PROVIDER_* env keys the way the Full overlay path does when overlay.provider.is_some(). A user switching engines via simple string may silently carry over legacy BYOK provider credentials into Detection. Low severity since simple-string overrides are Copilot-only today, but the asymmetry is worth a comment or matching the Full strip behaviour.

✅ What Looks Good

• Prompt injection defense: Custom prompt strings go through sanitize_config → ADO-expression rejection → SHA-derived heredoc sentinel in prepare_threat_analysis_prompt_step. Triple-layer protection is the right pattern.
• SafeToProcess output symmetry: Both enabled and disabled code paths call .with_id(StepId::new("threatAnalysis")), so dependencies.Detection.outputs["threatAnalysis.SafeToProcess"] resolves identically in both branches — no silent pipeline wiring breakage when detection is disabled.
• Reserved-key update: Adding threat-detection to SAFE_OUTPUT_RESERVED_KEYS correctly prevents it from leaking into --enabled-tools, Stage-3 budgets, or unknown-key validation.
• serde conversion correctness: safe_outputs is HashMap<String, serde_json::Value> (types.rs:1254), so serde_json::from_value on value.clone() is zero-copy and lossless — not a YAML-to-JSON lossy round-trip.
• Test coverage: IR graph test covering both enabled/disabled shapes, engine overlay merge/replace semantics, COPILOT_PROVIDER_* stripping, and path-layout warnings on nested steps is thorough.

Generated by Rust PR Reviewer for #1716 · 88.7 AIC · ⌖ 5.7 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured feature with comprehensive tests and sound security boundaries. A few observations worth discussing.

Findings

⚠️ Suggestions

• src/compile/agentic_pipeline.rs:298-302 — validation path diverges from generation path
  The injection check uses common::normalize_source_path(input_path), but the actual runtime path is produced by common::generate_source_path(input_path) (which internally calls normalize_relative_path, a different code path). In the common case both produce the same filename component, but edge cases that normalize_relative_path handles differently (e.g. absolute paths falling back to filename-only) could theoretically pass validation yet produce a different string in the generated YAML. A single helper returning both the validated and generation-ready form would close this gap definitively — or at minimum a comment linking the two to explain why divergence is safe.

• src/compile/types.rs — args replacement vs env merge semantics in DetectionEngineConfig::resolve()
  When a Detection engine overlay is present, args replaces inherited args (options.args = value.clone()) while env merges (overlay keys win). This is documented in the struct comment, but authors who add --reasoning-effort=low may expect it to append to inherited agent args rather than replace them. The error message from validate_threat_detection_config when --model/--api-target appear in effective args could hint proactively that args: [] is required first to clear an inherited flag before the check fires.

• src/compile/types.rs — provider env-strip ordering in resolve()
  When overlay.provider.is_some(), inherited COPILOT_PROVIDER_* keys are stripped before the overlay env map is merged. The comment correctly explains that an overlay setting both provider: and env: { COPILOT_PROVIDER_BASE_URL: "..." } will surface a validation conflict rather than a silent double-set. The logic is correct, but this ordering is subtle enough that a dedicated test ("overlay with both provider: and env.COPILOT_PROVIDER_BASE_URL should error") would harden the invariant against future refactoring.

✅ What Looks Good

• The three-level config resolution (Bool → ThreatDetectionSetting → ThreatDetectionConfig) is clean and mirrors the gh-aw pattern.
• THREAT_DETECTION_KEY added to SAFE_OUTPUT_RESERVED_KEYS — no missed exclusion from tool-name enumeration.
• lint_front_matter_tasks returning anyhow::Result is the right call; surfacing the fallible threat_detection_config() call beats silently swallowing it.
• The disabled-detection pass-through preserves the full Detection job shape (typed outputs threatAnalysis/reviewedProposals, artifact publish, ManualReview conditions) — the downstream graph remains structurally identical whether or not AI analysis runs, which avoids conditional pipeline topology bugs.
• Security: sanitize_config + contains_ado_expression guard on the custom prompt field is correctly placed in threat_detection_config() so it runs at both compile-time and lint-time.
• Test coverage is solid: disabled case, provider-scoped firewall, cross-target compilation, source-path injection rejection, and IR edge assertions for enabled/disabled detection are all present.

Generated by Rust PR Reviewer for #1716 · 64.7 AIC · ⌖ 5.77 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — feature is well-structured and thoroughly tested. One logic asymmetry and one minor inefficiency worth noting.

Findings

⚠️ Suggestions

• agentic_pipeline.rs — detection timeout not inherited from top-level engine config. The new timeout block requires cfg.threat_detection.engine.is_some(), so a workflow with engine.timeout-minutes: 60 and no detection overlay gets an unconstrained Detection job. No regression vs. before this PR (Detection never had a timeout set), but it's asymmetric with how other engine fields are inherited via resolve(). Worth a brief note in docs/safe-outputs.md that Detection has no timeout unless an explicit overlay is provided.

• agentic_pipeline.rs — detection_engine_install_steps_yaml computed unconditionally. Built at context construction time even when threat_detection.is_enabled() == false, where it's never used. Minor waste; a guard at the call site would avoid the fallible install_steps work when detection is off.

✅ What Looks Good

• New source_path_suffix injection guard (reject_pipeline_injection on the user-controlled portion of the input path before it is embedded in bash steps) is solid proactive hardening. The companion integration test workflow_source_path_rejects_ado_expression_before_prompt_embedding gives it real coverage.
• resolve() credential cleanup when switching engine IDs is correct: COPILOT_PROVIDER_* env, provider, and github_app_token are all dropped when the base is copilot and the overlay switches away. Test threat_detection_engine_switch_drops_inherited_copilot_credentials covers the exact surface.
• Provider conflict detection: stripping COPILOT_PROVIDER_* from inherited env before merging the overlay (so a conflicting key inside env: still reaches validation and fails) is the right order. Covered by threat_detection_typed_provider_replaces_inherited_legacy_provider_env.
• lint_front_matter_tasks signature change to Result<Vec<LintFinding>> is properly propagated to all callers (inspect/cli.rs updated with ?; mcp_author goes through build_lint). The new test malformed_threat_detection_returns_error_instead_of_panicking validates the error path.
• Test coverage is comprehensive: unit tests for every overlay/merge/conflict scenario in types.rs, integration tests spanning all four targets, and a disabled-detection test that verifies the full graph topology (typed outputs, artifact publish, ManualReview condition wiring) is preserved when AI analysis is off.

Generated by Rust PR Reviewer for #1716 · 72.3 AIC · ⌖ 5.69 AIC · ⊞ 6.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured feature with solid test coverage. Two minor issues worth a quick fix.

Findings

🐛 Bugs / Logic Issues

• src/engine.rs (copilot_detection_env): Duplicate doc comment — the function has two /// Return ... sentences back-to-back:

  /// Return custom environment entries for a Detection Copilot run.
  ///
  /// Return the full effective custom environment for a Detection Copilot run.
  

  The first line should be removed (it's a leftover from an earlier edit).

⚠️ Suggestions

• src/compile/agentic_pipeline.rs (build_pipeline_context): The expect() on strip_prefix is internally justified (the invariant is correctly documented), but it panics instead of propagating an error. Since this is already on a Result-returning path, prefer .ok_or_else(|| anyhow::anyhow!("...")) or anyhow::bail! to keep the error model consistent and avoid a user-visible panic if the invariant is ever violated.

• src/compile/types.rs (ThreatDetectionConfig.steps / post_steps): The ##vso[-injection check applied to prompt is not applied to bash bodies inside steps/post-steps. This is consistent with how setup/teardown are handled elsewhere, and these are compile-time-authored fields, not runtime input. A brief comment clarifying the trust boundary would help future maintainers not add the wrong layer of sanitisation.

✅ What Looks Good

• The overlay resolution in DetectionEngineConfig::resolve() is careful: COPILOT_PROVIDER_* key cleanup is ordered correctly (base env stripped before overlay env is merged so overlay env still reaches validation), and the engine-switch path correctly drops provider and github_app_token.
• The IR graph invariant test (threat_detection_enabled_and_disabled_match_expected_ir_graph) asserting that job edges, step locations, and promoted outputs are identical for enabled vs disabled detection is exactly the right structural guarantee to write.
• lint_front_matter_tasks signature lift to anyhow::Result propagates errors cleanly through all call sites.
• Prompt sanitisation (sanitize_config + reject_pipeline_injection on source_path_suffix) covers the obvious injection vectors.

Generated by Rust PR Reviewer for #1716 · 89 AIC · ⌖ 5.75 AIC · ⊞ 6.3K · ◷