From 41515e3b8f24a5b832ed87bf53b2b45b5dc99236 Mon Sep 17 00:00:00 2001 From: dave Date: Thu, 9 Apr 2026 18:27:25 +0000 Subject: [PATCH] huskies: merge 503_bug_depends_on_pointing_at_an_archived_story_is_silently_treated_as_deps_met_surprising_users --- .huskies/README.md | 8 + .../agents/pool/auto_assign/auto_assign.rs | 70 +++++++- .../agents/pool/auto_assign/story_checks.rs | 59 +++++++ server/src/crdt_state.rs | 51 ++++++ server/src/http/mcp/story_tools.rs | 48 +++++- server/src/io/story_metadata.rs | 160 ++++++++++++++++++ 6 files changed, 393 insertions(+), 3 deletions(-) diff --git a/.huskies/README.md b/.huskies/README.md index d6f4f3da..b3368c82 100644 --- a/.huskies/README.md +++ b/.huskies/README.md @@ -88,6 +88,14 @@ Items move through stages by moving the file between directories: Items in `5_done` are auto-swept to `6_archived` after 4 hours by the server. +### Dependency Semantics (`depends_on`) + +Work items can declare `depends_on: [N, M]` in their front matter. The auto-assign loop promotes a backlog item to current only when every listed dependency has reached `5_done` **or** `6_archived`. + +**Archived = satisfied (recorded decision, bug 503):** A dep in `6_archived` counts as satisfied. Stories auto-sweep from `5_done` to `6_archived` after 4 hours, so by the time a dep is archived the dependent story is normally already in current. Changing this to "archived = abandoned" would re-block already-promoted stories when their deps sweep, which is a worse regression. + +**When the dep is already archived at creation time:** If you create a story whose `depends_on` points at a story already in `6_archived`, `create_story` will emit a visible warning. The dep will resolve immediately on the next promotion tick. If the archived dep was abandoned rather than cleanly completed, remove the `depends_on` field or move the new story back to backlog manually after promotion fires. + ### Filesystem Watcher The server watches `.story_kit/work/` for changes. When a file is created, moved, or modified, the watcher auto-commits with a deterministic message and broadcasts a WebSocket notification to the frontend. This means: diff --git a/server/src/agents/pool/auto_assign/auto_assign.rs b/server/src/agents/pool/auto_assign/auto_assign.rs index 3b15fe31..405ef970 100644 --- a/server/src/agents/pool/auto_assign/auto_assign.rs +++ b/server/src/agents/pool/auto_assign/auto_assign.rs @@ -14,8 +14,8 @@ use super::scan::{ is_story_assigned_for_stage, scan_stage_items, }; use super::story_checks::{ - has_merge_failure, has_review_hold, has_unmet_dependencies, is_story_blocked, - read_story_front_matter_agent, + check_archived_dependencies, has_merge_failure, has_review_hold, has_unmet_dependencies, + is_story_blocked, read_story_front_matter_agent, }; impl AgentPool { @@ -24,6 +24,14 @@ impl AgentPool { /// A story is only promoted if it explicitly lists `depends_on` AND every /// listed dependency has reached `5_done` or `6_archived`. Stories with no /// `depends_on` are left in the backlog for human scheduling. + /// + /// **Archived dep semantics:** a dep in `6_archived` counts as satisfied (since + /// stories auto-sweep from `5_done` to `6_archived` after 4 hours, and the + /// dependent story would normally already be promoted by then). However, if a + /// dep was already in `6_archived` when the dependent story was created (e.g. it + /// was abandoned/superseded before the dependent existed), a prominent warning is + /// logged so the user can see the promotion was triggered by an archived dep, not + /// a clean completion. fn promote_ready_backlog_stories(&self, project_root: &Path) { use crate::io::story_metadata::parse_front_matter; @@ -49,6 +57,17 @@ impl AgentPool { if has_unmet_dependencies(project_root, "1_backlog", story_id) { continue; } + // Warn if any deps were satisfied via archive rather than via clean done. + let archived_deps = check_archived_dependencies(project_root, "1_backlog", story_id); + if !archived_deps.is_empty() { + slog_warn!( + "[auto-assign] Story '{story_id}' is being promoted because deps \ + {archived_deps:?} are in 6_archived (not cleanly completed via 5_done). \ + These deps may have been abandoned or superseded. If this promotion is \ + unintentional, remove the depends_on or manually move the story back to \ + 1_backlog." + ); + } // All deps met — promote from backlog to current. slog!("[auto-assign] Story '{story_id}' deps met; promoting from backlog to current."); if let Err(e) = @@ -623,6 +642,53 @@ mod tests { ); } + // ── Bug 503: archived-dep promotion visibility ───────────────────────────── + + /// A backlog story whose dep is in 6_archived must still be promoted + /// (archived = satisfied), but the promotion must not silently skip the warning + /// path. This test verifies the promotion itself fires; the warning is a + /// slog_warn! side-effect that we can't easily assert on in unit tests. + #[tokio::test] + async fn auto_assign_promotes_backlog_story_when_dep_is_archived() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + let sk = root.join(".huskies"); + let backlog = sk.join("work/1_backlog"); + let current = sk.join("work/2_current"); + let archived = sk.join("work/6_archived"); + std::fs::create_dir_all(&backlog).unwrap(); + std::fs::create_dir_all(¤t).unwrap(); + std::fs::create_dir_all(&archived).unwrap(); + std::fs::write( + sk.join("project.toml"), + "[[agent]]\nname = \"coder-1\"\nstage = \"coder\"\n", + ) + .unwrap(); + // Dep 490 is in 6_archived (e.g. a CRDT spike that was archived/superseded). + std::fs::write(archived.join("490_spike_crdt.md"), "---\nname: CRDT Spike\n---\n") + .unwrap(); + // Story 478 depends on 490 (the archived spike). + std::fs::write( + backlog.join("478_story_dependent.md"), + "---\nname: Dependent\ndepends_on: [490]\n---\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3001); + pool.auto_assign_available_work(root).await; + + // Story 478 must be promoted to 2_current/ even though dep 490 is only in + // 6_archived (not in 5_done), because archived = satisfied. + assert!( + current.join("478_story_dependent.md").exists(), + "story 478 should be promoted to 2_current/ when dep 490 is in 6_archived" + ); + assert!( + !backlog.join("478_story_dependent.md").exists(), + "story 478 must be removed from 1_backlog/ after promotion" + ); + } + /// Stories in backlog with NO depends_on must NOT be auto-promoted. #[tokio::test] async fn auto_assign_does_not_promote_backlog_story_without_deps() { diff --git a/server/src/agents/pool/auto_assign/story_checks.rs b/server/src/agents/pool/auto_assign/story_checks.rs index bd6759bd..6568dc1e 100644 --- a/server/src/agents/pool/auto_assign/story_checks.rs +++ b/server/src/agents/pool/auto_assign/story_checks.rs @@ -84,6 +84,25 @@ pub(super) fn has_unmet_dependencies( !crate::io::story_metadata::check_unmet_deps(project_root, stage_dir, story_id).is_empty() } +/// Return the list of dependency story numbers that are in `6_archived` (satisfied +/// via archive rather than via a clean `5_done` completion). +/// +/// Used to emit a warning when backlog promotion fires because one or more deps were +/// archived. Returns an empty `Vec` when no deps are archived. Reads from CRDT +/// first; falls back to filesystem when CRDT is not initialised. +pub(super) fn check_archived_dependencies( + project_root: &Path, + stage_dir: &str, + story_id: &str, +) -> Vec { + // Prefer CRDT-based check when the item is known to CRDT. + if crate::crdt_state::read_item(story_id).is_some() { + return crate::crdt_state::check_archived_deps_crdt(story_id); + } + // Fallback: filesystem. + crate::io::story_metadata::check_archived_deps(project_root, stage_dir, story_id) +} + /// Return `true` if the story file has a `merge_failure` field in its front matter. pub(super) fn has_merge_failure(project_root: &Path, _stage_dir: &str, story_id: &str) -> bool { use crate::io::story_metadata::parse_front_matter; @@ -170,4 +189,44 @@ mod tests { std::fs::write(current.join("5_story_free.md"), "---\nname: Free\n---\n").unwrap(); assert!(!has_unmet_dependencies(tmp.path(), "2_current", "5_story_free")); } + + // ── Bug 503: archived-dep visibility ───────────────────────────────────── + + /// check_archived_dependencies returns dep IDs that are in 6_archived. + #[test] + fn check_archived_dependencies_returns_archived_ids() { + let tmp = tempfile::tempdir().unwrap(); + let backlog = tmp.path().join(".huskies/work/1_backlog"); + let archived = tmp.path().join(".huskies/work/6_archived"); + std::fs::create_dir_all(&backlog).unwrap(); + std::fs::create_dir_all(&archived).unwrap(); + std::fs::write(archived.join("500_spike_crdt.md"), "---\nname: CRDT Spike\n---\n").unwrap(); + std::fs::write( + backlog.join("503_story_dependent.md"), + "---\nname: Dependent\ndepends_on: [500]\n---\n", + ) + .unwrap(); + let archived_deps = + check_archived_dependencies(tmp.path(), "1_backlog", "503_story_dependent"); + assert_eq!(archived_deps, vec![500]); + } + + /// check_archived_dependencies returns empty when dep is in 5_done (not archived). + #[test] + fn check_archived_dependencies_empty_when_dep_in_done() { + let tmp = tempfile::tempdir().unwrap(); + let backlog = tmp.path().join(".huskies/work/1_backlog"); + let done = tmp.path().join(".huskies/work/5_done"); + std::fs::create_dir_all(&backlog).unwrap(); + std::fs::create_dir_all(&done).unwrap(); + std::fs::write(done.join("490_story_done.md"), "---\nname: Done\n---\n").unwrap(); + std::fs::write( + backlog.join("503_story_waiting.md"), + "---\nname: Waiting\ndepends_on: [490]\n---\n", + ) + .unwrap(); + let archived_deps = + check_archived_dependencies(tmp.path(), "1_backlog", "503_story_waiting"); + assert!(archived_deps.is_empty()); + } } diff --git a/server/src/crdt_state.rs b/server/src/crdt_state.rs index 9f6b22bb..765696d2 100644 --- a/server/src/crdt_state.rs +++ b/server/src/crdt_state.rs @@ -431,6 +431,7 @@ fn extract_item_view(item: &PipelineItemCrdt) -> Option { /// according to CRDT state. /// /// Returns `true` if the dependency is satisfied (item found in a done stage). +/// See `dep_is_archived_crdt` to distinguish archive-satisfied from cleanly-done. pub fn dep_is_done_crdt(dep_number: u32) -> bool { let prefix = format!("{dep_number}_"); if let Some(items) = read_all_items() { @@ -443,6 +444,22 @@ pub fn dep_is_done_crdt(dep_number: u32) -> bool { } } +/// Check whether a dependency (by numeric ID prefix) is specifically in `6_archived` +/// according to CRDT state. +/// +/// Used to detect when a dependency is satisfied via archive rather than via a clean +/// completion through `5_done`. Returns `false` when the CRDT layer is not initialised. +pub fn dep_is_archived_crdt(dep_number: u32) -> bool { + let prefix = format!("{dep_number}_"); + if let Some(items) = read_all_items() { + items.iter().any(|item| { + item.story_id.starts_with(&prefix) && item.stage == "6_archived" + }) + } else { + false + } +} + /// Check unmet dependencies for a story by reading its `depends_on` from the /// CRDT document and checking each dependency against CRDT state. /// @@ -461,6 +478,25 @@ pub fn check_unmet_deps_crdt(story_id: &str) -> Vec { .collect() } +/// Return the list of dependency numbers from `story_id`'s `depends_on` that are +/// specifically in `6_archived` according to CRDT state. +/// +/// Used to emit a warning when promotion fires because a dep is archived rather than +/// cleanly completed. Returns an empty `Vec` when no deps are archived. +pub fn check_archived_deps_crdt(story_id: &str) -> Vec { + let item = match read_item(story_id) { + Some(i) => i, + None => return Vec::new(), + }; + let deps = match item.depends_on { + Some(d) => d, + None => return Vec::new(), + }; + deps.into_iter() + .filter(|&dep| dep_is_archived_crdt(dep)) + .collect() +} + /// Hex-encode a byte slice (no external dep needed). mod hex { pub fn encode(bytes: &[u8]) -> String { @@ -793,4 +829,19 @@ mod tests { let result = check_unmet_deps_crdt("nonexistent_story"); assert!(result.is_empty()); } + + // ── Bug 503: archived-dep visibility ───────────────────────────────────── + + #[test] + fn dep_is_archived_crdt_returns_false_when_no_crdt_state() { + // When the global CRDT state is not initialised, must not panic. + let _ = dep_is_archived_crdt(9998); + } + + #[test] + fn check_archived_deps_crdt_returns_empty_when_item_not_found() { + // Non-existent story should return empty archived deps. + let result = check_archived_deps_crdt("nonexistent_story_archived"); + assert!(result.is_empty()); + } } diff --git a/server/src/http/mcp/story_tools.rs b/server/src/http/mcp/story_tools.rs index 92f2d411..7ae681b5 100644 --- a/server/src/http/mcp/story_tools.rs +++ b/server/src/http/mcp/story_tools.rs @@ -7,7 +7,7 @@ use crate::http::workflow::{ create_spike_file, create_story_file, list_bug_files, list_refactor_files, load_pipeline_state, load_upcoming_stories, update_story_in_file, validate_story_dirs, }; -use crate::io::story_metadata::{parse_front_matter, parse_unchecked_todos}; +use crate::io::story_metadata::{check_archived_deps, check_archived_deps_from_list, parse_front_matter, parse_unchecked_todos}; use crate::slog_warn; use crate::workflow::{TestCaseResult, TestStatus, evaluate_acceptance_with_coverage}; use serde_json::{Value, json}; @@ -40,6 +40,30 @@ pub(super) fn tool_create_story(args: &Value, ctx: &AppContext) -> Result Result } /// Return `true` if a story with the given numeric ID exists in `5_done` or `6_archived`. +/// +/// **Dependency semantics:** Both `5_done` and `6_archived` satisfy a `depends_on` entry. +/// Stories auto-sweep from `5_done` to `6_archived` after 4 hours, so by the time a dep +/// reaches `6_archived`, the dependent story has already been promoted. When a dep is +/// already in `6_archived` at the moment of promotion (e.g., it was manually archived or +/// abandoned before the dependent story was created), the dependency is still considered +/// satisfied — but a warning is logged so the user can see that the dep was archived, not +/// cleanly completed. Use `check_archived_deps` to detect this case. fn dep_is_done(project_root: &Path, dep_number: u32) -> bool { let prefix = format!("{dep_number}_"); let exact = dep_number.to_string(); @@ -319,6 +327,63 @@ fn dep_is_done(project_root: &Path, dep_number: u32) -> bool { false } +/// Return `true` if a story with the given numeric ID exists specifically in `6_archived` +/// (i.e., it satisfies a `depends_on` but via the archive rather than via a clean done). +fn dep_is_archived(project_root: &Path, dep_number: u32) -> bool { + let prefix = format!("{dep_number}_"); + let exact = dep_number.to_string(); + let dir = project_root.join(".huskies").join("work").join("6_archived"); + if let Ok(entries) = fs::read_dir(&dir) { + for entry in entries.flatten() { + let path = entry.path(); + if path.extension().and_then(|e| e.to_str()) != Some("md") { + continue; + } + if let Some(stem) = path.file_stem().and_then(|s| s.to_str()) + && (stem == exact || stem.starts_with(&prefix)) + { + return true; + } + } + } + false +} + +/// Return the list of dependency story numbers from `story_id`'s front matter +/// that are in `6_archived` (satisfied via archive rather than via normal done). +/// +/// Used to emit a warning when backlog promotion fires because a dep was archived +/// rather than cleanly completed. Returns an empty `Vec` when no deps are archived. +pub fn check_archived_deps(project_root: &Path, stage_dir: &str, story_id: &str) -> Vec { + let path = project_root + .join(".huskies") + .join("work") + .join(stage_dir) + .join(format!("{story_id}.md")); + let contents = match fs::read_to_string(&path) { + Ok(c) => c, + Err(_) => return Vec::new(), + }; + let deps = match parse_front_matter(&contents).ok().and_then(|m| m.depends_on) { + Some(d) => d, + None => return Vec::new(), + }; + deps.into_iter() + .filter(|&dep| dep_is_archived(project_root, dep)) + .collect() +} + +/// Given an explicit list of dep numbers, return those already in `6_archived`. +/// +/// Used at story-creation time when the dep list is known in memory (before the +/// story file has been written), so the caller does not need to parse the story. +pub fn check_archived_deps_from_list(project_root: &Path, deps: &[u32]) -> Vec { + deps.iter() + .copied() + .filter(|&dep| dep_is_archived(project_root, dep)) + .collect() +} + // ── In-memory content variants (no filesystem access) ─────────────── /// Remove a key from the YAML front matter of a markdown string (pure function). @@ -694,4 +759,99 @@ workflow: tdd assert!(!dep_is_done(tmp.path(), 101)); } + // ── Bug 503: archived-dep visibility ───────────────────────────────────── + + /// check_archived_deps returns the dep IDs that are in 6_archived. + #[test] + fn check_archived_deps_returns_archived_dep_numbers() { + let tmp = tempfile::tempdir().unwrap(); + let current = tmp.path().join(".huskies/work/2_current"); + let archived = tmp.path().join(".huskies/work/6_archived"); + std::fs::create_dir_all(¤t).unwrap(); + std::fs::create_dir_all(&archived).unwrap(); + // Dep 100 is in 6_archived; dep 101 is not anywhere. + std::fs::write(archived.join("100_spike_old.md"), "---\nname: Old\n---\n").unwrap(); + std::fs::write( + current.join("5_story_dependent.md"), + "---\nname: Dep\ndepends_on: [100, 101]\n---\n", + ) + .unwrap(); + let archived_deps = check_archived_deps(tmp.path(), "2_current", "5_story_dependent"); + assert_eq!(archived_deps, vec![100]); + } + + /// check_archived_deps returns empty when no deps are in 6_archived. + #[test] + fn check_archived_deps_returns_empty_when_dep_in_done() { + let tmp = tempfile::tempdir().unwrap(); + let backlog = tmp.path().join(".huskies/work/1_backlog"); + let done = tmp.path().join(".huskies/work/5_done"); + std::fs::create_dir_all(&backlog).unwrap(); + std::fs::create_dir_all(&done).unwrap(); + // Dep 200 is in 5_done (not archived). + std::fs::write(done.join("200_story_done.md"), "---\nname: Done\n---\n").unwrap(); + std::fs::write( + backlog.join("5_story_waiting.md"), + "---\nname: Waiting\ndepends_on: [200]\n---\n", + ) + .unwrap(); + let archived_deps = check_archived_deps(tmp.path(), "1_backlog", "5_story_waiting"); + assert!(archived_deps.is_empty()); + } + + /// check_archived_deps returns empty when story has no depends_on. + #[test] + fn check_archived_deps_returns_empty_when_no_deps() { + let tmp = tempfile::tempdir().unwrap(); + let current = tmp.path().join(".huskies/work/2_current"); + std::fs::create_dir_all(¤t).unwrap(); + std::fs::write(current.join("3_story_free.md"), "---\nname: Free\n---\n").unwrap(); + let archived_deps = check_archived_deps(tmp.path(), "2_current", "3_story_free"); + assert!(archived_deps.is_empty()); + } + + /// check_archived_deps_from_list returns archived dep IDs from an in-memory list. + #[test] + fn check_archived_deps_from_list_returns_archived_ids() { + let tmp = tempfile::tempdir().unwrap(); + let done = tmp.path().join(".huskies/work/5_done"); + let archived = tmp.path().join(".huskies/work/6_archived"); + std::fs::create_dir_all(&done).unwrap(); + std::fs::create_dir_all(&archived).unwrap(); + std::fs::write(done.join("10_story_done.md"), "---\nname: Done\n---\n").unwrap(); + std::fs::write(archived.join("20_story_old.md"), "---\nname: Old\n---\n").unwrap(); + // Only 20 is archived; 10 is in done, 30 is nowhere. + let result = check_archived_deps_from_list(tmp.path(), &[10, 20, 30]); + assert_eq!(result, vec![20]); + } + + /// check_archived_deps_from_list returns empty when no deps are archived. + #[test] + fn check_archived_deps_from_list_empty_when_no_archived_deps() { + let tmp = tempfile::tempdir().unwrap(); + let done = tmp.path().join(".huskies/work/5_done"); + std::fs::create_dir_all(&done).unwrap(); + std::fs::write(done.join("10_story_done.md"), "---\nname: Done\n---\n").unwrap(); + let result = check_archived_deps_from_list(tmp.path(), &[10]); + assert!(result.is_empty()); + } + + /// dep_is_archived returns true only for stories in 6_archived, not 5_done. + #[test] + fn dep_is_archived_distinguishes_done_from_archived() { + let tmp = tempfile::tempdir().unwrap(); + let done = tmp.path().join(".huskies/work/5_done"); + let archived = tmp.path().join(".huskies/work/6_archived"); + std::fs::create_dir_all(&done).unwrap(); + std::fs::create_dir_all(&archived).unwrap(); + std::fs::write(done.join("10_story_done.md"), "---\nname: Done\n---\n").unwrap(); + std::fs::write(archived.join("20_story_old.md"), "---\nname: Old\n---\n").unwrap(); + // 10 is in 5_done only — not archived. + assert!(!dep_is_archived(tmp.path(), 10)); + // 20 is in 6_archived — archived. + assert!(dep_is_archived(tmp.path(), 20)); + // 99 doesn't exist anywhere. + assert!(!dep_is_archived(tmp.path(), 99)); + } + }