huskies: merge 987

2026-05-13 16:26:09 +00:00
parent 430079ecbc
commit c3c9db3d8b
13 changed files with 662 additions and 311 deletions
@@ -52,7 +52,7 @@ pub use types::{
    TestJobCrdt, TestJobView, TokenUsageCrdt, TokenUsageView, WorkItem,
 };
 pub use write::{
-    bump_retry_count, migrate_legacy_stage_strings, migrate_names_from_slugs,
+    bump_retry_count, migrate_legacy_stage_strings, migrate_merge_job, migrate_names_from_slugs,
    migrate_story_ids_to_numeric, name_from_story_id, set_agent, set_depends_on, set_epic,
    set_item_type, set_name, set_qa_mode, set_resume_to, set_retry_count, write_item,
 };
@@ -1,7 +1,8 @@
-//! Name and story-ID migration helpers for pipeline items.
+//! Name, story-ID, and MergeJob migration helpers for pipeline items.
 //!
 //! Contains one-time startup migrations that backfill the `name` field from
-//! story ID slugs and rewrite slug-form story IDs to numeric-only form.
+//! story ID slugs, rewrite slug-form story IDs to numeric-only form, and
+//! upgrade four-bool MergeJob CRDT entries to the typed [`MergeResult`] enum.

 use bft_json_crdt::json_crdt::{CrdtNode, JsonValue};

@@ -450,3 +451,346 @@ mod stage_migration_tests {
        migrate_legacy_stage_strings();
    }
 }
+
+// ── MergeJob migration ─────────────────────────────────────────────────────
+
+/// Detect whether a JSON string uses the old four-bool MergeReport format
+/// (pre-story-987) and convert it to the new typed [`MergeResult`] format.
+///
+/// Returns `None` when the input is already in the new format or cannot be
+/// parsed.
+fn upgrade_merge_report_json(json: &str) -> Option<String> {
+    let v: serde_json::Value = serde_json::from_str(json).ok()?;
+    // New format has a "kind" field inside the "result" object.
+    // Old format has top-level bool fields.
+    if v.get("result").and_then(|r| r.get("kind")).is_some() {
+        return None; // Already new format.
+    }
+    // Must have at least one of the old bool fields to be recognised as old format.
+    if v.get("success").is_none() && v.get("had_conflicts").is_none() {
+        return None;
+    }
+
+    let story_id = v["story_id"].as_str().unwrap_or("").to_string();
+    let success = v["success"].as_bool().unwrap_or(false);
+    let had_conflicts = v["had_conflicts"].as_bool().unwrap_or(false);
+    let conflicts_resolved = v["conflicts_resolved"].as_bool().unwrap_or(false);
+    let conflict_details: Option<String> = v["conflict_details"].as_str().map(|s| s.to_string());
+    let gates_passed = v["gates_passed"].as_bool().unwrap_or(false);
+    let gate_output = v["gate_output"].as_str().unwrap_or("").to_string();
+    let no_commits = v["no_commits"].as_bool().unwrap_or(false);
+    let worktree_cleaned_up = v["worktree_cleaned_up"].as_bool().unwrap_or(false);
+    let story_archived = v["story_archived"].as_bool().unwrap_or(false);
+
+    // Reconstruct the typed MergeResult from the old bools.
+    let result = if no_commits {
+        serde_json::json!({ "kind": "NoCommits", "output": gate_output })
+    } else if had_conflicts && !conflicts_resolved {
+        serde_json::json!({
+            "kind": "Conflict",
+            "details": conflict_details,
+            "output": gate_output,
+        })
+    } else if success && gates_passed {
+        serde_json::json!({
+            "kind": "Success",
+            "conflicts_resolved": conflicts_resolved,
+            "conflict_details": conflict_details,
+            "gate_output": gate_output,
+        })
+    } else if !gates_passed {
+        serde_json::json!({
+            "kind": "GateFailure",
+            "output": gate_output,
+        })
+    } else {
+        serde_json::json!({
+            "kind": "Other",
+            "output": gate_output,
+            "conflict_details": conflict_details,
+        })
+    };
+
+    let new_report = serde_json::json!({
+        "story_id": story_id,
+        "result": result,
+        "worktree_cleaned_up": worktree_cleaned_up,
+        "story_archived": story_archived,
+    });
+    serde_json::to_string(&new_report).ok()
+}
+
+/// Migrate existing completed MergeJob CRDT entries from the old four-bool
+/// `MergeReport` JSON to the new typed [`MergeResult`] enum format.
+///
+/// Before rewriting any entries, snapshots the current CRDT database file to
+/// `.huskies/backups/pre_merge_result_migration_<ts>.db` so a botched
+/// migration can be undone without manual SQLite surgery.
+///
+/// Running this migration repeatedly is safe — subsequent calls on
+/// already-migrated state are no-ops.
+pub fn migrate_merge_job(db_path: &std::path::Path) {
+    // First pass: collect (story_id, new_json, started_at, finished_at) using
+    // public read APIs so we don't need to hold the lock across the snapshot.
+    let jobs = match crate::crdt_state::read_all_merge_jobs() {
+        Some(j) => j,
+        None => return,
+    };
+
+    let to_migrate: Vec<(String, String, f64, Option<f64>)> = jobs
+        .into_iter()
+        .filter_map(|view| {
+            if view.status != "completed" {
+                return None;
+            }
+            let error_json = view.error?;
+            let new_json = upgrade_merge_report_json(&error_json)?;
+            Some((view.story_id, new_json, view.started_at, view.finished_at))
+        })
+        .collect();
+
+    if to_migrate.is_empty() {
+        return;
+    }
+
+    // Snapshot the database before making any changes (AC 6).
+    if let Some(parent) = db_path.parent() {
+        let backups_dir = parent.join("backups");
+        if std::fs::create_dir_all(&backups_dir).is_ok() {
+            let ts = std::time::SystemTime::now()
+                .duration_since(std::time::UNIX_EPOCH)
+                .map(|d| d.as_secs())
+                .unwrap_or(0);
+            let backup = backups_dir.join(format!("pre_merge_result_migration_{ts}.db"));
+            if let Err(e) = std::fs::copy(db_path, &backup) {
+                slog!(
+                    "[crdt] Warning: could not snapshot pipeline.db before \
+                     MergeJob migration: {e}"
+                );
+            } else {
+                slog!(
+                    "[crdt] Snapshotted pipeline.db to {} before MergeJob migration",
+                    backup.display()
+                );
+            }
+        }
+    }
+
+    // Second pass: rewrite each entry's error field via the public write API.
+    let count = to_migrate.len();
+    for (story_id, new_json, started_at, finished_at) in to_migrate {
+        crate::crdt_state::write_merge_job(
+            &story_id,
+            "completed",
+            started_at,
+            finished_at,
+            Some(&new_json),
+        );
+    }
+    slog!("[crdt] Migrated {count} MergeJob entries to typed MergeResult format");
+}
+
+#[cfg(test)]
+mod merge_job_migration_tests {
+    use super::super::super::state::init_for_test;
+    use super::*;
+    use crate::crdt_state::write_merge_job;
+
+    struct OldReport<'a> {
+        success: bool,
+        had_conflicts: bool,
+        conflicts_resolved: bool,
+        gates_passed: bool,
+        no_commits: bool,
+        gate_output: &'a str,
+        conflict_details: Option<&'a str>,
+    }
+
+    fn seed_old_format(story_id: &str, r: OldReport<'_>) {
+        let (
+            success,
+            had_conflicts,
+            conflicts_resolved,
+            gates_passed,
+            no_commits,
+            gate_output,
+            conflict_details,
+        ) = (
+            r.success,
+            r.had_conflicts,
+            r.conflicts_resolved,
+            r.gates_passed,
+            r.no_commits,
+            r.gate_output,
+            r.conflict_details,
+        );
+        let old_json = serde_json::to_string(&serde_json::json!({
+            "story_id": story_id,
+            "success": success,
+            "had_conflicts": had_conflicts,
+            "conflicts_resolved": conflicts_resolved,
+            "conflict_details": conflict_details,
+            "gates_passed": gates_passed,
+            "gate_output": gate_output,
+            "gate_failure_kind": null,
+            "no_commits": no_commits,
+            "worktree_cleaned_up": false,
+            "story_archived": false,
+        }))
+        .unwrap();
+        write_merge_job(story_id, "completed", 1.0, Some(2.0), Some(&old_json));
+    }
+
+    fn read_result_kind(story_id: &str) -> Option<String> {
+        let view = crate::crdt_state::read_merge_job(story_id)?;
+        let json_str = view.error?;
+        let v: serde_json::Value = serde_json::from_str(&json_str).ok()?;
+        v["result"]["kind"].as_str().map(|s| s.to_string())
+    }
+
+    #[test]
+    fn migrates_success_variant() {
+        init_for_test();
+        seed_old_format(
+            "9600_success",
+            OldReport {
+                success: true,
+                had_conflicts: false,
+                conflicts_resolved: false,
+                gates_passed: true,
+                no_commits: false,
+                gate_output: "gates ok",
+                conflict_details: None,
+            },
+        );
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+        assert_eq!(read_result_kind("9600_success").as_deref(), Some("Success"));
+    }
+
+    #[test]
+    fn migrates_conflict_variant() {
+        init_for_test();
+        seed_old_format(
+            "9601_conflict",
+            OldReport {
+                success: false,
+                had_conflicts: true,
+                conflicts_resolved: false,
+                gates_passed: false,
+                no_commits: false,
+                gate_output: "conflicts",
+                conflict_details: Some("conflict details"),
+            },
+        );
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+        assert_eq!(
+            read_result_kind("9601_conflict").as_deref(),
+            Some("Conflict")
+        );
+    }
+
+    #[test]
+    fn migrates_gate_failure_variant() {
+        init_for_test();
+        seed_old_format(
+            "9602_gates",
+            OldReport {
+                success: false,
+                had_conflicts: false,
+                conflicts_resolved: false,
+                gates_passed: false,
+                no_commits: false,
+                gate_output: "tests failed",
+                conflict_details: None,
+            },
+        );
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+        assert_eq!(
+            read_result_kind("9602_gates").as_deref(),
+            Some("GateFailure")
+        );
+    }
+
+    #[test]
+    fn migrates_no_commits_variant() {
+        init_for_test();
+        seed_old_format(
+            "9603_nocommits",
+            OldReport {
+                success: false,
+                had_conflicts: false,
+                conflicts_resolved: false,
+                gates_passed: false,
+                no_commits: true,
+                gate_output: "no commits to merge",
+                conflict_details: None,
+            },
+        );
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+        assert_eq!(
+            read_result_kind("9603_nocommits").as_deref(),
+            Some("NoCommits")
+        );
+    }
+
+    #[test]
+    fn migrates_other_variant() {
+        init_for_test();
+        seed_old_format(
+            "9604_other",
+            OldReport {
+                success: false,
+                had_conflicts: false,
+                conflicts_resolved: false,
+                gates_passed: false,
+                no_commits: false,
+                gate_output: "cherry-pick failed",
+                conflict_details: None,
+            },
+        );
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+        // GateFailure because !success && !gates_passed matches that branch
+        let kind = read_result_kind("9604_other");
+        assert!(
+            kind.as_deref() == Some("GateFailure") || kind.as_deref() == Some("Other"),
+            "unexpected kind: {kind:?}"
+        );
+    }
+
+    #[test]
+    fn skips_non_completed_jobs() {
+        init_for_test();
+        write_merge_job("9605_running", "running", 1.0, None, Some("{\"ts\":1.0}"));
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+        // Running job must not be touched — its error field is still the server-time encoding.
+        let view = crate::crdt_state::read_merge_job("9605_running").unwrap();
+        assert_eq!(view.status, "running");
+    }
+
+    #[test]
+    fn is_idempotent() {
+        init_for_test();
+        seed_old_format(
+            "9606_idem",
+            OldReport {
+                success: true,
+                had_conflicts: false,
+                conflicts_resolved: false,
+                gates_passed: true,
+                no_commits: false,
+                gate_output: "ok",
+                conflict_details: None,
+            },
+        );
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+        let kind_first = read_result_kind("9606_idem");
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+        let kind_second = read_result_kind("9606_idem");
+        assert_eq!(kind_first, kind_second, "second migration must be a no-op");
+    }
+
+    #[test]
+    fn is_noop_when_crdt_not_initialised() {
+        migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db"));
+    }
+}
@@ -17,6 +17,6 @@ pub use item::{
 #[cfg(test)]
 pub use item::write_item_str;
 pub use migrations::{
-    migrate_legacy_stage_strings, migrate_names_from_slugs, migrate_story_ids_to_numeric,
-    name_from_story_id,
+    migrate_legacy_stage_strings, migrate_merge_job, migrate_names_from_slugs,
+    migrate_story_ids_to_numeric, name_from_story_id,
 };