From 7fa31c03a36d849a5a30bb84d460fb85c26967ef Mon Sep 17 00:00:00 2001 From: dave Date: Wed, 15 Apr 2026 13:19:17 +0000 Subject: [PATCH] huskies: merge 573_story_remove_criterion_mcp_tool_to_delete_an_acceptance_criterion --- server/src/http/mcp/mod.rs | 22 +++++- server/src/http/mcp/story_tools.rs | 82 ++++++++++++++++++++- server/src/http/workflow/mod.rs | 2 +- server/src/http/workflow/story_ops.rs | 102 ++++++++++++++++++++++++++ 4 files changed, 204 insertions(+), 4 deletions(-) diff --git a/server/src/http/mcp/mod.rs b/server/src/http/mcp/mod.rs index 8debde48..bcb087a1 100644 --- a/server/src/http/mcp/mod.rs +++ b/server/src/http/mcp/mod.rs @@ -553,6 +553,24 @@ fn handle_tools_list(id: Option) -> 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, 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] diff --git a/server/src/http/mcp/story_tools.rs b/server/src/http/mcp/story_tools.rs index 9a1a90ec..ebad1416 100644 --- a/server/src/http/mcp/story_tools.rs +++ b/server/src/http/mcp/story_tools.rs @@ -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 Result { + 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 { 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. /// diff --git a/server/src/http/workflow/mod.rs b/server/src/http/workflow/mod.rs index c3d05293..80018f19 100644 --- a/server/src/http/workflow/mod.rs +++ b/server/src/http/workflow/mod.rs @@ -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, diff --git a/server/src/http/workflow/story_ops.rs b/server/src/http/workflow/story_ops.rs index 8f15178f..c9552acb 100644 --- a/server/src/http/workflow/story_ops.rs +++ b/server/src/http/workflow/story_ops.rs @@ -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 = 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]