From aadbb1b2af72d18c914f7cdce2dff03a6b95ca2a Mon Sep 17 00:00:00 2001 From: Timmy Date: Tue, 12 May 2026 19:49:36 +0100 Subject: [PATCH] feat(932): add review_hold CRDT register + migrate callers off yaml_legacy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit review_hold is now a typed bool register on PipelineItemCrdt alongside blocked / mergemaster_attempted. Exposed via the typed setter `crdt_state::set_review_hold(story_id, value)` and the `WorkItem::review_hold()` accessor. Replaces the legacy `review_hold: true` YAML front-matter field. Migrated callers: - http/mcp/qa_tools.rs::tool_approve_qa — clear via set_review_hold(false) - agents/lifecycle.rs::reject_story_from_qa — clear via set_review_hold(false) - agents/pool/pipeline/advance/helpers.rs::write_review_hold_to_store — set via set_review_hold(true), no more content rewrite - agents/pool/auto_assign/reconcile.rs (two callsites) — set via set_review_hold(true) instead of FS YAML write - agents/pool/auto_assign/story_checks.rs::has_review_hold — reads the typed register instead of conflating with Stage::Frozen (real bug fix: the legacy implementation returned `stage.is_frozen()`, which made the auto-assigner treat *every* held-for-review item as frozen even when it wasn't actually parked at the freeze stage). Dead yaml_legacy helpers removed: - write_review_hold(path), write_review_hold_in_content(content) - clear_front_matter_field(path) — last caller was the qa_tools wrap The yaml_residue marker doc now only mentions 933; the 932 line is gone. Co-Authored-By: Claude Opus 4.7 (1M context) --- server/src/agents/lifecycle.rs | 47 ++++++++++------- .../src/agents/pool/auto_assign/reconcile.rs | 29 ++--------- .../agents/pool/auto_assign/story_checks.rs | 51 +++++++++++-------- .../agents/pool/pipeline/advance/helpers.rs | 21 ++------ server/src/crdt_state/mod.rs | 2 +- server/src/crdt_state/read.rs | 6 +++ server/src/crdt_state/types.rs | 17 +++++++ server/src/crdt_state/write/item.rs | 24 +++++++++ server/src/crdt_state/write/mod.rs | 2 +- server/src/db/yaml_legacy.rs | 35 ------------- server/src/http/mcp/qa_tools.rs | 13 +---- server/src/pipeline_state/projection.rs | 5 ++ 12 files changed, 122 insertions(+), 130 deletions(-) diff --git a/server/src/agents/lifecycle.rs b/server/src/agents/lifecycle.rs index b9f6a45e..d46505e8 100644 --- a/server/src/agents/lifecycle.rs +++ b/server/src/agents/lifecycle.rs @@ -212,27 +212,36 @@ pub fn move_story_to_qa(story_id: &str) -> Result<(), String> { .map_err(|e| e.to_string()) } -/// Move a story from `work/3_qa/` back to `work/2_current/`, clearing `review_hold` and writing notes. +/// Move a story from `work/3_qa/` back to `work/2_current/`, clearing +/// `review_hold` (story 932: CRDT register) and appending rejection notes. pub fn reject_story_from_qa(story_id: &str, notes: &str) -> Result<(), String> { - let notes_owned = notes.to_string(); - let transform: Box String> = Box::new(move |content: &str| { - let mut result = clear_front_matter_field_in_content(content, "review_hold"); - if !notes_owned.is_empty() { - result = - crate::db::yaml_legacy::write_rejection_notes_to_content(&result, ¬es_owned); - } - result - }); + crate::crdt_state::set_review_hold(story_id, false); - apply_transition( - story_id, - PipelineEvent::GatesFailed { - reason: notes.to_string(), - }, - Some(&*transform), - ) - .map(|_| ()) - .map_err(|e| e.to_string()) + if notes.is_empty() { + apply_transition( + story_id, + PipelineEvent::GatesFailed { + reason: notes.to_string(), + }, + None, + ) + .map(|_| ()) + .map_err(|e| e.to_string()) + } else { + let notes_owned = notes.to_string(); + let transform = move |content: &str| -> String { + crate::db::yaml_legacy::write_rejection_notes_to_content(content, ¬es_owned) + }; + apply_transition( + story_id, + PipelineEvent::GatesFailed { + reason: notes.to_string(), + }, + Some(&transform), + ) + .map(|_| ()) + .map_err(|e| e.to_string()) + } } /// Transition a story to the `Blocked` stage via the state machine. diff --git a/server/src/agents/pool/auto_assign/reconcile.rs b/server/src/agents/pool/auto_assign/reconcile.rs index 42ad4e86..35e0a28f 100644 --- a/server/src/agents/pool/auto_assign/reconcile.rs +++ b/server/src/agents/pool/auto_assign/reconcile.rs @@ -215,19 +215,8 @@ impl AgentPool { message: format!("Failed to advance to QA: {e}"), }); } else { - // Story 929 / sub-story 932: the review_hold signal still - // lives in YAML on disk because no CRDT register exists. - // Wrapped in `yaml_residue` so the gap is grep-findable. - let story_path = project_root - .join(".huskies/work/3_qa") - .join(format!("{story_id}.md")); - if let Err(e) = crate::db::yaml_legacy::yaml_residue( - crate::db::yaml_legacy::write_review_hold(&story_path), - ) { - eprintln!( - "[startup:reconcile] Failed to set review_hold on '{story_id}': {e}" - ); - } + // Story 932: review_hold is a typed CRDT register. + crate::crdt_state::set_review_hold(story_id, true); eprintln!( "[startup:reconcile] Moved '{story_id}' → 3_qa/ (qa: human — holding for review)." ); @@ -289,18 +278,8 @@ impl AgentPool { }; if needs_human_review { - // Story 929 / sub-story 932: see note above; review_hold - // is YAML-only until the CRDT register exists. - let story_path = project_root - .join(".huskies/work/3_qa") - .join(format!("{story_id}.md")); - if let Err(e) = crate::db::yaml_legacy::yaml_residue( - crate::db::yaml_legacy::write_review_hold(&story_path), - ) { - eprintln!( - "[startup:reconcile] Failed to set review_hold on '{story_id}': {e}" - ); - } + // Story 932: review_hold is a typed CRDT register. + crate::crdt_state::set_review_hold(story_id, true); eprintln!( "[startup:reconcile] '{story_id}' passed QA — holding for human review." ); diff --git a/server/src/agents/pool/auto_assign/story_checks.rs b/server/src/agents/pool/auto_assign/story_checks.rs index b6ef53ee..c322319d 100644 --- a/server/src/agents/pool/auto_assign/story_checks.rs +++ b/server/src/agents/pool/auto_assign/story_checks.rs @@ -21,17 +21,15 @@ pub(super) fn read_story_front_matter_agent( .filter(|s| !s.is_empty()) } -/// Return `true` if the story is in the `Frozen` pipeline stage. +/// Return `true` if the story has its `review_hold` CRDT register set. /// -/// In the typed CRDT model, `Frozen` is the authoritative representation of -/// stories that are held for human review (replacing the legacy -/// `review_hold: true` YAML front-matter field). The typed stage register is -/// the only source consulted — stale YAML is ignored. +/// Sub-story 932: `review_hold` is now a dedicated CRDT register on +/// `PipelineItemCrdt`, distinct from `Stage::Frozen`. The auto-assigner uses +/// this to keep human-QA items / spikes parked after gates pass until a +/// reviewer explicitly clears the hold (e.g. via `tool_approve_qa`). pub(super) fn has_review_hold(_project_root: &Path, _stage_dir: &str, story_id: &str) -> bool { - crate::pipeline_state::read_typed(story_id) - .ok() - .flatten() - .map(|item| item.stage.is_frozen()) + crate::crdt_state::read_item(story_id) + .map(|w| w.review_hold()) .unwrap_or(false) } @@ -138,29 +136,42 @@ mod tests { // ── has_review_hold ─────────────────────────────────────────────────────── #[test] - fn has_review_hold_returns_true_when_frozen() { + fn has_review_hold_returns_true_when_flag_set() { crate::crdt_state::init_for_test(); crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); - crate::db::write_item_with_content( - "890_spike_frozen", - "7_frozen", - "---\nname: Frozen Spike\n---\n# Spike\n", - crate::db::ItemMeta::named("Frozen Spike"), + crate::crdt_state::write_item( + "890_spike_held", + "3_qa", + Some("Held Spike"), + None, + None, + None, + None, + None, + None, + None, ); - assert!(has_review_hold(tmp.path(), "3_qa", "890_spike_frozen")); + crate::crdt_state::set_review_hold("890_spike_held", true); + assert!(has_review_hold(tmp.path(), "3_qa", "890_spike_held")); } #[test] - fn has_review_hold_returns_false_for_qa_stage() { + fn has_review_hold_returns_false_when_flag_unset() { crate::crdt_state::init_for_test(); crate::db::ensure_content_store(); let tmp = tempfile::tempdir().unwrap(); - crate::db::write_item_with_content( + crate::crdt_state::write_item( "890_spike_active_qa", "3_qa", - "---\nname: Active QA Spike\n---\n# Spike\n", - crate::db::ItemMeta::named("Active QA Spike"), + Some("Active QA Spike"), + None, + None, + None, + None, + None, + None, + None, ); assert!(!has_review_hold(tmp.path(), "3_qa", "890_spike_active_qa")); } diff --git a/server/src/agents/pool/pipeline/advance/helpers.rs b/server/src/agents/pool/pipeline/advance/helpers.rs index 39a24d90..2a9e811a 100644 --- a/server/src/agents/pool/pipeline/advance/helpers.rs +++ b/server/src/agents/pool/pipeline/advance/helpers.rs @@ -66,25 +66,10 @@ pub(super) fn resolve_qa_mode_from_store( .unwrap_or(default) } -/// Write review_hold to the content store. +/// Mark a story as held for human review (story 932: CRDT register). pub(super) fn write_review_hold_to_store(story_id: &str) { - if let Some(contents) = crate::db::read_content(story_id) { - let updated = crate::db::yaml_legacy::write_review_hold_in_content(&contents); - crate::db::write_content(story_id, &updated); - // Also persist to SQLite via shadow write. - let stage = crate::pipeline_state::read_typed(story_id) - .ok() - .flatten() - .map(|i| i.stage.dir_name().to_string()) - .unwrap_or_else(|| "3_qa".to_string()); - crate::db::write_item_with_content( - story_id, - &stage, - &updated, - crate::db::ItemMeta::from_yaml(&updated), - ); - } else { - slog_error!("[pipeline] Cannot write review_hold for '{story_id}': no content in store"); + if !crate::crdt_state::set_review_hold(story_id, true) { + slog_error!("[pipeline] Cannot set review_hold for '{story_id}': no CRDT entry"); } } diff --git a/server/src/crdt_state/mod.rs b/server/src/crdt_state/mod.rs index ef21ab6b..ed3da9dd 100644 --- a/server/src/crdt_state/mod.rs +++ b/server/src/crdt_state/mod.rs @@ -54,7 +54,7 @@ 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_blocked, set_depends_on, set_mergemaster_attempted, set_qa_mode, - set_retry_count, write_item, + set_retry_count, set_review_hold, write_item, }; #[cfg(test)] diff --git a/server/src/crdt_state/read.rs b/server/src/crdt_state/read.rs index b0ea1ebf..63e43c0f 100644 --- a/server/src/crdt_state/read.rs +++ b/server/src/crdt_state/read.rs @@ -348,6 +348,11 @@ pub(super) fn extract_item_view(item: &PipelineItemCrdt) -> Option None, }; + let review_hold = match item.review_hold.view() { + JsonValue::Bool(b) => Some(b), + _ => None, + }; + Some(PipelineItemView { story_id, stage, @@ -361,6 +366,7 @@ pub(super) fn extract_item_view(item: &PipelineItemCrdt) -> Option, + /// Set to `true` when a story is held for human review at a pipeline stage + /// boundary (e.g. spikes after QA passes; human-QA items after gates pass). + /// The auto-assigner skips items with this flag so a human can inspect the + /// state before the pipeline continues. Written as `false` (not removed) + /// when explicitly cleared (e.g. by `tool_approve_qa`). Sub-story 932 of + /// the 929 CRDT-only migration; replaces the legacy `review_hold: true` + /// YAML front-matter field. + pub review_hold: LwwRegisterCrdt, } /// CRDT node that holds a single peer's presence entry. @@ -197,6 +205,8 @@ pub struct WorkItem { pub(super) qa_mode: Option, /// Whether the auto-assigner has already attempted a mergemaster spawn. pub(super) mergemaster_attempted: Option, + /// Whether the item is held for human review (sub-story 932). + pub(super) review_hold: Option, } impl WorkItem { @@ -265,6 +275,11 @@ impl WorkItem { self.mergemaster_attempted.unwrap_or(false) } + /// Whether the item is held for human review. Returns `false` when unset. + pub fn review_hold(&self) -> bool { + self.review_hold.unwrap_or(false) + } + /// Construct a `WorkItem` for use in tests outside `crdt_state::*`. /// /// Within `crdt_state` use a struct literal directly (fields are `pub(super)`). @@ -284,6 +299,7 @@ impl WorkItem { merged_at: Option, qa_mode: Option, mergemaster_attempted: Option, + review_hold: Option, ) -> Self { Self { story_id: story_id.into(), @@ -298,6 +314,7 @@ impl WorkItem { merged_at, qa_mode, mergemaster_attempted, + review_hold, } } } diff --git a/server/src/crdt_state/write/item.rs b/server/src/crdt_state/write/item.rs index c07405b0..196d6fc4 100644 --- a/server/src/crdt_state/write/item.rs +++ b/server/src/crdt_state/write/item.rs @@ -38,6 +38,28 @@ pub fn set_depends_on(story_id: &str, deps: &[u32]) -> bool { true } +/// Set the `review_hold` CRDT flag for a pipeline item (sub-story 932). +/// +/// `true` marks the item as held for human review at a pipeline-stage boundary; +/// the auto-assigner skips items with this flag. `false` clears the hold and +/// is written explicitly (the register is not removed) so the cleared state +/// survives CRDT replay correctly. +/// +/// Returns `true` if the item was found and the op was applied, `false` otherwise. +pub fn set_review_hold(story_id: &str, value: bool) -> bool { + let Some(state_mutex) = get_crdt() else { + return false; + }; + let Ok(mut state) = state_mutex.lock() else { + return false; + }; + let Some(&idx) = state.index.get(story_id) else { + return false; + }; + apply_and_persist(&mut state, |s| s.crdt.doc.items[idx].review_hold.set(value)); + true +} + /// Set the `mergemaster_attempted` CRDT flag for a pipeline item. /// /// Passing `true` records that a mergemaster session has been spawned for this @@ -227,6 +249,7 @@ pub fn write_item( "merged_at": merged_at.unwrap_or(0.0), "qa_mode": "", "mergemaster_attempted": false, + "review_hold": false, }) .into(); @@ -254,6 +277,7 @@ pub fn write_item( item.merged_at.advance_seq(floor); item.qa_mode.advance_seq(floor); item.mergemaster_attempted.advance_seq(floor); + item.review_hold.advance_seq(floor); } // Broadcast a CrdtEvent for the new item. diff --git a/server/src/crdt_state/write/mod.rs b/server/src/crdt_state/write/mod.rs index 2f82bcc7..e9bd2aae 100644 --- a/server/src/crdt_state/write/mod.rs +++ b/server/src/crdt_state/write/mod.rs @@ -11,6 +11,6 @@ mod tests; pub use item::{ bump_retry_count, set_agent, set_blocked, set_depends_on, set_mergemaster_attempted, - set_qa_mode, set_retry_count, write_item, + set_qa_mode, set_retry_count, set_review_hold, write_item, }; pub use migrations::{migrate_names_from_slugs, migrate_story_ids_to_numeric, name_from_story_id}; diff --git a/server/src/db/yaml_legacy.rs b/server/src/db/yaml_legacy.rs index 3fccfd89..f1e0977a 100644 --- a/server/src/db/yaml_legacy.rs +++ b/server/src/db/yaml_legacy.rs @@ -9,8 +9,6 @@ use crate::io::story_metadata::QaMode; use serde::Deserialize; -use std::fs; -use std::path::Path; /// Identity wrapper that flags a yaml_legacy callsite blocked on adding a /// CRDT register (story 929 residue). Every wrap is a grep-findable marker — @@ -22,8 +20,6 @@ use std::path::Path; /// entirely (929 stage 10). /// /// Filed sub-stories enumerate each gap: -/// - 932: `review_hold` flag (write-side in qa_tools, read-side in -/// auto_assign). /// - 933: epic mechanism — `item_type` and `epic` link fields. pub fn yaml_residue(v: T) -> T { v @@ -200,37 +196,6 @@ pub(crate) fn write_merge_failure_in_content(contents: &str, reason: &str) -> St set_front_matter_field(contents, "merge_failure", &format!("\"{escaped}\"")) } -/// Write `review_hold: true` to story content. -pub(crate) fn write_review_hold_in_content(contents: &str) -> String { - set_front_matter_field(contents, "review_hold", "true") -} - -/// Remove a key from the YAML front matter of a story file on disk. -/// -/// Legacy filesystem-backed wrapper around -/// [`clear_front_matter_field_in_content`] for the small number of callers -/// that still read story files directly. -pub(crate) fn clear_front_matter_field(path: &Path, key: &str) -> Result<(), String> { - let contents = - fs::read_to_string(path).map_err(|e| format!("Failed to read story file: {e}"))?; - let updated = clear_front_matter_field_in_content(&contents, key); - if updated != contents { - fs::write(path, &updated).map_err(|e| format!("Failed to write story file: {e}"))?; - } - Ok(()) -} - -/// Write `review_hold: true` to the YAML front matter of a story file on disk. -/// -/// Legacy filesystem-backed wrapper around [`write_review_hold_in_content`]. -pub(crate) fn write_review_hold(path: &Path) -> Result<(), String> { - let contents = - fs::read_to_string(path).map_err(|e| format!("Failed to read story file: {e}"))?; - let updated = write_review_hold_in_content(&contents); - fs::write(path, &updated).map_err(|e| format!("Failed to write story file: {e}"))?; - Ok(()) -} - #[cfg(test)] mod tests { use super::*; diff --git a/server/src/http/mcp/qa_tools.rs b/server/src/http/mcp/qa_tools.rs index beec1fc0..79f86e2e 100644 --- a/server/src/http/mcp/qa_tools.rs +++ b/server/src/http/mcp/qa_tools.rs @@ -54,17 +54,8 @@ pub(super) async fn tool_approve_qa(args: &Value, ctx: &AppContext) -> Result