huskies: merge 573_story_remove_criterion_mcp_tool_to_delete_an_acceptance_criterion

2026-04-15 13:19:17 +00:00
parent 483489cc44
commit 7fa31c03a3
4 changed files with 204 additions and 4 deletions
@@ -553,6 +553,24 @@ fn handle_tools_list(id: Option<Value>) -> JsonRpcResponse {
                        "required": ["story_id", "criterion"]
                    }
                },
+                {
+                    "name": "remove_criterion",
+                    "description": "Remove an acceptance criterion from a story by its 0-based index (counting all criteria, both checked and unchecked). Returns an error if the index is out of range. Auto-commits via the filesystem watcher.",
+                    "inputSchema": {
+                        "type": "object",
+                        "properties": {
+                            "story_id": {
+                                "type": "string",
+                                "description": "Story identifier (filename stem, e.g. '28_my_story')"
+                            },
+                            "criterion_index": {
+                                "type": "integer",
+                                "description": "0-based index of the criterion to remove (counts all criteria)"
+                            }
+                        },
+                        "required": ["story_id", "criterion_index"]
+                    }
+                },
                {
                    "name": "update_story",
                    "description": "Update an existing story file. Can replace the '## User Story' and/or '## Description' section content, and/or set YAML front matter fields (e.g. agent, qa). Auto-commits via the filesystem watcher.",
@@ -1266,6 +1284,7 @@ async fn handle_tools_call(id: Option<Value>, params: &Value, ctx: &AppContext)
        "check_criterion" => story_tools::tool_check_criterion(&args, ctx),
        "edit_criterion" => story_tools::tool_edit_criterion(&args, ctx),
        "add_criterion" => story_tools::tool_add_criterion(&args, ctx),
+        "remove_criterion" => story_tools::tool_remove_criterion(&args, ctx),
        "update_story" => story_tools::tool_update_story(&args, ctx),
        // Spike lifecycle tools
        "create_spike" => story_tools::tool_create_spike(&args, ctx),
@@ -1449,7 +1468,8 @@ mod tests {
        assert!(names.contains(&"loc_file"));
        assert!(names.contains(&"dump_crdt"));
        assert!(names.contains(&"get_version"));
-        assert_eq!(tools.len(), 64);
+        assert!(names.contains(&"remove_criterion"));
+        assert_eq!(tools.len(), 65);
    }

    #[test]
@@ -6,8 +6,8 @@ use crate::http::context::AppContext;
 use crate::http::workflow::{
    add_criterion_to_file, check_criterion_in_file, create_bug_file, create_refactor_file,
    create_spike_file, create_story_file, edit_criterion_in_file, list_bug_files,
-    list_refactor_files, load_pipeline_state, load_upcoming_stories, update_story_in_file,
-    validate_story_dirs,
+    list_refactor_files, load_pipeline_state, load_upcoming_stories, remove_criterion_from_file,
+    update_story_in_file, validate_story_dirs,
 };
 use crate::io::story_metadata::{
    check_archived_deps, check_archived_deps_from_list, parse_front_matter, parse_unchecked_todos,
@@ -372,6 +372,24 @@ pub(super) fn tool_add_criterion(args: &Value, ctx: &AppContext) -> Result<Strin
    ))
 }

+pub(super) fn tool_remove_criterion(args: &Value, ctx: &AppContext) -> Result<String, String> {
+    let story_id = args
+        .get("story_id")
+        .and_then(|v| v.as_str())
+        .ok_or("Missing required argument: story_id")?;
+    let criterion_index = args
+        .get("criterion_index")
+        .and_then(|v| v.as_u64())
+        .ok_or("Missing required argument: criterion_index")? as usize;
+
+    let root = ctx.state.get_project_root()?;
+    remove_criterion_from_file(&root, story_id, criterion_index)?;
+
+    Ok(format!(
+        "Removed criterion {criterion_index} from story '{story_id}'."
+    ))
+}
+
 pub(super) fn tool_update_story(args: &Value, ctx: &AppContext) -> Result<String, String> {
    let story_id = args
        .get("story_id")
@@ -1745,6 +1763,66 @@ mod tests {
        assert!(result.unwrap().contains("Criterion 0 checked"));
    }

+    #[test]
+    fn tool_remove_criterion_missing_story_id() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_remove_criterion(&json!({"criterion_index": 0}), &ctx);
+        assert!(result.is_err());
+        assert!(result.unwrap_err().contains("story_id"));
+    }
+
+    #[test]
+    fn tool_remove_criterion_missing_criterion_index() {
+        let tmp = tempfile::tempdir().unwrap();
+        let ctx = test_ctx(tmp.path());
+        let result = tool_remove_criterion(&json!({"story_id": "1_test"}), &ctx);
+        assert!(result.is_err());
+        assert!(result.unwrap_err().contains("criterion_index"));
+    }
+
+    #[test]
+    fn tool_remove_criterion_removes_item() {
+        let tmp = tempfile::tempdir().unwrap();
+        setup_git_repo_in(tmp.path());
+
+        crate::db::ensure_content_store();
+        crate::db::write_item_with_content(
+            "9905_test",
+            "2_current",
+            "---\nname: Test\n---\n## Acceptance Criteria\n- [ ] Keep me\n- [ ] Remove me\n",
+        );
+
+        let ctx = test_ctx(tmp.path());
+        let result = tool_remove_criterion(
+            &json!({"story_id": "9905_test", "criterion_index": 1}),
+            &ctx,
+        );
+        assert!(result.is_ok(), "Expected ok: {result:?}");
+        assert!(result.unwrap().contains("Removed criterion 1"));
+    }
+
+    #[test]
+    fn tool_remove_criterion_out_of_range() {
+        let tmp = tempfile::tempdir().unwrap();
+        setup_git_repo_in(tmp.path());
+
+        crate::db::ensure_content_store();
+        crate::db::write_item_with_content(
+            "9906_test",
+            "2_current",
+            "---\nname: Test\n---\n## Acceptance Criteria\n- [ ] Only one\n",
+        );
+
+        let ctx = test_ctx(tmp.path());
+        let result = tool_remove_criterion(
+            &json!({"story_id": "9906_test", "criterion_index": 5}),
+            &ctx,
+        );
+        assert!(result.is_err());
+        assert!(result.unwrap_err().contains("out of range"));
+    }
+
    /// Regression test for bug 514: deleting a story must cancel its pending
    /// rate-limit retry timer so the tick loop cannot re-spawn an agent.
    ///
@@ -8,7 +8,7 @@ pub use bug_ops::{
 };
 pub use story_ops::{
    add_criterion_to_file, check_criterion_in_file, create_story_file, edit_criterion_in_file,
-    update_story_in_file,
+    remove_criterion_from_file, update_story_in_file,
 };
 pub use test_results::{
    read_test_results_from_story_file, write_coverage_baseline_to_story_file,
@@ -126,6 +126,53 @@ pub fn check_criterion_in_file(
    Ok(())
 }

+/// Remove an acceptance criterion from a story by its 0-based index (counting all criteria,
+/// both checked and unchecked). Returns an error if the index is out of range.
+pub fn remove_criterion_from_file(
+    project_root: &Path,
+    story_id: &str,
+    criterion_index: usize,
+) -> Result<(), String> {
+    let contents = read_story_content(project_root, story_id)?;
+
+    let mut count: usize = 0;
+    let mut found = false;
+    let new_lines: Vec<String> = contents
+        .lines()
+        .filter(|line| {
+            let trimmed = line.trim();
+            if trimmed.starts_with("- [ ] ") || trimmed.starts_with("- [x] ") {
+                if count == criterion_index {
+                    count += 1;
+                    found = true;
+                    return false;
+                }
+                count += 1;
+            }
+            true
+        })
+        .map(|s| s.to_string())
+        .collect();
+
+    if !found {
+        return Err(format!(
+            "Criterion index {criterion_index} out of range. Story '{story_id}' has \
+             {count} criteria (indices 0..{}).",
+            count.saturating_sub(1)
+        ));
+    }
+
+    let mut new_str = new_lines.join("\n");
+    if contents.ends_with('\n') {
+        new_str.push('\n');
+    }
+
+    let stage = story_stage(story_id).unwrap_or_else(|| "2_current".to_string());
+    write_story_content(project_root, story_id, &stage, &new_str);
+
+    Ok(())
+}
+
 /// Edit the text of an existing acceptance criterion without changing its checked state.
 ///
 /// Finds the criterion at `criterion_index` (0-based, counting all criteria regardless
@@ -578,6 +625,61 @@ mod tests {
        assert!(result.unwrap_err().contains("Acceptance Criteria"));
    }

+    // ── remove_criterion_from_file tests ──────────────────────────────────────
+
+    #[test]
+    fn remove_criterion_removes_by_index() {
+        let tmp = tempfile::tempdir().unwrap();
+        setup_story_in_fs(
+            tmp.path(),
+            "20_remove_test",
+            &story_with_ac_section(&["First", "Second", "Third"]),
+        );
+
+        remove_criterion_from_file(tmp.path(), "20_remove_test", 1).unwrap();
+
+        let contents = read_story_content(tmp.path(), "20_remove_test").unwrap();
+        assert!(contents.contains("- [ ] First"), "First should remain");
+        assert!(!contents.contains("Second"), "Second should be removed");
+        assert!(contents.contains("- [ ] Third"), "Third should remain");
+    }
+
+    #[test]
+    fn remove_criterion_shifts_indices() {
+        let tmp = tempfile::tempdir().unwrap();
+        setup_story_in_fs(
+            tmp.path(),
+            "21_remove_test",
+            &story_with_ac_section(&["A", "B", "C"]),
+        );
+
+        remove_criterion_from_file(tmp.path(), "21_remove_test", 0).unwrap();
+
+        let contents = read_story_content(tmp.path(), "21_remove_test").unwrap();
+        assert!(!contents.contains("- [ ] A"), "A should be removed");
+        assert!(contents.contains("- [ ] B"), "B should remain");
+        assert!(contents.contains("- [ ] C"), "C should remain");
+        // B is now at index 0, C at index 1 — verify by removing B next
+        remove_criterion_from_file(tmp.path(), "21_remove_test", 0).unwrap();
+        let contents2 = read_story_content(tmp.path(), "21_remove_test").unwrap();
+        assert!(!contents2.contains("- [ ] B"), "B should now be removed");
+        assert!(contents2.contains("- [ ] C"), "C should still remain");
+    }
+
+    #[test]
+    fn remove_criterion_out_of_range_returns_error() {
+        let tmp = tempfile::tempdir().unwrap();
+        setup_story_in_fs(
+            tmp.path(),
+            "22_remove_test",
+            &story_with_ac_section(&["Only"]),
+        );
+
+        let result = remove_criterion_from_file(tmp.path(), "22_remove_test", 5);
+        assert!(result.is_err(), "should fail for out-of-range index");
+        assert!(result.unwrap_err().contains("out of range"));
+    }
+
    // ── update_story_in_file tests ─────────────────────────────────────────────

    #[test]