From 09a8edc0a185f4c62a6de5c73a905639d7492274 Mon Sep 17 00:00:00 2001 From: dave Date: Wed, 13 May 2026 06:22:22 +0000 Subject: [PATCH] huskies: merge 919 --- PLAN.md | 30 ++++++----- server/src/agents/lifecycle.rs | 36 ++++++------- server/src/crdt_state/read.rs | 2 + server/src/crdt_state/write/migrations.rs | 2 + server/src/pipeline_state/tests.rs | 63 +++++++++++++++++++++-- server/src/pipeline_state/transition.rs | 47 ++++++++++++++--- server/src/pipeline_state/types.rs | 14 +++-- 7 files changed, 146 insertions(+), 48 deletions(-) diff --git a/PLAN.md b/PLAN.md index 68d7d159..e975c3ed 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1,22 +1,24 @@ -# Plan: Story 945 — Delete WorkItem flag soup - -Goal: Fold `blocked`, `review_hold`, `frozen`, and `mergemaster_attempted` -flag fields into `Stage` / `ArchiveReason` / `ExecutionState` variants so the -typed state machine is the single source of truth. +# Plan: Story 919 ## ACs → implementation locations -- AC 1 (delete flag fields): `server/src/crdt_state/types.rs` (PipelineItemCrdt, WorkItem) — DONE; `server/src/pipeline_state/types.rs` (PipelineItem) — DONE. -- AC 2 (variants + `match`-based callers): New variants `Stage::MergeFailureFinal`, `Stage::Frozen { resume_to }`, `Stage::ReviewHold { resume_to, reason }` in `pipeline_state/types.rs` — DONE. Auto-assigner, watchdog, unblock/freeze/unfreeze, merge pipeline all `match` on Stage — DONE. -- AC 3 (no Option flag-poking): grep for `set_blocked|set_review_hold|set_frozen|set_mergemaster_attempted` returns no results — DONE. -- AC 4 (cargo check/clippy/test pass): `run_check` clean, `run_tests` reports 2908 passed / 0 failed — DONE. +- AC 1 (`MergeFailure + Unblock → Merge`): `server/src/pipeline_state/transition.rs:295` — change `Ok(Coding)` to `Ok(Merge { feature_branch, commits_ahead })`. Also requires adding `feature_branch: BranchName` and `commits_ahead: NonZeroU32` to `Stage::MergeFailure` in `server/src/pipeline_state/types.rs:113`, and carrying those fields through the `Merge → MergeFailure` transition at `transition.rs:200`. +- AC 2 (`Coding (blocked) + Unblock → Coding` unchanged): `server/src/pipeline_state/transition.rs:289` — `(Blocked { .. }, Unblock) => Ok(Coding)` already correct; no change needed. +- AC 3 (regression test): `server/src/pipeline_state/tests.rs` — add `merge_failure_unblock_returns_to_merge` CRDT-based test. ## Decisions -- Resume target for `Frozen`/`ReviewHold`: stored as a sibling `resume_to: LwwRegisterCrdt` on `PipelineItemCrdt` rather than encoded into the stage string. Rejected: encoding into stage register (would require parsing variants out of strings and lose round-trip cleanness). -- Reason text for `Blocked`/`MergeFailure`/`MergeFailureFinal`/`ReviewHold`: kept on the Stage variant in memory, but the wire-form stage register only carries the canonical dir name (`"merge_failure_final"`, `"review_hold"`). Reasons are reconstructed at read time from companion CRDT data (MergeJob.error) where needed. Acceptable because reason is human-text, not load-bearing for routing. -- Pre-934 `7_frozen` legacy migration: rewrites stage to `"frozen"` and sets `resume_to = "backlog"`, restoring `Stage::Frozen { resume_to: Backlog }` on read. The defensive projection fallback still maps raw `7_frozen` → `Backlog` for un-migrated reads. +- Add `feature_branch` and `commits_ahead` to `MergeFailure` (rather than use synthetic defaults in the transition): allows the exact merge state to be restored on Unblock. Rejected: synthetic values at transition time (would require story_id not available in the transition function, or use a placeholder that loses the real branch name). +- Update all `Stage::MergeFailure { reason }` construction sites to include the new fields with synthetic defaults where real values are unavailable (CRDT read, `from_dir`, migration). ## Current state -All flag fields removed; all 47 modified files compile; 2908 tests pass; doc coverage clean. +Not started — fresh session. ## What's left -- [x] Commit changes +- [ ] Add `feature_branch: BranchName` and `commits_ahead: NonZeroU32` to `Stage::MergeFailure` in `types.rs` +- [ ] Update `types.rs` `from_dir` construction of `MergeFailure` +- [ ] Update `transition.rs`: carry fields in `Merge → MergeFailure`, self-loop, fix `MergeFailure + Unblock → Merge` +- [ ] Update `crdt_state/read.rs` `MergeFailure` construction +- [ ] Update `crdt_state/write/migrations.rs` `MergeFailure` construction +- [ ] Update `tests.rs`: all `Stage::MergeFailure { .. }` constructions, rename+fix unblock test +- [ ] Add CRDT-based regression test (AC3) +- [ ] Run run_check, fix any clippy/fmt issues +- [ ] Run run_tests, commit diff --git a/server/src/agents/lifecycle.rs b/server/src/agents/lifecycle.rs index ac31d419..2cd934c6 100644 --- a/server/src/agents/lifecycle.rs +++ b/server/src/agents/lifecycle.rs @@ -282,10 +282,11 @@ pub fn transition_to_merge_failure( Ok(fired) } -/// Transition a story out of `Blocked` back to `Coding` via the state machine. +/// Transition a story out of a blocked state via the state machine. /// /// Builds a `PipelineEvent::Unblock`, validates the transition, writes the -/// resulting `Stage::Coding` to the CRDT, and resets `retry_count` to 0. +/// result to the CRDT, and resets `retry_count` to 0. The destination stage +/// depends on the current stage: `Blocked` → `Coding`; `MergeFailure` → `Merge`. /// Returns `Err` on `TransitionError` — callers must NOT fall back to direct /// register writes. pub fn transition_to_unblocked(story_id: &str) -> Result<(), String> { @@ -333,8 +334,8 @@ fn map_stage_move_to_event( }), (Stage::Coding | Stage::Qa | Stage::Backlog, "done") => Ok(PipelineEvent::Close), (Stage::Blocked { .. }, "current") => Ok(PipelineEvent::Unblock), - // Story 893: MergeFailure + Unblock now goes to Coding (retry), so - // manual demotion to backlog uses Demote instead. + // Story 919: MergeFailure + Unblock goes to Merge (re-attempt); manual + // demotion to backlog uses Demote to park it without a retry. (Stage::MergeFailure { .. }, "backlog") => Ok(PipelineEvent::Demote), ( Stage::Archived { @@ -596,14 +597,14 @@ mod tests { ); } - // ── Story 893: MergeFailure unblock → Coding regression ───────────────── + // ── Story 919: MergeFailure unblock → Merge regression ────────────────── - /// Regression test (story 893): unblocking a story in `MergeFailure` via - /// `transition_to_unblocked` transitions it to `Stage::Coding`, not `Backlog`. - /// After the unblock, the auto-assigner can pick it up normally (it looks for - /// stories in `Coding` / active stages). + /// Regression test (story 919): unblocking a story in `MergeFailure` via + /// `transition_to_unblocked` transitions it to `Stage::Merge`, not `Coding` + /// or `Backlog`. After the unblock, the merge pipeline re-attempts the + /// squash-merge immediately. #[test] - fn unblock_merge_failure_story_lands_in_coding() { + fn unblock_merge_failure_story_lands_in_merge() { crate::db::ensure_content_store(); crate::db::write_item_with_content( "99893_story_merge_failure_unblock", @@ -626,26 +627,25 @@ mod tests { transition_to_unblocked("99893_story_merge_failure_unblock") .expect("transition_to_unblocked should succeed for MergeFailure story"); - // Story must land in Coding, not Backlog — the auto-assigner picks up - // Coding-stage stories without an extra DepsMet promotion step. + // Story must land in Merge — the mergemaster re-attempts the squash. let item = crate::pipeline_state::read_typed("99893_story_merge_failure_unblock") .expect("CRDT read should succeed") .expect("item should exist after unblock"); assert_eq!( item.stage.dir_name(), - "coding", - "MergeFailure story should land in Coding after unblock for immediate retry: {:?}", + "merge", + "MergeFailure story should land in Merge after unblock for immediate re-attempt: {:?}", item.stage ); assert!( - matches!(item.stage, Stage::Coding), - "stage should be Stage::Coding after unblock, got: {:?}", + matches!(item.stage, Stage::Merge { .. }), + "stage should be Stage::Merge after unblock, got: {:?}", item.stage ); - // auto_assign checks is_active() — Coding satisfies it. + // auto_assign checks is_active() — Merge satisfies it. assert!( item.stage.is_active(), - "Coding satisfies is_active() so auto_assign can pick it up: {:?}", + "Merge satisfies is_active() so auto_assign can pick it up: {:?}", item.stage ); } diff --git a/server/src/crdt_state/read.rs b/server/src/crdt_state/read.rs index 7569672d..51d0e08d 100644 --- a/server/src/crdt_state/read.rs +++ b/server/src/crdt_state/read.rs @@ -432,6 +432,8 @@ fn project_stage_for_view( }), "merge_failure" => Some(Stage::MergeFailure { reason: String::new(), + feature_branch: BranchName(format!("feature/story-{story_id}")), + commits_ahead: NonZeroU32::new(1).expect("1 is non-zero"), }), "merge_failure_final" => Some(Stage::MergeFailureFinal { reason: String::new(), diff --git a/server/src/crdt_state/write/migrations.rs b/server/src/crdt_state/write/migrations.rs index 49fd69ef..60d509b0 100644 --- a/server/src/crdt_state/write/migrations.rs +++ b/server/src/crdt_state/write/migrations.rs @@ -339,6 +339,8 @@ mod stage_migration_tests { "4_merge_failure", Stage::MergeFailure { reason: String::new(), + feature_branch: crate::pipeline_state::BranchName(String::new()), + commits_ahead: NonZeroU32::new(1).unwrap(), }, ), ]; diff --git a/server/src/pipeline_state/tests.rs b/server/src/pipeline_state/tests.rs index 9fb6dfbf..283e4986 100644 --- a/server/src/pipeline_state/tests.rs +++ b/server/src/pipeline_state/tests.rs @@ -687,6 +687,8 @@ fn merge_failure_transition_emits_event_with_full_reason() { fn merge_failure_plus_merge_failed_is_self_loop() { let s = Stage::MergeFailure { reason: "initial failure".into(), + feature_branch: fb("feature/story-1"), + commits_ahead: nz(1), }; let result = transition( s, @@ -758,18 +760,20 @@ fn repeated_merge_failure_apply_transition_no_error_no_duplicate_notification() ); } -// ── Story 893: MergeFailure + Unblock → Coding (retry) ───────────── +// ── Story 919: MergeFailure + Unblock → Merge (re-attempt) ───────── -/// AC1: `MergeFailure + Unblock` transitions to `Coding` (retry), not `Backlog`. +/// AC1: `MergeFailure + Unblock` transitions to `Merge` (re-attempt), not `Coding` or `Backlog`. #[test] -fn merge_failure_unblock_returns_to_coding() { +fn merge_failure_unblock_returns_to_merge() { let s = Stage::MergeFailure { reason: "conflicts in server/src/main.rs".into(), + feature_branch: fb("feature/story-42"), + commits_ahead: nz(3), }; let result = transition(s, PipelineEvent::Unblock).unwrap(); assert!( - matches!(result, Stage::Coding), - "MergeFailure + Unblock should return to Coding for immediate retry, got: {result:?}" + matches!(result, Stage::Merge { .. }), + "MergeFailure + Unblock should return to Merge for immediate re-attempt, got: {result:?}" ); } @@ -778,6 +782,8 @@ fn merge_failure_unblock_returns_to_coding() { fn merge_failure_demote_returns_to_backlog() { let s = Stage::MergeFailure { reason: "conflicts".into(), + feature_branch: fb("feature/story-1"), + commits_ahead: nz(1), }; let result = transition(s, PipelineEvent::Demote).unwrap(); assert!( @@ -794,6 +800,8 @@ fn merge_failure_demote_returns_to_backlog() { fn merge_failure_accept_pure_transition() { let s = Stage::MergeFailure { reason: "conflicts unresolvable".into(), + feature_branch: fb("feature/story-1"), + commits_ahead: nz(1), }; let result = transition(s, PipelineEvent::Accepted).unwrap(); assert!( @@ -852,4 +860,49 @@ fn merge_failure_accept_moves_to_done_via_crdt() { ); } +// ── Story 919: MergeFailure + Unblock → Merge (regression) ───────── + +/// AC3: CRDT-based regression — set stage to `MergeFailure`, call `unblock_story` +/// via `apply_transition`, assert the Stage register becomes `Stage::Merge`. +#[test] +fn merge_failure_unblock_moves_to_merge_via_crdt() { + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + + let story_id = "99919_story_merge_failure_unblock"; + crate::db::write_item_with_content( + story_id, + "merge_failure", + "---\nname: MergeFailure Unblock Regression\n---\n# Story\n", + crate::db::ItemMeta::named("MergeFailure Unblock Regression"), + ); + + let fired = super::apply::apply_transition(story_id, PipelineEvent::Unblock, None) + .expect("MergeFailure + Unblock should succeed"); + + assert!( + matches!(fired.before, Stage::MergeFailure { .. }), + "fired.before should be MergeFailure: {:?}", + fired.before + ); + assert!( + matches!(fired.after, Stage::Merge { .. }), + "fired.after should be Merge, not Coding or Backlog: {:?}", + fired.after + ); + + let item = read_typed(story_id) + .expect("CRDT read should succeed") + .expect("item should exist"); + assert_eq!( + item.stage.dir_name(), + "merge", + "CRDT stage should be merge after MergeFailure + Unblock" + ); + assert!( + !matches!(item.stage, Stage::MergeFailure { .. }), + "MergeFailure variant must not remain after Unblock" + ); +} + // ── ProjectionError Display ───────────────────────────────────────── diff --git a/server/src/pipeline_state/transition.rs b/server/src/pipeline_state/transition.rs index 51fdfb18..3a5fe58e 100644 --- a/server/src/pipeline_state/transition.rs +++ b/server/src/pipeline_state/transition.rs @@ -197,13 +197,34 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result Ok(MergeFailure { reason }), + ( + Merge { + feature_branch, + commits_ahead, + }, + MergeFailed { reason }, + ) => Ok(MergeFailure { + reason, + feature_branch, + commits_ahead, + }), // ── MergeFailure self-loop: repeated failure is a no-op ───────────── // When the mergemaster retries and fails again while the story is already // in MergeFailure, treat it as a silent self-transition so callers can // detect the no-op via `fired.before == MergeFailure` and skip re-notifying. - (MergeFailure { .. }, MergeFailed { reason }) => Ok(MergeFailure { reason }), + ( + MergeFailure { + feature_branch, + commits_ahead, + .. + }, + MergeFailed { reason }, + ) => Ok(MergeFailure { + reason, + feature_branch, + commits_ahead, + }), (Merge { .. }, MergeFailedFinal { reason }) => Ok(Archived { archived_at: now, @@ -278,7 +299,7 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result Ok(*resume_to), // ── MergemasterAttempted: MergeFailure → MergeFailureFinal ───── - (MergeFailure { reason }, MergemasterAttempted) => Ok(MergeFailureFinal { reason }), + (MergeFailure { reason, .. }, MergemasterAttempted) => Ok(MergeFailureFinal { reason }), (MergeFailureFinal { reason }, MergemasterAttempted) => Ok(MergeFailureFinal { reason }), // ── Unblock: from Frozen/ReviewHold → resume_to ──────────────── @@ -288,11 +309,21 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result Ok(Coding), - // ── Unblock MergeFailure → Coding (retry) ──────────────────────── - // `unblock_story` on a failed merge re-enters the coding stage so a - // coder agent can rework the branch, rather than routing back through - // the backlog (which would require an extra DepsMet promotion step). - (MergeFailure { .. }, Unblock) => Ok(Coding), + // ── Unblock MergeFailure → Merge (re-attempt) ──────────────────── + // `unblock_story` on a failed merge re-queues it for merge, restoring + // the exact `Merge { feature_branch, commits_ahead }` that was in place + // before the failure so the mergemaster can retry immediately. + ( + MergeFailure { + feature_branch, + commits_ahead, + .. + }, + Unblock, + ) => Ok(Merge { + feature_branch, + commits_ahead, + }), // ── Demote MergeFailure → Backlog (manual parking) ─────────────── // Lets operators park a failed-merge story in the backlog without an diff --git a/server/src/pipeline_state/types.rs b/server/src/pipeline_state/types.rs index 904728b0..44fd6983 100644 --- a/server/src/pipeline_state/types.rs +++ b/server/src/pipeline_state/types.rs @@ -108,9 +108,15 @@ pub enum Stage { /// Merge pipeline failed (conflicts or gate failures). Story is held here /// awaiting human intervention or retry. Unlike `Archived(MergeFailed)`, - /// this is a recoverable intermediate state — `Unblock` returns to `Coding` - /// (immediate agent retry) and `Demote` returns to `Backlog` (manual park). - MergeFailure { reason: String }, + /// this is a recoverable intermediate state — `Unblock` returns to `Merge` + /// (re-queues the merge) and `Demote` returns to `Backlog` (manual park). + MergeFailure { + reason: String, + /// Branch and commit count preserved from the preceding `Merge` state + /// so `Unblock` can reconstruct the exact `Merge` variant. + feature_branch: BranchName, + commits_ahead: NonZeroU32, + }, /// Merge pipeline failed AND mergemaster has already been auto-spawned to /// recover; the agent gave up. The story stays here awaiting human @@ -234,6 +240,8 @@ impl Stage { }), "merge_failure" => Some(Stage::MergeFailure { reason: String::new(), + feature_branch: BranchName(String::new()), + commits_ahead: NonZeroU32::new(1).expect("1 is non-zero"), }), "merge_failure_final" => Some(Stage::MergeFailureFinal { reason: String::new(),