From 9c563d3efae1e386e059f0b228d37ca1e6b4bcf6 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 24 Jun 2026 22:51:48 +0100 Subject: [PATCH 1/9] feat(compile): gate high-impact safe outputs behind manual review Wire the ManualValidation@1 builder into the canonical pipeline so safe outputs can require human approval via a new agentless ManualReview job. - safe-outputs.require-approval (section default) + per-tool override, accepting a bool or { approvers, notify-users, timeout-minutes, on-timeout, instructions }; reserved key filtered from tool enumeration - Pool::Server agentless IR variant emitting `pool: server` - mixed config splits Stage 3 into an automatic SafeOutputs job and a gated SafeOutputs_Reviewed job (distinct safe_outputs_reviewed artifact) - executor --only/--exclude tool filter - ManualReview only pauses when Detection cleared the run and the agent actually proposed a reviewed output (HasReviewedProposals); fail-closed - audit aggregates execution records across both safe-output artifacts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 11 +- docs/cli.md | 2 + docs/safe-outputs.md | 64 ++++++ src/audit/analyzers/safe_outputs.rs | 100 ++++++--- src/compile/agentic_pipeline.rs | 333 ++++++++++++++++++++++++++-- src/compile/common.rs | 19 +- src/compile/ir/job.rs | 5 + src/compile/ir/lower.rs | 37 ++++ src/compile/ir/summary.rs | 2 + src/compile/onees_ir.rs | 9 +- src/compile/types.rs | 219 ++++++++++++++++++ src/execute.rs | 87 +++++++- src/main.rs | 27 ++- tests/compiler_tests.rs | 165 ++++++++++++++ 14 files changed, 1020 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4c272ae4..38e9126d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,6 +39,15 @@ Every compiled pipeline runs as three sequential jobs: 3. **SafeOutputs (Stage 3)** — a non-agent executor applies approved safe outputs using a write-capable ADO token that the agent never sees. +**Optional manual review.** When a safe output is configured with +`require-approval` (see [`docs/safe-outputs.md`](docs/safe-outputs.md)), an +agentless `ManualReview` job (`pool: server`, `ManualValidation@1`) is inserted +between Detection and SafeOutputs to pause for human approval. With a mix of +gated and non-gated outputs, Stage 3 splits into an automatic `SafeOutputs` job +(applies non-gated outputs immediately) and a `SafeOutputs_Reviewed` job (gated +behind `ManualReview`, publishes `safe_outputs_reviewed`). The gate is +fail-closed and only pauses when the agent actually proposed a reviewed output. + ### Architecture ``` @@ -50,7 +59,7 @@ Every compiled pipeline runs as three sequential jobs: │ ├── compile/ # Pipeline compilation module │ │ ├── mod.rs # Module entry point and Compiler trait │ │ ├── common.rs # Shared helpers across targets -│ │ ├── agentic_pipeline.rs # Canonical Setup → Agent → Detection → SafeOutputs → Teardown shape (shared by every target); BuiltPipelineContext, build_pipeline_context, build_canonical_jobs, per-job builders, fold_agent_conditions, agent_job_variables_hoist +│ │ ├── agentic_pipeline.rs # Canonical Setup → Agent → Detection → (ManualReview?) → SafeOutputs(+SafeOutputs_Reviewed?) → Teardown shape (shared by every target); BuiltPipelineContext, build_pipeline_context, build_canonical_jobs, per-job builders incl. build_manual_review_job + SafeOutputsVariant split, fold_agent_conditions, agent_job_variables_hoist │ │ ├── ir/ # Typed Azure DevOps pipeline IR │ │ │ ├── mod.rs # IR module entry point and shared types │ │ │ ├── ids.rs # Stable IDs for jobs/steps/outputs in the IR diff --git a/docs/cli.md b/docs/cli.md index 2313cb4d..6431c21b 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -35,6 +35,8 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `--ado-org-url ` - Azure DevOps organization URL override - `--ado-project ` - Azure DevOps project name override - `--dry-run` - Validate inputs but skip ADO API calls (useful for local testing and QA review) + - `--only ` - Execute only these safe-output tools (repeatable). Used by the manual-review split for the approval-gated `SafeOutputs_Reviewed` job. + - `--exclude ` - Skip these safe-output tools (repeatable). Used by the manual-review split so the automatic `SafeOutputs` job applies non-gated outputs while reviewed ones wait. See [`docs/safe-outputs.md`](safe-outputs.md#manual-review-require-approval). - `configure` *(deprecated; hidden in --help)* - Alias forwarding to `secrets set GITHUB_TOKEN`. Existing scripts keep working but get a stderr warning. diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 7edd76e0..b46a461a 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -37,6 +37,70 @@ safe-outputs: Safe output configurations are passed to Stage 3 execution and used when processing safe outputs. +### Manual review (`require-approval`) + +High-impact safe outputs can be gated behind a human approval step +(`ManualValidation@1`) that pauses the run until a reviewer approves or rejects +in the Azure DevOps UI. This lets agents propose more consequential actions +(PRs, branches, queued builds, work items) safely. + +Set `require-approval` at the **section level** for a pipeline-wide default, +and/or inside an **individual tool** to override the default for that tool: + +```yaml +safe-outputs: + require-approval: true # global default: every output below needs review + create-pull-request: + target-branch: main + add-pr-comment: + require-approval: false # …except low-impact comments, which auto-apply +``` + +`require-approval` accepts either a bare boolean or an object for finer control: + +```yaml +safe-outputs: + create-pull-request: + require-approval: + approvers: ["[MyOrg]\\release-team"] # who may approve (empty → anyone with run permission) + notify-users: ["ops@example.com"] # who is emailed (empty → no email) + timeout-minutes: 120 # pending period (omit → job/stage timeout) + on-timeout: reject # reject (default, fail-closed) | resume + instructions: "Verify the proposed PR before approving." +``` + +Resolution per tool: the tool's own `require-approval` wins; otherwise the +section-level `require-approval` applies; otherwise the tool is **not** gated. + +**Defaults (bare `require-approval: true`)** — the run pauses on a Review panel; +**anyone with run permission** can approve or reject; **no** notification emails +are sent; and the validation **fails closed** on timeout (`on-timeout: reject`), +so un-approved outputs are never applied. + +**Reviewer message** — set `instructions` to control the text shown in the +Review panel and notification emails. It is plain text and supports pipeline +variable (`$(...)`) interpolation. When omitted, ado-aw generates a default +message listing the reviewed safe-output type(s) awaiting approval. + +**Execution shape** — manual review changes the compiled pipeline: + +- A new agentless `ManualReview` job (`pool: server`) runs `ManualValidation@1` + between Detection and the safe-output execution. +- It only pauses when Detection cleared the run (no prompt-injection / secret + leak) **and** the agent actually proposed a reviewed-type output (a Detection + step sets a `HasReviewedProposals` flag) — so the run never pauses for + nothing. +- When some tools are gated and others are not, execution **splits**: an + automatic `SafeOutputs` job applies the non-gated outputs immediately + (independent of the review outcome), while a separate `SafeOutputs_Reviewed` + job — gated behind `ManualReview` — applies the approved outputs and publishes + a distinct `safe_outputs_reviewed` artifact. A rejected or timed-out review + fails closed: the reviewed job is skipped while the automatic outputs are + unaffected. + +The Detection threat gate always runs first, so a flagged run applies nothing — +automatic or reviewed. + ### Executor authentication All write-bearing safe outputs (e.g. `create-pull-request`, diff --git a/src/audit/analyzers/safe_outputs.rs b/src/audit/analyzers/safe_outputs.rs index 9dc59ccf..0d72a722 100644 --- a/src/audit/analyzers/safe_outputs.rs +++ b/src/audit/analyzers/safe_outputs.rs @@ -65,11 +65,11 @@ pub async fn analyze_safe_outputs( ) -> anyhow::Result { let proposals_path = find_proposals_file(download_root).await?; let detection_path = find_detection_file(download_root).await?; - let executions_path = find_execution_file(download_root).await?; + let executions_paths = find_execution_files(download_root).await?; let proposals = load_proposals(proposals_path.as_deref()).await?; let detection = load_detection_verdict(detection_path.as_deref()).await?; - let executions = load_execution_records(executions_path.as_deref()).await?; + let executions = load_execution_records(&executions_paths).await?; let detection_gate_fired = detection.as_ref().is_some_and(DetectionVerdict::gate_fired); let items = if detection_gate_fired { @@ -202,17 +202,15 @@ async fn load_detection_verdict(path: Option<&Path>) -> anyhow::Result, + paths: &[PathBuf], ) -> anyhow::Result> { - let Some(path) = path else { - return Ok(Vec::new()); - }; - - let values = read_ndjson_file(path).await?; - values - .into_iter() - .enumerate() - .map(|(index, value)| { + let mut records = Vec::new(); + for path in paths { + let values = read_ndjson_file(path).await?; + for value in values { + // `index` is assigned across the merged set so records from + // multiple execution artifacts never collide during matching. + let index = records.len(); let mut record = serde_json::from_value::(value).with_context(|| { format!( @@ -225,9 +223,10 @@ async fn load_execution_records( record.status = record.status.trim().to_string(); record.context = normalize_optional_string(record.context); record.error = normalize_optional_string(record.error); - Ok(IndexedExecutionRecord { index, record }) - }) - .collect() + records.push(IndexedExecutionRecord { index, record }); + } + } + Ok(records) } fn build_execution_items( @@ -564,22 +563,16 @@ async fn find_detection_file(download_root: &Path) -> anyhow::Result anyhow::Result> { - let preferred = download_root - .join("safe_outputs") - .join(EXECUTED_NDJSON_FILENAME); - if fs::metadata(&preferred) - .await - .map(|m| m.is_file()) - .unwrap_or(false) - { - return Ok(Some(preferred)); - } - +async fn find_execution_files(download_root: &Path) -> anyhow::Result> { + // With manual review, execution splits across multiple artifacts + // (`safe_outputs/` for the automatic path and `safe_outputs_reviewed/` + // for the approval-gated path), each with its own `executed.ndjson`. + // Collect them all so the audit reflects the complete set of actions. let mut matches = Vec::new(); collect_named_files(download_root, EXECUTED_NDJSON_FILENAME, &mut matches).await?; matches.sort(); - Ok(matches.into_iter().next()) + matches.dedup(); + Ok(matches) } async fn top_level_dirs_with_prefix(root: &Path, prefix: &str) -> anyhow::Result> { @@ -738,6 +731,57 @@ mod tests { assert!(analysis.findings.is_empty()); } + #[tokio::test] + async fn execution_records_aggregate_across_split_artifacts() { + // Manual-review split: automatic outputs land in `safe_outputs/`, + // reviewed (approval-gated) outputs in `safe_outputs_reviewed/`. The + // audit must reflect both. + let temp_dir = TempDir::new().expect("create temp dir"); + write_ndjson( + &temp_dir + .path() + .join("agent_outputs_99") + .join("staging") + .join(SAFE_OUTPUT_FILENAME), + &[ + json!({"name": "add_pr_comment", "context": "c-1"}), + json!({"name": "create_pull_request", "context": "pr-1"}), + ], + ); + write_ndjson( + &temp_dir + .path() + .join("safe_outputs") + .join(EXECUTED_NDJSON_FILENAME), + &[json!({"name": "add_pr_comment", "status": "succeeded", "context": "c-1", "result": {"status": "ok"}})], + ); + write_ndjson( + &temp_dir + .path() + .join("safe_outputs_reviewed") + .join(EXECUTED_NDJSON_FILENAME), + &[json!({"name": "create_pull_request", "status": "succeeded", "context": "pr-1", "result": {"number": 9}})], + ); + + let analysis = analyze_safe_outputs(temp_dir.path()) + .await + .expect("analyze split safe outputs"); + + let summary = analysis.summary.expect("summary"); + assert_eq!(summary.proposed_count, 2); + // Both the automatic and the reviewed execution are counted. + assert_eq!(summary.executed_count, 2); + + let execution = analysis.execution.expect("execution"); + assert_eq!(execution.items.len(), 2); + assert!( + execution + .items + .iter() + .all(|item| item.status == SafeOutputStatus::Executed) + ); + } + #[tokio::test] async fn aggregate_detection_gate_rejects_all_proposals() { let temp_dir = TempDir::new().expect("create temp dir"); diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 05ad73fd..506cd8c4 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1,7 +1,8 @@ //! Typed-IR builder for the canonical agentic-pipeline shape. //! -//! Owns the Setup → Agent → Detection → SafeOutputs → Teardown -//! shape consumed by **every** compile target (`standalone`, `1es`, +//! Owns the Setup → Agent → Detection → (ManualReview?) → SafeOutputs +//! (+ SafeOutputs_Reviewed?) → Teardown shape consumed by **every** +//! compile target (`standalone`, `1es`, //! `job`, `stage`). Each target's wrapper module (`standalone_ir.rs`, //! `onees_ir.rs`, `job_ir.rs`, `stage_ir.rs`) is a one-screen //! envelope that calls [`build_pipeline_context`] and lifts the @@ -40,13 +41,22 @@ //! Emitted when filters / synthPr / user setup are present. //! - `Agent`: extensions + the static AWF / MCPG / agent-run scaffold. //! - `Detection`: threat-analysis pass that produces the -//! `threatAnalysis.SafeToProcess` output. +//! `threatAnalysis.SafeToProcess` output. When manual review is +//! configured it also produces `reviewedProposals.HasReviewedProposals`. +//! - `ManualReview` (optional): an agentless (`pool: server`) +//! `ManualValidation@1` gate inserted when a safe output is configured +//! with `require-approval`. Pauses for human approval only when the run +//! is safe **and** the agent proposed a reviewed-type output. Fail-closed +//! on rejection/timeout. //! - `SafeOutputs`: gated on Detection's `SafeToProcess` output via //! typed [`Condition::Eq`] over a typed //! [`crate::compile::ir::output::OutputRef`]. The lowering pass //! picks `dependencies.Detection.outputs['threatAnalysis.SafeToProcess']` //! — first production use of typed cross-job OutputRef in a -//! condition. +//! condition. With mixed `require-approval`, execution splits into this +//! automatic job (excludes reviewed tools) plus a `SafeOutputs_Reviewed` +//! job gated behind `ManualReview` (runs only the reviewed tools, +//! publishes a distinct `safe_outputs_reviewed` artifact). //! - `Teardown` (optional): user `teardown:` steps. use anyhow::Result; @@ -66,12 +76,16 @@ use super::ir::step::{ }; use super::ir::tasks::docker_installer::DockerInstaller; use super::ir::tasks::download_package::DownloadPackage; +use super::ir::tasks::manual_validation::{ManualValidation, OnTimeout}; use super::ir::tasks::nuget_authenticate::NuGetAuthenticate; use super::ir::{ CiTrigger, Parameter, ParameterDefault, ParameterKind, PipelineResource, PipelineVar, PrTrigger, RepositoryResource, Resources, Schedule, Triggers, }; -use super::types::{FrontMatter, OnConfig, PrMode, Repository as RepoCfg, SupplyChainConfig}; +use super::types::{ + ApprovalConfig, ApprovalOnTimeout, FrontMatter, OnConfig, PrMode, Repository as RepoCfg, + SupplyChainConfig, +}; /// Built pipeline context — the result of running every validation, /// scalar computation, extension declaration fanout, and canonical- @@ -354,7 +368,37 @@ pub(crate) fn build_canonical_jobs( &p, )?); jobs.push(build_detection_job(front_matter, cfg, &p)?); - jobs.push(build_safeoutputs_job(front_matter, cfg, &p)?); + if let Some(review) = build_manual_review_job(front_matter, cfg, &p)? { + jobs.push(review); + } + // Safe-outputs execution. With manual review, execution may split into an + // automatic job (runs immediately) and a reviewed job (gated behind the + // ManualReview approval). Partition decides the shape: + // - no reviewed tools → single default job (unchanged) + // - all reviewed tools → single default job, gated by ManualReview + // - mixed (auto + reviewed) → auto job + reviewed job + let (auto, reviewed) = front_matter.partition_safe_outputs_by_approval(); + if reviewed.is_empty() || auto.is_empty() { + jobs.push(build_safeoutputs_job( + front_matter, + cfg, + &p, + &SafeOutputsVariant::default_single(), + )?); + } else { + jobs.push(build_safeoutputs_job( + front_matter, + cfg, + &p, + &SafeOutputsVariant::automatic(&reviewed), + )?); + jobs.push(build_safeoutputs_job( + front_matter, + cfg, + &p, + &SafeOutputsVariant::reviewed(&reviewed), + )?); + } if let Some(teardown) = build_teardown_job(front_matter, cfg, &p)? { jobs.push(teardown); } @@ -377,9 +421,10 @@ impl<'a> JobPrefix<'a> { /// prefix is provided. pub(crate) fn id(&self, base: &str) -> Result { match (self.0, base) { - (Some(prefix), "Agent" | "Detection" | "SafeOutputs") => { - JobId::new(format!("{prefix}_{base}")) - } + ( + Some(prefix), + "Agent" | "Detection" | "ManualReview" | "SafeOutputs" | "SafeOutputs_Reviewed", + ) => JobId::new(format!("{prefix}_{base}")), _ => JobId::new(base), } } @@ -998,6 +1043,17 @@ fn build_detection_job( steps.push(Step::Bash(prepare_analyzed_outputs_step())); // Evaluate threat analysis — DECLARES TYPED OUTPUT steps.push(Step::Bash(evaluate_threat_analysis_step())); + // When manual review is configured, detect whether the agent actually + // proposed any approval-gated outputs — DECLARES TYPED OUTPUT. The + // ManualReview gate is conditioned on this so the run never pauses for a + // human when there is nothing to review. + let (_, reviewed_tools) = front_matter.partition_safe_outputs_by_approval(); + if !reviewed_tools.is_empty() { + steps.push(Step::Bash(detect_reviewed_proposals_step( + &cfg.working_directory, + &reviewed_tools, + ))); + } // Copy logs steps.push(Step::Bash(copy_logs_step(&cfg.engine_log_dir, true))); // Publish @@ -1012,10 +1068,69 @@ fn build_detection_job( Ok(job) } +/// Describes one safe-outputs execution job. The canonical graph emits a +/// single default variant in the common case, or — when manual review splits +/// execution — an automatic variant (`--exclude` the reviewed tools) plus a +/// reviewed variant (`--only` the reviewed tools) gated behind ManualReview. +struct SafeOutputsVariant { + /// Canonical job base name passed to `JobPrefix::id`. + base: &'static str, + /// Job `displayName`. + display: &'static str, + /// Published pipeline-artifact name (must be unique per run). + artifact: &'static str, + /// Trailing `--only`/`--exclude` flags for `ado-aw execute` (or empty). + filter_args: String, +} + +impl SafeOutputsVariant { + /// The default single-job variant: no filter, canonical names. + fn default_single() -> Self { + Self { + base: "SafeOutputs", + display: "SafeOutputs", + artifact: "safe_outputs", + filter_args: String::new(), + } + } + + /// The automatic variant in a split: excludes every reviewed tool. + fn automatic(reviewed: &[String]) -> Self { + Self { + base: "SafeOutputs", + display: "SafeOutputs", + artifact: "safe_outputs", + filter_args: filter_flags("--exclude", reviewed), + } + } + + /// The reviewed variant in a split: runs only the reviewed tools. + fn reviewed(reviewed: &[String]) -> Self { + Self { + base: "SafeOutputs_Reviewed", + display: "SafeOutputs (reviewed)", + artifact: "safe_outputs_reviewed", + filter_args: filter_flags("--only", reviewed), + } + } +} + +/// Build a ` -- ` run for `ado-aw execute` (leading space so it +/// concatenates onto the fixed command). Tool names are compiler-controlled +/// safe-output identifiers (ASCII alphanumeric/hyphen), so no quoting needed. +fn filter_flags(flag: &str, tools: &[String]) -> String { + let mut s = String::new(); + for t in tools { + s.push_str(&format!(" {flag} {t}")); + } + s +} + fn build_safeoutputs_job( front_matter: &FrontMatter, cfg: &StandaloneCtx, prefix: &JobPrefix<'_>, + variant: &SafeOutputsVariant, ) -> Result { let mut steps: Vec = Vec::new(); steps.push(checkout_self_step()); @@ -1053,17 +1168,18 @@ fn build_safeoutputs_job( &cfg.source_path, &cfg.working_directory, &cfg.executor_ado_env, + &variant.filter_args, )?)); // Copy logs steps.push(Step::Bash(copy_logs_safeoutputs_step(&cfg.engine_log_dir))); // Publish steps.push(Step::Publish(PublishStep { path: "$(Agent.TempDirectory)/staging".to_string(), - artifact: "safe_outputs".to_string(), + artifact: variant.artifact.to_string(), condition: Some(Condition::Always), })); - let mut job = Job::new(prefix.id("SafeOutputs")?, "SafeOutputs", cfg.pool.clone()); + let mut job = Job::new(prefix.id(variant.base)?, variant.display, cfg.pool.clone()); job.steps = steps; // **Marquee**: condition uses typed Expr::StepOutput on Detection's // threatAnalysis.SafeToProcess output. Lowering picks the cross-job @@ -1082,6 +1198,129 @@ fn build_safeoutputs_job( Ok(job) } +/// Build the agentless **ManualReview** job (a `ManualValidation@1` server +/// task) when any enabled safe-output tool resolves to require manual review. +/// +/// Returns `Ok(None)` when no tool requires approval (the common case — the +/// canonical graph is then unchanged). The gate sits between Detection and +/// SafeOutputs; its condition reuses Detection's `threatAnalysis.SafeToProcess` +/// output so a run flagged unsafe never pauses for a human, and a rejected +/// validation fails the gate so SafeOutputs (which depends on it) is skipped — +/// fail-closed by default. +fn build_manual_review_job( + front_matter: &FrontMatter, + cfg: &StandaloneCtx, + prefix: &JobPrefix<'_>, +) -> Result> { + let (_, reviewed) = front_matter.partition_safe_outputs_by_approval(); + if reviewed.is_empty() { + return Ok(None); + } + let approval = aggregate_approval_config(front_matter, &reviewed); + + let mut job = Job::new(prefix.id("ManualReview")?, "Manual Review", Pool::Server); + job.steps = vec![Step::Task(build_manual_validation_step(&approval, &reviewed))]; + // The validation's pending period is bounded by the agentless job timeout. + if let Some(mins) = approval.timeout_minutes { + job.timeout = Some(std::time::Duration::from_secs(60 * (mins as u64))); + } + let _ = cfg; // pool/compiler context not needed for an agentless gate + job.condition = Some(Condition::And(vec![ + Condition::Succeeded, + Condition::Eq( + Expr::StepOutput(OutputRef::new( + StepId::new("threatAnalysis")?, + "SafeToProcess", + )), + Expr::Literal("true".to_string()), + ), + // Only pause for a human when the agent actually proposed an + // approval-gated output (set by Detection's reviewedProposals step). + Condition::Eq( + Expr::StepOutput(OutputRef::new( + StepId::new("reviewedProposals")?, + "HasReviewedProposals", + )), + Expr::Literal("true".to_string()), + ), + ])); + Ok(Some(job)) +} + +/// Fold the per-tool/global approval settings of every reviewed tool into the +/// single settings object that drives the whole-pipeline `ManualValidation@1` +/// gate. Lists are unioned; the timeout is the strictest (smallest) provided; +/// `on-timeout` is fail-closed (`reject`) unless *every* contributing config +/// explicitly asks to `resume`. +fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> ApprovalConfig { + use std::collections::BTreeSet; + let mut approvers: BTreeSet = BTreeSet::new(); + let mut notify: BTreeSet = BTreeSet::new(); + let mut timeout_minutes: Option = None; + let mut all_resume = true; + let mut instructions: Option = None; + + for tool in reviewed { + let Some(cfg) = front_matter.tool_requires_approval(tool) else { + continue; + }; + approvers.extend(cfg.approvers); + notify.extend(cfg.notify_users); + if let Some(t) = cfg.timeout_minutes { + timeout_minutes = Some(timeout_minutes.map_or(t, |existing| existing.min(t))); + } + match cfg.on_timeout { + Some(ApprovalOnTimeout::Resume) => {} + _ => all_resume = false, + } + if instructions.is_none() { + instructions = cfg.instructions; + } + } + + ApprovalConfig { + approvers: approvers.into_iter().collect(), + notify_users: notify.into_iter().collect(), + timeout_minutes, + on_timeout: Some(if all_resume { + ApprovalOnTimeout::Resume + } else { + ApprovalOnTimeout::Reject + }), + instructions, + } +} + +/// Build the `ManualValidation@1` step from the aggregated approval settings. +fn build_manual_validation_step(approval: &ApprovalConfig, reviewed: &[String]) -> TaskStep { + let mut builder = ManualValidation::new(approval.notify_users.join(", ")); + if !approval.approvers.is_empty() { + builder = builder.approvers(approval.approvers.join(", ")); + } + let instructions = approval + .instructions + .clone() + .unwrap_or_else(|| default_review_instructions(reviewed)); + builder = builder.instructions(instructions); + let on_timeout = match approval.on_timeout { + Some(ApprovalOnTimeout::Resume) => OnTimeout::Resume, + _ => OnTimeout::Reject, + }; + builder = builder.on_timeout(on_timeout); + builder.into_step() +} + +/// Default reviewer message when the author did not set `instructions`. +fn default_review_instructions(reviewed: &[String]) -> String { + format!( + "This run is paused for manual review. The agent has proposed safe \ + outputs of the following type(s) that require approval before they \ + are applied: {}. Approve (Resume) to apply them, or Reject to discard \ + them.", + reviewed.join(", ") + ) +} + fn build_teardown_job( front_matter: &FrontMatter, cfg: &StandaloneCtx, @@ -1118,18 +1357,52 @@ fn wire_explicit_dependencies(jobs: &mut [Job], prefix: &JobPrefix<'_>) -> Resul let setup_id = prefix.id("Setup")?; let agent_id = prefix.id("Agent")?; let detection_id = prefix.id("Detection")?; + let manualreview_id = prefix.id("ManualReview")?; let safeoutputs_id = prefix.id("SafeOutputs")?; + let reviewed_id = prefix.id("SafeOutputs_Reviewed")?; let teardown_id = prefix.id("Teardown")?; let has_setup = jobs.iter().any(|j| j.id == setup_id); + let has_review = jobs.iter().any(|j| j.id == manualreview_id); + // The reviewed execution job only exists in the mixed (split) case. + let has_reviewed_job = jobs.iter().any(|j| j.id == reviewed_id); for j in jobs.iter_mut() { if j.id == agent_id && has_setup { j.depends_on = vec![setup_id.clone()]; } else if j.id == detection_id { j.depends_on = vec![agent_id.clone()]; - } else if j.id == safeoutputs_id { + } else if j.id == manualreview_id { + // Agentless gate: depends on Detection (its condition reads + // Detection's threatAnalysis.SafeToProcess output). j.depends_on = vec![agent_id.clone(), detection_id.clone()]; + } else if j.id == safeoutputs_id { + // The "SafeOutputs" job is the automatic path. It is gated behind + // ManualReview only when it is the *sole* execution job (all tools + // reviewed); in the mixed split it runs immediately after Detection + // alongside the separate reviewed job. + j.depends_on = if has_review && !has_reviewed_job { + vec![ + agent_id.clone(), + detection_id.clone(), + manualreview_id.clone(), + ] + } else { + vec![agent_id.clone(), detection_id.clone()] + }; + } else if j.id == reviewed_id { + // Reviewed execution runs only after the approval gate clears, so a + // rejected review fails closed (this job is skipped). + j.depends_on = vec![ + agent_id.clone(), + detection_id.clone(), + manualreview_id.clone(), + ]; } else if j.id == teardown_id { - j.depends_on = vec![safeoutputs_id.clone()]; + // Teardown waits on every execution job that exists. + let mut deps = vec![safeoutputs_id.clone()]; + if has_reviewed_job { + deps.push(reviewed_id.clone()); + } + j.depends_on = deps; } } Ok(()) @@ -1808,9 +2081,12 @@ fn execute_safe_outputs_step( source_path: &str, working_directory: &str, executor_ado_env: &str, + filter_args: &str, ) -> Result { + // `filter_args` is either empty or a leading-space-prefixed run of + // `--only ` / `--exclude ` flags appended to the command. let script = format!( - "ado-aw execute --source \"{source_path}\" --safe-output-dir \"$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)\" --output-dir \"$(Agent.TempDirectory)/staging\"\n\ + "ado-aw execute --source \"{source_path}\" --safe-output-dir \"$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)\" --output-dir \"$(Agent.TempDirectory)/staging\"{filter_args}\n\ EXIT_CODE=$?\n\ if [ $EXIT_CODE -eq 2 ]; then\n \ echo \"##vso[task.complete result=SucceededWithIssues;]Executor completed with warnings\"\n \ @@ -2057,6 +2333,35 @@ fn evaluate_threat_analysis_step() -> BashStep { .with_condition(Condition::Always) } +/// Scan the agent's proposed safe-output NDJSON for any approval-gated tool +/// and publish a `HasReviewedProposals` output variable. The ManualReview gate +/// is conditioned on this so a run never pauses for a human when the agent did +/// not propose anything that requires review. +fn detect_reviewed_proposals_step(working_directory: &str, reviewed: &[String]) -> BashStep { + // `reviewed` are compiler-controlled safe-output names (ASCII + // alphanumeric/hyphen only — see `validate::is_safe_tool_name`), so they + // are safe to embed directly in a grep alternation. + let alternation = reviewed.join("|"); + let script = format!( + "HAS_REVIEWED=\"false\"\n\ + PROPOSALS=$(find \"{working_directory}/safe_outputs\" -name \"safe_outputs.ndjson\" 2>/dev/null | head -n 1)\n\ + if [ -n \"$PROPOSALS\" ] && [ -f \"$PROPOSALS\" ]; then\n \ + if grep -Eq '\"name\"[[:space:]]*:[[:space:]]*\"({alternation})\"' \"$PROPOSALS\"; then\n \ + HAS_REVIEWED=\"true\"\n \ + fi\n\ + fi\n\ + echo \"##vso[task.setvariable variable=HasReviewedProposals;isOutput=true]$HAS_REVIEWED\"\n\ + echo \"HasReviewedProposals set to: $HAS_REVIEWED\"\n" + ); + bash("Detect reviewed proposals", script) + .with_id( + StepId::new("reviewedProposals") + .expect("reviewedProposals is a valid StepId — see StepId::new contract"), + ) + .with_output(OutputDecl::new("HasReviewedProposals")) + .with_condition(Condition::Always) +} + fn verify_mcp_backends_step() -> BashStep { // Debug-only probe (emitted when --debug-pipeline is on). Probes every // MCPG backend via MCP initialize + tools/list to surface broken diff --git a/src/compile/common.rs b/src/compile/common.rs index 659f529f..dca7dd96 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1491,7 +1491,7 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { let mut seen = HashSet::new(); let mut tools: Vec = Vec::new(); let mut effective_mcp_tool_count = 0usize; - for key in front_matter.safe_outputs.keys() { + for key in front_matter.safe_output_tool_names() { if !validate::is_safe_tool_name(key) { eprintln!( "Warning: skipping invalid safe-output tool name '{}' (must be ASCII alphanumeric/hyphens only)", @@ -1745,7 +1745,7 @@ pub fn validate_safe_outputs_keys(front_matter: &FrontMatter) -> Result<()> { let mut unknown: Vec<(String, Vec<&'static str>)> = Vec::new(); let mut invalid_names: Vec = Vec::new(); - for key in front_matter.safe_outputs.keys() { + for key in front_matter.safe_output_tool_names() { if !validate::is_safe_tool_name(key) { invalid_names.push(key.clone()); continue; @@ -3913,6 +3913,21 @@ ado-aw-debug: assert!(args.contains("--enabled-tools report-incomplete")); } + #[test] + fn test_generate_enabled_tools_args_skips_require_approval_reserved_key() { + // The reserved section-level `require-approval` key must never be + // treated as a tool name in `--enabled-tools`. + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n require-approval: true\n create-pull-request:\n target-branch: main\n---\n" + ).unwrap(); + let args = generate_enabled_tools_args(&fm); + assert!( + !args.contains("require-approval"), + "reserved require-approval key must not be emitted as a tool: {args}" + ); + assert!(args.contains("--enabled-tools create-pull-request")); + } + #[test] fn test_generate_enabled_tools_args_no_duplicates() { // If a diagnostic tool is also in safe-outputs, it shouldn't appear twice diff --git a/src/compile/ir/job.rs b/src/compile/ir/job.rs index 56a7e74a..e6d260ab 100644 --- a/src/compile/ir/job.rs +++ b/src/compile/ir/job.rs @@ -180,6 +180,11 @@ pub enum Pool { /// Optional `os:` field (1ES pool OS). os: Option, }, + /// `server` — an agentless (server) job. Emits the scalar + /// `pool: server`. Required for server-only tasks such as + /// `ManualValidation@1`; such jobs must contain no agent steps + /// (no checkout, downloads, or shell/`ado-aw` invocations). + Server, } impl Job { diff --git a/src/compile/ir/lower.rs b/src/compile/ir/lower.rs index 159e1196..fce98594 100644 --- a/src/compile/ir/lower.rs +++ b/src/compile/ir/lower.rs @@ -804,6 +804,10 @@ fn merge_condition_with_template_param(internal: &str, param_name: &str) -> Stri } fn lower_pool(pool: &Pool) -> Value { + if let Pool::Server = pool { + // Agentless/server job: ADO expects the scalar `pool: server`. + return s("server"); + } let mut m = Mapping::new(); match pool { Pool::VmImage(img) => { @@ -818,6 +822,7 @@ fn lower_pool(pool: &Pool) -> Value { m.insert(s("os"), s(os)); } } + Pool::Server => unreachable!("handled above"), } Value::Mapping(m) } @@ -1474,6 +1479,38 @@ mod tests { ); } + #[test] + fn lower_job_emits_agentless_server_pool_scalar() { + use crate::compile::ir::step::TaskStep; + + let mut job = Job::new( + JobId::new("ManualReview").unwrap(), + "Manual Review", + Pool::Server, + ); + job.push_step(Step::Task(TaskStep::new("ManualValidation@1", "Approve"))); + let p = Pipeline { + name: "t".into(), + parameters: Vec::new(), + resources: Resources::default(), + triggers: Triggers::default(), + variables: Vec::new(), + body: PipelineBody::Jobs(vec![job]), + shape: PipelineShape::Standalone, + }; + let v = super::lower(&p).unwrap(); + let yaml = serde_yaml::to_string(&v).unwrap(); + // Agentless job uses the scalar `pool: server`, never a mapping. + assert!( + yaml.contains("pool: server"), + "expected scalar `pool: server` in:\n{yaml}" + ); + assert!( + !yaml.contains("vmImage"), + "server pool must not emit a vmImage mapping:\n{yaml}" + ); + } + #[test] fn lower_job_hoists_runtime_expression_to_job_variable() { let body = "dependencies.Agent.result"; diff --git a/src/compile/ir/summary.rs b/src/compile/ir/summary.rs index d8d6e6b7..f52952d5 100644 --- a/src/compile/ir/summary.rs +++ b/src/compile/ir/summary.rs @@ -149,6 +149,7 @@ pub enum PoolSummary { image: Option, os: Option, }, + Server, } /// A single step's public summary. @@ -344,6 +345,7 @@ fn summarize_pool(p: &super::job::Pool) -> PoolSummary { image: image.clone(), os: os.clone(), }, + super::job::Pool::Server => PoolSummary::Server, } } diff --git a/src/compile/onees_ir.rs b/src/compile/onees_ir.rs index 212d153e..ddf2eeef 100644 --- a/src/compile/onees_ir.rs +++ b/src/compile/onees_ir.rs @@ -39,7 +39,7 @@ use super::agentic_pipeline::build_pipeline_context; use super::common; use super::extensions::{CompileContext, Extension}; use super::ir::ids::StageId; -use super::ir::job::JobTemplateContext; +use super::ir::job::{JobTemplateContext, Pool}; use super::ir::{OneEsSdlConfig, Pipeline, PipelineBody, PipelineShape, RepositoryResource}; use super::types::FrontMatter; @@ -86,6 +86,13 @@ pub fn build_onees_pipeline( let mut jobs = built.jobs; for job in jobs.iter_mut() { + // Agentless (server) jobs — e.g. the ManualReview `ManualValidation@1` + // gate — are not build jobs and must not be wrapped in a 1ES + // `templateContext:` (which would suppress `pool: server` and nest the + // server task under a build-job step list). + if job.pool == Pool::Server { + continue; + } job.template_context = Some(JobTemplateContext::default()); } diff --git a/src/compile/types.rs b/src/compile/types.rs index 9a832b92..5c59fa3e 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -734,7 +734,131 @@ pub struct FrontMatter { pub supply_chain: Option, } +/// Reserved keys inside the `safe-outputs:` map that configure the section +/// itself rather than naming a safe-output tool. These must never be treated +/// as tool names (e.g. in `--enabled-tools`, Stage-3 budgets, or unknown-key +/// validation). +pub const SAFE_OUTPUT_RESERVED_KEYS: &[&str] = &["require-approval"]; + +/// Automatic action a manual-validation gate takes when its pending period +/// elapses with no human response. Mirrors `ManualValidation@1`'s `onTimeout`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum ApprovalOnTimeout { + /// Reject the run on timeout (fail-closed — the default). + Reject, + /// Resume (approve) the run on timeout. + Resume, +} + +/// Detailed manual-review settings for a safe-output approval gate. Lowered +/// into a `ManualValidation@1` agentless job. See `docs/safe-outputs.md`. +#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize)] +#[serde(deny_unknown_fields, rename_all = "kebab-case")] +pub struct ApprovalConfig { + /// Users/groups permitted to act on the validation. Empty → anyone with + /// run permission can approve or reject. + #[serde(default)] + pub approvers: Vec, + /// Users/groups to email when the validation is pending. Empty → no email. + #[serde(default)] + pub notify_users: Vec, + /// Pending-period timeout in minutes. None → ADO job/stage timeout applies. + #[serde(default)] + pub timeout_minutes: Option, + /// Automatic action on timeout. None → fail-closed (`reject`). + #[serde(default)] + pub on_timeout: Option, + /// Free-text message shown to the reviewer (run "Review" panel + email). + /// None → an auto-generated summary of the proposed outputs is used. + #[serde(default)] + pub instructions: Option, +} + +/// The `require-approval` value, accepted either as a bare boolean toggle or a +/// detailed [`ApprovalConfig`] object. Usable at the `safe-outputs:` section +/// level (global default) or inside an individual tool's config (override). +#[derive(Debug, Clone, PartialEq, Eq, Deserialize)] +#[serde(untagged)] +pub enum RequireApproval { + /// `require-approval: true|false`. + Bool(bool), + /// `require-approval: { approvers: …, on-timeout: …, … }`. + Detailed(ApprovalConfig), +} + +impl RequireApproval { + /// Whether manual review is required by this setting. + pub fn is_required(&self) -> bool { + match self { + RequireApproval::Bool(b) => *b, + RequireApproval::Detailed(_) => true, + } + } + + /// The reviewer settings (defaults for the bare-boolean form). + pub fn config(&self) -> ApprovalConfig { + match self { + RequireApproval::Bool(_) => ApprovalConfig::default(), + RequireApproval::Detailed(c) => c.clone(), + } + } +} + impl FrontMatter { + /// Iterator over enabled safe-output **tool** names, skipping reserved + /// section-level config keys (e.g. `require-approval`). Every consumer that + /// treats `safe-outputs:` keys as tool names MUST go through this so a + /// reserved key is never mistaken for a tool. + pub fn safe_output_tool_names(&self) -> impl Iterator { + self.safe_outputs + .keys() + .filter(|k| !SAFE_OUTPUT_RESERVED_KEYS.contains(&k.as_str())) + } + + /// Section-level (global) `require-approval` default, if configured. + pub fn global_require_approval(&self) -> Option { + self.safe_outputs + .get("require-approval") + .and_then(|v| serde_json::from_value::(v.clone()).ok()) + } + + /// Per-tool `require-approval` override for `tool`, if present. + fn tool_require_approval(&self, tool: &str) -> Option { + self.safe_outputs + .get(tool) + .and_then(|v| v.get("require-approval")) + .and_then(|v| serde_json::from_value::(v.clone()).ok()) + } + + /// Effective approval setting for `tool`: the per-tool override if present, + /// otherwise the section-level default. Returns `Some(config)` only when + /// the tool's outputs require manual review. + pub fn tool_requires_approval(&self, tool: &str) -> Option { + let setting = self + .tool_require_approval(tool) + .or_else(|| self.global_require_approval())?; + setting.is_required().then(|| setting.config()) + } + + /// Partition enabled safe-output tool names into `(auto, reviewed)` where + /// `reviewed` tools require manual approval and `auto` tools do not. Both + /// lists are sorted for deterministic emission. + pub fn partition_safe_outputs_by_approval(&self) -> (Vec, Vec) { + let mut auto = Vec::new(); + let mut reviewed = Vec::new(); + for tool in self.safe_output_tool_names() { + if self.tool_requires_approval(tool).is_some() { + reviewed.push(tool.clone()); + } else { + auto.push(tool.clone()); + } + } + auto.sort(); + reviewed.sort(); + (auto, reviewed) + } + /// Get the schedule configuration (if any). pub fn schedule(&self) -> Option<&ScheduleConfig> { self.on_config.as_ref().and_then(|o| o.schedule.as_ref()) @@ -2677,6 +2801,101 @@ Body assert!(fm.safe_outputs.contains_key("upload-pipeline-artifact")); } + #[test] + fn test_require_approval_global_bool() { + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + require-approval: true + create-pull-request: {} + add-pr-comment: {} +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + // Reserved key is not surfaced as a tool name. + let tools: Vec<&String> = fm.safe_output_tool_names().collect(); + assert!(!tools.iter().any(|t| t.as_str() == "require-approval")); + assert_eq!(tools.len(), 2); + // Global default makes every tool require approval. + assert!(fm.tool_requires_approval("create-pull-request").is_some()); + assert!(fm.tool_requires_approval("add-pr-comment").is_some()); + let (auto, reviewed) = fm.partition_safe_outputs_by_approval(); + assert!(auto.is_empty()); + assert_eq!(reviewed, vec!["add-pr-comment", "create-pull-request"]); + } + + #[test] + fn test_require_approval_per_tool_override() { + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + require-approval: true + create-pull-request: + require-approval: false + add-pr-comment: {} +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + // Per-tool false overrides the global true. + assert!(fm.tool_requires_approval("create-pull-request").is_none()); + assert!(fm.tool_requires_approval("add-pr-comment").is_some()); + let (auto, reviewed) = fm.partition_safe_outputs_by_approval(); + assert_eq!(auto, vec!["create-pull-request"]); + assert_eq!(reviewed, vec!["add-pr-comment"]); + } + + #[test] + fn test_require_approval_detailed_object() { + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + create-pull-request: + require-approval: + approvers: ["[Org]\\release"] + notify-users: ["ops@example.com"] + timeout-minutes: 120 + on-timeout: resume + instructions: "Review the proposed PR." +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + let cfg = fm + .tool_requires_approval("create-pull-request") + .expect("approval required"); + assert_eq!(cfg.approvers, vec!["[Org]\\release"]); + assert_eq!(cfg.notify_users, vec!["ops@example.com"]); + assert_eq!(cfg.timeout_minutes, Some(120)); + assert_eq!(cfg.on_timeout, Some(ApprovalOnTimeout::Resume)); + assert_eq!(cfg.instructions.as_deref(), Some("Review the proposed PR.")); + } + + #[test] + fn test_require_approval_absent_means_no_review() { + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + create-pull-request: {} +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + assert!(fm.tool_requires_approval("create-pull-request").is_none()); + let (auto, reviewed) = fm.partition_safe_outputs_by_approval(); + assert_eq!(auto, vec!["create-pull-request"]); + assert!(reviewed.is_empty()); + } + #[test] fn test_front_matter_parses_ado_aw_debug() { let content = r#"--- diff --git a/src/execute.rs b/src/execute.rs index ee09e006..23921e5e 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -28,10 +28,38 @@ use crate::sanitize::neutralize_pipeline_commands; // Re-export memory types for use by main.rs pub use crate::tools::cache_memory::{MemoryConfig, process_agent_memory}; +/// Selects which safe-output entries Stage 3 executes, by tool name. +/// +/// Used to split execution into an automatic path and a manual-review path: +/// the auto execution job `exclude`s the reviewed tools, and the reviewed +/// execution job runs `only` the reviewed tools (after the approval gate). +/// An empty filter (the default) executes every entry. +#[derive(Debug, Default, Clone)] +pub struct ToolFilter { + /// When non-empty, only entries whose tool name appears here run. + pub only: Vec, + /// Entries whose tool name appears here are skipped. + pub exclude: Vec, +} + +impl ToolFilter { + /// Whether an entry with tool name `tool` is permitted by this filter. + pub fn allows(&self, tool: &str) -> bool { + if !self.only.is_empty() && !self.only.iter().any(|t| t == tool) { + return false; + } + if self.exclude.iter().any(|t| t == tool) { + return false; + } + true + } +} + /// Execute all safe outputs from the NDJSON file in the specified directory pub async fn execute_safe_outputs( safe_output_dir: &Path, ctx: &ExecutionContext, + filter: &ToolFilter, ) -> Result> { let safe_output_path = safe_output_dir.join(SAFE_OUTPUT_FILENAME); @@ -113,6 +141,17 @@ pub async fn execute_safe_outputs( .get("name") .and_then(|name| name.as_str()) .unwrap_or("unknown"); + // Skip entries the active filter excludes (manual-review split: the + // auto job excludes reviewed tools; the reviewed job runs only them). + if !filter.allows(proposal_tool_name) { + debug!( + "[{}/{}] Skipping entry for tool '{}' (filtered out)", + i + 1, + entries.len(), + proposal_tool_name + ); + continue; + } debug!( "[{}/{}] Executing entry: {}", i + 1, @@ -671,6 +710,30 @@ mod tests { assert_eq!(extract_entry_context(&entry), " (work item #42)"); } + #[test] + fn test_tool_filter_allows() { + // Empty filter allows everything. + let f = ToolFilter::default(); + assert!(f.allows("create-pull-request")); + assert!(f.allows("add-pr-comment")); + + // `only` restricts to the listed tools. + let f = ToolFilter { + only: vec!["create-pull-request".into()], + exclude: vec![], + }; + assert!(f.allows("create-pull-request")); + assert!(!f.allows("add-pr-comment")); + + // `exclude` removes the listed tools. + let f = ToolFilter { + only: vec![], + exclude: vec!["create-pull-request".into()], + }; + assert!(!f.allows("create-pull-request")); + assert!(f.allows("add-pr-comment")); + } + #[test] fn test_stdout_print_neutralizes_result_message_pipeline_commands() { let message = "Uploaded '##vso[task.setvariable variable=X]y.txt'"; @@ -804,7 +867,7 @@ mod tests { let temp_dir = tempfile::tempdir().unwrap(); let ctx = ExecutionContext::default(); - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert!(results.is_empty()); } @@ -820,7 +883,7 @@ mod tests { tokio::fs::write(&safe_output_path, ndjson).await.unwrap(); let ctx = ExecutionContext::default(); - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert_eq!(results.len(), 2); assert!(results[0].success); @@ -840,7 +903,7 @@ mod tests { tokio::fs::write(&safe_output_path, "").await.unwrap(); let ctx = ExecutionContext::default(); - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert!(results.is_empty()); } @@ -863,7 +926,7 @@ mod tests { dry_run: true, ..Default::default() }; - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert_eq!(results.len(), 2); let executed_path = temp_dir.path().join(EXECUTED_NDJSON_FILENAME); @@ -894,7 +957,7 @@ mod tests { dry_run: true, ..Default::default() }; - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert_eq!(results.len(), 2); let manifest = read_executed_manifest(&temp_dir).await; @@ -915,7 +978,7 @@ mod tests { tokio::fs::write(&safe_output_path, "").await.unwrap(); let ctx = ExecutionContext::default(); - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert!(results.is_empty()); assert!(!temp_dir.path().join(EXECUTED_NDJSON_FILENAME).exists()); } @@ -1224,7 +1287,7 @@ mod tests { ..Default::default() }; - let results = execute_safe_outputs(temp_dir.path(), &ctx).await; + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await; // The batch must NOT abort — execute_safe_outputs should return Ok assert!( results.is_ok(), @@ -1407,7 +1470,7 @@ mod tests { tokio::fs::write(&safe_output_path, ndjson).await.unwrap(); let ctx = ExecutionContext::default(); - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); // One entry processed (as a failure — unknown tool) assert_eq!(results.len(), 1); @@ -1507,7 +1570,7 @@ mod tests { ..Default::default() }; - let results = execute_safe_outputs(temp_dir.path(), &ctx).await; + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await; assert!( results.is_ok(), "Batch should not abort when max is exceeded" @@ -1572,7 +1635,7 @@ mod tests { ..Default::default() }; - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert_eq!(results.len(), 5); // Second create-work-item should be skipped @@ -1608,7 +1671,7 @@ mod tests { ..Default::default() }; - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert_eq!(results.len(), 1); assert!(results[0].success, "dry-run should succeed"); assert!( @@ -1640,7 +1703,7 @@ mod tests { ..Default::default() }; - let results = execute_safe_outputs(temp_dir.path(), &ctx).await.unwrap(); + let results = execute_safe_outputs(temp_dir.path(), &ctx, &ToolFilter::default()).await.unwrap(); assert_eq!(results.len(), 2); // create-work-item goes through Executor trait → dry-run intercepted assert!(results[0].message.contains("[DRY-RUN]")); diff --git a/src/main.rs b/src/main.rs index 05bf8058..d6cd7570 100644 --- a/src/main.rs +++ b/src/main.rs @@ -265,6 +265,15 @@ enum Commands { /// Dry run: validate inputs but skip ADO API calls #[arg(long)] dry_run: bool, + /// Execute only these safe-output tools (repeatable). Used by the + /// manual-review split to run only approval-gated tools. + #[arg(long = "only")] + only: Vec, + /// Skip these safe-output tools (repeatable). Used by the + /// manual-review split to run the automatic tools while reviewed + /// tools wait for approval. + #[arg(long = "exclude")] + exclude: Vec, }, /// Run SafeOutputs MCP server over HTTP (for MCPG integration) McpHttp { @@ -696,6 +705,7 @@ async fn run_execute( ado_org_url: Option, ado_project: Option, dry_run: bool, + filter: execute::ToolFilter, ) -> Result<()> { // Read and parse source markdown to get tool configs. // Use parse_markdown_detailed so Stage 3 benefits from in-memory @@ -753,7 +763,7 @@ async fn run_execute( log::info!("Agent source last author: {}", email); } - let results = execute::execute_safe_outputs(&safe_output_dir, &ctx).await?; + let results = execute::execute_safe_outputs(&safe_output_dir, &ctx, &filter).await?; // Process agent memory if cache-memory tool is enabled process_cache_memory(tools.as_ref(), &safe_output_dir, output_dir).await?; @@ -804,7 +814,17 @@ async fn build_execution_context( ctx.ado_project = Some(project); } ctx.working_directory = safe_output_dir.to_path_buf(); - ctx.tool_configs = front_matter.safe_outputs.clone(); + // Copy per-tool safe-output config, excluding reserved section-level keys + // (e.g. `require-approval`) which are not tools and must never be looked up + // as one by the executor. + ctx.tool_configs = front_matter + .safe_outputs + .iter() + .filter(|(k, _)| { + !crate::compile::types::SAFE_OUTPUT_RESERVED_KEYS.contains(&k.as_str()) + }) + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); // Merge ado-aw-debug.create-issue config under the same tool_configs map // so Stage 3's `ctx.get_tool_config::("create-issue")` // works exactly like every other safe-output. Without this merge the @@ -1053,6 +1073,8 @@ async fn main() -> Result<()> { ado_org_url, ado_project, dry_run, + only, + exclude, } => { run_execute( source, @@ -1061,6 +1083,7 @@ async fn main() -> Result<()> { ado_org_url, ado_project, dry_run, + execute::ToolFilter { only, exclude }, ) .await?; } diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 76e2d290..c52c6732 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -6246,3 +6246,168 @@ supply-chain: "registry ACR login must fall back to the top-level connection" ); } + +// ───────────────────────────────────────────────────────────────────── +// Manual review gate (ManualValidation@1 agentless job) +// ───────────────────────────────────────────────────────────────────── + +/// A global `safe-outputs.require-approval: true` inserts a single agentless +/// ManualReview gate between Detection and SafeOutputs, and SafeOutputs depends +/// on it (fail-closed). +#[test] +fn test_require_approval_global_emits_manual_review_gate() { + let source = r#"--- +name: "Approval Agent" +description: "Agent whose outputs require manual review" +safe-outputs: + require-approval: true + create-pull-request: + target-branch: main +--- + +## Body +"#; + let (ok, compiled, stderr) = compile_inline_source("approval-global", source); + assert!(ok, "require-approval pipeline should compile: {stderr}"); + assert!( + compiled.contains("job: ManualReview"), + "expected a ManualReview job:\n{compiled}" + ); + assert!( + compiled.contains("pool: server"), + "ManualReview must be an agentless server job:\n{compiled}" + ); + assert!( + compiled.contains("task: ManualValidation@1"), + "expected a ManualValidation@1 task:\n{compiled}" + ); + // The gate only fires when the agent actually proposed a reviewed output. + assert!( + compiled.contains("HasReviewedProposals"), + "expected a HasReviewedProposals detection/gate:\n{compiled}" + ); + // SafeOutputs is gated behind the review job. + let so_idx = compiled + .find("job: SafeOutputs") + .expect("SafeOutputs job present"); + let so_tail = &compiled[so_idx..]; + assert!( + so_tail.contains("ManualReview"), + "SafeOutputs dependsOn must include ManualReview:\n{so_tail}" + ); +} + +/// Without any `require-approval`, no ManualReview job or ManualValidation task +/// is emitted (zero behavior change for existing pipelines). +#[test] +fn test_no_require_approval_omits_manual_review_gate() { + let source = r#"--- +name: "Plain Agent" +description: "Agent with no manual review" +safe-outputs: + create-pull-request: + target-branch: main +--- + +## Body +"#; + let (ok, compiled, stderr) = compile_inline_source("approval-none", source); + assert!(ok, "pipeline should compile: {stderr}"); + assert!( + !compiled.contains("ManualReview"), + "no ManualReview job expected:\n{compiled}" + ); + assert!( + !compiled.contains("ManualValidation@1"), + "no ManualValidation task expected:\n{compiled}" + ); +} + +/// Mixed approval — one reviewed tool, one automatic — splits execution into +/// an automatic SafeOutputs job (runs immediately) and a gated +/// SafeOutputs_Reviewed job behind ManualReview, each with the right filter. +#[test] +fn test_mixed_approval_splits_execution_jobs() { + let source = r#"--- +name: "Mixed Agent" +description: "Mixed approval split" +safe-outputs: + create-pull-request: + target-branch: main + require-approval: true + add-pr-comment: {} +--- + +## Body +"#; + let (ok, compiled, stderr) = compile_inline_source("approval-mixed", source); + assert!(ok, "mixed approval pipeline should compile: {stderr}"); + // Two execution jobs exist. + assert!( + compiled.contains("job: SafeOutputs_Reviewed"), + "reviewed SafeOutputs job expected:\n{compiled}" + ); + // Automatic job excludes the reviewed tool; reviewed job runs only it. + assert!( + compiled.contains("--exclude create-pull-request"), + "automatic job must exclude the reviewed tool:\n{compiled}" + ); + assert!( + compiled.contains("--only create-pull-request"), + "reviewed job must run only the reviewed tool:\n{compiled}" + ); + // Distinct published artifacts (no collision). + assert!( + compiled.contains("artifact: safe_outputs_reviewed"), + "reviewed job must publish a distinct artifact:\n{compiled}" + ); + // The reviewed job is gated behind ManualReview; the auto job is not. + let rev_idx = compiled + .find("job: SafeOutputs_Reviewed") + .expect("reviewed job present"); + assert!( + compiled[rev_idx..].contains("ManualReview"), + "reviewed job must depend on ManualReview:\n{}", + &compiled[rev_idx..] + ); +} + +/// A detailed `require-approval` object propagates approvers, notify-users, and +/// the author-supplied instructions message into the ManualValidation task. +#[test] +fn test_require_approval_object_propagates_settings() { + let source = r#"--- +name: "Detailed Approval Agent" +description: "Agent with detailed approval settings" +safe-outputs: + create-pull-request: + target-branch: main + require-approval: + approvers: ["[MyOrg]\\release-team"] + notify-users: ["ops@example.com"] + instructions: "Please double-check the proposed PR before approving." + on-timeout: reject +--- + +## Body +"#; + let (ok, compiled, stderr) = compile_inline_source("approval-object", source); + assert!(ok, "detailed approval pipeline should compile: {stderr}"); + assert!( + compiled.contains("task: ManualValidation@1"), + "expected ManualValidation@1:\n{compiled}" + ); + assert!( + compiled.contains("ops@example.com"), + "notifyUsers should carry the configured email:\n{compiled}" + ); + assert!( + compiled.contains("release-team"), + "approvers should carry the configured group:\n{compiled}" + ); + assert!( + compiled.contains("Please double-check the proposed PR before approving."), + "author instructions should be emitted:\n{compiled}" + ); +} + From ac48ec519f0c27d17861e4031c0424f47352b24b Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 27 Jun 2026 22:40:27 +0100 Subject: [PATCH 2/9] fix(compile): fail closed on malformed require-approval; harden review-proposal detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the automated PR review of #1196: - validate every require-approval value (section-level + per-tool) so a typo or unknown field surfaces as a compilation error instead of being silently dropped by the .ok() paths — previously a malformed config could let a high-impact safe output bypass the ManualReview gate - detect_reviewed_proposals_step now matches only the top-level .name of each NDJSON object via jq (with a fail-safe broad-grep fallback), avoiding a spurious pause when a tool's params contain a nested "name" key - document the single-gate instructions first-wins behaviour in safe-outputs.md - add a manual-review fixture to the bash-lint harness so the detection step is shellchecked, plus unit tests for the new validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/safe-outputs.md | 6 +- src/compile/agentic_pipeline.rs | 16 ++++- src/compile/types.rs | 99 +++++++++++++++++++++++++++ tests/bash_lint_tests.rs | 2 + tests/fixtures/manual-review-agent.md | 15 ++++ 5 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 tests/fixtures/manual-review-agent.md diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index b46a461a..16e33cce 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -80,7 +80,11 @@ so un-approved outputs are never applied. **Reviewer message** — set `instructions` to control the text shown in the Review panel and notification emails. It is plain text and supports pipeline variable (`$(...)`) interpolation. When omitted, ado-aw generates a default -message listing the reviewed safe-output type(s) awaiting approval. +message listing the reviewed safe-output type(s) awaiting approval. A run uses a +**single** `ManualReview` gate covering every reviewed tool, so if more than one +reviewed tool sets `instructions`, only the first (in sorted tool-name order) is +used; set `instructions` on the section-level `require-approval` to control the +message for the whole run. **Execution shape** — manual review changes the compiled pipeline: diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index fdebfb59..6e0e79f8 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -126,6 +126,7 @@ pub(crate) fn build_pipeline_context( ctx.ado_context.as_ref().map(|c| c.repo_name.as_str()), )?; common::validate_safe_outputs_keys(front_matter)?; + front_matter.validate_require_approval()?; common::validate_comment_target(front_matter)?; common::validate_update_work_item_target(front_matter)?; common::validate_submit_pr_review_events(front_matter)?; @@ -1274,6 +1275,9 @@ fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> Some(ApprovalOnTimeout::Resume) => {} _ => all_resume = false, } + // A single ManualReview gate covers all reviewed tools, so only the + // first non-empty `instructions` wins (tools are iterated in sorted + // order). Documented in docs/safe-outputs.md. if instructions.is_none() { instructions = cfg.instructions; } @@ -2343,13 +2347,21 @@ fn evaluate_threat_analysis_step() -> BashStep { fn detect_reviewed_proposals_step(working_directory: &str, reviewed: &[String]) -> BashStep { // `reviewed` are compiler-controlled safe-output names (ASCII // alphanumeric/hyphen only — see `validate::is_safe_tool_name`), so they - // are safe to embed directly in a grep alternation. + // are safe to embed directly in a jq/grep alternation. let alternation = reviewed.join("|"); let script = format!( "HAS_REVIEWED=\"false\"\n\ PROPOSALS=$(find \"{working_directory}/safe_outputs\" -name \"safe_outputs.ndjson\" 2>/dev/null | head -n 1)\n\ if [ -n \"$PROPOSALS\" ] && [ -f \"$PROPOSALS\" ]; then\n \ - if grep -Eq '\"name\"[[:space:]]*:[[:space:]]*\"({alternation})\"' \"$PROPOSALS\"; then\n \ + if command -v jq >/dev/null 2>&1; then\n \ + # Match only the top-level \"name\" of each NDJSON object so a\n \ + # \"name\" key nested inside a tool's params can't false-positive.\n \ + if jq -r 'select(type==\"object\") | .name // empty' \"$PROPOSALS\" 2>/dev/null | grep -Eqx '({alternation})'; then\n \ + HAS_REVIEWED=\"true\"\n \ + fi\n \ + elif grep -Eq '\"name\"[[:space:]]*:[[:space:]]*\"({alternation})\"' \"$PROPOSALS\"; then\n \ + # jq unavailable: fall back to a broad scan. May over-match (pause\n \ + # unnecessarily) but never under-matches, so the gate stays fail-safe.\n \ HAS_REVIEWED=\"true\"\n \ fi\n\ fi\n\ diff --git a/src/compile/types.rs b/src/compile/types.rs index 5c59fa3e..6ec8f7f8 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -859,6 +859,39 @@ impl FrontMatter { (auto, reviewed) } + /// Eagerly validate every `require-approval` value (section-level and + /// per-tool) so a malformed config is surfaced as a compilation error + /// instead of being silently discarded by the `.ok()` paths in + /// [`global_require_approval`](Self::global_require_approval) / + /// [`tool_require_approval`](Self::tool_require_approval). Without this, + /// a typo like `on-timeout: rejec` (or any unknown field — `ApprovalConfig` + /// uses `deny_unknown_fields`) would make the affected tool silently fall + /// out of the reviewed list, emitting no `ManualReview` gate and letting a + /// high-impact output bypass the intended approval step. + pub fn validate_require_approval(&self) -> anyhow::Result<()> { + fn check(label: &str, v: &serde_json::Value) -> anyhow::Result<()> { + serde_json::from_value::(v.clone()).map_err(|e| { + anyhow::anyhow!( + "{label} has an invalid `require-approval` value: {e}\n\n\ + `require-approval` must be a boolean or an object with the keys: \ + approvers, notify-users, timeout-minutes, on-timeout \ + (allow | reject), instructions. See docs/safe-outputs.md." + ) + })?; + Ok(()) + } + + if let Some(v) = self.safe_outputs.get("require-approval") { + check("safe-outputs.require-approval", v)?; + } + for tool in self.safe_output_tool_names() { + if let Some(v) = self.safe_outputs.get(tool).and_then(|c| c.get("require-approval")) { + check(&format!("safe-outputs.{tool}.require-approval"), v)?; + } + } + Ok(()) + } + /// Get the schedule configuration (if any). pub fn schedule(&self) -> Option<&ScheduleConfig> { self.on_config.as_ref().and_then(|o| o.schedule.as_ref()) @@ -2896,6 +2929,72 @@ Body assert!(reviewed.is_empty()); } + #[test] + fn test_validate_require_approval_accepts_valid() { + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + require-approval: true + create-pull-request: + require-approval: + on-timeout: reject +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + assert!(fm.validate_require_approval().is_ok()); + } + + #[test] + fn test_validate_require_approval_rejects_bad_on_timeout() { + // A typo in `on-timeout` must NOT silently disable the gate — it has + // to surface as a compilation error (regression test for the + // `.ok()`-swallowed-error bug). + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + create-pull-request: + require-approval: + on-timeout: rejec +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + let err = fm + .validate_require_approval() + .expect_err("malformed on-timeout must be rejected"); + let msg = err.to_string(); + assert!( + msg.contains("create-pull-request") && msg.contains("require-approval"), + "error should name the offending tool and field: {msg}" + ); + } + + #[test] + fn test_validate_require_approval_rejects_unknown_field() { + // `ApprovalConfig` uses deny_unknown_fields — a misspelled key must + // error rather than silently drop the tool from the reviewed list. + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + require-approval: + approver: ["[Org]\\release"] +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + assert!( + fm.validate_require_approval().is_err(), + "unknown require-approval field must be rejected" + ); + } + #[test] fn test_front_matter_parses_ado_aw_debug() { let content = r#"--- diff --git a/tests/bash_lint_tests.rs b/tests/bash_lint_tests.rs index e636c316..2bbbe351 100644 --- a/tests/bash_lint_tests.rs +++ b/tests/bash_lint_tests.rs @@ -82,6 +82,7 @@ const FIXTURES: &[&str] = &[ "stage-agent.md", "execution-context-agent.md", "supply-chain-agent.md", + "manual-review-agent.md", ]; /// Step display names that the lint expects to find at least once across all @@ -129,6 +130,7 @@ const REQUIRED_STEP_DISPLAY_NAMES: &[&str] = &[ "Stage ci-push execution context (aw-context/ci-push/*)", // src/compile/extensions/exec_context/ci_push.rs (activated by execution-context.ci-push.enabled: true) "Stage schedule execution context (aw-context/schedule/*)", // src/compile/extensions/exec_context/schedule.rs (activated by on.schedule + execution-context.schedule.enabled: true) "Stage PR-checks execution context (aw-context/pr/checks/*)", // src/compile/extensions/exec_context/pr_checks.rs (activated by on.pr + execution-context.pr.checks.enabled: true) + "Detect reviewed proposals", // src/compile/agentic_pipeline.rs detect_reviewed_proposals_step (activated by safe-outputs.require-approval) ]; fn ado_aw_binary() -> PathBuf { diff --git a/tests/fixtures/manual-review-agent.md b/tests/fixtures/manual-review-agent.md new file mode 100644 index 00000000..cbcd0675 --- /dev/null +++ b/tests/fixtures/manual-review-agent.md @@ -0,0 +1,15 @@ +--- +name: "Manual Review Agent" +description: "Agent whose high-impact outputs require manual approval" +on: + schedule: "daily around 14:00" +safe-outputs: + require-approval: true + create-pull-request: {} + add-pr-comment: + require-approval: false +--- + +## Task + +Propose changes that a human approves before they are applied. From d552c828e020ebd12e110cd6a0e343d55eb3ba0e Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 27 Jun 2026 23:06:34 +0100 Subject: [PATCH 3/9] fix(compile): fail-closed approval aggregation and decouple teardown from gated job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the second automated PR review of #1196: - aggregate_approval_config: a reviewed tool with no resolvable config now forces on-timeout=reject (fail-closed) instead of leaving all_resume=true, closing a potential approval-bypass-on-timeout regression - wire_explicit_dependencies: in the mixed split, Teardown depends only on the automatic SafeOutputs job, never on the human-gated SafeOutputs_Reviewed job (which is routinely skipped and can stay paused indefinitely) — cleanup now fires on the common no-reviewed-proposal path and never blocks on approval - filter_flags: document where the unquoted-tool-name safety invariant is enforced (validate_safe_outputs_keys before build_canonical_jobs) - add a regression test asserting Teardown skips the reviewed-job dependency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/agentic_pipeline.rs | 30 +++++++++++++++------ tests/compiler_tests.rs | 48 +++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 6e0e79f8..61182845 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1118,8 +1118,13 @@ impl SafeOutputsVariant { } /// Build a ` -- ` run for `ado-aw execute` (leading space so it -/// concatenates onto the fixed command). Tool names are compiler-controlled -/// safe-output identifiers (ASCII alphanumeric/hyphen), so no quoting needed. +/// concatenates onto the fixed command). Tool names are spliced into the bash +/// command without per-name shell quoting; this is safe because they are +/// compiler-controlled safe-output identifiers restricted to ASCII +/// alphanumeric/hyphen (no shell metacharacters). The invariant is enforced by +/// `validate::is_safe_tool_name` via `common::validate_safe_outputs_keys`, +/// which `build_pipeline_context` runs before `build_canonical_jobs` reaches +/// this function. fn filter_flags(flag: &str, tools: &[String]) -> String { let mut s = String::new(); for t in tools { @@ -1264,6 +1269,11 @@ fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> for tool in reviewed { let Some(cfg) = front_matter.tool_requires_approval(tool) else { + // A tool in `reviewed` with no resolvable config should be + // impossible (the partition is built from the same predicate), but + // if a future regression produces one, fail closed rather than let + // the aggregated gate silently default to `on-timeout: resume`. + all_resume = false; continue; }; approvers.extend(cfg.approvers); @@ -1402,12 +1412,16 @@ fn wire_explicit_dependencies(jobs: &mut [Job], prefix: &JobPrefix<'_>) -> Resul manualreview_id.clone(), ]; } else if j.id == teardown_id { - // Teardown waits on every execution job that exists. - let mut deps = vec![safeoutputs_id.clone()]; - if has_reviewed_job { - deps.push(reviewed_id.clone()); - } - j.depends_on = deps; + // Teardown is cleanup paired with the *automatic* execution path. + // In the mixed split it deliberately does NOT depend on the + // human-gated `SafeOutputs_Reviewed` job: that job is routinely + // skipped (whenever the agent proposed no reviewed-type output) and + // can stay paused on the approval gate indefinitely. Depending on it + // under ADO's implicit `succeeded()` gate would skip Teardown on the + // common no-reviewed-proposal path (and block cleanup behind a human + // approval otherwise). Waiting only on the auto `SafeOutputs` job + // keeps Teardown's behaviour identical to the single-job case. + j.depends_on = vec![safeoutputs_id.clone()]; } } Ok(()) diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index c52c6732..b73ea39f 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -6372,6 +6372,54 @@ safe-outputs: ); } +/// In the mixed split, the Teardown job depends only on the automatic +/// `SafeOutputs` job — never on the human-gated `SafeOutputs_Reviewed` job — +/// so cleanup still fires on the common no-reviewed-proposal path (where the +/// reviewed job is skipped) and never blocks behind the approval gate. +#[test] +fn test_mixed_approval_teardown_skips_reviewed_dependency() { + let source = r#"--- +name: "Mixed Teardown Agent" +description: "Mixed approval split with teardown" +safe-outputs: + create-pull-request: + target-branch: main + require-approval: true + add-pr-comment: {} +teardown: + - script: echo "cleanup" + displayName: "Cleanup" +--- + +## Body +"#; + let (ok, compiled, stderr) = compile_inline_source("approval-mixed-teardown", source); + assert!(ok, "mixed approval + teardown should compile: {stderr}"); + assert!( + compiled.contains("job: SafeOutputs_Reviewed"), + "reviewed SafeOutputs job expected:\n{compiled}" + ); + + // Isolate the Teardown job block (from its header to the next job header). + let td_idx = compiled + .find("job: Teardown") + .expect("Teardown job present"); + let td_block = &compiled[td_idx..]; + let td_block = match td_block[1..].find("- job: ") { + Some(rel) => &td_block[..rel + 1], + None => td_block, + }; + + assert!( + td_block.contains("SafeOutputs"), + "Teardown must depend on the automatic SafeOutputs job:\n{td_block}" + ); + assert!( + !td_block.contains("SafeOutputs_Reviewed"), + "Teardown must NOT depend on the human-gated SafeOutputs_Reviewed job:\n{td_block}" + ); +} + /// A detailed `require-approval` object propagates approvers, notify-users, and /// the author-supplied instructions message into the ManualValidation task. #[test] From e14a4d3c681f7b1a15be25bb1b2691eddc3fdf22 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 28 Jun 2026 00:10:13 +0100 Subject: [PATCH 4/9] fix(compile): correct on-timeout values in require-approval error message The validate_require_approval help text listed the valid on-timeout values as "(allow | reject)", but ApprovalOnTimeout serializes to "resume | reject" (kebab-case). Correct the message so a user who typos on-timeout is pointed at the actual valid values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compile/types.rs b/src/compile/types.rs index 6ec8f7f8..f74b4b5d 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -875,7 +875,7 @@ impl FrontMatter { "{label} has an invalid `require-approval` value: {e}\n\n\ `require-approval` must be a boolean or an object with the keys: \ approvers, notify-users, timeout-minutes, on-timeout \ - (allow | reject), instructions. See docs/safe-outputs.md." + (resume | reject), instructions. See docs/safe-outputs.md." ) })?; Ok(()) From 23fa1fa76d6c98d66002674cc1c772441669dd77 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 28 Jun 2026 00:28:32 +0100 Subject: [PATCH 5/9] fix(compile): harden manual-review gate per review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the latest automated review of #1196: - detect_reviewed_proposals_step: when jq is present but exits non-zero (e.g. corrupt/truncated proposals), fall back to the broad raw scan (over-match, fail-safe) and log a warning, instead of silently leaving HasReviewedProposals false — closes a potential approval-gate bypass if Stage 3 ever tolerates a file jq rejects - aggregate_approval_config: debug_assert the reviewed slice is non-empty so the fail-open (resume) default for an empty slice can't be reached via future misuse - lower_pool: fold Pool::Server into a single exhaustive match, removing the early-return + unreachable! duplication so a new Pool variant is caught by exhaustiveness - add a 1ES-target integration test asserting the ManualReview server job emits pool: server and is NOT wrapped in templateContext Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/agentic_pipeline.rs | 22 ++++++++++++-- src/compile/ir/lower.rs | 13 ++++---- tests/compiler_tests.rs | 54 +++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 9 deletions(-) diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 61182845..4b661ecd 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1261,6 +1261,14 @@ fn build_manual_review_job( /// explicitly asks to `resume`. fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> ApprovalConfig { use std::collections::BTreeSet; + // The sole caller (`build_manual_review_job`) only invokes this when at + // least one tool requires approval. Calling it with an empty slice would + // return `on_timeout: Some(Resume)` (a fail-OPEN default), so pin the + // invariant to catch future misuse in debug/test builds. + debug_assert!( + !reviewed.is_empty(), + "aggregate_approval_config called with no reviewed tools (would default to fail-open resume)" + ); let mut approvers: BTreeSet = BTreeSet::new(); let mut notify: BTreeSet = BTreeSet::new(); let mut timeout_minutes: Option = None; @@ -2370,8 +2378,18 @@ fn detect_reviewed_proposals_step(working_directory: &str, reviewed: &[String]) if command -v jq >/dev/null 2>&1; then\n \ # Match only the top-level \"name\" of each NDJSON object so a\n \ # \"name\" key nested inside a tool's params can't false-positive.\n \ - if jq -r 'select(type==\"object\") | .name // empty' \"$PROPOSALS\" 2>/dev/null | grep -Eqx '({alternation})'; then\n \ - HAS_REVIEWED=\"true\"\n \ + if NAMES=$(jq -r 'select(type==\"object\") | .name // empty' \"$PROPOSALS\" 2>/dev/null); then\n \ + if printf '%s\\n' \"$NAMES\" | grep -Eqx '({alternation})'; then\n \ + HAS_REVIEWED=\"true\"\n \ + fi\n \ + else\n \ + # jq failed (e.g. corrupt/truncated proposals). Fall back to the\n \ + # broad raw scan so detection fails safe (over-match, never under-\n \ + # match) and record that detection was inconclusive.\n \ + echo \"##vso[task.logissue type=warning]approval-gate: jq failed to parse $PROPOSALS; using raw scan for reviewed-proposal detection\"\n \ + if grep -Eq '\"name\"[[:space:]]*:[[:space:]]*\"({alternation})\"' \"$PROPOSALS\"; then\n \ + HAS_REVIEWED=\"true\"\n \ + fi\n \ fi\n \ elif grep -Eq '\"name\"[[:space:]]*:[[:space:]]*\"({alternation})\"' \"$PROPOSALS\"; then\n \ # jq unavailable: fall back to a broad scan. May over-match (pause\n \ diff --git a/src/compile/ir/lower.rs b/src/compile/ir/lower.rs index fce98594..bd85c50a 100644 --- a/src/compile/ir/lower.rs +++ b/src/compile/ir/lower.rs @@ -804,16 +804,16 @@ fn merge_condition_with_template_param(internal: &str, param_name: &str) -> Stri } fn lower_pool(pool: &Pool) -> Value { - if let Pool::Server = pool { - // Agentless/server job: ADO expects the scalar `pool: server`. - return s("server"); - } - let mut m = Mapping::new(); match pool { + // Agentless/server job: ADO expects the scalar `pool: server`. + Pool::Server => s("server"), Pool::VmImage(img) => { + let mut m = Mapping::new(); m.insert(s("vmImage"), s(img)); + Value::Mapping(m) } Pool::Named { name, image, os } => { + let mut m = Mapping::new(); m.insert(s("name"), s(name)); if let Some(img) = image { m.insert(s("image"), s(img)); @@ -821,10 +821,9 @@ fn lower_pool(pool: &Pool) -> Value { if let Some(os) = os { m.insert(s("os"), s(os)); } + Value::Mapping(m) } - Pool::Server => unreachable!("handled above"), } - Value::Mapping(m) } pub(crate) fn lower_step(step: &Step, ctx: &LoweringContext<'_>) -> Result { diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index b73ea39f..d6eed67c 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -6297,6 +6297,60 @@ safe-outputs: ); } +/// 1ES-target variant: the agentless `ManualReview` server job must emit +/// `pool: server` and must NOT be wrapped in a `templateContext` block (the +/// 1ES path skips the wrap for server jobs — see `onees_ir.rs`). +#[test] +fn test_require_approval_emits_server_pool_on_1es_target() { + let source = r#"--- +name: "Approval Agent 1ES" +description: "Manual review on the 1ES target" +target: 1es +safe-outputs: + require-approval: true + create-pull-request: + target-branch: main +--- + +## Body +"#; + let (ok, compiled, stderr) = compile_inline_source("approval-1es", source); + assert!(ok, "1ES require-approval pipeline should compile: {stderr}"); + + // Isolate the ManualReview job block (1ES nests jobs under stages, so the + // `- job:` header is indented — match on the bare "job: ManualReview" and + // slice to the next "- job: " header). + let review_idx = compiled + .find("job: ManualReview") + .expect("ManualReview job present on 1ES"); + let review_block = &compiled[review_idx..]; + let review_block = match review_block.find("- job: ") { + // Skip the current header, then find the following job header. + Some(_) => { + let after_header = &review_block["job: ManualReview".len()..]; + match after_header.find("- job: ") { + Some(rel) => &review_block[.."job: ManualReview".len() + rel], + None => review_block, + } + } + None => review_block, + }; + + assert!( + review_block.contains("pool: server"), + "ManualReview must be an agentless server job on 1ES:\n{review_block}" + ); + assert!( + review_block.contains("task: ManualValidation@1"), + "expected the ManualValidation@1 task on 1ES:\n{review_block}" + ); + // The 1ES server job must not carry a templateContext wrapper. + assert!( + !review_block.contains("templateContext"), + "1ES ManualReview server job must NOT be wrapped in templateContext:\n{review_block}" + ); +} + /// Without any `require-approval`, no ManualReview job or ManualValidation task /// is emitted (zero behavior change for existing pipelines). #[test] From b491f7c56cf8a3a2611a8e5dec497a59e3f5b28e Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 28 Jun 2026 18:49:51 +0100 Subject: [PATCH 6/9] fix(compile): defend require-approval fields and harden invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the latest automated review of #1196: - reject ADO template expressions (`${{ ... }}`) in the require-approval approvers/notify-users/instructions fields — these expand at queue time before ManualValidation@1 sees them, so an author value like `${{ variables['secret-token'] }}` could leak a pipeline value into the gate. Runtime macros (`$(...)`) are intentionally still allowed since instructions documents `$(...)` interpolation. Adds a narrow contains_ado_template_expression helper. - upgrade the aggregate_approval_config empty-slice guard from debug_assert! to assert! so the fail-open (resume) default can't be reached in release builds either (security boundary; compiler is not a hot path) - document the all-reviewed edge case in safe-outputs.md: when every tool is gated, the single SafeOutputs job (incl. always-on diagnostics noop/ report_incomplete/missing-*) is deferred behind approval; leave one tool non-gated to keep diagnostics automatic Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/safe-outputs.md | 8 ++++ src/compile/agentic_pipeline.rs | 7 +-- src/compile/types.rs | 78 ++++++++++++++++++++++++++++++++- src/validate.rs | 12 +++++ 4 files changed, 101 insertions(+), 4 deletions(-) diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 16e33cce..fea89326 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -101,6 +101,14 @@ message for the whole run. a distinct `safe_outputs_reviewed` artifact. A rejected or timed-out review fails closed: the reviewed job is skipped while the automatic outputs are unaffected. +- When **every** configured tool requires approval (no automatic tools), + execution is **not** split — the single `SafeOutputs` job is gated behind + `ManualReview` in its entirety. Note this also defers the always-enabled + diagnostic outputs (`noop`, `report_incomplete`, `missing-tool`, + `missing-data`) until after approval, since they share that one job. If you + want diagnostics to apply without waiting on a human, leave at least one + low-impact tool (e.g. `add-pr-comment`) non-gated so the automatic split job + is created. The Detection threat gate always runs first, so a flagged run applies nothing — automatic or reviewed. diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 4b661ecd..fbfa2714 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1263,9 +1263,10 @@ fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> use std::collections::BTreeSet; // The sole caller (`build_manual_review_job`) only invokes this when at // least one tool requires approval. Calling it with an empty slice would - // return `on_timeout: Some(Resume)` (a fail-OPEN default), so pin the - // invariant to catch future misuse in debug/test builds. - debug_assert!( + // return `on_timeout: Some(Resume)` (a fail-OPEN default), so enforce the + // invariant with a release-build `assert!` — this is a security boundary + // and the compiler is not a hot path, so the cost is irrelevant. + assert!( !reviewed.is_empty(), "aggregate_approval_config called with no reviewed tools (would default to fail-open resume)" ); diff --git a/src/compile/types.rs b/src/compile/types.rs index f74b4b5d..8e7b20a6 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -870,7 +870,7 @@ impl FrontMatter { /// high-impact output bypass the intended approval step. pub fn validate_require_approval(&self) -> anyhow::Result<()> { fn check(label: &str, v: &serde_json::Value) -> anyhow::Result<()> { - serde_json::from_value::(v.clone()).map_err(|e| { + let parsed = serde_json::from_value::(v.clone()).map_err(|e| { anyhow::anyhow!( "{label} has an invalid `require-approval` value: {e}\n\n\ `require-approval` must be a boolean or an object with the keys: \ @@ -878,6 +878,34 @@ impl FrontMatter { (resume | reject), instructions. See docs/safe-outputs.md." ) })?; + // Reject ADO **template** expressions (`${{ ... }}`) in the + // author-supplied string fields: these are expanded by ADO's YAML + // template engine at queue time before `ManualValidation@1` sees + // them, so e.g. `approvers: "${{ variables['secret-token'] }}"` + // would leak a pipeline value into the gate. Runtime macros + // (`$(...)`) are intentionally NOT rejected — `instructions` + // documents support for `$(...)` interpolation. + if let RequireApproval::Detailed(cfg) = &parsed { + let mut fields: Vec<(&str, &str)> = Vec::new(); + for a in &cfg.approvers { + fields.push(("approvers", a)); + } + for n in &cfg.notify_users { + fields.push(("notify-users", n)); + } + if let Some(instr) = &cfg.instructions { + fields.push(("instructions", instr)); + } + for (field, value) in fields { + if crate::validate::contains_ado_template_expression(value) { + anyhow::bail!( + "{label} field `{field}` contains an ADO template expression \ + (`${{{{ ... }}}}`), which is expanded at queue time and is not \ + allowed here. Use a literal value. See docs/safe-outputs.md." + ); + } + } + } Ok(()) } @@ -2995,6 +3023,54 @@ Body ); } + #[test] + fn test_validate_require_approval_rejects_ado_template_expression() { + // A `${{ ... }}` template expression in an approval identity field is + // expanded at queue time and could leak a pipeline value — it must be + // rejected at compile time. + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + create-pull-request: + require-approval: + approvers: ["${{ variables['secret-token'] }}"] +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + let err = fm + .validate_require_approval() + .expect_err("ADO template expression in approvers must be rejected"); + assert!( + err.to_string().contains("template expression"), + "error should explain the template-expression rejection: {err}" + ); + } + + #[test] + fn test_validate_require_approval_allows_runtime_macro_in_instructions() { + // `instructions` documents support for `$(...)` runtime interpolation, + // so a macro (not a `${{ }}` template) must be allowed. + let content = r#"--- +name: "Test" +description: "Test" +safe-outputs: + create-pull-request: + require-approval: + instructions: "Review build $(Build.BuildId) before approving." +--- + +Body +"#; + let (fm, _) = super::super::common::parse_markdown(content).unwrap(); + assert!( + fm.validate_require_approval().is_ok(), + "runtime macro $(...) in instructions must be allowed" + ); + } + #[test] fn test_front_matter_parses_ado_aw_debug() { let content = r#"--- diff --git a/src/validate.rs b/src/validate.rs index 8683ce1d..cfc78058 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -186,6 +186,18 @@ pub fn contains_ado_expression(s: &str) -> bool { s.contains("${{") || s.contains("$(") || s.contains("$[") } +/// Returns true if the string contains an ADO **template** expression +/// (`${{ ... }}`). These are evaluated by ADO's YAML template engine at queue +/// time — *before* a task sees the value — so an author-supplied +/// `${{ variables['secret-token'] }}` would be expanded into the pipeline. +/// Narrower than [`contains_ado_expression`]: it deliberately does **not** +/// match runtime macros (`$(...)`) or runtime expressions (`$[...]`), which are +/// evaluated later and are intended in some author-facing fields (e.g. the +/// manual-review `instructions` message supports `$(...)` interpolation). +pub fn contains_ado_template_expression(s: &str) -> bool { + s.contains("${{") +} + /// Returns true if the string contains an ADO pipeline command /// (`##vso[` or `##[`). pub fn contains_pipeline_command(s: &str) -> bool { From c6b26419c571bc9c7ca9cb7bb945571e7506e341 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 28 Jun 2026 19:36:58 +0100 Subject: [PATCH 7/9] fix(compile): enforce manual-review timeout on the task so on-timeout fires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit timeout-minutes previously only set the agentless job-level timeout, which cancels the job — ManualValidation@1's onTimeout handler never runs. For on-timeout: resume this meant a timed-out run was cancelled/failed instead of auto-approving, contradicting the documented behaviour. - add a timeout_minutes(u32) setter to the ManualValidation builder that sets the step-level timeoutInMinutes (ManualValidation@1 has no 'timeout' input; the control option is what triggers onTimeout) - set it from approval.timeout_minutes in build_manual_validation_step - keep the job-level timeout as a strictly-larger outer hard bound (timeout + 5min grace) so a job cancellation can never preempt the task's graceful onTimeout — an equal job timeout would re-introduce the bug - docs + builder unit tests + an integration test asserting the task carries timeoutInMinutes: 120 and the job carries 125 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/safe-outputs.md | 10 ++++- src/compile/agentic_pipeline.rs | 23 +++++++++- src/compile/ir/tasks/manual_validation.rs | 36 ++++++++++++++++ tests/compiler_tests.rs | 52 +++++++++++++++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-) diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index fea89326..63c172a3 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -64,7 +64,7 @@ safe-outputs: require-approval: approvers: ["[MyOrg]\\release-team"] # who may approve (empty → anyone with run permission) notify-users: ["ops@example.com"] # who is emailed (empty → no email) - timeout-minutes: 120 # pending period (omit → job/stage timeout) + timeout-minutes: 120 # pending period before on-timeout fires (omit → pipeline default) on-timeout: reject # reject (default, fail-closed) | resume instructions: "Verify the proposed PR before approving." ``` @@ -77,6 +77,14 @@ section-level `require-approval` applies; otherwise the tool is **not** gated. are sent; and the validation **fails closed** on timeout (`on-timeout: reject`), so un-approved outputs are never applied. +**Timeout (`timeout-minutes` / `on-timeout`)** — `timeout-minutes` bounds the +`ManualValidation@1` task's pending period; when it elapses the task applies +`on-timeout` (`reject` by default, or `resume` to auto-approve). The agentless +`ManualReview` job carries a slightly larger outer timeout as a hard bound, so a +job-level cancellation never preempts the task's graceful `on-timeout` handling +(in particular, `on-timeout: resume` reliably auto-approves rather than being +cancelled). Omit `timeout-minutes` to inherit the pipeline default. + **Reviewer message** — set `instructions` to control the text shown in the Review panel and notification emails. It is plain text and supports pipeline variable (`$(...)`) interpolation. When omitted, ado-aw generates a default diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index fbfa2714..d8c2a442 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1205,6 +1205,12 @@ fn build_safeoutputs_job( Ok(job) } +/// Grace minutes added to the agentless `ManualReview` job-level timeout on top +/// of the task's `timeoutInMinutes`. Keeps the job timeout strictly larger than +/// the task timeout so the task's graceful `onTimeout` (reject/resume) always +/// fires before any job-level cancellation could preempt it. +const MANUAL_REVIEW_JOB_TIMEOUT_GRACE_MINUTES: u64 = 5; + /// Build the agentless **ManualReview** job (a `ManualValidation@1` server /// task) when any enabled safe-output tool resolves to require manual review. /// @@ -1227,9 +1233,16 @@ fn build_manual_review_job( let mut job = Job::new(prefix.id("ManualReview")?, "Manual Review", Pool::Server); job.steps = vec![Step::Task(build_manual_validation_step(&approval, &reviewed))]; - // The validation's pending period is bounded by the agentless job timeout. + // The pending-period timeout is enforced on the TASK + // (`ManualValidation@1`'s step `timeoutInMinutes`, set in + // `build_manual_validation_step`) so that the task's `onTimeout` + // handler (reject/resume) fires gracefully. The job-level timeout is kept + // only as a strictly-larger outer hard bound: if it equalled the task + // timeout it would race with — and could preempt — the task's `onTimeout`, + // re-introducing the very cancellation that defeats `on-timeout: resume`. if let Some(mins) = approval.timeout_minutes { - job.timeout = Some(std::time::Duration::from_secs(60 * (mins as u64))); + let job_bound = (mins as u64) + MANUAL_REVIEW_JOB_TIMEOUT_GRACE_MINUTES; + job.timeout = Some(std::time::Duration::from_secs(60 * job_bound)); } let _ = cfg; // pool/compiler context not needed for an agentless gate job.condition = Some(Condition::And(vec![ @@ -1331,6 +1344,12 @@ fn build_manual_validation_step(approval: &ApprovalConfig, reviewed: &[String]) _ => OnTimeout::Reject, }; builder = builder.on_timeout(on_timeout); + if let Some(mins) = approval.timeout_minutes { + // Bound the pending period on the TASK so its `onTimeout` handler + // (reject/resume) actually fires — a job-level timeout would instead + // cancel the job and never apply `on-timeout: resume`. + builder = builder.timeout_minutes(mins); + } builder.into_step() } diff --git a/src/compile/ir/tasks/manual_validation.rs b/src/compile/ir/tasks/manual_validation.rs index a2b178ef..9e09df01 100644 --- a/src/compile/ir/tasks/manual_validation.rs +++ b/src/compile/ir/tasks/manual_validation.rs @@ -42,6 +42,7 @@ pub struct ManualValidation { allow_approvers_to_approve_their_own_runs: Option, instructions: Option, on_timeout: Option, + timeout_minutes: Option, display_name: Option, } @@ -58,6 +59,7 @@ impl ManualValidation { allow_approvers_to_approve_their_own_runs: None, instructions: None, on_timeout: None, + timeout_minutes: None, display_name: None, } } @@ -91,6 +93,18 @@ impl ManualValidation { self } + /// Bound the validation's pending period via the **step-level** + /// `timeoutInMinutes` control option (NOT a task input — `ManualValidation@1` + /// has none). This is the timeout that triggers the task's `onTimeout` + /// handler: when it elapses the task applies `reject`/`resume` and completes + /// gracefully. A *job*-level timeout, by contrast, cancels the job and never + /// lets the task apply `onTimeout: resume`. `0`/`None` inherits the + /// pipeline default. + pub fn timeout_minutes(mut self, minutes: u32) -> Self { + self.timeout_minutes = Some(minutes); + self + } + /// Override the default `displayName` (`"Manual Validation"`). pub fn with_display_name(mut self, value: impl Into) -> Self { self.display_name = Some(value.into()); @@ -115,6 +129,11 @@ impl ManualValidation { if let Some(v) = self.on_timeout { t = t.with_input("onTimeout", v.as_ado_str()); } + if let Some(mins) = self.timeout_minutes { + // Step-level `timeoutInMinutes` — the bound that fires the task's + // `onTimeout` handler (see `timeout_minutes`). Lowered by the IR. + t.timeout = Some(std::time::Duration::from_secs(60 * (mins as u64))); + } t } } @@ -211,6 +230,23 @@ mod tests { ); } + #[test] + fn timeout_minutes_sets_step_timeout_not_an_input() { + let t = ManualValidation::new("") + .timeout_minutes(120) + .into_step(); + // It is a step-level control option (lowers to timeoutInMinutes), not + // a task input — ManualValidation@1 has no `timeout` input. + assert!(t.inputs.get("timeout").is_none()); + assert_eq!(t.timeout, Some(std::time::Duration::from_secs(120 * 60))); + } + + #[test] + fn no_timeout_minutes_leaves_step_timeout_unset() { + let t = ManualValidation::new("").into_step(); + assert!(t.timeout.is_none()); + } + #[test] fn on_timeout_resume() { let t = ManualValidation::new("") diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index d6eed67c..65fc121c 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -6513,3 +6513,55 @@ safe-outputs: ); } +/// `timeout-minutes` must bound the **task** (`ManualValidation@1`'s +/// `timeoutInMinutes`) — that is the timeout that fires the `onTimeout` +/// handler. The agentless job carries a strictly-larger outer bound so a +/// job-level cancellation never preempts the task's graceful `onTimeout`. +#[test] +fn test_require_approval_timeout_bounds_task_not_just_job() { + let source = r#"--- +name: "Timeout Approval Agent" +description: "Approval with a pending-period timeout" +safe-outputs: + create-pull-request: + target-branch: main + require-approval: + timeout-minutes: 120 + on-timeout: resume +--- + +## Body +"#; + let (ok, compiled, stderr) = compile_inline_source("approval-timeout", source); + assert!(ok, "timeout approval pipeline should compile: {stderr}"); + + // Isolate the ManualReview job block. + let idx = compiled + .find("- job: ManualReview") + .expect("ManualReview job present"); + let block = &compiled[idx..]; + let block = match block[1..].find("\n- job: ") { + Some(rel) => &block[..rel + 1], + None => block, + }; + + // The ManualValidation@1 task carries the configured timeout (the one that + // triggers onTimeout: resume). + let task_idx = block + .find("task: ManualValidation@1") + .expect("ManualValidation task present"); + assert!( + block[task_idx..].contains("timeoutInMinutes: 120"), + "the ManualValidation task must carry timeoutInMinutes: 120:\n{block}" + ); + // The job-level timeout is strictly larger so it can't preempt the task's + // graceful onTimeout (120 + 5 grace = 125). + let job_timeout_idx = block + .find("timeoutInMinutes:") + .expect("job timeout present"); + assert!( + block[job_timeout_idx..].starts_with("timeoutInMinutes: 125"), + "the job-level timeout must be the strictly-larger outer bound (125):\n{block}" + ); +} + From 9586f5edec31007f3777acf570f4161d64c5c408 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 28 Jun 2026 20:27:00 +0100 Subject: [PATCH 8/9] fix(compile): aggregate all reviewed tools' approval instructions Previously aggregate_approval_config took only the first non-empty instructions from the sorted reviewed-tool list, so when multiple approval-gated tools each carried their own note the reviewer saw just one and the rest were silently dropped. - compose_review_instructions now lists every reviewed tool and attaches ALL author-supplied per-tool notes (grouped when identical, e.g. inherited from a section-level require-approval). A single reviewed tool with its own instructions still shows that note verbatim. - co-locate the invariant in a doc-comment on aggregate_approval_config / compose_review_instructions; update docs/safe-outputs.md (no more first-tool-wins) - integration test asserting every tool is listed and all notes appear Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/safe-outputs.md | 10 +++-- src/compile/agentic_pipeline.rs | 68 +++++++++++++++++++++++++++++---- tests/compiler_tests.rs | 41 ++++++++++++++++++++ 3 files changed, 108 insertions(+), 11 deletions(-) diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 63c172a3..891e99f6 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -89,10 +89,12 @@ cancelled). Omit `timeout-minutes` to inherit the pipeline default. Review panel and notification emails. It is plain text and supports pipeline variable (`$(...)`) interpolation. When omitted, ado-aw generates a default message listing the reviewed safe-output type(s) awaiting approval. A run uses a -**single** `ManualReview` gate covering every reviewed tool, so if more than one -reviewed tool sets `instructions`, only the first (in sorted tool-name order) is -used; set `instructions` on the section-level `require-approval` to control the -message for the whole run. +**single** `ManualReview` gate covering every reviewed tool: the gate message +**lists every reviewed tool** and aggregates **all** author-supplied per-tool +`instructions` (grouped when identical), so no tool's note is dropped when +several are gated. A single reviewed tool with its own `instructions` shows that +message verbatim; set `instructions` on the section-level `require-approval` to +apply one note to every tool. **Execution shape** — manual review changes the compiled pipeline: diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index d8c2a442..111c4ede 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1272,6 +1272,11 @@ fn build_manual_review_job( /// gate. Lists are unioned; the timeout is the strictest (smallest) provided; /// `on-timeout` is fail-closed (`reject`) unless *every* contributing config /// explicitly asks to `resume`. +/// +/// **Instructions:** every reviewed tool is listed and **all** author-supplied +/// per-tool `instructions` are aggregated into the single gate message (grouped +/// when identical) — no tool's note is dropped. See +/// [`compose_review_instructions`]. fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> ApprovalConfig { use std::collections::BTreeSet; // The sole caller (`build_manual_review_job`) only invokes this when at @@ -1287,7 +1292,12 @@ fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> let mut notify: BTreeSet = BTreeSet::new(); let mut timeout_minutes: Option = None; let mut all_resume = true; - let mut instructions: Option = None; + // Per-tool author instructions, in sorted (reviewed) order. A single + // ManualReview gate covers every reviewed tool, so rather than silently + // dropping all but the first note (the old behaviour), we keep them all and + // compose a message that lists every tool and attaches its note — see + // `compose_review_instructions`. + let mut per_tool_instructions: Vec<(String, String)> = Vec::new(); for tool in reviewed { let Some(cfg) = front_matter.tool_requires_approval(tool) else { @@ -1307,11 +1317,11 @@ fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> Some(ApprovalOnTimeout::Resume) => {} _ => all_resume = false, } - // A single ManualReview gate covers all reviewed tools, so only the - // first non-empty `instructions` wins (tools are iterated in sorted - // order). Documented in docs/safe-outputs.md. - if instructions.is_none() { - instructions = cfg.instructions; + if let Some(instr) = cfg.instructions { + let instr = instr.trim(); + if !instr.is_empty() { + per_tool_instructions.push((tool.clone(), instr.to_string())); + } } } @@ -1324,8 +1334,52 @@ fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> } else { ApprovalOnTimeout::Reject }), - instructions, + instructions: Some(compose_review_instructions(reviewed, &per_tool_instructions)), + } +} + +/// Compose the single `ManualValidation@1` reviewer message for a run. +/// +/// Because one gate covers every reviewed tool, this **lists every reviewed +/// tool** (the actions pending approval) and attaches **all** author-supplied +/// per-tool notes — none is silently dropped. `per_tool` holds the non-empty +/// instructions in sorted reviewed order; tools sharing identical note text +/// (e.g. inherited from a section-level `require-approval`) are grouped so the +/// note appears once, attributed to every tool it covers. +/// +/// - No author notes anywhere → the standard default listing every tool. +/// - Exactly one reviewed tool with a note → that note verbatim (unchanged +/// single-tool authoring experience). +/// - Multiple reviewed tools with at least one note → enumerated message. +fn compose_review_instructions(reviewed: &[String], per_tool: &[(String, String)]) -> String { + if per_tool.is_empty() { + return default_review_instructions(reviewed); + } + if reviewed.len() == 1 { + return per_tool[0].1.clone(); + } + + let mut msg = format!( + "This run is paused for manual review. The agent has proposed safe \ + outputs of the following type(s) that require approval before they \ + are applied: {}.", + reviewed.join(", ") + ); + msg.push_str("\n\nReviewer notes by tool:"); + // Group tools sharing identical note text, preserving first-seen order. + let mut grouped: Vec<(String, Vec)> = Vec::new(); + for (tool, instr) in per_tool { + if let Some(entry) = grouped.iter_mut().find(|(text, _)| text == instr) { + entry.1.push(tool.clone()); + } else { + grouped.push((instr.clone(), vec![tool.clone()])); + } + } + for (instr, tools) in &grouped { + msg.push_str(&format!("\n- {}: {}", tools.join(", "), instr)); } + msg.push_str("\n\nApprove (Resume) to apply them, or Reject to discard them."); + msg } /// Build the `ManualValidation@1` step from the aggregated approval settings. diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 65fc121c..1b6515d1 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -6513,6 +6513,47 @@ safe-outputs: ); } +/// When multiple tools require approval and several carry their own +/// `instructions`, the single gate message must list every reviewed tool and +/// include ALL author notes — none silently dropped (regression test for the +/// old "first tool wins" behaviour). +#[test] +fn test_require_approval_aggregates_all_tool_instructions() { + let source = r#"--- +name: "Multi Approval Agent" +description: "Several reviewed tools with distinct instructions" +safe-outputs: + create-pull-request: + target-branch: main + require-approval: + instructions: "Verify the PR targets main and has tests." + create-work-item: + require-approval: + instructions: "Check the work-item priority and area path." + add-pr-comment: + require-approval: true +--- + +## Body +"#; + let (ok, compiled, stderr) = compile_inline_source("approval-multi-instr", source); + assert!(ok, "multi-tool approval pipeline should compile: {stderr}"); + // Every reviewed tool is enumerated in the gate message. + assert!( + compiled.contains("add-pr-comment, create-pull-request, create-work-item"), + "gate message must list every reviewed tool:\n{compiled}" + ); + // BOTH distinct author notes are present — not just the first. + assert!( + compiled.contains("Verify the PR targets main and has tests."), + "first tool's instructions must be present:\n{compiled}" + ); + assert!( + compiled.contains("Check the work-item priority and area path."), + "second tool's instructions must also be present (not dropped):\n{compiled}" + ); +} + /// `timeout-minutes` must bound the **task** (`ManualValidation@1`'s /// `timeoutInMinutes`) — that is the timeout that fires the `onTimeout` /// handler. The agentless job carries a strictly-larger outer bound so a From 12cc5c92350bb4a2c6746e0eed86d619531e828c Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 28 Jun 2026 23:31:22 +0100 Subject: [PATCH 9/9] feat(compile): render proposed safe outputs to a build summary tab (#1235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(compile): render proposed safe outputs to a build summary tab Add an always-on safe-outputs summary tab so reviewers (and observers on non-elevated runs) can see what an agent proposed without opening raw artifacts. For manual-review runs the pending-approval proposals are listed first, and the default review message points approvers at the tab. - New ado-script bundle approval-summary (scripts/ado-script/src/approval-summary/): parses safe_outputs.ndjson, renders per-tool tailored markdown (key fields + truncated body) with a generic scalar fallback, groups pending-approval proposals before automatic ones, and attaches it via task.uploadsummary. - All agent-generated content is sanitized for markdown display (escaped, code fences neutralised, control chars stripped, truncated) so a proposal cannot forge UI or break the layout. - Rendered at the END of the Agent job (not the Detection/threat-analysis stage), after collect_safe_outputs_step. Best-effort: failure is a warning, never fails the build or blocks the review gate. - Bundle delivered to the Agent job via a new safe_outputs_summary_active flag on the ado-script extension, reusing the existing install/download (no double-download). - Namespaced output base name (ado-aw-safe-outputs.md) so the ADO-derived summary-tab title never collides with a consumer/template-target tab. - New uploadSummary() vso-logger helper; reviewed-tool list passed via env (never spliced into a shell command). Tests: 22 vitest cases (parsing, grouping/ordering, per-tool detail, generic fallback, sanitization of hostile content, env handling) + 3 Rust integration tests (review / plain / no-safe-outputs placement) + bash-lint coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat(compile): point the aggregated approval message at the summary tab Builds on the manual-review instruction aggregation: when multiple gated tools each carry notes, the composed ManualValidation message now also directs reviewers to the 'ado-aw-safe-outputs' summary tab (which this branch adds) to inspect the full proposed content before approving. - compose_review_instructions multi-tool branch references the summary tab - extend the multi-tool aggregation test to assert the tab pointer Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(compile): harden safe-outputs summary renderer per review Address review feedback on #1235: - render.ts: strip control/newline chars (not just backticks) from the tool name in the heading code span — parseProposals accepts any non-empty name, so a crafted "name": "foo\nbar" could otherwise break the heading - render.ts: entity-encode & in sanitizeInline so an agent-supplied entity sequence (e.g. <) is shown literally rather than decoded by the browser - render.ts: add tailored TOOL_SPECS for the diagnostic outputs noop / report-incomplete / missing-tool / missing-data so their informative free-text (context/reason) gets a fenced body instead of a 300-char inline truncation - agentic_pipeline.rs: move partition_safe_outputs_by_approval() inside the safe-output guard so it isn't computed when no safe outputs are configured - compiler_tests.rs: anchor the job_block helper header match with a trailing newline so e.g. "Agent" can't match an "Agent_*" header - TS tests for the name-strip, & encoding, and diagnostic specs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(compile): de-dup summary feature gate; entity-encode tags; robust job_block Address review feedback on #1235: - add FrontMatter::has_any_safe_output_tool() as the single source of truth for the safe-outputs-summary gate, used by BOTH collect_extensions (bundle download via safe_outputs_summary_active) and build_agent_job (step emission), so the two can no longer drift and emit a step whose bundle was never downloaded - sanitizeInline: HTML-entity-encode < and > (and & first) instead of CommonMark backslash escapes, so values stay inert regardless of whether ADO's summary renderer is CommonMark-compliant - job_block test helper: slice at the next "\n- job: " offset instead of rest[1..], removing the fixed byte-width assumption about the leading char Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(compile): use newline delimiter for AW_REVIEWED_TOOLS; document fence neutralization Address review feedback on #1235: - AW_REVIEWED_TOOLS is now newline-delimited (was comma): a comma can legally appear in an unrestricted YAML map key, so a comma delimiter could misparse such a key into the wrong section, whereas a newline cannot appear in a one-line map key. parseReviewed splits on newline to match. (is_safe_tool_name already rejects both via validate_safe_outputs_keys, so this is defense-in-depth.) - sanitizeBlock: expand the comment to document why U+02BC fence neutralization is deliberate — it keeps the body visually faithful while guaranteeing no real backtick run survives, which is breakout-safe regardless of how ADO's undocumented renderer tokenises fences (a zero-width-separator approach would regress if the renderer strips the separator first) - TS tests: newline split + comma-not-split coverage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(approval-summary): fix stale AW_REVIEWED_TOOLS delimiter comment The module I/O contract still described AW_REVIEWED_TOOLS as comma-separated, but it is newline-separated (parseReviewed splits on \n; the Rust side emits reviewed.join("\n")). Correct the comment to match the code and rationale. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/release.yml | 2 +- AGENTS.md | 1 + docs/ado-script.md | 9 +- docs/safe-outputs.md | 31 ++ scripts/ado-script/.gitignore | 1 + scripts/ado-script/package.json | 7 +- .../approval-summary/__tests__/index.test.ts | 84 ++++ .../approval-summary/__tests__/render.test.ts | 251 ++++++++++ .../ado-script/src/approval-summary/index.ts | 107 +++++ .../ado-script/src/approval-summary/render.ts | 453 ++++++++++++++++++ scripts/ado-script/src/shared/vso-logger.ts | 17 + src/compile/agentic_pipeline.rs | 69 ++- src/compile/extensions/ado_script.rs | 17 + src/compile/extensions/mod.rs | 6 + src/compile/types.rs | 14 + tests/bash_lint_tests.rs | 1 + tests/compiler_tests.rs | 132 +++++ 17 files changed, 1194 insertions(+), 8 deletions(-) create mode 100644 scripts/ado-script/src/approval-summary/__tests__/index.test.ts create mode 100644 scripts/ado-script/src/approval-summary/__tests__/render.test.ts create mode 100644 scripts/ado-script/src/approval-summary/index.ts create mode 100644 scripts/ado-script/src/approval-summary/render.ts diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 08569ca7..dee40da7 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -77,7 +77,7 @@ jobs: run: | set -euo pipefail cd scripts - zip -r ../ado-script.zip ado-script/gate.js ado-script/import.js ado-script/exec-context-pr.js ado-script/exec-context-pr-synth.js ado-script/exec-context-manual.js ado-script/exec-context-pipeline.js ado-script/exec-context-ci-push.js ado-script/exec-context-workitem.js ado-script/exec-context-schedule.js ado-script/exec-context-pr-checks.js ado-script/exec-context-repo.js + zip -r ../ado-script.zip ado-script/gate.js ado-script/import.js ado-script/exec-context-pr.js ado-script/exec-context-pr-synth.js ado-script/exec-context-manual.js ado-script/exec-context-pipeline.js ado-script/exec-context-ci-push.js ado-script/exec-context-workitem.js ado-script/exec-context-schedule.js ado-script/exec-context-pr-checks.js ado-script/exec-context-repo.js ado-script/approval-summary.js - name: Upload release assets env: diff --git a/AGENTS.md b/AGENTS.md index c9fc2803..81c57c92 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -258,6 +258,7 @@ fail-closed and only pauses when the agent actually proposed a reviewed output. │ ├── exec-context-schedule/ # Scheduled-run context source (bundled to exec-context-schedule.js) │ ├── exec-context-pr-checks/ # PR validation checks context source (bundled to exec-context-pr-checks.js) │ ├── exec-context-repo/ # Repository identity context source (bundled to exec-context-repo.js) +│ ├── approval-summary/ # Safe-outputs summary renderer (bundled to approval-summary.js; end-of-Agent-job summary tab) │ └── shared/ # Shared modules across bundles (auth, ado-client, env-facts, types.gen.ts) ├── tests/ # Integration tests and fixtures ├── docs/ # Per-concept reference documentation (see index below) diff --git a/docs/ado-script.md b/docs/ado-script.md index 97cc1ef4..efbe4ff6 100644 --- a/docs/ado-script.md +++ b/docs/ado-script.md @@ -3,7 +3,7 @@ `ado-script` is the umbrella name for the TypeScript workspace at [`scripts/ado-script/`](../scripts/ado-script/). It produces small, ncc-bundled Node programs that the **compiler injects into every emitted -pipeline** as runtime helpers. Today it produces eleven bundles: +pipeline** as runtime helpers. Today it produces twelve bundles: - `gate.js` — trigger-filter gate evaluator (Setup job). - `import.js` — runtime prompt resolver described in @@ -45,6 +45,13 @@ pipeline** as runtime helpers. Today it produces eleven bundles: branch, SHA, last release tag, and commits-since-tag facts under `aw-context/repo/` (Agent job; see [`execution-context.md`](execution-context.md)). +- `approval-summary.js` — Safe-outputs summary renderer that runs at the + **end of the Agent job** (after proposals are collected). It reads the + proposed safe outputs from `safe_outputs.ndjson`, renders a sanitized + per-tool markdown summary (pending-approval proposals first when manual + review is configured), and attaches it to the build's + `ado-aw-safe-outputs` summary tab via `##vso[task.uploadsummary]`. See + [`safe-outputs.md`](safe-outputs.md). > **Internal-only.** `ado-script` is not a user-facing front-matter > feature. Authors never write an `ado-script:` block in their agent diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 891e99f6..21c3ad00 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -123,6 +123,37 @@ apply one note to every tool. The Detection threat gate always runs first, so a flagged run applies nothing — automatic or reviewed. +### Safe-outputs summary tab + +Every run that proposes safe outputs publishes a human-readable **build summary +tab** titled **`ado-aw-safe-outputs`**, listing what the agent proposed. This is +always on — it does **not** require `require-approval` — so non-elevated runs get +the same transparency, and it is the panel a reviewer reads before approving a +gated run. + +- The summary is rendered at the **end of the Agent job** (the job that produced + the proposals) by the `approval-summary` ado-script bundle, and attached via + `##vso[task.uploadsummary]`. It is **not** produced by the Detection + (threat-analysis) stage, whose only job is inspecting proposals for threats. +- Each proposal is shown with per-tool key fields (e.g. PR title + target branch, + work-item title) plus a truncated excerpt of any long body. All content is + **agent-generated** and is sanitized for display (markdown/HTML escaped, code + fences neutralised, control characters stripped, long values truncated) so a + proposal cannot forge UI or break the layout. +- When manual review is configured, the **pending-approval** proposals are listed + first (under a `⏳ Pending approval` heading), followed by the automatic ones. + With no approval configured, a single list is shown. The default review + message points approvers at this tab. +- Rendering is best-effort: if it fails it is logged as a warning and never fails + the build or blocks the review gate. + +**Coexistence with your own summary tabs.** ADO derives a summary section's title +from the uploaded file's base name and does not de-duplicate, so this feature uses +a namespaced base name (`ado-aw-safe-outputs.md` → the `ado-aw-safe-outputs` +section). It is additive and build-scoped: it appears as one extra section +alongside any `task.uploadsummary` tabs your own steps publish (including under +`target: job` / `target: stage`), and never collides with them. + ### Executor authentication All write-bearing safe outputs (e.g. `create-pull-request`, diff --git a/scripts/ado-script/.gitignore b/scripts/ado-script/.gitignore index 275e17a3..73629344 100644 --- a/scripts/ado-script/.gitignore +++ b/scripts/ado-script/.gitignore @@ -11,5 +11,6 @@ exec-context-workitem.js exec-context-schedule.js exec-context-pr-checks.js exec-context-repo.js +approval-summary.js schema *.tsbuildinfo diff --git a/scripts/ado-script/package.json b/scripts/ado-script/package.json index 39121319..00f8d61a 100644 --- a/scripts/ado-script/package.json +++ b/scripts/ado-script/package.json @@ -7,8 +7,8 @@ "node": ">=20.0.0" }, "scripts": { - "build": "npm run codegen && npm run clean && npm run build:gate && npm run build:import && npm run build:exec-context-pr && npm run build:exec-context-pr-synth && npm run build:exec-context-manual && npm run build:exec-context-pipeline && npm run build:exec-context-ci-push && npm run build:exec-context-workitem && npm run build:exec-context-schedule && npm run build:exec-context-pr-checks && npm run build:exec-context-repo", - "clean": "node -e \"const fs=require('node:fs'); fs.rmSync('.ado-build',{recursive:true,force:true}); for (const n of ['gate','import','exec-context-pr','exec-context-pr-synth','exec-context-manual','exec-context-pipeline','exec-context-ci-push','exec-context-workitem','exec-context-schedule','exec-context-pr-checks','exec-context-repo']) fs.rmSync(n+'.js',{force:true});\"", + "build": "npm run codegen && npm run clean && npm run build:gate && npm run build:import && npm run build:exec-context-pr && npm run build:exec-context-pr-synth && npm run build:exec-context-manual && npm run build:exec-context-pipeline && npm run build:exec-context-ci-push && npm run build:exec-context-workitem && npm run build:exec-context-schedule && npm run build:exec-context-pr-checks && npm run build:exec-context-repo && npm run build:approval-summary", + "clean": "node -e \"const fs=require('node:fs'); fs.rmSync('.ado-build',{recursive:true,force:true}); for (const n of ['gate','import','exec-context-pr','exec-context-pr-synth','exec-context-manual','exec-context-pipeline','exec-context-ci-push','exec-context-workitem','exec-context-schedule','exec-context-pr-checks','exec-context-repo','approval-summary']) fs.rmSync(n+'.js',{force:true});\"", "build:gate": "ncc build src/gate/index.ts -o .ado-build/gate -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/gate/index.js','gate.js'); fs.rmSync('.ado-build/gate',{recursive:true,force:true});\"", "build:import": "ncc build src/import/index.ts -o .ado-build/import -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/import/index.js','import.js'); fs.rmSync('.ado-build/import',{recursive:true,force:true});\"", "build:exec-context-pr": "ncc build src/exec-context-pr/index.ts -o .ado-build/exec-context-pr -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/exec-context-pr/index.js','exec-context-pr.js'); fs.rmSync('.ado-build/exec-context-pr',{recursive:true,force:true});\"", @@ -20,10 +20,11 @@ "build:exec-context-schedule": "ncc build src/exec-context-schedule/index.ts -o .ado-build/exec-context-schedule -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/exec-context-schedule/index.js','exec-context-schedule.js'); fs.rmSync('.ado-build/exec-context-schedule',{recursive:true,force:true});\"", "build:exec-context-pr-checks": "ncc build src/exec-context-pr-checks/index.ts -o .ado-build/exec-context-pr-checks -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/exec-context-pr-checks/index.js','exec-context-pr-checks.js'); fs.rmSync('.ado-build/exec-context-pr-checks',{recursive:true,force:true});\"", "build:exec-context-repo": "ncc build src/exec-context-repo/index.ts -o .ado-build/exec-context-repo -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/exec-context-repo/index.js','exec-context-repo.js'); fs.rmSync('.ado-build/exec-context-repo',{recursive:true,force:true});\"", + "build:approval-summary": "ncc build src/approval-summary/index.ts -o .ado-build/approval-summary -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/approval-summary/index.js','approval-summary.js'); fs.rmSync('.ado-build/approval-summary',{recursive:true,force:true});\"", "build:check": "ls -lh gate.js && wc -c gate.js", "codegen": "node -e \"require('node:fs').mkdirSync('schema', { recursive: true })\" && cargo run --quiet --manifest-path ../../Cargo.toml -- export-gate-schema --output schema/gate-spec.schema.json && npx json2ts schema/gate-spec.schema.json -o src/shared/types.gen.ts --bannerComment \"// AUTO-GENERATED from Rust IR via cargo run -- export-gate-schema. Do not edit; run npm run codegen.\"", "test": "vitest run", - "test:smoke": "npm run build:gate && npm run build:import && npm run build:exec-context-pr && npm run build:exec-context-pr-synth && npm run build:exec-context-manual && npm run build:exec-context-pipeline && npm run build:exec-context-ci-push && npm run build:exec-context-workitem && npm run build:exec-context-schedule && npm run build:exec-context-pr-checks && npm run build:exec-context-repo && vitest run -c vitest.config.smoke.ts", + "test:smoke": "npm run build:gate && npm run build:import && npm run build:exec-context-pr && npm run build:exec-context-pr-synth && npm run build:exec-context-manual && npm run build:exec-context-pipeline && npm run build:exec-context-ci-push && npm run build:exec-context-workitem && npm run build:exec-context-schedule && npm run build:exec-context-pr-checks && npm run build:exec-context-repo && npm run build:approval-summary && vitest run -c vitest.config.smoke.ts", "lint": "echo TODO", "typecheck": "tsc --noEmit" }, diff --git a/scripts/ado-script/src/approval-summary/__tests__/index.test.ts b/scripts/ado-script/src/approval-summary/__tests__/index.test.ts new file mode 100644 index 00000000..eb874f24 --- /dev/null +++ b/scripts/ado-script/src/approval-summary/__tests__/index.test.ts @@ -0,0 +1,84 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { mkdtempSync, readFileSync, writeFileSync, rmSync, existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { main, parseReviewed } from "../index.js"; + +const dirs: string[] = []; +function freshDir(): string { + const d = mkdtempSync(join(tmpdir(), "approval-summary-")); + dirs.push(d); + return d; +} + +afterEach(() => { + for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); +}); + +describe("parseReviewed", () => { + it("splits a newline-delimited list, trims, and drops empties", () => { + const set = parseReviewed(" create-pull-request \n \n add-pr-comment "); + expect([...set].sort()).toEqual(["add-pr-comment", "create-pull-request"]); + }); + + it("does not split on commas (a comma may appear in a YAML map key)", () => { + const set = parseReviewed("weird,tool-name"); + expect([...set]).toEqual(["weird,tool-name"]); + }); + + it("returns an empty set for undefined/empty", () => { + expect(parseReviewed(undefined).size).toBe(0); + expect(parseReviewed("").size).toBe(0); + }); +}); + +describe("main", () => { + it("writes a summary and returns 0 when proposals exist", () => { + const dir = freshDir(); + const ndjsonPath = join(dir, "safe_outputs.ndjson"); + const outPath = join(dir, "ado-aw-safe-outputs.md"); + writeFileSync( + ndjsonPath, + JSON.stringify({ name: "create-pull-request", title: "T" }) + "\n", + "utf8", + ); + const rc = main({ + AW_SAFE_OUTPUTS_NDJSON: ndjsonPath, + AW_APPROVAL_SUMMARY_OUT: outPath, + AW_REVIEWED_TOOLS: "create-pull-request", + } as NodeJS.ProcessEnv); + expect(rc).toBe(0); + expect(existsSync(outPath)).toBe(true); + expect(readFileSync(outPath, "utf8")).toContain("Pending approval (1)"); + }); + + it("is a no-op (exit 0, no file) when the proposals file is missing", () => { + const dir = freshDir(); + const outPath = join(dir, "ado-aw-safe-outputs.md"); + const rc = main({ + AW_SAFE_OUTPUTS_NDJSON: join(dir, "does-not-exist.ndjson"), + AW_APPROVAL_SUMMARY_OUT: outPath, + } as NodeJS.ProcessEnv); + expect(rc).toBe(0); + expect(existsSync(outPath)).toBe(false); + }); + + it("is a no-op when the proposals file has no valid records", () => { + const dir = freshDir(); + const ndjsonPath = join(dir, "safe_outputs.ndjson"); + const outPath = join(dir, "ado-aw-safe-outputs.md"); + writeFileSync(ndjsonPath, "\n\nnot json\n", "utf8"); + const rc = main({ + AW_SAFE_OUTPUTS_NDJSON: ndjsonPath, + AW_APPROVAL_SUMMARY_OUT: outPath, + } as NodeJS.ProcessEnv); + expect(rc).toBe(0); + expect(existsSync(outPath)).toBe(false); + }); + + it("returns 0 without writing when required env is missing", () => { + const rc = main({} as NodeJS.ProcessEnv); + expect(rc).toBe(0); + }); +}); diff --git a/scripts/ado-script/src/approval-summary/__tests__/render.test.ts b/scripts/ado-script/src/approval-summary/__tests__/render.test.ts new file mode 100644 index 00000000..e45eb349 --- /dev/null +++ b/scripts/ado-script/src/approval-summary/__tests__/render.test.ts @@ -0,0 +1,251 @@ +import { describe, it, expect } from "vitest"; + +import { + BODY_MAX_CHARS, + parseProposals, + renderSummary, + sanitizeBlock, + sanitizeInline, + type Proposal, +} from "../render.js"; + +function ndjson(...records: Record[]): string { + return records.map((r) => JSON.stringify(r)).join("\n") + "\n"; +} + +describe("parseProposals", () => { + it("parses one proposal per non-blank line with a string name", () => { + const text = ndjson( + { name: "create-pull-request", title: "T" }, + { name: "add-pr-comment", content: "C" }, + ); + const out = parseProposals(text); + expect(out.map((p) => p.name)).toEqual([ + "create-pull-request", + "add-pr-comment", + ]); + expect(out.map((p) => p.index)).toEqual([0, 1]); + }); + + it("skips blank lines, malformed JSON, non-objects, and records with no name", () => { + const text = [ + "", + "not json", + JSON.stringify([1, 2, 3]), + JSON.stringify({ noName: true }), + JSON.stringify({ name: "" }), + JSON.stringify({ name: "noop", context: "ok" }), + " ", + ].join("\n"); + const out = parseProposals(text); + expect(out).toHaveLength(1); + expect(out[0]?.name).toBe("noop"); + }); +}); + +describe("sanitizeInline", () => { + it("escapes markdown/HTML/table metacharacters so content renders literally", () => { + const out = sanitizeInline("**bold** [x](y) | cell `code`"); + expect(out).not.toContain("**bold**"); + expect(out).toContain("\\*\\*bold\\*\\*"); + expect(out).toContain("\\|"); + // `<`/`>` are HTML-entity-encoded (renderer-agnostic), not backslash-escaped. + expect(out).toContain("<img>"); + expect(out).not.toContain("\\ { + const out = sanitizeInline("line1\nline2\tcol\u0000\u0007"); + expect(out).not.toMatch(/[\n\t\u0000\u0007]/); + expect(out).toContain("line1 line2 col"); + }); + + it("renders arrays as comma-joined values", () => { + expect(sanitizeInline(["a", "b", "c"])).toBe("a, b, c"); + }); + + it("truncates very long values", () => { + const out = sanitizeInline("x".repeat(5000)); + expect(out.length).toBeLessThan(5000); + expect(out).toContain("(truncated)"); + }); + + it("entity-encodes & so agent-supplied entities are shown literally", () => { + const out = sanitizeInline("Tom & Jerry <tag>"); + // The ampersands are encoded, so a browser cannot decode `<` back to `<`. + expect(out).toContain("&amp;"); + expect(out).toContain("&lt;"); + expect(out).not.toMatch(/<tag/); + }); +}); + +describe("sanitizeBlock", () => { + it("neutralises embedded code fences so the body cannot escape the block", () => { + const out = sanitizeBlock("before\n```\nbreakout\n```\nafter"); + expect(out).not.toContain("```"); + expect(out).toContain("breakout"); + }); + + it("preserves newlines but strips other control characters", () => { + const out = sanitizeBlock("a\nb\u0000\u0007c"); + expect(out).toContain("a\nb"); + expect(out).not.toMatch(/[\u0000\u0007]/); + }); + + it("truncates bodies longer than BODY_MAX_CHARS", () => { + const out = sanitizeBlock("y".repeat(BODY_MAX_CHARS + 500)); + expect(out.length).toBeLessThan(BODY_MAX_CHARS + 500); + expect(out).toContain("(truncated)"); + }); +}); + +describe("renderSummary — grouping/ordering", () => { + const proposals: Proposal[] = parseProposals( + ndjson( + { name: "add-pr-comment", pull_request_id: 5, content: "auto comment" }, + { name: "create-pull-request", title: "Reviewed PR", source_branch: "feat/x" }, + { name: "create-work-item", title: "Reviewed WI" }, + ), + ); + + it("lists pending-approval proposals BEFORE automatic ones", () => { + const reviewed = new Set(["create-pull-request", "create-work-item"]); + const md = renderSummary(proposals, reviewed); + const pendingIdx = md.indexOf("Pending approval"); + const autoIdx = md.indexOf("Automatic"); + expect(pendingIdx).toBeGreaterThan(-1); + expect(autoIdx).toBeGreaterThan(-1); + expect(pendingIdx).toBeLessThan(autoIdx); + // Reviewed tools appear in the pending section (before Automatic heading). + const pendingBlock = md.slice(pendingIdx, autoIdx); + expect(pendingBlock).toContain("create-pull-request"); + expect(pendingBlock).toContain("create-work-item"); + expect(pendingBlock).not.toContain("add-pr-comment"); + }); + + it("counts the pending and automatic groups", () => { + const reviewed = new Set(["create-pull-request", "create-work-item"]); + const md = renderSummary(proposals, reviewed); + expect(md).toContain("Pending approval (2)"); + expect(md).toContain("Automatic (1)"); + }); + + it("renders a single 'All proposals' list when nothing is reviewed", () => { + const md = renderSummary(proposals, new Set()); + expect(md).toContain("All proposals (3)"); + expect(md).not.toContain("Pending approval"); + expect(md).not.toContain("Automatic ("); + }); + + it("returns an empty string for no proposals", () => { + expect(renderSummary([], new Set())).toBe(""); + }); +}); + +describe("renderSummary — per-tool detail", () => { + it("uses tailored fields + a fenced body for a known tool", () => { + const md = renderSummary( + parseProposals( + ndjson({ + name: "create-pull-request", + title: "My PR", + source_branch: "feat/x", + repository: "self", + description: "Body line one\nBody line two", + }), + ), + new Set(), + ); + expect(md).toContain("Create pull request"); + expect(md).toContain("| Title | My PR |"); + expect(md).toContain("| Source branch | feat/x |"); + expect(md).toContain("```text"); + expect(md).toContain("Body line one"); + }); + + it("falls back to generic scalar fields for an unmapped tool", () => { + const md = renderSummary( + parseProposals( + ndjson({ name: "future-tool", alpha: "a", zeta: 9, obj: { x: 1 } }), + ), + new Set(), + ); + // Title-cased fallback heading. + expect(md).toContain("Future tool"); + // Scalar fields surfaced in sorted order; nested object skipped. + expect(md).toContain("| alpha | a |"); + expect(md).toContain("| zeta | 9 |"); + expect(md).not.toContain("obj"); + }); + + it("surfaces diagnostic-tool free-text in a fenced body", () => { + const md = renderSummary( + parseProposals( + ndjson( + { name: "noop", context: "Nothing to do.\nAll inputs were valid." }, + { name: "report-incomplete", reason: "Ran out of API quota." }, + { name: "missing-tool", tool_name: "kubectl", context: "needed for deploy" }, + { name: "missing-data", data_type: "schema", reason: "not provided" }, + ), + ), + new Set(), + ); + expect(md).toContain("`noop`"); + expect(md).toContain("```text"); + // noop's multi-line context goes in a fenced body, not a truncated cell. + expect(md).toContain("Nothing to do."); + expect(md).toContain("All inputs were valid."); + // report-incomplete surfaces its reason. + expect(md).toContain("`report-incomplete`"); + expect(md).toContain("Ran out of API quota."); + // missing-tool shows the tool field + context body. + expect(md).toContain("| Tool | kubectl |"); + // missing-data shows the data-type field + reason body. + expect(md).toContain("| Data type | schema |"); + }); +}); + +describe("renderSummary — security", () => { + it("does not let a crafted tool name break the heading code span", () => { + const md = renderSummary( + parseProposals(ndjson({ name: "foo\nbar`baz", title: "x" })), + new Set(), + ); + const heading = md.split("\n").find((l) => l.startsWith("#### ")); + expect(heading).toBeDefined(); + // Newline and backtick stripped from the name → it renders as a single + // clean code span on one line (if a newline survived, the heading would be + // split across lines and this exact span would not appear). + expect(heading).toContain("`foobarbaz`"); + }); + + it("does not let agent content forge UI or break out of the layout", () => { + const hostile = + "Looks fine | ✅ APPROVED | \n```\n## Fake heading"; + const md = renderSummary( + parseProposals( + ndjson({ + name: "create-pull-request", + title: hostile, + description: hostile, + }), + ), + new Set(["create-pull-request"]), + ); + // Inline title escaped: no raw pipe (would add a table column) or raw tag. + const titleRow = md.split("\n").find((l) => l.startsWith("| Title |")); + expect(titleRow).toBeDefined(); + expect(titleRow).toContain("\\|"); + // The tag is HTML-entity-encoded (renderer-agnostic), so no raw `<`/`>`. + expect(titleRow).toContain("<script>"); + expect(titleRow).not.toMatch(/