From 3891de685cd1b0f14d567737b605be3fadf8c50b Mon Sep 17 00:00:00 2001 From: dave Date: Tue, 12 May 2026 15:43:02 +0000 Subject: [PATCH] huskies: merge 888 --- server/src/chat/commands/depends.rs | 43 +++++++++ .../src/http/mcp/story_tools/story/update.rs | 96 +++++++++++++++++++ server/src/http/workflow/utils.rs | 14 +-- 3 files changed, 147 insertions(+), 6 deletions(-) diff --git a/server/src/chat/commands/depends.rs b/server/src/chat/commands/depends.rs index 97714de5..2d396e8c 100644 --- a/server/src/chat/commands/depends.rs +++ b/server/src/chat/commands/depends.rs @@ -272,6 +272,49 @@ mod tests { ); } + /// AC4 regression (chat path): set [1,2,3] → clear → replace [4,5]. + /// Verifies each write is reflected in CRDT and that replace does not append. + #[test] + fn depends_set_clear_replace_regression() { + let tmp = tempfile::TempDir::new().unwrap(); + crate::crdt_state::init_for_test(); + write_story_file( + tmp.path(), + "1_backlog", + "9920_story_scr.md", + "---\nname: SCR\n---\n", + ); + + // Set to [1, 2, 3]. + let out = depends_cmd_with_root(tmp.path(), "9920 1 2 3").unwrap(); + assert!(out.contains("1"), "response should mention dep 1: {out}"); + let view = crate::crdt_state::read_item("9920_story_scr").expect("CRDT must have story"); + assert_eq!( + view.depends_on, + Some(vec![1, 2, 3]), + "CRDT should hold [1,2,3]: {view:?}" + ); + + // Clear. + let out = depends_cmd_with_root(tmp.path(), "9920").unwrap(); + assert!(out.contains("Cleared"), "clear should confirm: {out}"); + let view = crate::crdt_state::read_item("9920_story_scr").expect("CRDT must have story"); + assert_eq!( + view.depends_on, None, + "CRDT should be None after clear: {view:?}" + ); + + // Replace with [4, 5] — must not append to old list. + let out = depends_cmd_with_root(tmp.path(), "9920 4 5").unwrap(); + assert!(out.contains("4"), "response should mention dep 4: {out}"); + let view = crate::crdt_state::read_item("9920_story_scr").expect("CRDT must have story"); + assert_eq!( + view.depends_on, + Some(vec![4, 5]), + "CRDT should hold exactly [4,5] after replace: {view:?}" + ); + } + #[test] fn depends_finds_story_in_any_stage() { let tmp = tempfile::TempDir::new().unwrap(); diff --git a/server/src/http/mcp/story_tools/story/update.rs b/server/src/http/mcp/story_tools/story/update.rs index ce4172a6..b63cea1f 100644 --- a/server/src/http/mcp/story_tools/story/update.rs +++ b/server/src/http/mcp/story_tools/story/update.rs @@ -212,6 +212,102 @@ mod tests { ); } + /// AC4 regression: set [1,2,3] → clear [] → replace [4,5] — CRDT reflects + /// each write, and replace never appends. + #[test] + fn tool_update_story_depends_on_set_clear_replace() { + let tmp = tempfile::tempdir().unwrap(); + crate::crdt_state::init_for_test(); + setup_story_for_update( + tmp.path(), + "888_deps_scr", + "---\nname: Deps SCR\n---\n\nNo sections.\n", + ); + let ctx = test_ctx(tmp.path()); + + // Set to [1, 2, 3]. + let r = tool_update_story( + &json!({"story_id": "888_deps_scr", "front_matter": {"depends_on": [1, 2, 3]}}), + &ctx, + ); + assert!(r.is_ok(), "set [1,2,3] should succeed: {r:?}"); + let view = crate::crdt_state::read_item("888_deps_scr").expect("CRDT must have story"); + assert_eq!( + view.depends_on, + Some(vec![1, 2, 3]), + "CRDT should hold [1,2,3] after set" + ); + + // Clear to []. + let r = tool_update_story( + &json!({"story_id": "888_deps_scr", "front_matter": {"depends_on": []}}), + &ctx, + ); + assert!(r.is_ok(), "clear [] should succeed: {r:?}"); + let view = crate::crdt_state::read_item("888_deps_scr").expect("CRDT must have story"); + assert_eq!( + view.depends_on, None, + "CRDT should be None after clearing to []" + ); + + // Replace with [4, 5] — must not append to previous [1,2,3]. + let r = tool_update_story( + &json!({"story_id": "888_deps_scr", "front_matter": {"depends_on": [4, 5]}}), + &ctx, + ); + assert!(r.is_ok(), "replace [4,5] should succeed: {r:?}"); + let view = crate::crdt_state::read_item("888_deps_scr").expect("CRDT must have story"); + assert_eq!( + view.depends_on, + Some(vec![4, 5]), + "CRDT should hold exactly [4,5] after replace (not [1,2,3,4,5])" + ); + } + + /// Regression: clearing depends_on must survive a subsequent update to another + /// field. Before the fix, write_story_content would restore the old YAML + /// depends_on value into the CRDT register, overwriting the clear. + #[test] + fn tool_update_story_clear_depends_on_survives_subsequent_update() { + let tmp = tempfile::tempdir().unwrap(); + crate::crdt_state::init_for_test(); + // Story created WITH depends_on in YAML so write_story_content would + // previously restore it. + setup_story_for_update( + tmp.path(), + "888_deps_persist", + "---\nname: Deps Persist\ndepends_on: [100, 200]\n---\n\nNo sections.\n", + ); + let ctx = test_ctx(tmp.path()); + + // Seed CRDT with the YAML deps (simulates the initial write path). + crate::crdt_state::set_depends_on("888_deps_persist", &[100, 200]); + + // Clear deps via update_story. + let r = tool_update_story( + &json!({"story_id": "888_deps_persist", "front_matter": {"depends_on": []}}), + &ctx, + ); + assert!(r.is_ok(), "clear should succeed: {r:?}"); + let view = crate::crdt_state::read_item("888_deps_persist").expect("CRDT must have story"); + assert_eq!(view.depends_on, None, "CRDT should be None after clear"); + + // Now update a different field — this triggers write_story_content with + // the stale YAML (which still has depends_on: [100, 200]). + let r = tool_update_story( + &json!({"story_id": "888_deps_persist", "name": "Deps Persist Updated"}), + &ctx, + ); + assert!(r.is_ok(), "subsequent name update should succeed: {r:?}"); + + // The CRDT must still be None — the YAML value must not have been restored. + let view = crate::crdt_state::read_item("888_deps_persist").expect("CRDT must have story"); + assert_eq!( + view.depends_on, None, + "CRDT depends_on must remain None after unrelated update (write_story_content must not restore YAML value)" + ); + } + #[test] fn tool_update_story_depends_on_routes_to_crdt_not_yaml() { let tmp = tempfile::tempdir().unwrap(); diff --git a/server/src/http/workflow/utils.rs b/server/src/http/workflow/utils.rs index b6aa9fca..6b5aafb6 100644 --- a/server/src/http/workflow/utils.rs +++ b/server/src/http/workflow/utils.rs @@ -17,12 +17,14 @@ pub(crate) fn write_story_content( stage: &str, content: &str, ) { - crate::db::write_item_with_content( - story_id, - stage, - content, - crate::db::ItemMeta::from_yaml(content), - ); + let mut meta = crate::db::ItemMeta::from_yaml(content); + // CRDT is the single source of truth for depends_on. Never overwrite the + // register from YAML here — the typed setter (crdt_state::set_depends_on) + // is the only authorised write path. Passing None leaves the existing + // register untouched on update and initialises new items to "" so the + // explicit set_depends_on call in each create function takes effect. + meta.depends_on = None; + crate::db::write_item_with_content(story_id, stage, content, meta); } /// Determine what stage a story is in (from CRDT).