fix: address review findings for gt deacon pending command

1. MAJOR: Add PruneStalePending call to runDeaconPending no-arg path
2. MAJOR: Add comprehensive tests with archive side-effect verification
3. MINOR: ClearPendingSpawn now clears ALL matching sessions (not just first)
4. MINOR: ClearPendingSpawn is idempotent (no error if session not found)
5. MINOR: nil mailbox now returns error instead of silent success

Refactored pending.go to extract mailbox-dependent logic into testable
internal functions (checkMailboxForSpawns, clearPendingSpawnFromList,
pruneStalePendingFromList) while keeping public API unchanged.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: julianknutsen

internal/cmd/deacon.go

@@ -1180,6 +1180,15 @@ func runDeaconPending(cmd *cobra.Command, args []string) error {
 	}
 
 	// No args: list pending spawns with captured output
+	// First, prune stale pending spawns (>5 min old with dead sessions)
+	pruned, err := polecat.PruneStalePending(townRoot, 5*time.Minute)
+	if err != nil {
+		// Non-fatal: log and continue
+		fmt.Printf("%s Warning: prune stale pending: %v\n", style.Dim.Render("⚠"), err)
+	} else if pruned > 0 {
+		fmt.Printf("%s Pruned %d stale pending spawn(s)\n", style.Dim.Render("♻"), pruned)
+	}
+
 	pending, err := polecat.CheckInboxForSpawns(townRoot)
 	if err != nil {
 		return fmt.Errorf("checking inbox: %w", err)

internal/polecat/pending.go

@@ -37,25 +37,26 @@ type PendingSpawn struct {
 	mailbox *mail.Mailbox `json:"-"`
 }
 
-// CheckInboxForSpawns discovers pending spawns from POLECAT_STARTED messages
-// in the Deacon's inbox. Uses mail as source of truth (ZFC principle).
-func CheckInboxForSpawns(townRoot string) ([]*PendingSpawn, error) {
-	// Get Deacon's mailbox
+// getDeaconMailbox resolves the Deacon's mailbox from a town root.
+func getDeaconMailbox(townRoot string) (*mail.Mailbox, error) {
 	router := mail.NewRouter(townRoot)
 	mailbox, err := router.GetMailbox("deacon/")
 	if err != nil {
 		return nil, fmt.Errorf("getting deacon mailbox: %w", err)
 	}
+	return mailbox, nil
+}
 
-	// Get all messages (both read and unread - we track by archival status)
+// checkMailboxForSpawns discovers pending spawns from POLECAT_STARTED messages
+// in the given mailbox. Extracted from CheckInboxForSpawns for testability.
+func checkMailboxForSpawns(mailbox *mail.Mailbox) ([]*PendingSpawn, error) {
 	messages, err := mailbox.List()
 	if err != nil {
 		return nil, fmt.Errorf("listing messages: %w", err)
 	}
 
 	var pending []*PendingSpawn
 
-	// Look for POLECAT_STARTED messages
 	for _, msg := range messages {
 		if !strings.HasPrefix(msg.Subject, "POLECAT_STARTED ") {
 			continue
@@ -96,6 +97,16 @@ func CheckInboxForSpawns(townRoot string) ([]*PendingSpawn, error) {
 	return pending, nil
 }
 
+// CheckInboxForSpawns discovers pending spawns from POLECAT_STARTED messages
+// in the Deacon's inbox. Uses mail as source of truth (ZFC principle).
+func CheckInboxForSpawns(townRoot string) ([]*PendingSpawn, error) {
+	mailbox, err := getDeaconMailbox(townRoot)
+	if err != nil {
+		return nil, err
+	}
+	return checkMailboxForSpawns(mailbox)
+}
+
 // TriggerResult holds the result of attempting to trigger a pending spawn.
 type TriggerResult struct {
 	Spawn     *PendingSpawn
@@ -167,47 +178,61 @@ func TriggerPendingSpawns(townRoot string, timeout time.Duration) ([]TriggerResu
 	return results, nil
 }
 
-// ClearPendingSpawn archives the POLECAT_STARTED message for a specific session,
-// removing it from the pending list. Used by the Deacon after observing that a
-// session has been triggered via AI-based observation.
-func ClearPendingSpawn(townRoot, sessionName string) error {
-	pending, err := CheckInboxForSpawns(townRoot)
-	if err != nil {
-		return fmt.Errorf("checking inbox: %w", err)
-	}
-
+// clearPendingSpawnFromList archives all POLECAT_STARTED messages for a specific
+// session from the given pending list. Extracted for testability.
+func clearPendingSpawnFromList(pending []*PendingSpawn, sessionName string) error {
 	for _, ps := range pending {
 		if ps.Session == sessionName {
-			if ps.mailbox != nil {
-				return ps.mailbox.Archive(ps.MailID)
+			if ps.mailbox == nil {
+				return fmt.Errorf("nil mailbox for pending spawn session %s (mail_id: %s)", sessionName, ps.MailID)
+			}
+			if err := ps.mailbox.Archive(ps.MailID); err != nil {
+				return fmt.Errorf("archiving mail %s for session %s: %w", ps.MailID, sessionName, err)
 			}
-			return nil
 		}
 	}
-
-	return fmt.Errorf("no pending spawn found for session: %s", sessionName)
+	return nil
 }
 
-// PruneStalePending archives POLECAT_STARTED messages older than the given age.
-// Old spawns likely had their sessions die without triggering.
-func PruneStalePending(townRoot string, maxAge time.Duration) (int, error) {
+// ClearPendingSpawn archives all POLECAT_STARTED messages for a specific session,
+// removing it from the pending list. Used by the Deacon after observing that a
+// session has been triggered via AI-based observation.
+// Idempotent: returns nil if no matching pending spawn is found (race-safe).
+func ClearPendingSpawn(townRoot, sessionName string) error {
 	pending, err := CheckInboxForSpawns(townRoot)
 	if err != nil {
-		return 0, err
+		return fmt.Errorf("checking inbox: %w", err)
 	}
+	return clearPendingSpawnFromList(pending, sessionName)
+}
 
+// pruneStalePendingFromList archives POLECAT_STARTED messages older than the
+// given age from the provided pending list. Extracted for testability.
+func pruneStalePendingFromList(pending []*PendingSpawn, maxAge time.Duration) (int, error) {
 	cutoff := time.Now().Add(-maxAge)
 	pruned := 0
 
 	for _, ps := range pending {
 		if ps.SpawnedAt.Before(cutoff) {
-			// Archive stale spawn message
-			if ps.mailbox != nil {
-				_ = ps.mailbox.Archive(ps.MailID)
+			if ps.mailbox == nil {
+				return pruned, fmt.Errorf("nil mailbox for stale pending spawn (mail_id: %s)", ps.MailID)
+			}
+			if err := ps.mailbox.Archive(ps.MailID); err != nil {
+				return pruned, fmt.Errorf("archiving stale mail %s: %w", ps.MailID, err)
 			}
 			pruned++
 		}
 	}
 
 	return pruned, nil
 }
+
+// PruneStalePending archives POLECAT_STARTED messages older than the given age.
+// Old spawns likely had their sessions die without triggering.
+func PruneStalePending(townRoot string, maxAge time.Duration) (int, error) {
+	pending, err := CheckInboxForSpawns(townRoot)
+	if err != nil {
+		return 0, err
+	}
+	return pruneStalePendingFromList(pending, maxAge)
+}