feat: add comment-on-work-item safe output tool (#80)

* feat: add comment-on-work-item safe output tool

Add a new safe output tool that allows agents to comment on existing
Azure DevOps work items. This is the ADO equivalent of gh-aw's
add-comment tool.

Features:
- Agent provides work_item_id and body (markdown comment text)
- Required 'target' field in frontmatter scopes which work items
  can be commented on: wildcard, specific ID(s), or area path prefix
- max field (default: 1) limits comments per run
- Area path targets validated via ADO API at Stage 2
- Compile-time validation ensures target is specified

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

* refactor: remove redundant area: prefix from comment-on-work-item target field

The target field type system already disambiguates naturally:
- number → single work item ID
- array of numbers → list of IDs
- "*" → wildcard
- any other string → area path prefix

The area: prefix was unnecessary since no other string variant
could collide with area paths.

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

* fix: close access-control bypass and enforce max for comment-on-work-item

Three fixes for comment-on-work-item:

1. Silent access-control bypass: the None arm in execute_impl now
   uses expect() instead of if-let on area_path_prefix(), making
   any mismatch between allows_id and area_path_prefix a hard
   panic rather than a silent pass-through.

2. max never enforced: Stage 1 (mcp.rs) no longer hardcodes max=1
   via write_safe_output_file_with_maximum — it uses the unbounded
   write_safe_output_file, matching update-work-item. Stage 2
   (execute.rs) now enforces the configured max with the same
   skip-and-continue pattern used by update-work-item.

3. Default config is wildcard: target is now Option<CommentTarget>
   (default None). If tool_configs is missing or malformed at
   runtime, execute_impl fails explicitly instead of silently
   granting unrestricted access.

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

* fix: propagate write errors and harden comment-on-work-item

- update_work_item MCP handler: propagate write_safe_output_file
  errors instead of silently discarding them with let _ =
- update_work_item MCP handler: remove redundant pre-sanitization;
  the Sanitize trait impl handles this in Stage 2 via
  execute_sanitized, matching the pattern other tools follow
- comment-on-work-item: use case-insensitive area path prefix
  matching since ADO area paths are case-insensitive
- Add integration tests for comment-on-work-item compile-time
  validation: requires target field, requires write SC, and
  succeeds with both

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

* fix: harden area path matching and improve code clarity

- Replace expect() with unreachable!() for the impossible branch
  where allows_id returns None but area_path_prefix is also None
- Require path separator boundary in area path prefix matching so
  prefix "4x4" does not accidentally match "4x4Production"
- Add comments explaining why max budget is consumed before
  execution (prevents unbounded retries against failing endpoints)

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

* fix: restore Stage 1 sanitization for update-work-item and add fixture test

- Restore defense-in-depth sanitization in the update-work-item MCP
  handler via result.sanitize_fields() before persisting to NDJSON,
  using the Sanitize trait impl rather than field-by-field calls
- Add comment-on-work-item fixture (tests/fixtures/) and integration
  test verifying compiled pipeline output includes the tool reference,
  safeoutputs MCP, write SC, and has no unreplaced template markers

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

* fix: require explicit target for update-work-item and fix Unicode boundary check

- update-work-item target is now Option<TargetConfig> (default None),
  matching the comment-on-work-item pattern. Omitting target no longer
  silently defaults to "*" (unrestricted). Both compile-time and
  runtime validation reject missing target explicitly.
- Add validate_update_work_item_target compile-time check in both
  standalone and 1ES compilers.
- Fix area path boundary check to use str slicing (ap[pf.len()..])
  instead of byte indexing, which is sound because starts_with
  guarantees pf.len() is a valid char boundary in ap.
- Add integration test for update-work-item missing target.

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

---------

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

Author: jamesadevine

AGENTS.md

@@ -39,6 +39,7 @@ Alongside the correctly generated pipeline yaml, an agent file is generated from
 │   ├── sanitize.rs       # Input sanitization for safe outputs
 │   └── tools/            # MCP tool implementations
 │       ├── mod.rs
+│       ├── comment_on_work_item.rs
 │       ├── create_pr.rs
 │       ├── create_wiki_page.rs
 │       ├── create_work_item.rs
@@ -760,6 +761,31 @@ Safe output configurations are passed to Stage 2 execution and used when process
 
 ### Available Safe Output Tools
 
+#### comment-on-work-item
+Adds a comment to an existing Azure DevOps work item. This is the ADO equivalent of gh-aw's `add-comment` tool.
+
+**Agent parameters:**
+- `work_item_id` - The work item ID to comment on (required, must be positive)
+- `body` - Comment text in markdown format (required, must be at least 10 characters)
+
+**Configuration options (front matter):**
+- `max` - Maximum number of comments per run (default: 1)
+- `target` - **Required** — scoping policy for which work items can be commented on:
+  - `"*"` - Any work item in the project (unrestricted, must be explicit)
+  - `12345` - A specific work item ID
+  - `[12345, 67890]` - A list of allowed work item IDs
+  - `"Some\\Path"` - Work items under the specified area path prefix (any string that isn't `"*"`, validated via ADO API at Stage 2)
+
+**Example configuration:**
+```yaml
+safe-outputs:
+  comment-on-work-item:
+    max: 3
+    target: "4x4\\QED"
+```
+
+**Note:** The `target` field is required. If omitted, compilation fails with an error. This ensures operators are intentional about which work items agents can comment on.
+
 #### create-work-item
 Creates an Azure DevOps work item.
 

src/compile/common.rs

@@ -548,6 +548,7 @@ pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> Stri
 const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = &[
     "create-pull-request",
     "create-work-item",
+    "comment-on-work-item",
     "update-work-item",
     "create-wiki-page",
     "update-wiki-page",
@@ -582,6 +583,56 @@ pub fn validate_write_permissions(front_matter: &FrontMatter) -> Result<()> {
     Ok(())
 }
 
+/// Validate that comment-on-work-item has a required `target` field when configured.
+pub fn validate_comment_target(front_matter: &FrontMatter) -> Result<()> {
+    if let Some(config_value) = front_matter.safe_outputs.get("comment-on-work-item") {
+        // Check that "target" key is present in the config
+        if let Some(obj) = config_value.as_object() {
+            if !obj.contains_key("target") {
+                anyhow::bail!(
+                    "safe-outputs.comment-on-work-item requires a 'target' field to scope \
+                     which work items the agent can comment on. Options:\n\n  \
+                     target: \"*\"           # any work item (unrestricted)\n  \
+                     target: 12345          # specific work item ID\n  \
+                     target: [12345, 67890] # list of work item IDs\n  \
+                     target: \"Path\\\\Sub\"   # work items under area path prefix\n"
+                );
+            }
+        } else {
+            // If the value is not an object (e.g., `comment-on-work-item: true`), that's invalid
+            anyhow::bail!(
+                "safe-outputs.comment-on-work-item must be a configuration object with at \
+                 least a 'target' field. Example:\n\n  \
+                 safe-outputs:\n    comment-on-work-item:\n      target: \"*\"\n"
+            );
+        }
+    }
+    Ok(())
+}
+
+/// Validate that update-work-item has a required `target` field when configured.
+pub fn validate_update_work_item_target(front_matter: &FrontMatter) -> Result<()> {
+    if let Some(config_value) = front_matter.safe_outputs.get("update-work-item") {
+        if let Some(obj) = config_value.as_object() {
+            if !obj.contains_key("target") {
+                anyhow::bail!(
+                    "safe-outputs.update-work-item requires a 'target' field to scope \
+                     which work items the agent can update. Options:\n\n  \
+                     target: \"*\"   # any work item (unrestricted)\n  \
+                     target: 42    # specific work item ID\n"
+                );
+            }
+        } else {
+            anyhow::bail!(
+                "safe-outputs.update-work-item must be a configuration object with at \
+                 least a 'target' field. Example:\n\n  \
+                 safe-outputs:\n    update-work-item:\n      target: \"*\"\n      title: true\n"
+            );
+        }
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/compile/onees.rs

@@ -22,7 +22,8 @@ use super::common::{
     generate_ci_trigger, generate_copilot_ado_env, generate_executor_ado_env,
     generate_pipeline_path, generate_pipeline_resources, generate_pr_trigger,
     generate_repositories, generate_schedule, generate_source_path,
-    generate_working_directory, replace_with_indent, validate_write_permissions,
+    generate_working_directory, replace_with_indent, validate_comment_target,
+    validate_update_work_item_target, validate_write_permissions,
 };
 use super::types::{FrontMatter, McpConfig};
 
@@ -132,6 +133,10 @@ displayName: "Finalize""#,
 
         // Validate that write-requiring safe-outputs have a write service connection
         validate_write_permissions(front_matter)?;
+        // Validate comment-on-work-item has required target field
+        validate_comment_target(front_matter)?;
+        // Validate update-work-item has required target field
+        validate_update_work_item_target(front_matter)?;
 
         // Replace all template markers
         let compiler_version = env!("CARGO_PKG_VERSION");

src/compile/standalone.rs

@@ -21,6 +21,8 @@ use super::common::{
     generate_pr_trigger, generate_repositories, generate_schedule, generate_source_path,
     generate_working_directory, replace_with_indent, sanitize_filename,
     validate_write_permissions,
+    validate_comment_target,
+    validate_update_work_item_target,
 };
 use super::types::{FrontMatter, McpConfig};
 use crate::allowed_hosts::{CORE_ALLOWED_HOSTS, mcp_required_hosts};
@@ -124,6 +126,10 @@ impl Compiler for StandaloneCompiler {
 
         // Validate that write-requiring safe-outputs have a write service connection
         validate_write_permissions(front_matter)?;
+        // Validate comment-on-work-item has required target field
+        validate_comment_target(front_matter)?;
+        // Validate update-work-item has required target field
+        validate_update_work_item_target(front_matter)?;
 
         // Load threat analysis prompt template
         let threat_analysis_prompt = include_str!("../../templates/threat-analysis.md");

src/execute.rs

@@ -10,7 +10,7 @@ use std::path::Path;
 
 use crate::ndjson::{self, SAFE_OUTPUT_FILENAME};
 use crate::tools::{
-    CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, ExecutionContext, ExecutionResult,
+    CommentOnWorkItemConfig, CommentOnWorkItemResult, CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, ExecutionContext, ExecutionResult,
     Executor, UpdateWikiPageResult, UpdateWorkItemConfig, UpdateWorkItemResult,
 };
 
@@ -92,6 +92,11 @@ pub async fn execute_safe_outputs(
     let max_update_wi = update_wi_config.max as usize;
     let mut update_wi_executed: usize = 0;
 
+    // Fetch the comment-on-work-item max once; same skip-and-continue pattern
+    let comment_wi_config: CommentOnWorkItemConfig = ctx.get_tool_config("comment-on-work-item");
+    let max_comment_wi = comment_wi_config.max as usize;
+    let mut comment_wi_executed: usize = 0;
+
     let mut results = Vec::new();
     for (i, entry) in entries.iter().enumerate() {
         let entry_json = serde_json::to_string(entry).unwrap_or_else(|_| "<invalid>".to_string());
@@ -102,7 +107,9 @@ pub async fn execute_safe_outputs(
             entry_json
         );
 
-        // Enforce update-work-item max: skip excess entries rather than aborting the whole batch
+        // Enforce update-work-item max: skip excess entries rather than aborting the whole batch.
+        // Budget is consumed before execution so that failed attempts (target policy rejection,
+        // network errors) still count — this prevents unbounded retries against a failing endpoint.
         if entry.get("name").and_then(|n| n.as_str()) == Some("update-work-item") {
             if update_wi_executed >= max_update_wi {
                 let wi_id = entry
@@ -134,6 +141,39 @@ pub async fn execute_safe_outputs(
             update_wi_executed += 1;
         }
 
+        // Enforce comment-on-work-item max: same skip-and-continue pattern as update-work-item.
+        // Budget is consumed before execution so that failed attempts still count.
+        if entry.get("name").and_then(|n| n.as_str()) == Some("comment-on-work-item") {
+            if comment_wi_executed >= max_comment_wi {
+                let wi_id = entry
+                    .get("work_item_id")
+                    .and_then(|v| v.as_i64())
+                    .map(|id| format!(" (work item #{})", id))
+                    .unwrap_or_default();
+                warn!(
+                    "[{}/{}] Skipping comment-on-work-item{} entry: max ({}) already reached for this run",
+                    i + 1,
+                    entries.len(),
+                    wi_id,
+                    max_comment_wi
+                );
+                let result = ExecutionResult::failure(format!(
+                    "Skipped{}: maximum comment-on-work-item count ({}) already reached. \
+                     Increase 'max' in safe-outputs.comment-on-work-item to allow more comments.",
+                    wi_id, max_comment_wi
+                ));
+                println!(
+                    "[{}/{}] comment-on-work-item - ✗ - {}",
+                    i + 1,
+                    entries.len(),
+                    result.message
+                );
+                results.push(result);
+                continue;
+            }
+            comment_wi_executed += 1;
+        }
+
         match execute_safe_output(entry, ctx).await {
             Ok((tool_name, result)) => {
                 if result.success {
@@ -209,6 +249,17 @@ pub async fn execute_safe_output(
             );
             output.execute_sanitized(ctx).await?
         }
+        "comment-on-work-item" => {
+            debug!("Parsing comment-on-work-item payload");
+            let mut output: CommentOnWorkItemResult = serde_json::from_value(entry.clone())
+                .map_err(|e| anyhow::anyhow!("Failed to parse comment-on-work-item: {}", e))?;
+            debug!(
+                "comment-on-work-item: work_item_id={}, body length={}",
+                output.work_item_id,
+                output.body.len()
+            );
+            output.execute_sanitized(ctx).await?
+        }
         "update-work-item" => {
             debug!("Parsing update-work-item payload");
             let mut output: UpdateWorkItemResult = serde_json::from_value(entry.clone())
@@ -523,6 +574,48 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn test_execute_malformed_comment_on_work_item_returns_err() {
+        // Missing required fields (work_item_id and body)
+        let entry = serde_json::json!({"name": "comment-on-work-item"});
+        let ctx = ExecutionContext::default();
+
+        let result = execute_safe_output(&entry, &ctx).await;
+        assert!(result.is_err());
+    }
+
+    #[tokio::test]
+    async fn test_execute_comment_on_work_item_missing_context() {
+        let entry = serde_json::json!({
+            "name": "comment-on-work-item",
+            "work_item_id": 12345,
+            "body": "This is a comment on the work item."
+        });
+
+        // Context without required fields (ado_org_url, etc.)
+        let ctx = ExecutionContext {
+            ado_org_url: None,
+            ado_organization: None,
+            ado_project: None,
+            access_token: None,
+            working_directory: PathBuf::from("."),
+            source_directory: PathBuf::from("."),
+            tool_configs: HashMap::new(),
+            repository_id: None,
+            repository_name: None,
+            allowed_repositories: HashMap::new(),
+        };
+
+        let result = execute_safe_output(&entry, &ctx).await;
+        assert!(result.is_err());
+        assert!(
+            result
+                .unwrap_err()
+                .to_string()
+                .contains("AZURE_DEVOPS_ORG_URL")
+        );
+    }
+
     /// Excess update-work-item entries beyond `max` are skipped (failure result added) rather than
     /// aborting the entire batch. Other tool entries must still execute.
     #[tokio::test]
@@ -541,7 +634,8 @@ mod tests {
         // Config: update-work-item with max=1 (default), title=true so the field check passes
         let update_cfg = serde_json::json!({
             "title": true,
-            "max": 1
+            "max": 1,
+            "target": "*"
         });
         let mut tool_configs = HashMap::new();
         tool_configs.insert("update-work-item".to_string(), update_cfg);

src/mcp.rs

@@ -9,8 +9,9 @@ use serde_json::Value;
 use std::path::PathBuf;
 
 use crate::ndjson::{self, SAFE_OUTPUT_FILENAME};
-use crate::sanitize::sanitize as sanitize_text;
+use crate::sanitize::{Sanitize, sanitize as sanitize_text};
 use crate::tools::{
+    CommentOnWorkItemParams, CommentOnWorkItemResult,
     CreatePrParams, CreatePrResult, CreateWikiPageParams, CreateWikiPageResult,
     CreateWorkItemParams, CreateWorkItemResult,
     UpdateWikiPageParams, UpdateWikiPageResult, MissingDataParams, MissingDataResult,
@@ -336,6 +337,35 @@ impl SafeOutputs {
         Ok(CallToolResult::success(vec![]))
     }
 
+    #[tool(
+        name = "comment-on-work-item",
+        description = "Add a comment to an existing Azure DevOps work item. \
+Provide the work item ID and the comment body in markdown. The comment will be \
+posted during safe output processing. Target restrictions may apply based on \
+pipeline configuration."
+    )]
+    async fn comment_on_work_item(
+        &self,
+        params: Parameters<CommentOnWorkItemParams>,
+    ) -> Result<CallToolResult, McpError> {
+        info!(
+            "Tool called: comment-on-work-item - work item #{}",
+            params.0.work_item_id
+        );
+        debug!("Body length: {} chars", params.0.body.len());
+        // Sanitize untrusted agent-provided text fields (IS-01)
+        let mut sanitized = params.0;
+        sanitized.body = sanitize_text(&sanitized.body);
+        let result: CommentOnWorkItemResult = sanitized.try_into()?;
+        self.write_safe_output_file(&result).await
+            .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?;
+        info!("Comment queued for work item #{}", result.work_item_id);
+        Ok(CallToolResult::success(vec![Content::text(format!(
+            "Comment queued for work item #{}. The comment will be posted during safe output processing.",
+            result.work_item_id
+        ))]))
+    }
+
     #[tool(
         name = "update-work-item",
         description = "Update an existing Azure DevOps work item. Only fields explicitly enabled \
@@ -349,19 +379,11 @@ fields you want to update."
         params: Parameters<UpdateWorkItemParams>,
     ) -> Result<CallToolResult, McpError> {
         info!("Tool called: update-work-item - id={}", params.0.id);
-        // Sanitize untrusted agent-provided text fields (IS-01)
-        let mut sanitized = params.0;
-        sanitized.title = sanitized.title.map(|t| sanitize_text(&t));
-        sanitized.body = sanitized.body.map(|b| sanitize_text(&b));
-        sanitized.state = sanitized.state.map(|s| sanitize_text(&s));
-        sanitized.area_path = sanitized.area_path.map(|p| sanitize_text(&p));
-        sanitized.iteration_path = sanitized.iteration_path.map(|p| sanitize_text(&p));
-        sanitized.assignee = sanitized.assignee.map(|a| sanitize_text(&a));
-        sanitized.tags = sanitized
-            .tags
-            .map(|ts| ts.into_iter().map(|t| sanitize_text(&t)).collect());
-        let result: UpdateWorkItemResult = sanitized.try_into()?;
-        let _ = self.write_safe_output_file(&result).await;
+        let mut result: UpdateWorkItemResult = params.0.try_into()?;
+        // Sanitize before persisting to NDJSON (defense-in-depth; Stage 2 sanitizes again)
+        result.sanitize_fields();
+        self.write_safe_output_file(&result).await
+            .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?;
         info!("Work item update queued for #{}", result.id);
         Ok(CallToolResult::success(vec![Content::text(format!(
             "Work item #{} update queued. Changes will be applied during safe output processing.",