huskies: merge 919

PLAN.md

@@ -1,22 +1,24 @@
-# Plan: Story 945 — Delete WorkItem flag soup
+# Plan: Story 919
-
-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.
 
 ## 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 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 (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 2 (`Coding (blocked) + Unblock → Coding` unchanged): `server/src/pipeline_state/transition.rs:289` — `(Blocked { .. }, Unblock) => Ok(Coding)` already correct; no change needed.
-- AC 3 (no Option<bool> flag-poking): grep for `set_blocked|set_review_hold|set_frozen|set_mergemaster_attempted` returns no results — DONE.
+- AC 3 (regression test): `server/src/pipeline_state/tests.rs` — add `merge_failure_unblock_returns_to_merge` CRDT-based test.
-- AC 4 (cargo check/clippy/test pass): `run_check` clean, `run_tests` reports 2908 passed / 0 failed — DONE.
 
 ## Decisions
-- Resume target for `Frozen`/`ReviewHold`: stored as a sibling `resume_to: LwwRegisterCrdt<String>` 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).
+- 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).
-- 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.
+- 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).
-- 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.
 
 ## 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

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
+        // Story 919: MergeFailure + Unblock goes to Merge (re-attempt); manual
-        // manual demotion to backlog uses Demote instead.
+        // 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
+    /// Regression test (story 919): unblocking a story in `MergeFailure` via
-    /// `transition_to_unblocked` transitions it to `Stage::Coding`, not `Backlog`.
+    /// `transition_to_unblocked` transitions it to `Stage::Merge`, not `Coding`
-    /// After the unblock, the auto-assigner can pick it up normally (it looks for
+    /// or `Backlog`.  After the unblock, the merge pipeline re-attempts the
-    /// stories in `Coding` / active stages).
+    /// 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
+        // Story must land in Merge — the mergemaster re-attempts the squash.
-        // Coding-stage stories without an extra DepsMet promotion step.
         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",
+            "merge",
-            "MergeFailure story should land in Coding after unblock for immediate retry: {:?}",
+            "MergeFailure story should land in Merge after unblock for immediate re-attempt: {:?}",
             item.stage
         );
         assert!(
-            matches!(item.stage, Stage::Coding),
+            matches!(item.stage, Stage::Merge { .. }),
-            "stage should be Stage::Coding after unblock, got: {:?}",
+            "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
         );
     }

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(),

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(),
                 },
             ),
         ];

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),
+        matches!(result, Stage::Merge { .. }),
-        "MergeFailure + Unblock should return to Coding for immediate retry, got: {result:?}"
+        "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 ─────────────────────────────────────────

server/src/pipeline_state/transition.rs

@@ -197,13 +197,34 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result<Stage, Transitio
         }
 
         // ── MergeFailed: Merge → MergeFailure (recoverable intermediate) ──
-        (Merge { .. }, MergeFailed { reason }) => 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<Stage, Transitio
         (Stage::ReviewHold { resume_to, .. }, ReviewHoldCleared) => 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<Stage, Transitio
         // ── Unblock: Blocked → Coding ─────────────────────────────────
         (Blocked { .. }, Unblock) => Ok(Coding),
 
-        // ── Unblock MergeFailure → Coding (retry) ────────────────────────
+        // ── Unblock MergeFailure → Merge (re-attempt) ────────────────────
-        // `unblock_story` on a failed merge re-enters the coding stage so a
+        // `unblock_story` on a failed merge re-queues it for merge, restoring
-        // coder agent can rework the branch, rather than routing back through
+        // the exact `Merge { feature_branch, commits_ahead }` that was in place
-        // the backlog (which would require an extra DepsMet promotion step).
+        // before the failure so the mergemaster can retry immediately.
-        (MergeFailure { .. }, Unblock) => Ok(Coding),
+        (
+            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

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`
+    /// this is a recoverable intermediate state — `Unblock` returns to `Merge`
-    /// (immediate agent retry) and `Demote` returns to `Backlog` (manual park).
+    /// (re-queues the merge) and `Demote` returns to `Backlog` (manual park).
-    MergeFailure { reason: String },
+    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(),