From 93c609b03e4fcbf5f0735fa3e5bc56a6443b5d45 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Wed, 30 Sep 2026 19:32:57 -0500 Subject: [PATCH 01/10] Fix(cli): keep help descriptions in their column and explain what vault remove can forget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Names wider than the help's command column ran straight into their descriptions (`""Answer in a thread`, `Follow the desktop…`). Such a name now takes a line of its own and the description starts at the column, so every row stays aligned. `zn vault list` shows the desktop app's vaults and servers next to zn's own without saying so, and `zn vault remove` could only forget zn's own; its error pointed back at the list. The list now marks each entry's source (terminal or app, also in the JSON) and says which commands act on which. `vault remove`, `disconnect` and `use` explain that an entry belongs to the desktop app, suggest the one saved name containing the word that was typed, and list zn's own names. `vault remove` of a server also drops its token, as `disconnect` does, supports --json, and completes zn's saved names. `zn use ` adopts that folder instead of failing. --- internal/cli/commands_vault_capture.go | 51 +++++++-- internal/cli/commands_workspace.go | 105 +++++++++++++++--- internal/cli/completion.go | 5 + internal/cli/help.go | 26 +++-- internal/cli/help_test.go | 87 +++++++++++++++ internal/cli/vault_remove_test.go | 143 +++++++++++++++++++++++++ 6 files changed, 386 insertions(+), 31 deletions(-) create mode 100644 internal/cli/help_test.go create mode 100644 internal/cli/vault_remove_test.go diff --git a/internal/cli/commands_vault_capture.go b/internal/cli/commands_vault_capture.go index f4a286d..687499d 100644 --- a/internal/cli/commands_vault_capture.go +++ b/internal/cli/commands_vault_capture.go @@ -65,21 +65,44 @@ func cmdVaultInfo(ctx context.Context, b backend.Backend, args Args) error { } // vaultListEntry keeps `root` for local entries so scripts reading the JSON -// keep working now that servers appear in the same list. +// keep working now that servers appear in the same list. Source says who +// saved the entry: "terminal" for zn's own list (what `zn use` and `zn vault +// remove` act on), "app" for the ZenNotes desktop app's, which zn only reads. type vaultListEntry struct { Name string `json:"name"` Kind string `json:"kind"` + Source string `json:"source"` Root string `json:"root,omitempty"` BaseURL string `json:"baseUrl,omitempty"` LastOpenedAt *int64 `json:"lastOpenedAt"` IsDefault bool `json:"isDefault"` } +const ( + sourceTerminal = "terminal" + sourceApp = "app" +) + +// desktopWorkspaces is the desktop app's vault and server list in the shape +// of zn's own, so the same name/path/URL/host matching applies to both. +func desktopWorkspaces() config.Workspaces { + ws := config.Workspaces{} + for _, v := range config.KnownVaults() { + ws.Vaults = append(ws.Vaults, config.LocalWorkspace{Name: v.Name, Root: v.Root}) + } + for _, p := range config.RemoteProfiles() { + ws.Servers = append(ws.Servers, config.ServerWorkspace{Name: p.Name, URL: p.BaseURL}) + } + return ws +} + func cmdVaultList(args Args) error { source, err := backend.ResolveWorkspaceSource(args.Str("workspace-source")) if err != nil { return err } + // Every listed name must resolve through `--vault ` in this + // source, and a desktop-managed zn never resolves zn's own names. ws := config.Workspaces{} if source == "terminal" { ws = config.LoadWorkspaces() @@ -99,23 +122,23 @@ func cmdVaultList(args Args) error { seenURL := map[string]bool{} for _, v := range ws.Vaults { seenRoot[filepath.Clean(v.Root)] = true - entries = append(entries, vaultListEntry{Name: v.Name, Kind: "local", Root: v.Root, IsDefault: isDefault("local", v.Root, "")}) + entries = append(entries, vaultListEntry{Name: v.Name, Kind: "local", Source: sourceTerminal, Root: v.Root, IsDefault: isDefault("local", v.Root, "")}) } for _, s := range ws.Servers { - seenURL[strings.ToLower(s.URL)] = true - entries = append(entries, vaultListEntry{Name: s.Name, Kind: "remote", BaseURL: s.URL, IsDefault: isDefault("remote", "", s.URL)}) + seenURL[strings.ToLower(strings.TrimRight(s.URL, "/"))] = true + entries = append(entries, vaultListEntry{Name: s.Name, Kind: "remote", Source: sourceTerminal, BaseURL: s.URL, IsDefault: isDefault("remote", "", s.URL)}) } for _, v := range config.KnownVaults() { if seenRoot[filepath.Clean(v.Root)] { continue } - entries = append(entries, vaultListEntry{Name: v.Name, Kind: "local", Root: v.Root, LastOpenedAt: v.LastOpenedAt, IsDefault: isDefault("local", v.Root, "")}) + entries = append(entries, vaultListEntry{Name: v.Name, Kind: "local", Source: sourceApp, Root: v.Root, LastOpenedAt: v.LastOpenedAt, IsDefault: isDefault("local", v.Root, "")}) } for _, p := range config.RemoteProfiles() { - if seenURL[strings.ToLower(p.BaseURL)] { + if seenURL[strings.ToLower(strings.TrimRight(p.BaseURL, "/"))] { continue } - entries = append(entries, vaultListEntry{Name: p.Name, Kind: "remote", BaseURL: p.BaseURL, LastOpenedAt: p.LastConnectedAt, IsDefault: isDefault("remote", "", p.BaseURL)}) + entries = append(entries, vaultListEntry{Name: p.Name, Kind: "remote", Source: sourceApp, BaseURL: p.BaseURL, LastOpenedAt: p.LastConnectedAt, IsDefault: isDefault("remote", "", p.BaseURL)}) } if args.Bool("json") { emitJSON(entries) @@ -127,11 +150,15 @@ func cmdVaultList(args Args) error { } nameWidth := 4 kindWidth := 0 + hasApp := false for _, e := range entries { nameWidth = max(nameWidth, len([]rune(e.Name))) if e.Kind == "remote" { kindWidth = 6 } + if e.Source == sourceApp { + hasApp = true + } } for _, e := range entries { marker := " " @@ -146,11 +173,19 @@ func cmdVaultList(args Args) error { if kindWidth > 0 { kind = pad(e.Kind, kindWidth) + " " } + source := "" + if hasApp { + source = pad(e.Source, len(sourceTerminal)) + " " + } location := e.Root if location == "" { location = e.BaseURL } - emitLine(fmt.Sprintf("%s %s %s%s %s", marker, pad(e.Name, nameWidth), kind, pad(age, 8), location)) + emitLine(fmt.Sprintf("%s %s %s%s%s %s", marker, pad(e.Name, nameWidth), kind, source, pad(age, 8), location)) + } + if hasApp { + emitLine("") + emitLine("Entries marked app are saved by the ZenNotes desktop app and managed there. `zn use`, `zn disconnect` and `zn vault remove` act on zn's own (terminal) entries.") } return nil } diff --git a/internal/cli/commands_workspace.go b/internal/cli/commands_workspace.go index 6d8b88d..0eaeea8 100644 --- a/internal/cli/commands_workspace.go +++ b/internal/cli/commands_workspace.go @@ -153,7 +153,7 @@ func cmdDisconnect(args Args) error { } saved := ws.FindServer(name) if saved == nil { - return fmt.Errorf("No saved server named %q. `zn vault list` shows them.", name) + return notSavedError(ws, name, "server") } url := saved.URL ws.Remove(saved.Name) @@ -169,6 +169,53 @@ func cmdDisconnect(args Args) error { return nil } +// notSavedError explains why a selector matched nothing zn saved. `zn vault +// list` also shows the desktop app's entries, so a name copied from there +// gets told who owns it instead of a bare "no such name". +func notSavedError(ws config.Workspaces, selector, noun string) error { + desktop := desktopWorkspaces() + if v := desktop.FindVault(selector); v != nil { + return fmt.Errorf("%q is a vault saved by the ZenNotes desktop app, not by zn; remove it in the app's vault switcher. zn only lists it.%s", v.Name, savedNamesHint(ws)) + } + if s := desktop.FindServer(selector); s != nil { + return fmt.Errorf("%q is a server saved by the ZenNotes desktop app, not by zn; remove it in the app's server settings. zn only lists it.%s", s.Name, savedNamesHint(ws)) + } + hint := savedNamesHint(ws) + if guess := containingName(selector, append(ws.Names(), desktop.Names()...)); guess != "" { + hint = fmt.Sprintf(" Did you mean %q?%s", guess, hint) + } + return fmt.Errorf("No saved %s named %q.%s", noun, selector, hint) +} + +// savedNamesHint names what zn itself saved, which is what `zn use`, +// `zn disconnect` and `zn vault remove` accept. +func savedNamesHint(ws config.Workspaces) string { + names := ws.Names() + if len(names) == 0 { + return " zn has no saved vaults or servers of its own; `zn init`, `zn vault add` and `zn connect` save one." + } + return " zn's saved entries are: " + strings.Join(names, ", ") + ". A server's URL or host works too." +} + +// containingName is the one saved name containing the selector as a word, +// for `zn vault remove workspace` against "workspace (notes.example.com)". +func containingName(selector string, names []string) string { + needle := strings.ToLower(strings.TrimSpace(selector)) + if needle == "" { + return "" + } + match := "" + for _, name := range names { + if strings.Contains(strings.ToLower(name), needle) { + if match != "" { + return "" + } + match = name + } + } + return match +} + // cmdUse makes a saved vault or server the default; `app` follows the // desktop app again; a path or URL is saved first. func cmdUse(ctx context.Context, args Args) error { @@ -186,16 +233,32 @@ func cmdUse(ctx context.Context, args Args) error { return nil } if _, ok := backend.TargetForWorkspace(ws, sel, ""); !ok { + desktop := desktopWorkspaces() switch { case backend.LooksLikeServerURL(sel): return cmdConnect(ctx, args) + case desktop.FindVault(sel) != nil: + // A vault the desktop app knows: save its folder under the same + // name so the terminal can keep using it by that name. + v := desktop.FindVault(sel) + ws.AddVault(v.Name, v.Root) + sel = v.Root + case desktop.FindServer(sel) != nil: + // The app keeps that server's token where zn cannot read it, so + // this is a connect: it asks for the token once. + s := desktop.FindServer(sel) + args.Positionals = []string{s.URL} + if args.Str("name") == "" { + args.push("name", s.Name) + } + return cmdConnect(ctx, args) default: root, err := filepath.Abs(config.ExpandHome(sel)) if err != nil { return err } if info, err := os.Stat(root); err != nil || !info.IsDir() { - return fmt.Errorf("%q is neither a saved name nor a folder. `zn vault list` shows the names.", sel) + return fmt.Errorf("%v `zn use` also takes a folder path or a server URL.", notSavedError(ws, sel, "vault or server")) } ws.AddVault("", root) } @@ -250,30 +313,38 @@ func cmdVaultAdd(args Args) error { return nil } -// cmdVaultRemove forgets a saved vault or server (files stay untouched). +// cmdVaultRemove forgets a saved vault or server. Note files stay; a +// server's token goes with it, as with `zn disconnect`. func cmdVaultRemove(args Args) error { name := strings.TrimSpace(args.Positional(0)) if name == "" { - return errors.New("Usage: zn vault remove ") + return errors.New("Usage: zn vault remove (see `zn vault list`)") } ws := config.LoadWorkspaces() - kind, ok := ws.Remove(name) - if !ok { - if v := ws.FindVault(name); v != nil { - kind, _ = ws.Remove(v.Name) - ok = true - } else if s := ws.FindServer(name); s != nil { - kind, _ = ws.Remove(s.Name) - ok = true - } - } - if !ok { - return fmt.Errorf("No saved vault or server named %q. `zn vault list` shows them.", name) + var kind, saved, location string + if v := ws.FindVault(name); v != nil { + kind, saved, location = "local", v.Name, v.Root + } else if s := ws.FindServer(name); s != nil { + kind, saved, location = "remote", s.Name, s.URL + } else { + return notSavedError(ws, name, "vault or server") } + ws.Remove(saved) if err := config.SaveWorkspaces(ws); err != nil { return err } - emitOK(fmt.Sprintf("Forgot %s (%s). Nothing on disk changed.", name, kind)) + if kind == "remote" { + _ = config.DeleteToken(location) + } + if args.Bool("json") { + emitJSON(map[string]any{"ok": true, "name": saved, "kind": kind, "location": location}) + return nil + } + if kind == "remote" { + emitOK(fmt.Sprintf("Forgot server %s (%s) and its token. Nothing on the server changed.", saved, location)) + return nil + } + emitOK(fmt.Sprintf("Forgot vault %s (%s). The notes are still there.", saved, location)) return nil } diff --git a/internal/cli/completion.go b/internal/cli/completion.go index b0c4e8e..c2c320d 100644 --- a/internal/cli/completion.go +++ b/internal/cli/completion.go @@ -59,11 +59,16 @@ func completionCandidates(words []string) []string { if len(before) > 0 { previous = before[len(before)-1] } + if len(before) >= 2 && before[len(before)-2] == "vault" && (previous == "remove" || previous == "rm") { + previous = "vault remove" + } switch previous { case "--server", "disconnect": for _, s := range config.LoadWorkspaces().Servers { candidates = append(candidates, s.Name) } + case "vault remove": + candidates = config.LoadWorkspaces().Names() case "--vault", "use": candidates = config.LoadWorkspaces().Names() for _, v := range config.KnownVaults() { diff --git a/internal/cli/help.go b/internal/cli/help.go index 28381a7..ff110f7 100644 --- a/internal/cli/help.go +++ b/internal/cli/help.go @@ -13,7 +13,9 @@ import ( const ( terminalColumnsFallback = 80 terminalColumnsCap = 100 - commandColumnWidth = 26 + commandColumnWidth = 28 + // columnGap is the least space between a row's name and its description. + columnGap = 2 ) var ansiRe = regexp.MustCompile(`\x1b\[[0-9;]*m`) @@ -78,7 +80,7 @@ var helpSections = []helpSection{ {"disconnect [name]", "Forget a saved server and its token", "--json"}, {"use ", "Set the terminal's saved default; app follows desktop again. Desktop-installed commands keep following desktop unless --workspace-source terminal is set", "--json"}, {"vault add ", "Remember an existing folder as a vault", "--name --no-default --json"}, - {"vault remove ", "Forget a saved vault or server (files stay)", ""}, + {"vault remove ", "Forget a saved vault or server (its token too; note files stay)", "--json"}, {"vault info", "Vault path (or server) + per-folder counts", "--json"}, {"vault list", "Known vaults and servers; the default is marked with *", "--json"}, {"vault mode [root|inbox]", "Show or switch the notes layout (moves the notes, rewrites favorites)", "--json"}, @@ -262,15 +264,27 @@ func (s helpStyle) header(width int) []string { func (s helpStyle) section(heading string, rows []helpRow, width int) []string { out := []string{s.bold(s.yellow(heading))} descWidth := width - commandColumnWidth - 2 + indent := " " + strings.Repeat(" ", commandColumnWidth) for _, row := range rows { descLines := wrapLines(row.description, descWidth) - out = append(out, " "+padVisible(s.magenta(row.name), commandColumnWidth)+descLines[0]) - for _, cont := range descLines[1:] { - out = append(out, " "+strings.Repeat(" ", commandColumnWidth)+cont) + name := " " + s.magenta(row.name) + if visibleLen(row.name)+columnGap > commandColumnWidth { + // A name too wide for the column gets a line of its own; the + // description still starts at the column, so nothing is glued + // to it and the rows below stay aligned. + out = append(out, name) + for _, line := range descLines { + out = append(out, indent+line) + } + } else { + out = append(out, padVisible(name, len(indent))+descLines[0]) + for _, cont := range descLines[1:] { + out = append(out, indent+cont) + } } if row.flags != "" { for _, line := range wrapLines(row.flags, descWidth) { - out = append(out, " "+strings.Repeat(" ", commandColumnWidth)+s.dim(s.cyan(line))) + out = append(out, indent+s.dim(s.cyan(line))) } } } diff --git a/internal/cli/help_test.go b/internal/cli/help_test.go new file mode 100644 index 0000000..0187988 --- /dev/null +++ b/internal/cli/help_test.go @@ -0,0 +1,87 @@ +package cli + +import ( + "strings" + "testing" +) + +// checkHelpRows asserts every row keeps its description in the description +// column: either on the same line after at least columnGap spaces, or, for a +// name too wide for the column, on the next line at the column. A name must +// never run straight into its description (`""Answer in a thread`). +func checkHelpRows(t *testing.T, lines []string, rows []helpRow) { + t.Helper() + indent := " " + strings.Repeat(" ", commandColumnWidth) + for _, row := range rows { + prefix := " " + row.name + idx := -1 + for i, line := range lines { + if line == prefix || strings.HasPrefix(line, prefix+" ") { + idx = i + break + } + } + if idx < 0 { + t.Errorf("%q: not rendered as its own row (glued to its description?)", row.name) + continue + } + firstWord := strings.Fields(row.description)[0] + rest := strings.TrimPrefix(lines[idx], prefix) + if strings.TrimSpace(rest) == "" { + if idx+1 >= len(lines) || !strings.HasPrefix(lines[idx+1], indent+firstWord) { + t.Errorf("%q: description should start on the next line at column %d, got %q", row.name, len(indent), lines[idx+1]) + } + continue + } + column := len(lines[idx]) - len(strings.TrimLeft(rest, " ")) + if column != len(indent) || !strings.HasPrefix(strings.TrimLeft(rest, " "), firstWord) { + t.Errorf("%q: description starts at column %d, want %d: %q", row.name, column, len(indent), lines[idx]) + } + if len(rest)-len(strings.TrimLeft(rest, " ")) < columnGap { + t.Errorf("%q: fewer than %d spaces before the description: %q", row.name, columnGap, lines[idx]) + } + } +} + +func TestHelpRowsNeverGlueNameToDescription(t *testing.T) { + t.Setenv("NO_COLOR", "1") + t.Setenv("FORCE_COLOR", "") + rows := append([]helpRow{}, globalFlags...) + rows = append(rows, environmentRows...) + for _, sec := range helpSections { + rows = append(rows, sec.rows...) + } + checkHelpRows(t, strings.Split(RenderHelp(nil), "\n"), rows) + + // Scoped help renders the global flags through the same layout. + for _, name := range []string{"comment reply", "list", "server"} { + lines := strings.Split(RenderScopedHelp(name, nil), "\n") + start := -1 + for i, line := range lines { + if line == "GLOBAL FLAGS" { + start = i + } + } + if start < 0 { + t.Fatalf("scoped help for %q has no GLOBAL FLAGS section", name) + } + checkHelpRows(t, lines[start:], globalFlags) + } +} + +func TestHelpWrapsLongDescriptionsAtTheColumn(t *testing.T) { + t.Setenv("NO_COLOR", "1") + t.Setenv("FORCE_COLOR", "") + out := RenderHelp(nil) + indent := " " + strings.Repeat(" ", commandColumnWidth) + for _, line := range strings.Split(out, "\n") { + if strings.HasPrefix(line, indent+" ") { + t.Errorf("continuation line indented past the column: %q", line) + } + } + for _, glued := range []string{`""Answer`, `""Start`, `Resolve`, `Follow`} { + if strings.Contains(out, glued) { + t.Errorf("help still glues a name to its description: %q", glued) + } + } +} diff --git a/internal/cli/vault_remove_test.go b/internal/cli/vault_remove_test.go new file mode 100644 index 0000000..e728daa --- /dev/null +++ b/internal/cli/vault_remove_test.go @@ -0,0 +1,143 @@ +package cli + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/ZenNotes/tui/internal/config" +) + +// The desktop app's vaults and servers appear in `zn vault list` next to +// zn's own, but only zn's own can be forgotten here. A name copied from the +// list must be told who owns it and what zn would accept instead. +func TestVaultRemoveExplainsDesktopEntriesAndListsZnsOwn(t *testing.T) { + dir := isolatedCLI(t) + root := filepath.Join(dir, "notes") + if err := os.MkdirAll(root, 0o755); err != nil { + t.Fatal(err) + } + app, _ := json.Marshal(map[string]any{ + "workspaceMode": "remote", + "localVaults": []map[string]any{{"name": "vault", "root": root, "lastOpenedAt": 1700000000000}}, + "remoteWorkspaceProfiles": []map[string]any{{ + "name": "workspace (zennotes.mydomain.com)", "baseUrl": "https://zennotes.mydomain.com", "lastConnectedAt": 1700000000000, + }}, + }) + if err := os.WriteFile(filepath.Join(config.UserDataDir(), config.AppConfigFile), app, 0o600); err != nil { + t.Fatal(err) + } + ws := config.LoadWorkspaces() + ws.AddVault("alpha", filepath.Join(dir, "alpha")) + ws.AddServer("", "https://zennotes.mydomain.com") + if err := config.SaveWorkspaces(ws); err != nil { + t.Fatal(err) + } + if err := config.SaveToken("https://zennotes.mydomain.com", "secret"); err != nil { + t.Fatal(err) + } + + out := captureOutput(t, func() { _ = Main([]string{"vault", "list"}) }) + for _, want := range []string{"alpha", "terminal", "vault", "app", "Entries marked app"} { + if !strings.Contains(out, want) { + t.Errorf("vault list should show %q:\n%s", want, out) + } + } + out = captureOutput(t, func() { _ = Main([]string{"vault", "list", "--json"}) }) + var entries []vaultListEntry + if err := json.Unmarshal([]byte(out), &entries); err != nil { + t.Fatal(err) + } + sources := map[string]string{} + for _, e := range entries { + sources[e.Name] = e.Source + } + if sources["alpha"] != "terminal" || sources["zennotes.mydomain.com"] != "terminal" || sources["vault"] != "app" { + t.Fatalf("sources: %v", sources) + } + if _, listed := sources["workspace (zennotes.mydomain.com)"]; listed { + t.Fatal("the desktop profile for a URL zn saved must not be listed twice") + } + + // Kevin's attempts, in order. + code := 0 + out = captureOutput(t, func() { code = Main([]string{"vault", "remove", "workspace"}) }) + if code != 1 || !strings.Contains(out, `Did you mean "workspace (zennotes.mydomain.com)"`) || !strings.Contains(out, "zn's saved entries are: alpha, zennotes.mydomain.com") { + t.Fatalf("partial name: exit %d %s", code, out) + } + out = captureOutput(t, func() { code = Main([]string{"vault", "remove", "vault"}) }) + if code != 1 || !strings.Contains(out, "saved by the ZenNotes desktop app") { + t.Fatalf("desktop vault: exit %d %s", code, out) + } + out = captureOutput(t, func() { code = Main([]string{"vault", "remove", "zennotes.mydomain.com", "--json"}) }) + if code != 0 || !strings.Contains(out, `"kind": "remote"`) { + t.Fatalf("remove by host: exit %d %s", code, out) + } + if config.LoadWorkspaces().FindServer("zennotes.mydomain.com") != nil { + t.Fatal("server still saved") + } + if config.LoadToken("https://zennotes.mydomain.com") != "" { + t.Fatal("vault remove must drop the server's token like disconnect does") + } + // Now only the desktop profile knows that URL. + out = captureOutput(t, func() { code = Main([]string{"vault", "remove", "zennotes.mydomain.com"}) }) + if code != 1 || !strings.Contains(out, "server saved by the ZenNotes desktop app") { + t.Fatalf("desktop server by host: exit %d %s", code, out) + } + out = captureOutput(t, func() { code = Main([]string{"vault", "remove", "alpha"}) }) + if code != 0 || !strings.Contains(out, "Forgot vault alpha") { + t.Fatalf("remove vault: exit %d %s", code, out) + } + out = captureOutput(t, func() { code = Main([]string{"vault", "remove", "alpha"}) }) + if code != 1 || !strings.Contains(out, "zn has no saved vaults or servers of its own") { + t.Fatalf("nothing left: exit %d %s", code, out) + } + out = captureOutput(t, func() { code = Main([]string{"disconnect", "workspace (zennotes.mydomain.com)"}) }) + if code != 1 || !strings.Contains(out, "saved by the ZenNotes desktop app") { + t.Fatalf("disconnect desktop profile: exit %d %s", code, out) + } +} + +// `zn use ` adopts the folder the app knows under that +// name instead of failing with "neither a saved name nor a folder". +func TestUseAdoptsADesktopVaultByName(t *testing.T) { + dir := isolatedCLI(t) + root := filepath.Join(dir, "desktop-notes") + if err := os.MkdirAll(root, 0o755); err != nil { + t.Fatal(err) + } + app, _ := json.Marshal(map[string]any{"localVaults": []map[string]any{{"name": "Desk", "root": root}}}) + if err := os.WriteFile(filepath.Join(config.UserDataDir(), config.AppConfigFile), app, 0o600); err != nil { + t.Fatal(err) + } + code := 0 + out := captureOutput(t, func() { code = Main([]string{"use", "desk", "--json"}) }) + if code != 0 || !strings.Contains(out, `"default": "Desk"`) { + t.Fatalf("use: exit %d %s", code, out) + } + if v := config.LoadWorkspaces().FindVault("Desk"); v == nil || v.Root != root { + t.Fatalf("not adopted: %+v", config.LoadWorkspaces()) + } + out = captureOutput(t, func() { code = Main([]string{"use", "nothing-like-it"}) }) + if code != 1 || !strings.Contains(out, "zn's saved entries are: Desk") || !strings.Contains(out, "folder path or a server URL") { + t.Fatalf("unknown: exit %d %s", code, out) + } +} + +func TestVaultRemoveCompletesZnsOwnNames(t *testing.T) { + dir := isolatedCLI(t) + ws := config.LoadWorkspaces() + ws.AddVault("alpha", filepath.Join(dir, "a")) + ws.AddServer("home", "http://127.0.0.1:7878") + if err := config.SaveWorkspaces(ws); err != nil { + t.Fatal(err) + } + for _, verb := range []string{"remove", "rm"} { + out := captureOutput(t, func() { _ = Main([]string{"__complete", "--", "vault", verb, ""}) }) + if strings.TrimSpace(out) != "alpha\nhome" { + t.Fatalf("vault %s completion: %q", verb, out) + } + } +} From 79fbc660d77a0ea57912123de38291dfedfa05f5 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Wed, 30 Sep 2026 19:37:37 -0500 Subject: [PATCH 02/10] Fix(cli): stop every command from querying the terminal background at startup Bubble Tea v1 asks Lip Gloss for the terminal background in a package init, so on a terminal that never answers OSC 11 (the Linux console, some IDE and serial terminals) every zn command, `zn --version` included, stalled for termenv's five-second timeout, and a late answer could land in the next stdin read, such as the `zn setup` prompt. internal/termbg initializes before Bubble Tea (Go orders package initialization by import path among ready packages, and ZenNotes sorts before charmbracelet) and presets Lip Gloss's answer from COLORFGBG or the dark default, so that init no longer touches the terminal. The two places that need the real answer, the terminal app's auto theme and `zn read --pretty`, call termbg.Detect, which asks the terminal once and on purpose. A Linux pty test runs the binary against a terminal that never answers and fails if a query is written or startup takes seconds. --- internal/cli/pretty.go | 5 +- internal/termbg/termbg.go | 66 +++++++++++++++ internal/termbg/termbg_linux_test.go | 119 +++++++++++++++++++++++++++ internal/termbg/termbg_test.go | 44 ++++++++++ internal/tui/theme.go | 3 +- 5 files changed, 234 insertions(+), 3 deletions(-) create mode 100644 internal/termbg/termbg.go create mode 100644 internal/termbg/termbg_linux_test.go create mode 100644 internal/termbg/termbg_test.go diff --git a/internal/cli/pretty.go b/internal/cli/pretty.go index 6e17dde..19583e2 100644 --- a/internal/cli/pretty.go +++ b/internal/cli/pretty.go @@ -7,8 +7,9 @@ import ( "github.com/charmbracelet/glamour" "github.com/charmbracelet/glamour/styles" - "github.com/charmbracelet/lipgloss" "golang.org/x/term" + + "github.com/ZenNotes/tui/internal/termbg" ) // renderPretty formats a note through Glamour for the terminal: the @@ -28,7 +29,7 @@ func renderPretty(body, style string) (string, error) { case !isTTY && (style == "" || style == "auto"): opts = append(opts, glamour.WithStandardStyle(styles.NoTTYStyle)) case style == "" || style == "auto": - if lipgloss.HasDarkBackground() { + if termbg.Detect() { opts = append(opts, glamour.WithStandardStyle(styles.DarkStyle)) } else { opts = append(opts, glamour.WithStandardStyle(styles.LightStyle)) diff --git a/internal/termbg/termbg.go b/internal/termbg/termbg.go new file mode 100644 index 0000000..1045e98 --- /dev/null +++ b/internal/termbg/termbg.go @@ -0,0 +1,66 @@ +// Package termbg decides the terminal's background color for Lip Gloss +// before Bubble Tea can ask the terminal itself. +// +// Bubble Tea v1 queries the terminal in a package init (tea_init.go: "make +// sure Lip Gloss and Termenv query the terminal before any Bubble Tea +// Program runs") whenever stdout is a terminal. Termenv waits up to +// OSCTimeout, five seconds, for the answer. Terminals that never answer +// OSC 11 (the Linux console, some IDE and serial terminals, a few +// multiplexer setups) therefore stalled every zn command, `zn --version` +// included, for five seconds, and the belated answer could land in the +// next thing reading stdin, such as the `zn setup` prompt. +// +// Go initializes packages in import-path order among those whose imports +// are ready, and github.com/ZenNotes sorts before github.com/charmbracelet, +// so this package's init runs after Lip Gloss's and before Bubble Tea's. +// Presetting the answer here makes that init a no-op; the two places that +// want the real answer, the terminal app's auto theme and `zn read +// --pretty`, call Detect and ask the terminal deliberately. +package termbg + +import ( + "os" + "strconv" + "strings" + "sync" + + "github.com/charmbracelet/lipgloss" + "github.com/muesli/termenv" +) + +func init() { + lipgloss.SetHasDarkBackground(Guess(os.Getenv("COLORFGBG"))) +} + +// Guess is the answer without asking the terminal: what COLORFGBG says +// (rxvt, Konsole and others export it as "fg;bg"), else dark, the same +// fallback termenv uses when a query goes unanswered. +func Guess(colorFGBG string) bool { + parts := strings.Split(strings.TrimSpace(colorFGBG), ";") + if len(parts) < 2 { + return true + } + bg, err := strconv.Atoi(parts[len(parts)-1]) + if err != nil || bg < 0 || bg > 15 { + return true + } + _, _, lightness := termenv.ConvertToRGB(termenv.ANSIColor(bg)).Hsl() + return lightness < 0.5 +} + +var ( + detectOnce sync.Once + detected bool +) + +// Detect asks the terminal on stdout once and remembers the answer, also +// for Lip Gloss's default renderer. It must run before Bubble Tea owns the +// terminal; it is the one place zn pays for the query, and only when the +// caller needs the real background. +func Detect() bool { + detectOnce.Do(func() { + detected = lipgloss.NewRenderer(os.Stdout).HasDarkBackground() + lipgloss.SetHasDarkBackground(detected) + }) + return detected +} diff --git a/internal/termbg/termbg_linux_test.go b/internal/termbg/termbg_linux_test.go new file mode 100644 index 0000000..09d8fd9 --- /dev/null +++ b/internal/termbg/termbg_linux_test.go @@ -0,0 +1,119 @@ +//go:build linux + +package termbg_test + +import ( + "bytes" + "fmt" + "os" + "os/exec" + "strings" + "sync" + "testing" + "time" + + "golang.org/x/sys/unix" +) + +// openPTY returns the master side and the slave side of a new pseudo +// terminal, so a child process sees a real terminal on its standard streams. +func openPTY(t *testing.T) (*os.File, *os.File) { + t.Helper() + master, err := os.OpenFile("/dev/ptmx", os.O_RDWR|unix.O_NOCTTY, 0) + if err != nil { + t.Skipf("no pty available: %v", err) + } + n, err := unix.IoctlGetInt(int(master.Fd()), unix.TIOCGPTN) + if err != nil { + t.Fatal(err) + } + if err := unix.IoctlSetPointerInt(int(master.Fd()), unix.TIOCSPTLCK, 0); err != nil { + t.Fatal(err) + } + slave, err := os.OpenFile(fmt.Sprintf("/dev/pts/%d", n), os.O_RDWR|unix.O_NOCTTY, 0) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { master.Close(); slave.Close() }) + return master, slave +} + +// runOnTerminal runs this test binary as a child on a pty and returns what it +// wrote. answer, when set, plays the terminal's part for background queries. +func runOnTerminal(t *testing.T, mode string, answer func(master *os.File, seen []byte)) (string, time.Duration) { + t.Helper() + master, slave := openPTY(t) + cmd := exec.Command(os.Args[0]) + cmd.Env = append(os.Environ(), "ZN_TERMBG_CHILD="+mode, "TERM=xterm-256color", "COLORFGBG=") + cmd.Stdin, cmd.Stdout, cmd.Stderr = slave, slave, slave + cmd.SysProcAttr = &unix.SysProcAttr{Setsid: true, Setctty: true, Ctty: 0} + start := time.Now() + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + slave.Close() + var out bytes.Buffer + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + buf := make([]byte, 4096) + for { + n, err := master.Read(buf) + if n > 0 { + out.Write(buf[:n]) + if answer != nil { + answer(master, out.Bytes()) + } + } + if err != nil { + return + } + } + }() + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case <-done: + case <-time.After(8 * time.Second): + _ = cmd.Process.Kill() + t.Fatalf("child did not exit; output so far: %q", out.String()) + } + elapsed := time.Since(start) + master.Close() + wg.Wait() + return out.String(), elapsed +} + +// A terminal that never answers OSC 11 must not hold every command for +// termenv's five-second timeout, and no query may be written at all. +func TestStartupDoesNotQueryTheTerminal(t *testing.T) { + out, elapsed := runOnTerminal(t, "startup", nil) + if !strings.Contains(out, "child-ok") { + t.Fatalf("child output: %q", out) + } + if strings.Contains(out, "\x1b]11;?") || strings.Contains(out, "\x1b[6n") { + t.Fatalf("startup still queries the terminal: %q", out) + } + if elapsed > 3*time.Second { + t.Fatalf("startup took %s on a terminal that does not answer background queries", elapsed) + } +} + +// Detect is the deliberate query: it asks and believes the terminal. +func TestDetectAsksTheTerminalOnce(t *testing.T) { + var replied sync.Once + out, _ := runOnTerminal(t, "detect", func(master *os.File, seen []byte) { + if bytes.Contains(seen, []byte("\x1b[6n")) { + replied.Do(func() { + _, _ = master.Write([]byte("\x1b]11;rgb:ffff/ffff/ffff\x1b\\\x1b[1;1R")) + }) + } + }) + if !strings.Contains(out, "light") { + t.Fatalf("Detect ignored the terminal's light background: %q", out) + } + if strings.Count(out, "\x1b]11;?") != 1 { + t.Fatalf("expected exactly one background query: %q", out) + } +} diff --git a/internal/termbg/termbg_test.go b/internal/termbg/termbg_test.go new file mode 100644 index 0000000..12a46e6 --- /dev/null +++ b/internal/termbg/termbg_test.go @@ -0,0 +1,44 @@ +package termbg_test + +import ( + "fmt" + "os" + "testing" + + // Linked on purpose: its init is the terminal query this package + // pre-empts, so the child processes below behave like zn does. + _ "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" + + "github.com/ZenNotes/tui/internal/termbg" +) + +// The test binary doubles as the child process the pty tests run: it must +// import Bubble Tea and termbg exactly like zn, and nothing else can. +func TestMain(m *testing.M) { + switch os.Getenv("ZN_TERMBG_CHILD") { + case "": + os.Exit(m.Run()) + case "startup": + fmt.Println("child-ok") + case "detect": + fmt.Println(map[bool]string{true: "dark", false: "light"}[termbg.Detect()]) + } + os.Exit(0) +} + +func TestGuessReadsCOLORFGBG(t *testing.T) { + for input, dark := range map[string]bool{"": true, "15;0": true, "0;15": false, "0;7": false, "7;default;0": true, "junk": true, "0;99": true} { + if got := termbg.Guess(input); got != dark { + t.Errorf("Guess(%q) = %v, want %v", input, got, dark) + } + } +} + +// After init, Lip Gloss has an explicit answer, so nothing that asks the +// default renderer queries the terminal. +func TestDefaultRendererNeedsNoQuery(t *testing.T) { + if got := lipgloss.HasDarkBackground(); got != termbg.Guess(os.Getenv("COLORFGBG")) { + t.Fatalf("default renderer reports %v, want the preset guess", got) + } +} diff --git a/internal/tui/theme.go b/internal/tui/theme.go index 2a9d790..269dd93 100644 --- a/internal/tui/theme.go +++ b/internal/tui/theme.go @@ -7,6 +7,7 @@ import ( "github.com/charmbracelet/lipgloss" "github.com/ZenNotes/tui/internal/config" + "github.com/ZenNotes/tui/internal/termbg" "github.com/ZenNotes/tui/internal/themes" ) @@ -70,7 +71,7 @@ type Theme struct { // run before Bubble Tea takes over the terminal, so Run calls it once and // the answer is reused for `:theme system` later. func DetectDarkBackground() bool { - return lipgloss.HasDarkBackground() + return termbg.Detect() } // themeSelection is the theme the preferences ask for: the desktop's From accefcf2a290639c67fc5e4b5aee55320483e63a Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Wed, 30 Sep 2026 19:39:48 -0500 Subject: [PATCH 03/10] Fix(vault): recognise an empty frontmatter block so record pages stop growing fences A record page for a row with only a title was written with an empty frontmatter block (`---\n---`), which the frontmatter regex did not recognise, so every `zn base set` re-mirror treated the fences as body and prepended another pair: four fences after one edit, six after two. Frontmatter now accepts an empty block, trying the empty form before the lazy one so a horizontal rule later in the body cannot be mistaken for the closing fence; prepend and excerpts honour it too. ComposePageBody writes no block when a row has no properties to mirror, so re-mirroring is idempotent, and pages from older builds shed their stray fences on the next pass. The unused frontmatterOnlyR copy in tasks.go is gone. --- internal/database/doc.go | 6 ++++- internal/database/schema_test.go | 39 +++++++++++++++++++++++++++++ internal/vault/frontmatter_test.go | 40 ++++++++++++++++++++++++++++++ internal/vault/parse.go | 23 +++++++++++------ internal/vault/tasks.go | 7 +++--- 5 files changed, 102 insertions(+), 13 deletions(-) create mode 100644 internal/vault/frontmatter_test.go diff --git a/internal/database/doc.go b/internal/database/doc.go index 4a4046c..bceabdf 100644 --- a/internal/database/doc.go +++ b/internal/database/doc.go @@ -245,7 +245,8 @@ func (d *Doc) RecordTitle(row Row) string { } // ComposePageBody composes a record page: the row's properties as flat YAML -// frontmatter (id and title fields omitted) followed by body. +// frontmatter (id and title fields omitted) followed by body. A row with no +// properties to mirror gets no frontmatter block at all. func (d *Doc) ComposePageBody(row Row, body string) string { titleID := d.TitleFieldID() lines := []string{"---"} @@ -260,6 +261,9 @@ func (d *Doc) ComposePageBody(row Row, body string) string { lines = append(lines, f.Name+":") } } + if len(lines) == 1 { + return strings.TrimLeft(body, "\n") + } lines = append(lines, "---") return strings.Join(lines, "\n") + "\n" + strings.TrimLeft(body, "\n") } diff --git a/internal/database/schema_test.go b/internal/database/schema_test.go index d91e548..5e8f22a 100644 --- a/internal/database/schema_test.go +++ b/internal/database/schema_test.go @@ -3,6 +3,8 @@ package database import ( "strings" "testing" + + "github.com/ZenNotes/tui/internal/vault" ) func testDoc(t *testing.T) *Doc { @@ -97,3 +99,40 @@ func TestNoteLinksAndDuplicate(t *testing.T) { t.Fatalf("duplicate: %+v", d.Rows) } } + +// A record page for a row with nothing but a title has no properties to +// mirror, so it gets no frontmatter block; re-mirroring is idempotent either +// way instead of stacking `---` fences on every `zn base set`. +func TestComposePageBodyIsIdempotentWithoutProperties(t *testing.T) { + d := testDoc(t) + if err := d.DeleteField("f2"); err != nil { + t.Fatal(err) + } + row := d.Rows[0] + page := d.ComposePageBody(row, "# Dune\n\nbody\n") + if page != "# Dune\n\nbody\n" { + t.Fatalf("page without properties: %q", page) + } + _, body, _ := vault.Frontmatter(page) + if again := d.ComposePageBody(row, body); again != page { + t.Fatalf("re-mirror changed the page: %q", again) + } + // Pages written by older builds carry an empty block; it is recognised + // as frontmatter and replaced, not stacked. + _, body, ok := vault.Frontmatter("---\n---\n---\n---\n# Dune\n\nbody\n") + if !ok { + t.Fatal("empty frontmatter block not recognised") + } + if again := d.ComposePageBody(row, body); again != "---\n---\n# Dune\n\nbody\n" { + t.Fatalf("one stray block should remain to be stripped on the next pass, got %q", again) + } + full := testDoc(t) + withProps := full.ComposePageBody(full.Rows[0], "# Dune\n\nbody\n") + if withProps != "---\nStatus: todo\n---\n# Dune\n\nbody\n" { + t.Fatalf("page with properties: %q", withProps) + } + _, body, _ = vault.Frontmatter(withProps) + if again := full.ComposePageBody(full.Rows[0], body); again != withProps { + t.Fatalf("re-mirror with properties changed the page: %q", again) + } +} diff --git a/internal/vault/frontmatter_test.go b/internal/vault/frontmatter_test.go new file mode 100644 index 0000000..d7fd7e4 --- /dev/null +++ b/internal/vault/frontmatter_test.go @@ -0,0 +1,40 @@ +package vault + +import "testing" + +// An empty block is still a block: record pages without properties carry +// one, and treating it as body text made every re-mirror add another. +func TestFrontmatterRecognisesAnEmptyBlock(t *testing.T) { + cases := []struct { + body, block, rest string + ok bool + }{ + {"---\n---\n# Title\n\nbody\n", "", "# Title\n\nbody\n", true}, + {"---\r\n---\r\n# T\n", "", "# T\n", true}, + {"---\n---", "", "", true}, + {"---\ntitle: x\n---\n# T\n", "title: x", "# T\n", true}, + {"---\ntitle: x\ntags: [a]\n---\nbody", "title: x\ntags: [a]", "body", true}, + // A later horizontal rule is body, not the closing fence. + {"---\n---\n# T\n\n---\n\nmore\n", "", "# T\n\n---\n\nmore\n", true}, + {"---\n----- not a fence\ntitle: x\n---\nbody", "----- not a fence\ntitle: x", "body", true}, + {"# No frontmatter\n---\n---\n", "", "# No frontmatter\n---\n---\n", false}, + {"---\nunterminated\n", "", "---\nunterminated\n", false}, + {"", "", "", false}, + } + for _, c := range cases { + block, rest, ok := Frontmatter(c.body) + if ok != c.ok || block != c.block || rest != c.rest { + t.Errorf("Frontmatter(%q) = (%q, %q, %v), want (%q, %q, %v)", c.body, block, rest, ok, c.block, c.rest, c.ok) + } + } +} + +func TestPrependKeepsAnEmptyFrontmatterBlockOnTop(t *testing.T) { + got := PrependToBody("---\n---\n# T\n", "new line") + if got != "---\n---\nnew line\n\n# T\n" { + t.Fatalf("prepend: %q", got) + } + if excerpt := BuildExcerpt("---\n---\n# Real title\n\nThe body.\n"); excerpt != "Real title The body." { + t.Fatalf("excerpt after empty block: %q", excerpt) + } +} diff --git a/internal/vault/parse.go b/internal/vault/parse.go index 7995f32..e5d0891 100644 --- a/internal/vault/parse.go +++ b/internal/vault/parse.go @@ -9,13 +9,17 @@ import ( // Regexes mirror the extractors in the desktop's vault-ops.ts and the // server's parse.go so the extracted metadata matches those builds. var ( - fenceLineRe = regexp.MustCompile("^[ \t]*(`{3,}|~{3,})(.*)$") - inlineCodeRe = regexp.MustCompile("`[^`\n]*`") - tagRe = regexp.MustCompile(`(?:^|\s)#(\p{L}[\p{L}\d_/-]*)`) - wikilinkRe = regexp.MustCompile(`(!?)\[\[([^\]|]+?)(?:\|[^\]]+)?\]\]`) - linkRe = regexp.MustCompile(`(!?)\[[^\]]*\]\(([^)\s]+)(?:\s+"[^"]*")?\)`) - embedRe = regexp.MustCompile(`!\[\[([^\]|]+?)(?:\|[^\]]+)?\]\]`) - frontmatterRe = regexp.MustCompile(`(?s)\A---\r?\n(.*?)\r?\n---\r?\n?`) + fenceLineRe = regexp.MustCompile("^[ \t]*(`{3,}|~{3,})(.*)$") + inlineCodeRe = regexp.MustCompile("`[^`\n]*`") + tagRe = regexp.MustCompile(`(?:^|\s)#(\p{L}[\p{L}\d_/-]*)`) + wikilinkRe = regexp.MustCompile(`(!?)\[\[([^\]|]+?)(?:\|[^\]]+)?\]\]`) + linkRe = regexp.MustCompile(`(!?)\[[^\]]*\]\(([^)\s]+)(?:\s+"[^"]*")?\)`) + embedRe = regexp.MustCompile(`!\[\[([^\]|]+?)(?:\|[^\]]+)?\]\]`) + // A block may be empty (`---\n---`): record pages without properties + // are written that way, and failing to see such a block made every + // re-mirror prepend another one. The empty form is tried first so a + // body's later horizontal rule cannot be mistaken for the closing fence. + frontmatterRe = regexp.MustCompile(`(?s)\A---\r?\n(?:---(?:\r?\n|\z)|(.*?)\r?\n---\r?\n?)`) headingRe = regexp.MustCompile(`(?m)^#{1,6}\s+`) imageMdRe = regexp.MustCompile(`!\[[^\]]*\]\([^)]*\)`) mdLinkRe = regexp.MustCompile(`\[([^\]]+)\]\([^)]*\)`) @@ -80,7 +84,10 @@ func Frontmatter(body string) (block string, rest string, ok bool) { if m == nil { return "", body, false } - return body[m[2]:m[3]], body[m[1]:], true + if m[2] >= 0 { + block = body[m[2]:m[3]] + } + return block, body[m[1]:], true } // ParseFrontmatterFields parses a frontmatter block into flat fields: diff --git a/internal/vault/tasks.go b/internal/vault/tasks.go index 54b0ef7..94f63e2 100644 --- a/internal/vault/tasks.go +++ b/internal/vault/tasks.go @@ -771,10 +771,9 @@ func RemoveTaskLine(markdown string, taskIndex int) (line string, body string, o } var ( - tasksHeadingRe = regexp.MustCompile(`(?i)^ {0,3}(#{1,6})\s+Tasks\s*$`) - anyHeadingRe = regexp.MustCompile(`^ {0,3}(#{1,6})\s+`) - thematicBreakRe = regexp.MustCompile(`^ {0,3}(?:(?:-[ \t]*){3,}|(?:\*[ \t]*){3,}|(?:_[ \t]*){3,})$`) - frontmatterOnlyR = regexp.MustCompile(`(?s)\A---\n.*?\n---\n?`) + tasksHeadingRe = regexp.MustCompile(`(?i)^ {0,3}(#{1,6})\s+Tasks\s*$`) + anyHeadingRe = regexp.MustCompile(`^ {0,3}(#{1,6})\s+`) + thematicBreakRe = regexp.MustCompile(`^ {0,3}(?:(?:-[ \t]*){3,}|(?:\*[ \t]*){3,}|(?:_[ \t]*){3,})$`) ) // InsertTasksUnderTasksHeading places task lines at the end of a `# Tasks` From d0878800d8354ebbff77d90ac262bc110931be26 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Wed, 30 Sep 2026 19:40:51 -0500 Subject: [PATCH 04/10] Fix(cli): take a frontmatter body after --body, accept tag find --tag, rank swaps as one typo A value flag refused any next token starting with `--`, so a body that opens with a frontmatter fence failed with "--body needs a value". Only `--` itself, `-h` and `--` count as the next flag now; `---` and `-x` are values. `zn tag find --tag ` was accepted by the parser and then rejected for a missing positional; the flag satisfies it like --path does elsewhere. Typo suggestions count a swap of adjacent letters as one edit, so `lsit` suggests list instead of init. --- internal/cli/args.go | 12 ++++++++- internal/cli/args_test.go | 56 +++++++++++++++++++++++++++++++++++++++ internal/cli/registry.go | 54 +++++++++++++++++++++++-------------- 3 files changed, 101 insertions(+), 21 deletions(-) diff --git a/internal/cli/args.go b/internal/cli/args.go index 38c951b..d0ce40c 100644 --- a/internal/cli/args.go +++ b/internal/cli/args.go @@ -25,6 +25,16 @@ type Args struct { var shortFlagRe = regexp.MustCompile(`^-[A-Za-z][\w-]*$`) +// longFlagRe is `--` followed by a letter. `---` is not a flag, so a body +// that starts with a frontmatter fence can follow --body. +var longFlagRe = regexp.MustCompile(`^--[A-Za-z]`) + +// startsAFlag says whether the token after a value flag is the next flag +// (or the `--` terminator) rather than the value. +func startsAFlag(token string) bool { + return token == "--" || token == "-h" || longFlagRe.MatchString(token) +} + // valuelessFlags are the long flags that are switches, never `--flag `. // Without this list a switch written before a positional swallowed it: // `zn open --new-window ~/notes` parsed as new-window="~/notes" with no path, @@ -68,7 +78,7 @@ func Parse(argv []string) Args { continue } name := token[2:] - if !valuelessFlags[name] && i+1 < len(argv) && !strings.HasPrefix(argv[i+1], "--") { + if !valuelessFlags[name] && i+1 < len(argv) && !startsAFlag(argv[i+1]) { args.push(name, argv[i+1]) i++ } else { diff --git a/internal/cli/args_test.go b/internal/cli/args_test.go index 4bec861..8569ba2 100644 --- a/internal/cli/args_test.go +++ b/internal/cli/args_test.go @@ -2,6 +2,8 @@ package cli import ( "reflect" + "strconv" + "strings" "testing" ) @@ -64,3 +66,57 @@ func TestOpenLaunchArgsAndMessage(t *testing.T) { t.Fatalf("two, -n: %q", got) } } + +// A body that opens with a frontmatter fence starts with `---`, which is +// not a flag; it must be taken as the value, while a real flag or the `--` +// terminator after a value flag still means the value is missing. +func TestValueFlagsAcceptValuesThatStartWithDashes(t *testing.T) { + fm := "---\ntitle: x\n---\nbody" + name, args, err := parseCommand([]string{"write", "a.md", "--body", fm}) + if err != nil || name != "write" || args.Str("body") != fm { + t.Fatalf("frontmatter body: %q %v %v", name, args.Flags, err) + } + if _, args, err := parseCommand([]string{"capture", "--title", "---", "text"}); err != nil || args.Str("title") != "---" { + t.Fatalf("bare --- as a value: %v %v", args.Flags, err) + } + if _, args, err := parseCommand([]string{"write", "a.md", "--body", "-x"}); err != nil || args.Str("body") != "-x" { + t.Fatalf("single dash value: %v %v", args.Flags, err) + } + for _, argv := range [][]string{ + {"create", "--title", "--json"}, + {"create", "--title", "--", "x"}, + {"create", "--title", "-h"}, + {"create", "--title"}, + } { + if _, _, err := parseCommand(argv); err == nil || !strings.Contains(err.Error(), "needs a value") { + t.Errorf("%v: want a missing-value error, got %v", argv, err) + } + } + if got := Parse([]string{"--body", fm}); got.Str("body") != fm { + t.Fatalf("Parse: %v", got.Flags) + } +} + +func TestSuggestionsCountSwapsAsOneEdit(t *testing.T) { + roots := commandRoots() + for input, want := range map[string]string{"lsit": "list", "serach": "search", "craete": "create", "sevrer": "server", "lits": "list", "statsu": "status"} { + if got := suggestion(input, roots); got != " Did you mean "+strconv.Quote(want)+"?" { + t.Errorf("suggestion(%q) = %q, want %q", input, got, want) + } + } + if got := suggestion("zzzzzz", roots); got != "" { + t.Errorf("far-off input must not get a suggestion: %q", got) + } +} + +// `--tag` is the flag form of tag find's positional; it must satisfy the +// positional requirement the way --path does for note commands. +func TestTagFindAcceptsTheTagFlag(t *testing.T) { + name, args, err := parseCommand([]string{"tag", "find", "--tag", "work", "--json"}) + if err != nil || name != "tag find" || args.Str("tag") != "work" { + t.Fatalf("tag find --tag: %q %v %v", name, args.Flags, err) + } + if _, _, err := parseCommand([]string{"tag", "find"}); err == nil { + t.Fatal("tag find without a tag must be a usage error") + } +} diff --git a/internal/cli/registry.go b/internal/cli/registry.go index 41b82b6..5aac676 100644 --- a/internal/cli/registry.go +++ b/internal/cli/registry.go @@ -177,6 +177,9 @@ func validatePositionals(spec commandSpec, args Args) error { if spec.Name == "task toggle" && args.Str("id") != "" { minimum-- } + if spec.Name == "tag find" && args.Str("tag") != "" { + minimum-- + } if spec.Name == "base create" && args.Str("title") != "" { minimum-- } @@ -216,7 +219,7 @@ func parseFlag(argv []string, i int, allowed map[string]bool, args *Args) (int, return i, fmt.Errorf("unknown flag --%s.%s", name, suggestion(name, choices)) } if takesValue && !assigned { - if i+1 >= len(argv) || strings.HasPrefix(argv[i+1], "--") || argv[i+1] == "-h" { + if i+1 >= len(argv) || startsAFlag(argv[i+1]) { return i, fmt.Errorf("--%s needs a value", name) } i++ @@ -275,25 +278,8 @@ func suggestion(input string, choices []string) string { sort.Strings(choices) best, distance := "", 3 for _, choice := range choices { - row := make([]int, len(choice)+1) - for j := range row { - row[j] = j - } - for i := range input { - prev := row[0] - row[0] = i + 1 - for j := range choice { - cost := 0 - if input[i] != choice[j] { - cost = 1 - } - old := row[j+1] - row[j+1] = min(row[j+1]+1, row[j]+1, prev+cost) - prev = old - } - } - if row[len(choice)] < distance { - best, distance = choice, row[len(choice)] + if d := editDistance(input, choice); d < distance { + best, distance = choice, d } } if best == "" { @@ -301,3 +287,31 @@ func suggestion(input string, choices []string) string { } return " Did you mean " + strconv.Quote(best) + "?" } + +// editDistance counts insertions, deletions, substitutions and swaps of +// adjacent characters, so `lsit` is one edit from `list` rather than two +// (which tied it with `init` and lost on alphabetical order). +func editDistance(a, b string) int { + rows, cols := len(a)+1, len(b)+1 + d := make([][]int, rows) + for i := range d { + d[i] = make([]int, cols) + d[i][0] = i + } + for j := 0; j < cols; j++ { + d[0][j] = j + } + for i := 1; i < rows; i++ { + for j := 1; j < cols; j++ { + cost := 0 + if a[i-1] != b[j-1] { + cost = 1 + } + d[i][j] = min(d[i-1][j]+1, d[i][j-1]+1, d[i-1][j-1]+cost) + if i > 1 && j > 1 && a[i-1] == b[j-2] && a[i-2] == b[j-1] { + d[i][j] = min(d[i][j], d[i-2][j-2]+1) + } + } + } + return d[rows-1][cols-1] +} From 0fce48307883b9e6daafcb1af678e6340024cc4c Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Wed, 30 Sep 2026 20:01:52 -0500 Subject: [PATCH 05/10] Fix(config): never overwrite a workspaces.toml or credentials.toml zn could not parse A file that failed to parse was treated as an empty list, so the next command that saved (connect, init, vault add, use, a server setup) wrote that empty list back and the user lost every saved vault, server and token. The load now remembers the problem, saving refuses with the parse error and what to do, and a broken token store yields no tokens rather than a fresh file holding one. `zn doctor` gains a "saved vaults" check and extends the credentials check to parse errors; `zn status` warns and `zn vault list` says zn's own entries are not being shown. --- internal/cli/commands_diagnostics.go | 17 ++++-- internal/cli/commands_vault_capture.go | 5 +- internal/cli/vault_remove_test.go | 57 ++++++++++++++++++++ internal/config/workspaces.go | 62 ++++++++++++++++++--- internal/config/workspaces_test.go | 75 ++++++++++++++++++++++++++ 5 files changed, 203 insertions(+), 13 deletions(-) diff --git a/internal/cli/commands_diagnostics.go b/internal/cli/commands_diagnostics.go index 9538a5e..8011bda 100644 --- a/internal/cli/commands_diagnostics.go +++ b/internal/cli/commands_diagnostics.go @@ -28,6 +28,7 @@ type workspaceStatus struct { Location string `json:"location,omitempty"` AuthConfigured bool `json:"authConfigured"` Problem string `json:"problem,omitempty"` + Warning string `json:"warning,omitempty"` InstallationOwner string `json:"installationOwner,omitempty"` } @@ -41,6 +42,9 @@ func currentStatus(args Args) (workspaceStatus, error) { if install, err := selfupdate.Current(); err == nil { st.InstallationOwner = install.Owner } + if _, err := config.LoadWorkspacesFile(); err != nil { + st.Warning = "saved vaults and servers are unavailable: " + err.Error() + } switch { case args.Str("server") != "": st.SelectedBy = "--server" @@ -91,6 +95,9 @@ func cmdStatus(args Args) error { emitLine("Installation owner: " + st.InstallationOwner) emitLine("Config: " + st.ConfigPath) emitLine("Workspace source: " + st.Source + " · selected by " + st.SelectedBy) + if st.Warning != "" { + emitLine("Warning: " + st.Warning) + } if st.Problem != "" { emitLine(st.Problem) } else { @@ -118,6 +125,8 @@ func cmdDoctor(ctx context.Context, args Args) error { checks = append(checks, diagnosticCheck{Name: name, OK: err == nil, Message: message}) } add("configuration", config.ConfigTomlPath(), config.ValidateConfig()) + ws, wsErr := config.LoadWorkspacesFile() + add("saved vaults", fmt.Sprintf("%d vaults and %d servers in %s", len(ws.Vaults), len(ws.Servers), config.WorkspacesPath()), wsErr) if installation, err := selfupdate.Current(); err == nil { add("installation", installation.Owner+": "+installation.Instruction, nil) } @@ -131,11 +140,11 @@ func cmdDoctor(ctx context.Context, args Args) error { add("editor", "VISUAL/EDITOR unset; config edit uses the platform fallback", nil) } if info, err := os.Stat(config.CredentialsPath()); err == nil { - var permissionErr error - if runtime.GOOS != "windows" && info.Mode().Perm()&0o077 != 0 { - permissionErr = fmt.Errorf("credentials.toml must be private (mode 0600)") + problem := config.CredentialsProblem() + if problem == nil && runtime.GOOS != "windows" && info.Mode().Perm()&0o077 != 0 { + problem = fmt.Errorf("credentials.toml must be private (mode 0600)") } - add("credentials", "Stored credentials are private", permissionErr) + add("credentials", "Stored credentials are private", problem) } target, err := ResolveTargetFromArgs(args) add("workspace", "Workspace selected", err) diff --git a/internal/cli/commands_vault_capture.go b/internal/cli/commands_vault_capture.go index 687499d..9d208e4 100644 --- a/internal/cli/commands_vault_capture.go +++ b/internal/cli/commands_vault_capture.go @@ -105,7 +105,10 @@ func cmdVaultList(args Args) error { // source, and a desktop-managed zn never resolves zn's own names. ws := config.Workspaces{} if source == "terminal" { - ws = config.LoadWorkspaces() + var loadErr error + if ws, loadErr = config.LoadWorkspacesFile(); loadErr != nil { + emitError(fmt.Sprintf("%v; zn's own vaults and servers are not shown. `zn doctor` has details.", loadErr)) + } } defaultTarget, err := ResolveTargetFromArgs(args) if err != nil && !errors.Is(err, config.ErrNoVault) { diff --git a/internal/cli/vault_remove_test.go b/internal/cli/vault_remove_test.go index e728daa..10d8ca9 100644 --- a/internal/cli/vault_remove_test.go +++ b/internal/cli/vault_remove_test.go @@ -141,3 +141,60 @@ func TestVaultRemoveCompletesZnsOwnNames(t *testing.T) { } } } + +// With an unparseable workspaces.toml, every command that would save must +// refuse rather than write an empty list over the user's saved entries. +func TestCommandsRefuseToOverwriteABrokenWorkspacesFile(t *testing.T) { + dir := isolatedCLI(t) + ws := config.LoadWorkspaces() + ws.AddVault("alpha", filepath.Join(dir, "alpha")) + if err := config.SaveWorkspaces(ws); err != nil { + t.Fatal(err) + } + broken := []byte("default = \"alpha\"\n[[vault]]\nname = \"alpha\"\nroot = \"/a\"\n[vault]\nname = \"oops\"\n") + if err := os.WriteFile(config.WorkspacesPath(), broken, 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(dir, "beta"), 0o755); err != nil { + t.Fatal(err) + } + code := 0 + out := captureOutput(t, func() { code = Main([]string{"vault", "add", filepath.Join(dir, "beta"), "--name", "beta"}) }) + if code == 0 || !strings.Contains(out, "not saving") || !strings.Contains(out, "workspaces.toml") { + t.Fatalf("vault add: exit %d %s", code, out) + } + out = captureOutput(t, func() { code = Main([]string{"init", filepath.Join(dir, "gamma")}) }) + if code == 0 || !strings.Contains(out, "not saving") { + t.Fatalf("init: exit %d %s", code, out) + } + if raw, _ := os.ReadFile(config.WorkspacesPath()); string(raw) != string(broken) { + t.Fatal("workspaces.toml was overwritten") + } + out = captureOutput(t, func() { code = Main([]string{"doctor", "--json"}) }) + var report struct { + Checks []diagnosticCheck `json:"checks"` + } + if err := json.Unmarshal([]byte(strings.SplitN(out, "\n{\"error\"", 2)[0]), &report); err != nil { + t.Fatalf("doctor json: %v %s", err, out) + } + found := false + for _, c := range report.Checks { + if c.Name == "saved vaults" { + found = true + if c.OK || !strings.Contains(c.Message, "workspaces.toml") { + t.Fatalf("saved vaults check: %+v", c) + } + } + } + if !found || code == 0 { + t.Fatalf("doctor must fail on a broken workspaces file: exit %d %s", code, out) + } + out = captureOutput(t, func() { code = Main([]string{"status"}) }) + if !strings.Contains(out, "Warning: saved vaults and servers are unavailable") { + t.Fatalf("status warning: %s", out) + } + out = captureOutput(t, func() { _ = Main([]string{"vault", "list"}) }) + if !strings.Contains(out, "zn's own vaults and servers are not shown") { + t.Fatalf("vault list warning: %s", out) + } +} diff --git a/internal/config/workspaces.go b/internal/config/workspaces.go index 27bc45a..463ee2a 100644 --- a/internal/config/workspaces.go +++ b/internal/config/workspaces.go @@ -39,6 +39,11 @@ type Workspaces struct { Default string `toml:"default"` Vaults []LocalWorkspace `toml:"vault"` Servers []ServerWorkspace `toml:"server"` + // loadErr remembers that the file on disk exists but could not be read + // or parsed. The list is then empty, and SaveWorkspaces refuses to + // replace the file with it: a stray edit must not cost the user every + // saved vault and server on the next `zn connect`. + loadErr error } // WorkspacesPath is where zn's vault and server list lives. @@ -47,19 +52,37 @@ func WorkspacesPath() string { return filepath.Join(PortableConfigDir(), "worksp // CredentialsPath is where server tokens live (mode 0600). func CredentialsPath() string { return filepath.Join(PortableConfigDir(), "credentials.toml") } -// LoadWorkspaces reads the list; a missing file is an empty list. +// LoadWorkspaces reads the list; a missing or unreadable file is an empty +// list. LoadWorkspacesFile also reports the latter. func LoadWorkspaces() Workspaces { + ws, _ := LoadWorkspacesFile() + return ws +} + +// LoadWorkspacesFile reads the list and says when the file exists but +// could not be read or parsed; a missing file is simply an empty list. +func LoadWorkspacesFile() (Workspaces, error) { var ws Workspaces raw, err := os.ReadFile(WorkspacesPath()) + if errors.Is(err, os.ErrNotExist) { + return ws, nil + } + if err == nil { + _, err = toml.Decode(string(raw), &ws) + } if err != nil { - return ws + ws = Workspaces{loadErr: fmt.Errorf("%s: %w", WorkspacesPath(), err)} + return ws, ws.loadErr } - _, _ = toml.Decode(string(raw), &ws) - return ws + return ws, nil } -// SaveWorkspaces writes the list atomically. +// SaveWorkspaces writes the list atomically. A list that stands in for a +// file zn could not read is never written back over it. func SaveWorkspaces(ws Workspaces) error { + if ws.loadErr != nil { + return fmt.Errorf("not saving: %v. Overwriting the file would lose the vaults and servers it lists; fix or move it, then retry.", ws.loadErr) + } var buf bytes.Buffer buf.WriteString("# Vaults and servers zn knows. `zn connect`, `zn vault add` and `zn use` edit it.\n") if err := toml.NewEncoder(&buf).Encode(ws); err != nil { @@ -70,22 +93,45 @@ func SaveWorkspaces(ws Workspaces) error { type credentials struct { Tokens map[string]string `toml:"tokens"` + // loadErr: see Workspaces.loadErr; a token store zn could not read is + // never overwritten with a fresh one holding a single token. + loadErr error } func loadCredentials() credentials { + c, _ := loadCredentialsFile() + return c +} + +func loadCredentialsFile() (credentials, error) { c := credentials{Tokens: map[string]string{}} raw, err := os.ReadFile(CredentialsPath()) + if errors.Is(err, os.ErrNotExist) { + return c, nil + } + if err == nil { + _, err = toml.Decode(string(raw), &c) + } if err != nil { - return c + c = credentials{Tokens: map[string]string{}, loadErr: fmt.Errorf("%s: %w", CredentialsPath(), err)} + return c, c.loadErr } - _, _ = toml.Decode(string(raw), &c) if c.Tokens == nil { c.Tokens = map[string]string{} } - return c + return c, nil +} + +// CredentialsProblem says when the token store exists but cannot be read. +func CredentialsProblem() error { + _, err := loadCredentialsFile() + return err } func saveCredentials(c credentials) error { + if c.loadErr != nil { + return fmt.Errorf("not saving: %v. Overwriting the file would lose the other saved tokens; fix or move it, then retry.", c.loadErr) + } var buf bytes.Buffer buf.WriteString("# Server tokens for zn, keyed by URL. Keep this file private.\n") if err := toml.NewEncoder(&buf).Encode(c); err != nil { diff --git a/internal/config/workspaces_test.go b/internal/config/workspaces_test.go index a69a033..e710473 100644 --- a/internal/config/workspaces_test.go +++ b/internal/config/workspaces_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" ) @@ -49,3 +50,77 @@ func TestWorkspacesRoundTripAndTokens(t *testing.T) { t.Fatal("token deleted") } } + +// A workspaces.toml zn cannot parse must not be mistaken for an empty list +// and overwritten by the next command that saves: that would delete every +// vault and server the user connected. +func TestCorruptWorkspacesFileIsNeverOverwritten(t *testing.T) { + dir := t.TempDir() + t.Setenv("ZENNOTES_CONFIG_DIR", dir) + ws := Workspaces{} + ws.AddVault("alpha", filepath.Join(dir, "a")) + ws.AddServer("home", "https://notes.example.com") + if err := SaveWorkspaces(ws); err != nil { + t.Fatal(err) + } + original, _ := os.ReadFile(WorkspacesPath()) + broken := append(original, []byte("\n[server]\nname = \"x\"\n")...) // a table where an array of tables is expected + if err := os.WriteFile(WorkspacesPath(), broken, 0o644); err != nil { + t.Fatal(err) + } + + loaded, err := LoadWorkspacesFile() + if err == nil || len(loaded.Vaults) != 0 || len(loaded.Servers) != 0 { + t.Fatalf("a broken file must report its problem and list nothing: %+v %v", loaded, err) + } + loaded.AddVault("beta", filepath.Join(dir, "b")) + if err := SaveWorkspaces(loaded); err == nil || !strings.Contains(err.Error(), "not saving") { + t.Fatalf("save must refuse: %v", err) + } + after, _ := os.ReadFile(WorkspacesPath()) + if string(after) != string(broken) { + t.Fatal("the broken file was replaced") + } + + // Repaired, everything is back and saving works again. + if err := os.WriteFile(WorkspacesPath(), original, 0o644); err != nil { + t.Fatal(err) + } + repaired, err := LoadWorkspacesFile() + if err != nil || len(repaired.Vaults) != 1 || len(repaired.Servers) != 1 { + t.Fatalf("repaired: %+v %v", repaired, err) + } + if err := SaveWorkspaces(repaired); err != nil { + t.Fatal(err) + } + if _, err := LoadWorkspacesFile(); err != nil { + t.Fatal(err) + } +} + +func TestCorruptCredentialsFileIsNeverOverwritten(t *testing.T) { + dir := t.TempDir() + t.Setenv("ZENNOTES_CONFIG_DIR", dir) + if err := SaveToken("https://a.example.com", "token-a"); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(CredentialsPath(), []byte("[tokens]\n\"https://a.example.com\" = \"token-a\n"), 0o600); err != nil { + t.Fatal(err) + } + if CredentialsProblem() == nil { + t.Fatal("problem not reported") + } + if LoadToken("https://a.example.com") != "" { + t.Fatal("a broken store must not yield tokens") + } + if err := SaveToken("https://b.example.com", "token-b"); err == nil || !strings.Contains(err.Error(), "not saving") { + t.Fatalf("save must refuse: %v", err) + } + raw, _ := os.ReadFile(CredentialsPath()) + if string(raw) != "[tokens]\n\"https://a.example.com\" = \"token-a\n" { + t.Fatal("the broken store was replaced") + } + if err := DeleteToken("https://a.example.com"); err != nil { + t.Fatalf("deleting from a broken store is a no-op, not an overwrite: %v", err) + } +} From 7d36458e95445a13c7224872604cf2727c9e0dd6 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Wed, 30 Sep 2026 20:03:44 -0500 Subject: [PATCH 06/10] Fix(cli): report a missing note on delete, and exit quietly when a server run or MCP session ends `zn delete --yes` on a local vault printed "Deleted" and exited 0 while a server answers 404; the vault now reports "Note not found". Ctrl-C on `zn server run` printed "context canceled" twice (the exec error joined with the context's) and exited 130 for a shutdown the operator asked for; it exits 0 after the server's own shutdown log line. `zn mcp` exited 1 with "server is closing: EOF" whenever the client closed the pipe, which is how every MCP client ends a session. --- internal/mcp/server.go | 13 ++++++++++++- internal/server/lifecycle.go | 7 ++++++- internal/vault/vault.go | 6 +++++- internal/vault/vault_test.go | 20 ++++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 2d896e1..df9ded7 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -11,6 +11,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "sort" "strings" @@ -1093,7 +1094,17 @@ func Run(ctx context.Context, opts Options) error { if transport == nil { transport = &sdk.StdioTransport{} } - return server.Run(ctx, transport) + err := server.Run(ctx, transport) + if err == nil || ctx.Err() != nil || isClientDisconnect(err) { + return nil + } + return err +} + +// isClientDisconnect recognises the SDK's report that the client closed +// the pipe, which is how MCP clients end a session: not an error. +func isClientDisconnect(err error) bool { + return errors.Is(err, io.EOF) || strings.Contains(err.Error(), "server is closing") } func errorResult(text string) *sdk.CallToolResult { diff --git a/internal/server/lifecycle.go b/internal/server/lifecycle.go index 31ae915..c216422 100644 --- a/internal/server/lifecycle.go +++ b/internal/server/lifecycle.go @@ -340,7 +340,12 @@ func (m *Manager) Run(ctx context.Context, name string, output io.Writer) error } cmd.Stdout, cmd.Stderr = output, output if err := cmd.Run(); err != nil { - return errors.Join(err, ctx.Err()) + if ctx.Err() != nil { + // Ctrl-C: the operator stopped the server, which has already + // logged its shutdown; that is not an error to report twice. + return nil + } + return err } return nil } diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 01549db..bd349c7 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -812,7 +812,11 @@ func (v *Vault) DeleteNote(rel string) error { if err != nil { return err } - if err := os.Remove(abs); err != nil && !errors.Is(err, os.ErrNotExist) { + // A server answers 404 here; saying "Deleted" for a note that was never + // there hid typos in scripts, so the local vault reports it too. + if err := os.Remove(abs); errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("Note not found: %s", v.relPosix(abs)) + } else if err != nil { return err } v.invalidateLayout() diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 733f2ae..dbea22f 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -429,3 +429,23 @@ func TestCLIDialect(t *testing.T) { t.Fatalf("cli toggle #1 should reopen 'two': %q", next) } } + +// Deleting a note that is not there is an error, as it is on a server; +// "Deleted" for a typo hid the mistake from scripts. +func TestDeleteNoteReportsAMissingNote(t *testing.T) { + v := newTestVault(t) + writeFile(t, filepath.Join(v.Root(), "inbox", "Real.md"), "# Real\n") + if err := v.DeleteNote("inbox/Real.md"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(v.Root(), "inbox", "Real.md")); !os.IsNotExist(err) { + t.Fatal("note still exists") + } + err := v.DeleteNote("inbox/Real.md") + if err == nil || !strings.Contains(err.Error(), "Note not found: inbox/Real.md") { + t.Fatalf("second delete: %v", err) + } + if err := v.DeleteNote("inbox/never.md"); err == nil { + t.Fatal("deleting a note that never existed must fail") + } +} From 79ee1fc15eace70db061f8b789f9dbc7f58fc6cf Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Wed, 30 Sep 2026 20:14:17 -0500 Subject: [PATCH 07/10] Docs(readme): describe vault remove, the app marker in vault list and the unparseable-file guard --- README.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 60c4d53..4fdf2df 100644 --- a/README.md +++ b/README.md @@ -167,14 +167,17 @@ TUI when stdin/stdout are terminals and prints help when piped. Later: | Command | What it does | | --- | --- | -| `zn vault list` | Every saved vault and server, the default starred | +| `zn vault list` | Every saved vault and server, the default starred. Entries marked `app` come from the desktop app and are managed there | | `zn use ` | Set the terminal default (`zn use app` follows desktop again); desktop-managed scripts keep their own app default | | `zn disconnect ` | Forget a server and its token | +| `zn vault remove ` | Forget one of zn's own vaults or servers (a server's token too); note files stay | | `zn vault mode root\|inbox` | Move a vault between the flat layout and the classic `inbox/` layout | The list lives in `~/.config/zennotes/workspaces.toml`; server tokens go to `credentials.toml` next to it, readable only by you. Scripts and CI can skip -the store with `--server ` and `ZENNOTES_REMOTE_TOKEN`. +the store with `--server ` and `ZENNOTES_REMOTE_TOKEN`. If either file +stops parsing, zn says so (`zn doctor`, `zn status`) and refuses to write over +it rather than replace it with an empty list. ## Commands From 7675ad89c684a543cd3374dd154964bfc36eeb2f Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Wed, 30 Sep 2026 20:19:01 -0500 Subject: [PATCH 08/10] Test(termbg): run the pty children without the CI variable termenv keys off --- internal/termbg/termbg_linux_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/termbg/termbg_linux_test.go b/internal/termbg/termbg_linux_test.go index 09d8fd9..4ec58bf 100644 --- a/internal/termbg/termbg_linux_test.go +++ b/internal/termbg/termbg_linux_test.go @@ -44,7 +44,15 @@ func runOnTerminal(t *testing.T, mode string, answer func(master *os.File, seen t.Helper() master, slave := openPTY(t) cmd := exec.Command(os.Args[0]) - cmd.Env = append(os.Environ(), "ZN_TERMBG_CHILD="+mode, "TERM=xterm-256color", "COLORFGBG=") + // termenv treats any output as a non-terminal while CI is set, which + // would make both tests pass without exercising anything; the child is + // a user's terminal session, so it runs without the runner's variables. + for _, kv := range os.Environ() { + if k, _, _ := strings.Cut(kv, "="); k != "CI" && k != "GITHUB_ACTIONS" && k != "TERM" && k != "COLORFGBG" && k != "ZN_TERMBG_CHILD" { + cmd.Env = append(cmd.Env, kv) + } + } + cmd.Env = append(cmd.Env, "ZN_TERMBG_CHILD="+mode, "TERM=xterm-256color") cmd.Stdin, cmd.Stdout, cmd.Stderr = slave, slave, slave cmd.SysProcAttr = &unix.SysProcAttr{Setsid: true, Setctty: true, Ctty: 0} start := time.Now() From 6fb0444120052b2f71134e56fb13bd05489426f6 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Thu, 1 Oct 2026 07:05:28 -0500 Subject: [PATCH 09/10] Fix(cli): preserve workspace and credential safety on errors Prefer exact saved names during removal, report token cleanup failures, and restore tokens if saving the workspace list fails. Reject invalid credential tables, keep tokens out of parse diagnostics, and preserve the default when connecting with corrupt credentials. Add regression coverage for the PR #6 review findings. --- internal/cli/commands_workspace.go | 70 ++++++++++++++++---- internal/cli/vault_remove_test.go | 101 +++++++++++++++++++++++++++++ internal/cli/workspace_test.go | 29 +++++++++ internal/config/workspaces.go | 27 +++++++- internal/config/workspaces_test.go | 34 +++++++++- internal/tui/commands_parity.go | 4 ++ 6 files changed, 247 insertions(+), 18 deletions(-) diff --git a/internal/cli/commands_workspace.go b/internal/cli/commands_workspace.go index 0eaeea8..ddcbb9a 100644 --- a/internal/cli/commands_workspace.go +++ b/internal/cli/commands_workspace.go @@ -78,6 +78,9 @@ func cmdConnect(ctx context.Context, args Args) error { if raw == "" { return errors.New("Usage: zn connect [--name ] [--token ] [--no-default]") } + if err := config.CredentialsProblem(); err != nil { + return fmt.Errorf("not connecting: %w", err) + } ws := config.LoadWorkspaces() baseURL := "" name := strings.TrimSpace(args.Str("name")) @@ -156,16 +159,15 @@ func cmdDisconnect(args Args) error { return notSavedError(ws, name, "server") } url := saved.URL - ws.Remove(saved.Name) - if err := config.SaveWorkspaces(ws); err != nil { + savedName := saved.Name + if err := forgetSavedWorkspace(ws, savedName, url); err != nil { return err } - _ = config.DeleteToken(url) if args.Bool("json") { emitJSON(map[string]any{"ok": true, "name": name, "url": url}) return nil } - emitOK(fmt.Sprintf("Forgot %s (%s)", saved.Name, url)) + emitOK(fmt.Sprintf("Forgot %s (%s)", savedName, url)) return nil } @@ -322,19 +324,37 @@ func cmdVaultRemove(args Args) error { } ws := config.LoadWorkspaces() var kind, saved, location string - if v := ws.FindVault(name); v != nil { - kind, saved, location = "local", v.Name, v.Root - } else if s := ws.FindServer(name); s != nil { - kind, saved, location = "remote", s.Name, s.URL - } else { - return notSavedError(ws, name, "vault or server") + // An exact saved name wins over another entry's folder or host. Resolving + // those aliases first can silently forget the wrong workspace. + for _, v := range ws.Vaults { + if strings.EqualFold(v.Name, name) { + kind, saved, location = "local", v.Name, v.Root + break + } } - ws.Remove(saved) - if err := config.SaveWorkspaces(ws); err != nil { - return err + if saved == "" { + for _, s := range ws.Servers { + if strings.EqualFold(s.Name, name) { + kind, saved, location = "remote", s.Name, s.URL + break + } + } + } + if saved == "" { + if v := ws.FindVault(name); v != nil { + kind, saved, location = "local", v.Name, v.Root + } else if s := ws.FindServer(name); s != nil { + kind, saved, location = "remote", s.Name, s.URL + } else { + return notSavedError(ws, name, "vault or server") + } } + baseURL := "" if kind == "remote" { - _ = config.DeleteToken(location) + baseURL = location + } + if err := forgetSavedWorkspace(ws, saved, baseURL); err != nil { + return err } if args.Bool("json") { emitJSON(map[string]any{"ok": true, "name": saved, "kind": kind, "location": location}) @@ -348,6 +368,28 @@ func cmdVaultRemove(args Args) error { return nil } +// forgetSavedWorkspace keeps the entry retryable if token cleanup fails, and +// restores its token if saving the workspace list fails afterwards. +func forgetSavedWorkspace(ws config.Workspaces, name, baseURL string) error { + token := "" + if baseURL != "" { + token = config.LoadToken(baseURL) + if err := config.DeleteToken(baseURL); err != nil { + return fmt.Errorf("could not remove the saved token: %w", err) + } + } + ws.Remove(name) + if err := config.SaveWorkspaces(ws); err != nil { + if token != "" { + if restoreErr := config.SaveToken(baseURL, token); restoreErr != nil { + return errors.Join(err, fmt.Errorf("could not restore the saved token: %w", restoreErr)) + } + } + return err + } + return nil +} + // cmdInit creates a vault folder with its settings file and a first note, // remembers it, and makes it the default. func cmdInit(args Args) error { diff --git a/internal/cli/vault_remove_test.go b/internal/cli/vault_remove_test.go index 10d8ca9..4ec80a5 100644 --- a/internal/cli/vault_remove_test.go +++ b/internal/cli/vault_remove_test.go @@ -142,6 +142,107 @@ func TestVaultRemoveCompletesZnsOwnNames(t *testing.T) { } } +func TestVaultRemovePrefersSavedNamesOverPathsAndHosts(t *testing.T) { + for _, kind := range []string{"local", "remote", "remote host"} { + t.Run(kind, func(t *testing.T) { + dir := isolatedCLI(t) + t.Chdir(dir) + ws := config.Workspaces{} + if kind == "remote host" { + ws.AddServer("keep", "https://home") + } else { + ws.AddVault("keep", filepath.Join(dir, "home")) + } + if kind == "local" { + ws.AddVault("home", filepath.Join(dir, "other")) + } else { + ws.AddServer("home", "https://notes.example.com") + if err := config.SaveToken("https://notes.example.com", "token"); err != nil { + t.Fatal(err) + } + } + ws.Default = "keep" + if err := config.SaveWorkspaces(ws); err != nil { + t.Fatal(err) + } + code := 0 + out := captureOutput(t, func() { code = Main([]string{"vault", "remove", "home", "--json"}) }) + if code != 0 { + t.Fatalf("remove: exit %d %s", code, out) + } + remaining := config.LoadWorkspaces() + if names := remaining.Names(); len(names) != 1 || names[0] != "keep" || remaining.Default != "keep" { + t.Fatalf("removed the wrong entry: %+v; output: %s", remaining, out) + } + if kind != "local" && config.LoadToken("https://notes.example.com") != "" { + t.Fatal("the selected server's token was not deleted") + } + }) + } +} + +func TestForgetServerReportsStorageFailuresAndCanBeRetried(t *testing.T) { + for _, command := range [][]string{{"vault", "remove", "home"}, {"disconnect", "home"}} { + for _, problem := range []string{"corrupt", "write failure", "workspace write failure"} { + t.Run(strings.Join(command, " ")+"/"+problem, func(t *testing.T) { + isolatedCLI(t) + ws := config.Workspaces{Default: "home"} + ws.AddServer("home", "https://notes.example.com") + if err := config.SaveWorkspaces(ws); err != nil { + t.Fatal(err) + } + if err := config.SaveToken("https://notes.example.com", "token"); err != nil { + t.Fatal(err) + } + original, err := os.ReadFile(config.CredentialsPath()) + if err != nil { + t.Fatal(err) + } + broken := []byte("[tokens]\n\"https://notes.example.com\" = \"token\n") + failedPath := config.CredentialsPath() + if problem == "workspace write failure" { + failedPath = config.WorkspacesPath() + } + if problem == "corrupt" { + err = os.WriteFile(config.CredentialsPath(), broken, 0o600) + } else { + err = os.Mkdir(failedPath+".tmp", 0o700) + } + if err != nil { + t.Fatal(err) + } + code := 0 + out := captureOutput(t, func() { code = Main(append(command, "--json")) }) + if code == 0 || !strings.Contains(out, filepath.Base(failedPath)) { + t.Errorf("must report failed removal: exit %d %s", code, out) + } + if saved := config.LoadWorkspaces(); saved.FindServer("home") == nil || saved.Default != "home" { + t.Fatalf("must keep the entry so removal can be retried: %+v", saved) + } + want := original + if problem == "corrupt" { + want = broken + } + if raw, err := os.ReadFile(config.CredentialsPath()); err != nil || string(raw) != string(want) { + t.Fatalf("credentials changed on failure: %q %v", raw, err) + } + if problem == "corrupt" { + err = os.WriteFile(config.CredentialsPath(), original, 0o600) + } else { + err = os.Remove(failedPath + ".tmp") + } + if err != nil { + t.Fatal(err) + } + out = captureOutput(t, func() { code = Main(command) }) + if code != 0 || config.LoadWorkspaces().FindServer("home") != nil || config.LoadToken("https://notes.example.com") != "" { + t.Fatalf("retry did not remove the server and token: exit %d %s", code, out) + } + }) + } + } +} + // With an unparseable workspaces.toml, every command that would save must // refuse rather than write an empty list over the user's saved entries. func TestCommandsRefuseToOverwriteABrokenWorkspacesFile(t *testing.T) { diff --git a/internal/cli/workspace_test.go b/internal/cli/workspace_test.go index dd36d7f..dea2771 100644 --- a/internal/cli/workspace_test.go +++ b/internal/cli/workspace_test.go @@ -103,6 +103,35 @@ func TestConnectInitUseAndList(t *testing.T) { } } +func TestConnectWithBrokenCredentialsKeepsTheSavedDefault(t *testing.T) { + dir := isolatedCLI(t) + ws := config.Workspaces{Default: "notes"} + ws.AddVault("notes", dir) + if err := config.SaveWorkspaces(ws); err != nil { + t.Fatal(err) + } + original, err := os.ReadFile(config.WorkspacesPath()) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(config.CredentialsPath(), []byte("[tokens]\n\"https://other.example.com\" = \"unterminated\n"), 0o600); err != nil { + t.Fatal(err) + } + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"root":"/srv/notes","name":"notes"}`)) + })) + defer server.Close() + code := 0 + out := captureOutput(t, func() { code = Main([]string{"connect", server.URL, "--token", "right", "--json"}) }) + if code == 0 || !strings.Contains(out, "credentials.toml") { + t.Fatalf("must report the broken store: exit %d %s", code, out) + } + if raw, err := os.ReadFile(config.WorkspacesPath()); err != nil || string(raw) != string(original) { + t.Fatalf("failed connection changed saved workspaces: %s %v", raw, err) + } +} + // A desktop-managed zn follows the app, so `zn connect` must not claim zn // now uses the server by default; what it does is give zn (and zn mcp) the // token for the server whenever the app has it open. diff --git a/internal/config/workspaces.go b/internal/config/workspaces.go index 463ee2a..7709cc8 100644 --- a/internal/config/workspaces.go +++ b/internal/config/workspaces.go @@ -110,7 +110,27 @@ func loadCredentialsFile() (credentials, error) { return c, nil } if err == nil { - _, err = toml.Decode(string(raw), &c) + var md toml.MetaData + md, err = toml.Decode(string(raw), &c) + if err != nil { + // Parser messages and key names can contain the token itself. Keep + // only the location; these errors also appear in doctor output. + var parseErr toml.ParseError + if errors.As(err, &parseErr) { + err = fmt.Errorf("invalid credentials TOML at line %d; fix or move the file, then retry", parseErr.Position.Line) + } else { + err = errors.New("invalid credentials TOML; fix or move the file, then retry") + } + } else { + // The decoder silently ignores non-table values assigned to a map. + // Treat that as a broken store instead of overwriting it as empty. + for _, key := range md.Keys() { + if len(key) == 1 && strings.EqualFold(key[0], "tokens") && md.Type(key...) != "Hash" { + err = errors.New("credentials must contain a [tokens] table; fix or move the file, then retry") + break + } + } + } } if err != nil { c = credentials{Tokens: map[string]string{}, loadErr: fmt.Errorf("%s: %w", CredentialsPath(), err)} @@ -154,7 +174,10 @@ func SaveToken(baseURL, token string) error { // DeleteToken forgets a server token. func DeleteToken(baseURL string) error { - c := loadCredentials() + c, err := loadCredentialsFile() + if err != nil { + return err + } if _, ok := c.Tokens[tokenKey(baseURL)]; !ok { return nil } diff --git a/internal/config/workspaces_test.go b/internal/config/workspaces_test.go index e710473..928aadd 100644 --- a/internal/config/workspaces_test.go +++ b/internal/config/workspaces_test.go @@ -120,7 +120,37 @@ func TestCorruptCredentialsFileIsNeverOverwritten(t *testing.T) { if string(raw) != "[tokens]\n\"https://a.example.com\" = \"token-a\n" { t.Fatal("the broken store was replaced") } - if err := DeleteToken("https://a.example.com"); err != nil { - t.Fatalf("deleting from a broken store is a no-op, not an overwrite: %v", err) + if err := DeleteToken("https://a.example.com"); err == nil { + t.Fatal("deleting from a broken store must report that the token could not be removed") + } +} + +func TestCredentialErrorsDoNotExposeFileContents(t *testing.T) { + const secret = "testsecrettoken" + for name, broken := range map[string]string{ + "unquoted value": "[tokens]\n\"https://notes.example.com\" = " + secret + "\n", + "token as key": "[tokens]\n" + secret + " = \"unterminated\n", + "wrong type": "tokens = \"" + secret + "\"\n", + } { + t.Run(name, func(t *testing.T) { + t.Setenv("ZENNOTES_CONFIG_DIR", t.TempDir()) + if err := os.WriteFile(CredentialsPath(), []byte(broken), 0o600); err != nil { + t.Fatal(err) + } + for operation, err := range map[string]error{ + "diagnose": CredentialsProblem(), + "save": SaveToken("https://other.example.com", "other"), + "delete": DeleteToken("https://notes.example.com"), + } { + if err == nil || !strings.Contains(err.Error(), "credentials.toml") { + t.Errorf("%s: expected an actionable credentials error, got %v", operation, err) + } else if strings.Contains(err.Error(), secret) { + t.Errorf("%s exposed a credential in the diagnostic: %v", operation, err) + } + } + if raw, err := os.ReadFile(CredentialsPath()); err != nil || string(raw) != broken { + t.Fatalf("invalid credentials were overwritten: %q %v", raw, err) + } + }) } } diff --git a/internal/tui/commands_parity.go b/internal/tui/commands_parity.go index cd89ffb..03a4544 100644 --- a/internal/tui/commands_parity.go +++ b/internal/tui/commands_parity.go @@ -440,6 +440,10 @@ func (a *App) connectServerFlow(rawURL string) { token = target.AuthToken } finish := func(a *App, token string) { + if err := config.CredentialsProblem(); err != nil { + a.notifyError("not connecting: " + err.Error()) + return + } client := remote.NewClient(baseURL, token) if _, err := client.GetCurrentVault(a.ctx); err != nil { switch remote.StatusOf(err) { From ee4fe4287d8b48c39472208c170862367f33cc5c Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Thu, 1 Oct 2026 07:05:34 -0500 Subject: [PATCH 10/10] Fix(mcp): preserve transport errors during shutdown Only suppress cancellation and normal EOF disconnects, including the SDK's exact closing message. Preserve genuine transport failures and deadlines, with regression coverage for normal client disconnects and error classification. --- internal/mcp/run_test.go | 54 ++++++++++++++++++++++++++++++++++++++++ internal/mcp/server.go | 6 +++-- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/internal/mcp/run_test.go b/internal/mcp/run_test.go index 3641000..412a63f 100644 --- a/internal/mcp/run_test.go +++ b/internal/mcp/run_test.go @@ -3,6 +3,9 @@ package mcp import ( "context" "encoding/json" + "errors" + "fmt" + "io" "net/http" "net/http/httptest" "os" @@ -16,6 +19,57 @@ import ( "github.com/ZenNotes/tui/internal/backend" ) +type failingTransport struct{ err error } + +func (t failingTransport) Connect(context.Context) (sdk.Connection, error) { + return nil, t.err +} + +func TestRunOnlySuppressesNormalShutdownErrors(t *testing.T) { + for _, tc := range []struct { + name string + err error + ok bool + }{ + {"EOF", io.EOF, true}, + {"wrapped EOF", fmt.Errorf("read: %w", io.EOF), true}, + {"SDK EOF", errors.New("server is closing: EOF"), true}, + {"canceled", context.Canceled, true}, + {"deadline", context.DeadlineExceeded, false}, + {"truncated input", errors.New("server is closing: unexpected EOF"), false}, + {"transport failure", errors.New("server is closing: input/output error"), false}, + } { + t.Run(tc.name, func(t *testing.T) { + err := Run(context.Background(), Options{Transport: failingTransport{tc.err}}) + if tc.ok && err != nil { + t.Fatalf("normal shutdown must be quiet: %v", err) + } + if !tc.ok && !errors.Is(err, tc.err) { + t.Fatalf("lost transport failure: got %v, want %v", err, tc.err) + } + }) + } +} + +func TestRunReturnsOnClientDisconnect(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + serverT, clientT := sdk.NewInMemoryTransports() + done := make(chan error, 1) + go func() { done <- Run(ctx, Options{Transport: serverT}) }() + client := sdk.NewClient(&sdk.Implementation{Name: "test", Version: "0"}, nil) + cs, err := client.Connect(ctx, clientT, nil) + if err != nil { + t.Fatal(err) + } + if err := cs.Close(); err != nil { + t.Fatal(err) + } + if err := <-done; err != nil { + t.Fatalf("client disconnect must be quiet: %v", err) + } +} + // runServer serves Run over an in-memory transport and returns a connected // client; resolve is consulted before every tool call, like the app's config. func runServer(t *testing.T, resolve func() (backend.Target, error)) *sdk.ClientSession { diff --git a/internal/mcp/server.go b/internal/mcp/server.go index df9ded7..258a602 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -1095,7 +1095,7 @@ func Run(ctx context.Context, opts Options) error { transport = &sdk.StdioTransport{} } err := server.Run(ctx, transport) - if err == nil || ctx.Err() != nil || isClientDisconnect(err) { + if err == nil || errors.Is(err, context.Canceled) || isClientDisconnect(err) { return nil } return err @@ -1104,7 +1104,9 @@ func Run(ctx context.Context, opts Options) error { // isClientDisconnect recognises the SDK's report that the client closed // the pipe, which is how MCP clients end a session: not an error. func isClientDisconnect(err error) bool { - return errors.Is(err, io.EOF) || strings.Contains(err.Error(), "server is closing") + // The SDK sometimes formats EOF with %v rather than wrapping it. Match + // that exact fallback; the same prefix can accompany real I/O failures. + return errors.Is(err, io.EOF) || err.Error() == "server is closing: EOF" } func errorResult(text string) *sdk.CallToolResult {