feat(compile): render proposed safe outputs to a build summary tab

[jamesadevine]

Summary

Adds 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. When manual review is configured (#1196), the pending-approval proposals are listed first and the default review message points approvers at the tab.

Stacked on #1196 (feat/safe-output-manual-review) — this PR targets that branch as its base so the diff is only the summary-tab work. Retarget to main once #1196 merges.

How it works

• 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-before-automatic, and attaches it via ##vso[task.uploadsummary]. Consistent with gate.js / import.js / exec-context-*.js (ncc-bundled, shipped in ado-script.zip).
• Rendered at the END of the Agent job (after collect_safe_outputs_step) — deliberately not in the Detection/threat-analysis stage, whose only job is inspecting proposals for threats. Best-effort: a render failure is downgraded to a warning and never fails the build or blocks the review gate.
• Sanitization (security): all agent-generated content is escaped for markdown display (markdown/HTML metacharacters escaped, code fences neutralised, control chars stripped, long values truncated) so a proposal cannot forge UI (e.g. a fake "✅ approved" banner) or break the table layout.
• Coexistence with existing uploadsummary tabs: ADO derives a summary section's title from the uploaded file's base name and does not de-duplicate, so this uses a namespaced base name (ado-aw-safe-outputs.md → the ado-aw-safe-outputs section). It is additive and build-scoped — one extra section alongside any consumer/template-target tabs, never colliding (works under target: job / target: stage too).

Implementation notes

• Bundle delivered to the Agent job via a new safe_outputs_summary_active flag on the ado-script extension, reusing the existing guarded install/download (no double-download).
• New uploadSummary() helper in shared/vso-logger.ts; the reviewed-tool list is passed via env (never spliced into a shell command, so no tool name reaches a shell word-split).
• default_review_instructions now points approvers at the tab.
• Release-zip packaging, package.json build/clean/test:smoke chains, and .gitignore updated for the new bundle.
• Docs: docs/safe-outputs.md (tab + coexistence), docs/ado-script.md, AGENTS.md.

Test plan

• cargo clippy --all-targets — clean.
• Full Rust suite green with ENFORCE_BASH_LINT=1, including 3 new integration tests asserting the render step lands in the Agent job (not Detection) for the review / plain-safe-outputs / no-safe-outputs cases, plus bash-lint coverage of the new step.
• tsc --noEmit clean (pre-existing picomatch types issue aside); 407 vitest pass incl. 22 new (NDJSON parsing, pending-first grouping/ordering, per-tool detail, generic fallback, hostile-content sanitization, env handling).
• End-to-end: compiled a manual-review fixture and ran the built bundle — verified grouping, fenced bodies, and HTML escaping (<b> → \<b\>).

⚠️ Runtime-only follow-up (not verifiable in CI): confirm task.uploadsummary renders a tab on Microsoft-hosted / OneBranch / 1ES runners. It is a standard ADO logging command supported on all agent types (low risk); artifact-link fallback noted if a runner doesn't render the tab.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[github-actions]

🔍 Rust PR Review

Summary: looks good overall — well-structured feature with solid security fundamentals; a few minor issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/agentic_pipeline.rs:871-880 — partition_safe_outputs_by_approval() is called unconditionally before the if front_matter.safe_output_tool_names().next().is_some() guard that consumes the result. When no safe-output tools are configured, this allocates two Vec<String> that are immediately discarded. Minor, but inconsistent with the guard pattern used everywhere else. Move the destructure inside the if block:

  if front_matter.safe_output_tool_names().next().is_some() {
      let (_, reviewed_summary_tools) = front_matter.partition_safe_outputs_by_approval();
      steps.push(Step::Bash(safe_outputs_summary_step(&reviewed_summary_tools)));
  }

• tests/compiler_tests.rs:6052-6065 (job_block helper) — The search pattern format!("- job: {job}") is a plain substring match, so job_block(compiled, "Agent") would silently match a hypothetical - job: Agent_Reviewed job before - job: Agent. Currently harmless given the pipeline shape, but fragile. A minimal fix — anchor the search with a trailing newline/space: format!("- job: {job}\n").

⚠️ Suggestions

• scripts/ado-script/src/approval-summary/render.ts (TOOL_SPECS) — noop, missing-data, missing-tool, and report-incomplete are absent from TOOL_SPECS. They fall through to genericFields, which surfaces all scalar fields as inline cells (capped at 300 chars). For noop the message field is the useful content — and can be a multi-line, multi-sentence string — so it deserves a fenced-body spec (body: "message") rather than a 300-char inline truncation. The other three terminal signals similarly have reason / details fields that warrant fenced-body treatment. Absence doesn't cause a functional bug but produces a noticeably degraded reviewer experience for the most common no-side-effect outcome.

✅ What Looks Good

• Security fundamentals are solid: the reviewed tool list is passed exclusively through the typed env block (AW_REVIEWED_TOOLS) and never spliced into the shell command — no shell word-split exposure. The node '...' invocation uses a compile-time constant path so the command is static.
• Code-fence neutralization (sanitizeBlock: replace ``` with ʼʼʼ U+02BC) correctly prevents breakout without discarding the body content, and the hostile-content test verifies both inline (table injection) and block (fence breakout) vectors.
• Best-effort failure policy (Condition::Always + always-exit-0 in the bundle) ensures rendering can never block the build or gate; the ##vso[task.logissue type=warning] fallback in the bash wrapper gives observability without build impact.
• Namespaced output path (ado-aw-safe-outputs.md) preventing ADO summary-tab title collision is the right call; the doc update explains the coexistence contract clearly.
• Integration tests in compiler_tests.rs correctly assert step placement (Agent vs Detection job), reviewed-tool env-var content, and the no-safe-outputs absence case — exactly the right things to pin.

Generated by Rust PR Reviewer for issue #1235 · 248.2 AIC · ⌖ 12.6 AIC · ⊞ 36.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured feature with solid security thinking. A couple of minor issues worth addressing.

Findings

🐛 Bugs / Logic Issues

• render.ts:renderProposal — safeName incomplete sanitization. The tool name placed in the heading's code span only has backticks stripped (p.name.replace(/\/g, "")). The comment claims this is a "compiler-validated safe identifier", which is true for tools from the legitimate safe-outputs MCP server. However, parseProposalsaccepts *any* non-empty stringname— it doesn't validate for[a-z0-9-]. A crafted NDJSON with "name": "foo\nbar"would produce a broken heading:#### Title `foo\nbar`. Since the PR explicitly states all agent-generated content is escaped, a newline/control-char strip on safeNamewould close this gap:p.name.replace(/[`\n\r\u0000-\u001f]/g, "")`.

• agentic_pipeline.rs:879-881 — partition_safe_outputs_by_approval() called unconditionally. The partition is computed before the guard check, allocating two Vec<String> even when there are no safe output tools configured (where safe_output_tool_names().next().is_none()). Not incorrect, but the partition is only needed inside the if block:

  // Before
  let (_, reviewed_summary_tools) = front_matter.partition_safe_outputs_by_approval();
  if front_matter.safe_output_tool_names().next().is_some() {
      steps.push(Step::Bash(safe_outputs_summary_step(&reviewed_summary_tools)));
  }
  
  // After
  if front_matter.safe_output_tool_names().next().is_some() {
      let (_, reviewed_summary_tools) = front_matter.partition_safe_outputs_by_approval();
      steps.push(Step::Bash(safe_outputs_summary_step(&reviewed_summary_tools)));
  }

⚠️ Suggestions

• render.ts:sanitizeInline — & not escaped. HTML entities like <script> in an inline value survive to the output (as the literal string <script>). Since </> are already escaped, there's no injection path, but it means & renders as & in ADO's markdown, which is cosmetically wrong. Adding s = s.replace(/&/g, "\\&") (or %26 encoding) would be belt-and-suspenders.

• TOOL_SPECS missing entries for missing_data, missing_tool, noop, report_incomplete. These fall back to the generic scalar renderer, which works. But report_incomplete carries a reason field that's highly informative for a reviewer — a tailored spec would surface it prominently rather than leaving it in the generic table. Not blocking, but worth a follow-up.

✅ What Looks Good

• Security approach is sound: all agent-generated values flow through env vars (never shell word-split), and the sanitization pipeline (sanitizeInline / sanitizeBlock) is comprehensive for markdown injection, table-cell breaking, and code-fence escape.
• Code-fence neutralization with U+02BC is clean and correct.
• Best-effort failure policy (|| echo "##vso[task.logissue type=warning]...") is the right call for a transparency step.
• Step placement tests asserting the render step is in Agent and not in Detection are exactly the right tests to write for this feature.
• safe_outputs_summary_active flag in AdoScriptExtension and the condition in build_agent_job are in sync — the bundle is downloaded iff the step is emitted, with no possible mismatch.
• 22 new TypeScript tests, including hostile-content cases, give good coverage of the sanitization logic.

Generated by Rust PR Reviewer for issue #1235 · 291.1 AIC · ⌖ 20.3 AIC · ⊞ 36.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-considered threat model, strong test coverage, and clean design. A couple of findings worth addressing before merge.

Findings

⚠️ Suggestions

• src/compile/agentic_pipeline.rs ~line 1062 / src/compile/extensions/mod.rs ~line 1231 — The condition front_matter.safe_output_tool_names().next().is_some() is evaluated independently in two places: collect_extensions (sets safe_outputs_summary_active, which gates the bundle download) and build_agent_job (gates step emission). They're identical today but if they drift the step will try to run a bundle that was never downloaded, causing a silent build failure. The PR description acknowledges the flag-based approach, but there's no assertion or comment tying the two together. Consider either reading safe_outputs_summary_active off the already-resolved extension struct in build_agent_job, or adding a doc comment noting the invariant that they must agree.

• scripts/ado-script/src/approval-summary/render.ts ~line 855 — sanitizeInline escapes < and > as CommonMark punctuation escapes (\<, \>) rather than HTML entities (<, >). In a strict CommonMark renderer \< produces a literal < which is then entity-encoded by the HTML serialiser — safe. But ADO's build-summary renderer isn't documented as CommonMark-compliant, and the test at line 462 only asserts the output markdown string (\\<script\\>), not the final rendered HTML. If ADO emits \<script\> without further encoding, agent content could forge UI elements visible to the reviewer. Severity is low (authenticated context, ADO almost certainly strips scripts from summary tabs), but using </> would make this renderer-agnostic. Note: the & encoding step would need to come after substituting </> to avoid double-encoding the & in the entities.

• tests/compiler_tests.rs ~line 1270 — job_block does rest[1..] for a raw byte-slice offset. It's safe today since compiled YAML is ASCII, but this would panic if rest starts on a multi-byte UTF-8 boundary. A more robust alternative: rest.splitn(2, '\n').nth(1).unwrap_or_default() to walk past the first line without assuming byte width.

✅ What Looks Good

• Env-based parameter passing for AW_REVIEWED_TOOLS: tool names are never spliced into the shell command — they go through the typed env block only. The comment in safe_outputs_summary_step makes this explicit.
• sanitizeBlock fence neutralisation: replacing ``` with U+02BC modifier apostrophes (ʼʼʼ) is an elegant approach that prevents body content from escaping the fenced block while keeping the text visually intact.
• safeName in renderProposal: stripping backticks and control characters defensively before placing the name in a heading code span correctly prevents a crafted name from splitting the span across lines. The security test covering this case is thorough.
• Namespaced output path (ado-aw-safe-outputs.md): prevents ADO's auto-derived tab title from colliding with consumer-owned tabs. The doc comment in uploadSummary explicitly calls this out.
• Best-effort failure model: the || echo "##vso[task.logissue type=warning]..." shell fallback ensures a broken bundle never fails the build or blocks the review gate.
• Test coverage: 22 vitest unit tests covering NDJSON parsing, grouping/ordering, per-tool rendering, generic fallback, hostile-content sanitization, and env handling; 3 Rust integration tests asserting the step lands in the Agent job (not Detection) for all three pipeline variants; bash-lint coverage for the new step. This is the right level of rigor for a security-sensitive rendering path.

Generated by Rust PR Reviewer for issue #1235 · 375.4 AIC · ⌖ 12.7 AIC · ⊞ 34.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-designed feature with solid sanitization. Two minor issues worth addressing.

Findings

⚠️ Suggestions

• src/compile/agentic_pipeline.rs:2271 — AW_REVIEWED_TOOLS delimiter not guaranteed unambiguous
  reviewed.join(",") encodes tool names as a comma-separated list, and parseReviewed in index.ts splits on ,. The safe_outputs field is a HashMap<String, serde_json::Value> whose keys are unrestricted YAML strings — a user who writes a key like "create-pr,add-comment" with require-approval: true would have it silently misrouted to the "Automatic" section instead of "Pending approval". This doesn't affect any built-in tools (all are [a-z][a-z0-9-]+), but there's no compiler enforcement of that invariant for the map keys. Consider using \n as a delimiter (impossible in a YAML map key), or adding a validation pass on safe_output_tool_names() to reject keys containing , — similar to the existing validate_require_approval() pass.

• scripts/ado-script/src/approval-summary/render.ts:299 — code-fence replacement character may surprise reviewers
  sanitizeBlock replaces ``` with three \u02bc MODIFIER LETTER APOSTROPHE characters. These are visually close to backticks in many fonts, so a human reviewer comparing the summary tab to the raw proposal content might not notice the substitution. A more legible replacement (e.g. the zero-width-apostrophe replacement is correct for breakout prevention, but consider \u0060 (escaped) or annotating with a comment like [fences neutralized]) would make the intent clearer. This is cosmetic — the breakout test passes correctly.

✅ What Looks Good

• Env-based tool name passing (AW_REVIEWED_TOOLS via with_env, never spliced into the shell command) is the right call — tool names never reach a shell word-split.
• has_any_safe_output_tool() as single source of truth — the doc comment explicitly calls out the coupling between collect_extensions and build_agent_job, which prevents the step-emitted/bundle-downloaded drift described. Well done.
• Sanitization chain (sanitizeInline + sanitizeBlock) is thorough: HTML entity encoding before backslash escaping, correct & → & first to prevent entity re-encoding, and the security test suite covers the hostile-content cases (table-cell injection, fake headings, HTML tags).
• Best-effort failure handling — the || echo "##vso[task.logissue type=warning]..." fallback and the Condition::Always on the step are both necessary and correct; the summary can never block the build or manual-review gate.
• Step placement — emitted after collect_safe_outputs_step (so safe_outputs.ndjson exists) but before the artifact publish; integration tests assert it lands in Agent and not Detection.
• Three integration tests in compiler_tests.rs are well-structured and cover the non-trivial cases (review pipeline, plain pipeline, no safe-outputs).

Generated by Rust PR Reviewer for issue #1235 · 362 AIC · ⌖ 12.8 AIC · ⊞ 34.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-designed feature with solid sanitization and good test coverage. One documentation inconsistency in the TypeScript I/O contract, and a minor observation on the bash step.

Findings

🐛 Bugs / Logic Issues

• scripts/ado-script/src/approval-summary/index.ts (module-level JSDoc, line 26): The I/O contract comment says AW_REVIEWED_TOOLS is "comma-separated reviewed tool names", but parseReviewed splits on "\n" and the Rust side emits reviewed.join("\n"). The test confirms the actual behaviour is newline-delimited (parseReviewed(" create-pull-request \n \n add-pr-comment ")). The comment should read "newline-separated" to match both the code and the rationale explained in the Rust doc comment (a comma can legally appear in a YAML map key).

⚠️ Suggestions

• src/compile/agentic_pipeline.rs (safe_outputs_summary_step, ~line 2256): The bash step does not start with set -euo pipefail. This is consistent with the existing collect_safe_outputs_step pattern (also no set -e) and the || fallback means the step exits 0 on failure regardless, so this is not a correctness bug. Worth being aware of if shellcheck ever flags it with SC2155 or similar — but given ENFORCE_BASH_LINT=1 is clean, this is purely informational.

✅ What Looks Good

• Single source of truth for the gate predicate: has_any_safe_output_tool() is introduced as the canonical SSOT used by both collect_extensions (to arm the bundle download) and build_agent_job (to emit the step), with a doc comment explicitly warning that drift between these two sites would cause a "bundle not found" failure. Good defensive documentation.
• Env-var I/O for tool names: Tool names are passed via with_env(...) rather than interpolated into the shell command — the right call for a security-sensitive boundary.
• Sanitization ordering: sanitizeInline encodes & before </> (correct order to prevent double-encoding), and | is in the escape set (critical for table-cell injection). The sanitizeBlock backtick-fence neutralisation with U+02BC is sound — the regex targets exactly 3-char sequences matching the opening fence length, so no 3-backtick run can survive to close the block.
• Best-effort design: Condition::Always + || echo "##vso[task.logissue...]" ensures the summary step never fails the build or blocks the review gate.
• Test placement: The Rust integration tests asserting the render step lands in the Agent block (not Detection) are well-structured and use the new job_block helper cleanly.
• Security tests: The hostile-content test in render.test.ts covers pipe injection, HTML injection, and fence breakout in a single realistic payload.

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

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-reasoned security model, strong sanitization, and clean integration. A few low-severity notes below.

---

Findings

⚠️ Suggestions

• src/compile/agentic_pipeline.rs:2269 — Missing ; in the fallback ##vso[task.logissue] command

  The Rust-emitted bash fallback echo uses type=warning] (no trailing semicolon):

  echo "##vso[task.logissue type=warning]approval-summary step failed (non-fatal)"

  But vso-logger.ts consistently emits type=warning;]. Both are accepted by the ADO parser, but aligning them avoids a subtle inconsistency that could confuse future readers of generated YAML.

• tests/compiler_tests.rs — No test for multiple reviewed tools (multiline AW_REVIEWED_TOOLS)

  test_safe_outputs_summary_step_emitted_for_review_pipeline verifies the single-reviewed-tool case (AW_REVIEWED_TOOLS: create-pull-request). When two or more tools require approval, reviewed.join("\n") produces a newline-embedded string that serde_yaml will serialize as a block scalar — a different YAML shape that the test doesn't cover. While parseReviewed handles both forms correctly, a test like:

  // fixture: both create-pull-request and create-work-item have require-approval: true
  assert!(compiled.contains("create-pull-request") && compiled.contains("create-work-item"),
      "both reviewed tools must appear in AW_REVIEWED_TOOLS");

  would guard against regressions in the YAML lowering of multiline env values.

• scripts/ado-script/src/approval-summary/render.ts — missing-data TOOL_SPEC silently drops the context field

  MissingDataResult (Rust) serializes three fields: data_type, reason, and optionally context. The TS spec surfaces data_type inline and reason as the body, but context is never rendered. For diagnostic tools where the agent often puts actionable detail in context, reviewers may miss that information. Either add context as a secondary body/field, or add a brief code comment explaining the omission is intentional.

---

✅ What Looks Good

• HTML-entity encoding order is correct (& → & before </>) in sanitizeInline — a subtle ordering dependency that's easy to get wrong and would let a pre-existing < in agent content render as <.
• Code-fence breakout prevention: the U+02BC substitution approach is a good choice for this security-sensitive path. Because any 3+-backtick run becomes U+02BC×3, at most 2 real consecutive backticks can survive sanitization, making a 3-backtick closing fence formation impossible.
• AW_REVIEWED_TOOLS delimiter: newline instead of comma is the right call — the doc comment in safe_outputs_summary_step explaining why (commas are valid YAML map-key characters) is a useful record of the reasoning.
• Tool names through env, not shell: passing the reviewed list via the typed .with_env() block rather than splicing into the format!() shell command string correctly ensures is_safe_tool_name-validated names never reach a shell word-split.
• Single source of truth for the feature gate (FrontMatter::has_any_safe_output_tool): the doc comment explicitly calls out the drift risk and the collect_extensions/build_agent_job sites both route through it — good.
• Best-effort design: the || echo "##vso[task.logissue ...]" fallback and the Condition::Always on the step mean a render failure can never block the review gate. This is exactly the right failure policy for a transparency feature.

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