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 a6fab33ec..fbf22b6f9 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()) @@ -462,8 +486,12 @@ 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 stored global notification preference, +which controls both the completion and needs-input alerts — +run "zero config notify --help" for details. Flags: --json Print JSON summary 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 new file mode 100644 index 000000000..9caac13b4 --- /dev/null +++ b/internal/cli/config_notify.go @@ -0,0 +1,176 @@ +package cli + +import ( + "fmt" + "io" + "strings" + + "github.com/Gitlawb/zero/internal/config" +) + +// runConfigNotify implements `zero config notify`: with no flags it prints the +// 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 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 +// 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 { + return writeExecUsageError(stderr, err.Error()) + } + if help { + if err := writeConfigNotifyHelp(stdout); err != nil { + return exitCrash + } + return exitSuccess + } + + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + + if options.mode != "" || options.focus != "" || options.reset { + // 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 != "" { + current.Mode = options.mode + } + if options.focus != "" { + current.FocusMode = options.focus + } + return current + }) + if err != nil { + return writeAppError(stderr, err.Error(), exitUsage) + } + } + + // 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": current.Mode, + "focusMode": current.FocusMode, + }); err != nil { + return exitCrash + } + return exitSuccess + } + lines := []string{ + "Notify", + "mode: " + displayCLIValue(current.Mode, "(default)"), + "focusMode: " + displayCLIValue(current.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 stored global notification preference.\n"+ + "\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, 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 the stored preference\n"+ + "\n"+ + "Flags:\n"+ + " --mode Notification mechanism (both kinds)\n"+ + " --focus When the alert fires\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/cli/config_notify_test.go b/internal/cli/config_notify_test.go new file mode 100644 index 000000000..bcd56141c --- /dev/null +++ b/internal/cli/config_notify_test.go @@ -0,0 +1,481 @@ +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: 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 + // 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: (default)") { + t.Errorf("stdout should show unconfigured mode as (default), 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. 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", + "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"] != "" { + t.Errorf("mode = %v, want empty (unconfigured)", payload["mode"]) + } + if payload["focusMode"] != "" { + t.Errorf("focusMode = %v, want empty (unconfigured)", 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" + }], + "notify": {"mode": "both", "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", "--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) + } + // 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) + } +} + +// 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") + 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 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 := `{ + "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) + } +} + +// 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 + 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 a277d553b..cd00c1ebf 100644 --- a/internal/cli/exec_test.go +++ b/internal/cli/exec_test.go @@ -970,10 +970,10 @@ func TestRunExecUsesProjectConfigAndOpenAICompatibleProvider(t *testing.T) { "name": "local", "provider_kind": "openai-compatible", "base_url": "` + server.URL + `", - "api_key": "sk-local", - "model": "local-model" - }] - }` + "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/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..f6a6a5778 100644 --- a/internal/config/json_object_edit.go +++ b/internal/config/json_object_edit.go @@ -244,6 +244,53 @@ 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. +// +// 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 { + 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) == "{}" { + 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 data, 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 13038664e..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) } } @@ -1864,14 +1869,43 @@ func TestResolveNotifyInvalidFocusMode(t *testing.T) { } } -func TestResolveNotifyDefaultEmpty(t *testing.T) { - path := writeConfig(t, `{}`) - 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 != "" || resolved.Notify.FocusMode != "" { - t.Fatalf("unset notify should be empty, got %+v", resolved.Notify) + t.Fatalf("no config file: notify = %+v, want empty (resolver must not default)", resolved.Notify) + } + + 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 e3b6846f2..edf89de67 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -8,6 +8,7 @@ import ( "sort" "strings" + "github.com/Gitlawb/zero/internal/notify" "github.com/Gitlawb/zero/internal/providercatalog" ) @@ -591,6 +592,137 @@ 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 +} + +// 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 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 NotifyConfig{}, fmt.Errorf("invalid notify.focusMode %q: expected unfocused, always, or focused", focus) + } + } + 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) + } + data = []byte("{}") + } + 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 +} + // SetPet persists only the terminal-pet preference while preserving every // unrelated user setting through the config writer's atomic replace path. func SetPet(path string, pet string) (FileConfig, error) { diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index c66fc26ba..8b9eb71bd 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" ) @@ -369,6 +371,269 @@ 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 built-in + // defaults" signal — SetNotify must not reject blanks, and they must round + // trip unchanged. + 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) + } +} + +// 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, 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 +// 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 +// 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/notify/notify.go b/internal/notify/notify.go index 79906d5ac..3c9d39ed6 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 @@ -102,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 54ba06aef..2a81720c2 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) @@ -109,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") 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/commands.go b/internal/tui/commands.go index 5eea59d21..9691aab0d 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -36,6 +36,7 @@ const ( commandFast commandStyle commandTheme + commandNotify commandTranscript commandBash commandImage @@ -400,6 +401,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|focused]]", + group: commandGroupSession, + description: "Pick when Zero alerts you (completion and 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 9473de06a..7ffeb4a30 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -212,16 +212,21 @@ type model struct { keyBindings keyBindings themeMode themeMode // palette preference: system (default) or named palette hasDarkBg bool // last terminal background-detection result, if one is delivered - 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. @@ -876,6 +881,22 @@ type tuiAgentRunOptions struct { specDraft bool } +// effectiveTUINotifyMode returns the notification mode the TUI should use. An +// 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 == "" { + return notify.ModeBoth + } + return m +} + func newModel(ctx context.Context, options Options) model { if ctx == nil { ctx = context.Background() @@ -947,7 +968,7 @@ func newModel(ctx context.Context, options Options) model { runSpinner.Spinner.FPS = activeAnimationFrameInterval 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 @@ -1009,6 +1030,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(), @@ -4524,6 +4547,14 @@ func (m model) choosePicker() (tea.Model, tea.Cmd) { return m.showTransientNotice(m.themeAppliedNotice(), transientNoticeSuccess) } m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) + 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 } @@ -4932,6 +4963,18 @@ func (m model) dispatchCommand(command parsedCommand) (tea.Model, tea.Cmd) { } m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) 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 b81f3a6cc..e53682d15 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -2935,6 +2935,31 @@ func TestModelNotifierFocusAndCompletion(t *testing.T) { } } +func TestEffectiveTUINotifyMode(t *testing.T) { + cases := []struct { + in string + want notify.Mode + }{ + // 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}, + {"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 TestComposerBlinkStaysSolidWhileTyping(t *testing.T) { base := time.Date(2026, 7, 7, 12, 0, 0, 0, time.UTC) now := base diff --git a/internal/tui/notify_select.go b/internal/tui/notify_select.go new file mode 100644 index 000000000..9cbc5b91f --- /dev/null +++ b/internal/tui/notify_select.go @@ -0,0 +1,207 @@ +package tui + +import ( + "strings" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/notify" +) + +// notifyChoice is one row in the /notify picker: a (mode, focusMode) pair and +// the label the user reads. +type notifyChoice struct { + label string + mode string + focusMode string +} + +// 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. +// +// 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 { + 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 { + 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)" + } + // LIVE focus: an explicit token wins; otherwise preserve the in-session + // value (which started as the resolved pair, project precedence included). + liveFocus := 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 + focusExplicit = true + } + m.notifyMode = mode + m.notifyFocusMode = liveFocus + // Apply to the live notifier so the change takes effect on the next + // 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(liveFocus), + }) + } + lines := []string{ + "Notify", + "active mode: " + mode + ", focus: " + effectiveFocusLabel(liveFocus), + } + if note := m.persistNotifyPreference(mode, liveFocus, focusExplicit); note != "" { + lines = append(lines, note) + } + return m, strings.Join(lines, "\n") +} + +// 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, 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 "" + } + _, 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 "" +} + +// 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 { + sections := []commandSection{{ + Title: "State", + Lines: []string{ + "active mode: " + m.notifyMode, + "active focus: " + effectiveFocusLabel(m.notifyFocusMode), + }, + }} + rows := make([]string, 0, 12) + for _, c := range notifyPickerChoices() { + 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"}, + }) +} + +// 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..0bf549c23 --- /dev/null +++ b/internal/tui/notify_select_test.go @@ -0,0 +1,431 @@ +package tui + +import ( + "bytes" + "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, +// 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 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 always") + 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" || cfg.Notify.FocusMode != "always" { + t.Fatalf("notify = %+v, want mode=off focusMode=always", cfg.Notify) + } + + // 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) + } +} + +// 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) + } + 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", persisted.FocusMode) + } + + // Blank stays blank: with nothing stored, a mode-only change must not pin + // 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, + 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) + } + if persisted.FocusMode != "" { + t.Errorf("persisted focusMode = %q, want blank (unspecified stays unspecified)", persisted.FocusMode) + } +} + +// 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{}) + 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) + } +} + +// 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 + 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 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 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()) + } +} + +// `/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 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) { + 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` fails validation before persisting, so neither field +// changes. +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) + } +} + +// 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 { + 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) + } + // (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") + } + persisted := readNotifyBlock(t, cfgPath) + if persisted.Mode != "off" || persisted.FocusMode != "always" { + t.Fatalf("Enter changed the setting: got %+v, want off/always unchanged", persisted) + } +} + +// 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() + 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 { + 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 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 the live mode, got: %s", state) + } + if !strings.Contains(state, "active focus: always") { + 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) + } +} + +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) + } + 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 563dffe62..4727e3c11 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" @@ -29,6 +30,7 @@ const ( pickerSTTModel pickerSTTDownload pickerPet + pickerNotify ) // pickerItem is one selectable row: Label is shown, Value is passed to the @@ -1066,6 +1068,44 @@ func (m model) newThemePicker() *commandPicker { return &commandPicker{kind: pickerTheme, title: "Choose a theme", items: items, allItems: append([]pickerItem{}, items...), selected: selected} } +// newNotifyPicker lists the FULL (mode, focus) space from notifyPickerChoices +// (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. 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 + activeMode := m.notifyMode + if strings.TrimSpace(activeMode) == "" { + activeMode = string(notify.ModeBoth) + } + activeFocus := m.notifyFocusMode + 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(), + }) + 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. Theme candidates render // only in the picker preview; their active palette is applied only after Enter. // Safe to call with no picker open. Callers mutate through m.picker (a pointer),