Files
emcli/docs/superpowers/plans/2026-06-27-human-cli-grammar-redesign.md
T
2026-06-27 11:39:03 +01:00

1663 lines
56 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Human-facing CLI grammar redesign — Implementation Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Give all human-facing admin commands one consistent positional grammar (`emcli <group> <verb> <operands…> [--flags]`), close the silent-input/empty-operand bug classes, and add discoverability (`config list`, `account show`) — leaving the agent JSON commands frozen.
**Architecture:** The admin command handlers in `internal/cli/admin.go` (plus `doctor` in `run.go`) are rewritten to read primary targets as positional args and reserve flags for modifiers. New pure helpers (`normalizeVerb`, `policy.ValidWhitelistAddress`, a settings registry, `store.SetWhitelistEnabled`) carry the testable logic. The agent commands (`list`/`get`/`search`/`ack`/`send`) and their flags are untouched.
**Tech Stack:** Go 1.25, standard `flag`/`net/mail`, `github.com/mattn/go-isatty` (promoted from indirect to direct), `modernc.org/sqlite`. Tests are standard `go test` table/integration style via the existing `run(t, args...)` and `adminEnv(t)` helpers in `internal/cli/admin_test.go`.
## Global Constraints
- Module path: `git.dcglab.co.uk/steve/emcli`. Go version floor: `go 1.25.0`.
- Agent JSON commands `list`, `get`, `search`, `ack`, `send` MUST NOT change (flags, behavior, or output). `doctor` keeps its `--account` flag (agent-documented) while also accepting a positional account.
- Exit codes: `0` success, `1` runtime error, `2` usage error. Preserve these.
- Admin commands are human-readable on stdout; usage/errors go to stderr. (`account list` with an agent-only key keeps its existing reduced-JSON branch — do not touch it.)
- Whitelist direction is a **required** `--in`/`--out` flag — never a silent default.
- Verb aliases everywhere a verb appears: `remove` = `rm` = `del`; `list` = `ls`.
- Two safety rules in every rewritten handler: reject leftover/unexpected positional args, and reject empty primary operands.
- Spec: `docs/superpowers/specs/2026-06-27-human-cli-grammar-redesign-design.md`.
---
### Task 1: `normalizeVerb` alias helper
**Files:**
- Modify: `internal/cli/dispatch.go`
- Test: `internal/cli/dispatch_test.go` (create)
**Interfaces:**
- Produces: `func normalizeVerb(v string) string` — maps `"rm"`/`"del"``"remove"`, `"ls"``"list"`, else returns `v` unchanged.
- [ ] **Step 1: Write the failing test**
Create `internal/cli/dispatch_test.go`:
```go
package cli
import "testing"
func TestNormalizeVerb(t *testing.T) {
cases := map[string]string{
"rm": "remove", "del": "remove", "remove": "remove",
"ls": "list", "list": "list",
"add": "add", "enable": "enable", "": "",
}
for in, want := range cases {
if got := normalizeVerb(in); got != want {
t.Errorf("normalizeVerb(%q) = %q, want %q", in, got, want)
}
}
}
```
- [ ] **Step 2: Run test to verify it fails**
Run: `go test ./internal/cli/ -run TestNormalizeVerb`
Expected: FAIL (`undefined: normalizeVerb`).
- [ ] **Step 3: Implement the helper**
Append to `internal/cli/dispatch.go` (inside the existing `package cli`, no new imports):
```go
// normalizeVerb maps verb aliases to their canonical form. Applied at the
// subcommand-verb position of every admin group and at the top-level command.
func normalizeVerb(v string) string {
switch v {
case "rm", "del":
return "remove"
case "ls":
return "list"
}
return v
}
```
- [ ] **Step 4: Run test to verify it passes**
Run: `go test ./internal/cli/ -run TestNormalizeVerb`
Expected: PASS.
- [ ] **Step 5: Commit**
```bash
git add internal/cli/dispatch.go internal/cli/dispatch_test.go
git commit -m "feat(cli): add normalizeVerb alias helper"
```
---
### Task 2: `policy.ValidWhitelistAddress`
**Files:**
- Modify: `internal/policy/policy.go`
- Test: `internal/policy/policy_test.go` (create if absent; otherwise add the test function)
**Interfaces:**
- Produces: `func ValidWhitelistAddress(s string) error` — nil for a bare `local@domain` or a `@domain` wildcard (domain must contain a `.`); error otherwise (empty, no `@`, no dot in domain, or display-name form like `Bob <b@x.com>`).
- [ ] **Step 1: Write the failing test**
Create `internal/policy/policy_test.go` (or add to it):
```go
package policy
import "testing"
func TestValidWhitelistAddress(t *testing.T) {
good := []string{"tk555@protonmail.com", "a.b@sub.example.co.uk", "@example.com", "@sub.example.com"}
for _, s := range good {
if err := ValidWhitelistAddress(s); err != nil {
t.Errorf("ValidWhitelistAddress(%q) = %v, want nil", s, err)
}
}
bad := []string{"", " ", "notanaddress", "@", "@nodot", "a@nodot", "Bob <b@x.com>", "a@b@c.com"}
for _, s := range bad {
if err := ValidWhitelistAddress(s); err == nil {
t.Errorf("ValidWhitelistAddress(%q) = nil, want error", s)
}
}
}
```
- [ ] **Step 2: Run test to verify it fails**
Run: `go test ./internal/policy/ -run TestValidWhitelistAddress`
Expected: FAIL (`undefined: ValidWhitelistAddress`).
- [ ] **Step 3: Implement the validator**
In `internal/policy/policy.go`, add `"errors"` and `"fmt"` to the import block (it already imports `net/mail` and `strings`), then append:
```go
// ValidWhitelistAddress reports an error if s is not a usable whitelist entry.
// Accepted forms: a bare address "local@domain", or a domain wildcard "@domain".
// The domain must contain at least one dot. Display-name forms ("Bob <b@x>")
// are rejected because the store keeps the raw string, which would never match.
func ValidWhitelistAddress(s string) error {
s = strings.TrimSpace(s)
if s == "" {
return errors.New("address must not be empty")
}
bad := fmt.Errorf("invalid address %q: expected local@domain or @domain", s)
if strings.HasPrefix(s, "@") {
domain := s[1:]
if domain == "" || strings.Contains(domain, "@") || !strings.Contains(domain, ".") {
return bad
}
return nil
}
addr, err := mail.ParseAddress(s)
if err != nil || !strings.EqualFold(addr.Address, s) {
return bad // parse failure or a display-name/extra-token form
}
at := strings.LastIndex(addr.Address, "@")
if at < 1 || !strings.Contains(addr.Address[at+1:], ".") {
return bad
}
return nil
}
```
- [ ] **Step 4: Run test to verify it passes**
Run: `go test ./internal/policy/ -run TestValidWhitelistAddress`
Expected: PASS.
- [ ] **Step 5: Commit**
```bash
git add internal/policy/policy.go internal/policy/policy_test.go
git commit -m "feat(policy): add ValidWhitelistAddress"
```
---
### Task 3: config settings registry
**Files:**
- Create: `internal/cli/config_registry.go`
- Test: `internal/cli/config_registry_test.go`
**Interfaces:**
- Produces:
- `type settingDef struct { desc string; validate func(string) error }`
- `var settingsRegistry map[string]settingDef` — initially one key `"audit_retention_days"`.
- `func settingKeys() []string` — registry keys in stable sorted order.
- [ ] **Step 1: Write the failing test**
Create `internal/cli/config_registry_test.go`:
```go
package cli
import "testing"
func TestSettingsRegistry(t *testing.T) {
def, ok := settingsRegistry["audit_retention_days"]
if !ok {
t.Fatal("audit_retention_days must be registered")
}
if def.desc == "" {
t.Error("registered setting needs a description")
}
if err := def.validate("30"); err != nil {
t.Errorf("validate(30) = %v, want nil", err)
}
for _, bad := range []string{"-1", "abc", ""} {
if def.validate(bad) == nil {
t.Errorf("validate(%q) = nil, want error", bad)
}
}
if _, ok := settingsRegistry["nope"]; ok {
t.Error("unknown key must not be present")
}
keys := settingKeys()
if len(keys) != len(settingsRegistry) {
t.Fatalf("settingKeys len=%d, want %d", len(keys), len(settingsRegistry))
}
}
```
- [ ] **Step 2: Run test to verify it fails**
Run: `go test ./internal/cli/ -run TestSettingsRegistry`
Expected: FAIL (`undefined: settingsRegistry`).
- [ ] **Step 3: Implement the registry**
Create `internal/cli/config_registry.go`:
```go
package cli
import (
"fmt"
"sort"
"strconv"
)
// settingDef describes a configurable global setting for `config list`/`set`.
type settingDef struct {
desc string
validate func(string) error
}
// settingsRegistry is the authoritative set of valid config keys. `config set`
// rejects keys absent here; `config list` enumerates them.
var settingsRegistry = map[string]settingDef{
"audit_retention_days": {
desc: "Days to keep audit-log entries (integer >= 0)",
validate: func(v string) error {
n, err := strconv.Atoi(v)
if err != nil || n < 0 {
return fmt.Errorf("must be an integer >= 0, got %q", v)
}
return nil
},
},
}
// settingKeys returns the registry keys in stable sorted order.
func settingKeys() []string {
ks := make([]string, 0, len(settingsRegistry))
for k := range settingsRegistry {
ks = append(ks, k)
}
sort.Strings(ks)
return ks
}
```
- [ ] **Step 4: Run test to verify it passes**
Run: `go test ./internal/cli/ -run TestSettingsRegistry`
Expected: PASS.
- [ ] **Step 5: Commit**
```bash
git add internal/cli/config_registry.go internal/cli/config_registry_test.go
git commit -m "feat(cli): add config settings registry"
```
---
### Task 4: `store.SetWhitelistEnabled`
**Files:**
- Modify: `internal/store/whitelist.go`
- Test: `internal/store/whitelist_enabled_test.go` (create)
**Interfaces:**
- Consumes: existing `store` helpers `(*Store).accountID`, `b2i` (in `account.go`), `Direction` constants.
- Produces: `func (s *Store) SetWhitelistEnabled(account string, dir Direction, enabled bool) error` — sets `whitelist_in_enabled` or `whitelist_out_enabled` for the account; returns the `accountID` error (`ErrAccountNotFound`) if the account is missing.
- [ ] **Step 1: Write the failing test**
Create `internal/store/whitelist_enabled_test.go`:
```go
package store
import (
"path/filepath"
"testing"
)
func TestSetWhitelistEnabled(t *testing.T) {
st, err := Open(filepath.Join(t.TempDir(), "e.db"))
if err != nil {
t.Fatalf("open: %v", err)
}
defer st.Close()
k := make([]byte, 32)
if err := st.InitKeys(k, k); err != nil {
t.Fatalf("InitKeys: %v", err)
}
if _, err := st.AddAccount(Account{Name: "a", Mode: "RO", IMAPHost: "h", IMAPPort: 993, IMAPSecurity: "tls", AuthType: "password", Username: "u@x.com"}); err != nil {
t.Fatalf("AddAccount: %v", err)
}
if err := st.SetWhitelistEnabled("a", DirIn, true); err != nil {
t.Fatalf("SetWhitelistEnabled: %v", err)
}
got, err := st.GetAccount("a")
if err != nil {
t.Fatalf("GetAccount: %v", err)
}
if !got.WhitelistInEnabled || got.WhitelistOutEnabled {
t.Fatalf("flags wrong: in=%v out=%v", got.WhitelistInEnabled, got.WhitelistOutEnabled)
}
if err := st.SetWhitelistEnabled("missing", DirIn, true); err == nil {
t.Fatal("enabling on a missing account must error")
}
}
```
- [ ] **Step 2: Run test to verify it fails**
Run: `go test ./internal/store/ -run TestSetWhitelistEnabled`
Expected: FAIL (`undefined: SetWhitelistEnabled`).
- [ ] **Step 3: Implement the method**
Append to `internal/store/whitelist.go` (it already imports `fmt`):
```go
// SetWhitelistEnabled toggles one account's per-direction whitelist-enabled
// flag, leaving the address list and all other fields untouched.
func (s *Store) SetWhitelistEnabled(account string, dir Direction, enabled bool) error {
col := "whitelist_in_enabled"
if dir == DirOut {
col = "whitelist_out_enabled"
}
id, err := s.accountID(account)
if err != nil {
return err
}
_, err = s.db.Exec(fmt.Sprintf("UPDATE accounts SET %s=? WHERE id=?", col), b2i(enabled), id)
return err
}
```
- [ ] **Step 4: Run test to verify it passes**
Run: `go test ./internal/store/ -run TestSetWhitelistEnabled`
Expected: PASS.
- [ ] **Step 5: Commit**
```bash
git add internal/store/whitelist.go internal/store/whitelist_enabled_test.go
git commit -m "feat(store): add SetWhitelistEnabled"
```
---
### Task 5: rewrite `config` to positional grammar + `config list`
**Files:**
- Modify: `internal/cli/admin.go` (replace `runConfig`, lines ~234-287)
- Test: `internal/cli/admin_test.go` (replace `TestConfigSetGet`, `TestConfigSetRejectsBadRetention`; add new tests)
**Interfaces:**
- Consumes: `settingsRegistry`, `settingKeys()` (Task 3), `normalizeVerb` (Task 1), `store.GetSetting`/`SetSetting`.
- Produces: `config list` / `config get <key>` / `config set <key> <value>` with unknown-key rejection and registry validation; `ls` alias.
- [ ] **Step 1: Write the failing tests**
In `internal/cli/admin_test.go`, replace `TestConfigSetGet` and `TestConfigSetRejectsBadRetention` with:
```go
func TestConfigSetGet(t *testing.T) {
adminEnv(t)
if code, _, e := run(t, "config", "set", "audit_retention_days", "30"); code != 0 {
t.Fatalf("config set failed: %s", e)
}
code, out, _ := run(t, "config", "get", "audit_retention_days")
if code != 0 || !strings.Contains(out, "30") {
t.Fatalf("config get: code=%d out=%q", code, out)
}
}
func TestConfigSetRejectsBadRetention(t *testing.T) {
adminEnv(t)
if code, _, _ := run(t, "config", "set", "audit_retention_days", "-5"); code != 2 {
t.Fatal("negative retention must be a usage error")
}
if code, _, _ := run(t, "config", "set", "audit_retention_days", "abc"); code != 2 {
t.Fatal("non-integer retention must be a usage error")
}
}
func TestConfigRejectsUnknownKey(t *testing.T) {
adminEnv(t)
if code, _, e := run(t, "config", "set", "bogus", "1"); code != 2 || !strings.Contains(e, "unknown setting") {
t.Fatalf("set unknown key: code=%d err=%q", code, e)
}
if code, _, e := run(t, "config", "get", "bogus"); code != 2 || !strings.Contains(e, "unknown setting") {
t.Fatalf("get unknown key: code=%d err=%q", code, e)
}
}
func TestConfigList(t *testing.T) {
adminEnv(t)
run(t, "config", "set", "audit_retention_days", "42")
code, out, _ := run(t, "config", "ls") // alias
if code != 0 {
t.Fatalf("config ls exit=%d", code)
}
if !strings.Contains(out, "audit_retention_days") || !strings.Contains(out, "42") || !strings.Contains(out, "KEY") {
t.Fatalf("config list output wrong:\n%s", out)
}
}
```
- [ ] **Step 2: Run tests to verify they fail**
Run: `go test ./internal/cli/ -run 'TestConfig'`
Expected: FAIL (new behavior not implemented; `config ls`/unknown-key/list not handled).
- [ ] **Step 3: Replace `runConfig`**
In `internal/cli/admin.go`, replace the whole `runConfig` function with:
```go
// runConfig handles `config <list|get|set>` against the settings registry.
func runConfig(args []string, role store.Role, out, errOut io.Writer) int {
if len(args) == 0 || helpRequested(args[0]) {
printCmdUsage(out, "config")
if len(args) > 0 {
return 0
}
return 2
}
sub := normalizeVerb(args[0])
rest := args[1:]
st, err := openStore(role)
if err != nil {
fmt.Fprintf(errOut, "emcli: %v\n", err)
return 1
}
defer st.Close()
switch sub {
case "list":
if len(rest) > 0 {
fmt.Fprintf(errOut, "unexpected argument %q\n", rest[0])
return 2
}
fmt.Fprintf(out, "%-22s %-8s %s\n", "KEY", "VALUE", "DESCRIPTION")
for _, k := range settingKeys() {
v, err := st.GetSetting(k)
if err != nil {
v = "(unset)"
}
fmt.Fprintf(out, "%-22s %-8s %s\n", k, v, settingsRegistry[k].desc)
}
return 0
case "get":
if len(rest) != 1 {
fmt.Fprintln(errOut, "usage: emcli config get <key>")
return 2
}
key := rest[0]
if _, ok := settingsRegistry[key]; !ok {
fmt.Fprintf(errOut, "unknown setting %q (see: emcli config list)\n", key)
return 2
}
v, err := st.GetSetting(key)
if err != nil {
fmt.Fprintf(errOut, "config get: %s not set\n", key)
return 1
}
fmt.Fprintf(out, "%s = %s\n", key, v)
return 0
case "set":
if len(rest) != 2 {
fmt.Fprintln(errOut, "usage: emcli config set <key> <value>")
return 2
}
key, value := rest[0], rest[1]
def, ok := settingsRegistry[key]
if !ok {
fmt.Fprintf(errOut, "unknown setting %q (see: emcli config list)\n", key)
return 2
}
if def.validate != nil {
if err := def.validate(value); err != nil {
fmt.Fprintf(errOut, "%s %v\n", key, err)
return 2
}
}
if err := st.SetSetting(key, value); err != nil {
fmt.Fprintf(errOut, "config set: %v\n", err)
return 1
}
fmt.Fprintf(out, "%s = %s\n", key, value)
return 0
default:
fmt.Fprintf(errOut, "unknown config subcommand %q (want list|get|set)\n", sub)
return 2
}
}
```
Remove the now-unused `"strconv"` import from `admin.go` only if nothing else uses it (the old `runConfig` was the sole user; `account remove` etc. don't use it — verify with the build in Step 4 and drop the import if the compiler flags it).
- [ ] **Step 4: Run tests to verify they pass**
Run: `go test ./internal/cli/ -run 'TestConfig'`
Expected: PASS. If the build fails on an unused `strconv` import, remove it from `admin.go` and re-run.
- [ ] **Step 5: Commit**
```bash
git add internal/cli/admin.go internal/cli/admin_test.go
git commit -m "feat(cli): positional config grammar with registry + config list"
```
---
### Task 6: rewrite `whitelist` to new grammar (positional, required --in/--out, enable/disable)
**Files:**
- Modify: `internal/cli/admin.go` (replace `runWhitelist`, lines ~319-388; add a `flowName` helper)
- Test: `internal/cli/admin_test.go` (replace the three `TestWhitelist*` tests; add new ones)
**Interfaces:**
- Consumes: `normalizeVerb` (Task 1), `policy.ValidWhitelistAddress` (Task 2), `store.SetWhitelistEnabled` (Task 4), existing `store.AddWhitelist`/`RemoveWhitelist`/`ListWhitelist`/`GetAccount`, `store.DirIn`/`DirOut`.
- Produces: `whitelist <add|remove|list|enable|disable> <account> [addr…] --in|--out` with verb aliases, address validation, leftover-arg rejection, and an empty-list warning on `enable`.
- [ ] **Step 1: Write the failing tests**
In `internal/cli/admin_test.go`, replace `TestWhitelistMissingDirectionReported`, `TestWhitelistMissingSubcommandReported`, and `TestWhitelistListWorks` with:
```go
func TestWhitelistRequiresDirection(t *testing.T) {
adminEnv(t)
run(t, "account", "add", "bobby", "--imap-host", "h", "--username", "u@x.com")
code, _, errOut := run(t, "whitelist", "add", "bobby", "a@x.com")
if code != 2 || !strings.Contains(errOut, "--in") || !strings.Contains(errOut, "--out") {
t.Fatalf("missing direction must name --in/--out: code=%d err=%q", code, errOut)
}
}
func TestWhitelistAddListRemove(t *testing.T) {
adminEnv(t)
run(t, "account", "add", "bobby", "--imap-host", "h", "--username", "u@x.com")
if code, _, e := run(t, "whitelist", "add", "bobby", "a@x.com", "@y.com", "--out"); code != 0 {
t.Fatalf("add failed: %s", e)
}
code, out, _ := run(t, "whitelist", "list", "bobby", "--out")
if code != 0 || !strings.Contains(out, "a@x.com") || !strings.Contains(out, "@y.com") || !strings.Contains(out, "DISABLED") {
t.Fatalf("list wrong: code=%d out=%q", code, out)
}
if code, _, e := run(t, "whitelist", "rm", "bobby", "a@x.com", "--out"); code != 0 { // alias
t.Fatalf("rm failed: %s", e)
}
_, out, _ = run(t, "whitelist", "ls", "bobby", "--out") // alias
if strings.Contains(out, "a@x.com") {
t.Fatalf("address not removed:\n%s", out)
}
}
func TestWhitelistRejectsBadAddress(t *testing.T) {
adminEnv(t)
run(t, "account", "add", "bobby", "--imap-host", "h", "--username", "u@x.com")
if code, _, e := run(t, "whitelist", "add", "bobby", "notanaddress", "--in"); code != 2 || !strings.Contains(e, "invalid address") {
t.Fatalf("bad address must be rejected: code=%d err=%q", code, e)
}
// The original bug: a missing address must not silently insert a blank row.
if code, _, _ := run(t, "whitelist", "add", "bobby", "--in"); code != 2 {
t.Fatal("add with no address must be a usage error")
}
}
func TestWhitelistEnableDisable(t *testing.T) {
adminEnv(t)
run(t, "account", "add", "bobby", "--imap-host", "h", "--username", "u@x.com")
// Enabling an empty whitelist warns but succeeds.
code, _, errOut := run(t, "whitelist", "enable", "bobby", "--in")
if code != 0 || !strings.Contains(errOut, "empty") {
t.Fatalf("enable empty: code=%d err=%q", code, errOut)
}
_, out, _ := run(t, "whitelist", "list", "bobby", "--in")
if !strings.Contains(out, "ENABLED") {
t.Fatalf("expected ENABLED:\n%s", out)
}
if code, _, e := run(t, "whitelist", "disable", "bobby", "--in"); code != 0 {
t.Fatalf("disable failed: %s", e)
}
_, out, _ = run(t, "whitelist", "list", "bobby", "--in")
if !strings.Contains(out, "DISABLED") {
t.Fatalf("expected DISABLED:\n%s", out)
}
}
```
- [ ] **Step 2: Run tests to verify they fail**
Run: `go test ./internal/cli/ -run 'TestWhitelist'`
Expected: FAIL (old grammar still in place).
- [ ] **Step 3: Replace `runWhitelist` and add `flowName`**
In `internal/cli/admin.go`, replace the whole `runWhitelist` function with the following, and add the `flowName` helper directly above it. Add `"git.dcglab.co.uk/steve/emcli/internal/policy"` and `"strings"` to the import block.
```go
// flowName renders a direction for human-facing prose.
func flowName(dir store.Direction) string {
if dir == store.DirOut {
return "outbound"
}
return "inbound"
}
// runWhitelist handles `whitelist <add|remove|list|enable|disable> <account>
// [addr…] --in|--out`.
func runWhitelist(args []string, role store.Role, out, errOut io.Writer) int {
if len(args) == 0 || helpRequested(args[0]) {
printCmdUsage(out, "whitelist")
if len(args) > 0 {
return 0
}
return 2
}
sub := normalizeVerb(args[0])
switch sub {
case "add", "remove", "list", "enable", "disable": // valid
default:
fmt.Fprintf(errOut, "unknown whitelist subcommand %q (want add|remove|list|enable|disable)\n", sub)
return 2
}
// Split the remaining tokens into the direction flag and positionals.
var dir store.Direction
var dirSet bool
var pos []string
for _, a := range args[1:] {
switch a {
case "--in", "-in":
dir, dirSet = store.DirIn, true
case "--out", "-out":
dir, dirSet = store.DirOut, true
default:
if strings.HasPrefix(a, "-") {
fmt.Fprintf(errOut, "unknown flag %q (use --in or --out)\n", a)
return 2
}
pos = append(pos, a)
}
}
if !dirSet {
fmt.Fprintln(errOut, "direction is required: pass --in or --out")
return 2
}
if len(pos) == 0 {
fmt.Fprintln(errOut, "account is required")
return 2
}
account, addrs := pos[0], pos[1:]
st, err := openStore(role)
if err != nil {
fmt.Fprintf(errOut, "emcli: %v\n", err)
return 1
}
defer st.Close()
switch sub {
case "add", "remove":
if len(addrs) == 0 {
fmt.Fprintln(errOut, "at least one address is required")
return 2
}
for _, addr := range addrs {
if err := policy.ValidWhitelistAddress(addr); err != nil {
fmt.Fprintln(errOut, err)
return 2
}
}
for _, addr := range addrs {
if sub == "add" {
err = st.AddWhitelist(account, dir, addr)
} else {
err = st.RemoveWhitelist(account, dir, addr)
}
if err != nil {
fmt.Fprintf(errOut, "%s: %v\n", sub, err)
return 1
}
}
verb := "added"
if sub == "remove" {
verb = "removed"
}
fmt.Fprintf(out, "%s %d address(es) in the %s whitelist of %q\n", verb, len(addrs), dir, account)
return 0
case "list":
if len(addrs) > 0 {
fmt.Fprintf(errOut, "unexpected argument %q\n", addrs[0])
return 2
}
acc, err := st.GetAccount(account)
if err != nil {
fmt.Fprintf(errOut, "list: %v\n", err)
return 1
}
enabled := acc.WhitelistInEnabled
if dir == store.DirOut {
enabled = acc.WhitelistOutEnabled
}
state := "DISABLED"
if enabled {
state = "ENABLED"
}
entries, err := st.ListWhitelist(account, dir)
if err != nil {
fmt.Fprintf(errOut, "list: %v\n", err)
return 1
}
fmt.Fprintf(out, "%s whitelist of %q: %s\n", dir, account, state)
for _, a := range entries {
fmt.Fprintln(out, a)
}
return 0
case "enable", "disable":
if len(addrs) > 0 {
fmt.Fprintf(errOut, "unexpected argument %q\n", addrs[0])
return 2
}
enable := sub == "enable"
if enable {
if entries, _ := st.ListWhitelist(account, dir); len(entries) == 0 {
fmt.Fprintf(errOut, "warning: %s whitelist for %q is empty — this blocks ALL %s mail until you add addresses\n", dir, account, flowName(dir))
}
}
if err := st.SetWhitelistEnabled(account, dir, enable); err != nil {
fmt.Fprintf(errOut, "%s: %v\n", sub, err)
return 1
}
state := "disabled"
if enable {
state = "enabled"
}
fmt.Fprintf(out, "%s whitelist of %q %s\n", dir, account, state)
return 0
}
return 0
}
```
- [ ] **Step 4: Run tests to verify they pass**
Run: `go test ./internal/cli/ -run 'TestWhitelist'`
Expected: PASS.
- [ ] **Step 5: Commit**
```bash
git add internal/cli/admin.go internal/cli/admin_test.go
git commit -m "feat(cli): positional whitelist grammar with required direction, enable/disable, validation"
```
---
### Task 7: rewrite `account` add/edit/remove/list to positional grammar (drop whitelist flags, TTY confirm)
**Files:**
- Modify: `internal/cli/admin.go` (replace `runAccount`, lines ~14-217)
- Modify: `go.mod` / `go.sum` (promote `github.com/mattn/go-isatty` to direct via `go mod tidy`)
- Test: `internal/cli/admin_test.go` (update `TestAccountRemove`, `TestAccountRemoveMissing`, `TestAccountEditPartialPreservesOtherFields`, `TestAccountEditFromValidationRejectsMalformed` to positional grammar; add `TestAccountRemoveNoTTYNeedsYes`)
**Interfaces:**
- Consumes: `normalizeVerb` (Task 1), `store.AddAccount`/`UpdateAccount`/`DeleteAccount`/`GetAccount`/`ListAccounts`, `tui.ValidFromAddress`, `addInteractive`/`editInteractive` (unchanged), `crypto.AdminKeyFromEnv`.
- Produces: `account add [name] [field flags]`, `account edit <name> [field flags]`, `account remove <name> [--yes]` (TTY confirm), `account list`. **No `--whitelist-in`/`--whitelist-out` flags.** `account show` is added in Task 8 (its `case "show"` is wired here as a call to `accountShow`, defined in Task 8).
- Produces helper: `func confirmRemoval(name string, out io.Writer) bool` (TTY y/N prompt via `isatty`).
- [ ] **Step 1: Write/replace the failing tests**
In `internal/cli/admin_test.go`, replace the four named tests with positional-grammar versions and add the no-TTY test:
```go
func TestAccountRemove(t *testing.T) {
adminEnv(t)
run(t, "account", "add", "gone", "--imap-host", "h", "--username", "u@x.com")
if code, _, e := run(t, "account", "remove", "gone", "--yes"); code != 0 {
t.Fatalf("remove failed: %s", e)
}
_, out, _ := run(t, "account", "list")
if strings.Contains(out, "gone") {
t.Fatalf("account still listed after remove:\n%s", out)
}
}
func TestAccountRemoveMissing(t *testing.T) {
adminEnv(t)
if code, _, _ := run(t, "account", "remove", "nope", "--yes"); code == 0 {
t.Fatal("removing a missing account must be non-zero")
}
}
func TestAccountRemoveNoTTYNeedsYes(t *testing.T) {
adminEnv(t)
run(t, "account", "add", "keep", "--imap-host", "h", "--username", "u@x.com")
// Under `go test`, stdin is not a TTY, so without --yes this must refuse.
code, _, errOut := run(t, "account", "remove", "keep")
if code != 2 || !strings.Contains(errOut, "--yes") {
t.Fatalf("non-TTY remove without --yes must refuse: code=%d err=%q", code, errOut)
}
}
func TestAccountEditPartialPreservesOtherFields(t *testing.T) {
db := adminEnv(t)
run(t, "account", "add", "ed", "--mode", "RO",
"--imap-host", "imap.x.com", "--username", "u@x.com", "--password", "orig")
if code, _, e := run(t, "account", "edit", "ed", "--mode", "RW",
"--smtp-host", "smtp.x.com", "--smtp-port", "587", "--smtp-security", "starttls"); code != 0 {
t.Fatalf("edit failed: %s", e)
}
st, err := store.Open(db)
if err != nil {
t.Fatalf("open: %v", err)
}
defer st.Close()
adminKey, _ := crypto.AdminKeyFromEnv()
agentKey, _ := crypto.AgentKeyFromEnv()
if err := st.Unlock(store.RoleAdmin, adminKey, agentKey); err != nil {
t.Fatalf("Unlock: %v", err)
}
got, err := st.GetAccount("ed")
if err != nil {
t.Fatalf("GetAccount: %v", err)
}
if got.Mode != "RW" || got.SMTPHost != "smtp.x.com" || got.SMTPPort != 587 {
t.Fatalf("edit didn't apply: %+v", got)
}
if got.IMAPHost != "imap.x.com" || got.Username != "u@x.com" || got.Password != "orig" {
t.Fatalf("edit clobbered preserved fields: %+v", got)
}
}
func TestAccountEditFromValidationRejectsMalformed(t *testing.T) {
adminEnv(t)
run(t, "account", "add", "valacc", "--imap-host", "imap.x.com", "--username", "u@x.com")
code, _, errStr := run(t, "account", "edit", "valacc", "--from", "not an address")
if code != 2 {
t.Fatalf("expected exit code 2 for malformed --from, got %d (stderr: %q)", code, errStr)
}
if errStr == "" {
t.Fatal("expected an error message on stderr for malformed --from, got none")
}
}
```
- [ ] **Step 2: Run tests to verify they fail**
Run: `go test ./internal/cli/ -run 'TestAccount'`
Expected: FAIL (positional `account add gone` / `account remove gone` not yet supported).
- [ ] **Step 3: Promote go-isatty to a direct dependency**
Run:
```bash
go get github.com/mattn/go-isatty@v0.0.20
go mod tidy
```
Expected: `go.mod` now lists `github.com/mattn/go-isatty v0.0.20` in the direct `require` block (the `// indirect` comment is removed).
- [ ] **Step 4: Replace `runAccount` and add `confirmRemoval`**
In `internal/cli/admin.go`, add these imports: `"bufio"`, `"os"`, `"strings"` (if not already added in Task 6), and `"github.com/mattn/go-isatty"`. Replace the whole `runAccount` function with the version below, and add `confirmRemoval` directly above it.
```go
// confirmRemoval prompts on a TTY for a y/N answer. Non-TTY callers never reach
// here (the caller requires --yes when stdin is not a terminal).
func confirmRemoval(name string, out io.Writer) bool {
fmt.Fprintf(out, "Remove account %q? [y/N]: ", name)
line, _ := bufio.NewReader(os.Stdin).ReadString('\n')
line = strings.ToLower(strings.TrimSpace(line))
return line == "y" || line == "yes"
}
// runAccount handles `account <add|edit|remove|show|list>`. Human-readable
// output (except the agent-only reduced-JSON branch of `list`).
func runAccount(args []string, role store.Role, out, errOut io.Writer) int {
if len(args) == 0 || helpRequested(args[0]) {
printCmdUsage(out, "account")
fmt.Fprintln(out, "\nSubcommands: add, edit, remove, show, list")
if len(args) > 0 {
return 0
}
return 2
}
sub := normalizeVerb(args[0])
rest := args[1:]
st, err := openStore(role)
if err != nil {
if sub == "list" {
_ = Failure(CodeConfig, err.Error()).Write(out)
} else {
fmt.Fprintf(errOut, "emcli: %v\n", err)
}
return 1
}
defer st.Close()
switch sub {
case "add":
if len(rest) == 0 { // no args → interactive TUI form
return addInteractive(st, tui.Fields{}, out, errOut)
}
// Peel a leading positional name (if present) before flag parsing.
var name string
if !strings.HasPrefix(rest[0], "-") {
name, rest = rest[0], rest[1:]
}
fs := flag.NewFlagSet("account add", flag.ContinueOnError)
fs.SetOutput(errOut)
mode := fs.String("mode", "RO", "RO|RW")
host := fs.String("imap-host", "", "IMAP host")
port := fs.Int("imap-port", 993, "IMAP port")
sec := fs.String("imap-security", "tls", "tls|starttls")
smtpHost := fs.String("smtp-host", "", "SMTP host (RW accounts)")
smtpPort := fs.Int("smtp-port", 465, "SMTP port")
smtpSec := fs.String("smtp-security", "tls", "tls|starttls")
user := fs.String("username", "", "login username")
pass := fs.String("password", "", "login password")
from := fs.String("from", "", "send-as address (blank = use username)")
subj := fs.String("subject-regex", "", "inbound subject filter")
backlog := fs.Bool("process-backlog", false, "treat existing mail as new")
if err := fs.Parse(rest); err != nil {
return 2
}
if fs.NArg() > 0 {
fmt.Fprintf(errOut, "unexpected argument %q\n", fs.Arg(0))
return 2
}
if name == "" || *host == "" || *user == "" {
fmt.Fprintln(errOut, "name, --imap-host, and --username are required")
return 2
}
if err := tui.ValidFromAddress(*from); err != nil {
fmt.Fprintln(errOut, err)
return 2
}
acc := store.Account{
Name: name, Mode: *mode, IMAPHost: *host, IMAPPort: *port, IMAPSecurity: *sec,
AuthType: "password", Username: *user, Password: *pass,
FromAddress: *from, SubjectRegex: *subj, ProcessBacklog: *backlog,
}
if *mode == "RW" {
acc.SMTPHost, acc.SMTPPort, acc.SMTPSecurity = *smtpHost, *smtpPort, *smtpSec
}
if _, err := st.AddAccount(acc); err != nil {
fmt.Fprintf(errOut, "add account: %v\n", err)
return 1
}
fmt.Fprintf(out, "account %q added (%s)\n", name, *mode)
return 0
case "edit":
if len(rest) == 0 || strings.HasPrefix(rest[0], "-") {
fmt.Fprintln(errOut, "usage: emcli account edit <name> [flags]")
return 2
}
name := rest[0]
flagArgs := rest[1:]
if len(flagArgs) == 0 { // only name → interactive prefilled form
return editInteractive(st, name, out, errOut)
}
fs := flag.NewFlagSet("account edit", flag.ContinueOnError)
fs.SetOutput(errOut)
mode := fs.String("mode", "", "RO|RW")
host := fs.String("imap-host", "", "IMAP host")
port := fs.Int("imap-port", 0, "IMAP port")
sec := fs.String("imap-security", "", "tls|starttls")
smtpHost := fs.String("smtp-host", "", "SMTP host")
smtpPort := fs.Int("smtp-port", 0, "SMTP port")
smtpSec := fs.String("smtp-security", "", "tls|starttls")
user := fs.String("username", "", "login username")
pass := fs.String("password", "", "login password (blank keeps existing)")
from := fs.String("from", "", "send-as address (empty reverts to username)")
subj := fs.String("subject-regex", "", "inbound subject filter")
if err := fs.Parse(flagArgs); err != nil {
return 2
}
if fs.NArg() > 0 {
fmt.Fprintf(errOut, "unexpected argument %q\n", fs.Arg(0))
return 2
}
if err := tui.ValidFromAddress(*from); err != nil {
fmt.Fprintln(errOut, err)
return 2
}
acc, err := st.GetAccount(name)
if err != nil {
fmt.Fprintf(errOut, "edit: %v\n", err)
return 1
}
fs.Visit(func(f *flag.Flag) {
switch f.Name {
case "mode":
acc.Mode = *mode
case "imap-host":
acc.IMAPHost = *host
case "imap-port":
acc.IMAPPort = *port
case "imap-security":
acc.IMAPSecurity = *sec
case "smtp-host":
acc.SMTPHost = *smtpHost
case "smtp-port":
acc.SMTPPort = *smtpPort
case "smtp-security":
acc.SMTPSecurity = *smtpSec
case "username":
acc.Username = *user
case "password":
acc.Password = *pass
case "from":
acc.FromAddress = *from
case "subject-regex":
acc.SubjectRegex = *subj
}
})
if err := st.UpdateAccount(acc); err != nil {
fmt.Fprintf(errOut, "edit: %v\n", err)
return 1
}
fmt.Fprintf(out, "account %q updated\n", name)
return 0
case "remove":
if len(rest) == 0 || strings.HasPrefix(rest[0], "-") {
fmt.Fprintln(errOut, "usage: emcli account remove <name> [--yes]")
return 2
}
name := rest[0]
fs := flag.NewFlagSet("account remove", flag.ContinueOnError)
fs.SetOutput(errOut)
yes := fs.Bool("yes", false, "skip confirmation")
if err := fs.Parse(rest[1:]); err != nil {
return 2
}
if fs.NArg() > 0 {
fmt.Fprintf(errOut, "unexpected argument %q\n", fs.Arg(0))
return 2
}
if !*yes {
if !isatty.IsTerminal(os.Stdin.Fd()) {
fmt.Fprintf(errOut, "refusing to remove %q without --yes (no terminal for confirmation)\n", name)
return 2
}
if !confirmRemoval(name, out) {
fmt.Fprintln(out, "aborted")
return 1
}
}
if err := st.DeleteAccount(name); err != nil {
fmt.Fprintf(errOut, "remove: %v\n", err)
return 1
}
fmt.Fprintf(out, "account %q removed\n", name)
return 0
case "show":
return accountShow(st, rest, out, errOut)
case "list":
if len(rest) > 0 {
fmt.Fprintf(errOut, "unexpected argument %q\n", rest[0])
return 2
}
_, adminErr := crypto.AdminKeyFromEnv()
isAdmin := adminErr == nil
accs, err := st.ListAccounts()
if err != nil {
if isAdmin {
fmt.Fprintf(errOut, "list: %v\n", err)
} else {
_ = Failure(CodeDB, err.Error()).Write(out)
}
return 1
}
if !isAdmin {
items := make([]map[string]any, 0, len(accs))
for _, a := range accs {
items = append(items, map[string]any{
"name": a.Name, "from": a.SendFrom(), "can_send": a.Mode == "RW",
})
}
_ = Success(map[string]any{"accounts": items}).Write(out)
return 0
}
fmt.Fprintf(out, "%-16s %-4s %-28s %s\n", "NAME", "MODE", "IMAP", "USER")
for _, a := range accs {
fmt.Fprintf(out, "%-16s %-4s %-28s %s\n",
a.Name, a.Mode, fmt.Sprintf("%s:%d", a.IMAPHost, a.IMAPPort), a.Username)
}
return 0
default:
fmt.Fprintf(errOut, "unknown account subcommand %q (want add|edit|remove|show|list)\n", sub)
return 2
}
}
```
> Note: `case "show"` calls `accountShow`, which is implemented in Task 8. Until Task 8 lands, `go build` will fail on the undefined `accountShow`. Implement Tasks 7 and 8 back-to-back; do not run the full build between them. The Task 7 tests in Step 5 still pass because Go compiles the package as a whole once `accountShow` exists — so run Task 7's tests **after** Task 8 Step 3. (See Task 8.)
- [ ] **Step 5: (Deferred) — run after Task 8 Step 3**
Run: `go test ./internal/cli/ -run 'TestAccount'`
Expected: PASS.
- [ ] **Step 6: Commit (combined with Task 8)**
Commit happens at the end of Task 8 so the package always builds in committed history.
---
### Task 8: `account show`
**Files:**
- Create: `internal/cli/account_show.go`
- Test: `internal/cli/account_show_test.go`
**Interfaces:**
- Consumes: `store.GetAccount`, `store.Account` fields, `io.Writer`.
- Produces: `func accountShow(st *store.Store, rest []string, out, errOut io.Writer) int` — prints one account's config (mode, IMAP, SMTP, send-from, subject regex, whitelist enabled flags); never prints the password. Rejects missing/extra positional args (`account show <name>`).
- [ ] **Step 1: Write the failing test**
Create `internal/cli/account_show_test.go`:
```go
package cli
import (
"strings"
"testing"
)
func TestAccountShow(t *testing.T) {
adminEnv(t)
run(t, "account", "add", "shown", "--mode", "RW",
"--imap-host", "imap.x.com", "--imap-port", "993",
"--smtp-host", "smtp.x.com", "--smtp-port", "465",
"--username", "u@x.com", "--password", "secret", "--from", "me@x.com")
code, out, _ := run(t, "account", "show", "shown")
if code != 0 {
t.Fatalf("show exit=%d", code)
}
for _, want := range []string{"shown", "RW", "imap.x.com:993", "smtp.x.com:465", "u@x.com", "me@x.com"} {
if !strings.Contains(out, want) {
t.Fatalf("show missing %q:\n%s", want, out)
}
}
if strings.Contains(out, "secret") {
t.Fatalf("show must never print the password:\n%s", out)
}
}
func TestAccountShowMissingName(t *testing.T) {
adminEnv(t)
if code, _, _ := run(t, "account", "show"); code != 2 {
t.Fatal("show without a name must be a usage error")
}
if code, _, _ := run(t, "account", "show", "nope"); code == 0 {
t.Fatal("show of a missing account must be non-zero")
}
}
```
- [ ] **Step 2: Run test to verify it fails**
Run: `go test ./internal/cli/ -run 'TestAccountShow'`
Expected: FAIL (build error: `undefined: accountShow`).
- [ ] **Step 3: Implement `accountShow`**
Create `internal/cli/account_show.go`:
```go
package cli
import (
"fmt"
"io"
"git.dcglab.co.uk/steve/emcli/internal/store"
)
// accountShow renders one account's configuration. The password is never shown.
func accountShow(st *store.Store, rest []string, out, errOut io.Writer) int {
if len(rest) == 0 {
fmt.Fprintln(errOut, "usage: emcli account show <name>")
return 2
}
if len(rest) > 1 {
fmt.Fprintf(errOut, "unexpected argument %q\n", rest[1])
return 2
}
a, err := st.GetAccount(rest[0])
if err != nil {
fmt.Fprintf(errOut, "show: %v\n", err)
return 1
}
onOff := func(b bool) string {
if b {
return "enabled"
}
return "disabled"
}
smtp := "-"
if a.SMTPHost != "" {
smtp = fmt.Sprintf("%s:%d (%s)", a.SMTPHost, a.SMTPPort, a.SMTPSecurity)
}
subj := a.SubjectRegex
if subj == "" {
subj = "(none)"
}
fmt.Fprintf(out, "name: %s\n", a.Name)
fmt.Fprintf(out, "mode: %s\n", a.Mode)
fmt.Fprintf(out, "imap: %s:%d (%s)\n", a.IMAPHost, a.IMAPPort, a.IMAPSecurity)
fmt.Fprintf(out, "smtp: %s\n", smtp)
fmt.Fprintf(out, "username: %s\n", a.Username)
fmt.Fprintf(out, "send-from: %s\n", a.SendFrom())
fmt.Fprintf(out, "subject filter: %s\n", subj)
fmt.Fprintf(out, "inbound whitelist: %s\n", onOff(a.WhitelistInEnabled))
fmt.Fprintf(out, "outbound whitelist:%s\n", onOff(a.WhitelistOutEnabled))
return 0
}
```
- [ ] **Step 4: Run the Task 7 + Task 8 tests together**
Run: `go test ./internal/cli/ -run 'TestAccount'`
Expected: PASS (both `TestAccount*` from Task 7 and `TestAccountShow*` here).
- [ ] **Step 5: Commit Tasks 7 + 8 together**
```bash
go build ./...
git add internal/cli/admin.go internal/cli/account_show.go internal/cli/admin_test.go internal/cli/account_show_test.go go.mod go.sum
git commit -m "feat(cli): positional account grammar, account show, TTY remove confirm; drop whitelist flags"
```
---
### Task 9: `doctor` accepts a positional account (keeps `--account`)
**Files:**
- Modify: `internal/cli/run.go` (`runDoctor`, lines ~108-131)
- Test: `internal/cli/run_test.go` (add `TestDoctorPositionalAccount`)
**Interfaces:**
- Consumes: existing `openStore`, `newDepsLive`, `DoctorCmd`.
- Produces: `doctor [account]` and `doctor --account <name>` both work; giving both with different values is a usage error.
- [ ] **Step 1: Write the failing test**
The live `doctor` dials IMAP/SMTP, so the test asserts only argument handling: an unknown positional account must fail with a not-found-style error, not a flag-parse error, and must not require `--account`.
Add to `internal/cli/run_test.go`:
```go
func TestDoctorPositionalAccount(t *testing.T) {
adminEnv(t)
// No such account: doctor should reach account lookup and fail there (exit 1),
// proving the positional was accepted (not rejected as an unexpected arg).
code, _, errOut := run(t, "doctor", "ghost")
if code == 2 {
t.Fatalf("positional account must be accepted, got usage error: %q", errOut)
}
// Giving both positional and --account with different values is a usage error.
if code, _, _ := run(t, "doctor", "ghost", "--account", "other"); code != 2 {
t.Fatal("conflicting positional + --account must be a usage error")
}
}
```
(If `adminEnv` is defined only in `admin_test.go`, it is in the same `package cli` test binary and is available here — no import needed.)
- [ ] **Step 2: Run test to verify it fails**
Run: `go test ./internal/cli/ -run TestDoctorPositionalAccount`
Expected: FAIL (positional `ghost` currently rejected / `--account` only).
- [ ] **Step 3: Update `runDoctor`**
Replace the body of `runDoctor` in `internal/cli/run.go` (keep the function signature). Add `"strings"` to `run.go`'s imports if not present (it already imports `strings`).
```go
func runDoctor(args []string, role store.Role, out, errOut io.Writer) int {
// Accept a positional account (human form) or --account (agent form).
var positional string
rest := args
if len(args) > 0 && !strings.HasPrefix(args[0], "-") {
positional, rest = args[0], args[1:]
}
fs := flag.NewFlagSet("doctor", flag.ContinueOnError)
fs.SetOutput(errOut)
usageFlags(fs, "doctor", errOut)
accountFlag := fs.String("account", "", "check only this account")
if err := fs.Parse(rest); err != nil {
if errors.Is(err, flag.ErrHelp) {
return 0
}
return 2
}
if fs.NArg() > 0 {
fmt.Fprintf(errOut, "unexpected argument %q\n", fs.Arg(0))
return 2
}
account := *accountFlag
if positional != "" {
if account != "" && account != positional {
fmt.Fprintln(errOut, "give the account once, as a positional or --account, not both")
return 2
}
account = positional
}
st, err := openStore(role)
if err != nil {
fmt.Fprintf(errOut, "emcli: %v\n", err)
return 1
}
defer st.Close()
d := newDepsLive(st, out)
if err := DoctorCmd(d, account); err != nil {
return 1
}
return 0
}
```
- [ ] **Step 4: Run test to verify it passes**
Run: `go test ./internal/cli/ -run TestDoctorPositionalAccount`
Expected: PASS.
- [ ] **Step 5: Commit**
```bash
git add internal/cli/run.go internal/cli/run_test.go
git commit -m "feat(cli): doctor accepts positional account alongside --account"
```
---
### Task 10: top-level `ls` alias for the agent `list` command
**Files:**
- Modify: `internal/cli/run.go` (`Run`, the `switch cmd` around lines ~144-166)
- Test: `internal/cli/run_test.go` (add `TestTopLevelLsAlias`)
**Interfaces:**
- Consumes: `normalizeVerb` (Task 1).
- Produces: `emcli ls --account X` behaves exactly like `emcli list --account X`.
- [ ] **Step 1: Write the failing test**
Add to `internal/cli/run_test.go`:
```go
func TestTopLevelLsAlias(t *testing.T) {
adminEnv(t)
// `ls` with no --account must hit the same usage path as `list` (CodeUsage
// envelope on stdout, exit 2) — proving it routed to the agent list command.
code, out, _ := run(t, "ls")
if code != 2 || !strings.Contains(out, "account") {
t.Fatalf("ls should alias list (usage about --account): code=%d out=%q", code, out)
}
}
```
- [ ] **Step 2: Run test to verify it fails**
Run: `go test ./internal/cli/ -run TestTopLevelLsAlias`
Expected: FAIL (`unknown command "ls"`).
- [ ] **Step 3: Normalize the top-level command**
In `internal/cli/run.go` `Run`, change the dispatch so the command is normalized before the switch, and the agent path receives the canonical name. Replace:
```go
cmd, rest := args[0], args[1:]
role := commandRole(args)
switch cmd {
case "list", "get", "search", "ack":
return runAgent(cmd, rest, role, out, errOut)
```
with:
```go
cmd, rest := args[0], args[1:]
role := commandRole(args)
switch normalizeVerb(cmd) {
case "list", "get", "search", "ack":
return runAgent(normalizeVerb(cmd), rest, role, out, errOut)
```
Leave every other `case` (`send`, `account`, `whitelist`, `config`, `audit`, `doctor`, `init`, default) unchanged — `normalizeVerb` is a no-op for those tokens. `commandRole(args)` already returns `RoleAgent` for `ls` via its default branch, which is correct for `list`.
- [ ] **Step 4: Run test to verify it passes**
Run: `go test ./internal/cli/ -run TestTopLevelLsAlias`
Expected: PASS.
- [ ] **Step 5: Commit**
```bash
git add internal/cli/run.go internal/cli/run_test.go
git commit -m "feat(cli): top-level ls alias for list"
```
---
### Task 11: remove whitelist toggles from the interactive TUI account form
**Files:**
- Modify: `internal/tui/account.go` (struct field, row list, render, parse, `ToAccount`, `FieldsFromAccount`)
- Test: `internal/tui/account_test.go` (update any test asserting whitelist rows/fields; add a guard test)
**Interfaces:**
- Consumes: nothing new.
- Produces: the form no longer presents `Whitelist inbound`/`Whitelist outbound`; new accounts are created with both whitelist flags `false`. `process_backlog` row is retained.
- [ ] **Step 1: Inspect current usages**
Run: `grep -n "Whitelist\|whitelist" internal/tui/account.go internal/tui/*_test.go`
Confirm the sites: `Fields` struct (`WhitelistIn, WhitelistOut`), the `formFields`/rows slice (`whitelist_in`, `whitelist_out` rows ~147-148), the render `switch` (~192-195), the parse `switch` (~279-282), `ToAccount` (~93), `FieldsFromAccount` (~119-120).
- [ ] **Step 2: Write/adjust the failing test**
In `internal/tui/account_test.go`, add (and fix any existing test that counts fields or references whitelist rows):
```go
func TestFormHasNoWhitelistFields(t *testing.T) {
f := NewAccountForm(Fields{}, false)
out := f.View()
if strings.Contains(strings.ToLower(out), "whitelist") {
t.Fatalf("account form must not mention whitelists:\n%s", out)
}
}
```
Ensure the test file imports `"strings"`. (If `NewAccountForm`/`View` need a non-empty terminal to render, follow the pattern already used by existing tests in this file; if the form renders fields lazily, instead assert against the row-definition slice the file exposes.)
- [ ] **Step 3: Run test to verify it fails**
Run: `go test ./internal/tui/ -run TestFormHasNoWhitelistFields`
Expected: FAIL (form still renders whitelist rows).
- [ ] **Step 4: Remove the fields**
In `internal/tui/account.go`:
1. Struct: change `WhitelistIn, WhitelistOut, ProcessBacklog bool` (line ~27) to `ProcessBacklog bool`.
2. Row list (lines ~147-148): delete the two rows
`{key: "whitelist_in", label: "Whitelist inbound (y/n)", isBool: true},` and
`{key: "whitelist_out", label: "Whitelist outbound (y/n)", isBool: true},`.
3. Render `switch` (lines ~192-195): delete the `case "whitelist_in":` and `case "whitelist_out":` arms.
4. Parse `switch` (lines ~279-282): delete the `case "whitelist_in":` and `case "whitelist_out":` arms.
5. `ToAccount` (line ~93): remove `WhitelistInEnabled: f.WhitelistIn, WhitelistOutEnabled: f.WhitelistOut,` from the `store.Account{...}` literal (leave the flags at their zero value `false`).
6. `FieldsFromAccount` (lines ~119-120): remove the `WhitelistIn: a.WhitelistInEnabled,` and `WhitelistOut: a.WhitelistOutEnabled,` lines.
- [ ] **Step 5: Run tests to verify they pass**
Run: `go test ./internal/tui/`
Expected: PASS (the new guard test and all existing tui tests).
- [ ] **Step 6: Commit**
```bash
git add internal/tui/account.go internal/tui/account_test.go
git commit -m "feat(tui): drop whitelist toggles from account form (managed via whitelist group)"
```
---
### Task 12: rewrite help synopses + document the human/agent split + aliases
**Files:**
- Modify: `internal/cli/help.go` (`adminCmds` synopses; `printMainHelp` footer)
- Test: `internal/cli/help_test.go` (already exercises help; add an alias/synopsis assertion)
**Interfaces:**
- Consumes: nothing new.
- Produces: admin command synopses reflect the new grammar; main help notes the positional-vs-flag split and the aliases.
- [ ] **Step 1: Write the failing test**
Add to `internal/cli/help_test.go`:
```go
func TestHelpReflectsNewGrammar(t *testing.T) {
_, out, _ := run(t, "help", "whitelist")
if !strings.Contains(out, "<account>") || !strings.Contains(out, "--in") {
t.Fatalf("whitelist help should show positional account + --in/--out:\n%s", out)
}
_, out, _ = run(t, "help", "account")
if !strings.Contains(out, "show") {
t.Fatalf("account help should list the new show subcommand:\n%s", out)
}
_, mainOut, mainErr := run(t, "help")
if !strings.Contains(mainOut+mainErr, "rm") && !strings.Contains(mainOut+mainErr, "alias") {
t.Fatalf("main help should mention aliases:\n%s", mainOut+mainErr)
}
}
```
- [ ] **Step 2: Run test to verify it fails**
Run: `go test ./internal/cli/ -run TestHelpReflectsNewGrammar`
Expected: FAIL.
- [ ] **Step 3: Update `help.go`**
Replace the `adminCmds` slice entries for the changed commands with:
```go
var adminCmds = []cmdHelp{
{"init", "init", "Create the database and add the first account (interactive)."},
{"account", "account <add|edit|remove|show|list> [name] [flags]", "Manage accounts. `add`/`edit` take a positional name + field flags, or run with none for an interactive form."},
{"whitelist", "whitelist <add|remove|list|enable|disable> <account> [address…] --in|--out", "Manage inbound/outbound whitelists. Direction (--in/--out) is required."},
{"config", "config <list|get|set> [key] [value]", "List, get, or set global settings (e.g. audit_retention_days)."},
{"audit", "audit list [account] [--limit N]", "Show recent audit-log entries."},
{"doctor", "doctor [account]", "Check each account's IMAP/SMTP connectivity and auth."},
{"version", "version", "Print the emcli version."},
{"help", "help [command]", "Show this help, or detailed usage for one command."},
}
```
Then, in `printMainHelp`, before the final `Environment:` block, add:
```go
fmt.Fprint(w, "\nAliases: rm/del = remove, ls = list. Admin commands take positional\n")
fmt.Fprint(w, "operands (account/address/key); agent commands use flags (--account …).\n")
```
- [ ] **Step 4: Run the help tests to verify they pass**
Run: `go test ./internal/cli/ -run 'Help'`
Expected: PASS (`TestHelpReflectsNewGrammar` plus the existing help tests, which only assert command names are present).
- [ ] **Step 5: Commit**
```bash
git add internal/cli/help.go internal/cli/help_test.go
git commit -m "docs(cli): help reflects positional admin grammar + aliases"
```
---
### Task 13: update user-facing docs + full verification
**Files:**
- Modify: `README.md` (getting-started: no whitelist at init)
- Modify: `USER-MANUAL.md` (admin command reference → new grammar)
- Modify: `skills/emcli/AGENTIC-MANUAL.md` (verify `doctor --account` still documented as valid; no admin-grammar examples need changing since agents don't run admin commands)
- Modify: `skills/emcli/references/commands.md` (whitelist references, if they show invocation syntax)
**Interfaces:** none (documentation + verification task).
- [ ] **Step 1: Find every old-grammar example**
Run:
```bash
grep -rn "whitelist .* --account\|whitelist .* --address\|account .* --name\|--whitelist-in\|--whitelist-out\|config set\|config get" README.md USER-MANUAL.md skills/
```
Review each hit. Update admin examples to the new grammar:
- `account add --name X --imap-host …``account add X --imap-host …`
- `account remove --name X --yes``account remove X --yes`
- `whitelist in add --account X --address A``whitelist add X A --in`
- `whitelist out list --account X``whitelist list X --out`
- Add mention of `account show`, `config list`, and `whitelist enable/disable`.
- Confirm `emcli doctor --account gmail` in `AGENTIC-MANUAL.md` is left intact (still valid).
- [ ] **Step 2: Apply the edits**
Edit each file so every admin example uses the positional grammar and the new subcommands. Keep agent-command examples (`list`/`get`/`search`/`ack`/`send`/`doctor --account`) unchanged.
- [ ] **Step 3: Full build, vet, and test**
Run:
```bash
go build ./...
go vet ./...
go test ./...
```
Expected: build clean, vet clean, all tests PASS.
- [ ] **Step 4: Manual smoke test (optional but recommended)**
```bash
export EMCLI_ADMIN_KEY="$(head -c 32 /dev/urandom | base64)"
export EMCLI_KEY="$(head -c 32 /dev/urandom | base64)"
export EMCLI_DB="$(mktemp -d)/emcli.db"
go run ./cmd/emcli init # (Ctrl-C out of the form, or add a dummy account)
go run ./cmd/emcli account add bobby --imap-host imap.x.com --username u@x.com
go run ./cmd/emcli whitelist add bobby a@x.com @y.com --in
go run ./cmd/emcli whitelist list bobby --in
go run ./cmd/emcli whitelist enable bobby --in
go run ./cmd/emcli account show bobby
go run ./cmd/emcli config list
go run ./cmd/emcli whitelist add bobby "Tk555@protonmail.com" # must now ERROR (missing --in), not insert a blank row
```
Expected: the final command errors with a missing-direction usage message — the original bug is gone.
- [ ] **Step 5: Commit**
```bash
git add README.md USER-MANUAL.md skills/
git commit -m "docs: update admin command reference to positional grammar"
```
---
## Self-Review
**Spec coverage:**
- Unifying grammar (positional operands, flags for modifiers) → Tasks 59.
- Safety rule "no silently-ignored input" (leftover-arg rejection) → Tasks 5,6,7,8,9 (each `NArg()>0`/`len(rest)>0` guard).
- Safety rule "no empty primary operands" → Tasks 6,7,8 (account/address required checks).
- Verb aliases rm/del/ls → Task 1 + applied in Tasks 5,6,7 + top-level Task 10.
- whitelist required `--in`/`--out`, multi-address, validation, enable/disable, empty-list warning → Tasks 2,4,6.
- whitelist decoupled from onboarding (flags removed from `account add`, init flow, TUI form) → Tasks 7,11.
- config known-key registry + `config list` + unknown-key rejection → Tasks 3,5.
- `account show` → Task 8. `account remove` TTY confirm + `--yes` → Task 7.
- `doctor` positional + `--account` exception → Task 9.
- Frozen agent commands untouched → no task modifies `runAgent`/`runSend`/their flags; verified by unchanged `agent_test.go`/`send_test.go` in Task 13 Step 3.
- Help/docs → Tasks 12,13.
**Placeholder scan:** No TBD/TODO/"add validation"-style placeholders; every code step shows complete code.
**Type consistency:** `normalizeVerb` (Task 1) used identically in Tasks 5,6,7,10. `policy.ValidWhitelistAddress` (Task 2) consumed in Task 6. `settingsRegistry`/`settingKeys` (Task 3) consumed in Task 5. `store.SetWhitelistEnabled(account, dir, enabled)` (Task 4) consumed in Task 6. `accountShow(st, rest, out, errOut)` defined in Task 8, called in Task 7 — cross-task build dependency is called out explicitly in Task 7's note and Task 8's combined commit.
**Known cross-task build ordering:** Task 7 references `accountShow` (Task 8) and is committed together with Task 8 so committed history always builds. Task 5's `strconv` import removal is conditional on the compiler. Both are flagged inline.