diff --git a/server/src/http/mcp/story_tools/bug.rs b/server/src/http/mcp/story_tools/bug.rs index 5777c7c2..34f45441 100644 --- a/server/src/http/mcp/story_tools/bug.rs +++ b/server/src/http/mcp/story_tools/bug.rs @@ -44,9 +44,23 @@ pub(crate) fn tool_create_bug(args: &Value, ctx: &AppContext) -> Result> = args + let acceptance_criteria: Vec = args .get("acceptance_criteria") - .and_then(|v| serde_json::from_value(v.clone()).ok()); + .and_then(|v| serde_json::from_value(v.clone()).ok()) + .ok_or("Missing required argument: acceptance_criteria")?; + if acceptance_criteria.is_empty() { + return Err("acceptance_criteria must contain at least one entry".to_string()); + } + const JUNK_AC: &[&str] = &["", "todo", "tbd", "fixme", "xxx", "???"]; + let all_junk = acceptance_criteria + .iter() + .all(|ac| JUNK_AC.contains(&ac.trim().to_lowercase().as_str())); + if all_junk { + return Err( + "acceptance_criteria must contain at least one real entry (not just TODO/TBD/FIXME/XXX/???)." + .to_string(), + ); + } let depends_on: Option> = args .get("depends_on") .and_then(|v| serde_json::from_value(v.clone()).ok()); @@ -59,7 +73,7 @@ pub(crate) fn tool_create_bug(args: &Value, ctx: &AppContext) -> Result Result> = args + let acceptance_criteria: Vec = args .get("acceptance_criteria") - .and_then(|v| serde_json::from_value(v.clone()).ok()); + .and_then(|v| serde_json::from_value(v.clone()).ok()) + .ok_or("Missing required argument: acceptance_criteria")?; + if acceptance_criteria.is_empty() { + return Err("acceptance_criteria must contain at least one entry".to_string()); + } + const JUNK_AC: &[&str] = &["", "todo", "tbd", "fixme", "xxx", "???"]; + let all_junk = acceptance_criteria + .iter() + .all(|ac| JUNK_AC.contains(&ac.trim().to_lowercase().as_str())); + if all_junk { + return Err( + "acceptance_criteria must contain at least one real entry (not just TODO/TBD/FIXME/XXX/???)." + .to_string(), + ); + } let depends_on: Option> = args .get("depends_on") .and_then(|v| serde_json::from_value(v.clone()).ok()); @@ -41,7 +55,7 @@ pub(crate) fn tool_create_refactor(args: &Value, ctx: &AppContext) -> Result Result { )) .map_err(|e| format!("Serialization error: {e}")) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::http::test_helpers::test_ctx; + use serde_json::json; + + #[test] + fn tool_create_refactor_rejects_missing_acceptance_criteria() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_refactor(&json!({"name": "My Refactor"}), &ctx); + assert!(result.is_err()); + assert!( + result.unwrap_err().contains("acceptance_criteria"), + "error should mention acceptance_criteria" + ); + } + + #[test] + fn tool_create_refactor_rejects_empty_acceptance_criteria() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_refactor( + &json!({"name": "My Refactor", "acceptance_criteria": []}), + &ctx, + ); + assert!(result.is_err()); + assert!( + result.unwrap_err().contains("acceptance_criteria"), + "error should mention acceptance_criteria" + ); + } + + #[test] + fn tool_create_refactor_accepts_single_criterion() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_refactor( + &json!({"name": "Single Criterion Refactor", "acceptance_criteria": ["Code is clean"]}), + &ctx, + ); + assert!(result.is_ok(), "expected ok: {result:?}"); + assert!(result.unwrap().contains("Created refactor:")); + } + + #[test] + fn tool_create_refactor_rejects_all_junk_acceptance_criteria() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_refactor( + &json!({"name": "Junk Refactor", "acceptance_criteria": ["TODO", "TBD"]}), + &ctx, + ); + assert!(result.is_err()); + assert!( + result.unwrap_err().contains("real entry"), + "error should mention real entry" + ); + } + + #[test] + fn tool_create_refactor_accepts_mixed_junk_and_real_acceptance_criteria() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_refactor( + &json!({"name": "Mixed Refactor", "acceptance_criteria": ["TODO", "Real AC"]}), + &ctx, + ); + assert!(result.is_ok(), "expected ok for mixed AC: {result:?}"); + assert!(result.unwrap().contains("Created refactor:")); + } +} diff --git a/server/src/http/mcp/story_tools/spike.rs b/server/src/http/mcp/story_tools/spike.rs index d3ee8347..83cacaf7 100644 --- a/server/src/http/mcp/story_tools/spike.rs +++ b/server/src/http/mcp/story_tools/spike.rs @@ -29,9 +29,26 @@ pub(crate) fn tool_create_spike(args: &Value, ctx: &AppContext) -> Result = args + .get("acceptance_criteria") + .and_then(|v| serde_json::from_value(v.clone()).ok()) + .ok_or("Missing required argument: acceptance_criteria")?; + if acceptance_criteria.is_empty() { + return Err("acceptance_criteria must contain at least one entry".to_string()); + } + const JUNK_AC: &[&str] = &["", "todo", "tbd", "fixme", "xxx", "???"]; + let all_junk = acceptance_criteria + .iter() + .all(|ac| JUNK_AC.contains(&ac.trim().to_lowercase().as_str())); + if all_junk { + return Err( + "acceptance_criteria must contain at least one real entry (not just TODO/TBD/FIXME/XXX/???)." + .to_string(), + ); + } let root = ctx.state.get_project_root()?; - let spike_id = create_spike_file(&root, name, description)?; + let spike_id = create_spike_file(&root, name, description, &acceptance_criteria)?; Ok(format!("Created spike: {spike_id}")) } @@ -72,7 +89,8 @@ mod tests { fn tool_create_spike_rejects_empty_name() { let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); - let result = tool_create_spike(&json!({"name": "!!!"}), &ctx); + let result = + tool_create_spike(&json!({"name": "!!!", "acceptance_criteria": ["AC"]}), &ctx); assert!(result.is_err()); assert!(result.unwrap_err().contains("alphanumeric")); } @@ -83,7 +101,11 @@ mod tests { let ctx = test_ctx(tmp.path()); let result = tool_create_spike( - &json!({"name": "Compare Encoders", "description": "Which encoder is fastest?"}), + &json!({ + "name": "Compare Encoders", + "description": "Which encoder is fastest?", + "acceptance_criteria": ["Encoder comparison is documented"] + }), &ctx, ) .unwrap(); @@ -105,7 +127,11 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let ctx = test_ctx(tmp.path()); - let result = tool_create_spike(&json!({"name": "My Spike"}), &ctx).unwrap(); + let result = tool_create_spike( + &json!({"name": "My Spike", "acceptance_criteria": ["Spike findings documented"]}), + &ctx, + ) + .unwrap(); assert!( result.contains("_spike_my_spike"), "result should contain spike ID: {result}" @@ -118,4 +144,70 @@ mod tests { assert!(contents.starts_with("---\nname: \"My Spike\"\n---")); assert!(contents.contains("## Question\n\n- TBD\n")); } + + #[test] + fn tool_create_spike_rejects_missing_acceptance_criteria() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_spike(&json!({"name": "My Spike"}), &ctx); + assert!(result.is_err()); + assert!( + result.unwrap_err().contains("acceptance_criteria"), + "error should mention acceptance_criteria" + ); + } + + #[test] + fn tool_create_spike_rejects_empty_acceptance_criteria() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_spike( + &json!({"name": "My Spike", "acceptance_criteria": []}), + &ctx, + ); + assert!(result.is_err()); + assert!( + result.unwrap_err().contains("acceptance_criteria"), + "error should mention acceptance_criteria" + ); + } + + #[test] + fn tool_create_spike_accepts_single_criterion() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_spike( + &json!({"name": "Single Criterion Spike", "acceptance_criteria": ["Findings documented"]}), + &ctx, + ); + assert!(result.is_ok(), "expected ok: {result:?}"); + assert!(result.unwrap().contains("Created spike:")); + } + + #[test] + fn tool_create_spike_rejects_all_junk_acceptance_criteria() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_spike( + &json!({"name": "Junk Spike", "acceptance_criteria": ["TODO", "TBD"]}), + &ctx, + ); + assert!(result.is_err()); + assert!( + result.unwrap_err().contains("real entry"), + "error should mention real entry" + ); + } + + #[test] + fn tool_create_spike_accepts_mixed_junk_and_real_acceptance_criteria() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = test_ctx(tmp.path()); + let result = tool_create_spike( + &json!({"name": "Mixed Spike", "acceptance_criteria": ["TODO", "Real AC"]}), + &ctx, + ); + assert!(result.is_ok(), "expected ok for mixed AC: {result:?}"); + assert!(result.unwrap().contains("Created spike:")); + } } diff --git a/server/src/http/mcp/story_tools/story.rs b/server/src/http/mcp/story_tools/story.rs index cc059bbe..2a05ce65 100644 --- a/server/src/http/mcp/story_tools/story.rs +++ b/server/src/http/mcp/story_tools/story.rs @@ -30,9 +30,23 @@ pub(crate) fn tool_create_story(args: &Value, ctx: &AppContext) -> Result> = args + let acceptance_criteria: Vec = args .get("acceptance_criteria") - .and_then(|v| serde_json::from_value(v.clone()).ok()); + .and_then(|v| serde_json::from_value(v.clone()).ok()) + .ok_or("Missing required argument: acceptance_criteria")?; + if acceptance_criteria.is_empty() { + return Err("acceptance_criteria must contain at least one entry".to_string()); + } + const JUNK_AC: &[&str] = &["", "todo", "tbd", "fixme", "xxx", "???"]; + let all_junk = acceptance_criteria + .iter() + .all(|ac| JUNK_AC.contains(&ac.trim().to_lowercase().as_str())); + if all_junk { + return Err( + "acceptance_criteria must contain at least one real entry (not just TODO/TBD/FIXME/XXX/???)." + .to_string(), + ); + } let depends_on: Option> = args .get("depends_on") .and_then(|v| serde_json::from_value(v.clone()).ok()); @@ -46,7 +60,7 @@ pub(crate) fn tool_create_story(args: &Value, ctx: &AppContext) -> Result) -> JsonRpcResponse { "acceptance_criteria": { "type": "array", "items": { "type": "string" }, - "description": "Optional list of acceptance criteria" + "minItems": 1, + "description": "List of acceptance criteria (at least one required)" }, "depends_on": { "type": "array", @@ -42,7 +43,7 @@ pub(super) fn handle_tools_list(id: Option) -> JsonRpcResponse { "description": "If true, git-add and git-commit the new story file to the current branch" } }, - "required": ["name"] + "required": ["name", "acceptance_criteria"] } }, { @@ -447,9 +448,15 @@ pub(super) fn handle_tools_list(id: Option) -> JsonRpcResponse { "description": { "type": "string", "description": "Optional description / question the spike aims to answer" + }, + "acceptance_criteria": { + "type": "array", + "items": { "type": "string" }, + "minItems": 1, + "description": "List of acceptance criteria (at least one required)" } }, - "required": ["name"] + "required": ["name", "acceptance_criteria"] } }, { @@ -481,7 +488,8 @@ pub(super) fn handle_tools_list(id: Option) -> JsonRpcResponse { "acceptance_criteria": { "type": "array", "items": { "type": "string" }, - "description": "Optional list of acceptance criteria for the fix" + "minItems": 1, + "description": "List of acceptance criteria for the fix (at least one required)" }, "depends_on": { "type": "array", @@ -489,7 +497,7 @@ pub(super) fn handle_tools_list(id: Option) -> JsonRpcResponse { "description": "Optional list of story numbers this bug depends on (e.g. [42, 43]). Persisted as depends_on in YAML front matter." } }, - "required": ["name", "description", "steps_to_reproduce", "actual_result", "expected_result"] + "required": ["name", "description", "steps_to_reproduce", "actual_result", "expected_result", "acceptance_criteria"] } }, { @@ -517,7 +525,8 @@ pub(super) fn handle_tools_list(id: Option) -> JsonRpcResponse { "acceptance_criteria": { "type": "array", "items": { "type": "string" }, - "description": "Optional list of acceptance criteria" + "minItems": 1, + "description": "List of acceptance criteria (at least one required)" }, "depends_on": { "type": "array", @@ -525,7 +534,7 @@ pub(super) fn handle_tools_list(id: Option) -> JsonRpcResponse { "description": "Optional list of story numbers this refactor depends on (e.g. [42, 43]). Persisted as depends_on in YAML front matter." } }, - "required": ["name"] + "required": ["name", "acceptance_criteria"] } }, { diff --git a/server/src/http/workflow/bug_ops.rs b/server/src/http/workflow/bug_ops.rs index c9dac045..a065a91a 100644 --- a/server/src/http/workflow/bug_ops.rs +++ b/server/src/http/workflow/bug_ops.rs @@ -71,6 +71,7 @@ pub fn create_spike_file( root: &Path, name: &str, description: Option<&str>, + acceptance_criteria: &[String], ) -> Result { let spike_number = next_item_number(root)?; let slug = slugify_name(name); @@ -103,7 +104,15 @@ pub fn create_spike_file( content.push_str("## Findings\n\n"); content.push_str("- TBD\n\n"); content.push_str("## Recommendation\n\n"); - content.push_str("- TBD\n"); + content.push_str("- TBD\n\n"); + content.push_str("## Acceptance Criteria\n\n"); + if acceptance_criteria.is_empty() { + content.push_str("- [ ] TBD\n"); + } else { + for criterion in acceptance_criteria { + content.push_str(&format!("- [ ] {criterion}\n")); + } + } // Write to database content store and CRDT. write_story_content(root, &spike_id, "1_backlog", &content); @@ -511,7 +520,7 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let spike_id = - create_spike_file(tmp.path(), "Filesystem Watcher Architecture", None).unwrap(); + create_spike_file(tmp.path(), "Filesystem Watcher Architecture", None, &[]).unwrap(); assert!( spike_id.ends_with("_spike_filesystem_watcher_architecture"), @@ -549,7 +558,7 @@ mod tests { let description = "What is the best approach for watching filesystem events?"; let spike_id = - create_spike_file(tmp.path(), "FS Watcher Spike", Some(description)).unwrap(); + create_spike_file(tmp.path(), "FS Watcher Spike", Some(description), &[]).unwrap(); let contents = crate::db::read_content(&spike_id) .or_else(|| { @@ -565,7 +574,7 @@ mod tests { #[test] fn create_spike_file_uses_placeholder_when_no_description() { let tmp = tempfile::tempdir().unwrap(); - let spike_id = create_spike_file(tmp.path(), "My Spike", None).unwrap(); + let spike_id = create_spike_file(tmp.path(), "My Spike", None, &[]).unwrap(); let contents = crate::db::read_content(&spike_id) .or_else(|| { @@ -581,7 +590,7 @@ mod tests { #[test] fn create_spike_file_rejects_empty_name() { let tmp = tempfile::tempdir().unwrap(); - let result = create_spike_file(tmp.path(), "!!!", None); + let result = create_spike_file(tmp.path(), "!!!", None, &[]); assert!(result.is_err()); assert!(result.unwrap_err().contains("alphanumeric")); } @@ -590,7 +599,7 @@ mod tests { fn create_spike_file_with_special_chars_in_name_produces_valid_yaml() { let tmp = tempfile::tempdir().unwrap(); let name = "Spike: compare \"fast\" vs slow encoders"; - let result = create_spike_file(tmp.path(), name, None); + let result = create_spike_file(tmp.path(), name, None, &[]); assert!(result.is_ok(), "create_spike_file failed: {result:?}"); let spike_id = result.unwrap(); @@ -616,7 +625,7 @@ mod tests { "---\nname: Existing\n---\n", ); - let spike_id = create_spike_file(tmp.path(), "My Spike", None).unwrap(); + let spike_id = create_spike_file(tmp.path(), "My Spike", None, &[]).unwrap(); assert!( spike_id.ends_with("_spike_my_spike"), "expected ID to end with _spike_my_spike, got: {spike_id}"