feat: Add MCP server for AI agent integration

[UserAd]

Summary

• Add MCP (Model Context Protocol) server enabling AI agents to communicate via standardized STDIO transport
• Implement 4 tools: send, receive, status, list-recipients
• Support Claude Code, OpenAI Codex CLI, and Google Gemini CLI

Changes

New MCP Package (internal/mcp/)

• server.go - MCP server with STDIO transport, tmux validation, and 1-second exit on context loss
• tools.go - Tool definitions with JSON schemas
• handlers.go - Handler implementations wrapping existing CLI logic
• Comprehensive test coverage (81.2%)

CLI Updates

• Add agentmail mcp subcommand to start MCP server

Documentation

• README.md with configuration for Claude Code, Codex CLI, and Gemini CLI
• Complete specs in specs/010-mcp-server/

Tool Specifications

Tool	Description
send	Send message to another agent (64KB max)
receive	Receive oldest unread message (FIFO)
status	Set availability (ready/work/offline)
list-recipients	List available agents

Configuration Examples

Claude Code (~/.claude/settings.json):

{"mcpServers": {"agentmail": {"command": "agentmail", "args": ["mcp"]}}}

Codex CLI (~/.codex/config.toml):

[mcp_servers.agentmail]
command = "agentmail"
args = ["mcp"]

Gemini CLI (~/.gemini/settings.json):

{"mcpServers": {"agentmail": {"command": "agentmail", "args": ["mcp"]}}}

Test plan

• All 200+ tests pass with race detection
• MCP package coverage > 80% (81.2%)
• go vet and go fmt pass
• gosec passes (0 issues)
• Build produces working binary
• Manual test: agentmail mcp in tmux session

Summary by CodeRabbit

• New Features

  • Added an MCP server (agentmail mcp CLI) exposing four STDIO-based tools: send, receive, status, list-recipients (tmux-aware).

• Documentation

  • Added README MCP section, spec, plan, quickstart, data model, contracts/schema, research, tasks, and checklist docs with examples.

• Tests

  • Added extensive unit and integration tests covering server, tools, schemas, and edge cases.

• Chores

  • Bumped Go toolchain and updated CI Go versions.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@UserAd has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 203cb99 and 5e286df.

📒 Files selected for processing (3)

• internal/mcp/handlers.go
• internal/mcp/handlers_test.go
• internal/mcp/server_test.go

📝 Walkthrough

Walkthrough

Adds a tmux-aware MCP (Model Context Protocol) server to AgentMail with STDIO transport, four MCP tools (send, receive, status, list-recipients), CLI wiring, JSON Schema-backed inputs, extensive tests, and supporting specs/docs.

Changes

Cohort / File(s)	Summary
MCP server core internal/mcp/server.go, internal/mcp/doc.go, internal/mcp/server_test.go	New Server type and ServerOptions; constructs and runs an MCP SDK server over STDIO, registers tools, monitors tmux context (cancels on loss); includes lifecycle/accessor behavior and tmux-related tests.
Tool wiring & schemas internal/mcp/tools.go, internal/mcp/tools_test.go, specs/010-mcp-server/contracts/mcp-tools.json	Declares tool names and public arg types, builds JSON Schemas for tool inputs, registers tools with the MCP server, and tests discovery, schema shapes, and constants (including MaxMessageSize).
Handlers & business logic internal/mcp/handlers.go, internal/mcp/handlers_test.go	Implements send/receive/status/list-recipients logic (validation, size/status checks, mailbox persistence, ignore lists, tmux resolution), exposes MCP handler entry points, and adds comprehensive unit/integration tests.
CLI integration cmd/agentmail/main.go	Adds mcp subcommand that creates mcp.NewServer and invokes Server.Run, plus help text integration.
Documentation & specs README.md, specs/010-mcp-server/*, specs/010-mcp-server/data-model.md	Adds README section, quickstart, spec, plan, tasks, research, data model, contract, and checklist documenting design, usage, schemas, testing, and acceptance criteria.
Dependencies & CI go.mod, .github/workflows/test.yml, .github/workflows/release.yml, CLAUDE.md	Bumps Go toolchain to 1.25.5, adds github.com/modelcontextprotocol/go-sdk and related deps, and updates CI Go versions and gosec invocation; docs updated accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant Client as MCP Client / AI Assistant
    participant STDIO as STDIO Transport
    participant Server as AgentMail MCP Server
    participant Handlers as Tool Handlers
    participant Mail as Mailbox Infrastructure

    Client->>STDIO: JSON-RPC CallToolRequest
    STDIO->>Server: deliver request
    Server->>Handlers: route to handler (send/receive/status/list)
    Handlers->>Handlers: validate params (schema, size, status)
    Handlers->>Mail: read/write/update mailbox or recipient state
    Mail-->>Handlers: operation result
    Handlers->>Server: return CallToolResult (JSON payload or error)
    Server->>STDIO: send JSON-RPC response
    STDIO-->>Client: Tool result

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 79.81% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'feat: Add MCP server for AI agent integration' accurately and concisely summarizes the main change: introduction of an MCP (Model Context Protocol) server for AI agents. The title directly reflects the primary objective evident across all modified files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 20

🤖 Fix all issues with AI agents

In `@cmd/agentmail/main.go`:
- Around line 268-287: The shutdown check inside the Exec closure is fragile
because it compares errors with ==; replace the direct equality check (err ==
ctx.Err()) after server.Run(ctx, nil) with a robust errors.Is-based check (e.g.
errors.Is(err, ctx.Err()) and/or errors.Is(err, context.Canceled) ||
errors.Is(err, context.DeadlineExceeded)) so wrapped or cancel-cause errors are
recognized as normal shutdowns; import the errors package if not present and
keep the same control flow to return nil for normal cancellations and log/exit
for other errors.

In `@internal/mcp/handlers_test.go`:
- Around line 1739-1776: The failing test
TestListRecipientsHandler_EmptyWindowsList sets MockWindows but leaves
MockReceiver empty, causing doListRecipients (called via listRecipientsHandler)
to fall back to tmux.GetCurrentWindow and fail; update the test to set a
placeholder MockReceiver when using MockWindows (e.g., set
HandlerOptions.MockReceiver to a non-empty string) or alternatively change the
HandlerOptions setup in the test so MockReceiver is initialized whenever
MockWindows is provided, ensuring doListRecipients uses the mock values and does
not call tmux.GetCurrentWindow.
- Around line 2117-2162: The test TestReceiveHandler_InvalidRepoRootReturnsError
mutates global process state via os.Chdir to force a "not in a git repository"
error for receiveHandler; avoid changing WD—either stub/mock the git-root lookup
used by receiveHandler (replace the real lookup with a test double that returns
a "not in a git repository" error) or add an explicit non-parallel guard/skip so
the test cannot run concurrently; update the test to call
SetHandlerOptions(&HandlerOptions{...}) as before but remove os.Chdir/os.Getwd
usage and instead inject the failing git-root behavior (or skip when t.Parallel
would run) so the failure is deterministic without changing global working
directory.
- Around line 387-420: The test is ignoring errors returned by json.Unmarshal
for each receive response; update the four unmarshalling calls (for resp1,
resp2, resp3, resp4) to capture the error and fail the test immediately if
unmarshalling fails (e.g., if err != nil { t.Fatalf("unmarshal respN: %v", err)
}), referencing the existing receiveHandler calls and the
ReceiveResponse/ReceiveEmptyResponse variables so failures give clear
diagnostics instead of panicking on later assertions.
- Around line 2719-2731: The send test builds messages with
string(rune('A'+i%26)) which is non-obvious and then checks for "sent" in the
response even though SendResponse contains a message_id; update the loop using
fmt.Sprintf("Message %d", i) when calling makeSendRequest in sendHandler, and
change the success/error validation: if result.IsError extract the TextContent
and append the error text as before, but for non-error results assert the
response contains a message_id (e.g., check content.Text contains "message_id"
or otherwise verify the SendResponse structure) instead of looking for "sent".
Reference sendHandler, makeSendRequest and the TextContent/SendResponse check
for locating the code to modify.

In `@internal/mcp/handlers.go`:
- Around line 127-130: The check in the send handler currently treats a
self-send as "recipient not found"; update the error returned when recipient ==
sender to a specific message like "cannot send to self" so the caller and logs
get a clear, actionable message; locate the check in internal/mcp/handlers.go
where variables recipient and sender are compared and replace the
fmt.Errorf("recipient not found") with fmt.Errorf("cannot send to self") (or use
the existing error type if a package-level error constant is preferred).

In `@internal/mcp/server_test.go`:
- Around line 59-69: The test comment in TestNewServer_NilOptions is misleading:
it currently says the test "will only pass in tmux" but the test sets TMUX empty
and asserts NewServer(nil) returns an error; update the comment to state the
test documents that NewServer(nil) should fail when not running inside tmux (it
uses t.Setenv("TMUX", "") to simulate a non-tmux environment) so the expected
behavior is an error from NewServer; locate the TestNewServer_NilOptions
function and replace the misleading sentence with this clarified intent
referencing NewServer and the t.Setenv("TMUX", "") simulation.
- Around line 184-192: The test's error comparison is fragile because it checks
err != context.Canceled directly; change it to use errors.Is(err,
context.Canceled) so wrapped errors are detected: call server.Run(ctx,
&ServerOptions{SkipTmuxCheck: true}) as before, then assert that err is nil or
errors.Is(err, context.Canceled). Add an import for the standard "errors"
package if missing and update the t.Errorf conditional to use errors.Is for the
context.Canceled check.

In `@internal/mcp/tools_test.go`:
- Around line 378-397: The SC-001 performance assertion in
TestToolDiscovery_Performance is too strict for CI; update the test in
TestToolDiscovery_Performance to either skip when testing.Short() is true or
increase the allowed duration by a small buffer (e.g., 1s -> 2s or add a 500ms
tolerance) before calling clientSession.ListTools(ctx, nil), and keep the same
error handling for ListTools; reference the test function name
TestToolDiscovery_Performance and the ListTools call so maintainers can locate
and adjust the timing check accordingly.
- Around line 16-45: Modify setupTestServer to return a cleanup function and
ensure server-side resources are closed: capture the server-side session
returned by server.MCPServer().Connect (replace the discarded "_" with a
variable like serverSession), and return a func that closes serverSession, the
in-memory transports (serverTransport and clientTransport), and the Server
(e.g., server.Close() or server.Shutdown()). Update callers/tests to defer the
returned cleanup in addition to closing clientSession. This ensures
mcp.NewInMemoryTransports, server.MCPServer().Connect, and NewServer resources
are properly cleaned up.

In `@internal/mcp/tools.go`:
- Around line 95-98: The top-of-file comment for RegisterTools in
internal/mcp/tools.go is stale (says "Tool handlers are stubs that return 'not
implemented'") but the handlers are implemented; update the comment to reflect
that RegisterTools registers the fully implemented AgentMail tool handlers with
the MCP server (mentioning RegisterTools and Server) and remove the "stubs"
wording so the comment accurately describes the current implementation and test
coverage.
- Around line 46-60: The JSON schemas returned by sendToolSchema,
receiveToolSchema, statusToolSchema, and listRecipientsToolSchema are missing
"additionalProperties": false; update each json.RawMessage return to include
"additionalProperties": false at the root of the object schema (alongside "type"
and "properties") so the Go-defined schemas match the contract and reject
unexpected properties.
- Around line 43-61: Replace the hardcoded 65536 in sendToolSchema with the
existing MaxMessageSize constant: build the JSON string dynamically (e.g., via
fmt.Sprintf) or construct the schema with a small struct/map and json.Marshal so
the "maxLength" value uses MaxMessageSize; update imports to include "fmt" if
you use fmt.Sprintf and ensure sendToolSchema still returns json.RawMessage.

In `@README.md`:
- Around line 349-379: In the "MCP Tool Responses" section, add a blank line
both before and after each fenced code block so every triple-backtick block is
separated by an empty line from the surrounding text; adjust the blocks for
**send**, **receive** (message available), **receive** (no messages),
**status**, and **list-recipients** to include a blank line above the opening
```json and below the closing ``` for consistent Markdown formatting.

In `@specs/010-mcp-server/contracts/mcp-tools.json`:
- Around line 140-162: The outputSchema for the list-recipients tool only
defines the success shape and lacks an error branch; update the list-recipients
tool's outputSchema to mirror send/status by using oneOf with a success schema
(the existing recipients array object) and an error schema (e.g., object with
properties like "error" string and optional "code"/"details"), ensuring required
fields for the error branch and referencing the list-recipients operation name
and its outputSchema so callers can reliably handle failures.

In `@specs/010-mcp-server/data-model.md`:
- Around line 122-135: Add a language specifier (e.g., ```text) to the Message
Lifecycle and Recipient Status Lifecycle code fences and insert a blank line
above each heading (e.g., "### Message Lifecycle" and "### Recipient Status
Lifecycle") and between the code fence blocks and surrounding list paragraphs so
the diagrams render consistently; update both the "[Created] --send--> [Unread]
--receive--> [Read]" block and the recipient status block to use ```text and
ensure a blank line after the closing ``` and before the following list items.

In `@specs/010-mcp-server/quickstart.md`:
- Around line 21-34: The docs show two different configuration paths for Claude
settings; pick the canonical path used by the application (either
"~/.config/claude/mcp.json" or "~/.claude/settings.json"), then update both the
quickstart snippet (the mcp configuration JSON block that defines "mcpServers" /
"agentmail" / "command" / "args") and the README.md entry to use that same path
so they match exactly; ensure the example file path and any surrounding text
reflect the chosen canonical location.
- Around line 51-156: The fenced code blocks in the tool docs for the "send",
"receive", "status", and "list-recipients" sections are missing blank lines
before and/or after the triple-backtick fences; update each Example: and
Response: block so there is an empty line preceding the opening ``` and an empty
line following the closing ``` (i.e., ensure a blank line between the
surrounding paragraph/heading and the fenced block, and a blank line after the
fenced block before the next paragraph), and apply the same fix to all other
similar Example/Response code blocks in this file.

In `@specs/010-mcp-server/research.md`:
- Around line 1-182: Minor Markdown formatting: add a blank line before and
after each fenced code block (e.g., the ```go block under "Implementation
Pattern", the ```bash block under "Integration Tests", the ``` (Acceptance
Tests) snippet, and the ```go snippet under "Error Handling Pattern") so fenced
blocks are separated from surrounding text; update the document to insert a
single blank line above and below each fenced code block to satisfy Markdown
style.

In `@specs/010-mcp-server/tasks.md`:
- Around line 1-302: The markdown has minor spacing issues around headings
(e.g., headings like "## Phase 1: Setup (Shared Infrastructure)", "## Phase 2:
Foundational (MCP Server Core)", and other "##" / "---" section separators); fix
by inserting a single blank line before and after each heading and before
horizontal rules to follow common Markdown style, and scan the document for any
other headings (e.g., "## Phase 3: User Story 1 - Tool Discovery (Priority: P1)
🎯 MVP") to apply the same spacing so headings are visually separated and
linting warnings are resolved.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93cd835 and e142b9b.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (19)

• CLAUDE.md
• README.md
• cmd/agentmail/main.go
• go.mod
• internal/mcp/doc.go
• internal/mcp/handlers.go
• internal/mcp/handlers_test.go
• internal/mcp/server.go
• internal/mcp/server_test.go
• internal/mcp/tools.go
• internal/mcp/tools_test.go
• specs/010-mcp-server/checklists/requirements.md
• specs/010-mcp-server/contracts/mcp-tools.json
• specs/010-mcp-server/data-model.md
• specs/010-mcp-server/plan.md
• specs/010-mcp-server/quickstart.md
• specs/010-mcp-server/research.md
• specs/010-mcp-server/spec.md
• specs/010-mcp-server/tasks.md

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Must use Go 1.21+ for all source code implementation
Use Go Standard library only (os, os/exec, os/signal, encoding/json, fmt, io, bufio, time, sync, filepath, syscall) - avoid external dependencies except fsnotify (with justification)
Store messages in JSONL format in .agentmail/mailboxes/<recipient>.jsonl files with one message per line
Ensure file locking for atomic operations when reading/writing to .agentmail/ message files
Pass all quality gates: go test -v -race ./..., go vet ./..., go fmt ./..., and govulncheck ./... without errors
Pass gosec ./... security analysis without errors
AgentMail CLI recipient argument must be a valid tmux window name in the current session
Messages must be delivered in FIFO (First-In-First-Out) order when receiving
Mailman daemon must track stale agents and manage in-memory state for .agentmail/mailman.pid and .agentmail/recipients.jsonl

Files:

• cmd/agentmail/main.go
• internal/mcp/handlers_test.go
• internal/mcp/doc.go
• internal/mcp/tools.go
• internal/mcp/server.go
• internal/mcp/tools_test.go
• internal/mcp/handlers.go
• internal/mcp/server_test.go

🧠 Learnings (8)

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : AgentMail CLI recipient argument must be a valid tmux window name in the current session

Applied to files:

• cmd/agentmail/main.go
• README.md
• CLAUDE.md
• go.mod
• internal/mcp/handlers.go

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Ensure file locking for atomic operations when reading/writing to `.agentmail/` message files

Applied to files:

• cmd/agentmail/main.go
• CLAUDE.md
• go.mod

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Mailman daemon must track stale agents and manage in-memory state for `.agentmail/mailman.pid` and `.agentmail/recipients.jsonl`

Applied to files:

• cmd/agentmail/main.go
• specs/010-mcp-server/data-model.md
• README.md
• CLAUDE.md
• go.mod

📚 Learning: 2026-01-14T03:56:42.554Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.554Z
Learning: Applies to **/*.go : Use Go Standard library only (os, os/exec, os/signal, encoding/json, fmt, io, bufio, time, sync, filepath, syscall) - avoid external dependencies except fsnotify (with justification)

Applied to files:

• cmd/agentmail/main.go
• CLAUDE.md
• go.mod

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Store messages in JSONL format in `.agentmail/mailboxes/<recipient>.jsonl` files with one message per line

Applied to files:

• cmd/agentmail/main.go
• specs/010-mcp-server/data-model.md
• CLAUDE.md
• go.mod

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Use speckit specification and planning system for feature development with commands in `.claude/commands/`

Applied to files:

• README.md

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Build system must produce agentmail binary executable

Applied to files:

• README.md
• CLAUDE.md
• go.mod

📚 Learning: 2026-01-14T03:56:42.554Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.554Z
Learning: Applies to **/*.go : Must use Go 1.21+ for all source code implementation

Applied to files:

• CLAUDE.md
• go.mod

🧬 Code graph analysis (6)

cmd/agentmail/main.go (1)

internal/mcp/server.go (1)

• NewServer (35-71)

internal/mcp/handlers_test.go (5)

internal/mcp/tools.go (4)

• ToolSend (15-15)
• ToolReceive (16-16)
• ToolStatus (17-17)
• ToolListRecipients (18-18)

internal/mcp/handlers.go (7)

• SetHandlerOptions (36-38)
• HandlerOptions (15-28)
• ReceiveResponse (46-50)
• ReceiveEmptyResponse (53-55)
• SendResponse (41-43)
• StatusResponse (58-60)
• ListRecipientsResponse (63-65)

internal/mail/message.go (1)

• Message (10-16)

internal/cli/status.go (1)

• Status (57-117)

internal/cli/recipients.go (1)

• Recipients (22-89)

internal/mcp/tools.go (3)

internal/mail/message.go (1)

• Message (10-16)

internal/cli/status.go (1)

• Status (57-117)

internal/mcp/server.go (1)

• Server (20-23)

internal/mcp/server.go (2)

internal/tmux/tmux.go (1)

• InTmux (25-28)

internal/mcp/tools.go (1)

• RegisterTools (98-128)

internal/mcp/tools_test.go (2)

internal/mcp/server.go (4)

• Version (16-16)
• Server (20-23)
• NewServer (35-71)
• ServerOptions (26-31)

internal/mcp/tools.go (5)

• ToolSend (15-15)
• ToolReceive (16-16)
• ToolStatus (17-17)
• ToolListRecipients (18-18)
• MaxMessageSize (11-11)

internal/mcp/server_test.go (1)

internal/mcp/server.go (3)

• NewServer (35-71)
• ServerOptions (26-31)
• Version (16-16)

🪛 GitHub Actions: Test

internal/mcp/handlers_test.go

[error] 1762-1762: TestListRecipientsHandler_EmptyWindowsList failed: listRecipientsHandler returned error result: [0xc00003b200]

🪛 LanguageTool

specs/010-mcp-server/spec.md

[style] ~6-~6: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... receiving messages. For these agents I want to have MCP functionality with stdio trans...

(REP_WANT_TO_VB)

🪛 markdownlint-cli2 (0.18.1)

specs/010-mcp-server/plan.md

31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

47-47: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

57-57: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

118-118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

124-124: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

130-130: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

137-137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

specs/010-mcp-server/tasks.md

31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

47-47: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

57-57: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

118-118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

124-124: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

130-130: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

137-137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

specs/010-mcp-server/quickstart.md

62-62: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

73-73: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

86-86: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

94-94: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

103-103: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

117-117: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

127-127: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

140-140: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

148-148: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

specs/010-mcp-server/data-model.md

12-12: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

25-25: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

107-107: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

111-111: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

115-115: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

122-122: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

123-123: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

123-123: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

125-125: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

130-130: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

131-131: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

131-131: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

specs/010-mcp-server/spec.md

112-112: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

118-118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

121-121: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

129-129: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

specs/010-mcp-server/research.md

62-62: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

133-133: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

155-155: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

README.md

352-352: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

357-357: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

362-362: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

367-367: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

372-372: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

🔇 Additional comments (33)

internal/mcp/doc.go (1)

1-18: LGTM - Documentation is clear and well-structured.

The package documentation follows Go conventions and clearly describes the MCP server's purpose, tools, and usage.

CLAUDE.md (1)

97-98: LGTM - Active technologies documentation accurately reflects the MCP server additions.

The entries correctly document the Go version bump to 1.23 and the new MCP SDK dependency, maintaining consistency with the existing format.

go.mod (2)

3-3: Verify Go 1.23.0 requirement against actual features used in the codebase.

The coding guidelines require Go 1.21+ minimum. Before finalizing the version bump to 1.23.0, confirm whether the codebase uses Go 1.22+ or 1.23+ specific features (such as range over integers) that would necessitate this version, or if Go 1.21 compatibility is maintained.

---

5-9: All external dependencies have proper justifications.

The review comment incorrectly stated that ff/v3 has "no justification found." However, ff/v3 is justified in specs/005-claude-hooks-integration/research.md, which documents that it was added to provide robust flag parsing and subcommand support. This justification aligns with its usage in the codebase—seven hierarchical ffcli.Command structures that cannot be replicated with the standard library flag package.

Additionally, modelcontextprotocol/go-sdk is properly justified in specs/010-mcp-server/research.md (lines 28-32) under Constitution IV criteria: the MCP protocol requires complex handshakes, capability negotiation, and JSON Schema generation that exceed standard library capabilities. The official SDK is actively maintained and widely adopted (8M+ downloads).

All three dependencies satisfy the coding guidelines.

Likely an incorrect or invalid review comment.

README.md (2)

284-302: LGTM!

The MCP Server section is well-structured with clear tool descriptions, configuration examples for multiple AI assistants, and response format documentation. The table format for tools and the configuration snippets are helpful for users.

---

318-347: All external documentation URLs in the MCP Server configuration section are valid and accessible: code.claude.com/docs/en/mcp (Claude Code), developers.openai.com/codex/mcp/ (OpenAI Codex CLI), and geminicli.com/docs/tools/mcp-server/ (Google Gemini CLI) point to legitimate documentation resources.

specs/010-mcp-server/checklists/requirements.md (1)

1-49: LGTM!

The specification quality checklist comprehensively covers content quality, requirement completeness, EARS compliance, and feature readiness. The notes section provides useful context about which EARS patterns are applied to specific functional requirements.

cmd/agentmail/main.go (1)

243-267: LGTM!

The MCP command definition follows the established pattern of other commands in this file, with appropriate help text documenting the four exposed tools, STDIO transport requirement, tmux session dependency, and exit codes.

internal/mcp/server_test.go (2)

101-138: LGTM!

Excellent test for tmux context monitoring. The use of atomic.Bool for thread-safe state management, proper timeout handling (1100ms aligns with the FR-014 1-second requirement), and verification of the cancellation cause are all well implemented.

---

10-57: LGTM!

Good test coverage for NewServer with various configurations: tmux check failure, tmux check success, SkipTmuxCheck bypass, and accessor verification. The tests properly validate error messages and non-nil returns.

specs/010-mcp-server/quickstart.md (1)

158-186: LGTM!

The MCP Inspector testing instructions are helpful for developers to verify their setup. The examples cover all four tools with proper JSON argument formatting.

specs/010-mcp-server/spec.md (1)

1-180: Well-structured specification with comprehensive requirements.

The specification clearly defines user stories, acceptance scenarios, functional requirements (FR-001 to FR-016), and success criteria. The EARS patterns are consistently applied, and edge cases are documented. This provides a solid foundation for implementation and testing.

internal/mcp/tools_test.go (1)

47-83: Good test coverage for tool registration.

The test correctly validates that exactly 4 tools are registered with the expected names, using a map-based approach to track found tools. This ensures both presence of expected tools and absence of unexpected ones.

internal/mcp/tools.go (1)

130-152: Handler delegation pattern is clean and testable.

The thin handler functions that delegate to implementations in handlers.go provide good separation of concerns. This pattern allows the tool registration logic to remain focused on schema definition while keeping actual business logic in a dedicated file.

specs/010-mcp-server/plan.md (1)

95-97: Stale comment: "Tool handler implementations" vs "handlers.go" file description.

The comment on line 96 indicates handlers.go contains "Tool handler implementations", which is accurate. However, lines 95-97 in the docstring for RegisterTools (in tools.go) still references "stubs that return 'not implemented'" which contradicts the completed implementation status mentioned in PR objectives (81.2% coverage, all handlers implemented).

⛔ Skipped due to learnings

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Store messages in JSONL format in `.agentmail/mailboxes/<recipient>.jsonl` files with one message per line

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Ensure file locking for atomic operations when reading/writing to `.agentmail/` message files

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Mailman daemon must track stale agents and manage in-memory state for `.agentmail/mailman.pid` and `.agentmail/recipients.jsonl`

internal/mcp/handlers_test.go (2)

14-58: LGTM!

The test helper functions are well-structured with appropriate use of t.Helper(), proper error handling, and clean directory/file permissions.

---

565-624: LGTM!

The send handler test comprehensively validates message delivery, ID generation, and mailbox storage. Good boundary testing at exactly 64KB and 64KB+1.

internal/mcp/server.go (6)

1-16: LGTM!

Clean imports with appropriate separation between standard library, internal, and external packages. The Version variable pattern allows build-time injection via ldflags.

---

18-31: LGTM!

Well-designed struct with clear separation between MCP SDK server and logging. The TmuxChecker function field enables clean dependency injection for testing.

---

33-71: LGTM!

Clean initialization with proper nil-safety, tmux validation, and tool registration. The error path correctly logs before returning.

---

73-118: LGTM!

Well-structured run loop with proper context management. The error handling correctly distinguishes between tmux termination, normal shutdown (context.Canceled), and unexpected errors.

---

120-139: LGTM!

The 500ms polling interval satisfies the FR-014 requirement to exit within 1 second of tmux session loss. Clean implementation with proper ticker cleanup.

---

141-150: LGTM!

Clean accessor methods for the underlying MCP server and logger.

internal/mcp/handlers.go (9)

1-12: LGTM!

Clean imports with appropriate separation. Uses internal packages for mail storage and tmux interactions.

---

40-74: LGTM!

Response types correctly match the data model specification with appropriate JSON tags.

---

192-234: LGTM!

Clean handler implementation with proper parameter extraction, delegation to business logic, and consistent error handling through MCP content.

---

236-293: LGTM!

Correct FIFO implementation with proper error handling. The message is only returned after successfully marking it as read, ensuring consistency.

---

326-337: LGTM!

Clean validation using constants from the mail package ensures consistency with CLI validation.

---

339-387: LGTM!

Status handler correctly implements the notified flag reset semantics and delegates to the existing mail infrastructure for state persistence.

---

438-507: LGTM!

The list-recipients handler correctly implements the ignore list logic while always showing the current window. The slice initialization with []RecipientInfo{} ensures proper JSON encoding of empty arrays.

---

509-538: LGTM!

Consistent handler pattern with proper error handling and JSON encoding.

---

30-38: The concern about race conditions is unfounded. Tests do not use t.Parallel() and run sequentially, so there is no concurrent access to handlerOptions. Additionally, SetHandlerOptions is called exclusively from test code for test-only configuration injection, and the race detector (-race flag) is enabled in CI—if an actual race condition existed, it would be caught and fail the build.

Likely an incorrect or invalid review comment.

specs/010-mcp-server/data-model.md (1)

28-35: Update documentation to match actual RecipientState struct definition.

The struct documented as RecipientEntry does not exist in the codebase. The actual persistent recipient state struct is RecipientState in internal/mail/recipients.go, which uses Recipient (not Name) as the field name. Update the documentation to reflect the correct struct name and field names from the implementation.

Likely an incorrect or invalid review comment.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for t.Parallel() calls in the test file
rg -n "t\.Parallel\(\)" internal/mcp/handlers_test.go

# Also check if there are any other os.Chdir calls in the test file
echo "=== All os.Chdir calls ==="
rg -n "os\.Chdir" internal/mcp/handlers_test.go -A 2

# Check the test function signature to see if it's a table-driven test
echo "=== Test function context ==="
rg -B 3 "TestReceiveHandler_InvalidRepoRootReturnsError" internal/mcp/handlers_test.go | head -20

Repository: UserAd/AgentMail

Length of output: 917

---

🏁 Script executed:

#!/bin/bash
# Check if any t.Parallel() calls exist in the entire test file
echo "=== Checking for any t.Parallel() usage ==="
rg -c "t\.Parallel" internal/mcp/handlers_test.go || echo "No matches found"

# Check the go.mod to understand the project setup
echo "=== Go version requirement ==="
head -5 go.mod

# Check how many test functions exist in the file
echo "=== Total test functions ==="
rg "^func Test" internal/mcp/handlers_test.go | wc -l

# Let's see if there are any test setup patterns that might serialize tests
echo "=== Setup/teardown patterns ==="
rg "func TestMain" internal/mcp/handlers_test.go

# Check if mutex or sync patterns exist for directory operations
echo "=== Sync patterns in test file ==="
rg "sync\." internal/mcp/handlers_test.go | head -5

Repository: UserAd/AgentMail

Length of output: 289

---

Reconsider the approach to testing git root failures without changing working directory.

This test changes the process working directory using os.Chdir(), which is global process state. While the defer properly restores it, this pattern is problematic if any test executes concurrently. The test file doesn't use t.Parallel(), so sequential execution is the current design, but this is fragile and not enforced.

The suggestion to use t.Setenv is inapplicable—that only works for environment variables, not the working directory. Instead, consider:

• Mocking or stubbing the git root lookup function to return a fake error without changing directories
• Or explicitly document that this test must not run in parallel and skip it conditionally if t.Parallel() is called by future refactoring

🤖 Prompt for AI Agents

In `@internal/mcp/handlers_test.go` around lines 2117 - 2162, The test
TestReceiveHandler_InvalidRepoRootReturnsError mutates global process state via
os.Chdir to force a "not in a git repository" error for receiveHandler; avoid
changing WD—either stub/mock the git-root lookup used by receiveHandler (replace
the real lookup with a test double that returns a "not in a git repository"
error) or add an explicit non-parallel guard/skip so the test cannot run
concurrently; update the test to call SetHandlerOptions(&HandlerOptions{...}) as
before but remove os.Chdir/os.Getwd usage and instead inject the failing
git-root behavior (or skip when t.Parallel would run) so the failure is
deterministic without changing global working directory.

[UserAd]

Acknowledged. This is a valid concern about test isolation. The test doesn't use t.Parallel() so it won't run concurrently with other tests. For now this approach works, but we can refactor to use dependency injection if it becomes problematic.

[coderabbitai]

@UserAd, that sounds reasonable. Since the test suite runs sequentially without t.Parallel(), the current approach is safe and pragmatic. If you ever need to enable parallel test execution or want to improve test isolation, dependency injection for the git root lookup would be a good path forward. For now, the explicit defer ensures cleanup happens even on test failures, so you're covered.

---

🧠 Learnings used

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : AgentMail CLI recipient argument must be a valid tmux window name in the current session

---

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Ensure file locking for atomic operations when reading/writing to `.agentmail/` message files

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

LGTM - Research documentation is thorough and provides critical dependency justification.

The research document clearly explains the MCP SDK selection rationale and provides dependency justification per Constitution IV (lines 28-32). The testing strategy, architecture patterns, and error handling approaches are well-documented.

Note: Static analysis flagged minor markdown formatting issues (missing blank lines around fenced code blocks), but these are optional style improvements.

🧰 Tools

🪛 markdownlint-cli2 (0.18.1)

62-62: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

133-133: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

155-155: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

🤖 Prompt for AI Agents

In `@specs/010-mcp-server/research.md` around lines 1 - 182, Minor Markdown
formatting: add a blank line before and after each fenced code block (e.g., the
```go block under "Implementation Pattern", the ```bash block under "Integration
Tests", the ``` (Acceptance Tests) snippet, and the ```go snippet under "Error
Handling Pattern") so fenced blocks are separated from surrounding text; update
the document to insert a single blank line above and below each fenced code
block to satisfy Markdown style.

[UserAd]

Thank you for the positive feedback on the research documentation!

[coderabbitai]

@UserAd, you're very welcome! The research document is well-structured and provides excellent rationale for the technical decisions. Great work on documenting the dependency justification and testing strategy! 🐰

(ᵔᴥᵔ)

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

LGTM - Task documentation is comprehensive and well-organized.

The task breakdown is thorough with clear phases, dependencies, and checkpoints. All tasks marked complete align with the PR objectives showing 81.2% test coverage achieved.

Note: Static analysis flagged minor markdown formatting issues (missing blank lines around some headings), but these are optional style improvements.

🧰 Tools

🪛 markdownlint-cli2 (0.18.1)

31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

47-47: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

57-57: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

118-118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

124-124: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

130-130: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

137-137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🤖 Prompt for AI Agents

In `@specs/010-mcp-server/tasks.md` around lines 1 - 302, The markdown has minor
spacing issues around headings (e.g., headings like "## Phase 1: Setup (Shared
Infrastructure)", "## Phase 2: Foundational (MCP Server Core)", and other "##" /
"---" section separators); fix by inserting a single blank line before and after
each heading and before horizontal rules to follow common Markdown style, and
scan the document for any other headings (e.g., "## Phase 3: User Story 1 - Tool
Discovery (Priority: P1) 🎯 MVP") to apply the same spacing so headings are
visually separated and linting warnings are resolved.

[UserAd]

Thank you for confirming the task documentation is comprehensive!

[coderabbitai]

Rate Limit Exceeded

@UserAd have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 10 seconds before sending another message.

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In @.github/workflows/test.yml:
- Around line 77-80: The workflow removed the gosec action but left
security-events: write permission; either restore SARIF upload or revoke the
permission: modify the "Run gosec" step to produce SARIF (run gosec ./... -fmt
sarif -out gl-gosec.sarif or equivalent) and add an upload step using
actions/upload-sarif@v2 to send gl-gosec.sarif to GitHub Security, or if SARIF
reporting is not desired remove the repository-level permission
"security-events: write" to enforce least privilege; update the step named "Run
gosec" and the workflow permissions accordingly.
- Line 31: The gosec step currently invoked in the workflow does not produce
SARIF output despite the workflow granting security-events: write for SARIF
uploads; update the gosec invocation in the job's "gosec" step to include SARIF
output (e.g., enable "-fmt sarif" or equivalent flag and write to a file like
results.sarif) and then add or enable an upload step (using the upload-sarif
action) to upload that SARIF file so the granted security-events: write
permission is used as intended.

In `@internal/mcp/handlers_test.go`:
- Around line 198-202: The test reads result.Content[0] without ensuring Content
has elements; update the test (in the receiveHandler test block where result is
inspected) to first check len(result.Content) > 0 and call t.Fatalf or
t.Fatalf-like assertion if empty, then proceed to cast result.Content[0] to
*mcp.TextContent; this mirrors the existing pattern used in
TestReceiveHandler_NoMessagesReturnsEmptyStatus and prevents a panic when
Content is unexpectedly empty.

♻️ Duplicate comments (3)

internal/mcp/handlers_test.go (3)

387-420: Handle errors from json.Unmarshal in FIFO test.

Multiple json.Unmarshal calls have their errors ignored (lines 390, 399, 408, 417). While the test will likely fail on subsequent assertions if parsing fails, explicitly checking errors improves test diagnostics and makes failures easier to debug.

♻️ Proposed fix

 	// First receive should return msg001
 	result1, _ := receiveHandler(ctx, &mcp.CallToolRequest{})
 	text1 := result1.Content[0].(*mcp.TextContent).Text
 	var resp1 ReceiveResponse
-	json.Unmarshal([]byte(text1), &resp1)
+	if err := json.Unmarshal([]byte(text1), &resp1); err != nil {
+		t.Fatalf("Failed to unmarshal first response: %v", err)
+	}
 	if resp1.ID != "msg001" {
 		t.Errorf("First receive: expected 'msg001', got '%s'", resp1.ID)
 	}

Apply similar changes to lines 399, 408, and 417.

---

2117-2163: Consider mocking the git root lookup instead of changing working directory.

This test (and similar tests at lines 2209-2254 and 2299-2343) uses os.Chdir() to force a git root lookup failure. While the defer os.Chdir(origDir) properly restores the directory, this pattern mutates global process state and is fragile if tests ever run concurrently.

The test file doesn't currently use t.Parallel(), but this design constraint isn't enforced. Consider either:

1. Adding a mock/stub mechanism for the git root lookup in HandlerOptions (e.g., MockGitRootError bool)
2. Adding a comment explicitly documenting that these tests must not run in parallel

---

2720-2732: Bug: Incorrect success validation causes false error reporting.

The check at line 2729 !strings.Contains(content.Text, "sent") is incorrect. A successful SendResponse contains {"message_id": "..."}, not the word "sent". This means:

1. Successful sends return JSON without "sent" → condition is true → error is logged
2. The test will report errors for successful invocations

Additionally, the message generation string(rune('A'+i%26)) is non-obvious compared to fmt.Sprintf("Message %d", i).

🐛 Proposed fix

 		case 0:
 			// Send
-			result, err := sendHandler(ctx, makeSendRequest("agent-receiver", "Message "+string(rune('A'+i%26))))
+			result, err := sendHandler(ctx, makeSendRequest("agent-receiver", fmt.Sprintf("Message %d", i)))
 			if err != nil {
 				errors = append(errors, "send invocation error: "+err.Error())
 			} else if result.IsError {
 				content, _ := result.Content[0].(*mcp.TextContent)
-				if content != nil && !strings.Contains(content.Text, "sent") {
+				if content != nil {
 					errors = append(errors, "send invocation returned error: "+content.Text)
 				}
 			}

Note: You'll also need to add "fmt" to the imports.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 643ecff and ec8fd09.

📒 Files selected for processing (2)

• .github/workflows/test.yml
• internal/mcp/handlers_test.go

🧰 Additional context used

📓 Path-based instructions (2)

.github/workflows/*.{yaml,yml}

📄 CodeRabbit inference engine (CLAUDE.md)

GitHub Actions workflows must use Go 1.21+, semantic-version action for version calculation, and gh CLI for cross-repo updates

Files:

• .github/workflows/test.yml

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Must use Go 1.21+ for all source code implementation
Use Go Standard library only (os, os/exec, os/signal, encoding/json, fmt, io, bufio, time, sync, filepath, syscall) - avoid external dependencies except fsnotify (with justification)
Store messages in JSONL format in .agentmail/mailboxes/<recipient>.jsonl files with one message per line
Ensure file locking for atomic operations when reading/writing to .agentmail/ message files
Pass all quality gates: go test -v -race ./..., go vet ./..., go fmt ./..., and govulncheck ./... without errors
Pass gosec ./... security analysis without errors
AgentMail CLI recipient argument must be a valid tmux window name in the current session
Messages must be delivered in FIFO (First-In-First-Out) order when receiving
Mailman daemon must track stale agents and manage in-memory state for .agentmail/mailman.pid and .agentmail/recipients.jsonl

Files:

• internal/mcp/handlers_test.go

🧠 Learnings (6)

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to .github/workflows/*.{yaml,yml} : GitHub Actions workflows must use Go 1.21+, semantic-version action for version calculation, and gh CLI for cross-repo updates

Applied to files:

• .github/workflows/test.yml

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Pass `gosec ./...` security analysis without errors

Applied to files:

• .github/workflows/test.yml

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Pass all quality gates: `go test -v -race ./...`, `go vet ./...`, `go fmt ./...`, and `govulncheck ./...` without errors

Applied to files:

• .github/workflows/test.yml

📚 Learning: 2026-01-14T03:56:42.554Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.554Z
Learning: Applies to **/*.go : Must use Go 1.21+ for all source code implementation

Applied to files:

• .github/workflows/test.yml

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : AgentMail CLI recipient argument must be a valid tmux window name in the current session

Applied to files:

• internal/mcp/handlers_test.go

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Ensure file locking for atomic operations when reading/writing to `.agentmail/` message files

Applied to files:

• internal/mcp/handlers_test.go

🧬 Code graph analysis (1)

internal/mcp/handlers_test.go (5)

internal/mcp/tools.go (4)

• ToolSend (15-15)
• ToolReceive (16-16)
• ToolStatus (17-17)
• ToolListRecipients (18-18)

internal/mcp/handlers.go (6)

• HandlerOptions (15-28)
• ReceiveResponse (46-50)
• ReceiveEmptyResponse (53-55)
• SendResponse (41-43)
• StatusResponse (58-60)
• ListRecipientsResponse (63-65)

internal/mail/message.go (1)

• Message (10-16)

internal/cli/status.go (1)

• Status (57-117)

internal/cli/recipients.go (1)

• Recipients (22-89)

🔇 Additional comments (6)

internal/mcp/handlers_test.go (6)

1-12: LGTM!

Package declaration and imports are appropriate for this test file. The external MCP SDK import is necessary for the tool request/response types being tested.

---

14-58: LGTM!

Test helpers are well-structured:

• makeSendRequest safely ignores the json.Marshal error since marshaling a simple map[string]string cannot fail.
• setupTestMailbox properly cleans up the temp directory on creation failure before calling t.Fatalf.
• All helpers correctly use t.Helper() for proper test line reporting.

---

561-625: LGTM!

The send handler test comprehensively verifies:

• Message delivery and ID return (FR-004)
• Proper storage in the recipient's mailbox file
• Response format matches SendResponse structure

The test setup with MockSender and MockWindows correctly simulates the tmux environment.

---

667-709: LGTM!

Good boundary testing for the 64KB message size limit:

• Line 682 correctly tests with 65537 bytes (one byte over the 64KB limit)
• The companion test TestSendHandler_ExactlyMaxSizeSucceeds at line 1016 tests the exact boundary at 65536 bytes

This ensures the size validation uses > rather than >= for the comparison.

---

1134-1183: LGTM!

Excellent coverage of invalid status values including edge cases:

• Various incorrect strings ("invalid", "busy", "available")
• Case sensitivity ("READY", "Ready")
• Empty string ("")

The test properly verifies the error message format matches FR-016 requirements.

---

1739-1777: LGTM!

The previously flagged issue has been addressed - MockReceiver is now set to "test-window" (line 1748) to prevent the handler from falling through to tmux.GetCurrentWindow(). The comment on line 1745 clearly documents why this is necessary.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

Go 1.25.5 release version January 2026

💡 Result:

Go 1.25.5 was released on December 2, 2025 (it is the latest 1.25.x release as of January 14, 2026). [1][2]

Sources:
[1] OSS‑Security / Go announcement (go1.25.5, security fixes).
[2] SUSE / security update noting go1.25.5 released 2025‑12‑02.

---

🏁 Script executed:

find . -name "test.yml" -path ".github/workflows/*" -exec cat -n {} \;

Repository: UserAd/AgentMail

Length of output: 42

---

🏁 Script executed:

git ls-files ".github/workflows/" | grep -E "\.(yaml|yml)$"

Repository: UserAd/AgentMail

Length of output: 116

---

🏁 Script executed:

fd "test.yml" .github/

Repository: UserAd/AgentMail

Length of output: 86

---

🏁 Script executed:

cat -n .github/workflows/test.yml

Repository: UserAd/AgentMail

Length of output: 2658

---

Add SARIF output to gosec command to utilize the security-events permission.

Go 1.25.5 is valid (released December 2, 2025) and satisfies the Go 1.21+ requirement. However, the workflow grants security-events: write permission (line 17) with a comment stating it's "Required for uploading SARIF results," but the gosec step (lines 77-80) doesn't generate or upload SARIF output. Configure gosec to output SARIF format:

Suggested fix

      - name: Run gosec
        run: |
          go install github.com/securego/gosec/v2/cmd/gosec@latest
-         gosec ./...
+         gosec -fmt sarif -out results.sarif ./...
+
+      - name: Upload gosec SARIF report
+        uses: github/codeql-action/upload-sarif@v3
+        with:
+          sarif_file: results.sarif

🤖 Prompt for AI Agents

In @.github/workflows/test.yml at line 31, The gosec step currently invoked in
the workflow does not produce SARIF output despite the workflow granting
security-events: write for SARIF uploads; update the gosec invocation in the
job's "gosec" step to include SARIF output (e.g., enable "-fmt sarif" or
equivalent flag and write to a file like results.sarif) and then add or enable
an upload step (using the upload-sarif action) to upload that SARIF file so the
granted security-events: write permission is used as intended.

[UserAd]

Confirmed Go 1.25.5 is used to address govulncheck vulnerabilities in earlier versions.

[coderabbitai]

@UserAd, thank you for confirming the security motivation behind the Go 1.25.5 upgrade. That's an important clarification for addressing known vulnerabilities.

If you'd like to make use of the security-events: write permission that's already configured in the workflow, the SARIF output suggestion for gosec is still available (adding -fmt sarif -out results.sarif and uploading via the github/codeql-action/upload-sarif action). This would integrate gosec findings into GitHub's Security tab. However, if that's not a priority for this PR, feel free to defer it.

---

🧠 Learnings used

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to .github/workflows/*.{yaml,yml} : GitHub Actions workflows must use Go 1.21+, semantic-version action for version calculation, and gh CLI for cross-repo updates

---

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.554Z
Learning: Applies to **/*.go : Must use Go 1.21+ for all source code implementation

---

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Pass `gosec ./...` security analysis without errors

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

SARIF upload may be missing after gosec refactor.

The switch from the securego/gosec action to go install is a valid approach for Go version consistency. However, the workflow retains security-events: write permission (line 17), suggesting SARIF upload to GitHub's Security tab was previously intended but is now missing.

If security findings should appear in the Security tab, add SARIF output and upload:

Proposed fix to restore SARIF upload

       - name: Run gosec
         run: |
           go install github.com/securego/gosec/v2/cmd/gosec@latest
-          gosec ./...
+          gosec -fmt=sarif -out=gosec-results.sarif ./...
+
+      - name: Upload SARIF results
+        uses: github/codeql-action/upload-sarif@v3
+        with:
+          sarif_file: gosec-results.sarif

If SARIF upload is no longer needed, consider removing the security-events: write permission to follow least-privilege principles.

📝 Committable suggestion

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

Suggested change

	- name: Run gosec
	uses: securego/gosec@6decf96c3d272d5a8bbdcf9fddb5789d0be16a8d # v2.22.4
	with:
	args: ./...
	run: |
	go install github.com/securego/gosec/v2/cmd/gosec@latest
	gosec ./...
	- name: Run gosec
	run: |
	go install github.com/securego/gosec/v2/cmd/gosec@latest
	gosec -fmt=sarif -out=gosec-results.sarif ./...
	- name: Upload SARIF results
	uses: github/codeql-action/upload-sarif@v3
	with:
	sarif_file: gosec-results.sarif

🤖 Prompt for AI Agents

In @.github/workflows/test.yml around lines 77 - 80, The workflow removed the
gosec action but left security-events: write permission; either restore SARIF
upload or revoke the permission: modify the "Run gosec" step to produce SARIF
(run gosec ./... -fmt sarif -out gl-gosec.sarif or equivalent) and add an upload
step using actions/upload-sarif@v2 to send gl-gosec.sarif to GitHub Security, or
if SARIF reporting is not desired remove the repository-level permission
"security-events: write" to enforce least privilege; update the step named "Run
gosec" and the workflow permissions accordingly.

[UserAd]

The security-events permission was retained from the original workflow but SARIF upload is not currently implemented with the go install approach. This is intentional for now as the primary goal was to fix the Go version compatibility. SARIF upload can be added as a follow-up improvement if needed.

[coderabbitai]

@UserAd, understood! Thank you for clarifying. It makes sense to focus on the Go version compatibility fix in this PR and defer the SARIF upload implementation.

Would you like me to open a GitHub issue to track the SARIF upload enhancement as a follow-up? I can create one with the relevant context and link back to this discussion.

In the meantime, if you want to explicitly document that the security-events: write permission is reserved for future SARIF upload, you could add a brief comment in the workflow, but that's entirely optional.

---

🧠 Learnings used

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Pass `gosec ./...` security analysis without errors

---

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to .github/workflows/*.{yaml,yml} : GitHub Actions workflows must use Go 1.21+, semantic-version action for version calculation, and gh CLI for cross-repo updates

---

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Pass all quality gates: `go test -v -race ./...`, `go vet ./...`, `go fmt ./...`, and `govulncheck ./...` without errors

---

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.554Z
Learning: Applies to **/*.go : Must use Go 1.21+ for all source code implementation

[coderabbitai]

Actionable comments posted: 6

🤖 Fix all issues with AI agents

In `@internal/mcp/handlers.go`:
- Around line 347-351: The code currently returns the actual invalid value in
the error from the validateStatus check (see validateStatus and the
fmt.Errorf(...) return), so update the quickstart.md documentation to match this
behavior instead of showing a literal "X"; replace the example with something
like `Invalid status: <value>. Valid: ready, work, offline` (and ensure it
mentions the FR-016 format) so docs and the fmt.Errorf(...) message are
consistent.
- Around line 136-145: Errors from mail.FindGitRoot and mail.LoadIgnoreList are
being discarded; capture their returned errors and either log them for debugging
or explicitly document suppression. Update the code around gitRoot :=
opts.RepoRoot and the subsequent calls to mail.FindGitRoot() and
mail.LoadIgnoreList(gitRoot) to assign the error results to variables (e.g.,
err) and call the package logger (or log.Printf) to emit a brief debug/warn
message including the error and context (e.g., "FindGitRoot failed" /
"LoadIgnoreList failed"), or if intentional, replace the ignored assignments
with explicit "_ = err" comments to show suppression intent. Ensure you
reference mail.FindGitRoot, mail.LoadIgnoreList, gitRoot, and ignoreList when
making the change so the behavior is clear.
- Around line 30-38: The global variable handlerOptions is read/modified without
synchronization causing potential data races; make access thread-safe by
introducing a package-level sync.RWMutex (e.g., handlerOptionsMu) and a getter
getHandlerOptions() that uses RLock/RUnlock to return the current
*HandlerOptions, and update SetHandlerOptions to Lock/Unlock when assigning
handlerOptions; then replace direct reads of handlerOptions throughout the
package with calls to getHandlerOptions() to ensure safe concurrent access.

In `@internal/mcp/server_test.go`:
- Around line 194-202: Add a new test function (e.g.,
TestVersion_CanBeOverridden) alongside TestVersion_Default that documents and
exercises overriding the package-level Version via -ldflags; specifically,
create a test that logs the exact ldflags invocation (e.g., -ldflags "-X
agentmail/internal/mcp.Version=1.0.0") so CI/devs know how to build with a
custom Version, and keep the test non-failing (use t.Log) since ldflags are set
at build time, not at test runtime.

In `@internal/mcp/tools.go`:
- Around line 11-12: There are two duplicate constants (MaxMessageSize in
tools.go and MaxMessageSizeBytes in handlers.go); remove the duplicate and
consolidate to a single constant: keep MaxMessageSize in tools.go (used by the
schema) and update handlers.go to reference MaxMessageSize instead of
MaxMessageSizeBytes, and remove the MaxMessageSizeBytes declaration; update any
references/usages in the package to use MaxMessageSize so the value remains
consistent across code.

♻️ Duplicate comments (16)

README.md (1)

349-385: Markdown formatting has been improved.

The fenced code blocks in the MCP Tool Responses section now have proper blank lines around them, addressing the previous review feedback.

internal/mcp/server_test.go (2)

45-69: Test comment has been clarified.

The comment in TestNewServer_NilOptions now correctly states the intent: simulating a non-tmux environment to verify error behavior.

---

170-192: Error comparison now uses errors.Is correctly.

Line 189 properly uses errors.Is(err, context.Canceled) to handle potentially wrapped errors, addressing the previous review feedback.

specs/010-mcp-server/quickstart.md (2)

21-34: Configuration path is now consistent with README.

The Claude Code configuration path (~/.claude/settings.json) now matches the README.md documentation, resolving the previous inconsistency.

---

51-156: Add blank lines around fenced code blocks for markdown consistency.

Static analysis flagged multiple instances of missing blank lines around fenced code blocks. While this doesn't affect rendering in most parsers, adding blank lines is a markdown best practice.

📝 Example fix pattern

 **Example:**
+
 ```json
 {
   "name": "send",
   ...
 }

Response:

Apply this pattern to all **Example:** and **Response:** sections (lines 61-77, 85-107, 116-131, 139-156).
</details>

</blockquote></details>
<details>
<summary>internal/mcp/handlers.go (1)</summary><blockquote>

`127-130`: **Self-send error message has been improved.**

The error message is now "cannot send message to self" instead of the generic "recipient not found", providing clearer feedback to users.

</blockquote></details>
<details>
<summary>internal/mcp/tools.go (3)</summary><blockquote>

`44-63`: **Schema now correctly uses MaxMessageSize constant.**

The `sendToolSchema` function now uses `fmt.Sprintf` with `MaxMessageSize` to dynamically set the `maxLength` value, ensuring consistency with the constant definition.

---

`65-98`: **Schemas correctly include additionalProperties: false.**

All tool schemas now include the `additionalProperties: false` constraint, matching the contract specification in `specs/010-mcp-server/contracts/mcp-tools.json`.

---

`100-133`: **RegisterTools comment is now accurate.**

The comment correctly describes that each tool is registered with its JSON schema and corresponding handler that delegates to the implementation in handlers.go.

</blockquote></details>
<details>
<summary>internal/mcp/tools_test.go (2)</summary><blockquote>

`378-402`: **LGTM! Performance test is now CI-friendly.**

The `testing.Short()` skip and 1.5s threshold (up from 1s) address the flakiness concerns from the previous review. The comment on line 398 clearly explains the tolerance.

---

`16-45`: **Consider returning a cleanup function from `setupTestServer`.**

The server-side session returned by `server.MCPServer().Connect()` (line 32) is discarded. While in-memory transports likely share resources, explicitly closing the server session would ensure proper cleanup. This was noted in a previous review.

</blockquote></details>
<details>
<summary>internal/mcp/handlers_test.go (5)</summary><blockquote>

`365-430`: **LGTM! FIFO test with proper error handling for JSON parsing.**

The test correctly validates FIFO message ordering with four consecutive receives. All `json.Unmarshal` calls now include error handling (lines 391-393, 402-404, 413-415, 424-426), addressing the previous review feedback.

---

`1748-1786`: **LGTM! Empty windows list edge case handled correctly.**

The `MockReceiver` is now set to a non-empty value (`"test-window"`) as recommended in the previous review, preventing the handler from falling through to `tmux.GetCurrentWindow()`. The comment on line 1754 documents this requirement clearly.

---

`2707-2778`: **LGTM! 100-invocation stress test with corrected message generation.**

The test now uses `fmt.Sprintf("Message %d", i)` (line 2733) as recommended in the previous review. The error handling correctly checks `result.IsError` without the incorrect "sent" string check that was previously flagged.

---

`2126-2172`: **`os.Chdir` pattern is fragile for future refactoring.**

Multiple tests (lines 2127-2172, 2218-2263, 2308-2352) use `os.Chdir` to force "not in a git repository" errors. While tests don't currently use `t.Parallel()`, this global state mutation is fragile. Consider mocking the git root lookup function instead, or add explicit documentation that these tests cannot run in parallel.

---

`199-203`: **Add bounds check before accessing `result.Content[0]`.**

This test accesses `result.Content[0]` (line 200) without first checking that `Content` is non-empty, unlike similar tests (e.g., lines 94-96, 148-150). Add a bounds check for consistency and to provide clear diagnostics if the handler unexpectedly returns empty content.



<details>
<summary>🔧 Proposed fix</summary>

```diff
 	// Parse the response
+	if len(result.Content) == 0 {
+		t.Fatal("receiveHandler returned empty content")
+	}
 	textContent, ok := result.Content[0].(*mcp.TextContent)
 	if !ok {
 		t.Fatalf("receiveHandler content is not TextContent")
 	}

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec8fd09 and 9e4cccb.

📒 Files selected for processing (8)

• README.md
• cmd/agentmail/main.go
• internal/mcp/handlers.go
• internal/mcp/handlers_test.go
• internal/mcp/server_test.go
• internal/mcp/tools.go
• internal/mcp/tools_test.go
• specs/010-mcp-server/quickstart.md

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Must use Go 1.21+ for all source code implementation
Use Go Standard library only (os, os/exec, os/signal, encoding/json, fmt, io, bufio, time, sync, filepath, syscall) - avoid external dependencies except fsnotify (with justification)
Store messages in JSONL format in .agentmail/mailboxes/<recipient>.jsonl files with one message per line
Ensure file locking for atomic operations when reading/writing to .agentmail/ message files
Pass all quality gates: go test -v -race ./..., go vet ./..., go fmt ./..., and govulncheck ./... without errors
Pass gosec ./... security analysis without errors
AgentMail CLI recipient argument must be a valid tmux window name in the current session
Messages must be delivered in FIFO (First-In-First-Out) order when receiving
Mailman daemon must track stale agents and manage in-memory state for .agentmail/mailman.pid and .agentmail/recipients.jsonl

Files:

• internal/mcp/handlers_test.go
• internal/mcp/tools_test.go
• internal/mcp/server_test.go
• internal/mcp/tools.go
• cmd/agentmail/main.go
• internal/mcp/handlers.go

🧠 Learnings (6)

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : AgentMail CLI recipient argument must be a valid tmux window name in the current session

Applied to files:

• internal/mcp/handlers_test.go
• README.md
• cmd/agentmail/main.go
• internal/mcp/handlers.go

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Ensure file locking for atomic operations when reading/writing to `.agentmail/` message files

Applied to files:

• internal/mcp/handlers_test.go

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Mailman daemon must track stale agents and manage in-memory state for `.agentmail/mailman.pid` and `.agentmail/recipients.jsonl`

Applied to files:

• README.md

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Use speckit specification and planning system for feature development with commands in `.claude/commands/`

Applied to files:

• README.md

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Build system must produce agentmail binary executable

Applied to files:

• README.md

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Store messages in JSONL format in `.agentmail/mailboxes/<recipient>.jsonl` files with one message per line

Applied to files:

• internal/mcp/tools.go

🧬 Code graph analysis (6)

internal/mcp/handlers_test.go (3)

internal/mcp/tools.go (4)

• ToolSend (16-16)
• ToolReceive (17-17)
• ToolStatus (18-18)
• ToolListRecipients (19-19)

internal/mcp/handlers.go (7)

• SetHandlerOptions (36-38)
• HandlerOptions (15-28)
• ReceiveResponse (46-50)
• ReceiveEmptyResponse (53-55)
• SendResponse (41-43)
• StatusResponse (58-60)
• ListRecipientsResponse (63-65)

internal/mail/message.go (1)

• Message (10-16)

internal/mcp/tools_test.go (2)

internal/mcp/server.go (4)

• Version (16-16)
• Server (20-23)
• NewServer (35-71)
• ServerOptions (26-31)

internal/mcp/tools.go (5)

• ToolSend (16-16)
• ToolReceive (17-17)
• ToolStatus (18-18)
• ToolListRecipients (19-19)
• MaxMessageSize (12-12)

internal/mcp/server_test.go (1)

internal/mcp/server.go (3)

• NewServer (35-71)
• ServerOptions (26-31)
• Version (16-16)

internal/mcp/tools.go (1)

internal/mcp/server.go (1)

• Server (20-23)

cmd/agentmail/main.go (1)

internal/mcp/server.go (1)

• NewServer (35-71)

internal/mcp/handlers.go (7)

internal/mail/message.go (2)

• Message (10-16)
• GenerateID (23-37)

internal/cli/status.go (1)

• Status (57-117)

internal/cli/recipients.go (1)

• Recipients (22-89)

internal/tmux/tmux.go (3)

• GetCurrentWindow (55-68)
• WindowExists (97-110)
• ListWindows (72-93)

internal/mail/ignore.go (2)

• FindGitRoot (13-28)
• LoadIgnoreList (33-53)

internal/mail/mailbox.go (3)

• Append (61-101)
• FindUnread (151-165)
• MarkAsRead (229-295)

internal/mail/recipients.go (1)

• UpdateRecipientState (119-197)

🪛 markdownlint-cli2 (0.18.1)

specs/010-mcp-server/quickstart.md

62-62: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

73-73: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

86-86: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

94-94: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

103-103: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

117-117: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

127-127: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

140-140: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

148-148: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

🔇 Additional comments (17)

README.md (2)

284-294: LGTM! Clear MCP server documentation.

The MCP Server section provides a well-structured overview of the four available tools with their purposes clearly described in the table format.

---

303-347: Documentation links are valid and accessible.

All three external documentation URLs in the configuration examples are confirmed working:

• https://code.claude.com/docs/en/mcp (HTTP 200)
• https://developers.openai.com/codex/mcp/ (HTTP 200)
• https://geminicli.com/docs/tools/mcp-server/ (HTTP 200)

No action needed.

internal/mcp/server_test.go (3)

1-9: LGTM! Clean imports and package declaration.

The test file has appropriate imports including errors for proper error comparison with errors.Is.

---

11-43: Good test coverage for NewServer scenarios.

The tests cover the key scenarios: not in tmux (error), in tmux (success), and verify that the server components (mcpServer, logger) are properly initialized.

---

101-138: Robust tmux context monitoring test.

The test uses sync/atomic for thread-safe state manipulation and properly validates the context cancellation cause. The 1100ms timeout correctly accounts for the FR-014 requirement of 1-second detection.

specs/010-mcp-server/quickstart.md (1)

188-208: Good requirements and error handling documentation.

The requirements section clearly states the prerequisites (tmux session, PATH, repository structure), and the error handling section documents common error messages that align with the implementation in handlers.go.

internal/mcp/handlers.go (3)

76-93: Good input validation with appropriate size check.

The handler correctly validates:

1. Empty message check (line 85-87)
2. Message size limit using byte length (line 90-92), which correctly handles UTF-8 strings

---

266-293: FIFO message retrieval correctly implemented.

The receive handler correctly:

1. Retrieves unread messages (line 267)
2. Takes the oldest (first) message for FIFO order (line 280)
3. Marks it as read before returning (line 283)

This aligns with the coding guidelines requirement for FIFO delivery order.

---

486-507: Well-structured recipient filtering logic.

The implementation correctly:

1. Always includes the current window (even if ignored)
2. Filters out ignored windows for non-current entries
3. Returns a properly structured response

This matches the behavior documented in the README.

internal/mcp/tools.go (1)

135-157: Handler wrappers are appropriately thin.

The handler wrapper functions simply delegate to the implementation functions in handlers.go, maintaining clean separation between tool registration and business logic.

cmd/agentmail/main.go (2)

269-287: LGTM! Context cancellation handling is now robust.

The errors.Is checks for context.Canceled and context.DeadlineExceeded correctly handle normal shutdown scenarios, including wrapped errors and cancel-cause contexts. This addresses the previous review feedback.

---

244-268: LGTM! Well-structured MCP command definition.

The command follows the existing pattern for commands without flags. The LongHelp text clearly documents the four MCP tools, transport mechanism, and exit codes, which will be helpful for users.

internal/mcp/tools_test.go (3)

47-83: LGTM!

Solid test verifying that exactly four tools are registered and all expected tool names are present using the exported constants.

---

121-202: LGTM! Thorough schema validation for send tool.

The test comprehensively validates the send tool schema structure: type checking, property presence (recipient, message), and required field verification. This ensures the MCP tool schema matches the specification.

---

433-460: LGTM! Good validation of schema constraint against constant.

The test correctly verifies that the maxLength in the schema matches the MaxMessageSize constant (65536), ensuring consistency between the schema and the handler's validation logic.

internal/mcp/handlers_test.go (2)

15-59: LGTM! Well-structured test helpers.

The helper functions follow good testing practices: t.Helper() calls for proper error attribution, proper cleanup setup with returned paths, and appropriate directory permissions.

---

61-118: LGTM! FIFO ordering test with proper bounds checking.

The test correctly validates FR-003 (oldest unread message returned first). The bounds check on result.Content (lines 94-96) addresses the previous review feedback about preventing panics on unexpected empty content.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Error message format differs from documentation.

The error message on line 350 uses "Invalid status: %s. Valid: ready, work, offline" but the quickstart.md (line 207) shows "Invalid status: X. Valid: ready, work, offline". These should be consistent.

The documentation shows a literal "X" as placeholder while the code uses the actual invalid value. Consider updating the documentation to show that the actual value is included:

- `Invalid status: <value>. Valid: ready, work, offline` - Bad status value

🤖 Prompt for AI Agents

In `@internal/mcp/handlers.go` around lines 347 - 351, The code currently returns
the actual invalid value in the error from the validateStatus check (see
validateStatus and the fmt.Errorf(...) return), so update the quickstart.md
documentation to match this behavior instead of showing a literal "X"; replace
the example with something like `Invalid status: <value>. Valid: ready, work,
offline` (and ensure it mentions the FR-016 format) so docs and the
fmt.Errorf(...) message are consistent.

[UserAd]

The error message in the code is correctly formatted as 'Invalid status: %s. Valid: ready, work, offline' where %s is replaced with the actual invalid value. The quickstart.md documentation shows 'X' as a placeholder for any invalid value. Both are consistent - the code shows the actual invalid value, while the doc uses 'X' as an example.

[coderabbitai]

Rate Limit Exceeded

@UserAd have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 1 seconds before sending another message.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@internal/mcp/handlers_test.go`:
- Line 283: Several tests directly index result.Content[0] (e.g., the cast to
mcp.TextContent) without guarding that result.Content has at least one element,
which can cause panics; update each test (including the example using
result.Content[0] and others flagged) to first assert len(result.Content) > 0
(or use require.Len/require.Greater from the test helpers) before accessing or
casting the first element, and then proceed to cast to *mcp.TextContent only
after that guard so all tests follow a consistent bounds-checking pattern.

♻️ Duplicate comments (3)

internal/mcp/handlers_test.go (3)

199-203: Add bounds check before accessing result.Content[0].

This test accesses result.Content[0] without first verifying that Content is non-empty. Other tests in this file (e.g., lines 94-96, 148-150) properly check len(result.Content) == 0 before access. Apply the same pattern here to prevent potential panic.

♻️ Proposed fix

 	// Parse the response
+	if len(result.Content) == 0 {
+		t.Fatal("receiveHandler returned empty content")
+	}
 	textContent, ok := result.Content[0].(*mcp.TextContent)
 	if !ok {
 		t.Fatalf("receiveHandler content is not TextContent")
 	}

---

2137-2149: Fragile test pattern using os.Chdir to simulate git root failure.

This test (and similar tests at lines 2228-2240, 2317-2329) mutates global process state via os.Chdir. While sequential test execution currently prevents issues, this pattern is fragile if t.Parallel() is ever introduced.

Consider refactoring to inject a mock git root lookup function via HandlerOptions instead of changing the working directory, which would make the test more robust and parallelizable.

---

2673-2680: Test doesn't actually verify the 2-second requirement (SC-004).

The test claims to verify that tool invocations complete within 2 seconds, but it doesn't measure elapsed time. The comment "If we got here within the test timeout, we're under 2 seconds" is incorrect—Go's default test timeout is 10 minutes.

🐛 Proposed fix to add explicit timing

 	// 1. Send tool
 	t.Run("send_under_2s", func(t *testing.T) {
+		start := time.Now()
 		_, err := sendHandler(ctx, makeSendRequest("agent-receiver", "Performance test message"))
+		elapsed := time.Since(start)
 		if err != nil {
 			t.Fatalf("sendHandler returned error: %v", err)
 		}
-		// If we got here within the test timeout, we're under 2 seconds
+		if elapsed > 2*time.Second {
+			t.Errorf("send took %v, expected < 2 seconds", elapsed)
+		}
 	})

Apply similar changes to the other subtests.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9e4cccb and 203cb99.

📒 Files selected for processing (4)

• internal/mcp/handlers_test.go
• internal/mcp/tools_test.go
• specs/010-mcp-server/data-model.md
• specs/010-mcp-server/quickstart.md

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Must use Go 1.21+ for all source code implementation
Use Go Standard library only (os, os/exec, os/signal, encoding/json, fmt, io, bufio, time, sync, filepath, syscall) - avoid external dependencies except fsnotify (with justification)
Store messages in JSONL format in .agentmail/mailboxes/<recipient>.jsonl files with one message per line
Ensure file locking for atomic operations when reading/writing to .agentmail/ message files
Pass all quality gates: go test -v -race ./..., go vet ./..., go fmt ./..., and govulncheck ./... without errors
Pass gosec ./... security analysis without errors
AgentMail CLI recipient argument must be a valid tmux window name in the current session
Messages must be delivered in FIFO (First-In-First-Out) order when receiving
Mailman daemon must track stale agents and manage in-memory state for .agentmail/mailman.pid and .agentmail/recipients.jsonl

Files:

• internal/mcp/handlers_test.go
• internal/mcp/tools_test.go

🧠 Learnings (4)

📓 Common learnings

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : AgentMail CLI recipient argument must be a valid tmux window name in the current session

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : AgentMail CLI recipient argument must be a valid tmux window name in the current session

Applied to files:

• internal/mcp/handlers_test.go

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Ensure file locking for atomic operations when reading/writing to `.agentmail/` message files

Applied to files:

• internal/mcp/handlers_test.go

📚 Learning: 2026-01-14T03:56:42.555Z

Learnt from: CR
Repo: UserAd/AgentMail PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T03:56:42.555Z
Learning: Applies to **/*.go : Mailman daemon must track stale agents and manage in-memory state for `.agentmail/mailman.pid` and `.agentmail/recipients.jsonl`

Applied to files:

• specs/010-mcp-server/data-model.md

🧬 Code graph analysis (2)

internal/mcp/handlers_test.go (5)

internal/mcp/tools.go (4)

• ToolSend (16-16)
• ToolReceive (17-17)
• ToolStatus (18-18)
• ToolListRecipients (19-19)

internal/mcp/handlers.go (6)

• HandlerOptions (15-28)
• ReceiveResponse (46-50)
• ReceiveEmptyResponse (53-55)
• SendResponse (41-43)
• StatusResponse (58-60)
• ListRecipientsResponse (63-65)

internal/mail/message.go (1)

• Message (10-16)

internal/cli/status.go (1)

• Status (57-117)

internal/cli/recipients.go (1)

• Recipients (22-89)

internal/mcp/tools_test.go (2)

internal/mcp/server.go (4)

• Version (16-16)
• Server (20-23)
• NewServer (35-71)
• ServerOptions (26-31)

internal/mcp/tools.go (5)

• ToolSend (16-16)
• ToolReceive (17-17)
• ToolStatus (18-18)
• ToolListRecipients (19-19)
• MaxMessageSize (12-12)

🔇 Additional comments (13)

specs/010-mcp-server/quickstart.md (1)

1-217: LGTM!

The quickstart documentation is well-structured and provides clear examples for configuring AI assistants, using the available tools, and testing with MCP Inspector. The configuration path (line 23) and markdown formatting issues from previous reviews have been addressed.

specs/010-mcp-server/data-model.md (1)

1-138: LGTM!

The data model documentation accurately describes the MCP-specific types and their alignment with existing AgentMail entities. The markdown formatting issues from the previous review (blank lines around headings, language specifiers for code fences) have been addressed.

internal/mcp/handlers_test.go (5)

15-51: Well-structured test helpers.

The makeSendRequest, setupTestMailbox, and writeTestMessages helpers provide clean abstractions for test setup. The use of t.Helper() ensures proper error location reporting.

---

61-118: Comprehensive FIFO ordering test.

Good coverage of FR-003 requirement with explicit verification that the oldest unread message is returned first, including proper bounds checking and response validation.

---

574-633: Thorough send handler validation.

The test properly verifies message delivery, ID generation, and persistence to the mailbox file. Good coverage of FR-004 requirement.

---

1083-1141: Good status handler test coverage.

Verifies both the MCP response format and the persistence of status to recipients.jsonl. Proper validation of FR-005 requirement.

---

1479-1535: Comprehensive list-recipients test.

Good coverage of FR-006 with verification that all agents are returned and properly enumerated.

internal/mcp/tools_test.go (6)

15-51: Well-designed test server setup with proper cleanup.

The setupTestServer function now returns a cleanup function that closes both client and server sessions, addressing the resource leak concern from the previous review.

---

53-89: Good tool registration verification.

Properly verifies all 4 tools are exposed and uses the constant names from tools.go for consistency.

---

127-208: Thorough send tool schema validation.

Validates the complete schema structure including type, properties, and required fields. Good coverage of the JSON Schema contract.

---

251-341: Comprehensive status tool enum validation.

Properly verifies the status enum contains all valid values (ready, work, offline) and that status is a required field.

---

384-408: Performance test improvements applied.

The test now skips in short mode and uses a 1.5s threshold to account for CI variability, addressing the flakiness concerns from the previous review.

---

439-466: Good maxLength schema constraint validation.

Verifies the send tool schema enforces the 64KB message limit via the maxLength constraint matching MaxMessageSize.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}