From 98d496b1ad5d6cbe1381c120818c07442b3c856f Mon Sep 17 00:00:00 2001 From: Timmy Date: Tue, 12 May 2026 13:13:01 +0100 Subject: [PATCH] fix(901): unblock_story works on CRDT-only stories post-865 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 901: `unblock_story` (and the chat `unblock` command) routed through `parse_front_matter` and errored with "Missing front matter" on any post-865 story (story content is now CRDT-only with no YAML on disk). In `chat/commands/unblock.rs::unblock_by_story_id`: - Drop the early `parse_front_matter` gate. - Read story name and blocked state from the CRDT register API instead of parsed YAML (`crdt_state::read_item`, `pipeline_state::read_typed`). - Keep the legacy fallback cleanup, but gate it on the content actually starting with a `---` YAML block, so CRDT-only stories don't hit a parse error there either. - Remove the now-unused `parse_front_matter` import. Surfaced a second sub-bug: even when the state-machine transition fired (`Blocked + Unblock → Coding`), the CRDT `blocked` register was never explicitly cleared. Pre-865 the YAML-strip content_transform cleared it as a side effect; post-865 there is no YAML to strip. - Add `crdt_state::set_blocked(story_id, bool)` parallel to `set_retry_count`. Wired through `crdt_state::write` and the crate-level re-export. - `agents::lifecycle::transition_to_unblocked` now calls `set_blocked(story_id, false)` alongside `set_retry_count(0)` so the legacy register stays in sync with the typed stage. Test: `unblock_command_works_on_crdt_only_story_no_yaml` seeds a CRDT entry with no YAML on disk, runs unblock, asserts success + cleared blocked + retry_count=0. All 10 existing unblock tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- server/src/agents/lifecycle.rs | 13 +- server/src/chat/commands/unblock.rs | 187 +++++++++++++++++++--------- server/src/crdt_state/mod.rs | 3 +- server/src/crdt_state/write/item.rs | 19 +++ server/src/crdt_state/write/mod.rs | 4 +- 5 files changed, 159 insertions(+), 67 deletions(-) diff --git a/server/src/agents/lifecycle.rs b/server/src/agents/lifecycle.rs index 1d06c460..b86cb41c 100644 --- a/server/src/agents/lifecycle.rs +++ b/server/src/agents/lifecycle.rs @@ -286,7 +286,11 @@ pub fn transition_to_unblocked(story_id: &str) -> Result<(), String> { .map(|_| ()) .map_err(|e| e.to_string())?; - // Reset the retry counter in the CRDT so the story gets a fresh budget. + // Reset CRDT registers so the legacy `blocked`/`retry_count` fields match + // the new typed stage. Pre-865, YAML stripping kept these in sync as a + // side-effect of the content_transform above; post-865 the content has no + // YAML, so we must clear the registers explicitly. + crate::crdt_state::set_blocked(story_id, false); crate::crdt_state::set_retry_count(story_id, 0); Ok(()) } @@ -312,9 +316,10 @@ fn map_stage_move_to_event( feature_branch: branch(), commits_ahead: nz1(), }), - (Stage::Coding, "backlog") | (Stage::Qa, "backlog") | (Stage::Merge { .. }, "backlog") => { - Ok(PipelineEvent::Demote) - } + (Stage::Coding, "backlog") + | (Stage::Qa, "backlog") + | (Stage::Merge { .. }, "backlog") + | (Stage::Blocked { .. }, "backlog") => Ok(PipelineEvent::Demote), (Stage::Qa, "current") => Ok(PipelineEvent::GatesFailed { reason: "manual move".to_string(), }), diff --git a/server/src/chat/commands/unblock.rs b/server/src/chat/commands/unblock.rs index 31d1c255..84d71fb1 100644 --- a/server/src/chat/commands/unblock.rs +++ b/server/src/chat/commands/unblock.rs @@ -5,7 +5,7 @@ //! and returns a confirmation. use super::CommandContext; -use crate::db::yaml_legacy::{clear_front_matter_field_in_content, parse_front_matter}; +use crate::db::yaml_legacy::clear_front_matter_field_in_content; use std::path::Path; /// Handle the `unblock` command. @@ -50,77 +50,76 @@ pub(crate) fn unblock_by_number(project_root: &Path, story_number: &str) -> Stri /// `blocked: true` / `merge_failure` front-matter, then routes through /// [`crate::agents::lifecycle::transition_to_unblocked`]. fn unblock_by_story_id(story_id: &str) -> String { - // Read content for the story name and legacy field checks. - let contents = match crate::db::read_content(story_id) { - Some(c) => c, - None => return format!("Failed to read story content for **{story_id}**"), - }; + // Post-865, story content is CRDT-only (no YAML front matter on disk or in + // the content store). Read the story name and blocked state from CRDT + // registers; do NOT call `parse_front_matter` on the content. + let crdt_item = crate::crdt_state::read_item(story_id); + let story_name = crdt_item + .as_ref() + .and_then(|i| i.name.clone()) + .unwrap_or_else(|| story_id.to_string()); - let meta = match parse_front_matter(&contents) { - Ok(m) => m, - Err(e) => return format!("Failed to parse front matter for **{story_id}**: {e}"), - }; - - let story_name = meta.name.as_deref().unwrap_or(story_id).to_string(); - - // Check if the story is blocked via the typed stage or legacy front-matter. - let typed_blocked = crate::pipeline_state::read_typed(story_id) - .ok() - .flatten() + // Canonical "is this story blocked?" comes from the typed pipeline state. + let typed_item = crate::pipeline_state::read_typed(story_id).ok().flatten(); + let typed_blocked = typed_item + .as_ref() .is_some_and(|item| item.stage.is_blocked()); - let has_blocked = meta.blocked == Some(true); - let has_merge_failure = meta.merge_failure.is_some(); + let typed_merge_failure = matches!( + typed_item.as_ref().map(|i| &i.stage), + Some(crate::pipeline_state::Stage::MergeFailure { .. }) + ); + // CRDT register fallback for items not yet projected into typed state. + let crdt_blocked = crdt_item.as_ref().and_then(|i| i.blocked).unwrap_or(false); - if !typed_blocked && !has_blocked && !has_merge_failure { + if !typed_blocked && !crdt_blocked { return format!("**{story_name}** ({story_id}) is not blocked. Nothing to unblock."); } - // Route through the state machine. - match crate::agents::lifecycle::transition_to_unblocked(story_id) { - Ok(()) => {} - Err(e) => { - // If the typed transition fails (e.g. legacy Archived item), - // fall back to clearing front-matter fields directly. - crate::slog_warn!( - "[unblock] State-machine transition failed for '{story_id}': {e}. \ - Falling back to front-matter cleanup." - ); - let mut updated = contents; - if has_blocked { + // Route through the state machine. This clears blocked/merge_failure/ + // retry_count via `fields_to_clear_transform` and resets retry_count in + // the CRDT. + if let Err(e) = crate::agents::lifecycle::transition_to_unblocked(story_id) { + // If the typed transition fails (e.g. a legacy Archived item with no + // valid `Unblock` transition out of its current stage), fall back to + // a direct CRDT/content cleanup. The legacy front-matter cleanup is + // gated on content actually still containing YAML, so post-865 + // CRDT-only stories don't hit a parse error. + crate::slog_warn!( + "[unblock] State-machine transition failed for '{story_id}': {e}. \ + Falling back to direct CRDT cleanup." + ); + + if let Some(content) = crate::db::read_content(story_id) { + // Only run legacy front-matter cleanup if the stored content still + // begins with a `---` YAML block. Post-865 content has been + // stripped and would no-op here anyway. + if content.trim_start().starts_with("---") { + let mut updated = content; updated = clear_front_matter_field_in_content(&updated, "blocked"); - } - if has_merge_failure { updated = clear_front_matter_field_in_content(&updated, "merge_failure"); + updated = clear_front_matter_field_in_content(&updated, "retry_count"); + crate::db::write_content(story_id, &updated); + let stage = typed_item + .as_ref() + .map(|i| i.stage.dir_name().to_string()) + .unwrap_or_else(|| "2_current".to_string()); + crate::db::write_item_with_content( + story_id, + &stage, + &updated, + crate::db::ItemMeta::from_yaml(&updated), + ); } - updated = clear_front_matter_field_in_content(&updated, "retry_count"); - - 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(|| "2_current".to_string()); - crate::db::write_item_with_content( - story_id, - &stage, - &updated, - crate::db::ItemMeta::from_yaml(&updated), - ); - crate::crdt_state::set_retry_count(story_id, 0); } + crate::crdt_state::set_retry_count(story_id, 0); } - let mut cleared = Vec::new(); - if typed_blocked || has_blocked { - cleared.push("blocked"); - } - if has_merge_failure { - cleared.push("merge_failure"); - } - format!( - "Unblocked **{story_name}** ({story_id}). Cleared: {}. Retry count reset to 0.", - cleared.join(", ") - ) + let cleared = if typed_merge_failure { + "merge_failure" + } else { + "blocked" + }; + format!("Unblocked **{story_name}** ({story_id}). Cleared: {cleared}. Retry count reset to 0.") } // --------------------------------------------------------------------------- @@ -278,6 +277,74 @@ mod tests { ); } + #[test] + fn unblock_command_works_on_crdt_only_story_no_yaml() { + // Bug 901 regression: post-865, story content is CRDT-only with no + // YAML front matter on disk or in the content store. unblock_story + // used to fail here with "Missing front matter" because the legacy + // code parsed YAML before consulting the CRDT. + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + let tmp = tempfile::TempDir::new().unwrap(); + + // CRDT-only content: pure markdown body, no `---` block. Matches the + // post-865 on-disk shape. + let body = "# Stuck Story\n\nNo YAML front matter — this is the post-865 shape.\n"; + let story_id = "9904_story_crdt_only"; + // Canonical post-865 blocked story: stage is `2_blocked` (typed + // Stage::Blocked), so transition_to_unblocked can fire the proper + // Blocked → Coding state-machine transition. + let stage = "2_blocked"; + + // Seed content store with the YAML-less body so find_story_by_number's + // content-store path returns it. + crate::db::write_item_with_content( + story_id, + stage, + body, + crate::db::ItemMeta::from_yaml(body), + ); + // Seed CRDT registers: blocked=true, retry_count=5, with a name so the + // response can echo it back instead of falling through to the raw id. + crate::crdt_state::write_item( + story_id, + stage, + Some("Stuck Story"), + None, + Some(5), + Some(true), + None, + None, + None, + None, + ); + + let output = unblock_cmd_with_root(tmp.path(), "9904").unwrap(); + + assert!( + !output.contains("Missing front matter") + && !output.contains("Failed to parse front matter"), + "must not error on YAML parse for CRDT-only stories: {output}" + ); + assert!( + output.contains("Unblocked") && output.contains("Stuck Story"), + "should confirm unblock with story name: {output}" + ); + + let item = crate::crdt_state::read_item(story_id) + .expect("story should still be in CRDT after unblock"); + assert_eq!( + item.retry_count, + Some(0), + "retry_count must be reset to 0 in CRDT after unblock" + ); + assert!( + !item.blocked.unwrap_or(false), + "blocked flag must be cleared in CRDT after unblock: {:?}", + item.blocked + ); + } + #[test] fn unblock_command_finds_story_in_any_stage() { let tmp = tempfile::TempDir::new().unwrap(); diff --git a/server/src/crdt_state/mod.rs b/server/src/crdt_state/mod.rs index f34a65e9..ece975a1 100644 --- a/server/src/crdt_state/mod.rs +++ b/server/src/crdt_state/mod.rs @@ -53,7 +53,8 @@ pub use types::{ }; pub use write::{ bump_retry_count, migrate_names_from_slugs, migrate_story_ids_to_numeric, name_from_story_id, - set_agent, set_depends_on, set_mergemaster_attempted, set_qa_mode, set_retry_count, write_item, + set_agent, set_blocked, set_depends_on, set_mergemaster_attempted, set_qa_mode, + set_retry_count, write_item, }; #[cfg(test)] diff --git a/server/src/crdt_state/write/item.rs b/server/src/crdt_state/write/item.rs index addedde5..c07405b0 100644 --- a/server/src/crdt_state/write/item.rs +++ b/server/src/crdt_state/write/item.rs @@ -285,6 +285,25 @@ pub fn set_retry_count(story_id: &str, count: i64) { } } +/// Set the `blocked` register on a story to the given value. +/// +/// Pure metadata operation — the item's stage is not changed. +/// Use this alongside a state-machine transition out of `Blocked` / +/// `MergeFailure` to keep the legacy `blocked` register in sync with the +/// typed stage post-865 (where YAML side-effects no longer clear the +/// register on their own). +pub fn set_blocked(story_id: &str, blocked: bool) { + let Some(state_mutex) = get_crdt() else { + return; + }; + let Ok(mut state) = state_mutex.lock() else { + return; + }; + if let Some(&idx) = state.index.get(story_id) { + apply_and_persist(&mut state, |s| s.crdt.doc.items[idx].blocked.set(blocked)); + } +} + /// Increment `retry_count` by 1 and return the new value. /// /// Pure metadata operation — the item's stage is not changed. diff --git a/server/src/crdt_state/write/mod.rs b/server/src/crdt_state/write/mod.rs index f3004d24..2f82bcc7 100644 --- a/server/src/crdt_state/write/mod.rs +++ b/server/src/crdt_state/write/mod.rs @@ -10,7 +10,7 @@ mod migrations; mod tests; pub use item::{ - bump_retry_count, set_agent, set_depends_on, set_mergemaster_attempted, set_qa_mode, - set_retry_count, write_item, + bump_retry_count, set_agent, set_blocked, set_depends_on, set_mergemaster_attempted, + set_qa_mode, set_retry_count, write_item, }; pub use migrations::{migrate_names_from_slugs, migrate_story_ids_to_numeric, name_from_story_id};