diff --git a/server/src/chat/commands/depends.rs b/server/src/chat/commands/depends.rs index 705671c8..62cd6fd8 100644 --- a/server/src/chat/commands/depends.rs +++ b/server/src/chat/commands/depends.rs @@ -7,9 +7,7 @@ //! Passing no dependency numbers clears the field entirely. use super::CommandContext; -use crate::io::story_metadata::{ - parse_front_matter, write_depends_on, write_depends_on_in_content, -}; +use crate::io::story_metadata::parse_front_matter; /// Handle the `depends` command. /// @@ -53,7 +51,7 @@ pub(super) fn handle_depends(ctx: &CommandContext) -> Option { } // Find the story by numeric prefix: CRDT → content store → filesystem. - let (story_id, stage_dir, path, content) = + let (story_id, _stage_dir, _path, content) = match crate::chat::lookup::find_story_by_number(ctx.effective_root(), num_str) { Some(found) => found, None => { @@ -69,49 +67,20 @@ pub(super) fn handle_depends(ctx: &CommandContext) -> Option { .and_then(|m| m.name) .unwrap_or_else(|| story_id.clone()); - // Prefer the CRDT content store; fall back to filesystem only when the - // story has not been loaded into the DB (e.g. very early startup or tests - // that haven't called write_item_with_content). - if let Some(existing) = crate::db::read_content(&story_id) { - let updated = write_depends_on_in_content(&existing, &deps); - crate::db::write_content(&story_id, &updated); - let stage = crate::pipeline_state::read_typed(&story_id) - .ok() - .flatten() - .map(|i| i.stage.dir_name().to_string()) - .unwrap_or_else(|| stage_dir.clone()); - crate::db::write_item_with_content(&story_id, &stage, &updated); - // Sync depends_on to the typed CRDT register. - crate::crdt_state::set_depends_on(&story_id, &deps); - if deps.is_empty() { - Some(format!( - "Cleared all dependencies for **{story_name}** ({story_id})." - )) - } else { - let nums: Vec = deps.iter().map(|n| n.to_string()).collect(); - Some(format!( - "Set depends_on: [{}] for **{story_name}** ({story_id}).", - nums.join(", ") - )) - } + // Write depends_on to the typed CRDT register — single source of truth. + // No YAML mutation: the CRDT register is the canonical location for deps. + crate::crdt_state::set_depends_on(&story_id, &deps); + + if deps.is_empty() { + Some(format!( + "Cleared all dependencies for **{story_name}** ({story_id})." + )) } else { - match write_depends_on(&path, &deps) { - Ok(()) if deps.is_empty() => { - crate::crdt_state::set_depends_on(&story_id, &[]); - Some(format!( - "Cleared all dependencies for **{story_name}** ({story_id})." - )) - } - Ok(()) => { - crate::crdt_state::set_depends_on(&story_id, &deps); - let nums: Vec = deps.iter().map(|n| n.to_string()).collect(); - Some(format!( - "Set depends_on: [{}] for **{story_name}** ({story_id}).", - nums.join(", ") - )) - } - Err(e) => Some(format!("Failed to update dependencies for {story_id}: {e}")), - } + let nums: Vec = deps.iter().map(|n| n.to_string()).collect(); + Some(format!( + "Set depends_on: [{}] for **{story_name}** ({story_id}).", + nums.join(", ") + )) } } @@ -207,7 +176,7 @@ mod tests { } #[test] - fn depends_sets_deps_and_writes_to_content_store() { + fn depends_sets_deps_in_crdt_not_yaml() { let tmp = tempfile::TempDir::new().unwrap(); write_story_file( tmp.path(), @@ -220,33 +189,86 @@ mod tests { output.contains("477") && output.contains("478"), "response should mention dep numbers: {output}" ); - let contents = crate::db::read_content("9910_story_foo") - .expect("content store should have updated story"); + // CRDT register must hold the deps. + let view = crate::crdt_state::read_item("9910_story_foo").expect("CRDT should have story"); + assert_eq!( + view.depends_on, + Some(vec![477, 478]), + "CRDT register should hold [477, 478]: {view:?}" + ); + // Content store YAML must NOT be mutated with depends_on. + let contents = + crate::db::read_content("9910_story_foo").expect("content store should have story"); assert!( - contents.contains("depends_on: [477, 478]"), - "content store should have depends_on set: {contents}" + !contents.contains("depends_on"), + "content store YAML must not contain depends_on after chat command: {contents}" ); } #[test] - fn depends_clears_deps_when_no_deps_given() { + fn depends_clears_deps_in_crdt_not_yaml() { let tmp = tempfile::TempDir::new().unwrap(); write_story_file( tmp.path(), "2_current", "9911_story_bar.md", - "---\nname: Bar\ndepends_on: [477]\n---\n", + "---\nname: Bar\n---\n", ); + // Pre-seed CRDT with deps so we can verify clearing. + crate::crdt_state::set_depends_on("9911_story_bar", &[477]); let output = depends_cmd_with_root(tmp.path(), "9911").unwrap(); assert!( output.contains("Cleared"), "should confirm clearing deps: {output}" ); - let contents = crate::db::read_content("9911_story_bar") - .expect("content store should have updated story"); + // CRDT register must be empty after clear. + let view = crate::crdt_state::read_item("9911_story_bar").expect("CRDT should have story"); + assert_eq!( + view.depends_on, None, + "CRDT register should be empty after clearing: {view:?}" + ); + // Content store YAML must not be mutated. + let contents = + crate::db::read_content("9911_story_bar").expect("content store should have story"); assert!( !contents.contains("depends_on"), - "content store should have depends_on cleared: {contents}" + "content store YAML must not contain depends_on after clear: {contents}" + ); + } + + /// Regression (AC3, chat path): chat `depends` must write deps to CRDT only — + /// no `depends_on` in the YAML content store. + /// The MCP counterpart is `tool_update_story_depends_on_routes_to_crdt_not_yaml` + /// in `http/mcp/story_tools/story/update.rs`. Both assert `Some(vec![500, 501])` + /// proving identical CRDT state across transports. + #[test] + fn chat_depends_sets_crdt_no_yaml_regression() { + let tmp = tempfile::TempDir::new().unwrap(); + write_story_file( + tmp.path(), + "1_backlog", + "8790_story_chat_dep.md", + "---\nname: Chat Dep\n---\n", + ); + + let out = depends_cmd_with_root(tmp.path(), "8790 500 501").unwrap(); + assert!( + out.contains("500"), + "chat response should mention dep: {out}" + ); + + let view = + crate::crdt_state::read_item("8790_story_chat_dep").expect("CRDT must have chat story"); + assert_eq!( + view.depends_on, + Some(vec![500, 501]), + "CRDT must hold [500, 501]: {view:?}" + ); + + let content = crate::db::read_content("8790_story_chat_dep").unwrap(); + assert!( + !content.contains("depends_on"), + "chat must not write depends_on to YAML: {content}" ); } diff --git a/server/src/http/mcp/story_tools/story/update.rs b/server/src/http/mcp/story_tools/story/update.rs index 07331510..d25081ba 100644 --- a/server/src/http/mcp/story_tools/story/update.rs +++ b/server/src/http/mcp/story_tools/story/update.rs @@ -41,8 +41,10 @@ pub(crate) fn tool_update_story(args: &Value, ctx: &AppContext) -> Result = arr .iter() @@ -148,7 +150,7 @@ mod tests { fs::create_dir_all(¤t).unwrap(); fs::write(current.join(format!("{story_id}.md")), content).unwrap(); crate::db::ensure_content_store(); - crate::db::write_content(story_id, content); + crate::db::write_item_with_content(story_id, "2_current", content); } #[test] @@ -206,7 +208,7 @@ mod tests { } #[test] - fn tool_update_story_front_matter_json_array_written_as_yaml_sequence() { + fn tool_update_story_depends_on_routes_to_crdt_not_yaml() { let tmp = tempfile::tempdir().unwrap(); setup_story_for_update( tmp.path(), @@ -221,20 +223,19 @@ mod tests { ); assert!(result.is_ok(), "Expected ok: {result:?}"); - let content = crate::db::read_content("504_arr_test").unwrap(); - // YAML inline sequences use spaces after commas - assert!( - content.contains("depends_on: [490, 491]"), - "array should be unquoted YAML: {content}" - ); - assert!( - !content.contains("depends_on: \""), - "array must not be quoted: {content}" + // CRDT register must hold the deps. + let view = crate::crdt_state::read_item("504_arr_test").expect("CRDT must have the story"); + assert_eq!( + view.depends_on, + Some(vec![490, 491]), + "CRDT register should hold [490, 491]: {view:?}" ); - // The YAML must be parseable as a vec - let meta = crate::io::story_metadata::parse_front_matter(&content) - .expect("front matter should parse"); - assert_eq!(meta.depends_on, Some(vec![490, 491])); + // Content store YAML must NOT contain depends_on — CRDT is sole source of truth. + let content = crate::db::read_content("504_arr_test").unwrap(); + assert!( + !content.contains("depends_on"), + "depends_on must not be written to YAML content store: {content}" + ); } } diff --git a/server/src/io/story_metadata/fields.rs b/server/src/io/story_metadata/fields.rs index d055582d..139f3752 100644 --- a/server/src/io/story_metadata/fields.rs +++ b/server/src/io/story_metadata/fields.rs @@ -94,25 +94,6 @@ pub fn write_review_hold(path: &Path) -> Result<(), String> { Ok(()) } -/// Write or update `depends_on:` field in the YAML front matter of a story file. -/// -/// Serialises `deps` as an inline YAML sequence, e.g. `[477, 478]`. -/// If `deps` is empty the field is removed. -/// If no front matter is present, this is a no-op (returns Ok). -pub fn write_depends_on(path: &Path, deps: &[u32]) -> Result<(), String> { - let contents = - fs::read_to_string(path).map_err(|e| format!("Failed to read story file: {e}"))?; - let updated = if deps.is_empty() { - remove_front_matter_field(&contents, "depends_on") - } else { - let nums: Vec = deps.iter().map(|n| n.to_string()).collect(); - let yaml_value = format!("[{}]", nums.join(", ")); - set_front_matter_field(&contents, "depends_on", &yaml_value) - }; - fs::write(path, &updated).map_err(|e| format!("Failed to write story file: {e}"))?; - Ok(()) -} - /// Remove a key from the YAML front matter of a markdown string (pure function). /// /// Returns the updated content. If no front matter or key is not found, @@ -152,20 +133,6 @@ pub fn write_mergemaster_attempted_in_content(contents: &str) -> String { set_front_matter_field(contents, "mergemaster_attempted", "true") } -/// Write or update `depends_on` in story content (pure function). -/// -/// Serialises `deps` as an inline YAML sequence, e.g. `[477, 478]`. -/// If `deps` is empty the field is removed. -pub fn write_depends_on_in_content(contents: &str, deps: &[u32]) -> String { - if deps.is_empty() { - remove_front_matter_field(contents, "depends_on") - } else { - let nums: Vec = deps.iter().map(|n| n.to_string()).collect(); - let yaml_value = format!("[{}]", nums.join(", ")); - set_front_matter_field(contents, "depends_on", &yaml_value) - } -} - #[cfg(test)] mod tests { use super::*; @@ -242,24 +209,4 @@ mod tests { assert!(contents.contains("review_hold: true")); assert!(contents.contains("name: My Spike")); } - - #[test] - fn write_depends_on_sets_field() { - let tmp = tempfile::tempdir().unwrap(); - let path = tmp.path().join("story.md"); - std::fs::write(&path, "---\nname: Test\n---\n# Story\n").unwrap(); - write_depends_on(&path, &[477, 478]).unwrap(); - let contents = std::fs::read_to_string(&path).unwrap(); - assert!(contents.contains("depends_on: [477, 478]"), "{contents}"); - } - - #[test] - fn write_depends_on_removes_field_when_empty() { - let tmp = tempfile::tempdir().unwrap(); - let path = tmp.path().join("story.md"); - std::fs::write(&path, "---\nname: Test\ndepends_on: [477]\n---\n# Story\n").unwrap(); - write_depends_on(&path, &[]).unwrap(); - let contents = std::fs::read_to_string(&path).unwrap(); - assert!(!contents.contains("depends_on"), "{contents}"); - } } diff --git a/server/src/io/story_metadata/mod.rs b/server/src/io/story_metadata/mod.rs index 913a21d9..e07e0cab 100644 --- a/server/src/io/story_metadata/mod.rs +++ b/server/src/io/story_metadata/mod.rs @@ -14,9 +14,8 @@ mod types; pub use deps::{check_archived_deps, check_archived_deps_from_list, check_unmet_deps}; pub use fields::{ clear_front_matter_field, clear_front_matter_field_in_content, set_front_matter_field, - write_depends_on, write_depends_on_in_content, write_merge_failure_in_content, - write_mergemaster_attempted_in_content, write_rejection_notes_to_content, write_review_hold, - write_review_hold_in_content, + write_merge_failure_in_content, write_mergemaster_attempted_in_content, + write_rejection_notes_to_content, write_review_hold, write_review_hold_in_content, }; pub use parser::{ is_story_frozen_in_store, parse_front_matter, parse_unchecked_todos, resolve_qa_mode,