Add PR ressurection safety check

Author: tstromberg

cmd/review-goose/github.go

@@ -589,6 +589,7 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u
 
 			// Update the PR in the slices directly
 			authorBot := result.turnData.PullRequest.AuthorBot
+			lastActivityAt := result.turnData.Analysis.LastActivity.Timestamp
 			if result.isOwner {
 				for i := range *outgoing {
 					if (*outgoing)[i].URL != result.url {
@@ -601,6 +602,7 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u
 					(*outgoing)[i].TestState = testState
 					(*outgoing)[i].WorkflowState = workflowState
 					(*outgoing)[i].AuthorBot = authorBot
+					(*outgoing)[i].LastActivityAt = lastActivityAt
 					break
 				}
 			} else {
@@ -615,6 +617,7 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u
 					(*incoming)[i].TestState = testState
 					(*incoming)[i].WorkflowState = workflowState
 					(*incoming)[i].AuthorBot = authorBot
+					(*incoming)[i].LastActivityAt = lastActivityAt
 					break
 				}
 			}

cmd/review-goose/main.go

@@ -71,6 +71,7 @@ const (
 	defaultMaxBrowserOpensDay = 20
 	startupGracePeriod        = 1 * time.Minute // Don't play sounds or auto-open for first minute
 	authRetryInterval         = 2 * time.Minute // Retry authentication periodically when in error state
+	ancientPRThreshold        = 24 * time.Hour  // Refuse to notify for PRs with no activity in this long (safety check)
 )
 
 // PR represents a pull request with metadata.
@@ -79,6 +80,7 @@ type PR struct {
 	CreatedAt         time.Time
 	TurnDataAppliedAt time.Time
 	FirstBlockedAt    time.Time // When this PR was first detected as blocked
+	LastActivityAt    time.Time // Most recent activity timestamp from Turn API (includes test completions)
 	Title             string
 	URL               string
 	Repository        string

cmd/review-goose/pr_state.go

@@ -3,6 +3,7 @@ package main
 import (
 	"log/slog"
 	"maps"
+	"slices"
 	"sync"
 	"time"
 )
@@ -18,18 +19,18 @@ type PRState struct {
 
 // PRStateManager manages all PR states with proper synchronization.
 type PRStateManager struct {
-	startTime          time.Time
-	states             map[string]*PRState
-	gracePeriodSeconds int
-	mu                 sync.RWMutex
+	startTime   time.Time
+	states      map[string]*PRState
+	gracePeriod time.Duration
+	mu          sync.RWMutex
 }
 
 // NewPRStateManager creates a new PR state manager.
 func NewPRStateManager(startTime time.Time) *PRStateManager {
 	return &PRStateManager{
-		states:             make(map[string]*PRState),
-		startTime:          startTime,
-		gracePeriodSeconds: 30,
+		states:      make(map[string]*PRState),
+		startTime:   startTime,
+		gracePeriod: 30 * time.Second,
 	}
 }
 
@@ -41,7 +42,7 @@ func (m *PRStateManager) UpdatePRs(incoming, outgoing []PR, hiddenOrgs map[strin
 	defer m.mu.Unlock()
 
 	now := time.Now()
-	inGracePeriod := time.Since(m.startTime) < time.Duration(m.gracePeriodSeconds)*time.Second
+	inGracePeriod := time.Since(m.startTime) < m.gracePeriod
 
 	slog.Debug("[STATE] UpdatePRs called",
 		"incoming", len(incoming), "outgoing", len(outgoing),
@@ -51,9 +52,7 @@ func (m *PRStateManager) UpdatePRs(incoming, outgoing []PR, hiddenOrgs map[strin
 	currentlyBlocked := make(map[string]bool)
 
 	// Process all PRs (both incoming and outgoing)
-	allPRs := make([]PR, 0, len(incoming)+len(outgoing))
-	allPRs = append(allPRs, incoming...)
-	allPRs = append(allPRs, outgoing...)
+	allPRs := slices.Concat(incoming, outgoing)
 
 	for i := range allPRs {
 		pr := allPRs[i]
@@ -64,14 +63,14 @@ func (m *PRStateManager) UpdatePRs(incoming, outgoing []PR, hiddenOrgs map[strin
 		}
 
 		// Check if PR is blocked
-		isBlocked := pr.NeedsReview || pr.IsBlocked
-		if !isBlocked {
+		blocked := pr.NeedsReview || pr.IsBlocked
+		if !blocked {
 			// PR is not blocked - remove from tracking if it was
-			if state, exists := m.states[pr.URL]; exists && state != nil {
+			if st, ok := m.states[pr.URL]; ok {
 				slog.Info("[STATE] State transition: blocked -> unblocked",
 					"repo", pr.Repository, "number", pr.Number, "url", pr.URL,
-					"was_blocked_since", state.FirstBlockedAt.Format(time.RFC3339),
-					"blocked_duration", time.Since(state.FirstBlockedAt).Round(time.Second))
+					"was_blocked_since", st.FirstBlockedAt.Format(time.RFC3339),
+					"blocked_duration", time.Since(st.FirstBlockedAt).Round(time.Second))
 				delete(m.states, pr.URL)
 			}
 			continue
@@ -121,55 +120,56 @@ func (m *PRStateManager) UpdatePRs(incoming, outgoing []PR, hiddenOrgs map[strin
 
 				// Should we notify for actual state transitions?
 				if !inGracePeriod && !state.HasNotified {
-					slog.Debug("[STATE] Will notify for newly blocked PR", "repo", pr.Repository, "number", pr.Number)
-					toNotify = append(toNotify, pr)
-					state.HasNotified = true
+					if isPRFreshEnoughForNotification(&pr, time.Since(m.startTime), nil) {
+						slog.Debug("[STATE] Will notify for newly blocked PR", "repo", pr.Repository, "number", pr.Number)
+						toNotify = append(toNotify, pr)
+						state.HasNotified = true
+					}
 				} else if inGracePeriod {
 					slog.Debug("[STATE] In grace period, not notifying", "repo", pr.Repository, "number", pr.Number)
 				}
 			}
 		} else {
-			// PR was already blocked in our state - just update data, preserve FirstBlockedAt
-			originalFirstBlocked := state.FirstBlockedAt
+			// PR was already blocked in our state - update data, preserve FirstBlockedAt
 			state.LastSeenBlocked = now
-			state.PR = pr // Update PR data
+			state.PR = pr
 
 			slog.Debug("[STATE] State transition: blocked -> blocked (no change)",
 				"repo", pr.Repository, "number", pr.Number, "url", pr.URL,
-				"original_first_blocked", originalFirstBlocked.Format(time.RFC3339),
-				"time_since_first_blocked", time.Since(originalFirstBlocked).Round(time.Second),
+				"original_first_blocked", state.FirstBlockedAt.Format(time.RFC3339),
+				"time_since_first_blocked", time.Since(state.FirstBlockedAt).Round(time.Second),
 				"has_notified", state.HasNotified)
 
 			// If we haven't notified yet and we're past grace period, notify now
 			// But don't notify for initial discovery PRs
 			if !state.HasNotified && !inGracePeriod && !state.IsInitialDiscovery {
-				slog.Info("[STATE] Past grace period, notifying for previously blocked PR",
-					"repo", pr.Repository, "number", pr.Number)
-				toNotify = append(toNotify, pr)
-				state.HasNotified = true
+				if isPRFreshEnoughForNotification(&pr, time.Since(m.startTime), state) {
+					slog.Info("[STATE] Past grace period, notifying for previously blocked PR",
+						"repo", pr.Repository, "number", pr.Number)
+					toNotify = append(toNotify, pr)
+					state.HasNotified = true
+				}
 			}
 		}
 	}
 
 	// Clean up states for PRs that are no longer in our lists
-	// Add more conservative cleanup with logging
-	removedCount := 0
-	for url, state := range m.states {
+	removed := 0
+	for url, st := range m.states {
 		if !currentlyBlocked[url] {
-			timeSinceLastSeen := time.Since(state.LastSeenBlocked)
 			slog.Info("[STATE] Removing stale PR state (no longer blocked)",
-				"url", url, "repo", state.PR.Repository, "number", state.PR.Number,
-				"first_blocked_at", state.FirstBlockedAt.Format(time.RFC3339),
-				"last_seen_blocked", state.LastSeenBlocked.Format(time.RFC3339),
-				"time_since_last_seen", timeSinceLastSeen.Round(time.Second),
-				"was_notified", state.HasNotified)
+				"url", url, "repo", st.PR.Repository, "number", st.PR.Number,
+				"first_blocked_at", st.FirstBlockedAt.Format(time.RFC3339),
+				"last_seen_blocked", st.LastSeenBlocked.Format(time.RFC3339),
+				"time_since_last_seen", time.Since(st.LastSeenBlocked).Round(time.Second),
+				"was_notified", st.HasNotified)
 			delete(m.states, url)
-			removedCount++
+			removed++
 		}
 	}
 
-	if removedCount > 0 {
-		slog.Info("[STATE] State cleanup completed", "removed_states", removedCount, "remaining_states", len(m.states))
+	if removed > 0 {
+		slog.Info("[STATE] State cleanup completed", "removed_states", removed, "remaining_states", len(m.states))
 	}
 
 	return toNotify
@@ -204,3 +204,67 @@ func (m *PRStateManager) ResetNotifications() {
 	}
 	slog.Info("[STATE] Reset notification flags", "prCount", len(m.states))
 }
+
+// isPRFreshEnoughForNotification checks if a PR has recent enough activity to warrant a notification.
+// This is a safety check to catch logic bugs that might resurrect ancient PRs.
+func isPRFreshEnoughForNotification(pr *PR, uptime time.Duration, prev *PRState) bool {
+	// Prefer LastActivityAt (from Turn API, includes test completions), fall back to UpdatedAt
+	recent := pr.LastActivityAt
+	src := "last_activity_at"
+	if recent.IsZero() {
+		recent = pr.UpdatedAt
+		src = "updated_at"
+	}
+
+	age := time.Since(recent)
+
+	slog.Info("[STATE] PR activity check for notification",
+		"repo", pr.Repository,
+		"number", pr.Number,
+		"most_recent_activity", recent.Format(time.RFC3339),
+		"activity_source", src,
+		"time_since_activity", age.Round(time.Second),
+		"updated_at", pr.UpdatedAt.Format(time.RFC3339),
+		"last_activity_at", pr.LastActivityAt.Format(time.RFC3339))
+
+	if age <= ancientPRThreshold {
+		return true
+	}
+
+	// PR is stale - log detailed debug info for resurrection investigation
+	if prev == nil {
+		slog.Error("[STATE] REFUSING TO NOTIFY: PR has no recent activity - possible logic bug resurrecting ancient PR",
+			"repo", pr.Repository,
+			"number", pr.Number,
+			"url", pr.URL,
+			"most_recent_activity", recent.Format(time.RFC3339),
+			"activity_source", src,
+			"time_since_activity", age.Round(time.Hour),
+			"threshold", ancientPRThreshold,
+			"updated_at", pr.UpdatedAt.Format(time.RFC3339),
+			"last_activity_at", pr.LastActivityAt.Format(time.RFC3339),
+			"app_uptime", uptime.Round(time.Second),
+			"transition_type", "new_blocked",
+			"previously_tracked", false)
+	} else {
+		slog.Error("[STATE] REFUSING TO NOTIFY: PR has no recent activity - possible logic bug resurrecting ancient PR",
+			"repo", pr.Repository,
+			"number", pr.Number,
+			"url", pr.URL,
+			"most_recent_activity", recent.Format(time.RFC3339),
+			"activity_source", src,
+			"time_since_activity", age.Round(time.Hour),
+			"threshold", ancientPRThreshold,
+			"updated_at", pr.UpdatedAt.Format(time.RFC3339),
+			"last_activity_at", pr.LastActivityAt.Format(time.RFC3339),
+			"app_uptime", uptime.Round(time.Second),
+			"transition_type", "existing_blocked",
+			"previously_tracked", true,
+			"prev_first_blocked_at", prev.FirstBlockedAt.Format(time.RFC3339),
+			"prev_last_seen_blocked", prev.LastSeenBlocked.Format(time.RFC3339),
+			"prev_was_notified", prev.HasNotified,
+			"time_since_first_blocked", time.Since(prev.FirstBlockedAt).Round(time.Second),
+			"time_since_last_seen", time.Since(prev.LastSeenBlocked).Round(time.Second))
+	}
+	return false
+}