feat(932): add review_hold CRDT register + migrate callers off yaml_legacy
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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)]
|
||||
|
||||
@@ -348,6 +348,11 @@ pub(super) fn extract_item_view(item: &PipelineItemCrdt) -> Option<PipelineItemV
|
||||
_ => 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<PipelineItemV
|
||||
merged_at,
|
||||
qa_mode,
|
||||
mergemaster_attempted,
|
||||
review_hold,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -78,6 +78,14 @@ pub struct PipelineItemCrdt {
|
||||
/// session for a content-conflict failure. Prevents repeated auto-spawns
|
||||
/// across restarts. Written as `false` (not removed) when cleared.
|
||||
pub mergemaster_attempted: LwwRegisterCrdt<bool>,
|
||||
/// 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<bool>,
|
||||
}
|
||||
|
||||
/// CRDT node that holds a single peer's presence entry.
|
||||
@@ -197,6 +205,8 @@ pub struct WorkItem {
|
||||
pub(super) qa_mode: Option<String>,
|
||||
/// Whether the auto-assigner has already attempted a mergemaster spawn.
|
||||
pub(super) mergemaster_attempted: Option<bool>,
|
||||
/// Whether the item is held for human review (sub-story 932).
|
||||
pub(super) review_hold: Option<bool>,
|
||||
}
|
||||
|
||||
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<f64>,
|
||||
qa_mode: Option<String>,
|
||||
mergemaster_attempted: Option<bool>,
|
||||
review_hold: Option<bool>,
|
||||
) -> Self {
|
||||
Self {
|
||||
story_id: story_id.into(),
|
||||
@@ -298,6 +314,7 @@ impl WorkItem {
|
||||
merged_at,
|
||||
qa_mode,
|
||||
mergemaster_attempted,
|
||||
review_hold,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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};
|
||||
|
||||
Reference in New Issue
Block a user