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>

Author: jamesadevine

src/execute.rs

@@ -10,7 +10,7 @@ use std::path::Path;
 
 use crate::ndjson::{self, SAFE_OUTPUT_FILENAME};
 use crate::tools::{
-    CommentOnWorkItemResult, 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());
@@ -134,6 +139,38 @@ pub async fn execute_safe_outputs(
             update_wi_executed += 1;
         }
 
+        // Enforce comment-on-work-item max: skip excess entries rather than aborting the whole batch
+        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 {

src/mcp.rs

@@ -357,14 +357,8 @@ pipeline configuration."
         let mut sanitized = params.0;
         sanitized.body = sanitize_text(&sanitized.body);
         let result: CommentOnWorkItemResult = sanitized.try_into()?;
-        let written = self.write_safe_output_file_with_maximum(&result, 1).await
+        let _ = self.write_safe_output_file(&result).await
             .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?;
-        if !written {
-            warn!("comment-on-work-item limit reached, ignoring");
-            return Ok(CallToolResult::success(vec![Content::text(
-                "Maximum number of comments reached for this run. This comment was not recorded.",
-            )]));
-        }
         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.",

src/tools/comment_on_work_item.rs

@@ -99,8 +99,9 @@ pub struct CommentOnWorkItemConfig {
     #[serde(default = "default_max")]
     pub max: u32,
 
-    /// Target scope — which work items can be commented on (required)
-    pub target: CommentTarget,
+    /// Target scope — which work items can be commented on.
+    /// `None` means no target was configured; execution must reject this.
+    pub target: Option<CommentTarget>,
 }
 
 fn default_max() -> u32 {
@@ -111,7 +112,7 @@ impl Default for CommentOnWorkItemConfig {
     fn default() -> Self {
         Self {
             max: default_max(),
-            target: CommentTarget::StringTarget("*".to_string()),
+            target: None,
         }
     }
 }
@@ -191,10 +192,21 @@ impl Executor for CommentOnWorkItemResult {
         let config: CommentOnWorkItemConfig = ctx.get_tool_config("comment-on-work-item");
         debug!("Target: {:?}, max: {}", config.target, config.max);
 
+        let target = match &config.target {
+            Some(t) => t,
+            None => {
+                return Ok(ExecutionResult::failure(
+                    "comment-on-work-item target is not configured. \
+                     This is required to scope which work items the agent can comment on."
+                        .to_string(),
+                ));
+            }
+        };
+
         let client = reqwest::Client::new();
 
         // Validate work item ID against target policy
-        match config.target.allows_id(self.work_item_id) {
+        match target.allows_id(self.work_item_id) {
             Some(true) => {
                 debug!(
                     "Work item #{} allowed by target policy",
@@ -209,29 +221,30 @@ impl Executor for CommentOnWorkItemResult {
             }
             None => {
                 // Area path validation — need to fetch the work item
-                if let Some(prefix) = config.target.area_path_prefix() {
-                    debug!(
-                        "Validating area path for work item #{} against prefix '{}'",
-                        self.work_item_id, prefix
-                    );
-                    match get_work_item_area_path(&client, org_url, project, token, self.work_item_id)
-                        .await
-                    {
-                        Ok(area_path) => {
-                            if !area_path.starts_with(prefix) {
-                                return Ok(ExecutionResult::failure(format!(
-                                    "Work item #{} has area path '{}' which is not under allowed prefix '{}'",
-                                    self.work_item_id, area_path, prefix
-                                )));
-                            }
-                            debug!("Area path '{}' validated against '{}'", area_path, prefix);
-                        }
-                        Err(e) => {
+                let prefix = target.area_path_prefix().expect(
+                    "allows_id returned None but area_path_prefix is also None; this is a bug",
+                );
+                debug!(
+                    "Validating area path for work item #{} against prefix '{}'",
+                    self.work_item_id, prefix
+                );
+                match get_work_item_area_path(&client, org_url, project, token, self.work_item_id)
+                    .await
+                {
+                    Ok(area_path) => {
+                        if !area_path.starts_with(prefix) {
                             return Ok(ExecutionResult::failure(format!(
-                                "Failed to validate area path for work item #{}: {}",
-                                self.work_item_id, e
+                                "Work item #{} has area path '{}' which is not under allowed prefix '{}'",
+                                self.work_item_id, area_path, prefix
                             )));
                         }
+                        debug!("Area path '{}' validated against '{}'", area_path, prefix);
+                    }
+                    Err(e) => {
+                        return Ok(ExecutionResult::failure(format!(
+                            "Failed to validate area path for work item #{}: {}",
+                            self.work_item_id, e
+                        )));
                     }
                 }
             }
@@ -382,6 +395,7 @@ mod tests {
     fn test_config_defaults() {
         let config = CommentOnWorkItemConfig::default();
         assert_eq!(config.max, 1);
+        assert!(config.target.is_none());
     }
 
     #[test]
@@ -392,6 +406,7 @@ target: "*"
 "#;
         let config: CommentOnWorkItemConfig = serde_yaml::from_str(yaml).unwrap();
         assert_eq!(config.max, 5);
+        assert!(config.target.is_some());
     }
 
     #[test]
@@ -401,8 +416,9 @@ max: 1
 target: 12345
 "#;
         let config: CommentOnWorkItemConfig = serde_yaml::from_str(yaml).unwrap();
-        assert!(config.target.allows_id(12345) == Some(true));
-        assert!(config.target.allows_id(99999) == Some(false));
+        let target = config.target.unwrap();
+        assert!(target.allows_id(12345) == Some(true));
+        assert!(target.allows_id(99999) == Some(false));
     }
 
     #[test]
@@ -415,9 +431,10 @@ target:
   - 300
 "#;
         let config: CommentOnWorkItemConfig = serde_yaml::from_str(yaml).unwrap();
-        assert!(config.target.allows_id(100) == Some(true));
-        assert!(config.target.allows_id(200) == Some(true));
-        assert!(config.target.allows_id(999) == Some(false));
+        let target = config.target.unwrap();
+        assert!(target.allows_id(100) == Some(true));
+        assert!(target.allows_id(200) == Some(true));
+        assert!(target.allows_id(999) == Some(false));
     }
 
     #[test]
@@ -426,8 +443,9 @@ target:
 target: "*"
 "#;
         let config: CommentOnWorkItemConfig = serde_yaml::from_str(yaml).unwrap();
-        assert!(config.target.allows_id(1) == Some(true));
-        assert!(config.target.allows_id(99999) == Some(true));
+        let target = config.target.unwrap();
+        assert!(target.allows_id(1) == Some(true));
+        assert!(target.allows_id(99999) == Some(true));
     }
 
     #[test]
@@ -436,8 +454,18 @@ target: "*"
 target: "4x4\\QED"
 "#;
         let config: CommentOnWorkItemConfig = serde_yaml::from_str(yaml).unwrap();
-        assert!(config.target.allows_id(1).is_none());
-        assert_eq!(config.target.area_path_prefix(), Some("4x4\\QED"));
+        let target = config.target.unwrap();
+        assert!(target.allows_id(1).is_none());
+        assert_eq!(target.area_path_prefix(), Some("4x4\\QED"));
+    }
+
+    #[test]
+    fn test_config_missing_target_defaults_to_none() {
+        let yaml = r#"
+max: 3
+"#;
+        let config: CommentOnWorkItemConfig = serde_yaml::from_str(yaml).unwrap();
+        assert!(config.target.is_none());
     }
 
     #[test]