Conversation
Run gocyclo on the codebase and document findings with recommendations: - 5 functions exceed complexity 20 (critical threshold) - Main issues: mock infrastructure, hook mode branching, two-phase processing - Recommendations include extracting helpers, interface-based testing, and splitting large functions Top offenders: - cli.Send (30): Multiple validation + mock support - cli.Receive (25): Hook mode conditional branches - daemon.CheckAndNotifyWithNotifier (24): Two-phase agent processing - mcp.doSend (21): Duplicates cli.Send validation
Extract helper methods and split large functions to reduce complexity: cli.Send: 30 → 15 (50% reduction) - Extract extractMessage, getSender, windowExists, loadIgnoreList, getRepoRoot cli.Receive: 25 → 9 (64% reduction) - Extract receiveCore with error struct - Add outputMessage helper - Consolidate hook mode handling daemon.CheckAndNotifyWithNotifier: 24 → 2 (92% reduction) - Split into notifyStatedAgents and notifyStatelessAgents - Extract processStatedAgent and processStatelessAgent mcp handlers: doSend 21 → 13, doListRecipients 13 → 8 - Add helper methods to HandlerOptions - Reuse patterns from CLI refactoring All tests pass. Only main.main (18) remains above 15, which is acceptable for CLI setup code.
📝 WalkthroughWalkthroughRefactors CLI, MCP handlers, daemon notification loop, and mail recipient logic by extracting helper methods, modularizing control flow, and adding mockable abstraction points; centralizes environment interactions and splits notification into stated and stateless phases for clearer, testable flows. (45 words) Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/mcp/handlers.go (1)
266-302: MissingUpdateLastReadAtcall in MCP receive handler.The CLI
receive.go(line 99) updates thelast_read_attimestamp after marking a message as read, but this MCP handler does not. This inconsistency could affect daemon notification logic that relies on this timestamp.Consider adding the same timestamp update:
♻️ Proposed fix
+ "time" + "agentmail/internal/mail"if err := mail.MarkAsRead(repoRoot, receiver, msg.ID); err != nil { return nil, fmt.Errorf("failed to mark message as read: %w", err) } + // Update last_read_at timestamp (best-effort) + _ = mail.UpdateLastReadAt(repoRoot, receiver, time.Now().UnixMilli()) + return ReceiveResponse{From: msg.From, ID: msg.ID, Message: msg.Message}, nil
🤖 Fix all issues with AI agents
In `@internal/cli/receive.go`:
- Around line 97-100: The code uses opts.SkipTmuxCheck to gate the
mail.UpdateLastReadAt(repoRoot, receiver, time.Now().UnixMilli()) call,
overloading SkipTmuxCheck's meaning; add a dedicated boolean option (e.g.,
SkipTimestampUpdate) to the options struct and use that flag instead of
SkipTmuxCheck when deciding whether to call mail.UpdateLastReadAt, or
alternatively rename SkipTmuxCheck to a more general flag like TestMode if
broader semantics are intended; update the receive logic to check the new option
(SkipTimestampUpdate) and add a short comment explaining the rationale if the
behavior is intentionally used for testing.
In `@internal/cli/send.go`:
- Around line 148-151: Split the combined recipient check in send.go into two
explicit checks to match the MCP handler: first if recipientExists is false,
print "error: recipient not found" and return 1; otherwise if recipient ==
sender, print "error: cannot send message to self" and return 1. Update the
condition involving recipientExists, recipient and sender (used in the send CLI
logic) so each case has its own message, mirroring the MCP handler's wording.
In `@internal/daemon/loop.go`:
- Around line 156-166: The logging conflates debounce timing with a separate "1h
protection" when checking recipient.ShouldNotify() inside processStatedAgent;
add a clear distinction by introducing a new predicate on mail.RecipientState
(e.g., IsProtected or ProtectedUntil) and use that in the non-ready branch so
processStatedAgent logs "protected for 1h" only when the new predicate is true,
leaving ShouldNotify() to represent the 60s debounce; update the log messages
and any callers of ShouldNotify() as needed (or, if you prefer minimal change,
add a concise comment above the ShouldNotify() check in processStatedAgent
explaining that ShouldNotify() is the 60s debounce and that protection is a
separate concept to be checked via IsProtected).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
internal/cli/receive.gointernal/cli/send.gointernal/daemon/loop.gointernal/mcp/handlers.go
🧰 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/receive.gointernal/mcp/handlers.gointernal/daemon/loop.gointernal/cli/send.go
🧠 Learnings (1)
📚 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/receive.gointernal/mcp/handlers.gointernal/daemon/loop.gointernal/cli/send.go
🧬 Code graph analysis (3)
internal/cli/receive.go (5)
internal/tmux/tmux.go (2)
GetCurrentWindow(55-68)WindowExists(97-110)internal/mail/ignore.go (1)
FindGitRoot(13-28)internal/mail/message.go (1)
Message(10-16)internal/mail/mailbox.go (1)
FindUnread(151-165)internal/mail/recipients.go (1)
UpdateLastReadAt(403-478)
internal/daemon/loop.go (2)
internal/mail/recipients.go (4)
ReadAllRecipients(54-80)RecipientState(30-36)StatusReady(17-17)ListMailboxRecipients(223-250)internal/mail/mailbox.go (1)
FindUnread(151-165)
internal/cli/send.go (5)
internal/cli/stdin.go (1)
IsStdinPipe(10-16)internal/mail/mailbox.go (1)
ReadAll(104-148)internal/tmux/tmux.go (3)
GetCurrentWindow(55-68)WindowExists(97-110)InTmux(25-28)internal/mail/ignore.go (2)
FindGitRoot(13-28)LoadIgnoreList(33-53)internal/mail/message.go (1)
GenerateID(23-37)
🔇 Additional comments (19)
internal/cli/receive.go (5)
22-26: LGTM on the error wrapper structure.The
receiveErrorstruct provides a clean way to bundle error messages with exit codes for the receive command flow.
28-47: LGTM on helper methods for mocking.The
getReceiverandwindowExistshelpers correctly delegate to mocks when configured, otherwise fall back to real tmux calls. This pattern enables clean unit testing.
49-55: LGTM on getRepoRoot helper.Correctly returns the configured
RepoRootor falls back tomail.FindGitRoot().
105-114: LGTM on outputMessage helper.Clean helper for consistent message formatting across hook and normal modes.
116-157: LGTM on the refactored Receive function.The separation of concerns is well done:
- tmux validation at the top
- Core logic delegated to
receiveCore- Clear branching based on hook mode vs normal mode
- Consistent exit codes and output streams
The behavior matches the documented contract in the comments.
internal/cli/send.go (5)
25-58: LGTM on extractMessage helper.The logic correctly prioritizes stdin content over argument-based messages, with proper handling of the pipe detection and mock content for testing.
60-79: LGTM on getSender and windowExists helpers.These follow the same mock/real delegation pattern as in
receive.go, maintaining consistency across the codebase.
81-97: LGTM on loadIgnoreList helper.The silent error handling is consistent with the existing
mail.LoadIgnoreListbehavior (per FR-016), where missing or inaccessible ignore files are treated as "no ignore list."
99-105: LGTM on getRepoRoot helper.Follows the same pattern as
receive.go.
166-187: LGTM on message generation and storage.The flow correctly generates an ID, constructs the message, stores it, and outputs the confirmation.
internal/daemon/loop.go (5)
133-154: LGTM on notifyStatedAgents.Clean separation of Phase 1 logic. The function correctly:
- Reads all recipients
- Builds the stated set for Phase 2 exclusion
- Delegates per-agent processing to
processStatedAgent
168-203: LGTM on the stated agent notification flow.The debounce check, unread message verification, notification dispatch, and flag update are correctly ordered and well-logged.
205-233: LGTM on notifyStatelessAgents.The Phase 2 logic correctly:
- Skips processing if tracker is nil
- Excludes stated agents from processing
- Cleans up stale tracker entries after processing
263-267: Verify intent: marking notified for non-existent windows.When a window doesn't exist (line 263-266), the code still calls
MarkNotified. This prevents retry spam within the interval, but the agent will be re-checked on every cycle since the mailbox file still exists.If this is the intended behavior (eventually notify when the window reappears), this is fine. Otherwise, consider adding a mechanism to track "window not found" state separately to avoid repeated window existence checks.
286-305: LGTM on CheckAndNotifyWithNotifier.Clean orchestration of the two-phase notification cycle with appropriate logging.
internal/mcp/handlers.go (4)
31-90: LGTM on MCP helper methods.The helpers follow the same mock/real delegation pattern as the CLI, enabling consistent testability across both interfaces.
148-214: LGTM on doSend handler logic.The validation flow is correct with proper separation of concerns:
- Message validation
- Sender identity
- Recipient existence
- Self-send prevention
- Ignore list check
- Repository context
- Message storage
The self-send error message ("cannot send message to self") is more descriptive than the CLI version.
348-380: LGTM on doStatus handler logic.The status validation, identity retrieval, and state update flow are correct.
431-464: LGTM on doListRecipients handler logic.The handler correctly:
- Gets the current window
- Lists all windows
- Filters out ignored windows (except the current window)
- Marks the current window appropriately
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Add IsProtected() method to RecipientState for the 1h protection period (work/offline status), keeping ShouldNotify() focused on the 60s debounce interval. This clarifies the distinction in processStatedAgent: - IsProtected(): checks if work/offline agent is within 1h window - ShouldNotify(): checks if 60s debounce has elapsed (ready agents)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/daemon/loop.go`:
- Around line 263-267: The current branch marks an agent as notified when the
window does not exist but logs "Skipping stateless agent..." which can be
misleading; update the log message in the block that checks !exists (the
opts.log call) to explicitly state that no window was found and the agent is
being marked as notified to avoid repeated checks, and add a brief inline
comment above the opts.StatelessTracker.MarkNotified(agent) call explaining the
rationale (i.e., treat missing window as implicit notification to stop polling).
Reference: the exists boolean, agent identifier, opts.log, and
opts.StatelessTracker.MarkNotified.
- Around line 229-232: StatelessTracker.Cleanup is being called with
mailboxRecipients (filesystem-derived) but expects active tmux windows; change
the call in loop.go to pass the actual tmux active windows instead of
mailboxRecipients—query tmux for current windows (e.g., via the existing tmux
querying helper or add a helper that returns activeWindows) and call
StatelessTracker.Cleanup(activeWindows); alternatively, if you intend cleanup to
be based on mailbox files, update the Cleanup signature/name and its docs to
reflect mailboxRecipients semantics (but prefer passing activeWindows to
preserve correct behavior).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
internal/daemon/loop.gointernal/mail/recipients.go
🧰 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/mail/recipients.gointernal/daemon/loop.go
🧠 Learnings (1)
📚 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/mail/recipients.gointernal/daemon/loop.go
🧬 Code graph analysis (1)
internal/daemon/loop.go (2)
internal/mail/recipients.go (5)
ReadAllRecipients(57-83)RecipientState(30-36)StatusReady(17-17)SetNotifiedFlag(395-401)ListMailboxRecipients(226-253)internal/mail/mailbox.go (1)
FindUnread(151-165)
🔇 Additional comments (5)
internal/mail/recipients.go (2)
38-45: LGTM! Clean extraction of protection logic.The
IsProtected()method correctly encapsulates the work/offline protection check. The logic properly:
- Returns
falsefor non-work/offline statuses (line 41-43)- Checks if the agent is within the 1-hour protection window (line 44)
This addresses the previous review comment about clarifying the dual use of
ShouldNotifyby separating the protection concept into its own method.
47-54: Documentation update aligns with new usage pattern.The updated comment correctly clarifies that
ShouldNotify()is specifically for the 60-second debounce check on ready agents, withIsProtected()handling the separate protection concern for work/offline states.internal/daemon/loop.go (3)
133-154: LGTM! Clean extraction of Phase 1 logic.The function correctly:
- Reads recipients and handles errors appropriately
- Builds the stated set for Phase 2 exclusion
- Delegates per-agent processing to the helper function
156-203: Well-structured per-agent logic with proper separation of concerns.The function correctly implements the refined notification logic:
- Uses
IsProtected()for non-ready agents (lines 160-164) - addresses the previous review feedback- Uses
ShouldNotify()only for the 60s debounce on ready agents (line 169)- Properly short-circuits on errors and empty mailboxes
The notification flag update (line 198) correctly runs regardless of whether
notifyis nil, supporting test mode where actual notifications are skipped.
286-305: LGTM! Clean orchestration with clear phase separation.The function correctly:
- Propagates Phase 1 errors (critical path - reading recipient state)
- Allows Phase 2 to proceed independently (best-effort for stateless agents)
- Documents the two-phase approach clearly in comments
The asymmetric error handling between phases is appropriate given their different criticality levels.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| opts.log("Found %d stateless agents", statelessCount) | ||
|
|
||
| // T021: Check for unread messages (FR-006) | ||
| unread, err := mail.FindUnread(opts.RepoRoot, mailboxRecipient) | ||
| opts.StatelessTracker.Cleanup(mailboxRecipients) | ||
| opts.log("Cleaned up stale entries from stateless tracker") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the StatelessTracker.Cleanup method (lines 59-76)
sed -n '59,76p' internal/daemon/loop.goRepository: UserAd/AgentMail
Length of output: 574
🏁 Script executed:
# Also check the full notifyStatelessAgents function to understand the context
sed -n '205,233p' internal/daemon/loop.goRepository: UserAd/AgentMail
Length of output: 1011
🏁 Script executed:
# Search for the definition of StatelessTracker and Cleanup method
rg -n -B2 -A10 "func.*Cleanup" internal/daemon/Repository: UserAd/AgentMail
Length of output: 1663
🏁 Script executed:
# Check what mailboxRecipients contains and how it's obtained
rg -n "mailboxRecipients" internal/daemon/loop.goRepository: UserAd/AgentMail
Length of output: 333
🏁 Script executed:
# Look for window-related operations and active window tracking
rg -n "window" internal/daemon/loop.go -iRepository: UserAd/AgentMail
Length of output: 2387
Incorrect parameter type: Cleanup should receive active tmux windows, not mailbox recipients.
The StatelessTracker.Cleanup() method (line 60) expects a list of active tmux windows to keep entries for. However, line 231 passes mailboxRecipients (agents with mailbox files), which are not equivalent:
- If a tmux window closes but its mailbox still exists, the tracker entry persists indefinitely
- If a mailbox is deleted but the window exists, the tracker entry is incorrectly removed
Since mailboxRecipients are from the filesystem and activeWindows should come from tmux, these semantics are mismatched. The current code should either:
- Query tmux for all active windows and pass that list to
Cleanup(), or - Update the
Cleanup()documentation and parameter name to reflect that mailbox recipients are the actual cleanup basis
🤖 Prompt for AI Agents
In `@internal/daemon/loop.go` around lines 229 - 232, StatelessTracker.Cleanup is
being called with mailboxRecipients (filesystem-derived) but expects active tmux
windows; change the call in loop.go to pass the actual tmux active windows
instead of mailboxRecipients—query tmux for current windows (e.g., via the
existing tmux querying helper or add a helper that returns activeWindows) and
call StatelessTracker.Cleanup(activeWindows); alternatively, if you intend
cleanup to be based on mailbox files, update the Cleanup signature/name and its
docs to reflect mailboxRecipients semantics (but prefer passing activeWindows to
preserve correct behavior).
| if !exists { | ||
| opts.log("Skipping stateless agent %q: window does not exist", agent) | ||
| opts.StatelessTracker.MarkNotified(agent) | ||
| return | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify intent: Marking as notified when window doesn't exist.
When a window doesn't exist (lines 263-267), the agent is marked as notified. This prevents repeated window existence checks, which is efficient behavior. However, the log message and semantic intent could be clearer since no actual notification occurred.
Consider updating the log message for clarity:
📝 Suggested improvement
if !exists {
opts.log("Skipping stateless agent %q: window does not exist", agent)
opts.StatelessTracker.MarkNotified(agent)
+ opts.log("Suppressed stateless agent %q for interval (window not found)", agent)
return
}Alternatively, a brief inline comment explaining the rationale would suffice.
🤖 Prompt for AI Agents
In `@internal/daemon/loop.go` around lines 263 - 267, The current branch marks an
agent as notified when the window does not exist but logs "Skipping stateless
agent..." which can be misleading; update the log message in the block that
checks !exists (the opts.log call) to explicitly state that no window was found
and the agent is being marked as notified to avoid repeated checks, and add a
brief inline comment above the opts.StatelessTracker.MarkNotified(agent) call
explaining the rationale (i.e., treat missing window as implicit notification to
stop polling). Reference: the exists boolean, agent identifier, opts.log, and
opts.StatelessTracker.MarkNotified.
Split the combined recipient check into two explicit checks: - "recipient not found" for non-existent recipients - "cannot send message to self" for self-send attempts This mirrors the MCP handler's error messages for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/cli/send.go`:
- Around line 81-97: The loadIgnoreList method currently discards errors from
mail.FindGitRoot() and mail.LoadIgnoreList(); modify SendOptions.loadIgnoreList
to capture those errors, treat "not found/missing file" as a non-fatal case
(return nil), but log or surface unexpected errors (e.g., permission/I/O)
instead of swallowing them. Locate the method loadIgnoreList on type SendOptions
and update the calls to mail.FindGitRoot() and mail.LoadIgnoreList(gitRoot) to
check returned errors and either (a) log them via an existing logger on
SendOptions (e.g., opts.Logger) or (b) return/propagate the error up to the
caller so callers of loadIgnoreList can handle/report it, ensuring only benign
"missing file" cases remain silent.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
internal/cli/send.gointernal/cli/send_test.go
🧰 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/send.gointernal/cli/send_test.go
🧠 Learnings (2)
📚 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/send.gointernal/cli/send_test.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/send.go
🧬 Code graph analysis (1)
internal/cli/send.go (4)
internal/cli/stdin.go (1)
IsStdinPipe(10-16)internal/mail/mailbox.go (1)
ReadAll(104-148)internal/tmux/tmux.go (3)
GetCurrentWindow(55-68)WindowExists(97-110)InTmux(25-28)internal/mail/ignore.go (2)
FindGitRoot(13-28)LoadIgnoreList(33-53)
🔇 Additional comments (8)
internal/cli/send.go (7)
25-58: LGTM - Message extraction logic is well-structured.The
extractMessagemethod properly handles stdin precedence over arguments (per FR-010), correctly trims trailing newlines, and propagates errors appropriately.
60-66: LGTM - Simple delegation with mock support.The
getSenderhelper cleanly abstracts tmux interaction for testability.
68-79: LGTM - Window existence check properly abstracted.The
windowExistshelper correctly iterates through mock windows when provided and delegates totmux.WindowExistsotherwise.
99-105: LGTM - Repository root resolution is straightforward.The
getRepoRoothelper properly returns the configured root or delegates tomail.FindGitRoot().
107-133: LGTM - Entry point validation is thorough.The Send function properly validates tmux environment, recipient argument presence, and message extraction with appropriate error messages and exit codes.
148-163: Correct separation of recipient validation and self-send check.The self-send check (lines 153-157) now correctly reports "error: cannot send message to self" separately from the "recipient not found" error (lines 148-151), aligning with the MCP handler behavior. This addresses the previous review feedback.
Note: The ignore list check (lines 159-163) intentionally reports "recipient not found" to avoid information disclosure about the ignore list contents.
165-194: LGTM - Message storage and success path are correct.The repository root resolution, message ID generation, and mail storage via
mail.Appendfollow the expected patterns. The success message format is consistent.internal/cli/send_test.go (1)
255-257: LGTM - Test expectation correctly updated.The expected error message now matches the implementation change in
send.go, which separates the self-send error from the recipient-not-found error for consistency with the MCP handler.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // loadIgnoreList loads the ignore list from file or returns mock. | ||
| func (opts *SendOptions) loadIgnoreList() map[string]bool { | ||
| if opts.MockIgnoreList != nil { | ||
| return opts.MockIgnoreList | ||
| } | ||
|
|
||
| gitRoot := opts.MockGitRoot | ||
| if gitRoot == "" { | ||
| gitRoot, _ = mail.FindGitRoot() | ||
| } | ||
| if gitRoot == "" { | ||
| return nil | ||
| } | ||
|
|
||
| ignoreList, _ := mail.LoadIgnoreList(gitRoot) | ||
| return ignoreList | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silently swallowed errors may hide configuration issues.
Both mail.FindGitRoot() (line 89) and mail.LoadIgnoreList() (line 95) errors are discarded. While returning nil is acceptable for missing files (per FR-016), other errors (e.g., permission issues on directories, I/O errors) are silently ignored, which could mask real problems.
Consider logging or returning errors for unexpected failures:
♻️ Proposed fix
func (opts *SendOptions) loadIgnoreList() map[string]bool {
if opts.MockIgnoreList != nil {
return opts.MockIgnoreList
}
gitRoot := opts.MockGitRoot
if gitRoot == "" {
- gitRoot, _ = mail.FindGitRoot()
+ var err error
+ gitRoot, err = mail.FindGitRoot()
+ if err != nil {
+ // Not in git repo - ignore list not applicable
+ return nil
+ }
}
if gitRoot == "" {
return nil
}
- ignoreList, _ := mail.LoadIgnoreList(gitRoot)
+ ignoreList, err := mail.LoadIgnoreList(gitRoot)
+ if err != nil {
+ // LoadIgnoreList returns nil,nil for missing/permission errors per FR-016
+ // Any other error is unexpected but non-fatal for send operation
+ return nil
+ }
return ignoreList
}🤖 Prompt for AI Agents
In `@internal/cli/send.go` around lines 81 - 97, The loadIgnoreList method
currently discards errors from mail.FindGitRoot() and mail.LoadIgnoreList();
modify SendOptions.loadIgnoreList to capture those errors, treat "not
found/missing file" as a non-fatal case (return nil), but log or surface
unexpected errors (e.g., permission/I/O) instead of swallowing them. Locate the
method loadIgnoreList on type SendOptions and update the calls to
mail.FindGitRoot() and mail.LoadIgnoreList(gitRoot) to check returned errors and
either (a) log them via an existing logger on SendOptions (e.g., opts.Logger) or
(b) return/propagate the error up to the caller so callers of loadIgnoreList can
handle/report it, ensuring only benign "missing file" cases remain silent.
Separates timestamp update control from tmux check to avoid overloading SkipTmuxCheck with unrelated behavior. The new SkipTimestampUpdate flag explicitly controls whether UpdateLastReadAt is called, providing clearer test isolation without coupling to tmux-related mocking.
Apply go fmt to fix struct field alignment after adding SkipTimestampUpdate field.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/cli/receive.go`:
- Around line 106-115: The outputMessage function writes the message body with
fmt.Fprint which may leave no trailing newline; update outputMessage (the
function named outputMessage) to ensure a newline after the message body by
using fmt.Fprintln(w, msg.Message) or otherwise appending "\n" to msg.Message so
the shell prompt appears on the next line when the message lacks a newline.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
internal/cli/receive.gointernal/cli/receive_test.go
🧰 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/receive_test.gointernal/cli/receive.go
🧠 Learnings (1)
📚 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/receive_test.gointernal/cli/receive.go
🧬 Code graph analysis (2)
internal/cli/receive_test.go (3)
internal/cli/send.go (1)
SkipTmuxCheck(14-23)internal/cli/recipients.go (1)
SkipTmuxCheck(12-18)internal/cli/status.go (1)
SkipTmuxCheck(13-17)
internal/cli/receive.go (5)
internal/tmux/tmux.go (3)
GetCurrentWindow(55-68)WindowExists(97-110)InTmux(25-28)internal/mail/ignore.go (1)
FindGitRoot(13-28)internal/mail/message.go (1)
Message(10-16)internal/mail/mailbox.go (2)
FindUnread(151-165)MarkAsRead(229-295)internal/mail/recipients.go (1)
UpdateLastReadAt(406-481)
🔇 Additional comments (6)
internal/cli/receive_test.go (1)
28-34: LGTM! Test updates correctly use the newSkipTimestampUpdateflag.The addition of
SkipTimestampUpdate: trueacross all test cases properly isolates tests from file system side effects related tolast_read_attimestamp updates. This addresses the previous review feedback about overloadingSkipTmuxChecksemantics.The test coverage is comprehensive, including:
- Normal and hook mode behaviors
- FIFO message ordering
- Edge cases (no messages, all read, window not found, not in tmux)
Also applies to: 68-74, 110-116, 170-176, 221-227, 262-269, 324-331, 387-394, 432-439, 474-481
internal/cli/receive.go (5)
14-21: LGTM! Well-structured options with dedicated testing flags.The
ReceiveOptionsstruct provides clean separation of concerns:
SkipTmuxCheckfor bypassing environment validationSkipTimestampUpdatefor isolating tests from timestamp side effects- Mock fields for dependency injection in tests
This addresses the previous review feedback and aligns with similar option structs in
send.go,status.go, andrecipients.go.
23-27: Clean internal error representation.The
receiveErrortype effectively couples error messages with exit codes, enablingreceiveCoreto communicate failure context without knowing about hook vs normal mode distinctions.
29-56: Well-designed mockable helpers.The helper methods (
getReceiver,windowExists,getRepoRoot) follow a consistent pattern: return mock values when provided, otherwise delegate to real implementations. This enables comprehensive unit testing without tmux or filesystem dependencies.
58-104: Core logic is well-structured with clear flow.The
receiveCoremethod cleanly separates concerns:
- Identity resolution (receiver window)
- Validation (window exists in session)
- Message retrieval (FIFO via
unread[0])- Side effects (mark as read, update timestamp)
The
(nil, nil)return for "no messages" is a clean sentinel pattern that distinguishes "no messages" from "error occurred."The best-effort semantics for
UpdateLastReadAt(line 100) are acceptable since timestamp tracking is auxiliary to the core receive functionality.
117-159: LGTM! Clean implementation with proper mode-based behavior.The
Receivefunction correctly implements the specified behaviors:
- Hook mode: Silent on errors/no messages (exit 0), output to stderr with "You got new mail" prefix (exit 2) when messages exist
- Normal mode: Verbose errors to stderr, output to stdout, appropriate exit codes
The delegation to
receiveCorekeeps the main function focused on presentation and mode-specific behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // outputMessage writes the message to the given writer. | ||
| func outputMessage(w io.Writer, msg *mail.Message, prefix string) { | ||
| if prefix != "" { | ||
| fmt.Fprintln(w, prefix) | ||
| } | ||
| fmt.Fprintf(w, "From: %s\n", msg.From) | ||
| fmt.Fprintf(w, "ID: %s\n", msg.ID) | ||
| fmt.Fprintln(w) | ||
| fmt.Fprint(w, msg.Message) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a trailing newline after message body.
Using fmt.Fprint on line 114 means no newline is appended after the message content. If the message itself doesn't end with a newline, the shell prompt will appear immediately after the message body, which may cause display issues.
🔧 Suggested fix
func outputMessage(w io.Writer, msg *mail.Message, prefix string) {
if prefix != "" {
fmt.Fprintln(w, prefix)
}
fmt.Fprintf(w, "From: %s\n", msg.From)
fmt.Fprintf(w, "ID: %s\n", msg.ID)
fmt.Fprintln(w)
- fmt.Fprint(w, msg.Message)
+ fmt.Fprintln(w, msg.Message)
}🤖 Prompt for AI Agents
In `@internal/cli/receive.go` around lines 106 - 115, The outputMessage function
writes the message body with fmt.Fprint which may leave no trailing newline;
update outputMessage (the function named outputMessage) to ensure a newline
after the message body by using fmt.Fprintln(w, msg.Message) or otherwise
appending "\n" to msg.Message so the shell prompt appears on the next line when
the message lacks a newline.
2 participants
Summary by CodeRabbit
Bug Fixes
Behavior
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.