fix(safe-outputs): address reviewer feedback on work-item filing

- Sanitize agent_last_committer via sanitize_config() to prevent
  pipeline command injection from malicious git committer identity
- Add enabled: bool to WorkItemReportConfig (default true) so
  operators can opt out of work-item filing with enabled: false
- Add message-content assertions to noop/missing-tool execute tests
- Document serde-default layering quirk for title: None in partial
  work-item configs
- Document --follow trade-off in discover_last_committer

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

Author: jamesadevine

docs/safe-outputs.md

@@ -209,6 +209,7 @@ The executor always files an Azure DevOps work item or appends a comment to an e
 safe-outputs:
   noop:
     work-item:                                  # Work item config — always active with these defaults
+      enabled: true                             # Set to false to disable work-item filing entirely
       title: "[ado-aw] Agent reported no operation"  # Default title (used to find existing items too)
       work-item-type: Task                      # Work item type (default: "Task")
       area-path: "MyProject\\MyTeam"            # Optional — area path
@@ -242,6 +243,7 @@ The executor always files an Azure DevOps work item or appends a comment to an e
 safe-outputs:
   missing-tool:
     work-item:                                     # Work item config — always active with these defaults
+      enabled: true                                # Set to false to disable work-item filing entirely
       title: "[ado-aw] Agent encountered missing tool"  # Default title (used to find existing items too)
       work-item-type: Task                         # Work item type (default: "Task")
       area-path: "MyProject\\MyTeam"               # Optional — area path

src/execute.rs

@@ -581,6 +581,11 @@ mod tests {
         // noop always attempts to file a work item; without ADO credentials it
         // returns a warning (success=true) rather than failing hard.
         assert!(result.success);
+        assert!(
+            result.message.contains("work item") || result.message.contains("not set"),
+            "noop warning should mention work item or missing config, got: {}",
+            result.message
+        );
     }
 
     #[tokio::test]
@@ -595,6 +600,11 @@ mod tests {
         // missing-tool always attempts to file a work item; without ADO credentials
         // it returns a warning (success=true) rather than failing hard.
         assert!(result.success);
+        assert!(
+            result.message.contains("work item") || result.message.contains("not set"),
+            "missing-tool warning should mention work item or missing config, got: {}",
+            result.message
+        );
     }
 
     #[tokio::test]

src/main.rs

@@ -361,6 +361,11 @@ async fn build_execution_context(
 /// Runs `git log -1 --format='%ae' -- <path>` in the file's parent directory.
 /// Returns `None` (with a debug log) when the lookup fails — e.g. shallow
 /// clone with no relevant history, or git is unavailable.
+///
+/// Note: we pass the bare filename (not a full path) so git resolves it
+/// relative to `cwd`. This means renames in history are not followed
+/// (`--follow` has its own edge-cases with merge commits and is not worth
+/// the complexity here).
 async fn discover_last_committer(path: &Path) -> Option<String> {
     let dir = path.parent().unwrap_or(Path::new("."));
     let output = tokio::process::Command::new("git")
@@ -377,7 +382,11 @@ async fn discover_last_committer(path: &Path) -> Option<String> {
                 log::debug!("git log returned no committer for {}", path.display());
                 None
             } else {
-                Some(email)
+                // Sanitize the email: git committer values can contain
+                // arbitrary text (e.g. ADO pipeline log commands like
+                // ##vso[task.setvariable ...]).  Apply the same config-level
+                // sanitization used for operator-supplied fields.
+                Some(crate::sanitize::sanitize_config(&email))
             }
         }
         Ok(o) => {

src/safeoutputs/missing_tool.rs

@@ -43,6 +43,7 @@ fn missing_tool_default_work_item_title() -> String {
 
 fn missing_tool_default_work_item() -> WorkItemReportConfig {
     WorkItemReportConfig {
+        enabled: true,
         title: Some(missing_tool_default_work_item_title()),
         work_item_type: "Task".to_string(),
         area_path: None,
@@ -164,6 +165,7 @@ mod tests {
     #[test]
     fn test_config_default_has_sensible_work_item() {
         let config = MissingToolConfig::default();
+        assert!(config.work_item.enabled);
         assert_eq!(config.work_item.title.as_deref(), Some("[ado-aw] Agent encountered missing tool"));
         assert_eq!(config.work_item.work_item_type, "Task");
         assert!(config.work_item.area_path.is_none());
@@ -199,6 +201,12 @@ work-item:
 
     #[test]
     fn test_config_partial_work_item_preserves_overrides() {
+        // When a partial work-item: block is provided in front matter (e.g.
+        // only `work-item-type:` with no `title:`), serde deserializes
+        // `title` as `None` via `#[serde(default)]` — NOT via the per-tool
+        // default function `missing_tool_default_work_item()`.  The caller's
+        // `unwrap_or(default_title)` in `file_or_append_work_item` recovers
+        // the intended title at execution time.
         let yaml = r#"
 work-item:
   work-item-type: Bug
@@ -211,6 +219,37 @@ work-item:
         assert_eq!(config.work_item.tags, vec!["agent-missing-tool"]);
     }
 
+    #[tokio::test]
+    async fn test_execute_impl_disabled_skips_work_item() {
+        let result: MissingToolResult = MissingToolParams {
+            tool_name: "bash".to_string(),
+            context: None,
+        }
+        .try_into()
+        .unwrap();
+
+        let mut ctx = crate::safeoutputs::ExecutionContext::default();
+        ctx.tool_configs.insert(
+            "missing-tool".to_string(),
+            serde_json::to_value(MissingToolConfig {
+                work_item: WorkItemReportConfig {
+                    enabled: false,
+                    ..missing_tool_default_work_item()
+                },
+            })
+            .unwrap(),
+        );
+
+        let exec = result.execute_impl(&ctx).await.unwrap();
+        assert!(exec.success);
+        assert!(!exec.is_warning());
+        assert!(
+            exec.message.contains("disabled"),
+            "expected disabled message, got: {}",
+            exec.message
+        );
+    }
+
     #[tokio::test]
     async fn test_execute_impl_without_ado_credentials_returns_warning() {
         let result: MissingToolResult = MissingToolParams {

src/safeoutputs/mod.rs

@@ -390,6 +390,8 @@ fn work_item_report_default_type() -> String {
 ///
 /// Both `noop` and `missing-tool` default to always creating/appending a work item.
 /// Override the defaults in front matter to customise the title, type, area path, etc.
+/// Set `enabled: false` to disable work-item filing entirely and restore the old
+/// pass-through behaviour.
 ///
 /// Example:
 /// ```yaml
@@ -404,12 +406,21 @@ fn work_item_report_default_type() -> String {
 /// ```
 #[derive(Debug, Clone, SanitizeConfig, Serialize, Deserialize)]
 pub struct WorkItemReportConfig {
+    /// Whether work-item filing is enabled (default: `true`).
+    /// Set to `false` to disable work-item creation/appending entirely.
+    #[serde(default = "default_enabled")]
+    pub enabled: bool,
+
     /// Title of the work item to file or append a comment to.
     /// If a non-closed work item with this exact title already exists,
     /// a comment is appended rather than creating a new work item.
     ///
     /// When `None`, each caller supplies a context-appropriate default
-    /// (e.g. noop vs missing-tool).
+    /// (e.g. noop vs missing-tool).  This can happen when a partial
+    /// `work-item:` block is provided in front matter (e.g. overriding
+    /// only `work-item-type:`) — serde deserializes `title` as `None`
+    /// because `#[serde(default)]` applies per-field, not via the
+    /// per-tool default function.
     #[serde(default)]
     pub title: Option<String>,
 
@@ -434,6 +445,10 @@ pub struct WorkItemReportConfig {
     pub include_stats: bool,
 }
 
+fn default_enabled() -> bool {
+    true
+}
+
 /// Search for a non-closed work item by exact title using WIQL.
 /// Returns the ID of the most-recently-changed matching work item, or `None` if none found.
 async fn wiql_find_work_item_by_title(
@@ -523,6 +538,11 @@ pub(crate) async fn file_or_append_work_item(
     body: &str,
     ctx: &ExecutionContext,
 ) -> anyhow::Result<ExecutionResult> {
+    if !config.enabled {
+        return Ok(ExecutionResult::success(
+            "Work-item filing disabled via enabled: false".to_string(),
+        ));
+    }
     let title = config.title.as_deref().unwrap_or(default_title);
     let org_url = match &ctx.ado_org_url {
         Some(u) => u,

src/safeoutputs/noop.rs

@@ -37,6 +37,7 @@ fn noop_default_work_item_title() -> String {
 
 fn noop_default_work_item() -> WorkItemReportConfig {
     WorkItemReportConfig {
+        enabled: true,
         title: Some(noop_default_work_item_title()),
         work_item_type: "Task".to_string(),
         area_path: None,
@@ -175,6 +176,7 @@ mod tests {
     #[test]
     fn test_config_default_has_sensible_work_item() {
         let config = NoopConfig::default();
+        assert!(config.work_item.enabled);
         assert_eq!(config.work_item.title.as_deref(), Some("[ado-aw] Agent reported no operation"));
         assert_eq!(config.work_item.work_item_type, "Task");
         assert!(config.work_item.area_path.is_none());
@@ -210,8 +212,12 @@ work-item:
 
     #[test]
     fn test_config_partial_work_item_preserves_overrides() {
-        // Regression: a partial work-item config (no title) must not
-        // silently discard the user's other overrides.
+        // When a partial work-item: block is provided in front matter (e.g.
+        // only `work-item-type:` with no `title:`), serde deserializes
+        // `title` as `None` via `#[serde(default)]` — NOT via the per-tool
+        // default function `noop_default_work_item()`.  The caller's
+        // `unwrap_or(default_title)` in `file_or_append_work_item` recovers
+        // the intended title at execution time.
         let yaml = r#"
 work-item:
   work-item-type: Bug
@@ -223,6 +229,37 @@ work-item:
         assert_eq!(config.work_item.area_path.as_deref(), Some("MyProject\\MyTeam"));
     }
 
+    #[tokio::test]
+    async fn test_execute_impl_disabled_skips_work_item() {
+        let result: NoopResult = NoopParams {
+            context: Some("nothing to do".to_string()),
+        }
+        .try_into()
+        .unwrap();
+
+        let mut ctx = crate::safeoutputs::ExecutionContext::default();
+        // Configure noop with enabled: false
+        ctx.tool_configs.insert(
+            "noop".to_string(),
+            serde_json::to_value(NoopConfig {
+                work_item: WorkItemReportConfig {
+                    enabled: false,
+                    ..noop_default_work_item()
+                },
+            })
+            .unwrap(),
+        );
+
+        let exec = result.execute_impl(&ctx).await.unwrap();
+        assert!(exec.success);
+        assert!(!exec.is_warning());
+        assert!(
+            exec.message.contains("disabled"),
+            "expected disabled message, got: {}",
+            exec.message
+        );
+    }
+
     #[tokio::test]
     async fn test_execute_impl_without_ado_credentials_returns_warning() {
         let result: NoopResult = NoopParams {