From a269245aee94734afbbcf8a51107d876f9d04ee8 Mon Sep 17 00:00:00 2001 From: Gaurav Bhatia Date: Tue, 1 Sep 2026 16:47:51 +0400 Subject: [PATCH 1/8] feat(notify): make permission-prompt alerts on by default and discoverable The notify system existed but was silent unless the user hand-edited config.json, with no UI surface to discover or change it. - resolver: fall back to mode=both, focusMode=unfocused when the notify block is missing or empty (Fixes #579) - tui: add /notify slash command with popup picker, mirroring /theme; explicit choices persist via config.SetNotify - cli: add `zero config notify` to read/update/reset the preference (--mode, --focus, --reset, --json) - config: add SetNotify writer using the existing atomic-write helper, validating against the same vocab the resolver accepts The TUI effectiveTUINotifyMode default (empty -> both) now matches the resolver. exec_test.go seeds notify.mode=off where a test asserted silent stderr, which the old empty-default implicitly provided. --- internal/cli/command_center.go | 29 +++- internal/cli/config_notify.go | 148 +++++++++++++++++ internal/cli/config_notify_test.go | 255 +++++++++++++++++++++++++++++ internal/cli/exec_test.go | 3 +- internal/config/resolver.go | 24 +++ internal/config/resolver_test.go | 55 ++++++- internal/config/writer.go | 44 +++++ internal/config/writer_test.go | 62 +++++++ internal/tui/commands.go | 8 + internal/tui/model.go | 63 +++++-- internal/tui/model_test.go | 23 +++ internal/tui/notify_select.go | 158 ++++++++++++++++++ internal/tui/notify_select_test.go | 184 +++++++++++++++++++++ internal/tui/picker.go | 27 +++ 14 files changed, 1068 insertions(+), 15 deletions(-) create mode 100644 internal/cli/config_notify.go create mode 100644 internal/cli/config_notify_test.go create mode 100644 internal/tui/notify_select.go create mode 100644 internal/tui/notify_select_test.go diff --git a/internal/cli/command_center.go b/internal/cli/command_center.go index e6c4f5dac..3cd05f4fa 100644 --- a/internal/cli/command_center.go +++ b/internal/cli/command_center.go @@ -26,6 +26,30 @@ type modelSummary = zerocommands.ModelSnapshot type providerCatalogSummary = zerocommands.ProviderCatalogSnapshot func runConfig(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + // The first non-flag argument is a subcommand. With no positional argument + // (or only flag arguments), the read-only summary path runs — this keeps + // `zero config` and `zero config --json` working unchanged. + command := "summary" + rest := args + if len(args) > 0 && !strings.HasPrefix(args[0], "-") { + command = strings.ToLower(strings.TrimSpace(args[0])) + rest = args[1:] + } + switch command { + case "summary": + return runConfigSummary(rest, stdout, stderr, deps) + case "notify": + return runConfigNotify(rest, stdout, stderr, deps) + case "help": + if err := writeConfigHelp(stdout); err != nil { + return exitCrash + } + return exitSuccess + } + return writeExecUsageError(stderr, fmt.Sprintf("unknown config command %q", command)) +} + +func runConfigSummary(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseCommandCenterArgs(args, false, false) if err != nil { return writeExecUsageError(stderr, err.Error()) @@ -433,8 +457,11 @@ func formatProviderCatalogValue(value string, fallback string) string { func writeConfigHelp(w io.Writer) error { _, err := fmt.Fprint(w, `Usage: zero config [flags] + zero config notify [flags] -Inspects resolved Go configuration without printing secrets. +Inspects resolved Go configuration without printing secrets. The notify +subcommand reads or updates the permission-prompt alert preference — +run "zero config notify --help" for details. Flags: --json Print JSON summary diff --git a/internal/cli/config_notify.go b/internal/cli/config_notify.go new file mode 100644 index 000000000..3715870f9 --- /dev/null +++ b/internal/cli/config_notify.go @@ -0,0 +1,148 @@ +package cli + +import ( + "fmt" + "io" + "strings" + + "github.com/Gitlawb/zero/internal/config" +) + +// runConfigNotify implements `zero config notify`: with no flags it prints the +// current mode/focusMode; --mode/--focus update them via the same +// config.SetNotify writer the TUI /notify command uses, so all surfaces stay +// in lockstep; --reset blanks both fields so the resolver defaults apply. +func runConfigNotify(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + options, help, err := parseConfigNotifyArgs(args) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + if help { + if err := writeConfigNotifyHelp(stdout); err != nil { + return exitCrash + } + return exitSuccess + } + + resolved, exitCode := resolveCommandCenterConfig(stderr, deps) + if exitCode != exitSuccess { + return exitCode + } + + if options.mode != "" || options.focus != "" || options.reset { + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + notify := config.NotifyConfig{Mode: options.mode, FocusMode: options.focus} + if options.reset { + notify = config.NotifyConfig{} + } + if _, err := config.SetNotify(configPath, notify); err != nil { + return writeAppError(stderr, err.Error(), exitUsage) + } + // Re-resolve so the printed value reflects what the next launch will + // actually use (e.g. a reset shows the built-in defaults). + resolved, exitCode = resolveCommandCenterConfig(stderr, deps) + if exitCode != exitSuccess { + return exitCode + } + } + + if options.json { + if err := writePrettyJSON(stdout, map[string]any{ + "mode": resolved.Notify.Mode, + "focusMode": resolved.Notify.FocusMode, + }); err != nil { + return exitCrash + } + return exitSuccess + } + lines := []string{ + "Notify", + "mode: " + displayCLIValue(resolved.Notify.Mode, "(default)"), + "focusMode: " + displayCLIValue(resolved.Notify.FocusMode, "(default)"), + } + if _, err := fmt.Fprintln(stdout, strings.Join(lines, "\n")); err != nil { + return exitCrash + } + return exitSuccess +} + +type configNotifyOptions struct { + mode string + focus string + reset bool + json bool +} + +func parseConfigNotifyArgs(args []string) (configNotifyOptions, bool, error) { + options := configNotifyOptions{} + for index := 0; index < len(args); index++ { + arg := args[index] + switch { + case arg == "-h" || arg == "--help" || arg == "help": + return options, true, nil + case arg == "--json": + options.json = true + case arg == "--reset": + options.reset = true + case arg == "--mode": + value, next, err := nextFlagValue(args, index, arg) + if err != nil { + return options, false, err + } + options.mode = value + index = next + case strings.HasPrefix(arg, "--mode="): + value, err := requiredInlineFlagValue(arg, "--mode") + if err != nil { + return options, false, err + } + options.mode = value + case arg == "--focus": + value, next, err := nextFlagValue(args, index, arg) + if err != nil { + return options, false, err + } + options.focus = value + index = next + case strings.HasPrefix(arg, "--focus="): + value, err := requiredInlineFlagValue(arg, "--focus") + if err != nil { + return options, false, err + } + options.focus = value + case strings.HasPrefix(arg, "-"): + return options, false, execUsageError{fmt.Sprintf("unknown flag %q", arg)} + default: + return options, false, execUsageError{fmt.Sprintf("unexpected argument %q", arg)} + } + } + return options, false, nil +} + +func writeConfigNotifyHelp(w io.Writer) error { + _, err := fmt.Fprint(w, "Usage:\n"+ + " zero config notify [flags]\n"+ + "\n"+ + "Print or update the permission-prompt notify preference.\n"+ + "\n"+ + "When run with no flag, prints the current mode and focusMode (the resolver\n"+ + "defaults to \"both\" and \"unfocused\" when the config block is empty).\n"+ + "\n"+ + "Examples:\n"+ + " zero config notify\n"+ + " zero config notify --json\n"+ + " zero config notify --mode both --focus unfocused\n"+ + " zero config notify --mode off\n"+ + " zero config notify --reset # clear config so the resolver defaults apply\n"+ + "\n"+ + "Flags:\n"+ + " --mode Notification mechanism\n"+ + " --focus When the alert fires\n"+ + " --reset Clear both fields so the resolver defaults apply\n"+ + " --json Machine-readable output\n"+ + " -h, --help Show this help\n") + return err +} diff --git a/internal/cli/config_notify_test.go b/internal/cli/config_notify_test.go new file mode 100644 index 000000000..c0bd9aa7f --- /dev/null +++ b/internal/cli/config_notify_test.go @@ -0,0 +1,255 @@ +package cli + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// `zero config notify` with no flag and a fresh config: the resolver applies +// the built-in defaults (mode=both, focusMode=unfocused) and the command +// reports them. This is the "just works" case a new user lands in. We use a +// real on-disk config (not the synthetic commandCenterDeps fixture, which +// returns an empty Notify field) because the resolver-default behavior is the +// whole point of this test. +func TestRunConfigNotifyPrintsResolverDefaults(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + // A valid openai profile so the resolver does not error with + // ErrNoActiveProvider. The notify defaults are applied independently of + // the provider resolution path. + seed := `{ + "activeProvider": "openai", + "providers": [{ + "name": "openai", + "providerKind": "openai", + "baseUrl": "https://api.openai.com/v1", + "model": "gpt-4.1", + "apiKeyEnv": "OPENAI_API_KEY" + }] + }` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + if !strings.Contains(stdout.String(), "mode: both") { + t.Errorf("stdout should show the default mode, got: %s", stdout.String()) + } + if !strings.Contains(stdout.String(), "focusMode: unfocused") { + t.Errorf("stdout should show the default focus, got: %s", stdout.String()) + } +} + +// `zero config notify --json` emits a machine-readable payload so scripts can +// read the resolved preference without parsing prose. Same real-resolver +// fixture as the print-defaults test, since the JSON path reads the same +// `resolved.Notify` that the print path does. +func TestRunConfigNotifyPrintsJSON(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + seed := `{ + "activeProvider": "openai", + "providers": [{ + "name": "openai", + "providerKind": "openai", + "baseUrl": "https://api.openai.com/v1", + "model": "gpt-4.1", + "apiKeyEnv": "OPENAI_API_KEY" + }] + }` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--json"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + var payload map[string]any + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, stdout.String()) + } + if payload["mode"] != "both" { + t.Errorf("mode = %v, want both", payload["mode"]) + } + if payload["focusMode"] != "unfocused" { + t.Errorf("focusMode = %v, want unfocused", payload["focusMode"]) + } +} + +// `zero config notify --mode off` writes the new value to disk and prints +// confirmation. The user can read it back by running the command again. +func TestRunConfigNotifyWritesModeChange(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + seed := `{ + "activeProvider": "openai", + "providers": [{ + "name": "openai", + "providerKind": "openai", + "baseUrl": "https://api.openai.com/v1", + "model": "gpt-4.1", + "apiKeyEnv": "OPENAI_API_KEY" + }] + }` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--mode", "off"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if cfg.Notify.Mode != "off" { + t.Errorf("Notify.Mode = %q, want off", cfg.Notify.Mode) + } + if !strings.Contains(stdout.String(), "mode: off") { + t.Errorf("stdout should confirm the change, got: %s", stdout.String()) + } +} + +// `--mode` and `--focus` together update both fields in one call. +func TestRunConfigNotifyWritesModeAndFocus(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + seed := `{ + "activeProvider": "openai", + "providers": [{ + "name": "openai", + "providerKind": "openai", + "baseUrl": "https://api.openai.com/v1", + "model": "gpt-4.1", + "apiKeyEnv": "OPENAI_API_KEY" + }] + }` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--mode", "both", "--focus", "unfocused"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if cfg.Notify.Mode != "both" || cfg.Notify.FocusMode != "unfocused" { + t.Errorf("Notify = %+v, want mode=both focusMode=unfocused", cfg.Notify) + } +} + +// `--mode loud` is a usage error. The config must not be mutated. +func TestRunConfigNotifyRejectsInvalidMode(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + seed := `{ + "activeProvider": "openai", + "providers": [{ + "name": "openai", + "providerKind": "openai", + "baseUrl": "https://api.openai.com/v1", + "model": "gpt-4.1", + "apiKeyEnv": "OPENAI_API_KEY" + }], + "notify": {"mode": "off"} + }` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--mode", "loud"}, &stdout, &stderr, deps) + if exitCode == exitSuccess { + t.Fatalf("expected failure for invalid mode, got success; stdout=%s", stdout.String()) + } + cfg := readFileConfig(t, configPath) + if cfg.Notify.Mode != "off" { + t.Errorf("Notify.Mode = %q, want off (unchanged after failed write)", cfg.Notify.Mode) + } +} + +// `--reset` blanks both fields so the resolver defaults apply on the next +// resolve. Useful for "go back to the recommended setup" after a custom value. +func TestRunConfigNotifyResetClearsStoredValues(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + seed := `{ + "activeProvider": "openai", + "providers": [{ + "name": "openai", + "providerKind": "openai", + "baseUrl": "https://api.openai.com/v1", + "model": "gpt-4.1", + "apiKeyEnv": "OPENAI_API_KEY" + }], + "notify": {"mode": "off", "focusMode": "always"} + }` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--reset"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if cfg.Notify.Mode != "" || cfg.Notify.FocusMode != "" { + t.Errorf("Notify after reset = %+v, want empty (defaults apply)", cfg.Notify) + } +} + +// `zero config` (no subcommand) still works after the dispatch change. +func TestRunConfigSummaryStillWorks(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config"}, &stdout, &stderr, commandCenterDeps(t)) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + if !strings.Contains(stdout.String(), "Config") { + t.Errorf("stdout should show the config summary, got: %s", stdout.String()) + } +} diff --git a/internal/cli/exec_test.go b/internal/cli/exec_test.go index 3cc4f8fe8..64a0a0895 100644 --- a/internal/cli/exec_test.go +++ b/internal/cli/exec_test.go @@ -968,7 +968,8 @@ func TestRunExecUsesProjectConfigAndOpenAICompatibleProvider(t *testing.T) { "base_url": "` + server.URL + `", "api_key": "sk-local", "model": "local-model" - }] + }], + "notify": {"mode": "off"} }` if err := os.WriteFile(filepath.Join(configDir, "config.json"), []byte(writeConfig), 0o600); err != nil { t.Fatal(err) diff --git a/internal/config/resolver.go b/internal/config/resolver.go index 713cc7c83..846759282 100644 --- a/internal/config/resolver.go +++ b/internal/config/resolver.go @@ -61,6 +61,18 @@ const MaxTurnsCeiling = 500 // (set 0 to always advertise every schema, e.g. for a model without tool_search). const defaultDeferThreshold = 3 +// defaultNotifyMode and defaultNotifyFocus are the fallback values used when +// config.json is missing, has no notify block, or has an empty notify block. +// both = terminal bell + OSC-9 desktop notification; unfocused = fire only +// when the TUI window is not the active window so users looking at the prompt +// are not spammed. The defaults make the permission-prompt alert "just work" +// for new users; the TUI /notify command and `zero config notify` let users +// change or opt out. +const ( + defaultNotifyMode = "both" + defaultNotifyFocus = "unfocused" +) + func Resolve(options ResolveOptions) (ResolvedConfig, error) { cfg := FileConfig{ MaxTurns: defaultMaxTurns, @@ -95,6 +107,18 @@ func Resolve(options ResolveOptions) (ResolvedConfig, error) { applyOverrides(&cfg, options.Overrides) + // Notify defaults: when the user has not configured notify (no block, or + // an empty block), apply the built-in defaults so the permission-prompt + // alert works out of the box. A user who explicitly sets notify.mode=off + // or notify.focusMode=focused still wins because their value is + // non-empty after the trim in the validation step below. + if strings.TrimSpace(cfg.Notify.Mode) == "" { + cfg.Notify.Mode = defaultNotifyMode + } + if strings.TrimSpace(cfg.Notify.FocusMode) == "" { + cfg.Notify.FocusMode = defaultNotifyFocus + } + if !cfg.Tools.deferThresholdSet && cfg.Tools.DeferThreshold == 0 { cfg.Tools.DeferThreshold = defaultDeferThreshold } diff --git a/internal/config/resolver_test.go b/internal/config/resolver_test.go index 6daa691a8..10981c982 100644 --- a/internal/config/resolver_test.go +++ b/internal/config/resolver_test.go @@ -1515,8 +1515,59 @@ func TestResolveNotifyDefaultEmpty(t *testing.T) { if err != nil { t.Fatalf("Resolve: %v", err) } - if resolved.Notify.Mode != "" || resolved.Notify.FocusMode != "" { - t.Fatalf("unset notify should be empty, got %+v", resolved.Notify) + // Missing notify block falls back to the built-in defaults so the + // permission-prompt alert works for users who never ran setup. + if resolved.Notify.Mode != "both" { + t.Errorf("unset notify.mode should default to %q, got %q", "both", resolved.Notify.Mode) + } + if resolved.Notify.FocusMode != "unfocused" { + t.Errorf("unset notify.focusMode should default to %q, got %q", "unfocused", resolved.Notify.FocusMode) + } +} + +func TestResolveNotifyDefaultEmptyBlock(t *testing.T) { + // An explicit empty notify block should behave the same as a missing one: + // fall back to the built-in defaults. + path := writeConfig(t, `{"notify":{}}`) + resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if resolved.Notify.Mode != "both" { + t.Errorf("empty notify.mode should default to %q, got %q", "both", resolved.Notify.Mode) + } + if resolved.Notify.FocusMode != "unfocused" { + t.Errorf("empty notify.focusMode should default to %q, got %q", "unfocused", resolved.Notify.FocusMode) + } +} + +func TestResolveNotifyDefaultPartialEmpty(t *testing.T) { + // Only one field is set; the other should still get the default. + path := writeConfig(t, `{"notify":{"mode":"off"}}`) + resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if resolved.Notify.Mode != "off" { + t.Errorf("notify.mode should be preserved as %q, got %q", "off", resolved.Notify.Mode) + } + if resolved.Notify.FocusMode != "unfocused" { + t.Errorf("empty notify.focusMode should default to %q, got %q", "unfocused", resolved.Notify.FocusMode) + } +} + +func TestResolveNotifyDefaultNoConfigFile(t *testing.T) { + // No config file at all: defaults should still apply so the + // permission-prompt alert is on for first-run users. + resolved, err := Resolve(ResolveOptions{UserConfigPath: "", Env: map[string]string{}}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if resolved.Notify.Mode != "both" { + t.Errorf("missing config: notify.mode should default to %q, got %q", "both", resolved.Notify.Mode) + } + if resolved.Notify.FocusMode != "unfocused" { + t.Errorf("missing config: notify.focusMode should default to %q, got %q", "unfocused", resolved.Notify.FocusMode) } } diff --git a/internal/config/writer.go b/internal/config/writer.go index f27740a01..6b35bf018 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -7,6 +7,8 @@ import ( "path/filepath" "sort" "strings" + + "github.com/Gitlawb/zero/internal/notify" ) func UpsertProvider(path string, profile ProviderProfile, setActive bool) (FileConfig, error) { @@ -176,6 +178,48 @@ func SetTheme(path string, theme string) (FileConfig, error) { return cfg, nil } +// SetNotify persists the TUI notification preference. Both fields are trimmed +// and validated against the accepted vocab (mode in {off,bell,notify,both}; +// focusMode in {unfocused,always,focused}) so a bad caller cannot write a value +// the resolver would later reject at startup. An empty Mode or FocusMode is +// stored as-is — the resolver applies the built-in defaults at read time, so a +// blank value means "use defaults" rather than "no notify" or "no focus rule". +func SetNotify(path string, value NotifyConfig) (FileConfig, error) { + path = strings.TrimSpace(path) + if path == "" { + return FileConfig{}, fmt.Errorf("config path is required") + } + value.Mode = strings.TrimSpace(value.Mode) + value.FocusMode = strings.TrimSpace(value.FocusMode) + if mode := value.Mode; mode != "" { + switch notify.Mode(mode) { + case notify.ModeOff, notify.ModeBell, notify.ModeNotify, notify.ModeBoth: + default: + return FileConfig{}, fmt.Errorf("invalid notify.mode %q: expected off, bell, notify, or both", mode) + } + } + if focus := value.FocusMode; focus != "" { + switch notify.FocusMode(focus) { + case notify.FocusUnfocused, notify.FocusAlways, notify.FocusFocused: + default: + return FileConfig{}, fmt.Errorf("invalid notify.focusMode %q: expected unfocused, always, or focused", focus) + } + } + cfg := FileConfig{} + if data, err := os.ReadFile(path); err == nil { + if err := json.Unmarshal(data, &cfg); err != nil { + return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + } else if !os.IsNotExist(err) { + return FileConfig{}, fmt.Errorf("read config %s: %w", path, err) + } + cfg.Notify = value + if err := writeConfigFile(path, cfg); err != nil { + return FileConfig{}, err + } + return cfg, nil +} + func normalizeFavoriteModels(models []string) []string { seen := map[string]bool{} favorites := make([]string, 0, len(models)) diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 93a849706..6600b4ecd 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -305,6 +305,68 @@ func TestSetThemePersistsUserPreference(t *testing.T) { } } +func TestSetNotifyPersistsValidValues(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "openai", + Providers: []ProviderProfile{ + {Name: "openai", ProviderKind: ProviderKindOpenAI, Model: "gpt-4.1"}, + }, + }, 0o600) + + cfg, err := SetNotify(path, NotifyConfig{Mode: " both ", FocusMode: " unfocused "}) + if err != nil { + t.Fatalf("SetNotify() error = %v", err) + } + if cfg.Notify.Mode != "both" || cfg.Notify.FocusMode != "unfocused" { + t.Fatalf("Notify = %+v, want mode=both focusMode=unfocused (trimmed)", cfg.Notify) + } + persisted := readConfigFixture(t, path) + if persisted.Notify.Mode != "both" || persisted.Notify.FocusMode != "unfocused" { + t.Fatalf("persisted Notify = %+v, want mode=both focusMode=unfocused", persisted.Notify) + } + if persisted.ActiveProvider != "openai" || len(persisted.Providers) != 1 { + t.Fatalf("provider config was not preserved by SetNotify: %#v", persisted) + } +} + +func TestSetNotifyRejectsInvalidMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ActiveProvider: "openai"}, 0o600) + if _, err := SetNotify(path, NotifyConfig{Mode: "loud", FocusMode: "unfocused"}); err == nil { + t.Fatal("expected error for invalid notify.mode") + } +} + +func TestSetNotifyRejectsInvalidFocusMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ActiveProvider: "openai"}, 0o600) + if _, err := SetNotify(path, NotifyConfig{Mode: "off", FocusMode: "sideways"}); err == nil { + t.Fatal("expected error for invalid notify.focusMode") + } +} + +func TestSetNotifyRejectsEmptyConfigPath(t *testing.T) { + if _, err := SetNotify("", NotifyConfig{Mode: "off"}); err == nil { + t.Fatal("expected error for empty config path") + } +} + +func TestSetNotifyBlankValuesPreservedAsDefaults(t *testing.T) { + // An empty mode/focusMode stored on disk is a valid "use the resolver + // defaults" signal — SetNotify must not reject blanks, and they must round + // trip unchanged so the resolver can apply its built-in fallback. + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ActiveProvider: "openai"}, 0o600) + if _, err := SetNotify(path, NotifyConfig{}); err != nil { + t.Fatalf("SetNotify({}) should accept blank values, got error: %v", err) + } + persisted := readConfigFixture(t, path) + if persisted.Notify.Mode != "" || persisted.Notify.FocusMode != "" { + t.Fatalf("blank notify values should round-trip, got %+v", persisted.Notify) + } +} + func TestRecapsPreferenceRoundTrips(t *testing.T) { // Default (unset) is ON. if !(PreferencesConfig{}).RecapsEnabled() { diff --git a/internal/tui/commands.go b/internal/tui/commands.go index e98665117..b8db8467d 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -35,6 +35,7 @@ const ( commandEffort commandStyle commandTheme + commandNotify commandTranscript commandBash commandImage @@ -342,6 +343,13 @@ var commandDefinitions = []commandDefinition{ description: "Pick a color theme (no arg opens the picker; auto detects the terminal background).", kind: commandTheme, }, + { + name: "/notify", + usage: "/notify [list|off|bell|notify|both [unfocused|always]]", + group: commandGroupSession, + description: "Pick when Zero alerts you it needs input. No arg opens the picker.", + kind: commandNotify, + }, { name: "/exit", aliases: []string{"/quit"}, diff --git a/internal/tui/model.go b/internal/tui/model.go index a6cc1c6a3..11698b154 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -130,16 +130,21 @@ type model struct { keyBindings keyBindings themeMode themeMode // palette preference: auto (default), dark, light hasDarkBg bool // last terminal background-detection result (auto mode) - userAgent string - compactRequests int - compactInFlight bool - compactFrame int - lastCompactResult *CompactResult - lastCompactError string - unpricedRequests int - unpricedTokens int - lastUsage usage.Normalized - lastUsageSeen bool + // notifyMode and notifyFocusMode track the user's in-session notify + // preference (from options.Notify, updated by /notify). The notifier built + // at startup is independent, so changes apply on the NEXT permission prompt. + notifyMode string + notifyFocusMode string + userAgent string + compactRequests int + compactInFlight bool + compactFrame int + lastCompactResult *CompactResult + lastCompactError string + unpricedRequests int + unpricedTokens int + lastUsage usage.Normalized + lastUsageSeen bool // turnLatencySum / turnLatencyCount accumulate completed-run wall time so // /context can show a rolling average turn latency (the "is it slow?" signal). // Reset by /new. @@ -673,6 +678,20 @@ type tuiAgentRunOptions struct { specDraft bool } +// effectiveTUINotifyMode returns the notification mode the TUI should use. An +// empty/unconfigured mode falls back to the resolver default ("both": terminal +// bell + OSC-9 desktop notification) so the permission-prompt alert works for +// new users without requiring them to hand-edit config.json. The /notify +// command persists explicit choices; the resolver applies the same default at +// read time, so this function and the resolver always agree. +func effectiveTUINotifyMode(mode string) notify.Mode { + m := notify.Mode(strings.TrimSpace(mode)) + if m == "" { + return notify.ModeBoth + } + return m +} + func newModel(ctx context.Context, options Options) model { if ctx == nil { ctx = context.Background() @@ -736,7 +755,7 @@ func newModel(ctx context.Context, options Options) model { runSpinner := spinner.New(spinner.WithSpinner(spinner.MiniDot)) notifier := notify.New(os.Stderr, notify.Config{ - Mode: notify.Mode(strings.TrimSpace(options.Notify.Mode)), + Mode: effectiveTUINotifyMode(options.Notify.Mode), FocusMode: notify.FocusMode(strings.TrimSpace(options.Notify.FocusMode)), }) // Opt-in webhook fan-out (ZERO_NOTIFY_WEBHOOK_URL). Delivery failures stay @@ -788,6 +807,8 @@ func newModel(ctx context.Context, options Options) model { keyBindings: resolvedKeyBindings, themeMode: resolveThemeMode(options.Theme, os.Getenv("ZERO_THEME"), options.SavedTheme), hasDarkBg: true, + notifyMode: string(effectiveTUINotifyMode(options.Notify.Mode)), + notifyFocusMode: strings.TrimSpace(options.Notify.FocusMode), userAgent: options.UserAgent, usageTracker: usageTracker, transcript: initialTranscript(), @@ -3796,6 +3817,14 @@ func (m model) choosePicker() (tea.Model, tea.Cmd) { // text /theme dispatch (M17). return m, tea.RequestBackgroundColor } + case pickerNotify: + // The picker item's Value is " "; reusing the text handler + // keeps validation, persistence, and the user-facing message in one + // place. There is no live preview for notify, so no follow-up command + // is needed here. + text := "" + m, text = m.handleNotifyCommand(item.Value) + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) } return m, cmd } @@ -4120,6 +4149,18 @@ func (m model) handleSubmit() (tea.Model, tea.Cmd) { return m, tea.RequestBackgroundColor } return m, nil + case commandNotify: + // Bare `/notify` opens the popup picker so the user can pick mode + focus + // with arrow keys, matching /model and /theme. An explicit + // `/notify off|bell|notify|both [unfocused|always]` runs the text handler. + if strings.TrimSpace(command.text) == "" { + m.picker = m.newNotifyPicker() + return m, nil + } + text := "" + m, text = m.handleNotifyCommand(command.text) + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) + return m, nil case commandImage: m = m.handleImageCommand(command.text) return m, nil diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index 4c4efbfdb..f3f54c8c2 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -2619,6 +2619,29 @@ func TestModelNotifierFocusAndCompletion(t *testing.T) { } } +func TestEffectiveTUINotifyMode(t *testing.T) { + cases := []struct { + in string + want notify.Mode + }{ + // Empty input falls through to the resolver default ("both": bell + + // OSC-9 desktop notification) so the permission-prompt alert works + // for users who never configured notify. + {"", notify.ModeBoth}, + {" ", notify.ModeBoth}, + {"off", notify.ModeOff}, + {"bell", notify.ModeBell}, + {"notify", notify.ModeNotify}, + {"both", notify.ModeBoth}, + {" bell ", notify.ModeBell}, + } + for _, c := range cases { + if got := effectiveTUINotifyMode(c.in); got != c.want { + t.Errorf("effectiveTUINotifyMode(%q) = %q, want %q", c.in, got, c.want) + } + } +} + func TestScrimViewportLine(t *testing.T) { // Blank lines are left untouched (no scrim). if got := scrimViewportLine(" ", 10); got != " " { diff --git a/internal/tui/notify_select.go b/internal/tui/notify_select.go new file mode 100644 index 000000000..d3c403139 --- /dev/null +++ b/internal/tui/notify_select.go @@ -0,0 +1,158 @@ +package tui + +import ( + "strings" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/notify" +) + +// notifyChoice is one row in the /notify picker. The mode and focusMode pair is +// the on-disk shape; the label is what the user reads. +type notifyChoice struct { + label string + subtitle string + mode string + focusMode string +} + +// notifyChoices is the ordered list shown by the /notify picker. "Unfocused + +// both" (the resolver default) is first because it is the most useful option +// for users who do not already have a strong opinion. "Silent" is last so the +// recommended path is also the visually-defaulted one. Adding a new (mode, +// focus) pair is enough to extend the picker and the /notify state list. +var notifyChoices = []notifyChoice{ + { + label: "Notify when unfocused (recommended)", + subtitle: "Sound + desktop notification when the terminal is in the background.", + mode: string(notify.ModeBoth), + focusMode: string(notify.FocusUnfocused), + }, + { + label: "Always notify", + subtitle: "Sound + desktop notification every time Zero needs your input.", + mode: string(notify.ModeBoth), + focusMode: string(notify.FocusAlways), + }, + { + label: "Bell only", + subtitle: "Terminal bell (no desktop notification) every time Zero needs your input.", + mode: string(notify.ModeBell), + focusMode: string(notify.FocusAlways), + }, + { + label: "Silent", + subtitle: "Show prompts in the TUI only — no extra sound or notification.", + mode: string(notify.ModeOff), + focusMode: string(notify.FocusUnfocused), + }, +} + +// handleNotifyCommand implements /notify [list|off|bell|notify|both [focus]]. +// Bare `/notify` opens the picker at the dispatch layer; a mode-only argument +// keeps the existing focusMode. Mirrors handleThemeCommand. +func (m model) handleNotifyCommand(args string) (model, string) { + tokens := strings.Fields(strings.TrimSpace(args)) + if len(tokens) == 0 || tokens[0] == "list" { + return m, m.notifyStateText() + } + mode := strings.ToLower(strings.TrimSpace(tokens[0])) + if !isValidNotifyMode(mode) { + return m, "Notify\nUnknown mode: " + tokens[0] + " (expected off, bell, notify, or both; run /notify with no argument to pick from the list)" + } + focus := "" + if len(tokens) > 1 { + focus = strings.ToLower(strings.TrimSpace(tokens[1])) + if !isValidNotifyFocusMode(focus) { + return m, "Notify\nUnknown focus mode: " + tokens[1] + " (expected unfocused, always, or focused)" + } + } else { + focus = m.notifyCurrentFocusMode() + } + m.notifyMode = mode + m.notifyFocusMode = focus + lines := []string{ + "Notify", + "active mode: " + mode + ", focus: " + focus, + "Changes apply on the next permission prompt in this session.", + } + if note := m.persistNotifyPreference(mode, focus); note != "" { + lines = append(lines, note) + } + return m, strings.Join(lines, "\n") +} + +// persistNotifyPreference writes the choice to user config so it survives a +// restart. Best-effort: returns a short note to surface on failure, or "" on +// success / when there is no config path (e.g. tests). +func (m model) persistNotifyPreference(mode string, focus string) string { + if strings.TrimSpace(m.userConfigPath) == "" { + return "" + } + if _, err := config.SetNotify(m.userConfigPath, config.NotifyConfig{ + Mode: mode, + FocusMode: focus, + }); err != nil { + return "note: could not save notify preference (" + err.Error() + ")" + } + return "" +} + +// notifyStateText renders the /notify state view: current mode + focus + the +// picker rows, so the user has the same information whether they ran +// `/notify list` or just opened the picker. +func (m model) notifyStateText() string { + activeMode := m.notifyCurrentMode() + activeFocus := m.notifyCurrentFocusMode() + sections := []commandSection{{ + Title: "State", + Lines: []string{ + "active mode: " + activeMode, + "active focus: " + activeFocus, + }, + }} + rows := make([]string, 0, len(notifyChoices)) + for _, c := range notifyChoices { + rows = append(rows, c.label) + } + sections = append(sections, commandSection{ + Title: "Available", + Lines: rows, + }) + return renderCommandOutput(commandOutput{ + Title: "Notify", + Status: commandStatusOK, + Sections: sections, + Hints: []string{"run /notify with no argument to open the picker, or /notify [focus] to change directly"}, + }) +} + +// notifyCurrentMode and notifyCurrentFocusMode return the in-session notify +// preference. newModel populates both from options.Notify via +// effectiveTUINotifyMode (which never returns ""), so no empty fallback is +// needed here. +func (m model) notifyCurrentMode() string { + return m.notifyMode +} + +func (m model) notifyCurrentFocusMode() string { + return m.notifyFocusMode +} + +// isValidNotifyMode reports whether s names one of the four notification modes. +func isValidNotifyMode(s string) bool { + switch s { + case string(notify.ModeOff), string(notify.ModeBell), string(notify.ModeNotify), string(notify.ModeBoth): + return true + } + return false +} + +// isValidNotifyFocusMode reports whether s names one of the three focus modes. +func isValidNotifyFocusMode(s string) bool { + switch s { + case string(notify.FocusUnfocused), string(notify.FocusAlways), string(notify.FocusFocused): + return true + } + return false +} diff --git a/internal/tui/notify_select_test.go b/internal/tui/notify_select_test.go new file mode 100644 index 000000000..9a8e84113 --- /dev/null +++ b/internal/tui/notify_select_test.go @@ -0,0 +1,184 @@ +package tui + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/notify" +) + +// A committed /notify choice is written to user config and reloaded at startup +// (via the resolver's defaults + the notifyMode/notifyFocusMode fields on the +// model), so a /notify choice survives restart, just like /theme. +func TestNotifyChoicePersistsAcrossRestart(t *testing.T) { + cfgPath := filepath.Join(t.TempDir(), "config.json") + + // First session: pick a non-default notify pair via the text handler (same + // commit path the picker uses via choosePicker). + m := newModel(context.Background(), Options{UserConfigPath: cfgPath}) + m, out := m.handleNotifyCommand("off") + if m.notifyMode != "off" { + t.Fatalf("notifyMode = %q, want off", m.notifyMode) + } + if !strings.Contains(out, "Notify") { + t.Fatalf("output should announce the change, got: %s", out) + } + data, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("notify commit should have written config: %v", err) + } + var cfg struct { + Notify config.NotifyConfig `json:"notify"` + } + if err := json.Unmarshal(data, &cfg); err != nil { + t.Fatalf("config is not valid JSON: %v", err) + } + if cfg.Notify.Mode != "off" { + t.Fatalf("notify.mode = %q, want off", cfg.Notify.Mode) + } + + // Second session: the persisted notify block seeds the model fields so the + // /notify state line is correct and a permission prompt uses the right + // notifier (the runtime notifier is built from options.Notify, which is + // populated by the resolver from the same file). + restarted := newModel(context.Background(), Options{UserConfigPath: cfgPath, Notify: config.NotifyConfig{Mode: "off"}}) + if restarted.notifyMode != "off" { + t.Fatalf("restarted notifyMode = %q, want off (from saved config)", restarted.notifyMode) + } +} + +// `/notify` with a mode-only arg keeps the existing focusMode. A common mistake +// would be to reset the focus rule on every mode change. +func TestNotifyCommandPreservesFocusOnModeOnly(t *testing.T) { + m := newModel(context.Background(), Options{}) + m.notifyFocusMode = string(notify.FocusAlways) + m, _ = m.handleNotifyCommand("off") + if m.notifyMode != "off" { + t.Errorf("notifyMode = %q, want off", m.notifyMode) + } + if m.notifyFocusMode != string(notify.FocusAlways) { + t.Errorf("notifyFocusMode = %q, want preserved %q", m.notifyFocusMode, notify.FocusAlways) + } +} + +// `/notify bell unfocused` updates both fields in one call. +func TestNotifyCommandSetsModeAndFocus(t *testing.T) { + m := newModel(context.Background(), Options{}) + m, _ = m.handleNotifyCommand("bell unfocused") + if m.notifyMode != "bell" { + t.Errorf("notifyMode = %q, want bell", m.notifyMode) + } + if m.notifyFocusMode != "unfocused" { + t.Errorf("notifyFocusMode = %q, want unfocused", m.notifyFocusMode) + } +} + +// `/notify loud` (invalid) returns an error message; the model's notifyMode +// is NOT mutated, so a typo cannot accidentally turn the alert off. +func TestNotifyCommandRejectsInvalidMode(t *testing.T) { + m := newModel(context.Background(), Options{}) + m.notifyMode = "both" + m, out := m.handleNotifyCommand("loud") + if m.notifyMode != "both" { + t.Errorf("invalid mode should not mutate state, got %q", m.notifyMode) + } + if !strings.Contains(out, "Unknown mode") { + t.Errorf("output should explain the error, got: %s", out) + } +} + +// `/notify bell sideways` rejects the focus mode but the call also failed +// validation before persisting, so neither field should change. +func TestNotifyCommandRejectsInvalidFocus(t *testing.T) { + m := newModel(context.Background(), Options{}) + m.notifyMode = "bell" + m.notifyFocusMode = "always" + m, out := m.handleNotifyCommand("bell sideways") + if m.notifyMode != "bell" || m.notifyFocusMode != "always" { + t.Errorf("invalid focus should not mutate state, got mode=%q focus=%q", m.notifyMode, m.notifyFocusMode) + } + if !strings.Contains(out, "Unknown focus mode") { + t.Errorf("output should explain the error, got: %s", out) + } +} + +// `/notify` with no argument opens the picker, just like /theme and /model. +func TestNotifyPickerOpensOnBareNotify(t *testing.T) { + m := newModel(context.Background(), Options{Notify: config.NotifyConfig{Mode: "off", FocusMode: "unfocused"}}) + m.input.SetValue("/notify") + + updated, cmd := m.Update(testKey(tea.KeyEnter)) + m = updated.(model) + if cmd != nil { + t.Fatalf("opening the notify picker should not emit a cmd, got %T", cmd) + } + if m.picker == nil || m.picker.kind != pickerNotify { + t.Fatalf("expected the notify picker to open, got %#v", m.picker) + } + if len(m.picker.items) != len(notifyChoices) { + t.Fatalf("picker has %d items, want %d", len(m.picker.items), len(notifyChoices)) + } + // The preselected row should match the active (mode, focus) pair. + sel := m.picker.items[m.picker.selected] + if sel.Value != "off unfocused" { + t.Errorf("preselected value = %q, want the active pair %q", sel.Value, "off unfocused") + } +} + +// The picker's Value strings are the same " " form the text +// handler accepts, so the commit path can be shared. This is the contract that +// lets choosePicker dispatch to handleNotifyCommand without translation. +func TestNotifyPickerValuesAreValidCommandArgs(t *testing.T) { + m := newModel(context.Background(), Options{}) + picker := m.newNotifyPicker() + for _, item := range picker.items { + tokens := strings.Fields(item.Value) + if len(tokens) != 2 { + t.Errorf("item %q has %d tokens, want 2 (mode focus)", item.Value, len(tokens)) + continue + } + if !isValidNotifyMode(tokens[0]) { + t.Errorf("item %q: mode %q is not a valid notify mode", item.Value, tokens[0]) + } + if !isValidNotifyFocusMode(tokens[1]) { + t.Errorf("item %q: focus %q is not a valid focus mode", item.Value, tokens[1]) + } + } +} + +// The /notify state view shows the current mode and focus so users can see +// the value before opening the picker. +func TestNotifyStateTextShowsActivePair(t *testing.T) { + m := newModel(context.Background(), Options{}) + m.notifyMode = "both" + m.notifyFocusMode = "unfocused" + state := m.notifyStateText() + if !strings.Contains(state, "active mode: both") { + t.Errorf("state should show active mode, got: %s", state) + } + if !strings.Contains(state, "active focus: unfocused") { + t.Errorf("state should show active focus, got: %s", state) + } +} + +// notifyCurrentMode / notifyCurrentFocusMode surface the in-session fields +// that newModel populates from options.Notify, so /notify reads the same +// value the runtime notifier uses. +func TestNotifyCurrentReflectsModelFields(t *testing.T) { + m := newModel(context.Background(), Options{}) + m.notifyMode = "bell" + m.notifyFocusMode = "always" + if got := m.notifyCurrentMode(); got != "bell" { + t.Errorf("notifyCurrentMode = %q, want bell", got) + } + if got := m.notifyCurrentFocusMode(); got != "always" { + t.Errorf("notifyCurrentFocusMode = %q, want always", got) + } +} diff --git a/internal/tui/picker.go b/internal/tui/picker.go index 39384b2a2..d1381874a 100644 --- a/internal/tui/picker.go +++ b/internal/tui/picker.go @@ -25,6 +25,7 @@ const ( pickerSession pickerTheme pickerSkill + pickerNotify ) // pickerItem is one selectable row: Label is shown, Value is passed to the @@ -907,6 +908,32 @@ func (m model) newThemePicker() *commandPicker { return &commandPicker{kind: pickerTheme, title: "select theme", items: items, allItems: append([]pickerItem{}, items...), selected: selected} } +// newNotifyPicker lists the four (mode, focus) pairs from notifyChoices. Each +// row's Value is the same synthetic string the text /notify handler accepts +// (" "), so /notify with no arg and the picker share one commit +// path through handleNotifyCommand. The currently active pair is preselected so +// the user can press Enter to keep it. There is no live preview — notify +// affects the next permission prompt, not the current view — so the picker +// does not call a preview function on move. +func (m model) newNotifyPicker() *commandPicker { + items := make([]pickerItem, 0, len(notifyChoices)) + selected := 0 + activeMode := m.notifyCurrentMode() + activeFocus := m.notifyCurrentFocusMode() + for _, c := range notifyChoices { + items = append(items, pickerItem{ + Group: "When Zero needs your input", + Label: c.label, + Value: c.mode + " " + c.focusMode, + Meta: c.subtitle, + }) + if c.mode == activeMode && c.focusMode == activeFocus { + selected = len(items) - 1 + } + } + return &commandPicker{kind: pickerNotify, title: "select notify mode", items: items, allItems: append([]pickerItem{}, items...), selected: selected} +} + // pickerMoved advances the open picker's cursor by delta and live-previews the new // selection where the picker supports it — stepping through the /theme popup // repaints the UI in the hovered palette. Safe to call with no picker open. Callers From 334c688e2d5d1e31809946c8f45121a1a8cec5f5 Mon Sep 17 00:00:00 2001 From: Gaurav Bhatia Date: Tue, 1 Sep 2026 17:46:21 +0400 Subject: [PATCH 2/8] fix(notify): apply review feedback from CodeRabbit - cli: omitted --mode/--focus flags now preserve the current resolved value instead of wiping it (--reset remains the only clearing path); aligns the CLI with the TUI's mode-only preservation behavior - tui: reject /notify inputs with more than two tokens instead of silently accepting them - tui: apply /notify choices to the live notifier via the new notify.Notifier.Configure, so the change takes effect on the next permission prompt in the same session (the previous message claimed this but only the persisted value was updated) - notify: add Notifier.Configure (mutex-guarded policy swap that preserves sinks, focus state, and the writer) - tests: mode-only/focus-only CLI preservation, live-notifier apply, trailing-argument rejection, Configure immediate-effect + sink retention --- internal/cli/config_notify.go | 15 +++++++++- internal/cli/config_notify_test.go | 45 +++++++++++++++++++++++++++++- internal/notify/notify.go | 12 +++++++- internal/notify/notify_test.go | 38 +++++++++++++++++++++++++ internal/tui/notify_select.go | 12 +++++++- internal/tui/notify_select_test.go | 38 +++++++++++++++++++++++++ 6 files changed, 156 insertions(+), 4 deletions(-) diff --git a/internal/cli/config_notify.go b/internal/cli/config_notify.go index 3715870f9..ffa15de7f 100644 --- a/internal/cli/config_notify.go +++ b/internal/cli/config_notify.go @@ -34,9 +34,22 @@ func runConfigNotify(args []string, stdout io.Writer, stderr io.Writer, deps app if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } - notify := config.NotifyConfig{Mode: options.mode, FocusMode: options.focus} + // Omitted flags preserve the current value — a full replace would let + // `--mode bell` silently wipe a configured focusMode. --reset is the + // only path that clears both fields. + notify := config.NotifyConfig{ + Mode: resolved.Notify.Mode, + FocusMode: resolved.Notify.FocusMode, + } if options.reset { notify = config.NotifyConfig{} + } else { + if options.mode != "" { + notify.Mode = options.mode + } + if options.focus != "" { + notify.FocusMode = options.focus + } } if _, err := config.SetNotify(configPath, notify); err != nil { return writeAppError(stderr, err.Error(), exitUsage) diff --git a/internal/cli/config_notify_test.go b/internal/cli/config_notify_test.go index c0bd9aa7f..e36ccd05c 100644 --- a/internal/cli/config_notify_test.go +++ b/internal/cli/config_notify_test.go @@ -110,7 +110,8 @@ func TestRunConfigNotifyWritesModeChange(t *testing.T) { "baseUrl": "https://api.openai.com/v1", "model": "gpt-4.1", "apiKeyEnv": "OPENAI_API_KEY" - }] + }], + "notify": {"mode": "both", "focusMode": "always"} }` if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { t.Fatalf("seed config: %v", err) @@ -131,11 +132,53 @@ func TestRunConfigNotifyWritesModeChange(t *testing.T) { if cfg.Notify.Mode != "off" { t.Errorf("Notify.Mode = %q, want off", cfg.Notify.Mode) } + // A mode-only update must preserve the configured focusMode, not wipe it. + if cfg.Notify.FocusMode != "always" { + t.Errorf("Notify.FocusMode = %q, want preserved %q", cfg.Notify.FocusMode, "always") + } if !strings.Contains(stdout.String(), "mode: off") { t.Errorf("stdout should confirm the change, got: %s", stdout.String()) } } +// A focus-only update preserves the configured mode. +func TestRunConfigNotifyFocusOnlyPreservesMode(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + seed := `{ + "activeProvider": "openai", + "providers": [{ + "name": "openai", + "providerKind": "openai", + "baseUrl": "https://api.openai.com/v1", + "model": "gpt-4.1", + "apiKeyEnv": "OPENAI_API_KEY" + }], + "notify": {"mode": "bell", "focusMode": "unfocused"} + }` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--focus", "always"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if cfg.Notify.Mode != "bell" { + t.Errorf("Notify.Mode = %q, want preserved %q", cfg.Notify.Mode, "bell") + } + if cfg.Notify.FocusMode != "always" { + t.Errorf("Notify.FocusMode = %q, want always", cfg.Notify.FocusMode) + } +} + // `--mode` and `--focus` together update both fields in one call. func TestRunConfigNotifyWritesModeAndFocus(t *testing.T) { configPath := filepath.Join(t.TempDir(), "config.json") diff --git a/internal/notify/notify.go b/internal/notify/notify.go index 79906d5ac..1376ac426 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -57,7 +57,7 @@ type Sink interface { // attached Sinks. Safe for concurrent use. type Notifier struct { w io.Writer - cfg Config // immutable after New; reads outside the lock are safe + cfg Config // swap at runtime via Configure; reads outside the lock are safe mu sync.Mutex focused bool @@ -71,6 +71,16 @@ func New(w io.Writer, cfg Config) *Notifier { return &Notifier{w: w, cfg: cfg} } +// Configure swaps the mode/focus policy at runtime. Sinks, focus state, and the +// writer are preserved, so an in-session preference change (e.g. the TUI's +// /notify command) applies from the next Notify call. Safe to call concurrently +// with Notify. +func (n *Notifier) Configure(cfg Config) { + n.mu.Lock() + n.cfg = cfg + n.mu.Unlock() +} + // AddSink registers an additional destination that receives every eligible // event (subject to the same mode/focus policy as the terminal). Sinks fire // even when the Notifier has no terminal writer, so a headless CI run can still diff --git a/internal/notify/notify_test.go b/internal/notify/notify_test.go index 54ba06aef..a565345b8 100644 --- a/internal/notify/notify_test.go +++ b/internal/notify/notify_test.go @@ -50,6 +50,44 @@ func TestSequence(t *testing.T) { } } +// Configure swaps the policy at runtime: after turning a silent notifier on, +// the next Notify emits; after turning a noisy one off, it stays silent. Sinks +// survive the swap (the TUI relies on this when /notify reconfigures mid-run). +func TestConfigureAppliesImmediatelyAndKeepsSinks(t *testing.T) { + var buf bytes.Buffer + n := New(&buf, Config{Mode: ModeOff}) + n.SetFocused(true) + + n.Notify(Completion, "x") + if buf.Len() != 0 { + t.Fatalf("off should be silent, got %q", buf.String()) + } + + sink := &recordingSink{} + n.AddSink(sink) + n.Configure(Config{Mode: ModeBell, FocusMode: FocusAlways}) + n.Notify(Completion, "x") + if buf.String() != "\x07" { + t.Fatalf("after Configure(bell) should bell, got %q", buf.String()) + } + sink.mu.Lock() + got := len(sink.events) + sink.mu.Unlock() + if got != 1 { + t.Fatalf("sink should still receive events after Configure, got %d", got) + } + + n.Configure(Config{Mode: ModeOff}) + buf.Reset() + n.Notify(Completion, "x") + sink.mu.Lock() + got = len(sink.events) + sink.mu.Unlock() + if buf.Len() != 0 || got != 1 { + t.Fatalf("after Configure(off) should be fully silent, buf=%q events=%d", buf.String(), got) + } +} + func TestSanitizeMessage(t *testing.T) { if got := sanitizeMessage("ok\x1b]0;evil\x07more\nx"); got != "ok]0;evilmorex" { t.Fatalf("sanitize=%q", got) diff --git a/internal/tui/notify_select.go b/internal/tui/notify_select.go index d3c403139..6aa7e2d84 100644 --- a/internal/tui/notify_select.go +++ b/internal/tui/notify_select.go @@ -56,6 +56,9 @@ func (m model) handleNotifyCommand(args string) (model, string) { if len(tokens) == 0 || tokens[0] == "list" { return m, m.notifyStateText() } + if len(tokens) > 2 { + return m, "Notify\nToo many arguments: " + args + " (usage: /notify [unfocused|always|focused])" + } mode := strings.ToLower(strings.TrimSpace(tokens[0])) if !isValidNotifyMode(mode) { return m, "Notify\nUnknown mode: " + tokens[0] + " (expected off, bell, notify, or both; run /notify with no argument to pick from the list)" @@ -71,10 +74,17 @@ func (m model) handleNotifyCommand(args string) (model, string) { } m.notifyMode = mode m.notifyFocusMode = focus + // Apply to the live notifier so the change takes effect on the next + // permission prompt in this session, not just after a restart. + if m.notifier != nil { + m.notifier.Configure(notify.Config{ + Mode: notify.Mode(mode), + FocusMode: notify.FocusMode(focus), + }) + } lines := []string{ "Notify", "active mode: " + mode + ", focus: " + focus, - "Changes apply on the next permission prompt in this session.", } if note := m.persistNotifyPreference(mode, focus); note != "" { lines = append(lines, note) diff --git a/internal/tui/notify_select_test.go b/internal/tui/notify_select_test.go index 9a8e84113..bbcedc564 100644 --- a/internal/tui/notify_select_test.go +++ b/internal/tui/notify_select_test.go @@ -1,6 +1,7 @@ package tui import ( + "bytes" "context" "encoding/json" "os" @@ -80,6 +81,43 @@ func TestNotifyCommandSetsModeAndFocus(t *testing.T) { } } +// The choice reaches the LIVE notifier immediately, so the change applies on +// the next permission prompt in this session (not only after a restart). +func TestNotifyCommandAppliesToLiveNotifier(t *testing.T) { + var buf bytes.Buffer + // Construct through newModel so both fields are populated the way the real + // session does; then swap in a buffer-backed notifier to observe output. + m := newModel(context.Background(), Options{Notify: config.NotifyConfig{Mode: "off", FocusMode: "always"}}) + m.notifier = notify.New(&buf, notify.Config{Mode: notify.ModeOff, FocusMode: notify.FocusAlways}) + m.notifier.SetFocused(true) + + m, _ = m.handleNotifyCommand("bell") + m.notifier.Notify(notify.Completion, "x") + if buf.String() != "\x07" { + t.Fatalf("live notifier should bell after /notify bell, got %q", buf.String()) + } + + m, _ = m.handleNotifyCommand("off") + m.notifier.Notify(notify.Completion, "x") + if buf.String() != "\x07" { + t.Fatalf("live notifier should go silent after /notify off, got %q", buf.String()) + } +} + +// `/notify off always typo` (more than two tokens) is rejected, not silently +// accepted as a successful change. +func TestNotifyCommandRejectsTrailingArguments(t *testing.T) { + m := newModel(context.Background(), Options{}) + m.notifyMode = "both" + m, out := m.handleNotifyCommand("off always typo") + if m.notifyMode != "both" { + t.Errorf("trailing args should not mutate state, got %q", m.notifyMode) + } + if !strings.Contains(out, "Too many arguments") { + t.Errorf("output should explain the rejection, got: %s", out) + } +} + // `/notify loud` (invalid) returns an error message; the model's notifyMode // is NOT mutated, so a typo cannot accidentally turn the alert off. func TestNotifyCommandRejectsInvalidMode(t *testing.T) { From 15929bf4581d3bb611f1bf02bee9958abdcc6541 Mon Sep 17 00:00:00 2001 From: Gaurav Bhatia Date: Tue, 1 Sep 2026 17:56:14 +0400 Subject: [PATCH 3/8] fix(notify): read cfg under the lock in Notify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Configure (334c688) made cfg mutable at runtime, but Notify still read n.cfg.Mode before acquiring n.mu — a data race with a concurrent Configure. Move the mode check inside the critical section and copy cfg to a local for all reads. Regression test TestConfigureConcurrentWithNotify runs Configure concurrently with Notify; verified it reports DATA RACE on the unfixed code and passes after the fix (go test -race -count=5). --- internal/notify/notify.go | 13 ++++++++----- internal/notify/notify_test.go | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/internal/notify/notify.go b/internal/notify/notify.go index 1376ac426..3c9d39ed6 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -112,18 +112,21 @@ func (n *Notifier) SetFocused(focused bool) { // Sinks are invoked outside the lock so a slow/blocking sink cannot stall a // concurrent Notify or SetFocused. func (n *Notifier) Notify(event Event, message string) { - if n.cfg.Mode == ModeOff || n.cfg.Mode == "" { + n.mu.Lock() + // cfg is mutable at runtime (Configure), so every read happens under the + // lock; shouldEmit/sequence work on the local copy. + cfg := n.cfg + if cfg.Mode == ModeOff || cfg.Mode == "" { + n.mu.Unlock() return } - - n.mu.Lock() - eligible := shouldEmit(n.cfg, event, n.focused) + eligible := shouldEmit(cfg, event, n.focused) var sinks []Sink if eligible && len(n.sinks) > 0 { sinks = append(sinks, n.sinks...) } if eligible && n.w != nil { - if seq := sequence(n.cfg.Mode, message); seq != "" { + if seq := sequence(cfg.Mode, message); seq != "" { _, _ = io.WriteString(n.w, seq) } } diff --git a/internal/notify/notify_test.go b/internal/notify/notify_test.go index a565345b8..2a81720c2 100644 --- a/internal/notify/notify_test.go +++ b/internal/notify/notify_test.go @@ -147,6 +147,21 @@ func TestNotifyRaceSafe(t *testing.T) { wg.Wait() } +// Configure mutates cfg under the lock while Notify reads it; this pair must +// be race-clean (run under -race). Regression for the unsynchronized cfg read +// Notify used to perform before acquiring the lock. +func TestConfigureConcurrentWithNotify(t *testing.T) { + n := New(&bytes.Buffer{}, Config{Mode: ModeBell, FocusMode: FocusAlways}) + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(3) + go func() { defer wg.Done(); n.Configure(Config{Mode: ModeBoth, FocusMode: FocusAlways}) }() + go func() { defer wg.Done(); n.Configure(Config{Mode: ModeOff}) }() + go func() { defer wg.Done(); n.Notify(AwaitingInput, "x") }() + } + wg.Wait() +} + func TestDefaultMessage(t *testing.T) { if DefaultMessage(Completion) != "Zero: ready" { t.Fatal("completion message") From 18e851af66d6b70d03b2367bbec45424ba6a6f79 Mon Sep 17 00:00:00 2001 From: Gaurav Bhatia Date: Thu, 3 Sep 2026 13:53:06 +0400 Subject: [PATCH 4/8] fix(notify): keep defaults out of the resolver; preserve the user's own values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the maintainer review (Vasanthdev2004) on PR #1001. All three findings share one root cause: the resolved config was treated as if it were the user's choice. - resolver: no longer defaults notify.mode/focusMode. The TUI's effectiveTUINotifyMode already maps empty -> both on its own, so the permission-prompt alert still works out of the box, while headless `zero exec` stays byte-identical to base (no BEL/OSC-9 on stderr under -o json), the exec empty-stderr fixture is restored, and the ZERO_NOTIFY_WEBHOOK_URL sink is not armed by an implicit default. - cli: `zero config notify` seeds omitted fields from the user's own file (new config.UserNotify), never from the resolved view — a project config's mode:off can no longer be copied into the user's global config, and blank stays blank instead of pinning today's default as an explicit choice. --reset remains the only clearing path. - tui: /notify mode-only changes preserve the focus stored in the user's own file (blank stays blank); the /notify picker enumerates the full 4x3 mode x focus space so every valid pair is a row, the current pair is always preselected, and Enter can never commit a setting the user did not choose. State view reads the stored pair. - tests: the three regressions from the review — exec writes nothing to stderr on a clean run (fixture restored + resolver empty-default test), a ProjectConfigPath test proving project notify cannot leak into the user file, and Enter on an open picker from a pair outside the old curated list (off, always) keeps the setting unchanged. --- internal/cli/config_notify.go | 24 ++-- internal/cli/config_notify_test.go | 153 ++++++++++++++++++++--- internal/cli/exec_test.go | 9 +- internal/config/resolver.go | 24 ---- internal/config/resolver_test.go | 86 +++++-------- internal/config/writer.go | 33 ++++- internal/config/writer_test.go | 37 +++++- internal/tui/notify_select.go | 141 ++++++++++++--------- internal/tui/notify_select_test.go | 193 ++++++++++++++++++++--------- internal/tui/picker.go | 28 +++-- 10 files changed, 484 insertions(+), 244 deletions(-) diff --git a/internal/cli/config_notify.go b/internal/cli/config_notify.go index ffa15de7f..9c8216971 100644 --- a/internal/cli/config_notify.go +++ b/internal/cli/config_notify.go @@ -34,13 +34,18 @@ func runConfigNotify(args []string, stdout io.Writer, stderr io.Writer, deps app if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } - // Omitted flags preserve the current value — a full replace would let - // `--mode bell` silently wipe a configured focusMode. --reset is the - // only path that clears both fields. - notify := config.NotifyConfig{ - Mode: resolved.Notify.Mode, - FocusMode: resolved.Notify.FocusMode, + // Seed omitted fields from the USER'S OWN file, never from the + // resolved view: resolved merges project config (so a repo's + // mode:off would be copied into the user's global settings) and + // carries no defaults here, but seeding from it would also pin + // defaults as explicit choices. Blank stays blank — blank means + // "use the built-in defaults". --reset is the only path that + // clears both fields. + current, err := config.UserNotify(configPath) + if err != nil { + return writeAppError(stderr, err.Error(), exitUsage) } + notify := current if options.reset { notify = config.NotifyConfig{} } else { @@ -141,8 +146,11 @@ func writeConfigNotifyHelp(w io.Writer) error { "\n"+ "Print or update the permission-prompt notify preference.\n"+ "\n"+ - "When run with no flag, prints the current mode and focusMode (the resolver\n"+ - "defaults to \"both\" and \"unfocused\" when the config block is empty).\n"+ + "When run with no flag, prints the current mode and focusMode; a field you\n"+ + "never set shows as (default) — the TUI alerts with bell + notification,\n"+ + "firing only when the terminal is unfocused. Omitted flags preserve the\n"+ + "values stored in YOUR config file; --reset clears both so the defaults\n"+ + "apply again.\n"+ "\n"+ "Examples:\n"+ " zero config notify\n"+ diff --git a/internal/cli/config_notify_test.go b/internal/cli/config_notify_test.go index e36ccd05c..eec1ec6d8 100644 --- a/internal/cli/config_notify_test.go +++ b/internal/cli/config_notify_test.go @@ -11,13 +11,11 @@ import ( "github.com/Gitlawb/zero/internal/config" ) -// `zero config notify` with no flag and a fresh config: the resolver applies -// the built-in defaults (mode=both, focusMode=unfocused) and the command -// reports them. This is the "just works" case a new user lands in. We use a -// real on-disk config (not the synthetic commandCenterDeps fixture, which -// returns an empty Notify field) because the resolver-default behavior is the -// whole point of this test. -func TestRunConfigNotifyPrintsResolverDefaults(t *testing.T) { +// `zero config notify` with no flag and a fresh config: nothing is configured, +// the resolver leaves the fields blank (defaults deliberately live in the TUI, +// not the resolver — maintainer review, PR #1001), and the command reports +// "(default)" for both. +func TestRunConfigNotifyPrintsUnconfiguredAsDefault(t *testing.T) { configPath := filepath.Join(t.TempDir(), "config.json") // A valid openai profile so the resolver does not error with // ErrNoActiveProvider. The notify defaults are applied independently of @@ -47,19 +45,60 @@ func TestRunConfigNotifyPrintsResolverDefaults(t *testing.T) { if exitCode != exitSuccess { t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) } - if !strings.Contains(stdout.String(), "mode: both") { - t.Errorf("stdout should show the default mode, got: %s", stdout.String()) + if !strings.Contains(stdout.String(), "mode: (default)") { + t.Errorf("stdout should show unconfigured mode as (default), got: %s", stdout.String()) } - if !strings.Contains(stdout.String(), "focusMode: unfocused") { - t.Errorf("stdout should show the default focus, got: %s", stdout.String()) + if !strings.Contains(stdout.String(), "focusMode: (default)") { + t.Errorf("stdout should show unconfigured focus as (default), got: %s", stdout.String()) } } -// `zero config notify --json` emits a machine-readable payload so scripts can -// read the resolved preference without parsing prose. Same real-resolver -// fixture as the print-defaults test, since the JSON path reads the same -// `resolved.Notify` that the print path does. +// `zero config notify --json` emits a machine-readable payload. A configured +// pair round-trips; unconfigured fields are empty strings (never defaults +// filled in), so scripts can distinguish "user chose" from "default". func TestRunConfigNotifyPrintsJSON(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + seed := `{ + "activeProvider": "openai", + "providers": [{ + "name": "openai", + "providerKind": "openai", + "baseUrl": "https://api.openai.com/v1", + "model": "gpt-4.1", + "apiKeyEnv": "OPENAI_API_KEY" + }], + "notify": {"mode": "bell", "focusMode": "always"} + }` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--json"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + var payload map[string]any + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, stdout.String()) + } + if payload["mode"] != "bell" { + t.Errorf("mode = %v, want bell", payload["mode"]) + } + if payload["focusMode"] != "always" { + t.Errorf("focusMode = %v, want always", payload["focusMode"]) + } +} + +// The unconfigured JSON shape: fields are empty strings, never defaults +// filled in, so scripts can tell "user chose" from "default". +func TestRunConfigNotifyPrintsJSONUnconfigured(t *testing.T) { configPath := filepath.Join(t.TempDir(), "config.json") seed := `{ "activeProvider": "openai", @@ -90,11 +129,11 @@ func TestRunConfigNotifyPrintsJSON(t *testing.T) { if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { t.Fatalf("output is not valid JSON: %v\n%s", err, stdout.String()) } - if payload["mode"] != "both" { - t.Errorf("mode = %v, want both", payload["mode"]) + if payload["mode"] != "" { + t.Errorf("mode = %v, want empty (unconfigured)", payload["mode"]) } - if payload["focusMode"] != "unfocused" { - t.Errorf("focusMode = %v, want unfocused", payload["focusMode"]) + if payload["focusMode"] != "" { + t.Errorf("focusMode = %v, want empty (unconfigured)", payload["focusMode"]) } } @@ -179,6 +218,82 @@ func TestRunConfigNotifyFocusOnlyPreservesMode(t *testing.T) { } } +// Maintainer regression (PR #1001): a partial update must seed from the USER'S +// OWN file, never from the resolved view. A project .zero/config.json setting +// mode=off resolves into the session, but `--focus always` inside that repo +// must NOT copy the project's off into the user's global config. +func TestRunConfigNotifyDoesNotCopyProjectNotifyIntoUserConfig(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "user.json") + projectPath := filepath.Join(t.TempDir(), "project.json") + if err := os.WriteFile(configPath, []byte(`{"activeProvider": "openai"}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(projectPath, []byte(`{"notify": {"mode": "off"}}`), 0o600); err != nil { + t.Fatal(err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + // Resolve EXACTLY the way production does: user config + project config + // merged, so resolved.Notify.Mode is the project's "off". + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{ + UserConfigPath: configPath, + ProjectConfigPath: projectPath, + Env: map[string]string{"OPENAI_API_KEY": "sk-test"}, + }) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--focus", "always"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + // The user's file must keep an unspecified mode unspecified — the + // project's "off" stays in the project file where it belongs. + if cfg.Notify.Mode != "" { + t.Errorf("user Notify.Mode = %q, want blank (project's off must not leak into the user file)", cfg.Notify.Mode) + } + if cfg.Notify.FocusMode != "always" { + t.Errorf("Notify.FocusMode = %q, want always", cfg.Notify.FocusMode) + } + // The project file is untouched. + project := readFileConfig(t, projectPath) + if project.Notify.Mode != "off" { + t.Errorf("project Notify.Mode = %q, want untouched off", project.Notify.Mode) + } +} + +// Maintainer regression (PR #1001): with a clean config, `--mode off` must not +// also pin focusMode as an explicit choice — blank means "use the built-in +// default", and a partial update keeps it that way. +func TestRunConfigNotifyDoesNotPinDefaultsAsExplicitChoices(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"activeProvider": "openai"}`), 0o600); err != nil { + t.Fatal(err) + } + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{"OPENAI_API_KEY": "sk-test"}}) + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify", "--mode", "off"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if cfg.Notify.Mode != "off" { + t.Errorf("Notify.Mode = %q, want off", cfg.Notify.Mode) + } + if cfg.Notify.FocusMode != "" { + t.Errorf("Notify.FocusMode = %q, want blank (unspecified stays unspecified; the default must not be pinned)", cfg.Notify.FocusMode) + } +} + // `--mode` and `--focus` together update both fields in one call. func TestRunConfigNotifyWritesModeAndFocus(t *testing.T) { configPath := filepath.Join(t.TempDir(), "config.json") diff --git a/internal/cli/exec_test.go b/internal/cli/exec_test.go index 63bf0d709..cd00c1ebf 100644 --- a/internal/cli/exec_test.go +++ b/internal/cli/exec_test.go @@ -970,11 +970,10 @@ func TestRunExecUsesProjectConfigAndOpenAICompatibleProvider(t *testing.T) { "name": "local", "provider_kind": "openai-compatible", "base_url": "` + server.URL + `", - "api_key": "sk-local", - "model": "local-model" - }], - "notify": {"mode": "off"} - }` + "api_key": "sk-local", + "model": "local-model" + }] +}` if err := os.WriteFile(filepath.Join(configDir, "config.json"), []byte(writeConfig), 0o600); err != nil { t.Fatal(err) } diff --git a/internal/config/resolver.go b/internal/config/resolver.go index d34939e12..16936874d 100644 --- a/internal/config/resolver.go +++ b/internal/config/resolver.go @@ -64,18 +64,6 @@ const MaxTurnsCeiling = 500 // (set 0 to always advertise every schema, e.g. for a model without tool_search). const defaultDeferThreshold = 3 -// defaultNotifyMode and defaultNotifyFocus are the fallback values used when -// config.json is missing, has no notify block, or has an empty notify block. -// both = terminal bell + OSC-9 desktop notification; unfocused = fire only -// when the TUI window is not the active window so users looking at the prompt -// are not spammed. The defaults make the permission-prompt alert "just work" -// for new users; the TUI /notify command and `zero config notify` let users -// change or opt out. -const ( - defaultNotifyMode = "both" - defaultNotifyFocus = "unfocused" -) - func Resolve(options ResolveOptions) (ResolvedConfig, error) { cfg := FileConfig{ MaxTurns: defaultMaxTurns, @@ -121,18 +109,6 @@ func Resolve(options ResolveOptions) (ResolvedConfig, error) { applyOverrides(&cfg, options.Overrides) - // Notify defaults: when the user has not configured notify (no block, or - // an empty block), apply the built-in defaults so the permission-prompt - // alert works out of the box. A user who explicitly sets notify.mode=off - // or notify.focusMode=focused still wins because their value is - // non-empty after the trim in the validation step below. - if strings.TrimSpace(cfg.Notify.Mode) == "" { - cfg.Notify.Mode = defaultNotifyMode - } - if strings.TrimSpace(cfg.Notify.FocusMode) == "" { - cfg.Notify.FocusMode = defaultNotifyFocus - } - if !cfg.Tools.deferThresholdSet && cfg.Tools.DeferThreshold == 0 { cfg.Tools.DeferThreshold = defaultDeferThreshold } diff --git a/internal/config/resolver_test.go b/internal/config/resolver_test.go index dd8ff0a99..6de99934b 100644 --- a/internal/config/resolver_test.go +++ b/internal/config/resolver_test.go @@ -1864,65 +1864,43 @@ func TestResolveNotifyInvalidFocusMode(t *testing.T) { } } -func TestResolveNotifyDefaultEmpty(t *testing.T) { - path := writeConfig(t, `{}`) - resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) - if err != nil { - t.Fatalf("Resolve: %v", err) - } - // Missing notify block falls back to the built-in defaults so the - // permission-prompt alert works for users who never ran setup. - if resolved.Notify.Mode != "both" { - t.Errorf("unset notify.mode should default to %q, got %q", "both", resolved.Notify.Mode) - } - if resolved.Notify.FocusMode != "unfocused" { - t.Errorf("unset notify.focusMode should default to %q, got %q", "unfocused", resolved.Notify.FocusMode) - } -} - -func TestResolveNotifyDefaultEmptyBlock(t *testing.T) { - // An explicit empty notify block should behave the same as a missing one: - // fall back to the built-in defaults. - path := writeConfig(t, `{"notify":{}}`) - resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) - if err != nil { - t.Fatalf("Resolve: %v", err) - } - if resolved.Notify.Mode != "both" { - t.Errorf("empty notify.mode should default to %q, got %q", "both", resolved.Notify.Mode) - } - if resolved.Notify.FocusMode != "unfocused" { - t.Errorf("empty notify.focusMode should default to %q, got %q", "unfocused", resolved.Notify.FocusMode) - } -} - -func TestResolveNotifyDefaultPartialEmpty(t *testing.T) { - // Only one field is set; the other should still get the default. - path := writeConfig(t, `{"notify":{"mode":"off"}}`) - resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) +// An unconfigured notify block must resolve EMPTY. Defaults live in the TUI +// (effectiveTUINotifyMode) and in `zero config notify`'s display, not in the +// resolver: a filled-in default here leaks into headless `zero exec` (BEL + +// OSC-9 bytes on stderr under -o json) and into the CLI/TUI preserve paths, +// which must treat "user never chose" differently from "user chose both" +// (maintainer review, PR #1001). +func TestResolveNotifyUnconfiguredStaysEmpty(t *testing.T) { + // No config file at all. + resolved, err := Resolve(ResolveOptions{Env: map[string]string{}}) if err != nil { t.Fatalf("Resolve: %v", err) } - if resolved.Notify.Mode != "off" { - t.Errorf("notify.mode should be preserved as %q, got %q", "off", resolved.Notify.Mode) + if resolved.Notify.Mode != "" || resolved.Notify.FocusMode != "" { + t.Fatalf("no config file: notify = %+v, want empty (resolver must not default)", resolved.Notify) } - if resolved.Notify.FocusMode != "unfocused" { - t.Errorf("empty notify.focusMode should default to %q, got %q", "unfocused", resolved.Notify.FocusMode) - } -} -func TestResolveNotifyDefaultNoConfigFile(t *testing.T) { - // No config file at all: defaults should still apply so the - // permission-prompt alert is on for first-run users. - resolved, err := Resolve(ResolveOptions{UserConfigPath: "", Env: map[string]string{}}) - if err != nil { - t.Fatalf("Resolve: %v", err) - } - if resolved.Notify.Mode != "both" { - t.Errorf("missing config: notify.mode should default to %q, got %q", "both", resolved.Notify.Mode) - } - if resolved.Notify.FocusMode != "unfocused" { - t.Errorf("missing config: notify.focusMode should default to %q, got %q", "unfocused", resolved.Notify.FocusMode) + for name, body := range map[string]string{ + "empty config": `{}`, + "empty block": `{"notify":{}}`, + "mode only": `{"notify":{"mode":"off"}}`, + } { + t.Run(name, func(t *testing.T) { + resolved, err := Resolve(ResolveOptions{UserConfigPath: writeConfig(t, body), Env: map[string]string{}}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if name == "mode only" { + if resolved.Notify.Mode != "off" { + t.Errorf("notify.mode = %q, want preserved off", resolved.Notify.Mode) + } + } else if resolved.Notify.Mode != "" { + t.Errorf("notify.mode = %q, want empty (resolver must not default)", resolved.Notify.Mode) + } + if resolved.Notify.FocusMode != "" { + t.Errorf("notify.focusMode = %q, want empty (resolver must not default)", resolved.Notify.FocusMode) + } + }) } } diff --git a/internal/config/writer.go b/internal/config/writer.go index 33f3f42cc..20724e839 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -592,12 +592,41 @@ func SetTheme(path string, theme string) (FileConfig, error) { return cfg, nil } +// UserNotify returns the notify block stored in the user's own config file at +// path, trimmed. A missing file or missing/empty block returns the zero value: +// callers that need to preserve "whatever the user already chose" on a partial +// update must seed from THIS value, not from the resolved view — the resolver +// merges project config and the TUI applies its own defaults, so seeding from +// resolved copies choices the user never made into their global file (a +// project's mode: off, or a pinned default) and across every other project. +func UserNotify(path string) (NotifyConfig, error) { + path = strings.TrimSpace(path) + if path == "" { + return NotifyConfig{}, nil + } + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return NotifyConfig{}, nil + } + return NotifyConfig{}, fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return NotifyConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + cfg.Notify.Mode = strings.TrimSpace(cfg.Notify.Mode) + cfg.Notify.FocusMode = strings.TrimSpace(cfg.Notify.FocusMode) + return cfg.Notify, nil +} + // SetNotify persists the TUI notification preference. Both fields are trimmed // and validated against the accepted vocab (mode in {off,bell,notify,both}; // focusMode in {unfocused,always,focused}) so a bad caller cannot write a value // the resolver would later reject at startup. An empty Mode or FocusMode is -// stored as-is — the resolver applies the built-in defaults at read time, so a -// blank value means "use defaults" rather than "no notify" or "no focus rule". +// stored as-is — blank means "use the built-in defaults" (the TUI's +// effectiveTUINotifyMode maps an empty mode to both; the notify package treats +// an empty focusMode as unfocused), not "off". func SetNotify(path string, value NotifyConfig) (FileConfig, error) { path = strings.TrimSpace(path) if path == "" { diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index da3c2ac0b..69a06a488 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -417,9 +417,9 @@ func TestSetNotifyRejectsEmptyConfigPath(t *testing.T) { } func TestSetNotifyBlankValuesPreservedAsDefaults(t *testing.T) { - // An empty mode/focusMode stored on disk is a valid "use the resolver + // An empty mode/focusMode stored on disk is a valid "use the built-in // defaults" signal — SetNotify must not reject blanks, and they must round - // trip unchanged so the resolver can apply its built-in fallback. + // trip unchanged. path := filepath.Join(t.TempDir(), "zero.json") writeConfigFixture(t, path, FileConfig{ActiveProvider: "openai"}, 0o600) if _, err := SetNotify(path, NotifyConfig{}); err != nil { @@ -431,6 +431,39 @@ func TestSetNotifyBlankValuesPreservedAsDefaults(t *testing.T) { } } +// UserNotify reads the notify block from the user's own file. Partial updates +// seed from this value so they preserve what the USER chose (blank included) +// instead of copying a project config's setting or a pinned default into the +// global file (maintainer review, PR #1001). +func TestUserNotify(t *testing.T) { + dir := t.TempDir() + + // Missing file: zero value, no error. + got, err := UserNotify(filepath.Join(dir, "missing.json")) + if err != nil { + t.Fatalf("missing file should not error: %v", err) + } + if got.Mode != "" || got.FocusMode != "" { + t.Fatalf("missing file = %+v, want zero value", got) + } + + // Present block: trimmed values returned. + path := filepath.Join(dir, "zero.json") + writeConfigFixture(t, path, FileConfig{Notify: NotifyConfig{Mode: " bell ", FocusMode: " always "}}, 0o600) + got, err = UserNotify(path) + if err != nil { + t.Fatalf("UserNotify: %v", err) + } + if got.Mode != "bell" || got.FocusMode != "always" { + t.Fatalf("UserNotify = %+v, want bell/always (trimmed)", got) + } + + // Blank path: zero value, no error. + if got, err = UserNotify(""); err != nil || got.Mode != "" || got.FocusMode != "" { + t.Fatalf("blank path = %+v err=%v, want zero value", got, err) + } +} + func TestRecapsPreferenceRoundTrips(t *testing.T) { // Default (unset) is ON. if !(PreferencesConfig{}).RecapsEnabled() { diff --git a/internal/tui/notify_select.go b/internal/tui/notify_select.go index 6aa7e2d84..d9df97180 100644 --- a/internal/tui/notify_select.go +++ b/internal/tui/notify_select.go @@ -7,50 +7,60 @@ import ( "github.com/Gitlawb/zero/internal/notify" ) -// notifyChoice is one row in the /notify picker. The mode and focusMode pair is -// the on-disk shape; the label is what the user reads. +// notifyChoice is one row in the /notify picker: a (mode, focusMode) pair and +// the label the user reads. type notifyChoice struct { label string - subtitle string mode string focusMode string } -// notifyChoices is the ordered list shown by the /notify picker. "Unfocused + -// both" (the resolver default) is first because it is the most useful option -// for users who do not already have a strong opinion. "Silent" is last so the -// recommended path is also the visually-defaulted one. Adding a new (mode, -// focus) pair is enough to extend the picker and the /notify state list. -var notifyChoices = []notifyChoice{ - { - label: "Notify when unfocused (recommended)", - subtitle: "Sound + desktop notification when the terminal is in the background.", - mode: string(notify.ModeBoth), - focusMode: string(notify.FocusUnfocused), - }, - { - label: "Always notify", - subtitle: "Sound + desktop notification every time Zero needs your input.", - mode: string(notify.ModeBoth), - focusMode: string(notify.FocusAlways), - }, - { - label: "Bell only", - subtitle: "Terminal bell (no desktop notification) every time Zero needs your input.", - mode: string(notify.ModeBell), - focusMode: string(notify.FocusAlways), - }, - { - label: "Silent", - subtitle: "Show prompts in the TUI only — no extra sound or notification.", - mode: string(notify.ModeOff), - focusMode: string(notify.FocusUnfocused), - }, +// notifyChoiceSubtitle renders the human explanation shown as the picker row's +// Meta text: " — ". +func (c notifyChoice) subtitle() string { + modeDescriptions := map[string]string{ + string(notify.ModeOff): "silent", + string(notify.ModeBell): "terminal bell only", + string(notify.ModeNotify): "desktop notification only", + string(notify.ModeBoth): "terminal bell + desktop notification", + } + focusDescriptions := map[string]string{ + string(notify.FocusUnfocused): "only when the terminal is in the background", + string(notify.FocusAlways): "every time", + string(notify.FocusFocused): "only while the terminal is focused", + } + return modeDescriptions[c.mode] + " — " + focusDescriptions[c.focusMode] +} + +// notifyPickerChoices enumerates the FULL mode x focus space (4 modes x 3 +// focus modes = 12 rows), the way newThemePicker enumerates every theme. The +// earlier 4-row curated list could not represent the other 8 valid pairs, so +// opening the picker on one of them fell through to row 0 and Enter silently +// committed a different setting than the user's current one (maintainer +// review, PR #1001). Every valid pair must have a row so Enter always keeps +// (or explicitly changes) the user's actual setting. +func notifyPickerChoices() []notifyChoice { + modes := []string{string(notify.ModeBoth), string(notify.ModeBell), string(notify.ModeNotify), string(notify.ModeOff)} + foci := []string{string(notify.FocusUnfocused), string(notify.FocusAlways), string(notify.FocusFocused)} + choices := make([]notifyChoice, 0, len(modes)*len(foci)) + for _, mode := range modes { + for _, focus := range foci { + choices = append(choices, notifyChoice{ + label: mode + " · " + focus, + mode: mode, + focusMode: focus, + }) + } + } + return choices } // handleNotifyCommand implements /notify [list|off|bell|notify|both [focus]]. -// Bare `/notify` opens the picker at the dispatch layer; a mode-only argument -// keeps the existing focusMode. Mirrors handleThemeCommand. +// Bare `/notify` opens the picker at the dispatch layer. A mode-only argument +// preserves the focusMode stored in the USER'S OWN config (blank stays blank); +// seeding from the model's in-session value would copy a project config's +// choice, or a pinned default, into the user's global file. Mirrors +// handleThemeCommand. func (m model) handleNotifyCommand(args string) (model, string) { tokens := strings.Fields(strings.TrimSpace(args)) if len(tokens) == 0 || tokens[0] == "list" { @@ -69,13 +79,16 @@ func (m model) handleNotifyCommand(args string) (model, string) { if !isValidNotifyFocusMode(focus) { return m, "Notify\nUnknown focus mode: " + tokens[1] + " (expected unfocused, always, or focused)" } - } else { - focus = m.notifyCurrentFocusMode() + } else if stored, err := m.storedNotify(); err == nil { + // Preserve what the USER chose (blank included); never the resolved + // view, which merges project config and in-session defaults. + focus = stored.FocusMode } m.notifyMode = mode m.notifyFocusMode = focus // Apply to the live notifier so the change takes effect on the next - // permission prompt in this session, not just after a restart. + // permission prompt in this session, not just after a restart. A blank + // focusMode is fine here: the notifier treats blank as "unfocused". if m.notifier != nil { m.notifier.Configure(notify.Config{ Mode: notify.Mode(mode), @@ -84,7 +97,7 @@ func (m model) handleNotifyCommand(args string) (model, string) { } lines := []string{ "Notify", - "active mode: " + mode + ", focus: " + focus, + "active mode: " + mode + ", focus: " + effectiveFocusLabel(focus), } if note := m.persistNotifyPreference(mode, focus); note != "" { lines = append(lines, note) @@ -92,6 +105,25 @@ func (m model) handleNotifyCommand(args string) (model, string) { return m, strings.Join(lines, "\n") } +// storedNotify reads the notify block from the user's own config file. Missing +// file or read error returns the zero value (best-effort, like the rest of the +// preference persistence). +func (m model) storedNotify() (config.NotifyConfig, error) { + if strings.TrimSpace(m.userConfigPath) == "" { + return config.NotifyConfig{}, nil + } + return config.UserNotify(m.userConfigPath) +} + +// effectiveFocusLabel renders a focus value for the state line: blank means the +// built-in "unfocused" default, so say so instead of showing an empty string. +func effectiveFocusLabel(focus string) string { + if strings.TrimSpace(focus) == "" { + return "unfocused (default)" + } + return focus +} + // persistNotifyPreference writes the choice to user config so it survives a // restart. Best-effort: returns a short note to surface on failure, or "" on // success / when there is no config path (e.g. tests). @@ -108,21 +140,24 @@ func (m model) persistNotifyPreference(mode string, focus string) string { return "" } -// notifyStateText renders the /notify state view: current mode + focus + the -// picker rows, so the user has the same information whether they ran +// notifyStateText renders the /notify state view: the stored preference plus +// every valid pair, so the user has the same information whether they ran // `/notify list` or just opened the picker. func (m model) notifyStateText() string { - activeMode := m.notifyCurrentMode() - activeFocus := m.notifyCurrentFocusMode() + stored, _ := m.storedNotify() + mode := stored.Mode + if strings.TrimSpace(mode) == "" { + mode = string(effectiveTUINotifyMode(mode)) + } sections := []commandSection{{ Title: "State", Lines: []string{ - "active mode: " + activeMode, - "active focus: " + activeFocus, + "active mode: " + mode, + "active focus: " + effectiveFocusLabel(stored.FocusMode), }, }} - rows := make([]string, 0, len(notifyChoices)) - for _, c := range notifyChoices { + rows := make([]string, 0, 12) + for _, c := range notifyPickerChoices() { rows = append(rows, c.label) } sections = append(sections, commandSection{ @@ -137,18 +172,6 @@ func (m model) notifyStateText() string { }) } -// notifyCurrentMode and notifyCurrentFocusMode return the in-session notify -// preference. newModel populates both from options.Notify via -// effectiveTUINotifyMode (which never returns ""), so no empty fallback is -// needed here. -func (m model) notifyCurrentMode() string { - return m.notifyMode -} - -func (m model) notifyCurrentFocusMode() string { - return m.notifyFocusMode -} - // isValidNotifyMode reports whether s names one of the four notification modes. func isValidNotifyMode(s string) bool { switch s { diff --git a/internal/tui/notify_select_test.go b/internal/tui/notify_select_test.go index bbcedc564..a38694401 100644 --- a/internal/tui/notify_select_test.go +++ b/internal/tui/notify_select_test.go @@ -15,16 +15,15 @@ import ( "github.com/Gitlawb/zero/internal/notify" ) -// A committed /notify choice is written to user config and reloaded at startup -// (via the resolver's defaults + the notifyMode/notifyFocusMode fields on the -// model), so a /notify choice survives restart, just like /theme. +// A committed /notify choice is written to user config and reloaded at startup, +// so a /notify choice survives restart, just like /theme. func TestNotifyChoicePersistsAcrossRestart(t *testing.T) { cfgPath := filepath.Join(t.TempDir(), "config.json") - // First session: pick a non-default notify pair via the text handler (same - // commit path the picker uses via choosePicker). + // First session: pick a notify pair via the text handler (the same commit + // path the picker uses via choosePicker). m := newModel(context.Background(), Options{UserConfigPath: cfgPath}) - m, out := m.handleNotifyCommand("off") + m, out := m.handleNotifyCommand("off always") if m.notifyMode != "off" { t.Fatalf("notifyMode = %q, want off", m.notifyMode) } @@ -41,31 +40,55 @@ func TestNotifyChoicePersistsAcrossRestart(t *testing.T) { if err := json.Unmarshal(data, &cfg); err != nil { t.Fatalf("config is not valid JSON: %v", err) } - if cfg.Notify.Mode != "off" { - t.Fatalf("notify.mode = %q, want off", cfg.Notify.Mode) + if cfg.Notify.Mode != "off" || cfg.Notify.FocusMode != "always" { + t.Fatalf("notify = %+v, want mode=off focusMode=always", cfg.Notify) } - // Second session: the persisted notify block seeds the model fields so the - // /notify state line is correct and a permission prompt uses the right - // notifier (the runtime notifier is built from options.Notify, which is - // populated by the resolver from the same file). - restarted := newModel(context.Background(), Options{UserConfigPath: cfgPath, Notify: config.NotifyConfig{Mode: "off"}}) - if restarted.notifyMode != "off" { - t.Fatalf("restarted notifyMode = %q, want off (from saved config)", restarted.notifyMode) + // Second session: the persisted notify block seeds startup (options.Notify + // is populated by the resolver from the same file). + restarted := newModel(context.Background(), Options{UserConfigPath: cfgPath, Notify: config.NotifyConfig{Mode: "off", FocusMode: "always"}}) + if restarted.notifyMode != "off" || restarted.notifyFocusMode != "always" { + t.Fatalf("restarted = mode %q focus %q, want off/always (from saved config)", restarted.notifyMode, restarted.notifyFocusMode) } } -// `/notify` with a mode-only arg keeps the existing focusMode. A common mistake -// would be to reset the focus rule on every mode change. -func TestNotifyCommandPreservesFocusOnModeOnly(t *testing.T) { - m := newModel(context.Background(), Options{}) - m.notifyFocusMode = string(notify.FocusAlways) +// `/notify bell` (mode-only) preserves the focusMode stored in the USER'S OWN +// file — not the resolved view. A project config's choice must not be copied +// into the user's global file, and a blank focus must stay blank (blank means +// "use the built-in default"), so nothing is pinned as an explicit choice the +// user never made. Maintainer review, PR #1001. +func TestNotifyModeOnlyPreservesStoredFocusNotResolved(t *testing.T) { + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, []byte(`{"notify":{"focusMode":"focused"}}`), 0o600); err != nil { + t.Fatal(err) + } + + // The in-session (resolved) focus disagrees with the user's file; the + // write must take the user's file. + m := newModel(context.Background(), Options{ + UserConfigPath: cfgPath, + Notify: config.NotifyConfig{Mode: "both", FocusMode: "unfocused"}, + }) + m, _ = m.handleNotifyCommand("bell") + persisted := readNotifyBlock(t, cfgPath) + if persisted.Mode != "bell" { + t.Errorf("persisted mode = %q, want bell", persisted.Mode) + } + if persisted.FocusMode != "focused" { + t.Errorf("persisted focusMode = %q, want the user-file value focused (not the resolved unfocused)", persisted.FocusMode) + } + + // Blank stays blank: with nothing stored, a mode-only change must not pin + // the default focus as an explicit choice. + blankPath := filepath.Join(t.TempDir(), "config.json") + m = newModel(context.Background(), Options{UserConfigPath: blankPath}) m, _ = m.handleNotifyCommand("off") - if m.notifyMode != "off" { - t.Errorf("notifyMode = %q, want off", m.notifyMode) + persisted = readNotifyBlock(t, blankPath) + if persisted.Mode != "off" { + t.Errorf("persisted mode = %q, want off", persisted.Mode) } - if m.notifyFocusMode != string(notify.FocusAlways) { - t.Errorf("notifyFocusMode = %q, want preserved %q", m.notifyFocusMode, notify.FocusAlways) + if persisted.FocusMode != "" { + t.Errorf("persisted focusMode = %q, want blank (unspecified stays unspecified)", persisted.FocusMode) } } @@ -85,19 +108,17 @@ func TestNotifyCommandSetsModeAndFocus(t *testing.T) { // the next permission prompt in this session (not only after a restart). func TestNotifyCommandAppliesToLiveNotifier(t *testing.T) { var buf bytes.Buffer - // Construct through newModel so both fields are populated the way the real - // session does; then swap in a buffer-backed notifier to observe output. m := newModel(context.Background(), Options{Notify: config.NotifyConfig{Mode: "off", FocusMode: "always"}}) m.notifier = notify.New(&buf, notify.Config{Mode: notify.ModeOff, FocusMode: notify.FocusAlways}) m.notifier.SetFocused(true) - m, _ = m.handleNotifyCommand("bell") + m, _ = m.handleNotifyCommand("bell always") m.notifier.Notify(notify.Completion, "x") if buf.String() != "\x07" { t.Fatalf("live notifier should bell after /notify bell, got %q", buf.String()) } - m, _ = m.handleNotifyCommand("off") + m, _ = m.handleNotifyCommand("off always") m.notifier.Notify(notify.Completion, "x") if buf.String() != "\x07" { t.Fatalf("live notifier should go silent after /notify off, got %q", buf.String()) @@ -132,8 +153,8 @@ func TestNotifyCommandRejectsInvalidMode(t *testing.T) { } } -// `/notify bell sideways` rejects the focus mode but the call also failed -// validation before persisting, so neither field should change. +// `/notify bell sideways` fails validation before persisting, so neither field +// changes. func TestNotifyCommandRejectsInvalidFocus(t *testing.T) { m := newModel(context.Background(), Options{}) m.notifyMode = "bell" @@ -147,11 +168,48 @@ func TestNotifyCommandRejectsInvalidFocus(t *testing.T) { } } -// `/notify` with no argument opens the picker, just like /theme and /model. -func TestNotifyPickerOpensOnBareNotify(t *testing.T) { - m := newModel(context.Background(), Options{Notify: config.NotifyConfig{Mode: "off", FocusMode: "unfocused"}}) - m.input.SetValue("/notify") +// The picker enumerates the FULL mode x focus space (12 rows), so every valid +// pair is representable and Enter can never silently commit a different pair +// than the user's current one. Maintainer review, PR #1001: with a 4-row +// curated list, opening the picker on (off, always) preselected row 0 +// (both, unfocused) and Enter changed the setting. +func TestNotifyPickerEnumeratesFullSpace(t *testing.T) { + m := newModel(context.Background(), Options{}) + picker := m.newNotifyPicker() + if len(picker.items) != 12 { + t.Fatalf("picker has %d items, want 12 (4 modes x 3 focus modes)", len(picker.items)) + } + seen := map[string]bool{} + for _, item := range picker.items { + if seen[item.Value] { + t.Errorf("duplicate picker row %q", item.Value) + } + seen[item.Value] = true + } + for _, mode := range []string{"off", "bell", "notify", "both"} { + for _, focus := range []string{"unfocused", "always", "focused"} { + if !seen[mode+" "+focus] { + t.Errorf("picker is missing row for valid pair %q %q", mode, focus) + } + } + } +} +// Every pair the picker preselects must also be a row: Enter on an open +// picker must keep (or explicitly change) the user's actual setting. This is +// the maintainer's suggested regression: send Enter to an open picker from a +// pair that is NOT in a 4-row curated list — with full enumeration the +// preselected row IS the current pair and the commit is a no-op change. +func TestNotifyPickerEnterOnUnlistedPairKeepsSetting(t *testing.T) { + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, []byte(`{"notify":{"mode":"off","focusMode":"always"}}`), 0o600); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + UserConfigPath: cfgPath, + Notify: config.NotifyConfig{Mode: "off", FocusMode: "always"}, + }) + m.input.SetValue("/notify") updated, cmd := m.Update(testKey(tea.KeyEnter)) m = updated.(model) if cmd != nil { @@ -160,13 +218,21 @@ func TestNotifyPickerOpensOnBareNotify(t *testing.T) { if m.picker == nil || m.picker.kind != pickerNotify { t.Fatalf("expected the notify picker to open, got %#v", m.picker) } - if len(m.picker.items) != len(notifyChoices) { - t.Fatalf("picker has %d items, want %d", len(m.picker.items), len(notifyChoices)) + // (off, always) must be preselected — it is a valid pair even though the + // old curated list could not represent it. + if sel := m.picker.items[m.picker.selected]; sel.Value != "off always" { + t.Fatalf("preselected = %q, want the current pair %q", sel.Value, "off always") + } + + // Enter commits the preselected row: the setting is unchanged. + updated, _ = m.Update(testKey(tea.KeyEnter)) + m = updated.(model) + if m.picker != nil { + t.Fatal("picker should close on Enter") } - // The preselected row should match the active (mode, focus) pair. - sel := m.picker.items[m.picker.selected] - if sel.Value != "off unfocused" { - t.Errorf("preselected value = %q, want the active pair %q", sel.Value, "off unfocused") + persisted := readNotifyBlock(t, cfgPath) + if persisted.Mode != "off" || persisted.FocusMode != "always" { + t.Fatalf("Enter changed the setting: got %+v, want off/always unchanged", persisted) } } @@ -176,6 +242,9 @@ func TestNotifyPickerOpensOnBareNotify(t *testing.T) { func TestNotifyPickerValuesAreValidCommandArgs(t *testing.T) { m := newModel(context.Background(), Options{}) picker := m.newNotifyPicker() + if len(picker.items) == 0 { + t.Fatal("picker has no items") + } for _, item := range picker.items { tokens := strings.Fields(item.Value) if len(tokens) != 2 { @@ -191,32 +260,34 @@ func TestNotifyPickerValuesAreValidCommandArgs(t *testing.T) { } } -// The /notify state view shows the current mode and focus so users can see -// the value before opening the picker. -func TestNotifyStateTextShowsActivePair(t *testing.T) { - m := newModel(context.Background(), Options{}) - m.notifyMode = "both" - m.notifyFocusMode = "unfocused" +// The /notify state view shows the stored mode and focus (and labels a blank +// focus as the default) so users see the real value before opening the picker. +func TestNotifyStateTextShowsStoredPair(t *testing.T) { + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, []byte(`{"notify":{"mode":"bell","focusMode":"always"}}`), 0o600); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{UserConfigPath: cfgPath}) state := m.notifyStateText() - if !strings.Contains(state, "active mode: both") { - t.Errorf("state should show active mode, got: %s", state) + if !strings.Contains(state, "active mode: bell") { + t.Errorf("state should show stored mode, got: %s", state) } - if !strings.Contains(state, "active focus: unfocused") { - t.Errorf("state should show active focus, got: %s", state) + if !strings.Contains(state, "active focus: always") { + t.Errorf("state should show stored focus, got: %s", state) } } -// notifyCurrentMode / notifyCurrentFocusMode surface the in-session fields -// that newModel populates from options.Notify, so /notify reads the same -// value the runtime notifier uses. -func TestNotifyCurrentReflectsModelFields(t *testing.T) { - m := newModel(context.Background(), Options{}) - m.notifyMode = "bell" - m.notifyFocusMode = "always" - if got := m.notifyCurrentMode(); got != "bell" { - t.Errorf("notifyCurrentMode = %q, want bell", got) +func readNotifyBlock(t *testing.T, path string) config.NotifyConfig { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read config %s: %v", path, err) } - if got := m.notifyCurrentFocusMode(); got != "always" { - t.Errorf("notifyCurrentFocusMode = %q, want always", got) + var cfg struct { + Notify config.NotifyConfig `json:"notify"` + } + if err := json.Unmarshal(data, &cfg); err != nil { + t.Fatalf("decode config %s: %v", path, err) } + return cfg.Notify } diff --git a/internal/tui/picker.go b/internal/tui/picker.go index e885f925d..1a72b2656 100644 --- a/internal/tui/picker.go +++ b/internal/tui/picker.go @@ -11,6 +11,7 @@ import ( "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/modelregistry" + "github.com/Gitlawb/zero/internal/notify" "github.com/Gitlawb/zero/internal/providercatalog" "github.com/Gitlawb/zero/internal/providermodelcatalog" "github.com/Gitlawb/zero/internal/providermodeldiscovery" @@ -1067,24 +1068,31 @@ func (m model) newThemePicker() *commandPicker { return &commandPicker{kind: pickerTheme, title: "Choose a theme", items: items, allItems: append([]pickerItem{}, items...), selected: selected} } -// newNotifyPicker lists the four (mode, focus) pairs from notifyChoices. Each +// newNotifyPicker lists the FULL (mode, focus) space from notifyPickerChoices +// (every valid pair has a row, so a stored setting like (off, always) is always +// preselectable and Enter can never silently commit a different pair). Each // row's Value is the same synthetic string the text /notify handler accepts // (" "), so /notify with no arg and the picker share one commit -// path through handleNotifyCommand. The currently active pair is preselected so -// the user can press Enter to keep it. There is no live preview — notify -// affects the next permission prompt, not the current view — so the picker -// does not call a preview function on move. +// path through handleNotifyCommand. A blank stored field resolves to its +// effective default for preselection only (both / unfocused — what actually +// fires today); committing any row writes an explicit pair. There is no live +// preview — notify affects the next permission prompt, not the current view. func (m model) newNotifyPicker() *commandPicker { - items := make([]pickerItem, 0, len(notifyChoices)) + choices := notifyPickerChoices() + items := make([]pickerItem, 0, len(choices)) selected := 0 - activeMode := m.notifyCurrentMode() - activeFocus := m.notifyCurrentFocusMode() - for _, c := range notifyChoices { + stored, _ := m.storedNotify() + activeMode := string(effectiveTUINotifyMode(stored.Mode)) + activeFocus := stored.FocusMode + if strings.TrimSpace(activeFocus) == "" { + activeFocus = string(notify.FocusUnfocused) + } + for _, c := range choices { items = append(items, pickerItem{ Group: "When Zero needs your input", Label: c.label, Value: c.mode + " " + c.focusMode, - Meta: c.subtitle, + Meta: c.subtitle(), }) if c.mode == activeMode && c.focusMode == activeFocus { selected = len(items) - 1 From 703d9f5c3c7bbe2a5fcb20cdcf950b046e3ec9e9 Mon Sep 17 00:00:00 2001 From: Gaurav Bhatia Date: Thu, 3 Sep 2026 14:17:14 +0400 Subject: [PATCH 5/8] fix(cli): zero config notify no longer requires a configured provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The command resolved the full config (providers included) before touching notification settings, so a fresh user with no provider hit ErrNoActiveProvider and could not read, set, or reset their notify preference — the exact first-run user this feature targets (CodeRabbit review, PR #1001). The command manages a user preference, so it now talks only to the user's own config file (config.UserNotify / config.SetNotify) and never runs config resolution. Display reports the user's stored values — a project config that overrides notify for one repo is not shown here, matching the write path's source-of-truth from the maintainer review. --- internal/cli/config_notify.go | 50 +++++++++++----------- internal/cli/config_notify_test.go | 67 ++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 25 deletions(-) diff --git a/internal/cli/config_notify.go b/internal/cli/config_notify.go index 9c8216971..b8a91a117 100644 --- a/internal/cli/config_notify.go +++ b/internal/cli/config_notify.go @@ -11,7 +11,16 @@ import ( // runConfigNotify implements `zero config notify`: with no flags it prints the // current mode/focusMode; --mode/--focus update them via the same // config.SetNotify writer the TUI /notify command uses, so all surfaces stay -// in lockstep; --reset blanks both fields so the resolver defaults apply. +// in lockstep; --reset blanks both fields so the built-in defaults apply. +// +// The command manages a user preference, so it talks ONLY to the user's own +// config file (config.UserNotify / config.SetNotify) and never runs the full +// config resolution: resolving providers would fail with ErrNoActiveProvider +// for a brand-new user, locking them out of setting notifications before they +// have even configured a provider (CodeRabbit review, PR #1001). The display +// therefore reports the USER'S stored values — a project config that +// overrides notify for one repo is not shown here, by the same logic the +// maintainer applied to the write path. func runConfigNotify(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseConfigNotifyArgs(args) if err != nil { @@ -24,23 +33,15 @@ func runConfigNotify(args []string, stdout io.Writer, stderr io.Writer, deps app return exitSuccess } - resolved, exitCode := resolveCommandCenterConfig(stderr, deps) - if exitCode != exitSuccess { - return exitCode + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) } if options.mode != "" || options.focus != "" || options.reset { - configPath, err := deps.userConfigPath() - if err != nil { - return writeAppError(stderr, err.Error(), exitCrash) - } - // Seed omitted fields from the USER'S OWN file, never from the - // resolved view: resolved merges project config (so a repo's - // mode:off would be copied into the user's global settings) and - // carries no defaults here, but seeding from it would also pin - // defaults as explicit choices. Blank stays blank — blank means - // "use the built-in defaults". --reset is the only path that - // clears both fields. + // Seed omitted fields from the USER'S OWN file. Blank stays blank — + // blank means "use the built-in defaults"; --reset is the only path + // that clears both fields. current, err := config.UserNotify(configPath) if err != nil { return writeAppError(stderr, err.Error(), exitUsage) @@ -59,18 +60,17 @@ func runConfigNotify(args []string, stdout io.Writer, stderr io.Writer, deps app if _, err := config.SetNotify(configPath, notify); err != nil { return writeAppError(stderr, err.Error(), exitUsage) } - // Re-resolve so the printed value reflects what the next launch will - // actually use (e.g. a reset shows the built-in defaults). - resolved, exitCode = resolveCommandCenterConfig(stderr, deps) - if exitCode != exitSuccess { - return exitCode - } } + // Report the stored values (blank renders as "(default)"). + current, err := config.UserNotify(configPath) + if err != nil { + return writeAppError(stderr, err.Error(), exitUsage) + } if options.json { if err := writePrettyJSON(stdout, map[string]any{ - "mode": resolved.Notify.Mode, - "focusMode": resolved.Notify.FocusMode, + "mode": current.Mode, + "focusMode": current.FocusMode, }); err != nil { return exitCrash } @@ -78,8 +78,8 @@ func runConfigNotify(args []string, stdout io.Writer, stderr io.Writer, deps app } lines := []string{ "Notify", - "mode: " + displayCLIValue(resolved.Notify.Mode, "(default)"), - "focusMode: " + displayCLIValue(resolved.Notify.FocusMode, "(default)"), + "mode: " + displayCLIValue(current.Mode, "(default)"), + "focusMode: " + displayCLIValue(current.FocusMode, "(default)"), } if _, err := fmt.Fprintln(stdout, strings.Join(lines, "\n")); err != nil { return exitCrash diff --git a/internal/cli/config_notify_test.go b/internal/cli/config_notify_test.go index eec1ec6d8..0b3e868a1 100644 --- a/internal/cli/config_notify_test.go +++ b/internal/cli/config_notify_test.go @@ -399,6 +399,73 @@ func TestRunConfigNotifyResetClearsStoredValues(t *testing.T) { } } +// CodeRabbit regression (PR #1001): the command manages a user preference and +// must not require provider resolution. A brand-new user with NO provider +// configured (the resolver would return ErrNoActiveProvider) can still read, +// set, and reset their notification preference. +func TestRunConfigNotifyWorksWithoutAnyProviderConfigured(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{}`), 0o600); err != nil { + t.Fatal(err) + } + // resolveConfig fails the way the real resolver does for a fresh user — + // the command must never call it, so a panic-free stub is enough to prove + // the point; use the failing resolver to catch any regression to the + // resolve-first shape. + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{}}) + } + + // Read works and shows defaults. + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"config", "notify"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("read: exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + if !strings.Contains(stdout.String(), "mode: (default)") { + t.Errorf("read should show (default), got: %s", stdout.String()) + } + + // Write works. + stdout.Reset() + exitCode = runWithDeps([]string{"config", "notify", "--mode", "bell", "--focus", "always"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("write: exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if cfg.Notify.Mode != "bell" || cfg.Notify.FocusMode != "always" { + t.Fatalf("write: Notify = %+v, want bell/always", cfg.Notify) + } + + // JSON read reflects the stored pair. + stdout.Reset() + exitCode = runWithDeps([]string{"config", "notify", "--json"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("json: exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + var payload map[string]any + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("json output invalid: %v\n%s", err, stdout.String()) + } + if payload["mode"] != "bell" || payload["focusMode"] != "always" { + t.Errorf("json = mode %v focus %v, want bell/always", payload["mode"], payload["focusMode"]) + } + + // Reset works. + stdout.Reset() + exitCode = runWithDeps([]string{"config", "notify", "--reset"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("reset: exit = %d, want %d: %s", exitCode, exitSuccess, stderr.String()) + } + cfg = readFileConfig(t, configPath) + if cfg.Notify.Mode != "" || cfg.Notify.FocusMode != "" { + t.Errorf("reset: Notify = %+v, want empty", cfg.Notify) + } +} + // `zero config` (no subcommand) still works after the dispatch change. func TestRunConfigSummaryStillWorks(t *testing.T) { var stdout bytes.Buffer From 4d83091995b314c6a452bc8fb8e2c9b739a20780 Mon Sep 17 00:00:00 2001 From: Gaurav Bhatia Date: Sat, 5 Sep 2026 14:00:04 +0400 Subject: [PATCH 6/8] fix(notify): separate live session policy from persisted user preference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review from jatmn (PR #1001). The three notification states — stored user file, resolved startup pair, live session — are distinct contracts and must not be conflated: - handleNotifyCommand now keeps two focus values per update: the LIVE focus (in-session, initialized from the resolved pair, so a project's focus rule keeps applying) feeds the notifier's Configure and the status line; the PERSISTED focus seeds from the user's own file (config.UserNotify), so a mode-only /notify writes the mode while the user's focus value — blank included — is preserved and project values never leak into the global file. - /notify list reports the live pair (model fields), not the stored file, which can legitimately disagree under a project override. - the /notify picker preselects the live pair; committing a row is an explicit choice, so Enter on the preselected row keeps the setting. - /notify list requires exactly one token, matching /theme, /effort, and /style (rejects "/notify list junk"). - shell completions: config gained a notify child with its flags (--mode/--focus/--reset/--json), plus a completion-tree assertion for both contexts so new CLI surfaces cannot go stale. - help/copy pass: the CLI help now describes the stored global notification preference controlling BOTH the completion and needs-input alerts, --reset as returning the TUI to its effective default (headless unconfigured stays silent), lists all three focus values in /notify usage, and drops the stale "resolver default" wording from effectiveTUINotifyMode and the command doc. - tests: contract-matrix coverage with the three states deliberately disagreeing (blank user file + project-resolved off/focused) asserting list/picker/preselection/Enter, mode-only and explicit-pair updates on both live and persisted sides, and restart precedence; the SA4006 lint hits are resolved by asserting the returned live model. lint-static: 0 issues. Affected suites green under -race; the only remaining local failures are the known environment flakes (TestRunAuthOpenRouterSavesMintedKey on macOS keychain under a sanitized env, and TestLoadProviderCommand* 5s subprocess timeouts under full-suite load — both pass in isolation and fail identically on clean origin/main here). --- internal/cli/completions.go | 6 +- internal/cli/completions_test.go | 5 + internal/cli/config_notify.go | 26 +++-- internal/tui/commands.go | 4 +- internal/tui/model.go | 12 +- internal/tui/notify_select.go | 70 ++++++----- internal/tui/notify_select_test.go | 182 +++++++++++++++++++++++++---- internal/tui/picker.go | 21 ++-- 8 files changed, 252 insertions(+), 74 deletions(-) diff --git a/internal/cli/completions.go b/internal/cli/completions.go index 5f9058946..e47d1e67d 100644 --- a/internal/cli/completions.go +++ b/internal/cli/completions.go @@ -40,7 +40,11 @@ var completionRoot = completionNode{ }, {names: []string{"daemon"}, children: leafNodes("start", "stop", "status", "run", "attach", "serve-remote", "link")}, {names: []string{"setup"}}, - {names: []string{"config"}}, + {names: []string{"config"}, flags: []string{"-h", "--help", "--json"}, children: []completionNode{ + {names: []string{"notify"}, flags: []string{ + "-h", "--help", "--mode", "--focus", "--reset", "--json", + }}, + }}, {names: []string{"models"}, children: []completionNode{{names: []string{"list", "ls"}}}}, {names: []string{"providers"}, children: []completionNode{ {names: []string{"current"}}, {names: []string{"list"}}, {names: []string{"catalog"}}, diff --git a/internal/cli/completions_test.go b/internal/cli/completions_test.go index b7eb059b7..ebfedbb72 100644 --- a/internal/cli/completions_test.go +++ b/internal/cli/completions_test.go @@ -163,6 +163,11 @@ func TestCompletionTreeCoversAliasesNestingAndCommonFlags(t *testing.T) { assertCandidates(t, byPath["completions"], "bash", "zsh", "fish", "powershell", "elvish") assertCandidates(t, byPath["plugins"], "list", "add", "info", "remove", "rm") assertCandidates(t, byPath["plugin"], "list", "add", "info", "remove", "rm") + // `config` gained the notify subcommand; the completion tree must expose + // it (and its flags) so shell completion cannot go stale for new CLI + // surfaces (maintainer review, PR #1001). + assertCandidates(t, byPath["config"], "notify", "--json", "--help") + assertCandidates(t, byPath["config notify"], "--mode", "--focus", "--reset", "--json", "--help") } func assertCandidates(t *testing.T, got []string, wants ...string) { diff --git a/internal/cli/config_notify.go b/internal/cli/config_notify.go index b8a91a117..07c4a7e48 100644 --- a/internal/cli/config_notify.go +++ b/internal/cli/config_notify.go @@ -9,9 +9,10 @@ import ( ) // runConfigNotify implements `zero config notify`: with no flags it prints the -// current mode/focusMode; --mode/--focus update them via the same +// stored mode/focusMode; --mode/--focus update them via the same // config.SetNotify writer the TUI /notify command uses, so all surfaces stay -// in lockstep; --reset blanks both fields so the built-in defaults apply. +// in lockstep; --reset blanks both fields so the TUI's effective default +// applies again (an unconfigured headless run stays silent). // // The command manages a user preference, so it talks ONLY to the user's own // config file (config.UserNotify / config.SetNotify) and never runs the full @@ -144,25 +145,30 @@ func writeConfigNotifyHelp(w io.Writer) error { _, err := fmt.Fprint(w, "Usage:\n"+ " zero config notify [flags]\n"+ "\n"+ - "Print or update the permission-prompt notify preference.\n"+ + "Print or update the stored global notification preference.\n"+ "\n"+ - "When run with no flag, prints the current mode and focusMode; a field you\n"+ + "The preference controls BOTH notification kinds: the turn-completion\n"+ + "(\"Zero: ready\") alert and the needs-input alert. mode off silences both;\n"+ + "the focus mode (unfocused, always, focused) applies to both.\n"+ + "\n"+ + "When run with no flag, prints the stored mode and focusMode; a field you\n"+ "never set shows as (default) — the TUI alerts with bell + notification,\n"+ - "firing only when the terminal is unfocused. Omitted flags preserve the\n"+ - "values stored in YOUR config file; --reset clears both so the defaults\n"+ - "apply again.\n"+ + "firing only when the terminal is unfocused, while an unconfigured\n"+ + "headless run stays silent. Omitted flags preserve the values stored in\n"+ + "YOUR config file; --reset clears both so the TUI's effective default\n"+ + "applies again.\n"+ "\n"+ "Examples:\n"+ " zero config notify\n"+ " zero config notify --json\n"+ " zero config notify --mode both --focus unfocused\n"+ " zero config notify --mode off\n"+ - " zero config notify --reset # clear config so the resolver defaults apply\n"+ + " zero config notify --reset # clear the stored preference\n"+ "\n"+ "Flags:\n"+ - " --mode Notification mechanism\n"+ + " --mode Notification mechanism (both kinds)\n"+ " --focus When the alert fires\n"+ - " --reset Clear both fields so the resolver defaults apply\n"+ + " --reset Clear the stored preference so the TUI effective default applies\n"+ " --json Machine-readable output\n"+ " -h, --help Show this help\n") return err diff --git a/internal/tui/commands.go b/internal/tui/commands.go index a82786127..9691aab0d 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -403,9 +403,9 @@ var commandDefinitions = []commandDefinition{ }, { name: "/notify", - usage: "/notify [list|off|bell|notify|both [unfocused|always]]", + usage: "/notify [list|off|bell|notify|both [unfocused|always|focused]]", group: commandGroupSession, - description: "Pick when Zero alerts you it needs input. No arg opens the picker.", + description: "Pick when Zero alerts you (completion and needs-input). No arg opens the picker.", kind: commandNotify, }, { diff --git a/internal/tui/model.go b/internal/tui/model.go index c9f07e89c..7ffeb4a30 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -882,11 +882,13 @@ type tuiAgentRunOptions struct { } // effectiveTUINotifyMode returns the notification mode the TUI should use. An -// empty/unconfigured mode falls back to the resolver default ("both": terminal -// bell + OSC-9 desktop notification) so the permission-prompt alert works for -// new users without requiring them to hand-edit config.json. The /notify -// command persists explicit choices; the resolver applies the same default at -// read time, so this function and the resolver always agree. +// empty/unconfigured mode falls back to the TUI's own effective default +// ("both": terminal bell + OSC-9 desktop notification) so the needs-input +// alert works for new users without requiring them to hand-edit config.json. +// The default lives HERE, deliberately not in config.Resolve: the resolver is +// shared with headless `zero exec`, which must stay byte-silent when +// unconfigured (maintainer review, PR #1001). The /notify command persists +// explicit choices; blank stays blank on disk. func effectiveTUINotifyMode(mode string) notify.Mode { m := notify.Mode(strings.TrimSpace(mode)) if m == "" { diff --git a/internal/tui/notify_select.go b/internal/tui/notify_select.go index d9df97180..092ae0414 100644 --- a/internal/tui/notify_select.go +++ b/internal/tui/notify_select.go @@ -56,14 +56,31 @@ func notifyPickerChoices() []notifyChoice { } // handleNotifyCommand implements /notify [list|off|bell|notify|both [focus]]. -// Bare `/notify` opens the picker at the dispatch layer. A mode-only argument -// preserves the focusMode stored in the USER'S OWN config (blank stays blank); -// seeding from the model's in-session value would copy a project config's -// choice, or a pinned default, into the user's global file. Mirrors -// handleThemeCommand. +// Bare `/notify` opens the picker at the dispatch layer. +// +// Two contracts share this handler and must stay separate (maintainer review, +// PR #1001): +// +// - LIVE policy (m.notifyMode/m.notifyFocusMode, m.notifier): what the +// current session uses, initialized by newModel from the RESOLVED pair +// (user config after project precedence). A mode-only change preserves the +// live focus so a project's focus rule keeps applying to the session, and +// the change reaches the notifier immediately. +// - PERSISTED policy (the user's own config file): a partial write seeds +// omitted fields from config.UserNotify — the user's global choice — +// never from the resolved view, so a project's value cannot leak into the +// global file and a blank stays blank. +// +// Mirrors handleThemeCommand. func (m model) handleNotifyCommand(args string) (model, string) { tokens := strings.Fields(strings.TrimSpace(args)) - if len(tokens) == 0 || tokens[0] == "list" { + if len(tokens) == 0 { + return m, m.notifyStateText() + } + if tokens[0] == "list" { + if len(tokens) != 1 { + return m, "Notify\nToo many arguments: " + args + " (usage: /notify list)" + } return m, m.notifyStateText() } if len(tokens) > 2 { @@ -73,33 +90,38 @@ func (m model) handleNotifyCommand(args string) (model, string) { if !isValidNotifyMode(mode) { return m, "Notify\nUnknown mode: " + tokens[0] + " (expected off, bell, notify, or both; run /notify with no argument to pick from the list)" } - focus := "" + // LIVE focus: an explicit token wins; otherwise preserve the in-session + // value (which started as the resolved pair, project precedence included). + liveFocus := m.notifyFocusMode + persistFocus := m.notifyFocusMode if len(tokens) > 1 { - focus = strings.ToLower(strings.TrimSpace(tokens[1])) + focus := strings.ToLower(strings.TrimSpace(tokens[1])) if !isValidNotifyFocusMode(focus) { return m, "Notify\nUnknown focus mode: " + tokens[1] + " (expected unfocused, always, or focused)" } + liveFocus = focus + persistFocus = focus } else if stored, err := m.storedNotify(); err == nil { - // Preserve what the USER chose (blank included); never the resolved - // view, which merges project config and in-session defaults. - focus = stored.FocusMode + // PERSISTED focus only: what the USER's global file holds (blank stays + // blank). The live session keeps its resolved focus above. + persistFocus = stored.FocusMode } m.notifyMode = mode - m.notifyFocusMode = focus + m.notifyFocusMode = liveFocus // Apply to the live notifier so the change takes effect on the next - // permission prompt in this session, not just after a restart. A blank + // notification in this session, not just after a restart. A blank // focusMode is fine here: the notifier treats blank as "unfocused". if m.notifier != nil { m.notifier.Configure(notify.Config{ Mode: notify.Mode(mode), - FocusMode: notify.FocusMode(focus), + FocusMode: notify.FocusMode(liveFocus), }) } lines := []string{ "Notify", - "active mode: " + mode + ", focus: " + effectiveFocusLabel(focus), + "active mode: " + mode + ", focus: " + effectiveFocusLabel(liveFocus), } - if note := m.persistNotifyPreference(mode, focus); note != "" { + if note := m.persistNotifyPreference(mode, persistFocus); note != "" { lines = append(lines, note) } return m, strings.Join(lines, "\n") @@ -140,20 +162,16 @@ func (m model) persistNotifyPreference(mode string, focus string) string { return "" } -// notifyStateText renders the /notify state view: the stored preference plus -// every valid pair, so the user has the same information whether they ran -// `/notify list` or just opened the picker. +// notifyStateText renders the /notify state view: the LIVE session policy +// (model fields, initialized from the resolved pair and updated by /notify) — +// not the stored file, which can legitimately disagree when a project config +// overrides notify for this session (maintainer review, PR #1001). func (m model) notifyStateText() string { - stored, _ := m.storedNotify() - mode := stored.Mode - if strings.TrimSpace(mode) == "" { - mode = string(effectiveTUINotifyMode(mode)) - } sections := []commandSection{{ Title: "State", Lines: []string{ - "active mode: " + mode, - "active focus: " + effectiveFocusLabel(stored.FocusMode), + "active mode: " + m.notifyMode, + "active focus: " + effectiveFocusLabel(m.notifyFocusMode), }, }} rows := make([]string, 0, 12) diff --git a/internal/tui/notify_select_test.go b/internal/tui/notify_select_test.go index a38694401..0bf549c23 100644 --- a/internal/tui/notify_select_test.go +++ b/internal/tui/notify_select_test.go @@ -52,37 +52,57 @@ func TestNotifyChoicePersistsAcrossRestart(t *testing.T) { } } -// `/notify bell` (mode-only) preserves the focusMode stored in the USER'S OWN -// file — not the resolved view. A project config's choice must not be copied -// into the user's global file, and a blank focus must stay blank (blank means -// "use the built-in default"), so nothing is pinned as an explicit choice the -// user never made. Maintainer review, PR #1001. -func TestNotifyModeOnlyPreservesStoredFocusNotResolved(t *testing.T) { +// Mode-only `/notify ` keeps the LIVE and PERSISTED policies separate +// (maintainer review, PR #1001): the live session preserves its in-session +// focus (initialized from the resolved pair, so a project's focus rule keeps +// applying), while the global write seeds from the USER'S OWN file — blank +// stays blank, and a project-derived value is never copied into the global +// config. The three states here deliberately disagree: +// +// user file: focusMode=focused, no mode +// resolved: both/unfocused (as a project override would produce) +// live start: both/unfocused +func TestNotifyModeOnlySplitsLiveAndPersistedFocus(t *testing.T) { cfgPath := filepath.Join(t.TempDir(), "config.json") if err := os.WriteFile(cfgPath, []byte(`{"notify":{"focusMode":"focused"}}`), 0o600); err != nil { t.Fatal(err) } - - // The in-session (resolved) focus disagrees with the user's file; the - // write must take the user's file. m := newModel(context.Background(), Options{ UserConfigPath: cfgPath, Notify: config.NotifyConfig{Mode: "both", FocusMode: "unfocused"}, }) + m, _ = m.handleNotifyCommand("bell") + + // LIVE: mode changes, focus keeps the in-session (resolved) value — the + // user-file focus must NOT bleed into the session. + if m.notifyMode != "bell" { + t.Errorf("live mode = %q, want bell", m.notifyMode) + } + if m.notifyFocusMode != "unfocused" { + t.Errorf("live focus = %q, want the in-session unfocused (not the user-file focused)", m.notifyFocusMode) + } + // PERSISTED: the mode is written; the focus seeded from the user's file + // (their explicit focused), never the resolved/live value. persisted := readNotifyBlock(t, cfgPath) if persisted.Mode != "bell" { t.Errorf("persisted mode = %q, want bell", persisted.Mode) } if persisted.FocusMode != "focused" { - t.Errorf("persisted focusMode = %q, want the user-file value focused (not the resolved unfocused)", persisted.FocusMode) + t.Errorf("persisted focusMode = %q, want the user-file value focused", persisted.FocusMode) } // Blank stays blank: with nothing stored, a mode-only change must not pin - // the default focus as an explicit choice. + // the default focus as an explicit choice, and live still keeps its value. blankPath := filepath.Join(t.TempDir(), "config.json") - m = newModel(context.Background(), Options{UserConfigPath: blankPath}) + m = newModel(context.Background(), Options{ + UserConfigPath: blankPath, + Notify: config.NotifyConfig{Mode: "off", FocusMode: "focused"}, + }) m, _ = m.handleNotifyCommand("off") + if m.notifyFocusMode != "focused" { + t.Errorf("live focus = %q, want the in-session focused preserved", m.notifyFocusMode) + } persisted = readNotifyBlock(t, blankPath) if persisted.Mode != "off" { t.Errorf("persisted mode = %q, want off", persisted.Mode) @@ -92,6 +112,97 @@ func TestNotifyModeOnlyPreservesStoredFocusNotResolved(t *testing.T) { } } +// The full contract matrix when the three notify states disagree (blank/different +// user file + project-resolved pair), per the maintainer review: display and +// preselection report the LIVE policy; partial updates change live and persist +// independently; restart precedence never copies project values into the +// global file. +func TestNotifyLivePolicyContractMatrix(t *testing.T) { + cfgPath := filepath.Join(t.TempDir(), "config.json") + // User file: nothing stored at all. + if err := os.WriteFile(cfgPath, []byte(`{}`), 0o600); err != nil { + t.Fatal(err) + } + // Project-resolved pair: off/focused (a project override shape). + m := newModel(context.Background(), Options{ + UserConfigPath: cfgPath, + Notify: config.NotifyConfig{Mode: "off", FocusMode: "focused"}, + }) + + // `/notify list` reports the live resolved pair, not the (blank) file. + state := m.notifyStateText() + if !strings.Contains(state, "active mode: off") || !strings.Contains(state, "active focus: focused") { + t.Errorf("list should report the live off/focused pair, got:\n%s", state) + } + + // The picker preselects the live pair; Enter without navigation does not + // change it. + m.input.SetValue("/notify") + updated, cmd := m.Update(testKey(tea.KeyEnter)) + if cmd != nil { + t.Fatalf("opening the picker should not emit a cmd, got %T", cmd) + } + m = updated.(model) + if sel := m.picker.items[m.picker.selected]; sel.Value != "off focused" { + t.Fatalf("preselected = %q, want the live pair %q", sel.Value, "off focused") + } + updated, _ = m.Update(testKey(tea.KeyEnter)) + m = updated.(model) + if m.notifyMode != "off" || m.notifyFocusMode != "focused" { + t.Errorf("Enter on preselected live pair changed live state: %q/%q", m.notifyMode, m.notifyFocusMode) + } + persisted := readNotifyBlock(t, cfgPath) + if persisted.Mode != "off" || persisted.FocusMode != "focused" { + t.Errorf("Enter on the live pair persisted %q/%q, want off/focused (explicit row commit)", persisted.Mode, persisted.FocusMode) + } + + // Reset the file to blank and re-seed the session for the partial-update + // checks (fresh states: blank file, live off/focused). + if err := os.WriteFile(cfgPath, []byte(`{}`), 0o600); err != nil { + t.Fatal(err) + } + m = newModel(context.Background(), Options{ + UserConfigPath: cfgPath, + Notify: config.NotifyConfig{Mode: "off", FocusMode: "focused"}, + }) + + // `/notify bell` (mode-only): live becomes bell/focused; the file gets + // mode bell and keeps its focus BLANK — the project's focused is not + // copied into the global file. + m, _ = m.handleNotifyCommand("bell") + if m.notifyMode != "bell" || m.notifyFocusMode != "focused" { + t.Errorf("after mode-only: live = %q/%q, want bell/focused", m.notifyMode, m.notifyFocusMode) + } + persisted = readNotifyBlock(t, cfgPath) + if persisted.Mode != "bell" { + t.Errorf("after mode-only: persisted mode = %q, want bell", persisted.Mode) + } + if persisted.FocusMode != "" { + t.Errorf("after mode-only: persisted focusMode = %q, want blank (project value must not leak)", persisted.FocusMode) + } + + // `/notify bell always` (explicit pair): both live fields change and both + // explicit values persist. + m, _ = m.handleNotifyCommand("bell always") + if m.notifyMode != "bell" || m.notifyFocusMode != "always" { + t.Errorf("after explicit pair: live = %q/%q, want bell/always", m.notifyMode, m.notifyFocusMode) + } + persisted = readNotifyBlock(t, cfgPath) + if persisted.Mode != "bell" || persisted.FocusMode != "always" { + t.Errorf("after explicit pair: persisted = %q/%q, want bell/always", persisted.Mode, persisted.FocusMode) + } + + // Restart precedence: a fresh resolve of the user file (now bell/always, + // no project) yields exactly the stored pair — nothing else leaked in. + resolved, err := config.Resolve(config.ResolveOptions{UserConfigPath: cfgPath, Env: map[string]string{}}) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if resolved.Notify.Mode != "bell" || resolved.Notify.FocusMode != "always" { + t.Errorf("restart resolve = %q/%q, want bell/always", resolved.Notify.Mode, resolved.Notify.FocusMode) + } +} + // `/notify bell unfocused` updates both fields in one call. func TestNotifyCommandSetsModeAndFocus(t *testing.T) { m := newModel(context.Background(), Options{}) @@ -139,6 +250,28 @@ func TestNotifyCommandRejectsTrailingArguments(t *testing.T) { } } +// `/notify list junk` is a usage error, not a successful list — matching +// /theme, /effort, and /style, which recognize only an exact `list` +// (maintainer review, PR #1001). Exact `/notify list` still shows state. +func TestNotifyCommandListRequiresExactlyOneToken(t *testing.T) { + m := newModel(context.Background(), Options{}) + m.notifyMode = "off" + m.notifyFocusMode = "focused" + + m, out := m.handleNotifyCommand("list junk") + if !strings.Contains(out, "Too many arguments") { + t.Errorf("`list junk` should be a usage error, got: %s", out) + } + if m.notifyMode != "off" || m.notifyFocusMode != "focused" { + t.Errorf("`list junk` should not mutate state, got %q/%q", m.notifyMode, m.notifyFocusMode) + } + + _, out = m.handleNotifyCommand("list") + if !strings.Contains(out, "active mode: off") || !strings.Contains(out, "active focus: focused") { + t.Errorf("exact `/notify list` should still show the live state, got: %s", out) + } +} + // `/notify loud` (invalid) returns an error message; the model's notifyMode // is NOT mutated, so a typo cannot accidentally turn the alert off. func TestNotifyCommandRejectsInvalidMode(t *testing.T) { @@ -260,20 +393,25 @@ func TestNotifyPickerValuesAreValidCommandArgs(t *testing.T) { } } -// The /notify state view shows the stored mode and focus (and labels a blank -// focus as the default) so users see the real value before opening the picker. -func TestNotifyStateTextShowsStoredPair(t *testing.T) { - cfgPath := filepath.Join(t.TempDir(), "config.json") - if err := os.WriteFile(cfgPath, []byte(`{"notify":{"mode":"bell","focusMode":"always"}}`), 0o600); err != nil { - t.Fatal(err) - } - m := newModel(context.Background(), Options{UserConfigPath: cfgPath}) +// The /notify state view reports the LIVE session policy (initialized from the +// resolved pair), not the stored file — the two can legitimately disagree when +// a project config overrides notify (maintainer review, PR #1001). A blank +// live focus renders as the unfocused default. +func TestNotifyStateTextShowsLivePair(t *testing.T) { + m := newModel(context.Background(), Options{Notify: config.NotifyConfig{Mode: "bell", FocusMode: "always"}}) state := m.notifyStateText() if !strings.Contains(state, "active mode: bell") { - t.Errorf("state should show stored mode, got: %s", state) + t.Errorf("state should show the live mode, got: %s", state) } if !strings.Contains(state, "active focus: always") { - t.Errorf("state should show stored focus, got: %s", state) + t.Errorf("state should show the live focus, got: %s", state) + } + + // Blank live focus renders as the default, never an empty string. + m = newModel(context.Background(), Options{Notify: config.NotifyConfig{Mode: "bell"}}) + state = m.notifyStateText() + if !strings.Contains(state, "active focus: unfocused (default)") { + t.Errorf("state should label a blank live focus as the default, got: %s", state) } } diff --git a/internal/tui/picker.go b/internal/tui/picker.go index 1a72b2656..4727e3c11 100644 --- a/internal/tui/picker.go +++ b/internal/tui/picker.go @@ -1069,21 +1069,26 @@ func (m model) newThemePicker() *commandPicker { } // newNotifyPicker lists the FULL (mode, focus) space from notifyPickerChoices -// (every valid pair has a row, so a stored setting like (off, always) is always +// (every valid pair has a row, so a setting like (off, always) is always // preselectable and Enter can never silently commit a different pair). Each // row's Value is the same synthetic string the text /notify handler accepts // (" "), so /notify with no arg and the picker share one commit -// path through handleNotifyCommand. A blank stored field resolves to its -// effective default for preselection only (both / unfocused — what actually -// fires today); committing any row writes an explicit pair. There is no live -// preview — notify affects the next permission prompt, not the current view. +// path through handleNotifyCommand. Preselection uses the LIVE session pair +// (m.notifyMode/m.notifyFocusMode, initialized from the resolved config) — not +// the stored file, which can disagree when a project config overrides notify +// for this session. Blank live fields resolve to their effective defaults for +// preselection only; committing a row is an explicit choice and persists that +// pair. There is no live preview — notify affects the next notification, not +// the current view. func (m model) newNotifyPicker() *commandPicker { choices := notifyPickerChoices() items := make([]pickerItem, 0, len(choices)) selected := 0 - stored, _ := m.storedNotify() - activeMode := string(effectiveTUINotifyMode(stored.Mode)) - activeFocus := stored.FocusMode + activeMode := m.notifyMode + if strings.TrimSpace(activeMode) == "" { + activeMode = string(notify.ModeBoth) + } + activeFocus := m.notifyFocusMode if strings.TrimSpace(activeFocus) == "" { activeFocus = string(notify.FocusUnfocused) } From 0716573f1574441c29af7530b91b78ea68ba37bd Mon Sep 17 00:00:00 2001 From: Gaurav Bhatia Date: Sun, 6 Sep 2026 15:07:06 +0400 Subject: [PATCH 7/8] fix(notify): close the remaining review findings (isolation, races, fidelity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the third review round (PR #1001) as one boundary pass: - BTW isolation: /notify mutations and the picker are now behind the existing btwCommandUnavailable guard (like /theme), because the side conversation shares the parent's notifier pointer — an unblocked /notify off inside BTW reconfigured the parent and wrote the global file while only the side's display changed. /notify list stays available. Regression: side-conversation attempt then return, checking the parent's actual notification output and the stored preference. - Serialization: new UpdateNotify runs read-merge-write as ONE transaction under a cross-process advisory lock (flock/LockFileEx on a .lock file beside the config, credstore's pattern), so two concurrent partial updates (--mode off vs --focus always) cannot interleave and silently undo each other's explicit change. The CLI and the TUI's mode-only persistence both go through it; the TUI's stale pre-read of the stored focus is gone (the merge reads under the lock). Verified 0/30 lost updates driving the real binary with two concurrent processes per round; a goroutine-level regression test asserts both explicit fields survive. - Fidelity: notification writes now edit ONLY the notify member's bytes (setNotifyJSONObject, on the existing byte-preserving JSON editor), so unrelated values survive with their explicit presence intact — including tools.deferThreshold: 0 and MCP disabled: false, which the typed serializer's omitempty cannot round-trip, and keys FileConfig does not model at all. A reset removes the notify member instead of leaving a {} husk. - Startup recovery: Resolve's ErrNoActiveProvider partial result now carries the parsed non-provider fields (notify, sandbox, tools, ...), and the setup-recovery branch that clears the resolved config keeps the notify block — a stored explicit mode:off no longer resurrects alerts through the provider-recovery path. Regression: stale activeProvider + stored off/always through recovery asserts the launched TUI receives the opt-out, not the both/unfocused default. - docs nits: zero config --help now describes both alert classes; the two remaining "resolver default" wordings updated. lint-static: 0 issues. Affected packages green under -race (the only local failure is the known TestRunAuthOpenRouterSavesMintedKey macOS keychain flake, which fails identically on clean origin/main here). --- internal/cli/app.go | 8 +- internal/cli/app_test.go | 67 +++++++++++++++++ internal/cli/command_center.go | 3 +- internal/cli/config_notify.go | 31 ++++---- internal/cli/config_notify_test.go | 5 +- internal/config/filelock_unix.go | 54 +++++++++++++ internal/config/filelock_windows.go | 54 +++++++++++++ internal/config/json_object_edit.go | 33 ++++++++ internal/config/resolver.go | 22 +++++- internal/config/resolver_test.go | 9 ++- internal/config/writer.go | 104 +++++++++++++++++++------ internal/config/writer_test.go | 113 ++++++++++++++++++++++++++++ internal/tui/btw.go | 7 ++ internal/tui/btw_test.go | 62 +++++++++++++++ internal/tui/model_test.go | 8 +- internal/tui/notify_select.go | 46 ++++++----- 16 files changed, 554 insertions(+), 72 deletions(-) create mode 100644 internal/config/filelock_unix.go create mode 100644 internal/config/filelock_windows.go diff --git a/internal/cli/app.go b/internal/cli/app.go index 44fa370ff..c2eeb207b 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -734,7 +734,13 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a resolved.Provider = usable resolved.ActiveProvider = usable.Name } else { - resolved = config.ResolvedConfig{} + // Fresh-onboarding reset, but the user's notification preference is + // NOT config-to-redo: clearing it here would surface as an empty + // Options.Notify, and the TUI's unconfigured default (both/unfocused) + // would then resurrect alerts a user explicitly turned off while the + // setup wizard runs (maintainer review, PR #1001). Carry the stored + // block through; other wizard-visible state starts clean as before. + resolved = config.ResolvedConfig{Notify: resolved.Notify} forceSetup = true } } diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index 1c12da4c8..43cef82bc 100644 --- a/internal/cli/app_test.go +++ b/internal/cli/app_test.go @@ -377,6 +377,73 @@ func TestRunNoArgsFallsBackToUsableProviderWhenNoneMarkedActive(t *testing.T) { } } +// Maintainer regression (PR #1001): a stored explicit notify opt-out must +// survive the provider-recovery startup path. With a stale activeProvider and +// another usable saved provider, Resolve returns the partial config alongside +// ErrNoActiveProvider and the recovery branch forwards it — dropping the +// notify policy there would surface an empty Options.Notify, and the TUI's +// unconfigured default (both) would resurrect alerts the user explicitly +// turned off. The same recovery branch that clears the resolved config for +// the setup wizard carries the stored notify block through. +func TestRunNoArgsPreservesNotifyOptOutThroughProviderRecovery(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + cwd := t.TempDir() + setCLIUserConfigRoot(t) + userConfigPath := filepath.Join(t.TempDir(), "zero", "config.json") + var launchedOptions tui.Options + launched := false + + usable := config.ProviderProfile{ + Name: "work", + ProviderKind: config.ProviderKindOpenAI, + BaseURL: config.OpenAIBaseURL, + APIKey: "sk-test", + Model: "gpt-test", + } + + exitCode := runWithDeps([]string{}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { + return cwd, nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + // Stale activeProvider + usable saved provider, and the user's + // config.json explicitly opts out of notifications. The fixed + // resolver carries the parsed notify block through the error path. + return config.ResolvedConfig{ + Providers: []config.ProviderProfile{usable}, + Notify: config.NotifyConfig{Mode: "off", FocusMode: "always"}, + }, fmt.Errorf("%w: active provider %q not found", config.ErrNoActiveProvider, "ghost") + }, + newProvider: func(profile config.ProviderProfile) (zeroruntime.Provider, error) { + return &cliFakeProvider{}, nil + }, + userConfigPath: func() (string, error) { + return userConfigPath, nil + }, + registerMCPTools: func(context.Context, *tools.Registry, config.MCPConfig, mcp.RegisterOptions) (mcpToolRuntime, error) { + return noopMCPRuntime{}, nil + }, + runTUI: func(ctx context.Context, options tui.Options) int { + launched = true + launchedOptions = options + return 0 + }, + }) + + if exitCode != 0 { + t.Fatalf("exit code = %d, want 0, stderr=%q", exitCode, stderr.String()) + } + if !launched { + t.Fatal("TUI was not launched") + } + // The explicit opt-out reaches the TUI intact — NOT the both/unfocused + // unconfigured default. + if launchedOptions.Notify.Mode != "off" || launchedOptions.Notify.FocusMode != "always" { + t.Fatalf("Options.Notify = %+v, want the stored off/always opt-out preserved through recovery", launchedOptions.Notify) + } +} + func TestRunNoArgsFailsWhenResolveErrorIsNotProviderRelated(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer diff --git a/internal/cli/command_center.go b/internal/cli/command_center.go index 393742f36..fbf22b6f9 100644 --- a/internal/cli/command_center.go +++ b/internal/cli/command_center.go @@ -489,7 +489,8 @@ func writeConfigHelp(w io.Writer) error { zero config notify [flags] Inspects resolved Go configuration without printing secrets. The notify -subcommand reads or updates the permission-prompt alert preference — +subcommand reads or updates the stored global notification preference, +which controls both the completion and needs-input alerts — run "zero config notify --help" for details. Flags: diff --git a/internal/cli/config_notify.go b/internal/cli/config_notify.go index 07c4a7e48..9caac13b4 100644 --- a/internal/cli/config_notify.go +++ b/internal/cli/config_notify.go @@ -40,25 +40,26 @@ func runConfigNotify(args []string, stdout io.Writer, stderr io.Writer, deps app } if options.mode != "" || options.focus != "" || options.reset { - // Seed omitted fields from the USER'S OWN file. Blank stays blank — - // blank means "use the built-in defaults"; --reset is the only path - // that clears both fields. - current, err := config.UserNotify(configPath) - if err != nil { - return writeAppError(stderr, err.Error(), exitUsage) - } - notify := current - if options.reset { - notify = config.NotifyConfig{} - } else { + // One serialized read-merge-write transaction: the lock covers + // reading the stored block, applying only the explicit fields, and + // replacing the file, so two concurrent partial updates (e.g. + // --mode off and --focus always from two terminals) cannot lose + // each other's change (maintainer review, PR #1001). Omitted + // fields preserve the values stored in the user's OWN file — blank + // stays blank; --reset is the only path that clears both fields. + _, err := config.UpdateNotify(configPath, func(current config.NotifyConfig) config.NotifyConfig { + if options.reset { + return config.NotifyConfig{} + } if options.mode != "" { - notify.Mode = options.mode + current.Mode = options.mode } if options.focus != "" { - notify.FocusMode = options.focus + current.FocusMode = options.focus } - } - if _, err := config.SetNotify(configPath, notify); err != nil { + return current + }) + if err != nil { return writeAppError(stderr, err.Error(), exitUsage) } } diff --git a/internal/cli/config_notify_test.go b/internal/cli/config_notify_test.go index 0b3e868a1..bcd56141c 100644 --- a/internal/cli/config_notify_test.go +++ b/internal/cli/config_notify_test.go @@ -363,8 +363,9 @@ func TestRunConfigNotifyRejectsInvalidMode(t *testing.T) { } } -// `--reset` blanks both fields so the resolver defaults apply on the next -// resolve. Useful for "go back to the recommended setup" after a custom value. +// `--reset` blanks both fields so the TUI's effective default applies again +// (an unconfigured headless run stays silent). Useful for "go back to the +// recommended setup" after a custom value. func TestRunConfigNotifyResetClearsStoredValues(t *testing.T) { configPath := filepath.Join(t.TempDir(), "config.json") seed := `{ diff --git a/internal/config/filelock_unix.go b/internal/config/filelock_unix.go new file mode 100644 index 000000000..09043fe48 --- /dev/null +++ b/internal/config/filelock_unix.go @@ -0,0 +1,54 @@ +//go:build !windows + +package config + +import ( + "errors" + "fmt" + "os" + "path/filepath" + + "golang.org/x/sys/unix" +) + +// acquireConfigLock takes an exclusive advisory lock (flock) on a lock file +// SEPARATE from the config file, serializing config read-modify-write +// transactions across processes AND goroutines (flock is held per open file +// description, so two opens in one process contend exactly as two processes +// do). The lock file is never renamed or removed: writeConfigData publishes +// via os.Rename, which replaces the data file's inode, so a lock taken on the +// data file itself would attach to an inode the next writer has already +// replaced and every writer would appear to hold it. Mirrors credstore's +// acquireFileLock, which documents the same invariant for the same reason. +// +// Blocking, not try-lock: a caller mid-transaction is expected to finish in +// milliseconds, and a failed `zero config notify --mode off` because a +// concurrent write held the lock would be worse than waiting briefly. +func acquireConfigLock(configPath string) (func() error, error) { + dir := filepath.Dir(configPath) + if dir == "" { + dir = "." + } + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("create config directory %s: %w", dir, err) + } + file, err := os.OpenFile(configPath+".lock", os.O_RDWR|os.O_CREATE, 0o600) + if err != nil { + return nil, fmt.Errorf("open config lock: %w", err) + } + if err := unix.Flock(int(file.Fd()), unix.LOCK_EX); err != nil { + _ = file.Close() + return nil, fmt.Errorf("lock config: %w", err) + } + return func() error { + // Close alone drops the flock; the explicit unlock is belt-and-braces, + // and its failure is reported rather than swallowed so a cleanup that + // did not complete is distinguishable from one that did. + unlockErr := unix.Flock(int(file.Fd()), unix.LOCK_UN) + closeErr := file.Close() + if err := errors.Join(unlockErr, closeErr); err != nil { + return fmt.Errorf("release config lock: %w", err) + } + return nil + }, nil +} diff --git a/internal/config/filelock_windows.go b/internal/config/filelock_windows.go new file mode 100644 index 000000000..cfc43dc8b --- /dev/null +++ b/internal/config/filelock_windows.go @@ -0,0 +1,54 @@ +//go:build windows + +package config + +import ( + "errors" + "fmt" + "os" + "path/filepath" + + "golang.org/x/sys/windows" +) + +// acquireConfigLock takes an exclusive OS lock (LockFileEx) serializing config +// read-modify-write transactions, matching the flock behaviour on unix. The +// lock file is SEPARATE from the data file because writeConfigData publishes +// via rename, and a lock on the renamed file would attach to an inode the next +// writer has already replaced. Mirrors credstore's acquireFileLock, which +// documents the same invariant for the same reason. Blocking (no +// LOCKFILE_FAIL_IMMEDIATELY) so a concurrent writer queues rather than failing +// the operation. +func acquireConfigLock(configPath string) (func() error, error) { + dir := filepath.Dir(configPath) + if dir == "" { + dir = "." + } + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("create config directory %s: %w", dir, err) + } + file, err := os.OpenFile(configPath+".lock", os.O_RDWR|os.O_CREATE, 0o600) + if err != nil { + return nil, fmt.Errorf("open config lock: %w", err) + } + handle := windows.Handle(file.Fd()) + overlapped := new(windows.Overlapped) + // Fixed 1-byte region, exclusive, blocking: a waiter queues rather than + // failing its config write. + flags := uint32(windows.LOCKFILE_EXCLUSIVE_LOCK) + if err := windows.LockFileEx(handle, flags, 0, 1, 0, overlapped); err != nil { + _ = file.Close() + return nil, fmt.Errorf("lock config: %w", err) + } + return func() error { + // Reported rather than swallowed: a release that did not complete must + // not look identical to one that did, and on Windows the handle staying + // open is what blocks the next writer's rename. + unlockErr := windows.UnlockFileEx(handle, 0, 1, 0, overlapped) + closeErr := file.Close() + if err := errors.Join(unlockErr, closeErr); err != nil { + return fmt.Errorf("release config lock: %w", err) + } + return nil + }, nil +} diff --git a/internal/config/json_object_edit.go b/internal/config/json_object_edit.go index 438d1ff20..9622696f7 100644 --- a/internal/config/json_object_edit.go +++ b/internal/config/json_object_edit.go @@ -244,6 +244,39 @@ func petJSONObject(encodedPet []byte) []byte { return append(result, '}') } +// setNotifyJSONObject replaces only the notify member's value, keeping the +// original bytes of every unrelated member so a notification save cannot +// reorder, reformat, or drop the user's other settings — including values whose +// EXPLICIT presence matters (tools.deferThreshold: 0, mcp servers' +// disabled: false) and keys the typed FileConfig does not model at all. The +// typed serializer cannot round-trip explicit zeros (omitempty drops them), so +// notification writes go through this byte-preserving editor instead +// (maintainer review, PR #1001). An empty notify block removes the member +// entirely so a reset leaves no `"notify": {}` husk behind. +func setNotifyJSONObject(data []byte, notify NotifyConfig) ([]byte, error) { + encoded, err := json.Marshal(notify) + if err != nil { + return nil, fmt.Errorf("encode notify preference: %w", err) + } + + rootStart := skipJSONSpace(data, 0) + root, err := parseJSONObject(data, rootStart) + if err != nil { + return nil, err + } + notifyIndex := lastJSONMember(root.members, "notify") + if string(encoded) == "{}" { + if notifyIndex < 0 { + return data, nil + } + return removeJSONMember(data, root, notifyIndex), nil + } + if notifyIndex < 0 { + return insertJSONMember(data, root, "notify", encoded), nil + } + return replaceJSONRange(data, root.members[notifyIndex].valueStart, root.members[notifyIndex].valueEnd, encoded), nil +} + func replaceJSONRange(data []byte, start, end int, replacement []byte) []byte { result := make([]byte, 0, len(data)-(end-start)+len(replacement)) result = append(result, data[:start]...) diff --git a/internal/config/resolver.go b/internal/config/resolver.go index 16936874d..8276f54c4 100644 --- a/internal/config/resolver.go +++ b/internal/config/resolver.go @@ -157,8 +157,26 @@ func Resolve(options ResolveOptions) (ResolvedConfig, error) { // On ErrNoActiveProvider, providers may still hold the successfully // normalized (but active-less) profile list — keep it so a caller can fall // back to an already-configured usable provider instead of treating this - // like a config with nothing set up at all. - return ResolvedConfig{Providers: providers}, err + // like a config with nothing set up at all. The non-provider fields parsed + // before the failure (notify, sandbox, tools, preferences, ...) are kept + // too: the TUI's provider-recovery path forwards this partial config, and + // dropping them would let the TUI's unconfigured notify default overwrite + // a stored explicit opt-out on startup (maintainer review, PR #1001). + partial := ResolvedConfig{ + Providers: providers, + MaxTurns: cfg.MaxTurns, + MCP: cfg.MCP, + Sandbox: cfg.Sandbox, + Notify: cfg.Notify, + Tools: cfg.Tools, + Swarm: cfg.Swarm, + Preferences: cfg.Preferences, + KeyBindings: cfg.KeyBindings, + LocalControl: cfg.LocalControl, + STT: cfg.STT, + CrossSessionInbound: cfg.CrossSessionInbound, + } + return partial, err } return ResolvedConfig{ diff --git a/internal/config/resolver_test.go b/internal/config/resolver_test.go index 6de99934b..e193e5b9f 100644 --- a/internal/config/resolver_test.go +++ b/internal/config/resolver_test.go @@ -1151,8 +1151,13 @@ func TestResolveRejectsActiveProviderWithoutConfiguredProfiles(t *testing.T) { if HasProviderProfile(resolved.Provider) { t.Fatalf("Provider = %#v, want zero value", resolved.Provider) } - if resolved.MaxTurns != 0 { - t.Fatalf("MaxTurns = %d, want zero on failed resolve", resolved.MaxTurns) + // The error path now carries the parsed non-provider fields through + // (maintainer review, PR #1001): the TUI's provider-recovery startup + // forwards this partial config, and a zero MaxTurns there would silently + // discard the user's turn budget. Matches the no-providers SUCCESS path, + // which has always defaulted MaxTurns. + if resolved.MaxTurns != defaultMaxTurns { + t.Fatalf("MaxTurns = %d, want default %d carried through the partial resolve", resolved.MaxTurns, defaultMaxTurns) } } diff --git a/internal/config/writer.go b/internal/config/writer.go index 20724e839..edf89de67 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -620,46 +620,106 @@ func UserNotify(path string) (NotifyConfig, error) { return cfg.Notify, nil } -// SetNotify persists the TUI notification preference. Both fields are trimmed -// and validated against the accepted vocab (mode in {off,bell,notify,both}; -// focusMode in {unfocused,always,focused}) so a bad caller cannot write a value -// the resolver would later reject at startup. An empty Mode or FocusMode is -// stored as-is — blank means "use the built-in defaults" (the TUI's -// effectiveTUINotifyMode maps an empty mode to both; the notify package treats -// an empty focusMode as unfocused), not "off". -func SetNotify(path string, value NotifyConfig) (FileConfig, error) { - path = strings.TrimSpace(path) - if path == "" { - return FileConfig{}, fmt.Errorf("config path is required") - } +// validateNotify trims both fields and checks them against the accepted vocab +// (mode in {off,bell,notify,both}; focusMode in {unfocused,always,focused}) so +// a bad caller cannot write a value the resolver would later reject at +// startup. An empty Mode or FocusMode is stored as-is — blank means "use the +// built-in defaults" (the TUI's effectiveTUINotifyMode maps an empty mode to +// both; the notify package treats an empty focusMode as unfocused), not "off". +func validateNotify(value NotifyConfig) (NotifyConfig, error) { value.Mode = strings.TrimSpace(value.Mode) value.FocusMode = strings.TrimSpace(value.FocusMode) if mode := value.Mode; mode != "" { switch notify.Mode(mode) { case notify.ModeOff, notify.ModeBell, notify.ModeNotify, notify.ModeBoth: default: - return FileConfig{}, fmt.Errorf("invalid notify.mode %q: expected off, bell, notify, or both", mode) + return NotifyConfig{}, fmt.Errorf("invalid notify.mode %q: expected off, bell, notify, or both", mode) } } if focus := value.FocusMode; focus != "" { switch notify.FocusMode(focus) { case notify.FocusUnfocused, notify.FocusAlways, notify.FocusFocused: default: - return FileConfig{}, fmt.Errorf("invalid notify.focusMode %q: expected unfocused, always, or focused", focus) + return NotifyConfig{}, fmt.Errorf("invalid notify.focusMode %q: expected unfocused, always, or focused", focus) } } - cfg := FileConfig{} - if data, err := os.ReadFile(path); err == nil { - if err := json.Unmarshal(data, &cfg); err != nil { - return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) + return value, nil +} + +// UpdateNotify runs one SERIALIZED read-modify-write transaction on the user's +// notify block. merge receives the currently stored block and returns the +// block to store; it must preserve any field the caller is not explicitly +// changing. The lock covers reading the stored block, applying merge, and +// replacing the file, so two concurrent partial updates (for example +// `--mode off` and `--focus always` from two terminals) cannot interleave +// their reads and writes and silently undo each other's explicit change +// (maintainer review, PR #1001 reproduced a 50/50 lost-update without it). +// +// The write replaces ONLY the notify member's bytes (setNotifyJSONObject), +// preserving every unrelated value and its explicit presence — including +// explicit zeros/falses like tools.deferThreshold: 0 or an MCP server's +// disabled: false, which the typed serializer's omitempty cannot round-trip — +// through the existing temp-file-and-rename atomic publish. Locking only the +// final write would leave the stale-field merge outside the transaction, so +// the whole sequence runs under one lock. +func UpdateNotify(path string, merge func(current NotifyConfig) NotifyConfig) (NotifyConfig, error) { + path = strings.TrimSpace(path) + if path == "" { + return NotifyConfig{}, fmt.Errorf("config path is required") + } + if merge == nil { + return NotifyConfig{}, fmt.Errorf("merge function is required") + } + release, err := acquireConfigLock(path) + if err != nil { + return NotifyConfig{}, err + } + defer func() { _ = release() }() + + current, err := UserNotify(path) + if err != nil { + return NotifyConfig{}, err + } + next, err := validateNotify(merge(current)) + if err != nil { + return NotifyConfig{}, err + } + + // Read the raw file and edit only the notify member. A missing file starts + // from an empty object so a first-time write does not need a pre-seeded + // config. + data, err := os.ReadFile(path) + if err != nil { + if !os.IsNotExist(err) { + return NotifyConfig{}, fmt.Errorf("read config %s: %w", path, err) } - } else if !os.IsNotExist(err) { - return FileConfig{}, fmt.Errorf("read config %s: %w", path, err) + data = []byte("{}") } - cfg.Notify = value - if err := writeConfigFile(path, cfg); err != nil { + updated, err := setNotifyJSONObject(data, next) + if err != nil { + return NotifyConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + if err := writeConfigData(path, updated); err != nil { + return NotifyConfig{}, err + } + return next, nil +} + +// SetNotify replaces the stored notification preference with value. It is a +// thin wrapper over the transactional UpdateNotify so existing callers keep +// their replace semantics under the same lock and byte-preserving write. +func SetNotify(path string, value NotifyConfig) (FileConfig, error) { + next, err := UpdateNotify(path, func(NotifyConfig) NotifyConfig { return value }) + if err != nil { + return FileConfig{}, err + } + // Re-read the full config so callers that inspect unrelated fields (and + // existing tests) keep their FileConfig-shaped result. + cfg, err := loadConfigFile(path) + if err != nil { return FileConfig{}, err } + cfg.Notify = next return cfg, nil } diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 69a06a488..311ba6e6b 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -3,6 +3,7 @@ package config import ( "encoding/json" "errors" + "fmt" "io/fs" "os" "os/exec" @@ -10,6 +11,7 @@ import ( "reflect" "runtime" "strings" + "sync" "testing" ) @@ -431,6 +433,117 @@ func TestSetNotifyBlankValuesPreservedAsDefaults(t *testing.T) { } } +// Maintainer regression (PR #1001): a notification save must preserve unrelated +// values whose EXPLICIT presence matters. The typed serializer's omitempty +// cannot round-trip tools.deferThreshold: 0 (explicit "never defer" vs unset +// "use default 3") or an MCP server's disabled: false (explicitly enabled), so +// SetNotify/UpdateNotify edit only the notify member's bytes instead. The +// assertions run against the RAW file, because reading through FileConfig +// would hide exactly the presence loss being tested. +func TestSetNotifyPreservesExplicitUnrelatedValues(t *testing.T) { + dir := t.TempDir() + + for name, body := range map[string]string{ + "explicit zero deferThreshold": `{ + "tools": {"deferThreshold": 0}, + "mcp": {"servers": {"firecrawl": {"command": "npx", "disabled": false}}} +}`, + "unknown keys stay untouched": `{ + "activeProvider": "openai", + "customTopLevel": {"nested": [1, 2, 3]}, + "notify": {"mode": "both"} +}`, + } { + t.Run(name, func(t *testing.T) { + path := filepath.Join(dir, name+".json") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + if _, err := SetNotify(path, NotifyConfig{Mode: "off", FocusMode: "always"}); err != nil { + t.Fatalf("SetNotify: %v", err) + } + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var obj map[string]json.RawMessage + if err := json.Unmarshal(raw, &obj); err != nil { + t.Fatalf("decode: %v", err) + } + if strings.Contains(name, "explicit zero") { + tools, ok := obj["tools"] + if !ok || !strings.Contains(string(tools), `"deferThreshold": 0`) { + t.Errorf("tools.deferThreshold: 0 lost through a notify write; tools = %s", string(tools)) + } + mcp, ok := obj["mcp"] + if !ok || !strings.Contains(string(mcp), `"disabled": false`) { + t.Errorf("mcp disabled: false lost through a notify write; mcp = %s", string(mcp)) + } + } + if strings.Contains(name, "unknown keys") { + if _, ok := obj["customTopLevel"]; !ok { + t.Error("unknown top-level key lost through a notify write") + } + } + }) + } +} + +// Maintainer regression (PR #1001): concurrent partial updates must not lose +// each other's explicit change. Two `zero config notify` calls (--mode off and +// --focus always) racing from both/unfocused previously interleaved their +// read-merge-write and one write silently undid the other (reproduced 50/50 +// without the lock). UpdateNotify serializes the whole transaction. +func TestUpdateNotifyConcurrentPartialUpdatesLoseNoField(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + if err := os.WriteFile(path, []byte(`{"notify":{"mode":"both","focusMode":"unfocused"}}`), 0o600); err != nil { + t.Fatal(err) + } + + var wg sync.WaitGroup + errs := make(chan error, 2) + wg.Add(2) + go func() { + defer wg.Done() + if _, err := UpdateNotify(path, func(current NotifyConfig) NotifyConfig { + current.Mode = "off" + return current + }); err != nil { + errs <- fmt.Errorf("mode update: %w", err) + } + }() + go func() { + defer wg.Done() + if _, err := UpdateNotify(path, func(current NotifyConfig) NotifyConfig { + current.FocusMode = "always" + return current + }); err != nil { + errs <- fmt.Errorf("focus update: %w", err) + } + }() + wg.Wait() + close(errs) + for err := range errs { + t.Fatalf("concurrent update failed: %v", err) + } + + stored, err := UserNotify(path) + if err != nil { + t.Fatalf("read stored: %v", err) + } + // BOTH explicit changes must survive: mode from writer 1, focus from + // writer 2. The old race let the second writer's stale focus + // ("unfocused") or stale mode ("both") win. + if stored.Mode != "off" { + t.Errorf("mode = %q, want off (concurrent focus update must not clobber it)", stored.Mode) + } + if stored.FocusMode != "always" { + t.Errorf("focusMode = %q, want always (concurrent mode update must not clobber it)", stored.FocusMode) + } +} + // UserNotify reads the notify block from the user's own file. Partial updates // seed from this value so they preserve what the USER chose (blank included) // instead of copying a project config's setting or a pinned default into the diff --git a/internal/tui/btw.go b/internal/tui/btw.go index 109b76bf6..7ccffa95b 100644 --- a/internal/tui/btw.go +++ b/internal/tui/btw.go @@ -218,6 +218,13 @@ func btwCommandUnavailable(command parsedCommand) bool { return arg != "" && arg != "status" case commandTheme: return arg != "list" + case commandNotify: + // Mutations reconfigure the shared notifier pointer and write the + // global preference; inside a BTW side conversation that would mutate + // the hidden parent's live policy and the user's global file while + // only the side surface's display fields change. Read-only list stays + // available (maintainer review, PR #1001). + return arg != "list" case commandConfig: return arg != "" default: diff --git a/internal/tui/btw_test.go b/internal/tui/btw_test.go index 0d2b958a7..d5333d646 100644 --- a/internal/tui/btw_test.go +++ b/internal/tui/btw_test.go @@ -1,6 +1,7 @@ package tui import ( + "bytes" "context" "os" "path/filepath" @@ -9,6 +10,8 @@ import ( tea "charm.land/bubbletea/v2" + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/notify" "github.com/Gitlawb/zero/internal/sessions" ) @@ -241,6 +244,12 @@ func TestBTWBlocksPersistentConfigurationCommands(t *testing.T) { "/mcp", "/rewind", "/compact", + // Notify mutations reconfigure the parent's notifier (shared pointer) + // and write the global preference; they must stay blocked in a BTW + // side conversation (maintainer review, PR #1001). + "/notify", + "/notify off", + "/notify bell always", } { t.Run(input, func(t *testing.T) { m := newBTWTestModel(t) @@ -265,6 +274,7 @@ func TestBTWAllowsReadOnlyConfigurationCommands(t *testing.T) { "/profile status", "/theme list", "/config", + "/notify list", } { t.Run(input, func(t *testing.T) { m := newBTWTestModel(t) @@ -278,6 +288,58 @@ func TestBTWAllowsReadOnlyConfigurationCommands(t *testing.T) { } } +// Maintainer regression (PR #1001): running notify mutations inside a BTW +// side conversation must not touch the PARENT's live notifier or the stored +// preference. The side surface shallow-copies the parent model and shares its +// notifier pointer, so an unblocked mutation would reconfigure the parent and +// write the global file while only the side's display fields change; after +// returning, the parent could report one policy in /notify list while emitting +// another. Checks both the parent's actual notification output and the stored +// preference, per the review. +func TestBTWNotifyMutationsLeaveParentAndStoredPreferenceIntact(t *testing.T) { + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, []byte(`{"notify":{"mode":"bell","focusMode":"always"}}`), 0o600); err != nil { + t.Fatal(err) + } + parent := newBTWTestModel(t) + parent.userConfigPath = cfgPath + parent.notifyMode = "bell" + parent.notifyFocusMode = "always" + var buf bytes.Buffer + parent.notifier = notify.New(&buf, notify.Config{Mode: notify.ModeBell, FocusMode: notify.FocusAlways}) + parent.notifier.SetFocused(true) + + // Enter the side conversation and attempt a mutation. + side, _ := parent.handleBTWCommand("") + updated, _ := side.dispatchCommand(parseCommand("/notify off")) + got := updated.(model) + if got.picker != nil { + t.Fatal("/notify inside BTW must not open the picker") + } + if !transcriptContains(got.transcript, "unavailable in a BTW conversation") { + t.Fatal("expected the BTW blocked-command guidance") + } + + // Return to the parent: the live policy still bells (actual output), and + // the stored preference is unchanged. + returned, _ := got.handleBTWCommand("") + parent = returned + parent.notifier.Notify(notify.Completion, "x") + if buf.String() != "\x07" { + t.Fatalf("parent notifier was reconfigured by the side conversation: got %q, want the original bell", buf.String()) + } + stored, err := config.UserNotify(cfgPath) + if err != nil { + t.Fatalf("read stored notify: %v", err) + } + if stored.Mode != "bell" || stored.FocusMode != "always" { + t.Fatalf("stored preference changed from inside BTW: %+v", stored) + } + if parent.notifyMode != "bell" || parent.notifyFocusMode != "always" { + t.Fatalf("parent live policy changed from inside BTW: %q/%q", parent.notifyMode, parent.notifyFocusMode) + } +} + func TestBTWExitBlockedWhileParentRunActive(t *testing.T) { m := newBTWTestModel(t) m.pending = true diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index 6da03498c..e53682d15 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -2940,9 +2940,11 @@ func TestEffectiveTUINotifyMode(t *testing.T) { in string want notify.Mode }{ - // Empty input falls through to the resolver default ("both": bell + - // OSC-9 desktop notification) so the permission-prompt alert works - // for users who never configured notify. + // Empty input falls through to the TUI's own effective default + // ("both": bell + OSC-9 desktop notification) so the needs-input + // alert works for users who never configured notify. The default + // lives here, NOT in config.Resolve — headless runs stay silent + // when unconfigured (maintainer review, PR #1001). {"", notify.ModeBoth}, {" ", notify.ModeBoth}, {"off", notify.ModeOff}, diff --git a/internal/tui/notify_select.go b/internal/tui/notify_select.go index 092ae0414..9cbc5b91f 100644 --- a/internal/tui/notify_select.go +++ b/internal/tui/notify_select.go @@ -93,18 +93,14 @@ func (m model) handleNotifyCommand(args string) (model, string) { // LIVE focus: an explicit token wins; otherwise preserve the in-session // value (which started as the resolved pair, project precedence included). liveFocus := m.notifyFocusMode - persistFocus := m.notifyFocusMode + focusExplicit := false if len(tokens) > 1 { focus := strings.ToLower(strings.TrimSpace(tokens[1])) if !isValidNotifyFocusMode(focus) { return m, "Notify\nUnknown focus mode: " + tokens[1] + " (expected unfocused, always, or focused)" } liveFocus = focus - persistFocus = focus - } else if stored, err := m.storedNotify(); err == nil { - // PERSISTED focus only: what the USER's global file holds (blank stays - // blank). The live session keeps its resolved focus above. - persistFocus = stored.FocusMode + focusExplicit = true } m.notifyMode = mode m.notifyFocusMode = liveFocus @@ -121,22 +117,12 @@ func (m model) handleNotifyCommand(args string) (model, string) { "Notify", "active mode: " + mode + ", focus: " + effectiveFocusLabel(liveFocus), } - if note := m.persistNotifyPreference(mode, persistFocus); note != "" { + if note := m.persistNotifyPreference(mode, liveFocus, focusExplicit); note != "" { lines = append(lines, note) } return m, strings.Join(lines, "\n") } -// storedNotify reads the notify block from the user's own config file. Missing -// file or read error returns the zero value (best-effort, like the rest of the -// preference persistence). -func (m model) storedNotify() (config.NotifyConfig, error) { - if strings.TrimSpace(m.userConfigPath) == "" { - return config.NotifyConfig{}, nil - } - return config.UserNotify(m.userConfigPath) -} - // effectiveFocusLabel renders a focus value for the state line: blank means the // built-in "unfocused" default, so say so instead of showing an empty string. func effectiveFocusLabel(focus string) string { @@ -147,16 +133,28 @@ func effectiveFocusLabel(focus string) string { } // persistNotifyPreference writes the choice to user config so it survives a -// restart. Best-effort: returns a short note to surface on failure, or "" on -// success / when there is no config path (e.g. tests). -func (m model) persistNotifyPreference(mode string, focus string) string { +// restart, as ONE serialized read-merge-write (config.UpdateNotify): the lock +// covers reading the stored block and replacing the file, so a concurrent +// partial update cannot interleave (maintainer review, PR #1001). +// +// Persist contract: an explicit pair replaces both fields. A mode-only change +// preserves the focus stored in the USER'S OWN file (blank stays blank) — the +// merge leaves current.FocusMode untouched rather than pre-reading it, which +// would reopen the stale-preservation window the lock is there to close. Best- +// effort: returns a short note to surface on failure, or "" on success / when +// there is no config path (e.g. tests). +func (m model) persistNotifyPreference(mode string, focus string, focusExplicit bool) string { if strings.TrimSpace(m.userConfigPath) == "" { return "" } - if _, err := config.SetNotify(m.userConfigPath, config.NotifyConfig{ - Mode: mode, - FocusMode: focus, - }); err != nil { + _, err := config.UpdateNotify(m.userConfigPath, func(current config.NotifyConfig) config.NotifyConfig { + current.Mode = mode + if focusExplicit { + current.FocusMode = focus + } + return current + }) + if err != nil { return "note: could not save notify preference (" + err.Error() + ")" } return "" From 8012ce1fb89f408074a9fcdc25fe6cc2c1faa23b Mon Sep 17 00:00:00 2001 From: Gaurav Bhatia Date: Sun, 6 Sep 2026 16:40:54 +0400 Subject: [PATCH 8/8] fix(config): notify reset removes every duplicate notify member MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hand-edited config may contain duplicate notify members (JSON decoders tolerate them; the last occurrence wins). The reset path removed only the final member, so an earlier block survived and became the effective preference on the next decode — `zero config notify --reset` reported success while the old value still applied (CodeRabbit review, PR #1001). Loop the removal until no notify member remains, mirroring setPetPreferenceJSON's duplicate handling. The replace path still edits the LAST member, which is the one a decoder treats as effective, so an earlier duplicate stays inert there. Regression TestSetNotifyResetRemovesEveryDuplicateNotifyMember verifies the reset-orphaned failure output on the pre-fix code and passes after; it also pins the no-op reset and the last-member-replace behavior. --- internal/config/json_object_edit.go | 20 ++++++++-- internal/config/writer_test.go | 57 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/internal/config/json_object_edit.go b/internal/config/json_object_edit.go index 9622696f7..f6a6a5778 100644 --- a/internal/config/json_object_edit.go +++ b/internal/config/json_object_edit.go @@ -253,6 +253,14 @@ func petJSONObject(encodedPet []byte) []byte { // notification writes go through this byte-preserving editor instead // (maintainer review, PR #1001). An empty notify block removes the member // entirely so a reset leaves no `"notify": {}` husk behind. +// +// A hand-edited file may contain DUPLICATE notify members (tolerated by JSON +// decoders, where the last occurrence wins). The reset path therefore removes +// every notify member — deleting only the last would leave an earlier block +// behind, and that survivor becomes the effective preference on the next +// decode, so the reset would silently fail. Mirrors setPetPreferenceJSON's +// duplicate handling. The replace path edits the LAST member, which is the one +// a decoder treats as effective, so an earlier duplicate stays inert. func setNotifyJSONObject(data []byte, notify NotifyConfig) ([]byte, error) { encoded, err := json.Marshal(notify) if err != nil { @@ -266,10 +274,16 @@ func setNotifyJSONObject(data []byte, notify NotifyConfig) ([]byte, error) { } notifyIndex := lastJSONMember(root.members, "notify") if string(encoded) == "{}" { - if notifyIndex < 0 { - return data, nil + for notifyIndex >= 0 { + data = removeJSONMember(data, root, notifyIndex) + rootStart = skipJSONSpace(data, 0) + root, err = parseJSONObject(data, rootStart) + if err != nil { + return nil, err + } + notifyIndex = lastJSONMember(root.members, "notify") } - return removeJSONMember(data, root, notifyIndex), nil + return data, nil } if notifyIndex < 0 { return insertJSONMember(data, root, "notify", encoded), nil diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 311ba6e6b..8b9eb71bd 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -491,6 +491,63 @@ func TestSetNotifyPreservesExplicitUnrelatedValues(t *testing.T) { } } +// Maintainer regression (PR #1001, CodeRabbit follow-up): a hand-edited config +// may contain DUPLICATE notify members (JSON decoders tolerate them and the +// last occurrence wins). A reset must remove EVERY notify member — removing +// only the last leaves the earlier block as the new effective preference, so +// `zero config notify --reset` would report success while the old value still +// applies. +func TestSetNotifyResetRemovesEveryDuplicateNotifyMember(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + duplicated := `{"notify":{"mode":"off"},"activeProvider":"openai","notify":{"mode":"bell"}}` + if err := os.WriteFile(path, []byte(duplicated), 0o600); err != nil { + t.Fatal(err) + } + + if _, err := SetNotify(path, NotifyConfig{}); err != nil { + t.Fatalf("SetNotify({}) reset: %v", err) + } + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var obj map[string]json.RawMessage + if err := json.Unmarshal(raw, &obj); err != nil { + t.Fatalf("decode: %v", err) + } + if _, still := obj["notify"]; still { + t.Fatalf("reset left a notify member behind: %s", string(raw)) + } + if obj["activeProvider"] == nil || string(obj["activeProvider"]) != `"openai"` { + t.Errorf("unrelated member lost through the reset: %s", string(raw)) + } + + // A reset on a file with NO notify member is a no-op, and a partial update + // against duplicates replaces the LAST member (the effective one under + // last-occurrence-wins decoding). + if err := os.WriteFile(path, []byte(duplicated), 0o600); err != nil { + t.Fatal(err) + } + if _, err := SetNotify(path, NotifyConfig{Mode: "off", FocusMode: "always"}); err != nil { + t.Fatalf("SetNotify replace: %v", err) + } + raw, err = os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + stored, err := UserNotify(path) + if err != nil { + t.Fatalf("UserNotify: %v", err) + } + if stored.Mode != "off" || stored.FocusMode != "always" { + t.Fatalf("after replace, effective notify = %+v, want off/always (last member replaced)", stored) + } + if !strings.Contains(string(raw), `"activeProvider"`) { + t.Errorf("unrelated member lost through the replace: %s", string(raw)) + } +} + // Maintainer regression (PR #1001): concurrent partial updates must not lose // each other's explicit change. Two `zero config notify` calls (--mode off and // --focus always) racing from both/unfocused previously interleaved their