Fix validatePostImport false positive for legitimate deletions (bd-pg1)
The sync validation was incorrectly triggering 'data loss detected' when issue count decreased after import, even for legitimate deletions recorded in deletions.jsonl. Changes: - Modified validatePostImport to accept jsonlPath and check deletions manifest - When issue count decreases, check if decrease is within recorded deletions - Updated all call sites in sync.go and daemon_sync.go - Added comprehensive tests for deletion-aware validation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -550,7 +550,7 @@ func createAutoImportFunc(ctx context.Context, store storage.Storage, log daemon
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := validatePostImport(beforeCount, afterCount); err != nil {
|
if err := validatePostImport(beforeCount, afterCount, jsonlPath); err != nil {
|
||||||
log.log("Post-import validation failed: %v", err)
|
log.log("Post-import validation failed: %v", err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -743,7 +743,7 @@ func createSyncFunc(ctx context.Context, store storage.Storage, autoCommit, auto
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := validatePostImport(beforeCount, afterCount); err != nil {
|
if err := validatePostImport(beforeCount, afterCount, jsonlPath); err != nil {
|
||||||
log.log("Post-import validation failed: %v", err)
|
log.log("Post-import validation failed: %v", err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ import (
|
|||||||
"sort"
|
"sort"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
|
"github.com/steveyegge/beads/internal/deletions"
|
||||||
"github.com/steveyegge/beads/internal/storage"
|
"github.com/steveyegge/beads/internal/storage"
|
||||||
"github.com/steveyegge/beads/internal/types"
|
"github.com/steveyegge/beads/internal/types"
|
||||||
)
|
)
|
||||||
@@ -287,10 +288,45 @@ func checkOrphanedDeps(ctx context.Context, store storage.Storage) ([]string, er
|
|||||||
}
|
}
|
||||||
|
|
||||||
// validatePostImport checks that import didn't cause data loss.
|
// validatePostImport checks that import didn't cause data loss.
|
||||||
// Returns error if issue count decreased (data loss) or nil if OK.
|
// Returns error if issue count decreased unexpectedly (data loss) or nil if OK.
|
||||||
func validatePostImport(before, after int) error {
|
// A decrease is legitimate if it matches deletions recorded in deletions.jsonl.
|
||||||
|
//
|
||||||
|
// Parameters:
|
||||||
|
// - before: issue count in DB before import
|
||||||
|
// - after: issue count in DB after import
|
||||||
|
// - jsonlPath: path to issues.jsonl (used to locate deletions.jsonl)
|
||||||
|
func validatePostImport(before, after int, jsonlPath string) error {
|
||||||
if after < before {
|
if after < before {
|
||||||
return fmt.Errorf("import reduced issue count: %d → %d (data loss detected!)", before, after)
|
// Count decrease - check if this matches legitimate deletions
|
||||||
|
decrease := before - after
|
||||||
|
|
||||||
|
// Load deletions manifest to check for legitimate deletions
|
||||||
|
beadsDir := filepath.Dir(jsonlPath)
|
||||||
|
deletionsPath := deletions.DefaultPath(beadsDir)
|
||||||
|
loadResult, err := deletions.LoadDeletions(deletionsPath)
|
||||||
|
if err != nil {
|
||||||
|
// If we can't load deletions, assume the worst
|
||||||
|
return fmt.Errorf("import reduced issue count: %d → %d (data loss detected! failed to verify deletions: %v)", before, after, err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// If there are deletions recorded, the decrease is likely legitimate
|
||||||
|
// We can't perfectly match because we don't know exactly which issues
|
||||||
|
// were deleted in this sync cycle vs previously. But if there are ANY
|
||||||
|
// deletions recorded and the decrease is reasonable, allow it.
|
||||||
|
numDeletions := len(loadResult.Records)
|
||||||
|
if numDeletions > 0 && decrease <= numDeletions {
|
||||||
|
// Legitimate deletion - decrease is accounted for by deletions manifest
|
||||||
|
fmt.Fprintf(os.Stderr, "Import complete: %d → %d issues (-%d, accounted for by %d deletion(s))\n",
|
||||||
|
before, after, decrease, numDeletions)
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// Decrease exceeds recorded deletions - potential data loss
|
||||||
|
if numDeletions > 0 {
|
||||||
|
return fmt.Errorf("import reduced issue count: %d → %d (-%d exceeds %d recorded deletion(s) - potential data loss!)",
|
||||||
|
before, after, decrease, numDeletions)
|
||||||
|
}
|
||||||
|
return fmt.Errorf("import reduced issue count: %d → %d (data loss detected! no deletions recorded)", before, after)
|
||||||
}
|
}
|
||||||
if after == before {
|
if after == before {
|
||||||
fmt.Fprintf(os.Stderr, "Import complete: no changes\n")
|
fmt.Fprintf(os.Stderr, "Import complete: no changes\n")
|
||||||
|
|||||||
@@ -163,22 +163,78 @@ func TestValidatePreExportSuite(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestValidatePostImport(t *testing.T) {
|
func TestValidatePostImport(t *testing.T) {
|
||||||
t.Run("issue count decreased fails", func(t *testing.T) {
|
t.Run("issue count decreased with no deletions fails", func(t *testing.T) {
|
||||||
err := validatePostImport(10, 5)
|
tmpDir := t.TempDir()
|
||||||
|
jsonlPath := filepath.Join(tmpDir, "issues.jsonl")
|
||||||
|
// No deletions.jsonl file exists
|
||||||
|
err := validatePostImport(10, 5, jsonlPath)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Error("Expected error for decreased issue count, got nil")
|
t.Error("Expected error for decreased issue count with no deletions, got nil")
|
||||||
|
}
|
||||||
|
if err != nil && !strings.Contains(err.Error(), "no deletions recorded") {
|
||||||
|
t.Errorf("Expected 'no deletions recorded' error, got: %v", err)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("issue count decreased within deletion count succeeds", func(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
jsonlPath := filepath.Join(tmpDir, "issues.jsonl")
|
||||||
|
deletionsPath := filepath.Join(tmpDir, "deletions.jsonl")
|
||||||
|
|
||||||
|
// Create deletions file with 5 deletions
|
||||||
|
deletionsContent := `{"id":"del-1","ts":"2024-01-01T00:00:00Z","by":"test"}
|
||||||
|
{"id":"del-2","ts":"2024-01-01T00:00:00Z","by":"test"}
|
||||||
|
{"id":"del-3","ts":"2024-01-01T00:00:00Z","by":"test"}
|
||||||
|
{"id":"del-4","ts":"2024-01-01T00:00:00Z","by":"test"}
|
||||||
|
{"id":"del-5","ts":"2024-01-01T00:00:00Z","by":"test"}
|
||||||
|
`
|
||||||
|
if err := os.WriteFile(deletionsPath, []byte(deletionsContent), 0600); err != nil {
|
||||||
|
t.Fatalf("Failed to write deletions file: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Decrease of 5 matches the 5 recorded deletions
|
||||||
|
err := validatePostImport(10, 5, jsonlPath)
|
||||||
|
if err != nil {
|
||||||
|
t.Errorf("Expected no error when decrease matches deletions, got: %v", err)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("issue count decreased exceeding deletion count fails", func(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
jsonlPath := filepath.Join(tmpDir, "issues.jsonl")
|
||||||
|
deletionsPath := filepath.Join(tmpDir, "deletions.jsonl")
|
||||||
|
|
||||||
|
// Create deletions file with only 2 deletions
|
||||||
|
deletionsContent := `{"id":"del-1","ts":"2024-01-01T00:00:00Z","by":"test"}
|
||||||
|
{"id":"del-2","ts":"2024-01-01T00:00:00Z","by":"test"}
|
||||||
|
`
|
||||||
|
if err := os.WriteFile(deletionsPath, []byte(deletionsContent), 0600); err != nil {
|
||||||
|
t.Fatalf("Failed to write deletions file: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Decrease of 5 exceeds the 2 recorded deletions
|
||||||
|
err := validatePostImport(10, 5, jsonlPath)
|
||||||
|
if err == nil {
|
||||||
|
t.Error("Expected error when decrease exceeds deletions, got nil")
|
||||||
|
}
|
||||||
|
if err != nil && !strings.Contains(err.Error(), "exceeds") {
|
||||||
|
t.Errorf("Expected 'exceeds' error, got: %v", err)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
t.Run("issue count same succeeds", func(t *testing.T) {
|
t.Run("issue count same succeeds", func(t *testing.T) {
|
||||||
err := validatePostImport(10, 10)
|
tmpDir := t.TempDir()
|
||||||
|
jsonlPath := filepath.Join(tmpDir, "issues.jsonl")
|
||||||
|
err := validatePostImport(10, 10, jsonlPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Errorf("Expected no error for same count, got: %v", err)
|
t.Errorf("Expected no error for same count, got: %v", err)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
t.Run("issue count increased succeeds", func(t *testing.T) {
|
t.Run("issue count increased succeeds", func(t *testing.T) {
|
||||||
err := validatePostImport(10, 15)
|
tmpDir := t.TempDir()
|
||||||
|
jsonlPath := filepath.Join(tmpDir, "issues.jsonl")
|
||||||
|
err := validatePostImport(10, 15, jsonlPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Errorf("Expected no error for increased count, got: %v", err)
|
t.Errorf("Expected no error for increased count, got: %v", err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -332,7 +332,7 @@ Use --merge to merge the sync branch back to main branch.`,
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
fmt.Fprintf(os.Stderr, "Warning: failed to count issues after import: %v\n", err)
|
fmt.Fprintf(os.Stderr, "Warning: failed to count issues after import: %v\n", err)
|
||||||
} else {
|
} else {
|
||||||
if err := validatePostImport(beforeCount, afterCount); err != nil {
|
if err := validatePostImport(beforeCount, afterCount, jsonlPath); err != nil {
|
||||||
fmt.Fprintf(os.Stderr, "Post-import validation failed: %v\n", err)
|
fmt.Fprintf(os.Stderr, "Post-import validation failed: %v\n", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user