Skip to content

Commit c3d5e43

Browse files
authored
fix: address PR review comments and markdown lint issues (#35)
* fix: address PR review comments and markdown lint issues - Use strings.HasPrefix for clearer intent in test assertions - Document edge case when both FindGitRoot and Getwd fail - Fix markdown lint errors (MD032, MD031, MD022, MD040, MD033) - Escape <error> in spec docs to avoid inline HTML warnings * docs: document mailman stop command and add post-implementation checklist Update README.md with mailman stop command usage, examples, and exit codes. Add Post-Implementation Checklist section to CLAUDE.md to ensure documentation stays in sync with the codebase after feature implementations.
1 parent 7e26655 commit c3d5e43

11 files changed

Lines changed: 51 additions & 9 deletions

File tree

‎CLAUDE.md‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,15 @@ This project uses speckit for feature specification and planning. Available comm
7878

7979
Templates are stored in `.specify/templates/` and project constitution in `.specify/memory/constitution.md`.
8080

81-
**Important:** After implementing a spec, always update `README.md` to reflect the new functionality.
81+
## Post-Implementation Checklist
82+
83+
After completing any feature implementation or significant work:
84+
85+
1. **Update README.md** - Ensure all new commands, options, and functionality are documented
86+
2. **Update spec status** - Mark the spec as "Implemented" in the spec.md file
87+
3. **Verify examples** - Ensure README examples still work with any changes
88+
89+
This checklist ensures documentation stays in sync with the codebase.
8290

8391
## Active Technologies
8492
- Go 1.21+ (per IC-001) + Standard library only (os/exec for tmux, encoding/json for JSONL) (001-agent-mail-structure)

‎README.md‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,24 +228,29 @@ agentmail status offline
228228

229229
### mailman
230230

231-
Start the mailman daemon to monitor mailboxes and notify agents.
231+
Start or stop the mailman daemon to monitor mailboxes and notify agents.
232232

233233
```bash
234234
agentmail mailman [--daemon]
235+
agentmail mailman stop
235236
```
236237

237238
**Flags:**
238239

239240
- `--daemon` - Run in background (daemonize)
240241

242+
**Subcommands:**
243+
244+
- `stop` - Stop the running daemon gracefully
245+
241246
**Behavior:**
242247

243248
- Uses file watching (fsnotify) for instant notification on mailbox changes
244249
- Includes 60-second safety timer that runs alongside watching
245250
- Sends notifications to agents with `ready` status that have unread mail
246251
- Notifications sent via tmux: `tmux send-keys -t <window> "Check your agentmail"`
247252
- Stores PID in `.agentmail/mailman.pid`
248-
- Gracefully shuts down on SIGTERM/SIGINT
253+
- Gracefully shuts down on SIGTERM/SIGINT or when `stop` command is issued
249254

250255
**Examples:**
251256

@@ -255,14 +260,22 @@ agentmail mailman
255260

256261
# Run as background daemon
257262
agentmail mailman --daemon
263+
264+
# Stop the running daemon
265+
agentmail mailman stop
258266
```
259267

260-
**Exit codes:**
268+
**Exit codes (start):**
261269

262-
- `0` - Daemon started/stopped successfully
270+
- `0` - Daemon started successfully
263271
- `1` - Error (failed to start, PID file error, etc.)
264272
- `2` - Daemon already running
265273

274+
**Exit codes (stop):**
275+
276+
- `0` - Stop signal sent successfully
277+
- `1` - Error (stop already pending or filesystem error)
278+
266279
### onboard
267280

268281
Output AI-optimized onboarding context about AgentMail.

‎internal/cli/mailman_stop.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@ type MailmanStopOptions struct {
1717
// MailmanStop implements the agentmail mailman stop command.
1818
// Creates a .stop file to signal the daemon to shut down.
1919
//
20+
// The function attempts to find the repository root via FindGitRoot,
21+
// falling back to os.Getwd if not in a git repository. If both fail,
22+
// repoRoot will be empty and file creation will fail with a clear error.
23+
//
2024
// Exit codes:
2125
// - 0: Success (stop signal sent)
2226
// - 1: Error (file exists or filesystem error)

‎internal/cli/mailman_stop_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"os"
66
"path/filepath"
7+
"strings"
78
"testing"
89

910
"agentmail/internal/daemon"
@@ -157,7 +158,7 @@ func TestMailmanStop_FilesystemError_ReturnsError(t *testing.T) {
157158

158159
// Verify error message contains expected prefix
159160
expectedPrefix := "Failed to send stop signal:"
160-
if len(stderr.String()) < len(expectedPrefix) || stderr.String()[:len(expectedPrefix)] != expectedPrefix {
161+
if !strings.HasPrefix(stderr.String(), expectedPrefix) {
161162
t.Errorf("Expected stderr to start with %q, got %q", expectedPrefix, stderr.String())
162163
}
163164

‎specs/012-mailman-stop/contracts/cli.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,14 @@ Failed to send stop signal: <error message>
5454
### Fire-and-Forget Pattern
5555

5656
The stop command creates a signal file and immediately exits with code 0. It does NOT:
57+
5758
- Wait for the daemon to terminate
5859
- Verify the daemon is running
5960
- Verify the daemon received the signal
6061
- Delete any files
6162

6263
The daemon is responsible for:
64+
6365
- Detecting the `.stop` file via file watcher
6466
- Removing the `.stop` file
6567
- Removing the `.pid` file
@@ -76,6 +78,7 @@ The stop mechanism uses file creation as inter-process communication:
7678
5. Daemon initiates shutdown and cleans up files
7779

7880
This approach:
81+
7982
- Requires no process validation
8083
- Works cross-platform
8184
- Uses existing file watcher infrastructure

‎specs/012-mailman-stop/data-model.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ This feature introduces a file-based signaling mechanism for stopping the daemon
1919
**Purpose**: Acts as an inter-process communication (IPC) mechanism between the CLI stop command and the running daemon.
2020

2121
**Operations**:
22+
2223
| Operation | Actor | Trigger |
2324
|-----------|-------|---------|
2425
| Create | CLI (stop command) | User runs `agentmail mailman stop` |

‎specs/012-mailman-stop/plan.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ Add `agentmail mailman stop` subcommand to gracefully terminate the mailman daem
3131
| IV. Standard Library | ✅ PASS | Uses only stdlib (os package for file operations) |
3232

3333
**Quality Gates Required**:
34+
3435
1. `gofmt -l .` - no output
3536
2. `go mod verify` - pass
3637
3. `go vet ./...` - pass

‎specs/012-mailman-stop/quickstart.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,11 +135,13 @@ mailmanCmd := &ffcli.Command{
135135
### Step 6: Write Tests
136136

137137
#### internal/cli/mailman_stop_test.go
138+
138139
- Test success case (file created)
139140
- Test "stop already pending" (file exists)
140141
- Test filesystem error (permissions)
141142

142143
#### internal/daemon/watcher_test.go (extend existing)
144+
143145
- Test stop file detection triggers shutdown
144146

145147
## Key Files to Modify

‎specs/012-mailman-stop/research.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
**Decision**: Create an empty `.stop` file in `.agentmail/` directory. Daemon detects via existing fsnotify watcher.
1313

1414
**Rationale**:
15+
1516
- File creation is a simple, cross-platform IPC mechanism
1617
- The daemon already has fsnotify watching `.agentmail/` for mailbox changes
1718
- No need for Unix signals, process validation, or syscall dependencies
@@ -33,12 +34,14 @@
3334
**Decision**: Use `os.OpenFile` with `O_CREATE|O_EXCL` flags to atomically create the file only if it doesn't exist.
3435

3536
**Rationale**:
37+
3638
- `O_EXCL` flag causes the open to fail if file exists
3739
- This is atomic at the filesystem level
3840
- No race conditions between check and create
3941
- Go's `os.OpenFile` supports this directly
4042

4143
**Implementation**:
44+
4245
```go
4346
// Atomic create - fails if file exists
4447
f, err := os.OpenFile(stopFilePath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600)
@@ -59,12 +62,14 @@ return "Stop signal sent"
5962
**Decision**: Extend the existing `FileWatcher` to detect CREATE events for `.stop` file in `.agentmail/` directory.
6063

6164
**Rationale**:
65+
6266
- The daemon already watches `.agentmail/` and `mailboxes/` directories
6367
- fsnotify provides CREATE events for new files
6468
- No additional polling or infrastructure needed
6569
- Detection is nearly instant (< 100ms typically)
6670

6771
**Implementation Notes**:
72+
6873
- The watcher's `Run()` function receives all events
6974
- Filter for `fsnotify.Create` events where filename is `.stop`
7075
- When detected, trigger graceful shutdown sequence
@@ -74,6 +79,7 @@ return "Stop signal sent"
7479
**Question**: What's the correct order for daemon shutdown?
7580

7681
**Decision**: Follow this sequence:
82+
7783
1. Detect `.stop` file
7884
2. Remove `.stop` file (acknowledge receipt)
7985
3. Close file watcher (stops notification loop)
@@ -82,11 +88,13 @@ return "Stop signal sent"
8288
6. Exit with code 0
8389

8490
**Rationale**:
91+
8592
- Removing `.stop` first prevents stale signal files
8693
- Closing watcher before PID removal ensures clean state
8794
- Matches existing signal-based shutdown sequence in `runForeground()`
8895

8996
**Existing Code Reference** (`internal/daemon/daemon.go:249-271`):
97+
9098
```go
9199
// Wait for shutdown signal or test stop
92100
<-sigChan

‎specs/012-mailman-stop/spec.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ An agent operator runs the stop command when a previous stop is already pending
5353
- **FR-001**: When `agentmail mailman stop` is invoked, the CLI shall attempt to create the file `.agentmail/.stop`.
5454
- **FR-002**: When the `.stop` file is created successfully, the CLI shall output "Stop signal sent" to stdout and exit with code 0.
5555
- **FR-003**: If the `.stop` file already exists, then the CLI shall output "Stop already pending" to stderr and exit with code 1.
56-
- **FR-004**: If the `.stop` file cannot be created due to a filesystem error, then the CLI shall output "Failed to send stop signal: <error>" to stderr and exit with code 1.
56+
- **FR-004**: If the `.stop` file cannot be created due to a filesystem error, then the CLI shall output "Failed to send stop signal: \<error\>" to stderr and exit with code 1.
5757

5858
**Daemon Stop File Detection:**
5959

0 commit comments

Comments
 (0)