feat(compile): gate high-impact safe outputs behind manual review

[jamesadevine]

Summary

Wires the previously-unused ManualValidation@1 builder into the compiler so agents can propose higher-impact safe outputs that a human approves before Stage 3 applies them.

Manual review is opt-in (default false) and configured under the existing safe-outputs: map, matching current conventions:

safe-outputs:
  require-approval: true          # section-level default for every tool
  create-pull-request: {}
  add-pr-comment:
    require-approval: false       # per-tool override wins over the default

require-approval accepts a bare boolean or an object for finer control:

require-approval:
  approvers: ["[Org]\\release-team"]   # empty → anyone with run permission
  notify-users: ["ops@example.com"]    # empty → no email
  timeout-minutes: 120                  # omit → job/stage timeout
  on-timeout: reject                    # default — fail-closed
  instructions: "Verify the proposed PR before approving."

Resolution / defaults: per-tool require-approval > section-level > unset (false). A bare require-approval: true pauses the run, lets anyone with run permission approve/reject, sends no emails, and fails closed on timeout.

Compiled pipeline shape

• New agentless ManualReview job (pool: server, ManualValidation@1) inserted between Detection and SafeOutputs.
• Automatic outputs always execute — the auto SafeOutputs job depends only on Agent + Detection (--exclude the reviewed tools) and runs in parallel with the review pause, independent of the approval outcome.
• Reviewed outputs run in a separate SafeOutputs_Reviewed job (--only the reviewed tools), gated behind ManualReview, publishing a distinct safe_outputs_reviewed artifact.
• The gate only pauses when Detection cleared the run and the agent actually proposed a reviewed-type output (a Detection step sets HasReviewedProposals) — no pointless pauses.
• The Detection threat gate still runs first: a flagged run applies nothing, automatic or reviewed.

Implementation notes

• RequireApproval/ApprovalConfig types + per-tool resolution (types.rs); the reserved require-approval key is filtered everywhere safe-outputs: keys are treated as tool names (common.rs, main.rs).
• New Pool::Server agentless IR variant emitting the scalar pool: server (ir/job.rs, ir/lower.rs, ir/summary.rs); 1ES path skips the templateContext wrap for server jobs (onees_ir.rs).
• ado-aw execute gains --only/--exclude tool filters (execute.rs, main.rs).
• Audit aggregates execution records across both safe-output artifacts (audit/analyzers/safe_outputs.rs).
• Shared canonical-graph wiring covers all targets (standalone, 1es, job, stage).

Test plan

• cargo build
• cargo clippy --all-targets --all-features — clean
• cargo test — 2237 unit tests + all integration suites green
• cargo test --test bash_lint_tests — shellcheck passes on the new bash bodies
• New tests: global gate, no-gate (backward compat), detailed object propagation, mixed auto/reviewed split, executor ToolFilter, conditional pause (HasReviewedProposals), and audit aggregation across split artifacts
• Existing golden lock files unchanged (the feature is inert without require-approval)

[github-actions]

🔍 Rust PR Review

Summary: Well-designed feature with one correctness issue that could silently bypass the approval gate on misconfiguration, plus a minor false-positive risk in the proposal-detection heuristic.

---

Findings

🐛 Bugs / Logic Issues

src/compile/types.rs — Silent failure on malformed require-approval config

Both global_require_approval() and tool_require_approval() use .ok() to discard deserialization errors:

.and_then(|v| serde_json::from_value::<RequireApproval>(v.clone()).ok())

If the user writes a syntactically legal YAML value that fails to deserialize as RequireApproval — for example a typo like on-timeout: rejec — the method silently returns None. Because tool_requires_approval() only returns Some when the setting exists and deserializes correctly, the tool silently falls out of the reviewed list, no ManualReview gate is emitted, and high-impact outputs bypass the intended approval step with no error or warning.

ApprovalConfig uses #[serde(deny_unknown_fields)], so even a mild typo in any field name will silently disable the gate for that tool.

The validate_safe_outputs_keys function validates key names but not require-approval values. The fix is to attempt the deserialization there (or in a dedicated validate_require_approval_config call) and surface it as a compilation error rather than None.

---

🔒 Security Concerns

None beyond the silent-failure issue above (which is correctness as much as security).

---

⚠️ Suggestions

src/compile/agentic_pipeline.rs:detect_reviewed_proposals_step — grep may produce false positives

if grep -Eq '"name"[[:space:]]*:[[:space:]]*"({alternation})"' "$PROPOSALS"

This pattern scans the raw NDJSON text and will match a "name" key anywhere in the document, including inside a tool's params. For example, a create-work-item output whose params happen to contain "name": "create-pull-request" would set HasReviewedProposals=true and unnecessarily pause the run. The consequence is a spurious human-review pause (not a security bypass, since ManualReview still requires approval), but it degrades UX.

A more robust check would anchor to top-level object keys, e.g. using jq if it is available in the agent environment, or tightening the pattern to only match at the start of a JSON object (^\{"name": with -E). Alternatively, since ado-aw itself is available on PATH in Stage 2, a dedicated ado-aw subcommand could be the right long-term approach.

src/compile/agentic_pipeline.rs:filter_flags — unquoted tool names in generated bash

s.push_str(&format!(" {flag} {t}"));

Tool names are not quoted before being spliced into the bash script. This is currently safe because is_safe_tool_name restricts them to [a-zA-Z0-9-]. However the correctness depends on the validation pass having run before build_safeoutputs_job is called — there is no type-level guarantee. A defense-in-depth quoting (" {flag} \"{t}\"") would make the relationship explicit.

src/compile/agentic_pipeline.rs:aggregate_approval_config — instructions first-wins is undocumented at the YAML level

When multiple reviewed tools each specify instructions, only the first one encountered is used. This is a reasonable simplification for a single-gate pipeline, but the docs (docs/safe-outputs.md) don't mention this: a user who sets per-tool instructions for two reviewed tools will silently lose one. Worth a one-liner note in the doc.

---

✅ What Looks Good

• SAFE_OUTPUT_RESERVED_KEYS + safe_output_tool_names(): filtering the reserved key out of every tool-name iteration path (generate_enabled_tools_args, validate_safe_outputs_keys, build_execution_context) is thorough and the unit test in common.rs covers it well.
• Pool::Server IR variant: the scalar pool: server lowering is clean, and the explicit guard in onees_ir.rs that skips the 1ES templateContext wrap for server jobs is exactly the right approach.
• aggregate_approval_config fail-closed default: all_resume initialized to true but set to false for every tool whose on_timeout is None correctly implements "reject unless every config explicitly opts out" semantics.
• build_execution_context fix: filtering require-approval out of ctx.tool_configs before Stage 3 is important correctness work — the executor must never see reserved keys as tool configs.
• SafeOutputsVariant abstraction: cleanly encapsulates the three execution shapes (single-default, auto, reviewed) without duplicating job-builder logic.
• Backward compatibility: the reviewed.is_empty() || auto.is_empty() gate that collapses to default_single() ensures existing pipelines are unaffected, and the golden lock files remaining unchanged confirms this.
• Test coverage: the combination of unit tests in types.rs, the lower IR test for pool: server, the four new integration tests in compiler_tests.rs, and the audit aggregation test is solid.

Generated by Rust PR Reviewer for issue #1196 · 273.3 AIC · ⌖ 12.6 AIC · ⊞ 34.9K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-architected feature with strong security defaults and thorough tests. Two issues worth addressing.

---

Findings

🐛 Bugs / Logic Issues

aggregate_approval_config — all_resume starts optimistic, continue keeps it that way
(src/compile/agentic_pipeline.rs, aggregate_approval_config)

let mut all_resume = true;
for tool in reviewed {
    let Some(cfg) = front_matter.tool_requires_approval(tool) else {
        continue;   // ← does NOT set all_resume = false
    };
    match cfg.on_timeout {
        Some(ApprovalOnTimeout::Resume) => {}
        _ => all_resume = false,
    }
}

If tool_requires_approval returns None for any tool in reviewed (shouldn't happen today, but could be a future regression), the continue leaves all_resume = true. That means the aggregated gate would silently default to on-timeout: resume — the opposite of fail-closed — even though no tool explicitly asked for it.

This is a security-sensitive code path (approval bypass on timeout). The conservative fix is to treat the missing-config case as "not resume":

let Some(cfg) = front_matter.tool_requires_approval(tool) else {
    all_resume = false;  // no config → fail-closed by default
    continue;
};

---

Teardown may be skipped when SafeOutputs_Reviewed is skipped (mixed split case)
(src/compile/agentic_pipeline.rs, wire_explicit_dependencies)

In the mixed split scenario (some auto, some reviewed tools), SafeOutputs_Reviewed gets added to Teardown's depends_on. In ADO, ADO's implicit succeeded() gate on a dependency means Teardown is skipped if SafeOutputs_Reviewed is skipped — which happens whenever the agent proposed no reviewed-type outputs that run. This means teardown scripts don't fire even though the auto SafeOutputs job ran successfully.

The pre-existing single-job form has the same issue (Teardown skips when SafeOutputs is skipped), so this isn't a regression per se. But adding a second skippable dependency to Teardown's depends_on increases the surface area. A succeededOrSkipped condition on SafeOutputs_Reviewed (or moving to an explicit Condition::SucceededOrFailed on the Teardown job itself) would prevent the gap.

---

⚠️ Suggestions

filter_flags embeds tool names without quoting — add inline comment
(src/compile/agentic_pipeline.rs, filter_flags)

The safety of embedding tool names directly in the bash command string (no shell quoting around each name) relies on validate::is_safe_tool_name having already rejected names containing shell metacharacters. This invariant holds today (validate_safe_outputs_keys runs before build_canonical_jobs), but the function body contains no comment to that effect. The analogous comment in detect_reviewed_proposals_step does explain the invariant — adding it to filter_flags would make the coupling explicit.

---

✅ What Looks Good

• validate_require_approval in build_pipeline_context — eagerly re-parses every require-approval value after the initial .ok() silent-swallow paths, so a typo like on-timeout: rejec surfaces as a compilation error instead of silently removing the gate. The regression test confirming this is exactly right.

• safe_output_tool_names() iterator — cleanly centralises the reserved-key filter for require-approval. All downstream consumers (validate_safe_outputs_keys, generate_enabled_tools_args, build_execution_context) route through it; no gap where the key could leak through as a tool name.

• Fail-closed defaults throughout — bare require-approval: true defaults to on-timeout: reject; aggregate_approval_config defaults to reject unless every contributing tool explicitly sets resume; ManualReview gate is conditioned on HasReviewedProposals so a run never stalls for a human when nothing requires review.

• Sorted partition output — partition_safe_outputs_by_approval sorts both lists, making --only/--exclude flags and the bash alternation pattern deterministic across runs and golden-file safe.

• Test coverage — the typo regression test (on-timeout: rejec), the deny_unknown_fields test, the execution_records_aggregate_across_split_artifacts audit test, and the four compiler integration tests cover the critical branches well.

Generated by Rust PR Reviewer for issue #1196 · 297.6 AIC · ⌖ 12.1 AIC · ⊞ 34.6K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation overall — fail-closed by design, well-tested, and backward-compatible. One bug in an error message found; everything else looks correct.

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs:877-878 — Wrong values in error message

  validate_require_approval emits this help text on a bad config:

  on-timeout (allow | reject)
  

  But ApprovalOnTimeout uses #[serde(rename_all = "kebab-case")] and has variants Reject and Resume — the valid YAML values are reject and resume. Saying allow instead of resume will mislead users who hit a typo in on-timeout.

  Fix:

  // line 877-878
  approvers, notify-users, timeout-minutes, on-timeout \
  (resume | reject), instructions. See docs/safe-outputs.md."

---

✅ What Looks Good

• Fail-closed everywhere it matters: default on_timeout is reject; aggregate_approval_config treats a missing on_timeout as reject; the ManualReview gate condition uses AND(Succeeded, SafeToProcess=true, HasReviewedProposals=true) so no pointless pauses and no approval bypass on unsafe runs.

• validate_require_approval() early-exit: The explicit validation pass prevents the .ok() silent-swallow pattern in global_require_approval/tool_require_approval from silently downgrading a high-impact output from reviewed to automatic due to a typo.

• detect_reviewed_proposals_step injection safety: Tool names come from safe_output_tool_names() which filters through is_safe_tool_name() ([a-zA-Z0-9-]+). No shell-dangerous chars can reach the jq alternation. The grep fallback is documented as over-matching rather than under-matching — correct fail-safe direction.

• Pool::Server IR variant: Clean separation; 1ES path correctly skips templateContext wrapping for agentless jobs.

• Teardown dependency design: Not depending on SafeOutputs_Reviewed in the mixed split is the right call — the comment explaining why is clear and matches ADO's implicit succeeded() semantics.

• Audit find_execution_files → Vec<PathBuf>: Collecting all executed.ndjson files across split artifacts and assigning contiguous indices avoids record collisions cleanly.

• ToolFilter::allows() semantics: only + exclude interact sensibly; the empty-filter default preserves all existing call sites without changes (confirmed by the large batch of updated call sites in execute.rs).

• Test coverage: New tests cover global gate, per-tool override, mixed split, all-reviewed, no-review, Teardown dependency, ToolFilter logic, and audit aggregation across split artifacts. All golden lock files unchanged confirms zero behavior change for existing pipelines.

Generated by Rust PR Reviewer for issue #1196 · 762.2 AIC · ⌖ 12.3 AIC · ⊞ 34.6K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the implementation is solid and security-conscious. Four minor observations worth considering before merge.

---

Findings

⚠️ Suggestions

src/compile/agentic_pipeline.rs — detect_reviewed_proposals_step: silent jq failure

if jq -r 'select(type=="object") | .name // empty' "$PROPOSALS" 2>/dev/null | grep -Eqx '({alternation})'; then

When jq is available but exits non-zero (e.g. a corrupt/truncated safe_outputs.ndjson), the 2>/dev/null silences the error and grep receives empty input → HAS_REVIEWED stays "false" → ManualReview gate never fires. The comment says this is "fail-safe", but that's only true if Stage 3 also fails on the same corrupt file. If Stage 3 somehow recovers (or a future change makes it more tolerant), a reviewed-type output could bypass the approval gate.

Consider adding a || true note or jq ... || HAS_REVIEWED="false" followed by an explicit echo "##vso[task.logissue type=warning]jq failed..." so the audit log at least records when detection was inconclusive. Or—simpler—add || HAS_REVIEWED="false" with a warning ##vso log message on jq failure so it's visible in the build log.

---

src/compile/agentic_pipeline.rs — aggregate_approval_config: missing invariant guard

fn aggregate_approval_config(front_matter: &FrontMatter, reviewed: &[String]) -> ApprovalConfig {
    let mut all_resume = true;
    ...
    for tool in reviewed { ... }
    // If reviewed is somehow empty: on_timeout = Resume (open gate)

The only caller (build_manual_review_job) already guards reviewed.is_empty(), but nothing inside aggregate_approval_config enforces this. If it were ever called with an empty slice, it returns on_timeout: Some(Resume) — a fail-open default. Adding debug_assert!(!reviewed.is_empty(), "aggregate_approval_config called with no reviewed tools") makes the invariant explicit and would catch future misuse in debug builds.

---

src/compile/ir/lower.rs — Pool::Server unreachable! arm

fn lower_pool(pool: &Pool) -> Value {
    if let Pool::Server = pool {
        return s("server");
    }
    let mut m = Mapping::new();
    match pool {
        ...
        Pool::Server => unreachable!("handled above"),
    }

Correct, but brittle: the exhaustive match arms are out of sync with the early-return guard. If Pool gains a new variant in the future, the compiler will force an arm into the match body but the reviewer must remember the guard exists above. Collapsing this into a single match expression would be cleaner:

fn lower_pool(pool: &Pool) -> Value {
    if matches!(pool, Pool::Server) {
        return s("server");
    }
    // ... or fold Server into the match directly
}

Minor — not blocking.

---

tests/compiler_tests.rs — no 1ES-target coverage for manual review

All five new integration tests use the implicit standalone target. The onees_ir.rs change (skip templateContext for Pool::Server jobs) has no direct test; the only coverage comes from bash_lint_tests which also uses the standalone fixture. A single target: 1es variant of test_require_approval_global_emits_manual_review_gate would confirm that the server-pool skip actually fires on the 1ES path and that the compiled artifact doesn't accidentally contain templateContext: ... under the ManualReview job.

---

✅ What Looks Good

• Security invariant for tool-name injection is airtight: validate_safe_outputs_keys → is_safe_tool_name (alphanumeric/hyphen only) runs in build_pipeline_context before build_canonical_jobs ever reaches detect_reviewed_proposals_step or filter_flags, so there's no path where an unvalidated name reaches a shell pattern.
• validate_require_approval() regression test is excellent: The test for the silent-.ok()-discard bug (on-timeout: rejec) is exactly the right thing to pin.
• SAFE_OUTPUT_RESERVED_KEYS constant + safe_output_tool_names() iterator: Clean, centralised fix for the reserved-key problem; every consumer that iterated safe_outputs.keys() directly is updated.
• Fail-closed throughout: on-timeout defaults to Reject; aggregate_approval_config requires all configs to opt in to Resume for the gate to resume; a rejected ManualReview fails the gate and cascades to skip SafeOutputs_Reviewed. The fail-closed posture is consistently enforced.
• Teardown dependency design is well-reasoned: The comment explaining why Teardown must not depend on SafeOutputs_Reviewed in the mixed split is precise and prevents a subtle always-block-or-skip pitfall.
• detect_reviewed_proposals_step jq vs grep duality: Matching only the top-level .name key with jq to avoid nested-field false-positives, with an acknowledged over-matching fallback, is a good layered defence.

Generated by Rust PR Reviewer for issue #1196 · 548.4 AIC · ⌖ 12.9 AIC · ⊞ 34.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured feature with strong security defaults and comprehensive tests. Two minor concerns worth noting.

Findings

🔒 Security Concerns

• src/compile/agentic_pipeline.rs (aggregate_approval_config) — The security-critical invariant protecting against fail-open on_timeout: Resume is guarded only by a debug_assert!:

  debug_assert!(
      !reviewed.is_empty(),
      "aggregate_approval_config called with no reviewed tools (would default to fail-open resume)"
  );

  In a release build, calling this with an empty reviewed slice yields on_timeout: Some(Resume) — a fail-open gate. The call site (build_manual_review_job) already guards this correctly with an is_empty() check before calling, so no current path triggers it. But for a security invariant of this nature, a regular assert! (which fires in release builds too) would be more defensive. Since this is a compiler binary rather than a hot path, the performance argument for debug_assert! doesn't apply.

• src/compile/types.rs (ApprovalConfig.instructions / approvers / notify_users) — These string fields are not checked for ADO template expressions (${{...}}). A value like approvers: "${{ variables['secret-token'] }}" would be expanded by ADO's YAML template engine at queue time before the ManualValidation@1 task sees it. This is an author-controlled field so the risk is low, but it's inconsistent with how other user-supplied strings (e.g. source_path) are validated via contains_ado_expression.

⚠️ Suggestions

• src/compile/agentic_pipeline.rs (detect_reviewed_proposals_step) — The grep fallback (when jq is unavailable) scans raw text and can false-positive on nested "name" keys inside a tool's params (e.g. a create_work_item param named "name": "create-pull-request"). This is already documented in the comment as an intentional over-match (fail-safe rather than fail-open), which is the right tradeoff. The note is just so reviewers are aware this is a known limitation of the fallback path, not a bug.

• src/compile/agentic_pipeline.rs (SafeOutputsVariant::default_single in the all-reviewed case) — When auto.is_empty() (every configured tool requires approval), the single SafeOutputs job uses default_single() with no filter flags. This means diagnostic safe outputs (noop, report_incomplete) — which are always enabled but not listed in safe_outputs: — only execute after human approval. This edge-case behaviour isn't explicitly documented in docs/safe-outputs.md and could surprise authors.

✅ What Looks Good

• Fail-closed by default throughout: bare require-approval: true → on_timeout: None → all_resume = false → on_timeout: Reject. Only overridable by every contributing tool explicitly setting on-timeout: resume.
• validate_require_approval eagerly re-deserialises every require-approval value: catches the silent-bypass bug (.ok()-swallowed typos like on-timeout: rejec) before compilation, not silently dropping the gate.
• filter_flags safety argument is well-founded: tool names are restricted to ASCII alphanumeric + hyphens by is_safe_tool_name, called before reaching this function, and the validation tests cover injection patterns.
• Pool::Server lowering: emits the scalar pool: server correctly; the VmImage/Named mapping construction is now correctly scoped inside each branch — the old code created a Mapping unconditionally and only populated it inside the match arm, which is now fixed as a side-effect of this PR.
• 1ES path correctly skips templateContext for server jobs: a ManualValidation@1 task in a templateContext wrapper would silently suppress the server pool, so the Pool::Server equality guard in onees_ir.rs is the right fix.
• Graph wiring is correct for all three split cases (none / mixed / all-reviewed): the has_review && !has_reviewed_job condition in wire_explicit_dependencies produces the right dependsOn edges.
• Tests cover the important cases: global gate, no-gate (backward compat), per-tool override, mixed split, teardown skips reviewed dependency, 1ES server pool, object propagation, and audit aggregation.

Generated by Rust PR Reviewer for issue #1196 · 393.7 AIC · ⌖ 12.3 AIC · ⊞ 34.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Nearly production-ready — one correctness bug with on-timeout: resume + timeout-minutes worth fixing before merge; everything else looks solid.

Findings

🐛 Bugs / Logic Issues

• src/compile/agentic_pipeline.rs / src/compile/ir/tasks/manual_validation.rs — timeout-minutes only sets the agentless job-level timeout (job.timeout), but ManualValidation@1's onTimeout handler fires when the task times out (via its own timeout input), not when the job times out. A job-level timeout cancels the job; the task never gets a chance to apply onTimeout: resume.
  For on-timeout: reject this is harmless (cancellation ≈ rejection). For on-timeout: resume, a timed-out run will cancel/fail instead of auto-approving — directly contradicting the documented on-timeout: resume behaviour.
  Fix: add a timeout(u32) setter to ManualValidation (emitting the timeout input in minutes), and call it in build_manual_validation_step alongside the existing job.timeout assignment. The job timeout can remain as the outer hard bound; the task timeout triggers the graceful onTimeout path.

✅ What Looks Good

• Security posture is strong: validate_require_approval() is called before any partition/access in build_pipeline_context, so the .ok()-swallowed-error paths in global_require_approval/tool_require_approval can never silently drop a gate and let a reviewed tool slip through as automatic. The explicit regression test for the on-timeout: rejec typo is a nice addition.
• Shell-injection surface is well-contained: filter_flags embeds tool names unquoted into the ado-aw execute command line, but validate_safe_outputs_keys runs is_safe_tool_name (ASCII alphanumeric/hyphen only) before any partition happens, so there's nothing exploitable there. Same guarantee covers the alternation in detect_reviewed_proposals_step's grep pattern.
• ADO template-expression injection blocked: validate_require_approval correctly rejects ${{ ... }} in approvers, notify-users, and instructions at compile time.
• Fail-closed defaults everywhere: aggregate_approval_config's all_resume = true logic correctly treats an absent on-timeout as non-resume (the wildcard _ => all_resume = false arm), and the assert!(!reviewed.is_empty()) guards the degenerate empty-slice case.
• Backward-compatible: the feature is entirely inert when no require-approval is set; the golden lock files remaining unchanged confirms this.
• Test coverage is thorough: global/per-tool resolution, mixed split, teardown dependency, object propagation, audit aggregation, and the silent-error regression case are all covered.

Generated by Rust PR Reviewer for issue #1196 · 382.1 AIC · ⌖ 12.4 AIC · ⊞ 34.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured feature with thoughtful security design. One logic clarity issue worth noting, but nothing blocking.

---

Findings

⚠️ Suggestions

• agentic_pipeline.rs:627 — aggregate_approval_config silently takes the first non-empty instructions from the sorted tool list when multiple tools each carry their own instructions. The reviewer is shown only one tool's message while multiple approval-gated tools are pending. The code comment says "Documented in docs/safe-outputs.md", but authors who set per-tool instructions will likely be surprised that only one is shown. Consider a brief doc-comment on the function (or on ApprovalConfig::instructions) spelling out "first tool in alphabetical order wins" so the invariant is co-located with the implementation.

• agentic_pipeline.rs:481 — filter_flags builds --exclude t1 --exclude t2 by appending one flag-value pair at a time. This is correct, but the existing comment ("leading space so it concatenates onto the fixed command") is easy to miss when reading the call-sites. The shell-safety rationale is well-documented, though it mirrors safety arguments already present in the surrounding code.

• agentic_pipeline.rs:770 — detect_reviewed_proposals_step does find "{working_directory}/safe_outputs" -name "safe_outputs.ndjson" 2>/dev/null | head -n 1. If working_directory were to contain a double-quote character this would break the bash quoting, but that's a pre-existing pattern in execute_safe_outputs_step (same style for source_path) rather than a new concern here.

✅ What Looks Good

• Fail-closed design throughout: on-timeout defaults to Reject, assert! in aggregate_approval_config enforces the non-empty-slice invariant (a security boundary, worth the always-on assert), and ManualReview's condition requires Succeeded && SafeToProcess == true && HasReviewedProposals == true so a flagged or failed Detection run can never open the gate.

• validate_require_approval() as an eager guard: The .ok() paths in global_require_approval/tool_require_approval could silently drop a malformed config (e.g. on-timeout: rejec) and remove the tool from the reviewed list without warning. The eager validation step called in build_pipeline_context catches this class of "silent fail-open" regression before compilation proceeds. The regression test (test_validate_require_approval_rejects_bad_on_timeout) is exactly right.

• Template-expression injection prevention: Blocking ${{ }} in approvers/notify_users while explicitly allowing $(...) runtime macros in instructions is the correct distinction for this ADO context, and the dedicated contains_ado_template_expression function keeps it narrow and auditable.

• Pool::Server lowering: Emitting the scalar pool: server (not a mapping) is the correct ADO YAML syntax for agentless/server jobs. The 1ES templateContext skip for Pool::Server is correct and well-tested.

• Test coverage: The unit tests in types.rs, the IR lowering test for pool: server, and the integration tests (test_mixed_approval_splits_execution_jobs, test_require_approval_timeout_bounds_task_not_just_job, the teardown dependency test, the audit aggregation test) cover the critical paths comprehensively.

Generated by Rust PR Reviewer for issue #1196 · 247.3 AIC · ⌖ 12.2 AIC · ⊞ 34.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the security-critical paths are well-guarded. Two minor issues worth addressing before merge.

Findings

⚠️ Suggestions

• src/compile/agentic_pipeline.rs — aggregate_approval_config uses assert! with a misleading comment

  assert!(
      !reviewed.is_empty(),
      "aggregate_approval_config called with no reviewed tools (would default to fail-open resume)"
  );

  The inline comment calls this "a release-build assert!", which reads like a debug_assert! (no-op in release). Rust's assert! always fires — debug_assert! is the release-stripped variant. The intent (keep the check in release for the security invariant) is correct; just the comment phrasing is wrong. Recommend updating the comment, or — more consistently with the project's anyhow::Result convention throughout — converting the function to return Result<ApprovalConfig> and replacing assert! with anyhow::ensure!. Not a bug in practice, but the comment currently says the opposite of what Rust does.

• src/compile/agentic_pipeline.rs — aggregate_approval_config timeout_minutes aggregation strategy is undocumented

  timeout_minutes = Some(timeout_minutes.map_or(t, |existing| existing.min(t)));

  When multiple reviewed tools configure different timeout-minutes values (e.g., one tool at 60, another at 120), the gate uses the minimum — the strictest bound governs. This is a reasonable conservative choice, but docs/safe-outputs.md doesn't mention the aggregation strategy and neither does the function's docstring. A user who configures a 60-minute timeout on a low-impact tool and 120 minutes on a high-impact one will find both governed by the 60-minute bound. Worth a one-liner in the timeout-minutes field docs.

✅ What Looks Good

• Security guard on validate_require_approval: The upfront eager validation (called from build_pipeline_context) catches every malformed require-approval value — bad on-timeout, unknown fields, bad types — so the downstream .ok()-silent paths in global_require_approval/tool_require_approval can never silently disable a gate the author intended. The regression test test_validate_require_approval_rejects_bad_on_timeout directly exercises the rejec typo scenario. Well done.

• ${{ ... }} template-expression rejection: validate_require_approval rejects ADO template expressions in approvers, notify-users, and instructions while deliberately allowing $(...) runtime macros in instructions. The split between contains_ado_template_expression (narrow) and contains_ado_expression (broad) is clean and the doc explains the rationale.

• filter_flags injection safety: Tool names in --exclude/--only are spliced into bash without per-name quoting. The comment correctly points to validate::is_safe_tool_name via validate_safe_outputs_keys, run in build_pipeline_context before build_canonical_jobs reaches this function. The invariant is real.

• detect_reviewed_proposals_step matching strategy: Using jq -r 'select(type=="object") | .name // empty' to extract only top-level name values prevents nested "name" keys in tool params from false-positiving. The grep fallback is intentionally over-match (pauses unnecessarily) rather than under-match (misses a required review). Correct failure mode.

• Pool::Server 1ES skip: The onees_ir.rs guard (if job.pool == Pool::Server { continue; }) correctly avoids wrapping the agentless ManualReview job in a templateContext block that would suppress pool: server.

• aggregate_approval_config fail-closed default: all_resume starts true and flips to false on any tool that is missing, absent an explicit Resume, or has None timeout. The only way the gate auto-approves on timeout is if every reviewed tool explicitly says on-timeout: resume. Conservative and correct.

• Test coverage: Global/per-tool override, absent approval, detailed-object propagation, mixed auto+reviewed split, teardown skipping the reviewed job, multi-tool instruction aggregation, timeout-bounds-task-not-job, audit across split artifacts, Pool::Server scalar lowering, ToolFilter::allows — comprehensive.

Generated by Rust PR Reviewer for issue #1196 · 415.3 AIC · ⌖ 12.6 AIC · ⊞ 34.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the implementation is correct and the security model is sound. One API design note worth tracking.

Findings

⚠️ Suggestions

• src/compile/types.rs — tool_requires_approval() implicitly depends on prior validate_require_approval() call

  global_require_approval() and the private tool_require_approval() both use .ok() to silently discard parse errors. A misconfigured on-timeout (or any unknown field — ApprovalConfig uses deny_unknown_fields) would return None, making the tool silently fall out of the reviewed list and skip the intended approval gate.

  The doc comment on validate_require_approval() correctly identifies this coupling, and build_pipeline_context always calls it before the .ok() paths matter. But since tool_requires_approval() is pub, a future caller outside the compilation pipeline could get silently incorrect results (no gate where one was intended).

  A safer API would propagate errors — e.g. have tool_requires_approval() return Result<Option<ApprovalConfig>> — but that's a larger refactor. At minimum, a # Panics / # Safety note on tool_requires_approval() reminding callers that validate_require_approval() must be called first would make this invariant visible at the call site.

✅ What Looks Good

• Fail-closed by design throughout: aggregate_approval_config uses assert! (not debug_assert!) to guard the empty-reviewed-list case, ensuring the invariant holds in release builds. The on-timeout aggregation defaults to Reject unless every contributing config explicitly opts in to Resume. Both are exactly right for a security gate.

• Shell injection prevention: Tool names embedded into the detect_reviewed_proposals_step regex alternation (reviewed.join("|")) are compiler-controlled identifiers constrained to [a-zA-Z0-9-] by validate::is_safe_tool_name, which is enforced before any bash generation. The comment in filter_flags correctly documents this invariant.

• ADO template expression blocking in validate_require_approval: ${{...}} is correctly rejected in approvers, notify_users, and instructions. The intentional carve-out for $(...) runtime macros (useful for things like approvers: "$(Build.RequestedFor)") is well-motivated and author-controlled, not agent-controlled.

• 1ES Pool::Server handling: The onees_ir.rs guard (if job.pool == Pool::Server { continue; }) correctly prevents the ManualReview agentless job from being wrapped in templateContext:, which would break the server pool emission. Good defensive check.

• SafeOutputsVariant split logic: All three cases are correctly handled — no reviewed tools (unchanged), all reviewed (single job gated by ManualReview), and mixed (auto job + reviewed job running in parallel from Detection, with the reviewed job gated). The wire_explicit_dependencies wiring matches.

• TypeScript rendering security: sanitizeInline and sanitizeBlock are thorough. The backtick-run substitution with U+02BC for fence escaping is the right approach — alternatives that keep real backticks (e.g. zero-width-space separation) could be stripped by a renderer before fence-scanning. The security tests in render.test.ts explicitly cover hostile tool names (backtick/newline injection) and table-cell/HTML-tag/fence escapes in field values.

Generated by Rust PR Reviewer for issue #1196 · 882.5 AIC · ⌖ 12.4 AIC · ⊞ 34.7K · ◷