Merge pull request #7 from githubnext/copilot/add-test-coverage-for-gaps

Add test coverage for generate_firewall_config, generate_copilot_params, compute_effective_workspace, validate_checkout_list, and ExecutionResult

Author: jamesadevine

agentic-pipelines/src/compile/common.rs

@@ -484,6 +484,145 @@ pub fn generate_pipeline_path(output_path: &std::path::Path) -> String {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::compile::types::{McpConfig, McpOptions, Repository};
+
+    /// Helper: create a minimal FrontMatter by parsing YAML
+    fn minimal_front_matter() -> FrontMatter {
+        let (fm, _) = parse_markdown("---\nname: test-agent\ndescription: test\n---\n").unwrap();
+        fm
+    }
+
+    // ─── compute_effective_workspace ─────────────────────────────────────────
+
+    #[test]
+    fn test_workspace_explicit_root() {
+        let ws = compute_effective_workspace(&Some("root".to_string()), &[], "agent");
+        assert_eq!(ws, "root");
+    }
+
+    #[test]
+    fn test_workspace_explicit_repo_with_checkouts() {
+        let checkouts = vec!["other-repo".to_string()];
+        let ws = compute_effective_workspace(&Some("repo".to_string()), &checkouts, "agent");
+        assert_eq!(ws, "repo");
+    }
+
+    #[test]
+    fn test_workspace_implicit_root_no_checkouts() {
+        let ws = compute_effective_workspace(&None, &[], "agent");
+        assert_eq!(ws, "root");
+    }
+
+    #[test]
+    fn test_workspace_implicit_repo_with_checkouts() {
+        let checkouts = vec!["other-repo".to_string()];
+        let ws = compute_effective_workspace(&None, &checkouts, "agent");
+        assert_eq!(ws, "repo");
+    }
+
+    #[test]
+    fn test_workspace_explicit_repo_no_checkouts_still_returns_repo() {
+        // Emits a warning but still returns "repo"
+        let ws = compute_effective_workspace(&Some("repo".to_string()), &[], "agent");
+        assert_eq!(ws, "repo");
+    }
+
+    // ─── validate_checkout_list ───────────────────────────────────────────────
+
+    #[test]
+    fn test_validate_checkout_list_empty_is_ok() {
+        let result = validate_checkout_list(&[], &[]);
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn test_validate_checkout_list_valid_alias_passes() {
+        let repos = vec![Repository {
+            repository: "my-repo".to_string(),
+            repo_type: "git".to_string(),
+            name: "org/my-repo".to_string(),
+            repo_ref: "refs/heads/main".to_string(),
+        }];
+        let checkout = vec!["my-repo".to_string()];
+        let result = validate_checkout_list(&repos, &checkout);
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn test_validate_checkout_list_unknown_alias_fails() {
+        let repos = vec![Repository {
+            repository: "my-repo".to_string(),
+            repo_type: "git".to_string(),
+            name: "org/my-repo".to_string(),
+            repo_ref: "refs/heads/main".to_string(),
+        }];
+        let checkout = vec!["unknown-alias".to_string()];
+        let result = validate_checkout_list(&repos, &checkout);
+        assert!(result.is_err());
+        assert!(result.unwrap_err().to_string().contains("unknown-alias"));
+    }
+
+    #[test]
+    fn test_validate_checkout_list_empty_checkout_of_nonempty_repos_ok() {
+        let repos = vec![Repository {
+            repository: "my-repo".to_string(),
+            repo_type: "git".to_string(),
+            name: "org/my-repo".to_string(),
+            repo_ref: "refs/heads/main".to_string(),
+        }];
+        let result = validate_checkout_list(&repos, &[]);
+        assert!(result.is_ok());
+    }
+
+    // ─── generate_copilot_params ──────────────────────────────────────────────
+
+    #[test]
+    fn test_copilot_params_bash_wildcard() {
+        let mut fm = minimal_front_matter();
+        fm.tools = Some(crate::compile::types::ToolsConfig {
+            bash: Some(vec![":*".to_string()]),
+            edit: None,
+        });
+        let params = generate_copilot_params(&fm);
+        assert!(params.contains("--allow-tool \"shell(:*)\""));
+    }
+
+    #[test]
+    fn test_copilot_params_bash_disabled() {
+        let mut fm = minimal_front_matter();
+        fm.tools = Some(crate::compile::types::ToolsConfig {
+            bash: Some(vec![]),
+            edit: None,
+        });
+        let params = generate_copilot_params(&fm);
+        assert!(!params.contains("shell("));
+    }
+
+    #[test]
+    fn test_copilot_params_custom_mcp_not_added_with_mcp_flag() {
+        let mut fm = minimal_front_matter();
+        fm.mcp_servers.insert(
+            "my-tool".to_string(),
+            McpConfig::WithOptions(McpOptions {
+                command: Some("node".to_string()),
+                ..Default::default()
+            }),
+        );
+        let params = generate_copilot_params(&fm);
+        // Custom MCPs (with command) should NOT appear as --mcp flags
+        assert!(!params.contains("--mcp my-tool"));
+    }
+
+    #[test]
+    fn test_copilot_params_builtin_mcp_added_with_mcp_flag() {
+        let mut fm = minimal_front_matter();
+        fm.mcp_servers
+            .insert("ado".to_string(), McpConfig::Enabled(true));
+        let params = generate_copilot_params(&fm);
+        assert!(params.contains("--mcp ado"));
+    }
+
+    // ─── sanitize_filename ────────────────────────────────────────────────────
 
     #[test]
     fn test_sanitize_filename_basic() {

agentic-pipelines/src/compile/standalone.rs

@@ -498,4 +498,107 @@ fn generate_memory_prompt() -> String {
         .to_string()
 }
 
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::compile::common::parse_markdown;
+    use crate::compile::types::{McpConfig, McpOptions};
+
+    fn minimal_front_matter() -> FrontMatter {
+        let (fm, _) = parse_markdown("---\nname: test-agent\ndescription: test\n---\n").unwrap();
+        fm
+    }
+
+    #[test]
+    fn test_generate_firewall_config_builtin_simple_enabled() {
+        let mut fm = minimal_front_matter();
+        fm.mcp_servers
+            .insert("ado".to_string(), McpConfig::Enabled(true));
+        let config = generate_firewall_config(&fm);
+        let upstream = config.upstreams.get("ado").unwrap();
+        assert_eq!(upstream.command, "agency");
+        assert_eq!(upstream.args, vec!["mcp", "ado"]);
+        assert_eq!(upstream.allowed, vec!["*"]);
+    }
+
+    #[test]
+    fn test_generate_firewall_config_builtin_with_allowed_list() {
+        let mut fm = minimal_front_matter();
+        fm.mcp_servers.insert(
+            "icm".to_string(),
+            McpConfig::WithOptions(McpOptions {
+                allowed: vec!["create_incident".to_string(), "get_incident".to_string()],
+                ..Default::default()
+            }),
+        );
+        let config = generate_firewall_config(&fm);
+        let upstream = config.upstreams.get("icm").unwrap();
+        assert_eq!(upstream.command, "agency");
+        assert_eq!(upstream.args, vec!["mcp", "icm"]);
+        assert_eq!(
+            upstream.allowed,
+            vec!["create_incident".to_string(), "get_incident".to_string()]
+        );
+    }
+
+    #[test]
+    fn test_generate_firewall_config_custom_mcp() {
+        let mut fm = minimal_front_matter();
+        fm.mcp_servers.insert(
+            "my-tool".to_string(),
+            McpConfig::WithOptions(McpOptions {
+                command: Some("node".to_string()),
+                args: vec!["server.js".to_string()],
+                allowed: vec!["do_thing".to_string()],
+                ..Default::default()
+            }),
+        );
+        let config = generate_firewall_config(&fm);
+        let upstream = config.upstreams.get("my-tool").unwrap();
+        assert_eq!(upstream.command, "node");
+        assert_eq!(upstream.args, vec!["server.js"]);
+        assert_eq!(upstream.allowed, vec!["do_thing"]);
+    }
 
+    #[test]
+    fn test_generate_firewall_config_custom_mcp_empty_allowed_defaults_to_wildcard() {
+        let mut fm = minimal_front_matter();
+        fm.mcp_servers.insert(
+            "my-tool".to_string(),
+            McpConfig::WithOptions(McpOptions {
+                command: Some("python".to_string()),
+                allowed: vec![],
+                ..Default::default()
+            }),
+        );
+        let config = generate_firewall_config(&fm);
+        let upstream = config.upstreams.get("my-tool").unwrap();
+        assert_eq!(upstream.allowed, vec!["*"]);
+    }
+
+    #[test]
+    fn test_generate_firewall_config_unknown_non_builtin_skipped() {
+        // An MCP that is neither built-in nor has a command should be skipped
+        let mut fm = minimal_front_matter();
+        fm.mcp_servers
+            .insert("phantom".to_string(), McpConfig::Enabled(true));
+        let config = generate_firewall_config(&fm);
+        assert!(!config.upstreams.contains_key("phantom"));
+    }
+
+    #[test]
+    fn test_generate_firewall_config_disabled_mcp_skipped() {
+        let mut fm = minimal_front_matter();
+        fm.mcp_servers
+            .insert("ado".to_string(), McpConfig::Enabled(false));
+        let config = generate_firewall_config(&fm);
+        assert!(!config.upstreams.contains_key("ado"));
+    }
+
+    #[test]
+    fn test_generate_firewall_config_empty_mcp_servers() {
+        let fm = minimal_front_matter();
+        let config = generate_firewall_config(&fm);
+        assert!(config.upstreams.is_empty());
+    }
+}

agentic-pipelines/src/compile/types.rs

@@ -313,7 +313,7 @@ pub enum McpConfig {
 }
 
 /// Detailed MCP options
-#[derive(Debug, Deserialize, Clone)]
+#[derive(Debug, Deserialize, Clone, Default)]
 pub struct McpOptions {
     /// Custom command (if present, it's a custom MCP - standalone only)
     #[serde(default)]

agentic-pipelines/src/tools/result.rs

@@ -228,3 +228,47 @@ macro_rules! tool_result {
         }
     };
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_execution_result_success() {
+        let r = ExecutionResult::success("all good");
+        assert!(r.success);
+        assert_eq!(r.message, "all good");
+        assert!(r.data.is_none());
+    }
+
+    #[test]
+    fn test_execution_result_success_with_data() {
+        let data = serde_json::json!({"id": 42});
+        let r = ExecutionResult::success_with_data("created", data.clone());
+        assert!(r.success);
+        assert_eq!(r.message, "created");
+        assert_eq!(r.data, Some(data));
+    }
+
+    #[test]
+    fn test_execution_result_failure() {
+        let r = ExecutionResult::failure("something broke");
+        assert!(!r.success);
+        assert_eq!(r.message, "something broke");
+        assert!(r.data.is_none());
+    }
+
+    #[test]
+    fn test_anyhow_to_mcp_error_preserves_message() {
+        let err = anyhow::anyhow!("test error message");
+        let mcp_err = anyhow_to_mcp_error(err);
+        assert!(mcp_err.message.contains("test error message"));
+    }
+
+    #[test]
+    fn test_anyhow_to_mcp_error_uses_invalid_params_code() {
+        let err = anyhow::anyhow!("some error");
+        let mcp_err = anyhow_to_mcp_error(err);
+        assert_eq!(mcp_err.code, rmcp::model::ErrorCode::INVALID_PARAMS);
+    }
+}