diff --git a/internal/store/store.go b/internal/store/store.go index 0978de38..ae9db016 100644 --- a/internal/store/store.go +++ b/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) diff --git a/internal/store/store_test.go b/internal/store/store_test.go index dac41370..3b7737cf 100644 --- a/internal/store/store_test.go +++ b/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) {