From 2c7b8d3610f0fef5d131458df043dffcb62c1287 Mon Sep 17 00:00:00 2001 From: Steve Cliff Date: Sat, 27 Jun 2026 11:39:03 +0100 Subject: [PATCH] docs: implementation plan for human CLI grammar redesign Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-06-27-human-cli-grammar-redesign.md | 1662 +++++++++++++++++ ...06-27-human-cli-grammar-redesign-design.md | 10 +- 2 files changed, 1670 insertions(+), 2 deletions(-) create mode 100644 docs/superpowers/plans/2026-06-27-human-cli-grammar-redesign.md diff --git a/docs/superpowers/plans/2026-06-27-human-cli-grammar-redesign.md b/docs/superpowers/plans/2026-06-27-human-cli-grammar-redesign.md new file mode 100644 index 0000000..5652c42 --- /dev/null +++ b/docs/superpowers/plans/2026-06-27-human-cli-grammar-redesign.md @@ -0,0 +1,1662 @@ +# 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 [--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 `). + +- [ ] **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 ", "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 ") +// 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 ` / `config set ` 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 ` 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 ") + 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 ") + 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 [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 +// [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 [field flags]`, `account remove [--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 `. 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 [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 [--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 `). + +- [ ] **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 ") + 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 ` 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, "") || !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 [name] [flags]", "Manage accounts. `add`/`edit` take a positional name + field flags, or run with none for an interactive form."}, + {"whitelist", "whitelist [address…] --in|--out", "Manage inbound/outbound whitelists. Direction (--in/--out) is required."}, + {"config", "config [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 5–9. +- 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. diff --git a/docs/superpowers/specs/2026-06-27-human-cli-grammar-redesign-design.md b/docs/superpowers/specs/2026-06-27-human-cli-grammar-redesign-design.md index a744f0c..8d5a2c3 100644 --- a/docs/superpowers/specs/2026-06-27-human-cli-grammar-redesign-design.md +++ b/docs/superpowers/specs/2026-06-27-human-cli-grammar-redesign-design.md @@ -131,8 +131,11 @@ booleans, default `false`, read fresh on every command via `Deps.setup` (which r only consult them at evaluation time. Nothing in account creation, `init`, or the policy engine needs them set at creation time, and toggling them later takes effect on the next command. -Therefore: remove `--whitelist-in` / `--whitelist-out` from `account add` and from `init`'s flow -entirely. Accounts are created with whitelists off; the user sets them up afterward: +Therefore: remove `--whitelist-in` / `--whitelist-out` from `account add`, from `init`'s flow, and +from the interactive TUI account form (the `Whitelist inbound`/`Whitelist outbound` toggles in +`internal/tui/account.go`) entirely — leaving them in the form would contradict the decoupling and +re-expose the empty-enabled-whitelist footgun. Accounts are created with whitelists off; the user +sets them up afterward: ``` emcli whitelist add bobby alice@example.com bob@example.com --in @@ -168,6 +171,9 @@ it. (Addresses are still lowercased on store as today.) leftover-positional and empty-operand guards; wire `normalizeVerb`. - `internal/cli/run.go` — `doctor` accepts positional + `--account`; routing unchanged otherwise. - `internal/cli/interactive.go` (`runInit`) — drop whitelist references. +- `internal/tui/account.go` — remove the `WhitelistIn`/`WhitelistOut` form fields, their rows, + and their render/parse/`ToAccount`/`FieldsFromAccount` handling (new accounts are created with + whitelists off; managed via the `whitelist` group thereafter). - `internal/cli/help.go` — rewrite admin synopses to the new grammar; document the agent-vs-human positional/flag split; list aliases. - New: settings registry (small map + validators) — location `internal/cli` (e.g.