Fix critical import bug: preserve closed_at timestamps during sync

**Problem:**
Closed issues were silently reopening during git sync/import operations.
When importing an issue update, the importer built an updates map with
status='closed' but NO closed_at timestamp. The UpdateIssue() function's
manageClosedAt() would only set closed_at when status was CHANGING to
closed, not when it was already closed. Result: closed_at got cleared,
effectively reopening issues.

**Root Cause:**
1. Importer built updates map without closed_at field (lines 443-451, 519-528)
2. closed_at was not in allowedUpdateFields whitelist
3. manageClosedAt() only managed closed_at for status TRANSITIONS
4. Import of already-closed issue → closed_at lost → issue reopens

**Impact:**
- WASM issues (bd-44d0, bd-8507, etc.) were closed on Nov 4 (commit 0df9144)
- They reopened as 'open' status during sync on Nov 5 (commit 8c9814a)
- Users had to repeatedly close the same issues
- Data integrity violation: status=closed with closed_at=NULL

**Fix:**
1. Add closed_at to allowedUpdateFields whitelist
2. Add closed_at to importer updates maps (both external_ref and ID paths)
3. Update manageClosedAt() to skip auto-management if closed_at explicitly provided
   - Preserves import timestamps while maintaining auto-management for CLI operations

**Testing:**
- All internal/importer tests pass
- All internal/storage/sqlite tests pass
- Explicitly tests timestamp preservation in TestImportWithExternalRef

**Files Changed:**
- internal/importer/importer.go: Add closed_at to updates maps
- internal/storage/sqlite/sqlite.go: Allow closed_at updates, respect explicit values

Amp-Thread-ID: https://ampcode.com/threads/T-53ed6e45-9d04-4a35-97e9-d1ec36321ab0
Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
Steve Yegge
2025-11-05 00:41:10 -08:00
parent 6f5687f934
commit 8b9a486056
3 changed files with 35 additions and 26 deletions

View File

@@ -391,6 +391,7 @@ var allowedUpdateFields = map[string]bool{
"issue_type": true,
"estimated_minutes": true,
"external_ref": true,
"closed_at": true,
}
// validatePriority validates a priority value
@@ -420,6 +421,14 @@ func determineEventType(oldIssue *types.Issue, updates map[string]interface{}) t
// manageClosedAt automatically manages the closed_at field based on status changes
func manageClosedAt(oldIssue *types.Issue, updates map[string]interface{}, setClauses []string, args []interface{}) ([]string, []interface{}) {
statusVal, hasStatus := updates["status"]
// If closed_at is explicitly provided in updates, it's already in setClauses/args
// and we should not override it (important for import operations that preserve timestamps)
_, hasExplicitClosedAt := updates["closed_at"]
if hasExplicitClosedAt {
return setClauses, args
}
if !hasStatus {
return setClauses, args
}
@@ -437,12 +446,10 @@ func manageClosedAt(oldIssue *types.Issue, updates map[string]interface{}, setCl
if newStatus == string(types.StatusClosed) {
// Changing to closed: ensure closed_at is set
if _, hasClosedAt := updates["closed_at"]; !hasClosedAt {
now := time.Now()
updates["closed_at"] = now
setClauses = append(setClauses, "closed_at = ?")
args = append(args, now)
}
now := time.Now()
updates["closed_at"] = now
setClauses = append(setClauses, "closed_at = ?")
args = append(args, now)
} else if oldIssue.Status == types.StatusClosed {
// Changing from closed to something else: clear closed_at
updates["closed_at"] = nil