huskies: merge 677_refactor_reject_promotion_to_current_coder_of_work_items_with_junk_only_acceptance_criteria

2026-04-27 16:26:15 +00:00
parent 5884dac825
commit 39a9766d7d
6 changed files with 431 additions and 30 deletions
@@ -44,9 +44,23 @@ pub(crate) fn tool_create_bug(args: &Value, ctx: &AppContext) -> Result<String,
        .get("expected_result")
        .and_then(|v| v.as_str())
        .ok_or("Missing required argument: expected_result")?;
-    let acceptance_criteria: Option<Vec<String>> = args
+    let acceptance_criteria: Vec<String> = 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<Vec<u32>> = 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<String,
        steps_to_reproduce,
        actual_result,
        expected_result,
-        acceptance_criteria.as_deref(),
+        Some(&acceptance_criteria),
        depends_on.as_deref(),
    )?;

@@ -236,7 +250,8 @@ mod tests {
                "description": "The app crashes on login.",
                "steps_to_reproduce": "1. Open app\n2. Click login",
                "actual_result": "500 error",
-                "expected_result": "Successful login"
+                "expected_result": "Successful login",
+                "acceptance_criteria": ["Login succeeds without error"]
            }),
            &ctx,
        )
@@ -300,6 +315,111 @@ mod tests {
        );
    }

+    #[test]
+    fn tool_create_bug_rejects_missing_acceptance_criteria() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_bug(
+            &json!({
+                "name": "Some Bug",
+                "description": "d",
+                "steps_to_reproduce": "s",
+                "actual_result": "a",
+                "expected_result": "e"
+            }),
+            &ctx,
+        );
+        assert!(result.is_err());
+        assert!(
+            result.unwrap_err().contains("acceptance_criteria"),
+            "error should mention acceptance_criteria"
+        );
+    }
+
+    #[test]
+    fn tool_create_bug_rejects_empty_acceptance_criteria() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_bug(
+            &json!({
+                "name": "Some Bug",
+                "description": "d",
+                "steps_to_reproduce": "s",
+                "actual_result": "a",
+                "expected_result": "e",
+                "acceptance_criteria": []
+            }),
+            &ctx,
+        );
+        assert!(result.is_err());
+        assert!(
+            result.unwrap_err().contains("acceptance_criteria"),
+            "error should mention acceptance_criteria"
+        );
+    }
+
+    #[test]
+    fn tool_create_bug_accepts_single_criterion() {
+        let tmp = tempfile::tempdir().unwrap();
+        setup_git_repo_in(tmp.path());
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_bug(
+            &json!({
+                "name": "Single Criterion Bug",
+                "description": "d",
+                "steps_to_reproduce": "s",
+                "actual_result": "a",
+                "expected_result": "e",
+                "acceptance_criteria": ["Bug is fixed"]
+            }),
+            &ctx,
+        );
+        assert!(result.is_ok(), "expected ok: {result:?}");
+        assert!(result.unwrap().contains("Created bug:"));
+    }
+
+    #[test]
+    fn tool_create_bug_rejects_all_junk_acceptance_criteria() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_bug(
+            &json!({
+                "name": "Junk Bug",
+                "description": "d",
+                "steps_to_reproduce": "s",
+                "actual_result": "a",
+                "expected_result": "e",
+                "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_bug_accepts_mixed_junk_and_real_acceptance_criteria() {
+        let tmp = tempfile::tempdir().unwrap();
+        setup_git_repo_in(tmp.path());
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_bug(
+            &json!({
+                "name": "Mixed Bug",
+                "description": "d",
+                "steps_to_reproduce": "s",
+                "actual_result": "a",
+                "expected_result": "e",
+                "acceptance_criteria": ["TODO", "Real AC"]
+            }),
+            &ctx,
+        );
+        assert!(result.is_ok(), "expected ok for mixed AC: {result:?}");
+        assert!(result.unwrap().contains("Created bug:"));
+    }
+
    #[test]
    fn tool_close_bug_missing_bug_id() {
        let tmp = tempfile::tempdir().unwrap();
@@ -29,9 +29,23 @@ pub(crate) fn tool_create_refactor(args: &Value, ctx: &AppContext) -> Result<Str
        .and_then(|v| v.as_str())
        .ok_or("Missing required argument: name")?;
    let description = args.get("description").and_then(|v| v.as_str());
-    let acceptance_criteria: Option<Vec<String>> = args
+    let acceptance_criteria: Vec<String> = 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<Vec<u32>> = 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<Str
        &root,
        name,
        description,
-        acceptance_criteria.as_deref(),
+        Some(&acceptance_criteria),
        depends_on.as_deref(),
    )?;

@@ -59,3 +73,76 @@ pub(crate) fn tool_list_refactors(ctx: &AppContext) -> Result<String, String> {
    ))
    .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:"));
+    }
+}
@@ -29,9 +29,26 @@ pub(crate) fn tool_create_spike(args: &Value, ctx: &AppContext) -> Result<String
        .and_then(|v| v.as_str())
        .ok_or("Missing required argument: name")?;
    let description = args.get("description").and_then(|v| v.as_str());
+    let acceptance_criteria: Vec<String> = 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:"));
+    }
 }
@@ -30,9 +30,23 @@ pub(crate) fn tool_create_story(args: &Value, ctx: &AppContext) -> Result<String
        .ok_or("Missing required argument: name")?;
    let user_story = args.get("user_story").and_then(|v| v.as_str());
    let description = args.get("description").and_then(|v| v.as_str());
-    let acceptance_criteria: Option<Vec<String>> = args
+    let acceptance_criteria: Vec<String> = 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<Vec<u32>> = 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<String
        name,
        user_story,
        description,
-        acceptance_criteria.as_deref(),
+        Some(&acceptance_criteria),
        depends_on.as_deref(),
        commit,
    )?;
@@ -498,7 +512,10 @@ mod tests {
    fn tool_create_story_rejects_empty_name() {
        let tmp = tempfile::tempdir().unwrap();
        let ctx = test_ctx(tmp.path());
-        let result = tool_create_story(&json!({"name": "!!!"}), &ctx);
+        let result = tool_create_story(
+            &json!({"name": "!!!", "acceptance_criteria": ["AC1"]}),
+            &ctx,
+        );
        assert!(result.is_err());
        assert!(result.unwrap_err().contains("alphanumeric"));
    }
@@ -512,6 +529,72 @@ mod tests {
        assert!(result.unwrap_err().contains("Missing required argument"));
    }

+    #[test]
+    fn tool_create_story_rejects_missing_acceptance_criteria() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_story(&json!({"name": "My Story"}), &ctx);
+        assert!(result.is_err());
+        assert!(
+            result.unwrap_err().contains("acceptance_criteria"),
+            "error should mention acceptance_criteria"
+        );
+    }
+
+    #[test]
+    fn tool_create_story_rejects_empty_acceptance_criteria() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_story(
+            &json!({"name": "My Story", "acceptance_criteria": []}),
+            &ctx,
+        );
+        assert!(result.is_err());
+        assert!(
+            result.unwrap_err().contains("acceptance_criteria"),
+            "error should mention acceptance_criteria"
+        );
+    }
+
+    #[test]
+    fn tool_create_story_accepts_single_criterion() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_story(
+            &json!({"name": "Single Criterion Story", "acceptance_criteria": ["It works"]}),
+            &ctx,
+        );
+        assert!(result.is_ok(), "expected ok: {result:?}");
+        assert!(result.unwrap().contains("Created story:"));
+    }
+
+    #[test]
+    fn tool_create_story_rejects_all_junk_acceptance_criteria() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_story(
+            &json!({"name": "Junk Story", "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_story_accepts_mixed_junk_and_real_acceptance_criteria() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_create_story(
+            &json!({"name": "Mixed Story", "acceptance_criteria": ["TODO", "Real AC"]}),
+            &ctx,
+        );
+        assert!(result.is_ok(), "expected ok for mixed AC: {result:?}");
+        assert!(result.unwrap().contains("Created story:"));
+    }
+
    #[test]
    fn tool_create_story_description_is_written_to_file() {
        let tmp = tempfile::tempdir().unwrap();
@@ -520,7 +603,8 @@ mod tests {
        let result = tool_create_story(
            &json!({
                "name": "Story With Description",
-                "description": "This is the background context."
+                "description": "This is the background context.",
+                "acceptance_criteria": ["Described well"]
            }),
            &ctx,
        )
@@ -30,7 +30,8 @@ pub(super) fn handle_tools_list(id: Option<Value>) -> 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<Value>) -> 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<Value>) -> 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<Value>) -> 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<Value>) -> 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<Value>) -> 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<Value>) -> 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"]
                    }
                },
                {