Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions .specify/memory/constitution.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,14 @@ New dependencies require documented rationale in research.md with:

## Quality Gates

Before any feature is considered complete:
Before any feature is considered complete (must match CI pipeline):

1. **Coverage**: `go test -cover ./...` reports >= 80%
2. **Race Detection**: `go test -race ./...` passes with no data races
1. **Formatting**: `gofmt -l .` produces no output (no unformatted files)
2. **Dependencies**: `go mod verify` passes with no errors
3. **Static Analysis**: `go vet ./...` passes with no errors
4. **Formatting**: `go fmt ./...` produces no changes
5. **Vulnerability Scan**: `govulncheck ./...` reports no known vulnerabilities
6. **Security Scan**: `gosec ./...` passes with no high/critical findings
4. **Tests**: `go test -v -race -coverprofile=coverage.out ./...` passes with >= 80% coverage
5. **Vulnerabilities**: `govulncheck ./...` reports no vulnerabilities
6. **Security**: `gosec ./...` reports no issues
7. **Spec Compliance**: All acceptance scenarios from spec.md pass

## Governance
Expand All @@ -104,4 +104,4 @@ This constitution supersedes all other development practices for AgentMail.
- Violations require explicit justification or constitution amendment
- `/speckit.analyze` checks constitution alignment automatically

**Version**: 1.2.0 | **Ratified**: 2026-01-11 | **Last Amended**: 2026-01-15
**Version**: 1.2.0 | **Ratified**: 2026-01-11 | **Last Amended**: 2026-01-17
18 changes: 11 additions & 7 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,16 @@ Once Go source files are added:
- `go vet ./...` - Run static analysis
- `go fmt ./...` - Format code

## Quality gates
## Quality Gates (CI Pipeline)

- The build system shall produce agentmail binary
- The build system shall pass `go test -v -race ./...` without errors
- The build system shall pass `go vet ./...` without errors
- The build system shall pass `go fmt ./...` without errors
- The build system shall pass `govulncheck ./...` without errors
- The build system shall pass `gosec ./...` without errors
The following checks run in CI and must pass before merge:

1. **Formatting**: `gofmt -l .` - must produce no output
2. **Dependencies**: `go mod verify` - must pass
3. **Static Analysis**: `go vet ./...` - must pass
4. **Tests**: `go test -v -race -coverprofile=coverage.out ./...` - must pass with >= 80% coverage
5. **Vulnerabilities**: `govulncheck ./...` - must report no vulnerabilities
6. **Security**: `gosec ./...` - must report no issues

## Testing in CI Environment

Expand Down Expand Up @@ -96,6 +98,8 @@ Templates are stored in `.specify/templates/` and project constitution in `.spec
- Go 1.21+ (per constitution IC-001) + Standard library only (os, time, syscall) + fsnotify (external - requires justification) (009-watch-files)
- Go 1.25.5 (per go.mod, constitution requires 1.21+) + github.com/modelcontextprotocol/go-sdk (official MCP SDK) (010-mcp-server)
- JSONL files in `.agentmail/` directory (existing infrastructure), MCP server via STDIO transport (010-mcp-server)
- Go 1.21+ (per constitution IC-001, project uses Go 1.25.3) + Standard library only (os/exec, encoding/json, syscall, time, os) (011-cleanup)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix Go version inconsistency.

Line 101 references "Go 1.25.3" while the rest of the document consistently uses "Go 1.25.5" (lines 59, 84, 92, 99). This inconsistency could confuse developers about the actual Go version in use.

📝 Proposed fix
-- Go 1.21+ (per constitution IC-001, project uses Go 1.25.3) + Standard library only (os/exec, encoding/json, syscall, time, os) (011-cleanup)
+- Go 1.21+ (per constitution IC-001, project uses Go 1.25.5) + Standard library only (os/exec, encoding/json, syscall, time, os) (011-cleanup)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Go 1.21+ (per constitution IC-001, project uses Go 1.25.3) + Standard library only (os/exec, encoding/json, syscall, time, os) (011-cleanup)
- Go 1.21+ (per constitution IC-001, project uses Go 1.25.5) + Standard library only (os/exec, encoding/json, syscall, time, os) (011-cleanup)
🤖 Prompt for AI Agents
In `@CLAUDE.md` at line 101, Update the inconsistent Go version string in
CLAUDE.md: replace the occurrence of "Go 1.25.3" with "Go 1.25.5" so it matches
the other references ("Go 1.25.5") in the document; ensure the sentence that
currently reads 'Go 1.21+ (per constitution IC-001, project uses Go 1.25.3) +
Standard library only ...' is edited to 'Go 1.21+ (per constitution IC-001,
project uses Go 1.25.5) + Standard library only ...' to keep the file
consistent.

- JSONL files in `.agentmail/` directory (recipients.jsonl, mailboxes/*.jsonl) (011-cleanup)

## Recent Changes
- 001-agent-mail-structure: Added Go 1.21+ (per IC-001) + Standard library only (os/exec for tmux, encoding/json for JSONL)
49 changes: 48 additions & 1 deletion cmd/agentmail/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,52 @@ Examples:
},
}

// Cleanup command flags
cleanupFlagSet := flag.NewFlagSet("agentmail cleanup", flag.ContinueOnError)
var (
staleHours int
deliveredHours int
dryRun bool
)
cleanupFlagSet.IntVar(&staleHours, "stale-hours", 48, "hours threshold for stale recipients")
cleanupFlagSet.IntVar(&deliveredHours, "delivered-hours", 2, "hours threshold for delivered messages")
cleanupFlagSet.BoolVar(&dryRun, "dry-run", false, "report what would be cleaned without deleting")

cleanupCmd := &ffcli.Command{
Name: "cleanup",
ShortUsage: "agentmail cleanup [flags]",
ShortHelp: "Remove stale data from AgentMail",
LongHelp: `Remove stale data from the AgentMail system.

This command cleans up:
- Offline recipients (windows that no longer exist)
- Stale recipients (not updated within threshold)
- Old delivered messages (read messages older than threshold)
- Empty mailbox files

Flags:
--stale-hours Hours threshold for stale recipients (default: 48)
--delivered-hours Hours threshold for delivered messages (default: 2)
--dry-run Report what would be cleaned without deleting

Examples:
agentmail cleanup
agentmail cleanup --dry-run
agentmail cleanup --stale-hours 24 --delivered-hours 1`,
FlagSet: cleanupFlagSet,
Exec: func(ctx context.Context, args []string) error {
exitCode := cli.Cleanup(os.Stdout, os.Stderr, cli.CleanupOptions{
StaleHours: staleHours,
DeliveredHours: deliveredHours,
DryRun: dryRun,
})
if exitCode != 0 {
os.Exit(exitCode)
}
return nil
},
Comment on lines +297 to +333
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Validate cleanup hour flags to prevent accidental mass deletion.

Negative values turn the cutoff into a future timestamp, which can remove all recipients/messages. Guard against <0 before invoking cleanup.

Proposed fix
 	Exec: func(ctx context.Context, args []string) error {
+		if staleHours < 0 || deliveredHours < 0 {
+			return errors.New("stale-hours and delivered-hours must be >= 0")
+		}
 		exitCode := cli.Cleanup(os.Stdout, os.Stderr, cli.CleanupOptions{
 			StaleHours:     staleHours,
 			DeliveredHours: deliveredHours,
 			DryRun:         dryRun,
 		})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cleanupFlagSet.IntVar(&staleHours, "stale-hours", 48, "hours threshold for stale recipients")
cleanupFlagSet.IntVar(&deliveredHours, "delivered-hours", 2, "hours threshold for delivered messages")
cleanupFlagSet.BoolVar(&dryRun, "dry-run", false, "report what would be cleaned without deleting")
cleanupCmd := &ffcli.Command{
Name: "cleanup",
ShortUsage: "agentmail cleanup [flags]",
ShortHelp: "Remove stale data from AgentMail",
LongHelp: `Remove stale data from the AgentMail system.
This command cleans up:
- Offline recipients (windows that no longer exist)
- Stale recipients (not updated within threshold)
- Old delivered messages (read messages older than threshold)
- Empty mailbox files
Flags:
--stale-hours Hours threshold for stale recipients (default: 48)
--delivered-hours Hours threshold for delivered messages (default: 2)
--dry-run Report what would be cleaned without deleting
Examples:
agentmail cleanup
agentmail cleanup --dry-run
agentmail cleanup --stale-hours 24 --delivered-hours 1`,
FlagSet: cleanupFlagSet,
Exec: func(ctx context.Context, args []string) error {
exitCode := cli.Cleanup(os.Stdout, os.Stderr, cli.CleanupOptions{
StaleHours: staleHours,
DeliveredHours: deliveredHours,
DryRun: dryRun,
})
if exitCode != 0 {
os.Exit(exitCode)
}
return nil
},
cleanupFlagSet.IntVar(&staleHours, "stale-hours", 48, "hours threshold for stale recipients")
cleanupFlagSet.IntVar(&deliveredHours, "delivered-hours", 2, "hours threshold for delivered messages")
cleanupFlagSet.BoolVar(&dryRun, "dry-run", false, "report what would be cleaned without deleting")
cleanupCmd := &ffcli.Command{
Name: "cleanup",
ShortUsage: "agentmail cleanup [flags]",
ShortHelp: "Remove stale data from AgentMail",
LongHelp: `Remove stale data from the AgentMail system.
This command cleans up:
- Offline recipients (windows that no longer exist)
- Stale recipients (not updated within threshold)
- Old delivered messages (read messages older than threshold)
- Empty mailbox files
Flags:
--stale-hours Hours threshold for stale recipients (default: 48)
--delivered-hours Hours threshold for delivered messages (default: 2)
--dry-run Report what would be cleaned without deleting
Examples:
agentmail cleanup
agentmail cleanup --dry-run
agentmail cleanup --stale-hours 24 --delivered-hours 1`,
FlagSet: cleanupFlagSet,
Exec: func(ctx context.Context, args []string) error {
if staleHours < 0 || deliveredHours < 0 {
return errors.New("stale-hours and delivered-hours must be >= 0")
}
exitCode := cli.Cleanup(os.Stdout, os.Stderr, cli.CleanupOptions{
StaleHours: staleHours,
DeliveredHours: deliveredHours,
DryRun: dryRun,
})
if exitCode != 0 {
os.Exit(exitCode)
}
return nil
},
🤖 Prompt for AI Agents
In `@cmd/agentmail/main.go` around lines 297 - 333, The cleanup command allows
negative values for staleHours and deliveredHours which can invert time cutoffs
and cause mass deletions; validate the flags in the Exec closure before calling
cli.Cleanup by checking staleHours and deliveredHours are >= 0 and returning a
non-zero exit (or printing an error) if not; update the Exec anonymous function
(the closure that constructs cli.CleanupOptions and calls cli.Cleanup) to
perform these checks and abort with a clear message rather than invoking
cli.Cleanup when values are negative.

}

// Root command help text
rootHelp := `agentmail - Inter-agent communication for tmux sessions

Expand All @@ -301,6 +347,7 @@ Commands:
mailman Start the mailman daemon
onboard Output agent onboarding context
mcp Start MCP server (STDIO transport)
cleanup Remove stale data from AgentMail

Use "agentmail <command> --help" for more information about a command.`

Expand All @@ -310,7 +357,7 @@ Use "agentmail <command> --help" for more information about a command.`
ShortHelp: "Inter-agent communication for tmux sessions",
LongHelp: rootHelp,
FlagSet: rootFlagSet,
Subcommands: []*ffcli.Command{sendCmd, receiveCmd, recipientsCmd, statusCmd, mailmanCmd, onboardCmd, mcpCmd},
Subcommands: []*ffcli.Command{sendCmd, receiveCmd, recipientsCmd, statusCmd, mailmanCmd, onboardCmd, mcpCmd, cleanupCmd},
Exec: func(ctx context.Context, args []string) error {
// No subcommand provided, show help
fmt.Fprintln(os.Stderr, rootHelp)
Expand Down
221 changes: 221 additions & 0 deletions internal/cli/cleanup.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
package cli

import (
"fmt"
"io"
"time"

"agentmail/internal/mail"
"agentmail/internal/tmux"
)

// formatSummary outputs the cleanup summary to stdout.
// If dryRun is true, uses "preview" language; otherwise uses "complete" language.
func formatSummary(stdout io.Writer, result CleanupResult, dryRun bool) {
if dryRun {
fmt.Fprintln(stdout, "Cleanup preview (dry-run):")
fmt.Fprintf(stdout, " Recipients to remove: %d (%d offline, %d stale)\n",
result.RecipientsRemoved, result.OfflineRemoved, result.StaleRemoved)
fmt.Fprintf(stdout, " Messages to remove: %d\n", result.MessagesRemoved)
fmt.Fprintf(stdout, " Mailboxes to remove: %d\n", result.MailboxesRemoved)
} else {
fmt.Fprintln(stdout, "Cleanup complete:")
fmt.Fprintf(stdout, " Recipients removed: %d (%d offline, %d stale)\n",
result.RecipientsRemoved, result.OfflineRemoved, result.StaleRemoved)
fmt.Fprintf(stdout, " Messages removed: %d\n", result.MessagesRemoved)
fmt.Fprintf(stdout, " Mailboxes removed: %d\n", result.MailboxesRemoved)
}
}

// formatSkippedWarning outputs a warning about skipped files to stderr.
func formatSkippedWarning(stderr io.Writer, skipped int) {
if skipped > 0 {
fmt.Fprintf(stderr, "Warning: Skipped %d locked file(s)\n", skipped)
}
}

// CleanupOptions configures the Cleanup command behavior
type CleanupOptions struct {
StaleHours int // Hours threshold for stale recipients (default: 48)
DeliveredHours int // Hours threshold for delivered messages (default: 2)
DryRun bool // If true, report what would be cleaned without deleting

// Testing options
RepoRoot string // Repository root (defaults to "." if empty)
SkipTmuxCheck bool // Skip real tmux check (for testing)
MockInTmux bool // Mocked value for InTmux() when SkipTmuxCheck is true
MockWindows []string // Mock list of tmux windows (for testing, nil means use real tmux)
}

// CleanupResult holds the counts from a cleanup operation
type CleanupResult struct {
RecipientsRemoved int // Total recipients removed
OfflineRemoved int // Recipients removed because window doesn't exist
StaleRemoved int // Recipients removed because updated_at expired
MessagesRemoved int // Messages removed (read + old)
MailboxesRemoved int // Empty mailbox files removed
FilesSkipped int // Files skipped due to lock contention
}

// Cleanup removes stale data from the AgentMail system.
// It removes offline recipients, stale recipients, old delivered messages, and empty mailboxes.
//
// FR-001: Compare each recipient in recipients.jsonl against current tmux window names
// FR-002: Remove recipients whose names don't match any current tmux window
// FR-020: Skip offline recipient check when not in tmux
// FR-021: Output warning when offline check is skipped due to non-tmux environment
// FR-022: Report zero recipients removed when recipients.jsonl doesn't exist
// FR-014: Output summary after cleanup
func Cleanup(stdout, stderr io.Writer, opts CleanupOptions) int {
result := CleanupResult{}

// Determine repository root
repoRoot := opts.RepoRoot
if repoRoot == "" {
repoRoot = "."
}

// Determine if we're in tmux
var inTmux bool
var isMocking bool

if opts.SkipTmuxCheck {
// Testing mode - use mocked values
inTmux = opts.MockInTmux
isMocking = true
} else {
// Production mode - use real tmux check
inTmux = tmux.InTmux()
isMocking = false
}

// Phase 1: Clean offline recipients (US1)
if inTmux {
// Get list of valid tmux windows
var windows []string
if isMocking {
windows = opts.MockWindows
} else {
var err error
windows, err = tmux.ListWindows()
if err != nil {
fmt.Fprintf(stderr, "Warning: failed to list tmux windows: %v\n", err)
// Continue without offline cleanup
windows = nil
}
}

// Clean or count offline recipients if we have a window list
if windows != nil {
if opts.DryRun {
// Dry-run mode: just count
count, err := mail.CountOfflineRecipients(repoRoot, windows)
if err != nil {
fmt.Fprintf(stderr, "Error counting offline recipients: %v\n", err)
return 1
}
result.OfflineRemoved = count
result.RecipientsRemoved += count
} else {
// Normal mode: actually remove
removed, err := mail.CleanOfflineRecipients(repoRoot, windows)
if err != nil {
fmt.Fprintf(stderr, "Error cleaning offline recipients: %v\n", err)
return 1
}
result.OfflineRemoved = removed
result.RecipientsRemoved += removed
}
}
} else {
// FR-020 & FR-021: Not in tmux - skip offline check with warning
fmt.Fprintln(stderr, "Warning: not running in tmux session, skipping offline recipient check")
}

// Phase 2: Clean stale recipients (US2)
// Remove recipients whose updated_at is older than StaleHours threshold
staleThreshold := time.Duration(opts.StaleHours) * time.Hour
if opts.DryRun {
// Dry-run mode: just count
count, err := mail.CountStaleStates(repoRoot, staleThreshold)
if err != nil {
fmt.Fprintf(stderr, "Error counting stale recipients: %v\n", err)
return 1
}
result.StaleRemoved = count
result.RecipientsRemoved += count
} else {
// Normal mode: actually remove
staleRemoved, err := mail.CleanStaleStates(repoRoot, staleThreshold)
if err != nil {
fmt.Fprintf(stderr, "Error cleaning stale recipients: %v\n", err)
return 1
}
result.StaleRemoved = staleRemoved
result.RecipientsRemoved += staleRemoved
}
Comment on lines +108 to +156
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Dry-run double-counts recipients that are both offline and stale.

In dry-run, offline and stale counts are computed independently against the same on-disk data. Any recipient that is both offline and stale will be counted twice, so the preview won’t match the actual removal order (offline first, then stale). Consider computing stale counts after excluding offline recipients (e.g., one pass over recipients with both predicates).

🤖 Prompt for AI Agents
In `@internal/cli/cleanup.go` around lines 108 - 156, Dry-run currently
double-counts recipients that are both offline and stale because
CountOfflineRecipients and CountStaleStates operate independently; update the
dry-run path to exclude offline recipients when counting stale ones.
Specifically, when opts.DryRun is true and windows != nil, first obtain the
offline recipient IDs (e.g., via mail.ListOfflineRecipients or extend
mail.CountOfflineRecipients to return IDs), then call a new/counting helper that
accepts an exclusion set (e.g., mail.CountStaleStatesExcluding(repoRoot,
staleThreshold, excludedIDs)) or filter the stale count by subtracting
recipients present in the offline set before assigning result.StaleRemoved and
result.RecipientsRemoved; mirror this logic for the case where windows == nil by
using an empty exclusion set. Ensure references to CountOfflineRecipients,
ListOfflineRecipients (or the modified CountOfflineRecipients), CountStaleStates
(or new CountStaleStatesExcluding), opts.DryRun, windows, staleThreshold, and
result are used to locate and implement the change.


// Phase 3: Clean old delivered messages (US3)
// Remove read messages older than DeliveredHours threshold
deliveredThreshold := time.Duration(opts.DeliveredHours) * time.Hour
Comment on lines +72 to +160
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard against zero/negative thresholds to avoid unintended mass cleanup.

CleanupOptions has zero-value defaults; with StaleHours/DeliveredHours unset (or negative), thresholds become <= 0 and can remove nearly everything, which contradicts the documented defaults (48h/2h). Consider defaulting or rejecting invalid values inside Cleanup to make the API safe by default.

🛡️ Suggested defaulting/validation
@@
 	// Determine repository root
 	repoRoot := opts.RepoRoot
 	if repoRoot == "" {
 		repoRoot = "."
 	}
+
+	// Apply safe defaults / validation for thresholds
+	if opts.StaleHours <= 0 {
+		opts.StaleHours = 48
+	}
+	if opts.DeliveredHours <= 0 {
+		opts.DeliveredHours = 2
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Determine repository root
repoRoot := opts.RepoRoot
if repoRoot == "" {
repoRoot = "."
}
// Determine if we're in tmux
var inTmux bool
var isMocking bool
if opts.SkipTmuxCheck {
// Testing mode - use mocked values
inTmux = opts.MockInTmux
isMocking = true
} else {
// Production mode - use real tmux check
inTmux = tmux.InTmux()
isMocking = false
}
// Phase 1: Clean offline recipients (US1)
if inTmux {
// Get list of valid tmux windows
var windows []string
if isMocking {
windows = opts.MockWindows
} else {
var err error
windows, err = tmux.ListWindows()
if err != nil {
fmt.Fprintf(stderr, "Warning: failed to list tmux windows: %v\n", err)
// Continue without offline cleanup
windows = nil
}
}
// Clean or count offline recipients if we have a window list
if windows != nil {
if opts.DryRun {
// Dry-run mode: just count
count, err := mail.CountOfflineRecipients(repoRoot, windows)
if err != nil {
fmt.Fprintf(stderr, "Error counting offline recipients: %v\n", err)
return 1
}
result.OfflineRemoved = count
result.RecipientsRemoved += count
} else {
// Normal mode: actually remove
removed, err := mail.CleanOfflineRecipients(repoRoot, windows)
if err != nil {
fmt.Fprintf(stderr, "Error cleaning offline recipients: %v\n", err)
return 1
}
result.OfflineRemoved = removed
result.RecipientsRemoved += removed
}
}
} else {
// FR-020 & FR-021: Not in tmux - skip offline check with warning
fmt.Fprintln(stderr, "Warning: not running in tmux session, skipping offline recipient check")
}
// Phase 2: Clean stale recipients (US2)
// Remove recipients whose updated_at is older than StaleHours threshold
staleThreshold := time.Duration(opts.StaleHours) * time.Hour
if opts.DryRun {
// Dry-run mode: just count
count, err := mail.CountStaleStates(repoRoot, staleThreshold)
if err != nil {
fmt.Fprintf(stderr, "Error counting stale recipients: %v\n", err)
return 1
}
result.StaleRemoved = count
result.RecipientsRemoved += count
} else {
// Normal mode: actually remove
staleRemoved, err := mail.CleanStaleStates(repoRoot, staleThreshold)
if err != nil {
fmt.Fprintf(stderr, "Error cleaning stale recipients: %v\n", err)
return 1
}
result.StaleRemoved = staleRemoved
result.RecipientsRemoved += staleRemoved
}
// Phase 3: Clean old delivered messages (US3)
// Remove read messages older than DeliveredHours threshold
deliveredThreshold := time.Duration(opts.DeliveredHours) * time.Hour
// Determine repository root
repoRoot := opts.RepoRoot
if repoRoot == "" {
repoRoot = "."
}
// Apply safe defaults / validation for thresholds
if opts.StaleHours <= 0 {
opts.StaleHours = 48
}
if opts.DeliveredHours <= 0 {
opts.DeliveredHours = 2
}
// Determine if we're in tmux
var inTmux bool
var isMocking bool
if opts.SkipTmuxCheck {
// Testing mode - use mocked values
inTmux = opts.MockInTmux
isMocking = true
} else {
// Production mode - use real tmux check
inTmux = tmux.InTmux()
isMocking = false
}
// Phase 1: Clean offline recipients (US1)
if inTmux {
// Get list of valid tmux windows
var windows []string
if isMocking {
windows = opts.MockWindows
} else {
var err error
windows, err = tmux.ListWindows()
if err != nil {
fmt.Fprintf(stderr, "Warning: failed to list tmux windows: %v\n", err)
// Continue without offline cleanup
windows = nil
}
}
// Clean or count offline recipients if we have a window list
if windows != nil {
if opts.DryRun {
// Dry-run mode: just count
count, err := mail.CountOfflineRecipients(repoRoot, windows)
if err != nil {
fmt.Fprintf(stderr, "Error counting offline recipients: %v\n", err)
return 1
}
result.OfflineRemoved = count
result.RecipientsRemoved += count
} else {
// Normal mode: actually remove
removed, err := mail.CleanOfflineRecipients(repoRoot, windows)
if err != nil {
fmt.Fprintf(stderr, "Error cleaning offline recipients: %v\n", err)
return 1
}
result.OfflineRemoved = removed
result.RecipientsRemoved += removed
}
}
} else {
// FR-020 & FR-021: Not in tmux - skip offline check with warning
fmt.Fprintln(stderr, "Warning: not running in tmux session, skipping offline recipient check")
}
// Phase 2: Clean stale recipients (US2)
// Remove recipients whose updated_at is older than StaleHours threshold
staleThreshold := time.Duration(opts.StaleHours) * time.Hour
if opts.DryRun {
// Dry-run mode: just count
count, err := mail.CountStaleStates(repoRoot, staleThreshold)
if err != nil {
fmt.Fprintf(stderr, "Error counting stale recipients: %v\n", err)
return 1
}
result.StaleRemoved = count
result.RecipientsRemoved += count
} else {
// Normal mode: actually remove
staleRemoved, err := mail.CleanStaleStates(repoRoot, staleThreshold)
if err != nil {
fmt.Fprintf(stderr, "Error cleaning stale recipients: %v\n", err)
return 1
}
result.StaleRemoved = staleRemoved
result.RecipientsRemoved += staleRemoved
}
// Phase 3: Clean old delivered messages (US3)
// Remove read messages older than DeliveredHours threshold
deliveredThreshold := time.Duration(opts.DeliveredHours) * time.Hour
🤖 Prompt for AI Agents
In `@internal/cli/cleanup.go` around lines 72 - 160, The Cleanup function must
guard against zero/negative thresholds from CleanupOptions (StaleHours,
DeliveredHours) before computing staleThreshold and deliveredThreshold; update
Cleanup (where opts is used) to validate and/or default these values (e.g., if
opts.StaleHours <= 0 set opts.StaleHours = 48 and if opts.DeliveredHours <= 0
set opts.DeliveredHours = 2) so time.Duration(opts.StaleHours)*time.Hour and
time.Duration(opts.DeliveredHours)*time.Hour cannot be non-positive, or
alternatively return an error if invalid; ensure you adjust any tests/mocks that
rely on Mock values (MockWindows/MockInTmux) as needed and reference
staleThreshold, deliveredThreshold, CountStaleStates and
CleanStaleStates/CleanDeliveredMessages code paths to confirm behavior.


// List all mailbox files
recipients, err := mail.ListMailboxRecipients(repoRoot)
if err != nil {
fmt.Fprintf(stderr, "Error listing mailboxes: %v\n", err)
return 1
}

if opts.DryRun {
// Dry-run mode: just count
for _, recipient := range recipients {
count, err := mail.CountOldMessages(repoRoot, recipient, deliveredThreshold)
if err != nil {
fmt.Fprintf(stderr, "Error counting messages in mailbox %s: %v\n", recipient, err)
return 1
}
result.MessagesRemoved += count
}
Comment on lines +169 to +178
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Dry-run should skip locked mailboxes instead of failing.

Normal mode treats mail.ErrFileLocked as a skip and continues, but dry-run exits with error on the same condition. This breaks parity and makes dry-run unusable if any mailbox is locked. Handle ErrFileLocked consistently and increment FilesSkipped.

♻️ Suggested fix
@@
 		for _, recipient := range recipients {
 			count, err := mail.CountOldMessages(repoRoot, recipient, deliveredThreshold)
 			if err != nil {
+				if err == mail.ErrFileLocked {
+					result.FilesSkipped++
+					continue
+				}
 				fmt.Fprintf(stderr, "Error counting messages in mailbox %s: %v\n", recipient, err)
 				return 1
 			}
 			result.MessagesRemoved += count
 		}
🤖 Prompt for AI Agents
In `@internal/cli/cleanup.go` around lines 169 - 178, Dry-run currently aborts on
mail.CountOldMessages returning mail.ErrFileLocked; change the dry-run loop over
recipients so that if mail.CountOldMessages returns mail.ErrFileLocked you
increment result.FilesSkipped and continue to the next recipient (do not return
1), but for other errors keep the existing error handling that prints to stderr
and returns 1; keep updating result.MessagesRemoved only when count succeeds.
This preserves parity with the normal delete path and uses the existing symbols
opts.DryRun, mail.CountOldMessages, mail.ErrFileLocked, result.MessagesRemoved,
result.FilesSkipped, and the recipients loop.

} else {
// Normal mode: actually clean each mailbox
for _, recipient := range recipients {
removed, err := mail.CleanOldMessages(repoRoot, recipient, deliveredThreshold)
if err != nil {
// Check if it's a lock failure - skip file and continue
if err == mail.ErrFileLocked {
result.FilesSkipped++
continue
}
fmt.Fprintf(stderr, "Error cleaning mailbox %s: %v\n", recipient, err)
return 1
}
result.MessagesRemoved += removed
}
}

// Phase 4: Remove empty mailboxes (US4)
// This runs AFTER message cleanup so that mailboxes emptied by Phase 3 are also removed
if opts.DryRun {
// Dry-run mode: just count
count, err := mail.CountEmptyMailboxes(repoRoot)
if err != nil {
fmt.Fprintf(stderr, "Error counting empty mailboxes: %v\n", err)
return 1
}
result.MailboxesRemoved = count
} else {
// Normal mode: actually remove
mailboxesRemoved, err := mail.RemoveEmptyMailboxes(repoRoot)
if err != nil {
fmt.Fprintf(stderr, "Error removing empty mailboxes: %v\n", err)
return 1
}
result.MailboxesRemoved = mailboxesRemoved
}

// Phase 5: Output summary (FR-014) and warnings
formatSummary(stdout, result, opts.DryRun)
formatSkippedWarning(stderr, result.FilesSkipped)

return 0
}
Loading