fix(store): apply repairable cloud-upgrade mutations even when a blocker is queued (#446) (#452)

Previously RepairCloudUpgrade returned Applied:false and skipped all repairs
when BlockedCount > 0, even if RepairableCount was also > 0. The message also
always picked Findings[0] (lowest seq, often a repairable entry) instead of
the first non-repairable finding, and omitted entity_key.

Now the repairable subset is applied first inside the existing transaction
path; the blocked report then carries Applied:true, accurate counts, and a
message that scans Findings for the first non-repairable entry (with
entity_key) so operators see the actual blocker.

Author: Alan-TheGentleman

internal/store/store.go

@@ -1231,37 +1231,70 @@ func (s *Store) RepairCloudUpgrade(project string, apply bool) (CloudUpgradeRepa
 	if err != nil {
 		return CloudUpgradeRepairReport{}, fmt.Errorf("diagnose legacy cloud upgrade mutations: %w", err)
 	}
+	// When both repairable and blocked mutations coexist we must not hold the
+	// entire repair pass hostage to the non-repairable entries. Apply the
+	// repairable subset first, then surface the residual blockers.
+	appliedRepairs := false
+	if apply && legacyReport.RepairableCount > 0 {
+		if err := s.applyCloudUpgradeLegacyMutationRepairs(project); err != nil {
+			return CloudUpgradeRepairReport{}, fmt.Errorf("apply cloud upgrade legacy mutation repairs: %w", err)
+		}
+		appliedRepairs = true
+	}
+
 	if legacyReport.BlockedCount > 0 {
-		first := legacyReport.Findings[0]
+		// Scan for the first non-repairable finding so the operator sees the
+		// actual blocker. Findings[0] is ordered by seq and may be a repairable
+		// entry, which previously produced a misleading error message (#446).
+		var blocked CloudUpgradeLegacyMutationFinding
+		for _, f := range legacyReport.Findings {
+			if !f.Repairable {
+				blocked = f
+				break
+			}
+		}
+		var msg string
+		switch {
+		case appliedRepairs:
+			msg = fmt.Sprintf("applied %d repairable payload(s); %d remain blocked: manual-action-required: %s (seq=%d entity=%s entity_key=%q op=%s)",
+				legacyReport.RepairableCount, legacyReport.BlockedCount, blocked.Message, blocked.Seq, blocked.Entity, blocked.EntityKey, blocked.Op)
+		case legacyReport.RepairableCount > 0:
+			msg = fmt.Sprintf("%d repairable payload(s) would apply; %d would remain blocked: manual-action-required: %s (seq=%d entity=%s entity_key=%q op=%s)",
+				legacyReport.RepairableCount, legacyReport.BlockedCount, blocked.Message, blocked.Seq, blocked.Entity, blocked.EntityKey, blocked.Op)
+		default:
+			msg = fmt.Sprintf("manual-action-required: %s (seq=%d entity=%s entity_key=%q op=%s)",
+				blocked.Message, blocked.Seq, blocked.Entity, blocked.EntityKey, blocked.Op)
+		}
 		return CloudUpgradeRepairReport{
 			Class:      UpgradeRepairClassBlocked,
 			ReasonCode: UpgradeReasonBlockedLegacyMutationManual,
-			Message:    fmt.Sprintf("manual-action-required: %s (seq=%d entity=%s op=%s)", first.Message, first.Seq, first.Entity, first.Op),
-			Applied:    false,
+			Message:    msg,
+			Applied:    appliedRepairs,
 		}, nil
 	}
+
 	if legacyReport.RepairableCount > 0 {
-		report := CloudUpgradeRepairReport{
-			Class:         UpgradeRepairClassRepairable,
-			ReasonCode:    UpgradeReasonRepairableLegacyMutationPayload,
-			Message:       fmt.Sprintf("project %q has %d repairable legacy mutation payload issue(s)", project, legacyReport.RepairableCount),
-			PlannedAction: "repair_legacy_mutation_payloads",
-			Applied:       false,
-		}
-		if !apply {
-			return report, nil
-		}
-		if err := s.applyCloudUpgradeLegacyMutationRepairs(project); err != nil {
-			return CloudUpgradeRepairReport{}, fmt.Errorf("apply cloud upgrade legacy mutation repairs: %w", err)
+		if !appliedRepairs {
+			return CloudUpgradeRepairReport{
+				Class:         UpgradeRepairClassRepairable,
+				ReasonCode:    UpgradeReasonRepairableLegacyMutationPayload,
+				Message:       fmt.Sprintf("project %q has %d repairable legacy mutation payload issue(s)", project, legacyReport.RepairableCount),
+				PlannedAction: "repair_legacy_mutation_payloads",
+				Applied:       false,
+			}, nil
 		}
-		report.Applied = true
-		report.Message = fmt.Sprintf("applied deterministic legacy mutation payload repairs for project %q", project)
 		_ = s.SaveCloudUpgradeState(CloudUpgradeState{
 			Project:     project,
 			Stage:       UpgradeStageRepairApplied,
 			RepairClass: UpgradeRepairClassRepairable,
 		})
-		return report, nil
+		return CloudUpgradeRepairReport{
+			Class:         UpgradeRepairClassRepairable,
+			ReasonCode:    UpgradeReasonRepairableLegacyMutationPayload,
+			Message:       fmt.Sprintf("applied deterministic legacy mutation payload repairs for project %q", project),
+			PlannedAction: "repair_legacy_mutation_payloads",
+			Applied:       true,
+		}, nil
 	}
 
 	requiresBackfill, err := s.projectSyncBackfillRequired(project)

internal/store/store_test.go

@@ -2118,6 +2118,148 @@ func TestUpgradeRepairDryRunAndApply(t *testing.T) {
 			t.Fatalf("expected missing provenance to remain blocked, got %+v", diagnosis)
 		}
 	})
+
+	// Regression test for GitHub issue #446: when both repairable and blocked
+	// mutations coexist, RepairCloudUpgrade(apply=true) must apply the
+	// repairable subset and return Applied:true with Class=Blocked (and the
+	// message must reference the actual blocker, not the low-seq repairable
+	// entry that happens to be first in Findings order).
+	t.Run("partial apply: repairable mutations applied even when a blocker is queued", func(t *testing.T) {
+		s := newTestStore(t)
+		if err := s.CreateSession("partial-repair-s1", "partial-repair-proj", "/tmp/partial-repair"); err != nil {
+			t.Fatalf("create session: %v", err)
+		}
+
+		// Create an observation so we have authoritative local state for the
+		// repairable mutation.
+		obsID, err := s.AddObservation(AddObservationParams{
+			SessionID: "partial-repair-s1",
+			Type:      "decision",
+			Title:     "Authoritative title",
+			Content:   "Authoritative content",
+			Project:   "partial-repair-proj",
+			Scope:     "project",
+		})
+		if err != nil {
+			t.Fatalf("add observation: %v", err)
+		}
+		if err := s.EnrollProject("partial-repair-proj"); err != nil {
+			t.Fatalf("enroll project: %v", err)
+		}
+
+		// Look up the observation's sync_id for payload construction.
+		var obsSyncID string
+		if err := s.db.QueryRow(`SELECT sync_id FROM observations WHERE id = ?`, obsID).Scan(&obsSyncID); err != nil {
+			t.Fatalf("lookup observation sync_id: %v", err)
+		}
+
+		// Insert a REPAIRABLE observation mutation (missing title — low seq,
+		// will naturally come before the blocker we insert next).
+		repairablePayload := `{"sync_id":"` + obsSyncID + `","session_id":"partial-repair-s1","type":"decision","content":"legacy payload missing title","scope":"project"}`
+		if _, err := s.execHook(s.db,
+			`INSERT INTO sync_mutations (target_key, entity, entity_key, op, payload, source, project) VALUES (?, ?, ?, ?, ?, ?, ?)`,
+			DefaultSyncTargetKey,
+			SyncEntityObservation,
+			obsSyncID,
+			SyncOpUpsert,
+			repairablePayload,
+			SyncSourceLocal,
+			"partial-repair-proj",
+		); err != nil {
+			t.Fatalf("insert repairable observation mutation: %v", err)
+		}
+
+		// Create two observations for source/target of the relation we use as
+		// the blocker mutation.
+		srcID, err := s.AddObservation(AddObservationParams{SessionID: "partial-repair-s1", Type: "decision", Title: "Src", Content: "src", Project: "partial-repair-proj", Scope: "project"})
+		if err != nil {
+			t.Fatalf("add source observation: %v", err)
+		}
+		dstID, err := s.AddObservation(AddObservationParams{SessionID: "partial-repair-s1", Type: "decision", Title: "Dst", Content: "dst", Project: "partial-repair-proj", Scope: "project"})
+		if err != nil {
+			t.Fatalf("add dest observation: %v", err)
+		}
+		var srcSyncID, dstSyncID string
+		if err := s.db.QueryRow(`SELECT sync_id FROM observations WHERE id = ?`, srcID).Scan(&srcSyncID); err != nil {
+			t.Fatalf("lookup src sync_id: %v", err)
+		}
+		if err := s.db.QueryRow(`SELECT sync_id FROM observations WHERE id = ?`, dstID).Scan(&dstSyncID); err != nil {
+			t.Fatalf("lookup dst sync_id: %v", err)
+		}
+
+		// Insert a BLOCKED relation mutation (missing provenance — high seq,
+		// comes after the repairable entry so Findings[0] would point to the
+		// repairable entry without the fix).
+		const blockerEntityKey = "rel-partial-blocked-446"
+		blockerPayload := `{"sync_id":"` + blockerEntityKey + `","source_id":"` + srcSyncID + `","target_id":"` + dstSyncID + `","relation":"compatible","project":"partial-repair-proj"}`
+		if _, err := s.execHook(s.db,
+			`INSERT INTO sync_mutations (target_key, entity, entity_key, op, payload, source, project) VALUES (?, ?, ?, ?, ?, ?, ?)`,
+			DefaultSyncTargetKey,
+			SyncEntityRelation,
+			blockerEntityKey,
+			SyncOpUpsert,
+			blockerPayload,
+			SyncSourceLocal,
+			"partial-repair-proj",
+		); err != nil {
+			t.Fatalf("insert blocked relation mutation: %v", err)
+		}
+
+		// Confirm pre-conditions: diagnosis must see exactly one repairable
+		// and one blocked entry.
+		diag, err := s.DiagnoseCloudUpgradeLegacyMutations("partial-repair-proj")
+		if err != nil {
+			t.Fatalf("pre-condition diagnose: %v", err)
+		}
+		if diag.RepairableCount != 1 || diag.BlockedCount != 1 {
+			t.Fatalf("expected 1 repairable + 1 blocked pre-condition, got %+v", diag)
+		}
+
+		// Find the seq of the blocked finding so we can assert the message
+		// references the right entry.
+		var blockerSeq int64
+		for _, f := range diag.Findings {
+			if !f.Repairable {
+				blockerSeq = f.Seq
+				break
+			}
+		}
+		if blockerSeq == 0 {
+			t.Fatal("pre-condition: could not locate blocked finding seq")
+		}
+		// Also verify that Findings[0] is NOT the blocker (i.e. lowest-seq is
+		// the repairable one); this is the exact condition that triggered the
+		// wrong-message bug.
+		if diag.Findings[0].Seq == blockerSeq {
+			t.Fatalf("pre-condition: expected Findings[0] to be repairable (lowest seq), but got blocker seq=%d", blockerSeq)
+		}
+
+		// === THE ACTUAL ASSERTION ===
+		// With apply=true, repairable mutations must be applied and the
+		// report must still surface the blocker clearly.
+		report, err := s.RepairCloudUpgrade("partial-repair-proj", true)
+		if err != nil {
+			t.Fatalf("repair with mixed repairable+blocker: %v", err)
+		}
+
+		// Applied MUST be true: at least the repairable mutation was applied.
+		if !report.Applied {
+			t.Fatalf("expected Applied=true (repairable subset was processed), got %+v", report)
+		}
+		// Class MUST be Blocked because the non-repairable mutation is still present.
+		if report.Class != UpgradeRepairClassBlocked {
+			t.Fatalf("expected Class=%q, got %q (full report: %+v)", UpgradeRepairClassBlocked, report.Class, report)
+		}
+		// The message must name the BLOCKER seq, not the repairable entry.
+		blockerSeqStr := fmt.Sprintf("seq=%d", blockerSeq)
+		if !strings.Contains(report.Message, blockerSeqStr) {
+			t.Fatalf("expected message to reference blocker seq (%s), got %q", blockerSeqStr, report.Message)
+		}
+		// The message must also include entity_key for debuggability.
+		if !strings.Contains(report.Message, blockerEntityKey) {
+			t.Fatalf("expected message to include entity_key=%q, got %q", blockerEntityKey, report.Message)
+		}
+	})
 }
 
 func TestRollbackCloudUpgradeSafetyBoundary(t *testing.T) {