feat(safeoutputs): add work-item filing to noop and missing-tool safe outputs

[Copilot]

Adds work-item filing behaviour to noop and missing-tool safe outputs. The Stage 3 executor always files an Azure DevOps work item or appends a comment to an existing one when these diagnostic tools are invoked. All fields have sensible defaults so no front-matter configuration is required.

Summary

• src/safeoutputs/mod.rs — shared infrastructure:

  • WorkItemReportConfig: configures title, work-item-type, area/iteration paths, tags, and stats inclusion
  • wiql_find_work_item_by_title(): WIQL search for a non-closed item matching the configured title
  • file_or_append_work_item(): comment on existing item if found; create new one otherwise. Returns a warning (not a failure) when ADO credentials are absent or the WIQL call fails, so these always-on tools never block the pipeline

• src/safeoutputs/noop.rs — NoopConfig with a work_item: WorkItemReportConfig field that defaults to a Task titled "Agent reported no operation"; executor always calls the shared helper

• src/safeoutputs/missing_tool.rs — MissingToolConfig with a work_item: WorkItemReportConfig field that defaults to a Task titled "Agent encountered missing tool"; executor always calls the shared helper

• docs/safe-outputs.md — documents the always-on behaviour and all override options

Both tools work with zero configuration. Override any field in front matter to customise the work item:

safe-outputs:
  noop:
    work-item:
      title: "Agent reported no operation"   # default
      work-item-type: Task                   # default
      area-path: "MyProject\\MyTeam"
      tags:
        - agent-noop

  missing-tool:
    work-item:
      title: "Agent encountered missing tool" # default
      work-item-type: Task                    # default
      area-path: "MyProject\\MyTeam"
      tags:
        - agent-missing-tool

Stage 3 runs a WIQL query to find any non-closed item with the configured title. If found, a comment is appended; otherwise a new work item is created. If ADO credentials are not available the executor succeeds with a warning instead of failing hard.

Test plan

cargo test — all existing and new unit tests pass. New tests cover config deserialization, default field values, and the graceful-warning fallback when ADO credentials are not present.

[jamesadevine]

@copilot implement sensible defaults for the workitem. These safeoutputs always should now create or append to a work item.

[Copilot]

@copilot implement sensible defaults for the workitem. These safeoutputs always should now create or append to a work item.

Done in d0257b8. Both tools now always file or append to a work item with no config required:

• noop → defaults to a Task titled "Agent reported no operation"
• missing-tool → defaults to a Task titled "Agent encountered missing tool"

The work-item block in front matter lets you override any field (title, type, area/iteration path, tags). Since these are always-on diagnostic tools, if ADO credentials aren't available or the WIQL search fails, the executor returns a warning (success=true) rather than failing the pipeline hard.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• spsprodeus21.vssps.visualstudio.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-3136f96127ec4700 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-3136f96127ec4700 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.1r/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.29d554253a7ua8jdy2djiuujt.10kbr5c.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.1w/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.2bikq83kqxzd59u4d0aoev3vs.10kbr5c.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.22/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.2boqvweqww93ilwsx0c22c3r0.10kbr5c.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.2o/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.2d2vuk299nc534pripnny1rfr.10kbr5c.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.2u/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.2hkf7twmmm5z0ktz2vjsee9ue.10kbr5c.rcgu.o f.006607excnbrprqp637f7y53i.0ywnp6d.rcgu.o f.04ihlpyw73h878m5ii3u14z4w.0ywnp6d.rcgu.o f.05s581wfkj8h57yyfztyfopmr.0ywnp6d.rcgu.o f.0a5uivvqg0yudio4gs881kurk.0ywnp6d.rcgu.o f.0avko3xidh2wk8781opolbaij.0ywnp6d.rcgu.o f.0g5xkpur9po42a3k6jocdfert.0ywnp6d.rcgu.o f.0gsn8ev1cnkk8096ejd9eaqnc.0ywnp6d.rcgu.o f.0hnyc0p3974v12h5g8kkwz19v.0ywnp6d.rcgu.o f.0xonaogqri1qh024baqfa7x2d.0ywnp6d.rcgu.o f.12nfqzyx5t92gdliu6u9j2t23.0ywnp6d.rcgu.o f.1bcwhu0k0xfbj80ouktlgq7gg.0ywnp6d.rcgu.o f.1bhjf3uobqgssktgi0e14aqnn.0ywnp6d.rcgu.o f.1hqz8k3c94266bfgrfbuybt2z.0ywnp6d.rcgu.o f.1huk6ich8t6qwyn0rf9m9q3l0.0ywnp6d.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-3136f96127ec4700 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-3136f96127ec4700 f.0a5uivvqg0yudio4gs881kurk.0ywnp6d.rcgu.o f.0avko3xidh2wk8781opolbaij.0ywnp6d.rcgu.o f.0g5xkpur9po42a3k6jocdfert.0ywnp6d.rcgu.o f.0gsn8ev1cnkk8096ejd9eaqnc.0ywnp6d.rcgu.o f.0hnyc0p3974v12h5g8kkwz19v.0ywnp6d.rcgu.o f.0xonaogqri1qh024baqfa7x2d.0ywnp6d.rcgu.o f.12nfqzyx5t92gdliu6u9j2t23.0ywnp6d.rcgu.o f.1bcwhu0k0xfbj80ouktlgq7gg.0ywnp6d.rcgu.o f.1bhjf3uobqgssktgi0e14aqnn.0ywnp6d.rcgu.o f.1hqz8k3c94266bfgrfbuybt2z.0ywnp6d.rcgu.o f.1huk6ich8t6qwyn0rf9m9q3l0.0ywnp6d.rcgu.o f.1t�� f.270d89qd8zd7pddtw479jvwb6.0ywnp6d.rcgu.o f.2i0rx46105zwgayhinkt9jur2.0ywnp6d.rcgu.o mbols.o d12dc65cce50cb.agit d12dc65cce50cb.aremote d12dc65cce50cb.aget-url d12dc65cce50cb.aorigin (dns block)
• spsprodweu4.vssps.visualstudio.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-3136f96127ec4700 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-3136f96127ec4700 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.1r/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.29d554253a7ua8jdy2djiuujt.10kbr5c.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.1w/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.2bikq83kqxzd59u4d0aoev3vs.10kbr5c.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.22/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.2boqvweqww93ilwsx0c22c3r0.10kbr5c.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.2o/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.2d2vuk299nc534pripnny1rfr.10kbr5c.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-6da1eb8e557c50ba.2u/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/bash_lint_tests-519976ce551c8145.2hkf7twmmm5z0ktz2vjsee9ue.10kbr5c.rcgu.o f.006607excnbrprqp637f7y53i.0ywnp6d.rcgu.o f.04ihlpyw73h878m5ii3u14z4w.0ywnp6d.rcgu.o f.05s581wfkj8h57yyfztyfopmr.0ywnp6d.rcgu.o f.0a5uivvqg0yudio4gs881kurk.0ywnp6d.rcgu.o f.0avko3xidh2wk8781opolbaij.0ywnp6d.rcgu.o f.0g5xkpur9po42a3k6jocdfert.0ywnp6d.rcgu.o f.0gsn8ev1cnkk8096ejd9eaqnc.0ywnp6d.rcgu.o f.0hnyc0p3974v12h5g8kkwz19v.0ywnp6d.rcgu.o f.0xonaogqri1qh024baqfa7x2d.0ywnp6d.rcgu.o f.12nfqzyx5t92gdliu6u9j2t23.0ywnp6d.rcgu.o f.1bcwhu0k0xfbj80ouktlgq7gg.0ywnp6d.rcgu.o f.1bhjf3uobqgssktgi0e14aqnn.0ywnp6d.rcgu.o f.1hqz8k3c94266bfgrfbuybt2z.0ywnp6d.rcgu.o f.1huk6ich8t6qwyn0rf9m9q3l0.0ywnp6d.rcgu.o (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-3136f96127ec4700 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-3136f96127ec4700 f.0a5uivvqg0yudio4gs881kurk.0ywnp6d.rcgu.o f.0avko3xidh2wk8781opolbaij.0ywnp6d.rcgu.o f.0g5xkpur9po42a3k6jocdfert.0ywnp6d.rcgu.o f.0gsn8ev1cnkk8096ejd9eaqnc.0ywnp6d.rcgu.o f.0hnyc0p3974v12h5g8kkwz19v.0ywnp6d.rcgu.o f.0xonaogqri1qh024baqfa7x2d.0ywnp6d.rcgu.o f.12nfqzyx5t92gdliu6u9j2t23.0ywnp6d.rcgu.o f.1bcwhu0k0xfbj80ouktlgq7gg.0ywnp6d.rcgu.o f.1bhjf3uobqgssktgi0e14aqnn.0ywnp6d.rcgu.o f.1hqz8k3c94266bfgrfbuybt2z.0ywnp6d.rcgu.o f.1huk6ich8t6qwyn0rf9m9q3l0.0ywnp6d.rcgu.o f.1t�� f.270d89qd8zd7pddtw479jvwb6.0ywnp6d.rcgu.o f.2i0rx46105zwgayhinkt9jur2.0ywnp6d.rcgu.o mbols.o d12dc65cce50cb.agit d12dc65cce50cb.aremote d12dc65cce50cb.aget-url d12dc65cce50cb.aorigin (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[github-actions]

🔍 Rust PR Review

Summary: Has one correctness bug (silent config loss) that should be fixed before merge; everything else looks solid.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/mod.rs (WorkItemReportConfig) — title is a bare String with no #[serde(default = ...)]. If a user provides a partial work-item: override (e.g., only work-item-type: Bug) without specifying title:, serde fails to deserialize WorkItemReportConfig, and get_tool_config's unwrap_or_default() silently falls back to the full struct default — losing all the user's overrides with no error or warning. The user has no way to know their configuration was discarded.

  Fix: either give title an explicit #[serde(default)] via a context-appropriate fallback function (but since the struct is shared across noop/missing-tool, this is tricky), or make title Option<String> and resolve the effective title at call sites with the context-appropriate default.

⚠️ Suggestions

• src/safeoutputs/mod.rs:wiql_find_work_item_by_title — A fresh reqwest::Client is created per call. This is inconsequential in practice (at most one call per pipeline run), but it's a minor divergence from ADO REST callers elsewhere in the codebase which also create their own clients. No action required — just noting for consistency.

• src/safeoutputs/mod.rs:file_or_append_work_item (~line 560) — ExecutionResult::failure() is returned (not a hard ? bail) when the comment POST or work item POST returns a non-2xx status. This is the right call given the always-on nature of the tools, and it's consistent with the credential-missing path. Just make sure callers/tests cover the failure-result path.

✅ What Looks Good

• Graceful-warning fallback for missing ADO credentials is correct and necessary — noop/missing-tool should never block a pipeline.
• Sanitization chain is complete: execute_sanitized() calls sanitize_content_fields() before execute_impl() so agent-supplied context/tool_name are sanitized before entering the work item body; get_tool_config calls sanitize_config_fields() on the operator-controlled config.
• WIQL injection protection: single-quote doubling (replace('\'', "''")) is the correct WIQL escaping convention, and the title comes from sanitized operator-controlled front matter, not agent input.
• multilineFieldsFormat path follows the established pattern already used in create_work_item.rs and update_work_item.rs.
• Test coverage is good: defaults, partial deserialization, and the no-credentials warning path are all exercised.

Generated by Rust PR Reviewer for issue #521 · ● 791.4K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Has one meaningful bug that breaks the stated design guarantee — the "never blocks the pipeline" promise is violated in HTTP error paths.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/mod.rs — HTTP failure paths return hard failure, breaking the "always-on" promise

  The PR description and docs state these tools "never block the pipeline" — the missing-credentials and WIQL-failure cases correctly return ExecutionResult::warning. But two HTTP error paths return ExecutionResult::failure (success=false), which causes main.rs:292-293 to call std::process::exit(1) and fail Stage 3:

  // comment-append failure (line ~614)
  Ok(ExecutionResult::failure(format!(
      "Failed to append comment to work item #{} (HTTP {}): {}",
      ...
  )))
  
  // work-item creation failure (line ~680)
  Ok(ExecutionResult::failure(format!(
      "Failed to create work item (HTTP {}): {}",
      ...
  )))

  Both should be ExecutionResult::warning(...) to match the pattern established for the missing-credentials and WIQL-failure paths above them. Any transient ADO 5xx or permissions issue would otherwise silently kill the pipeline — the exact scenario this design is supposed to be immune to.

• src/execute.rs — test assertion removed without replacement

  The assertion assert!(result.message.contains("No operation")) was deleted. While the behaviour has changed (good), nothing in the test now verifies the warning message content, so a regression that produces an empty or misleading message would pass undetected. Consider asserting exec.message.contains("not set") or similar.

⚠️ Suggestions

• src/main.rs:discover_last_committer — path.file_name()? silently returns None for unusual paths

  Paths like Path::new("") or paths ending in .. return None from file_name(). The ? in an Option-returning async block handles this correctly (returns None), but the failure is never logged. A log::debug!("git log skipped: no file_name for {}", path.display()) before the ? return would aid diagnostics — consistent with how all other failure cases in this function are logged.

• src/safeoutputs/mod.rs:wiql_find_work_item_by_title — WIQL single-quote escaping is correct but worth a test

  The comment correctly explains that doubling single quotes is the WIQL convention. Since the title is operator-controlled front-matter (not agent input), injection risk is low. A unit test for a title containing ' (e.g. "agent's task") confirming the escaping produces "agent''s task" would guard against future regressions.

✅ What Looks Good

• The three-tier graceful-degradation structure (no credentials → WIQL failure → HTTP call) is well-designed and clearly commented.
• #[derive(SanitizeConfig)] on WorkItemReportConfig properly sanitizes all String/Option<String>/Vec<String> fields including title, area_path, and tags before they reach the WIQL query.
• discover_last_committer runs git in a subprocess and handles all error variants without panicking — the fallback-to-None approach is exactly right for an optional enrichment feature.
• Config deserialization tests cover defaults, overrides, and the empty-map case — good coverage for the serde layer.

Generated by Rust PR Reviewer for issue #521 · ● 768.5K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the feature is well-structured and the graceful-degradation path is a solid design choice. Two findings worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/execute.rs:582-597 — integration tests don't assert is_warning()
  Both the noop and missing-tool execute-level tests were updated to remove the old message-content assertion but only assert result.success. The comment even says "returns a warning (success=true) rather than failing hard", yet neither test calls assert!(result.is_warning()). The per-module unit tests in noop.rs/missing_tool.rs do check this, so the integration tests are weaker than they could be. A no-credentials execution should produce a warning, and success=true alone doesn't distinguish ExecutionResult::success from ExecutionResult::warning.

⚠️ Suggestions

• src/main.rs:370 — discover_last_committer runs git log against bare filename only
  path.file_name()? extracts just the filename and current_dir(dir) changes into the parent. This works correctly for any path where the parent directory is the actual working tree location. However, path.parent() on a relative path with no directory component (e.g. Path::new("agent.md")) returns Some(""), and Command::current_dir("") will fail with ENOENT — causing the fallback Err(e) branch to fire and return None. In practice source is always an absolute path from clap, but a defensive let dir = path.parent().filter(|p| !p.as_os_str().is_empty()).unwrap_or(Path::new(".")); would make the intent explicit and remove the latent edge case.

• src/safeoutputs/mod.rs:525-535 — unbounded ADO error body included in warning messages
  The WIQL failure path does anyhow::bail!("WIQL query failed (HTTP {}): {}", status, error_body) where error_body is the raw HTTP response body. This propagates into the ExecutionResult::warning message and gets printed to stdout (neutralized by neutralize_pipeline_commands, so no injection risk). The concern is simply size — a verbose ADO error page could produce a very noisy pipeline log line. Truncating to, say, the first 512 bytes would keep the diagnostic value while keeping logs readable.

✅ What Looks Good

• Sanitization chain is correct: WorkItemReportConfig derives SanitizeConfig, get_tool_config calls sanitize_config_fields() before returning, and WIQL single-quote escaping has a clear comment explaining the WIQL escaping convention and why it is sufficient for operator-controlled config.
• Graceful degradation design: Returning ExecutionResult::warning instead of failing hard when ADO credentials are absent is exactly right for always-on diagnostic tools — other pipeline stages shouldn't be blocked by a missing write token.
• discover_last_committer error handling: All three failure branches (Ok(non-success), Ok(empty), Err) log at debug and return None cleanly. No panics, no hard failures.
• multilineFieldsFormat patch op is consistent with the existing create_work_item.rs and update_work_item.rs patterns — not a new pattern introduced by this PR.

Generated by Rust PR Reviewer for issue #521 · ● 1.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Has one unsanitized-input security concern and a no-opt-out breaking behavioral change worth discussing before merge.

---

Findings

🔒 Security Concerns

• src/safeoutputs/create_work_item.rs:361 — ctx.agent_last_committer is used directly as the System.AssignedTo value without any sanitization, while config.assignee goes through SanitizeConfig automatically. agent_last_committer is sourced from git log --format=%ae, and a malicious git committer email could contain ##vso[task.setvariable ...] or similar ADO pipeline log commands. The fix is simple: apply sanitize_config() either when storing the value in discover_last_committer (return Some(sanitize_config(&email))) or at the point of use in create_work_item.rs.

🐛 Bugs / Logic Issues

• src/safeoutputs/noop.rs & missing_tool.rs — When a user provides a partial work-item: override in front matter (e.g. work-item: { work-item-type: Bug } with no title:), serde deserializes WorkItemReportConfig.title as None (from #[serde(default)]), even though the per-tool default function (noop_default_work_item()) would have set a title. The file_or_append_work_item unwrap_or(default_title) fallback recovers correctly, so it's functionally fine — but the test comment on test_config_partial_work_item_preserves_overrides calling this a regression test suggests it was a surprise. This serde-default layering quirk could be a footgun for future WorkItemReportConfig callers. Worth a code comment or doc note to document that title: None is valid and means "use caller's default".

⚠️ Suggestions

• No opt-out mechanism — noop and missing-tool now unconditionally make ADO API calls on every invocation. Existing pipelines that call these tools will silently begin filing work items when the pipeline gains write credentials, with no way to disable it short of removing the safe-output calls entirely. Consider adding enabled: bool (default true) to WorkItemReportConfig so operators can explicitly set enabled: false to restore the old behaviour. The graceful credential-absent warning helps, but an intentional opt-out is cleaner.

• src/execute.rs:581 — The test previously asserted result.message.contains("No operation"), which is now gone. The comment explains why (work item filing changes the message), but it leaves the noop success path without any message-content assertion. A minimal assert!(exec.message.contains("work item") || exec.message.contains("not set")) would add signal without being brittle.

• src/main.rs:370 — discover_last_committer passes only path.file_name()? (the bare filename) to git log. This means renames in the history won't be followed (no --follow flag). That's a conscious trade-off, but it's worth a comment acknowledging it since --follow has its own edge cases with merges.

✅ What Looks Good

• The WIQL single-quote escaping (title.replace('\'', "''")) and the inline comment explaining why this is safe (operator-controlled, not agent-supplied) are well done.
• The soft-failure pattern (warning, not error) when ADO credentials are absent is the right call for always-on diagnostic tools.
• WorkItemReportConfig deriving SanitizeConfig and the get_tool_config path applying sanitize_config_fields() before returning means all front-matter string fields are properly sanitized — the agent_last_committer issue above is the only path that bypasses this.
• Test coverage for config defaults, partial overrides, and the no-credentials warning path is solid.

Generated by Rust PR Reviewer for issue #521 · ● 1.3M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid implementation with correct security handling. Two concerns worth addressing before merge: a silent breaking behavioral change and a missing HTTP timeout.

---

Findings

🐛 Bugs / Logic Issues

• src/execute.rs:582-587 — The test assertions for noop and missing-tool were loosened from result.message.contains("No operation") to result.message.contains("work item") || result.message.contains("not set"). The || makes this fairly broad; it could pass even if the message is completely unexpected. Worth tightening to at least test that result.is_warning() matches the expected state in this credential-absent path (I see the unit tests in noop.rs/missing_tool.rs do assert is_warning() — just not the integration test in execute.rs).

⚠️ Suggestions

• src/safeoutputs/mod.rs (file_or_append_work_item) — reqwest::Client::new() has no timeout configured. If the ADO endpoint is slow or unreachable (e.g. a flaky network), Stage 3 could hang indefinitely. Consider:

  let client = reqwest::Client::builder()
      .timeout(std::time::Duration::from_secs(30))
      .build()
      .unwrap_or_default();

• Breaking behavioral change — no changelog signal: Every existing pipeline that uses noop or missing-tool without a write ADO token will now get a warning result instead of a success result at Stage 3. The PR title is feat(safeoutputs):... which is correct for release-please, but the enabled: true default means existing consumers silently pick up the new behavior on next run with no opt-in. Given this repo squash-merges onto main and rolls out to all consumers, it may be worth:

  1. Noting in CHANGELOG.md or the PR description that this is a behavioral change for pipelines without write tokens.
  2. Confirming the warning result in Stage 3 is treated the same as success by any CI/CD gating logic in consuming repositories (i.e. it doesn't cause pipeline failures).

✅ What Looks Good

• WIQL injection escaping (wiql_find_work_item_by_title): the single-quote doubling is the correct WIQL escaping convention, the comment explaining the rationale is clear, and the title is operator-controlled (sanitized via sanitize_config_fields() before reaching this function).
• Git committer email sanitization (discover_last_committer): running sanitize_config() on the raw git output before placing it in agent_last_committer correctly neutralizes any ##vso[ sequences a malicious committer could embed in their git email.
• Graceful degradation: the three-stage credential check (ado_org_url, ado_project, access_token) returning ExecutionResult::warning rather than propagating errors is exactly right for always-on diagnostic tools.
• stdout neutralization: neutralize_pipeline_commands is already applied to result.message before it hits stdout in execute.rs (lines 217–218), so the work item title leaking through the message is protected.
• Test coverage is thorough — default deserialization, partial overrides, enabled: false, and the no-credentials warning path are all exercised.

Generated by Rust PR Reviewer for issue #521 · ● 573.8K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good with one naming/documentation bug and one design trade-off worth discussing.

Findings

🐛 Bugs / Logic Issues

• src/main.rs:372 — %ae vs committer email: discover_last_committer uses --format=%ae which is the author email, not the committer email (%ce). The function name, doc comment ("email of the person who last committed changes"), log message ("Agent source last committer: {}"), and field name agent_last_committer all say "committer" but the value retrieved is the author. For this project's squash-merge workflow (AGENTS.md), %ae actually gives the more meaningful value (the PR author, vs GitHub's noreply address for %ce), but the naming/documentation is consistently wrong. Either rename to discover_last_author / agent_last_author, or switch to %ce if committer is truly the intent.

⚠️ Suggestions

• src/safeoutputs/noop.rs, src/safeoutputs/missing_tool.rs — always-on default is a silent behavior change: enabled: true means every existing pipeline with ADO write credentials will now create/append a work item on every noop/missing-tool call, with no migration notice. For agents that legitimately reach noop repeatedly (e.g. a monitoring agent that checks and finds nothing to do), this could generate noisy work items. The graceful-warning path for missing credentials makes this non-breaking from a CI perspective, but the ADO project may fill up with Tasks. Worth calling out explicitly in the PR description as a behavior change (it's there, but a feat! or at least a note in the enabled: false docs might help operators opt out without reading the code).

• src/safeoutputs/mod.rs — reqwest::Client created per invocation: file_or_append_work_item builds a new reqwest::Client on each call. This is fine for the current "one call per pipeline run" use case, but if the function is ever called more frequently the connection pool won't be reused. Not a bug today, just something to keep in mind.

✅ What Looks Good

• WIQL escaping is correct and well-documented: The title.replace('\'', "''") escaping with the inline comment explaining why @project is safe and that title is operator-controlled (not agent-supplied) is exactly the right level of rigor here.
• sanitize_config_fields() is called via get_tool_config: The WorkItemReportConfig derives SanitizeConfig, and get_tool_config calls sanitize_config_fields() before returning — so any operator-supplied title, area path, tags, etc. are sanitized before they reach the WIQL query or REST calls.
• Graceful degradation on missing credentials: Returning ExecutionResult::warning (success=true) instead of failing hard keeps the always-on behavior from breaking pipelines that don't have a write service connection. The tests cover this path.
• Comment-or-create deduplication: Using WIQL to find an existing non-closed work item by title and appending a comment avoids flooding the project with duplicate Tasks across multiple pipeline runs.
• SanitizeContent applied before body composition: noop and missing-tool sanitize agent-provided context/tool_name via the Executor::execute_sanitized path before those values reach the work item description body.

Generated by Rust PR Reviewer for issue #521 · ● 584K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the design is sound and security hygiene is solid. Two issues worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/mod.rs:577 — reqwest::Client::builder().timeout(...).build().unwrap_or_default(): if the builder fails (rare but possible), Client::default() silently drops the 30-second timeout, leaving both the WIQL search and the work-item POST calls with no timeout. These could then hang indefinitely and stall Stage 3. Prefer .context("Failed to build HTTP client")? to surface the failure, or at minimum .expect("reqwest ClientBuilder failed unexpectedly") since there is no user-recoverable path here.

• src/execute.rs:584,607 — Both tests assert result.message.contains("not set"). The test passes today because ado_org_url is checked first ("AZURE_DEVOPS_ORG_URL not set; ..."), but the access-token path returns "No access token available ..." which does not contain "not set". If the credential-check order is ever reordered the test could start asserting the wrong thing silently. A more precise assertion (e.g. contains("AZURE_DEVOPS_ORG_URL not set") or a dedicated is_warning() check with the exact message) would be more robust.

✅ What Looks Good

• SanitizeConfig derive on WorkItemReportConfig correctly handles Vec<String> tags — confirmed in the derive macro.
• get_tool_config always calls sanitize_config_fields() before returning, so operator-supplied title/area-path values are sanitized before reaching the WIQL query.
• Single-quote doubling in wiql_find_work_item_by_title is the correct WIQL escaping convention, and the title is always operator-controlled (never agent-supplied).
• discover_last_author pipes the raw git output through sanitize_config() before populating agent_last_author, preventing any ##vso[ content from a malicious git history from reaching the ADO API call.
• The enabled: false escape hatch + graceful warning-not-failure for missing credentials is the right design for an always-on diagnostic tool.

Generated by Rust PR Reviewer for issue #521 · ● 576.3K · ◷