feat: add cleanup command for stale data removal#32
Conversation
Phase 1 of cleanup command implementation: - Add CreatedAt time.Time field to Message struct with omitempty tag - Update Append function to set CreatedAt timestamp when creating messages - Add comprehensive tests for timestamp serialization and backward compatibility
Phase 2 of cleanup command implementation: - Add CleanupOptions struct with StaleHours, DeliveredHours, DryRun - Add CleanupResult struct with all cleanup count fields - Implement TryLockWithTimeout for non-blocking file locks with 1s timeout - Add tests for lock timeout behavior - Register cleanup subcommand with flags in main.go
Phase 3 - User Story 1: Remove offline recipients - Add CleanOfflineRecipients function to compare recipients against valid tmux windows - Integrate offline cleanup in Cleanup function with tmux environment detection - Output warning when not in tmux session and offline check is skipped - Track OfflineRemoved count in CleanupResult - Add comprehensive tests for offline recipient cleanup scenarios
Phase 4 - User Story 2: Remove stale recipients - Update CleanStaleStates to return count of removed recipients - Integrate stale cleanup in Cleanup function with configurable StaleHours - Track StaleRemoved separately from OfflineRemoved in CleanupResult - Add tests for stale recipient removal with default and custom thresholds - Update all callers of CleanStaleStates for new (int, error) signature
Phase 5 - User Story 3: Remove old delivered messages - Add CleanOldMessages function to filter messages by read_flag and created_at - Iterate all mailbox files and apply message cleanup - Never delete unread messages regardless of age - Skip messages without created_at timestamp (backward compatibility) - Handle locked files gracefully (skip and increment FilesSkipped) - Add comprehensive tests for message cleanup scenarios
Phase 6 - User Story 4: Remove empty mailboxes - Add RemoveEmptyMailboxes function to identify and delete empty mailbox files - Integrate empty mailbox cleanup in Cleanup function after message cleanup - Track MailboxesRemoved count in CleanupResult - Handle missing mailboxes directory gracefully - Add comprehensive tests for empty mailbox cleanup scenarios
Phase 7 - User Stories 5 & 6: Verify exclusions - Add test verifying onboard output doesn't contain "cleanup" - Add test verifying MCP tools list doesn't include cleanup tool - Verify existing onboard.go and tools.go correctly exclude cleanup - Cleanup is an administrative command, not for agent-to-agent communication
Phase 8 - Output & Dry-Run: - Add formatSummary function for cleanup result output - Add formatSkippedWarning for locked file warnings - Add counting functions for dry-run mode: - CountOfflineRecipients, CountStaleStates - CountOldMessages, CountEmptyMailboxes - Dry-run reports what would be cleaned without making changes - Summary output shows breakdown of offline vs stale recipients - Add comprehensive tests for output and dry-run behavior
Phase 9 - Polish & Cross-Cutting Concerns: - All tests pass with race detection (go test -v -race ./...) - No static analysis issues (go vet ./...) - Code properly formatted (go fmt ./...) - No security issues (gosec ./...) - No vulnerabilities (govulncheck ./...) - Quickstart examples verified working
📝 WalkthroughWalkthroughAdds an agentmail cleanup CLI and orchestration to remove offline recipients, stale recipient states, old delivered messages, and empty mailboxes; introduces Message.CreatedAt, non-blocking file-lock helper, mailbox cleanup/count APIs, CLI flags (--stale-hours, --delivered-hours, --dry-run), tests, and spec/docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "cli.Cleanup"
participant Tmux
participant Mail
participant Mailbox
User->>CLI: run cleanup (opts)
CLI->>Tmux: ListWindows() (unless SkipTmuxCheck)
alt tmux present
Tmux-->>CLI: windows list
CLI->>Mail: CleanOfflineRecipients(validWindows)
Mail-->>CLI: offline removal count
else not in tmux
CLI-->>CLI: skip offline phase (emit warning)
end
CLI->>Mail: CountStaleStates / CleanStaleStates(threshold)
Mail-->>CLI: stale count
CLI->>Mailbox: For each recipient -> CleanOldMessages(threshold)
Mailbox->>Mailbox: TryLockWithTimeout -> read, filter, rewrite
alt file locked / timeout
Mailbox-->>CLI: ErrFileLocked (skip, increment FilesSkipped)
else success
Mailbox-->>CLI: messages removed count
end
CLI->>Mailbox: RemoveEmptyMailboxes()
Mailbox-->>CLI: mailboxes removed count
CLI->>CLI: formatSummary (dry-run vs actual)
CLI-->>User: summary + warnings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2026-01-15T18:50:18.457ZApplied to files:
📚 Learning: 2026-01-15T18:50:18.457ZApplied to files:
🧬 Code graph analysis (1)internal/mail/mailbox.go (2)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/daemon/loop.go (1)
302-306: Log cleanup errors instead of fully discarding them.Best‑effort is fine, but swallowing errors removes useful diagnostics during daemon operation.
Suggested adjustment
func cleanStaleStates(repoRoot string, logger io.Writer) { if logger != nil { fmt.Fprintf(logger, "[mailman] Cleaning stale recipient states (threshold: %v)\n", DefaultStaleThreshold) } - _, _ = mail.CleanStaleStates(repoRoot, DefaultStaleThreshold) // G104: best-effort cleanup, errors don't stop the daemon + _, err := mail.CleanStaleStates(repoRoot, DefaultStaleThreshold) // G104: best-effort cleanup + if err != nil && logger != nil { + fmt.Fprintf(logger, "[mailman] Cleanup stale states failed: %v\n", err) + } }
🤖 Fix all issues with AI agents
In `@cmd/agentmail/main.go`:
- Around line 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.
In `@internal/cli/cleanup.go`:
- Around line 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.
- Around line 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.
- Around line 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.
In `@internal/mail/mailbox.go`:
- Around line 276-280: Replace the blocking syscall.Flock(int(file.Fd()),
syscall.LOCK_EX) call with a non-blocking TryLockWithTimeout using a 1 second
timeout (i.e. TryLockWithTimeout(file.Fd(), time.Second)); if TryLockWithTimeout
reports a timeout, close the file and treat it as a skipped file (close file and
return/propagate a skip result instead of blocking), otherwise proceed as before
and ensure you release the lock on error/cleanup; update imports if needed and
remove the direct syscall.Flock/LOCK_EX usage.
- Around line 453-465: The loop calls ReadAll(repoRoot, recipient) which
releases its shared lock before os.Remove(filePath), creating a TOCTOU where
another process could append a message and be lost; fix by re-checking the
mailbox under an exclusive lock before removing: after ReadAll returns zero
messages, open the mailbox file (filePath) and acquire an exclusive lock (or
upgrade the existing lock) on that open descriptor, then re-validate that the
file is still empty (e.g., stat or read/seek to confirm size/has no new
messages) and only then call os.Remove (or unlink via the locked descriptor) and
increment removedCount; ensure any file opens/locks are properly closed/released
and handle IsNotExist/other errors the same way.
In `@specs/011-cleanup/tasks.md`:
- Around line 224-233: The fenced code blocks under the headings like "Phase 1
(Setup)" and "Phase 2 (Foundational)" are missing surrounding blank lines and a
language tag; update each block (e.g., the block containing "T001, T002, T003
can run in parallel (different files)" and the blocks at 229-233, 235-238,
240-244) to add a blank line before the opening fence and after the closing
fence and add a language identifier (use "text" or "text-plain") after the
opening ``` so they become, for example, a blank line, ```text, the content
line(s), then ``` and another blank line; ensure all similar fenced blocks in
this section follow the same pattern.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
cmd/agentmail/main.gointernal/cli/cleanup.gointernal/cli/cleanup_test.gointernal/cli/onboard_test.gointernal/daemon/loop.gointernal/daemon/loop_test.gointernal/mail/mailbox.gointernal/mail/mailbox_test.gointernal/mail/message.gointernal/mail/message_test.gointernal/mail/recipients.gointernal/mail/recipients_test.gointernal/mcp/tools_test.gospecs/011-cleanup/checklists/requirements.mdspecs/011-cleanup/data-model.mdspecs/011-cleanup/plan.mdspecs/011-cleanup/quickstart.mdspecs/011-cleanup/research.mdspecs/011-cleanup/spec.mdspecs/011-cleanup/tasks.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: All code must passgo test -v -race ./...without errors
All code must passgo vet ./...static analysis without errors
All code must passgo fmt ./...formatting without errors
All code must passgovulncheck ./...vulnerability scanning without errors
All code must passgosec ./...security analysis without errors
Use Go 1.21+ (project uses Go 1.25.5) with Standard library only - no external dependencies except where explicitly justified
Message storage must use JSONL format in.agentmail/mailboxes/<recipient>.jsonlfiles with file locking for atomic operations
AgentMail CLI must check that it is running inside a tmux session and use tmux window names as valid recipients
Files:
internal/mcp/tools_test.gointernal/daemon/loop.gointernal/mail/message_test.gointernal/daemon/loop_test.gointernal/mail/mailbox_test.gointernal/mail/mailbox.gointernal/cli/onboard_test.gointernal/cli/cleanup.gointernal/mail/message.gointernal/mail/recipients_test.gocmd/agentmail/main.gointernal/cli/cleanup_test.gointernal/mail/recipients.go
🧠 Learnings (3)
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : AgentMail CLI must check that it is running inside a tmux session and use tmux window names as valid recipients
Applied to files:
specs/011-cleanup/quickstart.mdinternal/daemon/loop_test.gointernal/mail/mailbox_test.gointernal/cli/cleanup.gospecs/011-cleanup/plan.mdcmd/agentmail/main.gointernal/cli/cleanup_test.gospecs/011-cleanup/data-model.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : Message storage must use JSONL format in `.agentmail/mailboxes/<recipient>.jsonl` files with file locking for atomic operations
Applied to files:
specs/011-cleanup/research.mdinternal/mail/mailbox_test.gointernal/mail/mailbox.gointernal/cli/cleanup.gospecs/011-cleanup/plan.mdinternal/mail/message.gointernal/cli/cleanup_test.gointernal/mail/recipients.gospecs/011-cleanup/data-model.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Build system must produce agentmail binary
Applied to files:
cmd/agentmail/main.go
🧬 Code graph analysis (12)
internal/mcp/tools_test.go (1)
internal/mcp/tools.go (4)
ToolSend(16-16)ToolReceive(17-17)ToolStatus(18-18)ToolListRecipients(19-19)
internal/daemon/loop.go (1)
internal/mail/recipients.go (1)
CleanStaleStates(255-320)
internal/mail/message_test.go (1)
internal/mail/message.go (1)
Message(11-18)
internal/daemon/loop_test.go (2)
internal/mail/recipients.go (1)
CleanStaleStates(255-320)internal/daemon/daemon_test.go (1)
TestStartDaemon_WatcherMode_CleansStaleStatesOnEvent(764-844)
specs/011-cleanup/research.md (1)
internal/daemon/daemon_test.go (1)
TestStartDaemon_WatcherMode_CleansStaleStatesOnEvent(764-844)
specs/011-cleanup/spec.md (1)
.specify/scripts/bash/update-agent-context.sh (1)
cleanup(109-114)
internal/mail/mailbox_test.go (2)
internal/mail/mailbox.go (4)
Append(86-129)ReadAll(132-176)TryLockWithTimeout(53-70)ErrFileLocked(24-24)internal/mail/message.go (1)
Message(11-18)
internal/mail/mailbox.go (2)
internal/mail/message.go (1)
Message(11-18)internal/mail/recipients.go (1)
ListMailboxRecipients(223-250)
internal/cli/onboard_test.go (1)
internal/cli/onboard.go (2)
Onboard(28-66)OnboardOptions(9-11)
specs/011-cleanup/plan.md (1)
internal/cli/integration_test.go (2)
TestIntegration_MultiAgentFileIsolation(176-263)TestIntegration_SendReceiveRoundTrip(24-98)
internal/mail/recipients_test.go (1)
internal/mail/recipients.go (1)
CleanStaleStates(255-320)
cmd/agentmail/main.go (1)
internal/cli/cleanup.go (2)
Cleanup(69-221)CleanupOptions(38-48)
🪛 LanguageTool
specs/011-cleanup/spec.md
[style] ~29-~29: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., Then no entries are removed. 3. Given recipients.jsonl is empty or doesn't ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~45-~45: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., Then that entry is retained. 3. Given user specifies --stale-hours 24, **...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...essages are never deleted by age). 3. Given a mailbox contains a message with `re...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~62-~62: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Then that message is retained. 4. Given user specifies --delivered-hours 1,...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~111-~111: This phrase is redundant. Consider using “outside”.
Context: ...Cases - What happens when cleanup runs outside of a tmux session? The offline recipient c...
(OUTSIDE_OF)
specs/011-cleanup/tasks.md
[style] ~190-~190: Consider using a different verb for a more formal wording.
Context: ... code - [X] T050 Run go vet ./... and fix any issues - [X] T051 Run `go fmt ./......
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
specs/011-cleanup/tasks.md
27-27: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
49-49: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
102-102: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
110-110: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
110-110: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (28)
internal/mcp/tools_test.go (1)
468-501: Good coverage for MCP tool exposure guard.This test clearly enforces that
cleanupstays out of MCP and that only the expected tool set is registered.internal/mail/message.go (1)
3-18: LGTM — CreatedAt integration is straightforward.The new timestamp field fits the cleanup requirements and doesn’t introduce behavioral risk.
internal/mail/message_test.go (1)
176-323: Nice test coverage for CreatedAt serialization and legacy handling.The new cases cover round-trip, explicit timestamps, and legacy payloads well.
internal/mail/recipients_test.go (1)
682-684: LGTM — tests now match the updated CleanStaleStates signature.Also applies to: 725-727, 752-754
specs/011-cleanup/quickstart.md (1)
1-143: Clear, actionable quickstart documentation.The usage examples, flags, and behavior notes read well and align with the feature scope.
internal/daemon/loop_test.go (1)
355-356: LGTM: updated CleanStaleStates call sites.Capturing the new count return keeps tests aligned with the updated function signature.
Also applies to: 385-386, 405-406, 556-557
internal/cli/onboard_test.go (1)
145-162: Good guardrail test for onboarding output.This prevents accidental exposure of admin-only commands in onboarding content.
specs/011-cleanup/checklists/requirements.md (1)
1-54: Solid checklist coverage.Clear acceptance/quality gates and traceability to the spec.
specs/011-cleanup/research.md (1)
1-136: Clear and well-structured research notes.The design rationale and decisions are well captured.
internal/mail/mailbox_test.go (4)
85-128: LGTM!The test correctly validates the
CreatedAttimestamp by capturing time boundaries before and after theAppendcall, then verifying the restored message'sCreatedAtfalls within that window. UsingReadAllfor validation is a good approach as it tests the actual read path.
152-208: LGTM!The test properly validates
CreatedAttimestamps for multiple appended messages, each with its own time window boundaries. The comprehensive field checks ensure message integrity is maintained across append operations.
508-522: LGTM!The test correctly verifies that
TryLockWithTimeoutsucceeds immediately on an unlocked file. Resource cleanup withdeferis properly handled.
524-559: LGTM!The test correctly simulates a lock contention scenario using two file handles to the same file. The 90ms lower bound check provides reasonable margin for OS scheduling variance while still verifying the timeout behavior. The
deferstatements ensure proper cleanup.specs/011-cleanup/plan.md (1)
1-104: LGTM!The implementation plan is well-structured with clear phases, constitution checks, and artifact tracking. The technical context accurately reflects the project's constraints (stdlib-only, JSONL storage, file locking).
specs/011-cleanup/spec.md (1)
1-193: LGTM!The specification is comprehensive with well-defined user stories, acceptance scenarios, functional requirements, and success criteria. Edge cases are thoroughly documented. The static analysis hints about repeated "Given" statements are expected in BDD/Gherkin-style acceptance scenarios and don't require changes.
internal/mail/recipients.go (4)
254-319: LGTM!The signature change to return
(int, error)enables callers to track how many recipients were cleaned. The logic correctly calculates the removed count and only writes back when changes occur.Minor note: The
os.IsNotExistcheck at line 261-263 is effectively unreachable sinceO_CREATEflag ensures the file is created, but it's harmless defensive code.
404-448: LGTM!The function efficiently removes offline recipients using a map for O(1) window lookup. The early returns for empty recipients and zero removals avoid unnecessary I/O. Delegation to
WriteAllRecipientsproperly handles file locking.
450-476: LGTM!The dry-run counting function correctly mirrors the logic of
CleanOfflineRecipientswithout modifying any data.
478-496: LGTM!The dry-run counting function correctly implements the same staleness check logic as
CleanStaleStateswithout modifying data.internal/mail/mailbox.go (3)
51-70: LGTM!The polling-based implementation with 10ms sleep intervals is appropriate for the 1-second timeout use case. Non-blocking lock attempts with
LOCK_NBflag correctly avoid blocking the caller. The simplicity aligns with the stdlib-only constraint.
111-112: LGTM!Setting
CreatedAtbefore marshaling ensures all newly appended messages include a timestamp for age-based cleanup.
471-537: LGTM!The dry-run counting functions correctly mirror their respective cleanup functions' logic without modifying any data. They provide accurate counts for the
--dry-runmode.specs/011-cleanup/data-model.md (1)
1-166: LGTM!The data model documentation is comprehensive and accurately reflects the implementation. The backward compatibility approach using
omitempty, validation rules for thresholds, and state transition diagrams are well-documented.internal/cli/cleanup_test.go (5)
18-485: Offline-recipient cleanup tests look solid.Covers removal/retention paths, missing files, non-tmux behavior, and direct helper usage.
490-713: Stale-recipient cleanup tests are thorough.Good coverage for default and custom thresholds plus offline+stale interactions.
718-1017: Old-delivered-message cleanup tests are comprehensive.Read/unread, age thresholds, and CreatedAt edge cases are well covered.
1023-1265: Empty-mailbox cleanup tests look good.Covers deletion, retention, missing directory, and helper behavior.
1271-1531: Summary/dry-run output tests are well structured.Validates messaging and no-op behavior in dry-run.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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 | ||
| }, |
There was a problem hiding this comment.
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.
| 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.
| // 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 |
There was a problem hiding this comment.
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.
| // 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.
| // 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 | ||
| } |
There was a problem hiding this comment.
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.
| 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 | ||
| } |
There was a problem hiding this comment.
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.
| // Acquire exclusive lock for atomic read-modify-write | ||
| if err := syscall.Flock(int(file.Fd()), syscall.LOCK_EX); err != nil { | ||
| _ = file.Close() // G104: error intentionally ignored in cleanup path | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
Use TryLockWithTimeout instead of blocking LOCK_EX for non-blocking cleanup.
Per FR-017 in the spec, cleanup should skip files that cannot be locked within 1 second. This function uses a blocking syscall.Flock(LOCK_EX) which will wait indefinitely if the file is locked by another process.
🔧 Proposed fix
- // Acquire exclusive lock for atomic read-modify-write
- if err := syscall.Flock(int(file.Fd()), syscall.LOCK_EX); err != nil {
- _ = file.Close() // G104: error intentionally ignored in cleanup path
- return 0, err
- }
+ // Acquire exclusive lock for atomic read-modify-write with timeout (FR-017)
+ if err := TryLockWithTimeout(file, time.Second); err != nil {
+ _ = file.Close() // G104: error intentionally ignored in cleanup path
+ return 0, err
+ }🤖 Prompt for AI Agents
In `@internal/mail/mailbox.go` around lines 276 - 280, Replace the blocking
syscall.Flock(int(file.Fd()), syscall.LOCK_EX) call with a non-blocking
TryLockWithTimeout using a 1 second timeout (i.e. TryLockWithTimeout(file.Fd(),
time.Second)); if TryLockWithTimeout reports a timeout, close the file and treat
it as a skipped file (close file and return/propagate a skip result instead of
blocking), otherwise proceed as before and ensure you release the lock on
error/cleanup; update imports if needed and remove the direct
syscall.Flock/LOCK_EX usage.
| // If file has content, check if it has any messages | ||
| // (could be empty after message cleanup left just whitespace/empty lines) | ||
| messages, err := ReadAll(repoRoot, recipient) | ||
| if err != nil { | ||
| continue // Skip files we can't read | ||
| } | ||
|
|
||
| if len(messages) == 0 { | ||
| if err := os.Remove(filePath); err != nil && !os.IsNotExist(err) { | ||
| return removedCount, err | ||
| } | ||
| removedCount++ | ||
| } |
There was a problem hiding this comment.
Potential TOCTOU race condition when removing non-zero-size empty mailboxes.
There's a race between ReadAll (which releases its shared lock after reading) and os.Remove. Another process could append a message between these two operations, causing the newly written message to be lost.
Consider holding an exclusive lock while checking emptiness and removing the file:
🔧 Proposed fix sketch
- // If file has content, check if it has any messages
- // (could be empty after message cleanup left just whitespace/empty lines)
- messages, err := ReadAll(repoRoot, recipient)
- if err != nil {
- continue // Skip files we can't read
- }
-
- if len(messages) == 0 {
- if err := os.Remove(filePath); err != nil && !os.IsNotExist(err) {
- return removedCount, err
- }
- removedCount++
- }
+ // If file has content, open with lock to check if it has any messages
+ file, err := os.OpenFile(filePath, os.O_RDWR, 0600)
+ if err != nil {
+ continue
+ }
+ if err := TryLockWithTimeout(file, time.Second); err != nil {
+ _ = file.Close()
+ continue // Skip locked files
+ }
+ data, err := io.ReadAll(file)
+ if err != nil {
+ _ = syscall.Flock(int(file.Fd()), syscall.LOCK_UN)
+ _ = file.Close()
+ continue
+ }
+ // Check if file has actual messages (not just whitespace)
+ hasMessages := false
+ for _, line := range strings.Split(string(data), "\n") {
+ if line != "" {
+ hasMessages = true
+ break
+ }
+ }
+ if !hasMessages {
+ _ = file.Close() // Close before remove
+ if err := os.Remove(filePath); err != nil && !os.IsNotExist(err) {
+ return removedCount, err
+ }
+ removedCount++
+ } else {
+ _ = syscall.Flock(int(file.Fd()), syscall.LOCK_UN)
+ _ = file.Close()
+ }🤖 Prompt for AI Agents
In `@internal/mail/mailbox.go` around lines 453 - 465, The loop calls
ReadAll(repoRoot, recipient) which releases its shared lock before
os.Remove(filePath), creating a TOCTOU where another process could append a
message and be lost; fix by re-checking the mailbox under an exclusive lock
before removing: after ReadAll returns zero messages, open the mailbox file
(filePath) and acquire an exclusive lock (or upgrade the existing lock) on that
open descriptor, then re-validate that the file is still empty (e.g., stat or
read/seek to confirm size/has no new messages) and only then call os.Remove (or
unlink via the locked descriptor) and increment removedCount; ensure any file
opens/locks are properly closed/released and handle IsNotExist/other errors the
same way.
| **Phase 1 (Setup)**: | ||
| ``` | ||
| T001, T002, T003 can run in parallel (different files) | ||
| ``` | ||
|
|
||
| **Phase 2 (Foundational)**: | ||
| ``` | ||
| T004, T005 in parallel (same file - types only) | ||
| T006, T007 in parallel with T004/T005 (different file) | ||
| ``` |
There was a problem hiding this comment.
Fix fenced code block formatting to satisfy markdownlint.
Several fenced blocks are missing surrounding blank lines and a language tag (MD031/MD040).
Proposed fix example
-**Phase 1 (Setup)**:
-```
-T001, T002, T003 can run in parallel (different files)
-```
+**Phase 1 (Setup)**:
+
+```text
+T001, T002, T003 can run in parallel (different files)
+```Also applies to: 229-233, 235-238, 240-244
🤖 Prompt for AI Agents
In `@specs/011-cleanup/tasks.md` around lines 224 - 233, The fenced code blocks
under the headings like "Phase 1 (Setup)" and "Phase 2 (Foundational)" are
missing surrounding blank lines and a language tag; update each block (e.g., the
block containing "T001, T002, T003 can run in parallel (different files)" and
the blocks at 229-233, 235-238, 240-244) to add a blank line before the opening
fence and after the closing fence and add a language identifier (use "text" or
"text-plain") after the opening ``` so they become, for example, a blank line,
```text, the content line(s), then ``` and another blank line; ensure all
similar fenced blocks in this section follow the same pattern.
…eletion Previously, messages without a created_at timestamp were skipped during cleanup. This change treats read messages without timestamps as legacy messages that should be cleaned up. Behavior: - Read messages without timestamp: DELETED (legacy delivered messages) - Unread messages without timestamp: KEPT (unread never deleted) - Read messages with recent timestamp: KEPT - Read messages with old timestamp: DELETED
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/mail/mailbox.go`:
- Around line 51-70: In TryLockWithTimeout, don't treat every syscall.Flock
error as lock contention; inspect the returned error (from syscall.Flock) and if
it's not a transient lock contention (syscall.EAGAIN or syscall.EWOULDBLOCK)
return that error immediately (so real errors like EBADF/ENOLCK surface), while
only looping/retrying when the error equals EAGAIN/EWOULDBLOCK; after the
timeout, translate persistent EAGAIN/EWOULDBLOCK into ErrFileLocked. Ensure this
logic is applied both for the initial non-blocking call and for retries in the
retry loop.
♻️ Duplicate comments (3)
specs/011-cleanup/tasks.md (1)
224-244: Fix fenced code blocks to satisfy MD031/MD040 (blank lines + language tags).Same issue as previously noted: the fenced blocks under Phase 1/2/User Story Tests/Cross-Story Parallelism need blank lines and a language tag (e.g.,
text).internal/mail/mailbox.go (2)
276-279: UseTryLockWithTimeoutso cleanup can skip locked mailboxes.
CleanOldMessagesstill uses a blockingLOCK_EX, which can hang the cleanup command and preventsFilesSkippedaccounting. Switch toTryLockWithTimeoutand returnErrFileLockedto let callers skip.🔧 Proposed fix
// Acquire exclusive lock for atomic read-modify-write - if err := syscall.Flock(int(file.Fd()), syscall.LOCK_EX); err != nil { + if err := TryLockWithTimeout(file, time.Second); err != nil { _ = file.Close() // G104: error intentionally ignored in cleanup path return 0, err }
447-459: Avoid TOCTOU when deleting empty mailboxes.
ReadAllreleases its shared lock beforeos.Remove, allowing a new message to be appended between check and delete. Re-check emptiness under an exclusive lock before removing.🔧 Proposed fix sketch
- // If file has content, check if it has any messages - // (could be empty after message cleanup left just whitespace/empty lines) - messages, err := ReadAll(repoRoot, recipient) - if err != nil { - continue // Skip files we can't read - } - - if len(messages) == 0 { - if err := os.Remove(filePath); err != nil && !os.IsNotExist(err) { - return removedCount, err - } - removedCount++ - } + // Re-check under exclusive lock to avoid TOCTOU + file, err := os.OpenFile(filePath, os.O_RDWR, 0600) + if err != nil { + continue + } + if err := TryLockWithTimeout(file, time.Second); err != nil { + _ = file.Close() + continue // Skip locked files + } + data, err := io.ReadAll(file) + if err != nil { + _ = syscall.Flock(int(file.Fd()), syscall.LOCK_UN) + _ = file.Close() + continue + } + hasMessages := false + for _, line := range strings.Split(string(data), "\n") { + if line != "" { + hasMessages = true + break + } + } + _ = syscall.Flock(int(file.Fd()), syscall.LOCK_UN) + _ = file.Close() + if !hasMessages { + if err := os.Remove(filePath); err != nil && !os.IsNotExist(err) { + return removedCount, err + } + removedCount++ + }Based on learnings, mailbox operations must use file locking for atomic operations.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
internal/cli/cleanup_test.gointernal/mail/mailbox.gospecs/011-cleanup/tasks.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: All code must passgo test -v -race ./...without errors
All code must passgo vet ./...static analysis without errors
All code must passgo fmt ./...formatting without errors
All code must passgovulncheck ./...vulnerability scanning without errors
All code must passgosec ./...security analysis without errors
Use Go 1.21+ (project uses Go 1.25.5) with Standard library only - no external dependencies except where explicitly justified
Message storage must use JSONL format in.agentmail/mailboxes/<recipient>.jsonlfiles with file locking for atomic operations
AgentMail CLI must check that it is running inside a tmux session and use tmux window names as valid recipients
Files:
internal/cli/cleanup_test.gointernal/mail/mailbox.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : Message storage must use JSONL format in `.agentmail/mailboxes/<recipient>.jsonl` files with file locking for atomic operations
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : AgentMail CLI must check that it is running inside a tmux session and use tmux window names as valid recipients
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Build system must produce agentmail binary
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : AgentMail CLI must check that it is running inside a tmux session and use tmux window names as valid recipients
Applied to files:
internal/cli/cleanup_test.gointernal/mail/mailbox.go
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : Message storage must use JSONL format in `.agentmail/mailboxes/<recipient>.jsonl` files with file locking for atomic operations
Applied to files:
internal/cli/cleanup_test.gointernal/mail/mailbox.go
🧬 Code graph analysis (2)
internal/cli/cleanup_test.go (3)
internal/mail/recipients.go (7)
RecipientState(30-36)StatusReady(17-17)StatusWork(18-18)WriteAllRecipients(108-136)ReadAllRecipients(54-80)StatusOffline(19-19)CleanOfflineRecipients(408-448)internal/cli/cleanup.go (2)
Cleanup(69-221)CleanupOptions(38-48)internal/mail/mailbox.go (3)
WriteAll(221-253)ReadAll(132-176)RemoveEmptyMailboxes(412-463)
internal/mail/mailbox.go (2)
internal/mail/message.go (1)
Message(11-18)internal/mail/recipients.go (1)
ListMailboxRecipients(223-250)
🪛 LanguageTool
specs/011-cleanup/tasks.md
[style] ~190-~190: Consider using a different verb for a more formal wording.
Context: ... code - [X] T050 Run go vet ./... and fix any issues - [X] T051 Run `go fmt ./......
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
specs/011-cleanup/tasks.md
225-225: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
225-225: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
230-230: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
230-230: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
236-236: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
236-236: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
241-241: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
241-241: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
internal/mail/mailbox.go (1)
111-112: Good: timestamping new messages enables age-based cleanup.Stamping
CreatedAton append ensures new messages are eligible for cleanup logic.internal/cli/cleanup_test.go (1)
1-1545: Test coverage is comprehensive and well-structured.Covers offline/stale removal, message cleanup, empty mailbox cleanup, dry-run behavior, and output formatting with good scenario coverage.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Update constitution Quality Gates to match CI exactly (7 checks) - Add go mod verify, govulncheck, gosec to quality gates - Update Go version reference to 1.25.5 (CI version) - Update CLAUDE.md docker example to golang:1.25.5 - Bump constitution version to 1.2.0
Resolve conflicts in constitution.md: - Keep condensed sync impact report format from main - Use main's clearer language constraint wording - Keep HEAD's quality gates that match CI pipeline exactly - Retain most recent amendment date
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- 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.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.specify/memory/constitution.mdCLAUDE.md
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : AgentMail CLI must check that it is running inside a tmux session and use tmux window names as valid recipients
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : Message storage must use JSONL format in `.agentmail/mailboxes/<recipient>.jsonl` files with file locking for atomic operations
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Build system must produce agentmail binary
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : Use Go 1.21+ (project uses Go 1.25.5) with Standard library only - no external dependencies except where explicitly justified
Applied to files:
CLAUDE.md.specify/memory/constitution.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : All code must pass `govulncheck ./...` vulnerability scanning without errors
Applied to files:
CLAUDE.md.specify/memory/constitution.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : All code must pass `gosec ./...` security analysis without errors
Applied to files:
CLAUDE.md.specify/memory/constitution.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : All code must pass `go vet ./...` static analysis without errors
Applied to files:
CLAUDE.md.specify/memory/constitution.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : All code must pass `go test -v -race ./...` without errors
Applied to files:
CLAUDE.md.specify/memory/constitution.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : AgentMail CLI must check that it is running inside a tmux session and use tmux window names as valid recipients
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Build system must produce agentmail binary
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : All code must pass `go fmt ./...` formatting without errors
Applied to files:
CLAUDE.md.specify/memory/constitution.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Applies to **/*.go : Message storage must use JSONL format in `.agentmail/mailboxes/<recipient>.jsonl` files with file locking for atomic operations
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-15T18:50:18.457Z
Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T18:50:18.457Z
Learning: Project constitution and architectural decisions should be documented in `.specify/memory/constitution.md`
Applied to files:
.specify/memory/constitution.md
🔇 Additional comments (3)
CLAUDE.md (1)
46-56: LGTM! Quality gates restructuring improves clarity.The consolidation into a numbered CI checklist with explicit coverage requirements (80%) makes the quality gates clearer and easier to reference.
.specify/memory/constitution.md (2)
82-89: Excellent formalization of quality gates!The changes strengthen the project's quality assurance:
- CI alignment note (line 82) ensures documentation matches automation.
- Formatting gate (line 84):
gofmt -l .is the correct check-only command (vs.go fmtwhich modifies files).- Dependencies gate (line 85): Adding
go mod verifyimproves supply chain integrity.- Test command specificity (line 87): Including
-coverprofile=coverage.outensures consistent coverage measurement across local and CI environments.- Security gates (lines 88-89):
govulncheckandgosecadditions align with project learnings and modern security practices.The reordering (Dependencies before Static Analysis) is logical, and all gates remain properly numbered.
Based on learnings, these gates were already expected (
gosec,govulncheck,go vet,go test -v -race) and are now properly formalized.
107-107: Metadata update is appropriate.The Last Amended date update to 2026-01-17 correctly documents this amendment. Version remaining at 1.2.0 is correct per the amendment process rules, as these are clarifications of existing practices rather than new principles.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| - 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) |
There was a problem hiding this comment.
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.
| - 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.
TryLockWithTimeout now inspects syscall.Flock errors and only retries on transient lock contention (EAGAIN/EWOULDBLOCK). Real errors like EBADF or ENOLCK are returned immediately, preventing them from being masked by the timeout loop.
1 participant
Summary
agentmail cleanupcommand that removes stale data from the AgentMail system--dry-runmode to preview changes without making them--stale-hoursand--delivered-hoursflags for custom thresholdsFeatures
--stale-hours--delivered-hours--dry-runUsage
Implementation
CreatedAttimestamp to Message structTest plan
go test -v -race ./...)go vet ./...)gosec ./...)govulncheck ./...)Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.