From 1c749b3a0181ed9d0f5d3cf1add9665d6715aa37 Mon Sep 17 00:00:00 2001 From: Jeroen D Date: Wed, 29 Apr 2026 20:22:12 +0000 Subject: [PATCH 1/2] feat(config): make worktree prompt branch policy configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the hardcoded main/master check that decided whether to prompt the user about creating a worktree with two new config fields under .chief/config.yaml: - worktree.alwaysPrompt: bool — when true, always prompt regardless of branch name. - worktree.promptBranchPattern: string (regex) — pattern matched against the current branch; when it matches, Chief prompts. Defaults preserve the prior behavior (pattern: ^(main|master)$); existing config files inherit the default on load. Invalid regex fails fast at startup with a field-named error. Save also validates so future write paths can't persist a broken pattern. Internal cleanup carried with this change: - Remove exported git.IsProtectedBranch in favor of a private isDefaultBranchName for the diff-base helper inside the git package; the TUI call site now consults the new policy method. - Rename DialogProtectedBranch to DialogWorktreePrompt; dialog title generalized so it stays accurate when triggered by regex match or alwaysPrompt on non-default branches. - NewAppWithOptions returns errors for invalid config (was os.Exit) and reliably releases the fsnotify watcher on early return. - prd.Watcher.Stop and prd.ProgressWatcher.Stop now release the underlying fsnotify watcher even when Start was never called, and are idempotent across repeated calls. - Octal literals normalized to the dominant legacy style (0755 / 0644) for repo-wide consistency. - Documentation updated with the new keys, examples, RE2 note, Markdown-escape clarification, and accurate Settings TUI scope. Co-Authored-By: Claude Opus 4.7 --- docs/reference/configuration.md | 22 ++- internal/agent/resolve_test.go | 4 +- internal/config/config.go | 62 ++++++- internal/config/config_test.go | 262 +++++++++++++++++++++++++++- internal/git/git.go | 8 +- internal/git/git_test.go | 6 +- internal/prd/progress.go | 12 +- internal/prd/progress_test.go | 20 +++ internal/prd/watcher.go | 13 +- internal/prd/watcher_test.go | 22 +++ internal/tui/app.go | 19 +- internal/tui/app_test.go | 41 +++++ internal/tui/branch_warning.go | 16 +- internal/tui/branch_warning_test.go | 49 ++++-- internal/update/update.go | 2 +- internal/update/update_test.go | 10 +- 16 files changed, 516 insertions(+), 52 deletions(-) create mode 100644 internal/tui/app_test.go diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 5417b850..7354dd8b 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -18,6 +18,8 @@ agent: cliPath: "" # optional path to CLI binary worktree: setup: "npm install" + alwaysPrompt: false + promptBranchPattern: "^(main|master)$" onComplete: push: true createPR: true @@ -30,9 +32,13 @@ onComplete: | `agent.provider` | string | `"claude"` | Agent CLI to use: `claude`, `codex`, `opencode`, or `cursor` | | `agent.cliPath` | string | `""` | Optional path to the agent binary (e.g. `/usr/local/bin/opencode`). If empty, Chief uses the provider name from PATH. | | `worktree.setup` | string | `""` | Shell command to run in new worktrees (e.g., `npm install`, `go mod download`) | +| `worktree.alwaysPrompt` | bool | `false` | When true, Chief always prompts about creating a git worktree before starting a loop, regardless of the current branch name. Overrides `promptBranchPattern`. | +| `worktree.promptBranchPattern` | string (regex) | `"^(main\|master)$"` | Regular expression matched against the current branch name. When it matches, Chief prompts about creating a git worktree. Empty string disables matching. Ignored when `alwaysPrompt` is true. Invalid regex causes Chief to fail at startup with an error naming the offending field. Patterns use Go's RE2 syntax — lookarounds and backreferences are not supported. | | `onComplete.push` | bool | `false` | Automatically push the branch to remote when a PRD completes | | `onComplete.createPR` | bool | `false` | Automatically create a pull request when a PRD completes (requires `gh` CLI) | +These settings are not yet exposed in the in-app Settings TUI; edit `.chief/config.yaml` directly. If you write `promptBranchPattern: ""` explicitly, Chief skips branch-name matching entirely; only `alwaysPrompt` will trigger the prompt. Default regex: `^(main|master)$`. The `\|` shown in the default column above is Markdown escaping for the table separator; the actual regex value uses an unescaped `|`. + ### Example Configurations **Minimal (defaults):** @@ -55,9 +61,23 @@ onComplete: createPR: true ``` +**Prompt for worktree on main, master, or any release branch:** + +```yaml +worktree: + promptBranchPattern: "^(main|master|release/.*)$" +``` + +**Always prompt for a worktree:** + +```yaml +worktree: + alwaysPrompt: true +``` + ## Settings TUI -Press `,` from any view in the TUI to open the Settings overlay. This provides an interactive way to view and edit all config values. +Press `,` from any view in the TUI to open the Settings overlay. This provides an interactive way to view and edit a subset of common config values. Settings are organized by section: diff --git a/internal/agent/resolve_test.go b/internal/agent/resolve_test.go index 32796f19..de0eece1 100644 --- a/internal/agent/resolve_test.go +++ b/internal/agent/resolve_test.go @@ -188,7 +188,7 @@ func TestCheckInstalled_found(t *testing.T) { func TestResolve_configFile(t *testing.T) { dir := t.TempDir() cfgPath := filepath.Join(dir, ".chief", "config.yaml") - if err := os.MkdirAll(filepath.Dir(cfgPath), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(cfgPath), 0755); err != nil { t.Fatal(err) } const yamlContent = ` @@ -196,7 +196,7 @@ agent: provider: codex cliPath: /usr/local/bin/codex ` - if err := os.WriteFile(cfgPath, []byte(yamlContent), 0o644); err != nil { + if err := os.WriteFile(cfgPath, []byte(yamlContent), 0644); err != nil { t.Fatal(err) } cfg, err := config.Load(dir) diff --git a/internal/config/config.go b/internal/config/config.go index 4523b1ba..61dd0e4c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,8 +1,10 @@ package config import ( + "fmt" "os" "path/filepath" + "regexp" "gopkg.in/yaml.v3" ) @@ -14,6 +16,8 @@ type Config struct { Worktree WorktreeConfig `yaml:"worktree"` OnComplete OnCompleteConfig `yaml:"onComplete"` Agent AgentConfig `yaml:"agent"` + + promptBranchRegex *regexp.Regexp } // AgentConfig holds agent CLI settings (Claude, Codex, OpenCode, or Cursor). @@ -24,7 +28,9 @@ type AgentConfig struct { // WorktreeConfig holds worktree-related settings. type WorktreeConfig struct { - Setup string `yaml:"setup"` + Setup string `yaml:"setup"` + AlwaysPrompt bool `yaml:"alwaysPrompt"` + PromptBranchPattern string `yaml:"promptBranchPattern"` } // OnCompleteConfig holds post-completion automation settings. @@ -35,7 +41,48 @@ type OnCompleteConfig struct { // Default returns a Config with zero-value defaults. func Default() *Config { - return &Config{} + cfg := &Config{ + Worktree: WorktreeConfig{ + PromptBranchPattern: "^(main|master)$", + }, + } + if err := cfg.Validate(); err != nil { + panic(fmt.Sprintf("config: default config failed to validate: %v", err)) + } + return cfg +} + +// Validate compiles derived config state (e.g., the prompt-branch regex +// cache) and reports configuration errors. Idempotent — safe to call +// multiple times. Callers must call Validate after mutating Config fields +// that affect derived state. +func (c *Config) Validate() error { + return c.compilePromptRegex() +} + +// compilePromptRegex compiles and caches the worktree prompt-branch regex. +func (c *Config) compilePromptRegex() error { + if c.Worktree.PromptBranchPattern == "" { + c.promptBranchRegex = nil + return nil + } + re, err := regexp.Compile(c.Worktree.PromptBranchPattern) + if err != nil { + return fmt.Errorf("invalid worktree.promptBranchPattern %q: %w", c.Worktree.PromptBranchPattern, err) + } + c.promptBranchRegex = re + return nil +} + +// ShouldPromptForWorktree reports whether Chief should prompt the user about using a git worktree for the given branch. +func (c *Config) ShouldPromptForWorktree(branch string) bool { + if c.Worktree.AlwaysPrompt { + return true + } + if c.promptBranchRegex == nil { + return false + } + return c.promptBranchRegex.MatchString(branch) } // configPath returns the full path to the config file. @@ -66,16 +113,23 @@ func Load(baseDir string) (*Config, error) { if err := yaml.Unmarshal(data, cfg); err != nil { return nil, err } + if err := cfg.Validate(); err != nil { + return nil, err + } return cfg, nil } // Save writes the config to .chief/config.yaml. func Save(baseDir string, cfg *Config) error { + if err := cfg.Validate(); err != nil { + return err + } + path := configPath(baseDir) // Ensure directory exists - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { return err } @@ -84,5 +138,5 @@ func Save(baseDir string, cfg *Config) error { return err } - return os.WriteFile(path, data, 0o644) + return os.WriteFile(path, data, 0644) } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index dacee39c..c77ba53b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "strings" "testing" ) @@ -11,12 +12,24 @@ func TestDefault(t *testing.T) { if cfg.Worktree.Setup != "" { t.Errorf("expected empty setup, got %q", cfg.Worktree.Setup) } + if cfg.Worktree.AlwaysPrompt { + t.Error("expected AlwaysPrompt to be false") + } + if cfg.Worktree.PromptBranchPattern != "^(main|master)$" { + t.Errorf("expected default PromptBranchPattern, got %q", cfg.Worktree.PromptBranchPattern) + } if cfg.OnComplete.Push { t.Error("expected Push to be false") } if cfg.OnComplete.CreatePR { t.Error("expected CreatePR to be false") } + if !cfg.ShouldPromptForWorktree("main") { + t.Error("expected default to prompt on main") + } + if cfg.ShouldPromptForWorktree("feature/x") { + t.Error("expected default to not prompt on feature/x") + } } func TestLoadNonExistent(t *testing.T) { @@ -27,6 +40,9 @@ func TestLoadNonExistent(t *testing.T) { if cfg.Worktree.Setup != "" { t.Errorf("expected empty setup, got %q", cfg.Worktree.Setup) } + if !cfg.ShouldPromptForWorktree("main") { + t.Error("expected default load to prompt on main") + } } func TestSaveAndLoad(t *testing.T) { @@ -62,6 +78,248 @@ func TestSaveAndLoad(t *testing.T) { } } +func TestSaveAndLoadPromptFields(t *testing.T) { + dir := t.TempDir() + + cfg := &Config{ + Worktree: WorktreeConfig{ + AlwaysPrompt: true, + PromptBranchPattern: "^release/.*$", + }, + } + + if err := Save(dir, cfg); err != nil { + t.Fatalf("Save failed: %v", err) + } + + loaded, err := Load(dir) + if err != nil { + t.Fatalf("Load failed: %v", err) + } + + if !loaded.Worktree.AlwaysPrompt { + t.Error("expected AlwaysPrompt to be true after round-trip") + } + if loaded.Worktree.PromptBranchPattern != "^release/.*$" { + t.Errorf("expected PromptBranchPattern %q, got %q", "^release/.*$", loaded.Worktree.PromptBranchPattern) + } + if !loaded.ShouldPromptForWorktree("release/v1") { + t.Error("expected ShouldPromptForWorktree to return true for release/v1") + } +} + +func TestLoadInvalidPromptRegex(t *testing.T) { + dir := t.TempDir() + chiefDir := filepath.Join(dir, ".chief") + if err := os.MkdirAll(chiefDir, 0755); err != nil { + t.Fatal(err) + } + yaml := "worktree:\n promptBranchPattern: \"[unclosed\"\n" + if err := os.WriteFile(filepath.Join(chiefDir, "config.yaml"), []byte(yaml), 0644); err != nil { + t.Fatal(err) + } + + _, err := Load(dir) + if err == nil { + t.Fatal("expected error for invalid regex, got nil") + } + if !strings.Contains(err.Error(), "worktree.promptBranchPattern") { + t.Errorf("expected error to mention worktree.promptBranchPattern, got %q", err.Error()) + } +} + +func TestLoadLegacyConfigInheritsDefaults(t *testing.T) { + dir := t.TempDir() + chiefDir := filepath.Join(dir, ".chief") + if err := os.MkdirAll(chiefDir, 0755); err != nil { + t.Fatal(err) + } + // Legacy configs without the new keys inherit Default() values; protected-branch + // safety is preserved on upgrade. + yaml := "worktree:\n setup: \"npm install\"\n" + if err := os.WriteFile(filepath.Join(chiefDir, "config.yaml"), []byte(yaml), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := Load(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Worktree.Setup != "npm install" { + t.Errorf("expected setup %q, got %q", "npm install", cfg.Worktree.Setup) + } + if cfg.Worktree.AlwaysPrompt { + t.Error("expected AlwaysPrompt to be false (default)") + } + if cfg.Worktree.PromptBranchPattern != "^(main|master)$" { + t.Errorf("expected PromptBranchPattern to inherit default %q, got %q", "^(main|master)$", cfg.Worktree.PromptBranchPattern) + } + if !cfg.ShouldPromptForWorktree("main") { + t.Error("expected ShouldPromptForWorktree(main) to be true after legacy load") + } + if cfg.ShouldPromptForWorktree("feature/x") { + t.Error("expected ShouldPromptForWorktree(feature/x) to be false after legacy load") + } +} + +func TestLoadExplicitEmptyPromptPattern(t *testing.T) { + dir := t.TempDir() + chiefDir := filepath.Join(dir, ".chief") + if err := os.MkdirAll(chiefDir, 0755); err != nil { + t.Fatal(err) + } + // An explicit empty value opts out of branch-name matching. + yaml := "worktree:\n promptBranchPattern: \"\"\n" + if err := os.WriteFile(filepath.Join(chiefDir, "config.yaml"), []byte(yaml), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := Load(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Worktree.PromptBranchPattern != "" { + t.Errorf("expected PromptBranchPattern to be empty, got %q", cfg.Worktree.PromptBranchPattern) + } + if cfg.ShouldPromptForWorktree("main") { + t.Error("expected ShouldPromptForWorktree(main) to be false with explicit empty pattern") + } +} + +func TestSaveValidatesBeforeWriting(t *testing.T) { + dir := t.TempDir() + cfg := &Config{ + Worktree: WorktreeConfig{ + PromptBranchPattern: "[bad", + }, + } + + err := Save(dir, cfg) + if err == nil { + t.Fatal("expected error from Save with invalid regex, got nil") + } + if !strings.Contains(err.Error(), "worktree.promptBranchPattern") { + t.Errorf("expected error to mention worktree.promptBranchPattern, got %q", err.Error()) + } + + if _, statErr := os.Stat(filepath.Join(dir, ".chief", "config.yaml")); !os.IsNotExist(statErr) { + t.Errorf("expected config file not to exist after failed Save, got stat err %v", statErr) + } +} + +func TestShouldPromptForWorktree(t *testing.T) { + tests := []struct { + name string + alwaysPrompt bool + pattern string + branch string + want bool + }{ + {"alwaysPrompt wins with empty pattern", true, "", "anything", true}, + {"alwaysPrompt wins over non-matching pattern", true, "^main$", "feature/x", true}, + {"default pattern matches main", false, "^(main|master)$", "main", true}, + {"default pattern matches master", false, "^(main|master)$", "master", true}, + {"default pattern rejects feature branch", false, "^(main|master)$", "feature/x", false}, + {"empty pattern with no alwaysPrompt rejects main", false, "", "main", false}, + {"release pattern matches release branch", false, "^release/.*$", "release/v1", true}, + {"release pattern rejects main", false, "^release/.*$", "main", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &Config{ + Worktree: WorktreeConfig{ + AlwaysPrompt: tt.alwaysPrompt, + PromptBranchPattern: tt.pattern, + }, + } + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate failed: %v", err) + } + got := cfg.ShouldPromptForWorktree(tt.branch) + if got != tt.want { + t.Errorf("ShouldPromptForWorktree(%q) = %v; want %v", tt.branch, got, tt.want) + } + }) + } +} + +func TestValidateInvalidRegex(t *testing.T) { + cfg := &Config{ + Worktree: WorktreeConfig{ + PromptBranchPattern: "[unclosed", + }, + } + err := cfg.Validate() + if err == nil { + t.Fatal("expected error for invalid regex, got nil") + } + if !strings.Contains(err.Error(), "worktree.promptBranchPattern") { + t.Errorf("expected error to mention worktree.promptBranchPattern, got %q", err.Error()) + } +} + +func TestValidateEmptyPatternIsValid(t *testing.T) { + cfg := &Config{ + Worktree: WorktreeConfig{ + PromptBranchPattern: "", + }, + } + if err := cfg.Validate(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + if cfg.ShouldPromptForWorktree("main") { + t.Error("expected ShouldPromptForWorktree to be false for empty pattern") + } + if cfg.ShouldPromptForWorktree("anything") { + t.Error("expected ShouldPromptForWorktree to be false for empty pattern") + } +} + +func TestLoadInvalidYAML(t *testing.T) { + dir := t.TempDir() + chiefDir := filepath.Join(dir, ".chief") + if err := os.MkdirAll(chiefDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(chiefDir, "config.yaml"), []byte("worktree: [unterminated\n"), 0644); err != nil { + t.Fatal(err) + } + + if _, err := Load(dir); err == nil { + t.Fatal("expected error for malformed YAML, got nil") + } +} + +func TestLoadReadError(t *testing.T) { + dir := t.TempDir() + chiefDir := filepath.Join(dir, ".chief") + if err := os.MkdirAll(chiefDir, 0755); err != nil { + t.Fatal(err) + } + // Make config.yaml a directory so ReadFile returns a non-IsNotExist error. + if err := os.MkdirAll(filepath.Join(chiefDir, "config.yaml"), 0755); err != nil { + t.Fatal(err) + } + + if _, err := Load(dir); err == nil { + t.Fatal("expected error when config.yaml is a directory, got nil") + } +} + +func TestSaveMkdirError(t *testing.T) { + dir := t.TempDir() + // Create a file at the path where the .chief directory needs to live. + blocking := filepath.Join(dir, ".chief") + if err := os.WriteFile(blocking, []byte("not a dir"), 0644); err != nil { + t.Fatal(err) + } + + if err := Save(dir, &Config{}); err == nil { + t.Fatal("expected error when .chief path is a file, got nil") + } +} + func TestExists(t *testing.T) { dir := t.TempDir() @@ -71,10 +329,10 @@ func TestExists(t *testing.T) { // Create the config chiefDir := filepath.Join(dir, ".chief") - if err := os.MkdirAll(chiefDir, 0o755); err != nil { + if err := os.MkdirAll(chiefDir, 0755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(chiefDir, "config.yaml"), []byte("{}"), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(chiefDir, "config.yaml"), []byte("{}"), 0644); err != nil { t.Fatal(err) } diff --git a/internal/git/git.go b/internal/git/git.go index fd1d2ca4..2e626dc5 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -18,8 +18,8 @@ func GetCurrentBranch(dir string) (string, error) { return strings.TrimSpace(string(output)), nil } -// IsProtectedBranch returns true if the branch name is main or master. -func IsProtectedBranch(branch string) bool { +// isDefaultBranchName returns true if the branch name is main or master. +func isDefaultBranchName(branch string) bool { return branch == "main" || branch == "master" } @@ -79,7 +79,7 @@ func GetDiff(dir string) (string, error) { } // If on a feature branch, diff against merge-base with main/master - if !IsProtectedBranch(branch) { + if !isDefaultBranchName(branch) { baseBranch, err := GetDefaultBranch(dir) if err == nil && baseBranch != "" { mergeBase, err := getMergeBase(dir, baseBranch, "HEAD") @@ -100,7 +100,7 @@ func GetDiffStats(dir string) (string, error) { return "", err } - if !IsProtectedBranch(branch) { + if !isDefaultBranchName(branch) { baseBranch, err := GetDefaultBranch(dir) if err == nil && baseBranch != "" { mergeBase, err := getMergeBase(dir, baseBranch, "HEAD") diff --git a/internal/git/git_test.go b/internal/git/git_test.go index b999b369..62586dea 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -129,7 +129,7 @@ func TestAddChiefToGitignore(t *testing.T) { }) } -func TestIsProtectedBranch(t *testing.T) { +func TestIsDefaultBranchName(t *testing.T) { tests := []struct { branch string expected bool @@ -143,9 +143,9 @@ func TestIsProtectedBranch(t *testing.T) { for _, tt := range tests { t.Run(tt.branch, func(t *testing.T) { - result := IsProtectedBranch(tt.branch) + result := isDefaultBranchName(tt.branch) if result != tt.expected { - t.Errorf("IsProtectedBranch(%q) = %v, want %v", tt.branch, result, tt.expected) + t.Errorf("isDefaultBranchName(%q) = %v, want %v", tt.branch, result, tt.expected) } }) } diff --git a/internal/prd/progress.go b/internal/prd/progress.go index b21fb40a..e8cf2cbf 100644 --- a/internal/prd/progress.go +++ b/internal/prd/progress.go @@ -93,6 +93,7 @@ type ProgressWatcher struct { done chan struct{} mu sync.Mutex running bool + closed bool } // NewProgressWatcher creates a new watcher for progress.md in the same @@ -130,17 +131,22 @@ func (w *ProgressWatcher) Start() error { return nil } -// Stop stops watching. +// Stop stops watching and releases resources held by the underlying fsnotify +// watcher. Safe to call before Start and idempotent across repeated calls. func (w *ProgressWatcher) Stop() { w.mu.Lock() - if !w.running { + if w.closed { w.mu.Unlock() return } + w.closed = true + wasRunning := w.running w.running = false w.mu.Unlock() - close(w.done) + if wasRunning { + close(w.done) + } w.watcher.Close() } diff --git a/internal/prd/progress_test.go b/internal/prd/progress_test.go index 2f6d2108..28c24450 100644 --- a/internal/prd/progress_test.go +++ b/internal/prd/progress_test.go @@ -263,3 +263,23 @@ func TestProgressPath(t *testing.T) { t.Errorf("ProgressPath() = %q, want %q", got, want) } } + +func TestProgressWatcherStopWithoutStart(t *testing.T) { + tmpDir := t.TempDir() + prdPath := filepath.Join(tmpDir, "prd.json") + + w, err := NewProgressWatcher(prdPath) + if err != nil { + t.Fatalf("NewProgressWatcher failed: %v", err) + } + + // Stop must release the underlying fsnotify watcher even when Start was + // never called (otherwise the inotify FD leaks). A subsequent Add to the + // closed fsnotify watcher should fail, proving Close ran. + w.Stop() + w.Stop() // idempotent + + if err := w.watcher.Add(tmpDir); err == nil { + t.Error("expected fsnotify.Add to fail after Stop, but it succeeded") + } +} diff --git a/internal/prd/watcher.go b/internal/prd/watcher.go index 09e2b2f5..351295c9 100644 --- a/internal/prd/watcher.go +++ b/internal/prd/watcher.go @@ -21,6 +21,7 @@ type Watcher struct { done chan struct{} mu sync.Mutex running bool + closed bool lastPRD *PRD } @@ -71,17 +72,23 @@ func (w *Watcher) Start() error { return nil } -// Stop stops watching the PRD file. +// Stop stops watching the PRD file and releases resources held by the +// underlying fsnotify watcher. Safe to call before Start (no goroutine to +// signal) and idempotent across repeated calls. func (w *Watcher) Stop() { w.mu.Lock() - if !w.running { + if w.closed { w.mu.Unlock() return } + w.closed = true + wasRunning := w.running w.running = false w.mu.Unlock() - close(w.done) + if wasRunning { + close(w.done) + } w.watcher.Close() } diff --git a/internal/prd/watcher_test.go b/internal/prd/watcher_test.go index 36d715f7..8a8c9a1d 100644 --- a/internal/prd/watcher_test.go +++ b/internal/prd/watcher_test.go @@ -193,6 +193,28 @@ func TestWatcherStop(t *testing.T) { watcher.Stop() // Should be safe } +func TestWatcherStopWithoutStart(t *testing.T) { + tmpDir := t.TempDir() + prdPath := createTestPRDMd(t, tmpDir, []UserStory{ + {ID: "US-001", Title: "Test Story", Passes: false}, + }) + + watcher, err := NewWatcher(prdPath) + if err != nil { + t.Fatalf("Failed to create watcher: %v", err) + } + + // Stop must release the underlying fsnotify watcher even when Start was + // never called (otherwise the inotify FD leaks). A second Add to the + // closed fsnotify watcher should fail, proving Close ran. + watcher.Stop() + watcher.Stop() // idempotent + + if err := watcher.watcher.Add(prdPath); err == nil { + t.Error("expected fsnotify.Add to fail after Stop, but it succeeded") + } +} + func TestHasStatusChanged(t *testing.T) { tests := []struct { name string diff --git a/internal/tui/app.go b/internal/tui/app.go index 37694fea..8ac4ae0f 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -277,6 +277,12 @@ func NewAppWithOptions(prdPath string, maxIter int, provider loop.Provider) (*Ap if err != nil { return nil, err } + appReady := false + defer func() { + if !appReady { + watcher.Stop() + } + }() // Determine base directory for PRD picker // If path contains .chief/prds/, go up to the project root (4 levels up from prd.json) @@ -290,7 +296,7 @@ func NewAppWithOptions(prdPath string, maxIter int, provider loop.Provider) (*Ap // Load project config cfg, err := config.Load(baseDir) if err != nil { - cfg = config.Default() + return nil, fmt.Errorf("failed to load .chief/config.yaml: %w", err) } // Prune stale worktrees on startup (clean git's internal tracking) @@ -316,6 +322,7 @@ func NewAppWithOptions(prdPath string, maxIter int, provider loop.Provider) (*Ap // Create picker with manager reference (for creating new PRDs) picker := NewPRDPicker(baseDir, prdName, manager) + appReady = true return &App{ prd: p, prdPath: prdPath, @@ -740,22 +747,22 @@ func (a App) startLoopForPRD(prdName string) (tea.Model, tea.Cmd) { relWorktreePath := fmt.Sprintf(".chief/worktrees/%s/", prdName) // Determine dialog context - isProtected := git.IsProtectedBranch(branch) + shouldPromptWorktree := a.config.ShouldPromptForWorktree(branch) anotherRunningInSameDir := a.isAnotherPRDRunningInSameDir(prdName) - if !isProtected && !anotherRunningInSameDir { + if !shouldPromptWorktree && !anotherRunningInSameDir { // No conflicts: skip the dialog entirely and start the loop directly return a.doStartLoop(prdName, prdDir) } var dialogCtx DialogContext - if isProtected { - dialogCtx = DialogProtectedBranch + if shouldPromptWorktree { + dialogCtx = DialogWorktreePrompt } else { dialogCtx = DialogAnotherPRDRunning } - // Show the dialog only for protected branch or another PRD running + // Show the dialog when worktree-prompt policy triggers or another PRD is running. a.branchWarning.SetSize(a.width, a.height) a.branchWarning.SetContext(branch, prdName, relWorktreePath) a.branchWarning.SetDialogContext(dialogCtx) diff --git a/internal/tui/app_test.go b/internal/tui/app_test.go new file mode 100644 index 00000000..28a79264 --- /dev/null +++ b/internal/tui/app_test.go @@ -0,0 +1,41 @@ +package tui + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestNewAppWithOptionsReturnsErrorOnInvalidConfig(t *testing.T) { + tmp := t.TempDir() + + prdDir := filepath.Join(tmp, ".chief", "prds", "test") + if err := os.MkdirAll(prdDir, 0755); err != nil { + t.Fatalf("failed to create prd dir: %v", err) + } + + prdPath := filepath.Join(prdDir, "prd.md") + prdContent := "# Test\n\n### US-001: First Story\n- [ ] Works\n" + if err := os.WriteFile(prdPath, []byte(prdContent), 0644); err != nil { + t.Fatalf("failed to write prd: %v", err) + } + + configPath := filepath.Join(tmp, ".chief", "config.yaml") + configContent := "worktree:\n promptBranchPattern: \"[unclosed\"\n" + if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + t.Fatalf("failed to write config: %v", err) + } + + // If NewAppWithOptions ever calls os.Exit on config error, this test will fail by killing the test binary. + app, err := NewAppWithOptions(prdPath, 5, nil) + if err == nil { + t.Fatal("expected NewAppWithOptions to return an error for invalid config") + } + if app != nil { + t.Fatal("expected nil App when config load fails") + } + if !strings.Contains(err.Error(), "worktree.promptBranchPattern") { + t.Fatalf("expected error to mention worktree.promptBranchPattern, got %q", err.Error()) + } +} diff --git a/internal/tui/branch_warning.go b/internal/tui/branch_warning.go index 106ca0cc..fb5e3afc 100644 --- a/internal/tui/branch_warning.go +++ b/internal/tui/branch_warning.go @@ -21,11 +21,11 @@ const ( type DialogContext int const ( - // DialogProtectedBranch: on a protected branch (main/master) - DialogProtectedBranch DialogContext = iota + // DialogWorktreePrompt: branch matches the worktree-prompt policy (default branches or configured pattern) + DialogWorktreePrompt DialogContext = iota // DialogAnotherPRDRunning: another PRD is already running in the same directory DialogAnotherPRDRunning - // DialogNoConflicts: not protected, nothing else running in same dir + // DialogNoConflicts: worktree prompt policy doesn't apply and nothing else is running in the same dir DialogNoConflicts ) @@ -81,7 +81,7 @@ func (b *BranchWarning) SetDialogContext(ctx DialogContext) { // buildOptions creates the option list based on the dialog context. func (b *BranchWarning) buildOptions() { switch b.context { - case DialogProtectedBranch: + case DialogWorktreePrompt: b.options = []dialogOption{ { label: "Create branch only", @@ -256,9 +256,9 @@ func (b *BranchWarning) Render() string { content.WriteString(footerStyle.Render("↑/↓: Navigate Enter: Select e: Edit branch Esc: Cancel")) } - // Modal box style - use warning color for protected branch, primary for others + // Modal box style - use warning color for the worktree prompt, primary for others borderColor := PrimaryColor - if b.context == DialogProtectedBranch { + if b.context == DialogWorktreePrompt { borderColor = WarningColor } @@ -280,8 +280,8 @@ func (b *BranchWarning) renderHeader(content *strings.Builder, modalWidth int) { titleStyle := lipgloss.NewStyle().Bold(true) switch b.context { - case DialogProtectedBranch: - content.WriteString(titleStyle.Foreground(WarningColor).Render("⚠️ Protected Branch Warning")) + case DialogWorktreePrompt: + content.WriteString(titleStyle.Foreground(WarningColor).Render("⚠️ Worktree Recommended")) content.WriteString("\n") content.WriteString(DividerStyle.Render(strings.Repeat("─", modalWidth-4))) content.WriteString("\n\n") diff --git a/internal/tui/branch_warning_test.go b/internal/tui/branch_warning_test.go index c637c477..fa327fee 100644 --- a/internal/tui/branch_warning_test.go +++ b/internal/tui/branch_warning_test.go @@ -1,19 +1,20 @@ package tui import ( + "strings" "testing" ) -func TestBranchWarningProtectedBranch(t *testing.T) { +func TestBranchWarningWorktreePrompt(t *testing.T) { bw := NewBranchWarning() bw.SetSize(80, 24) bw.SetContext("main", "auth", ".chief/worktrees/auth/") - bw.SetDialogContext(DialogProtectedBranch) + bw.SetDialogContext(DialogWorktreePrompt) bw.Reset() // Should have 4 options: worktree+branch, branch only, continue on main, cancel if len(bw.options) != 4 { - t.Fatalf("expected 4 options for protected branch, got %d", len(bw.options)) + t.Fatalf("expected 4 options for worktree prompt, got %d", len(bw.options)) } // First option should be "Create branch only" (recommended) @@ -108,7 +109,7 @@ func TestBranchWarningNavigation(t *testing.T) { bw := NewBranchWarning() bw.SetSize(80, 24) bw.SetContext("main", "auth", ".chief/worktrees/auth/") - bw.SetDialogContext(DialogProtectedBranch) + bw.SetDialogContext(DialogWorktreePrompt) bw.Reset() // Start at index 0 @@ -159,7 +160,7 @@ func TestBranchWarningBranchEdit(t *testing.T) { bw := NewBranchWarning() bw.SetSize(80, 24) bw.SetContext("main", "auth", ".chief/worktrees/auth/") - bw.SetDialogContext(DialogProtectedBranch) + bw.SetDialogContext(DialogWorktreePrompt) bw.Reset() // Default branch name @@ -206,7 +207,7 @@ func TestBranchWarningPathHints(t *testing.T) { bw := NewBranchWarning() bw.SetSize(80, 24) bw.SetContext("main", "auth", ".chief/worktrees/auth/") - bw.SetDialogContext(DialogProtectedBranch) + bw.SetDialogContext(DialogWorktreePrompt) // Check that options have correct path hints if bw.options[0].hint != "./ (current directory)" { @@ -222,7 +223,7 @@ func TestBranchWarningPathHints(t *testing.T) { func TestBranchWarningRender(t *testing.T) { // Test that Render doesn't panic for each context - contexts := []DialogContext{DialogProtectedBranch, DialogAnotherPRDRunning, DialogNoConflicts} + contexts := []DialogContext{DialogWorktreePrompt, DialogAnotherPRDRunning, DialogNoConflicts} for _, ctx := range contexts { bw := NewBranchWarning() bw.SetSize(80, 24) @@ -237,13 +238,41 @@ func TestBranchWarningRender(t *testing.T) { } } +func TestBranchWarningWorktreePromptRendersTitleAndBranchContext(t *testing.T) { + tests := []struct { + name string + branch string + }{ + {"protected branch", "main"}, + {"feature branch", "feature/x"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + bw := NewBranchWarning() + bw.SetSize(80, 24) + bw.SetContext(tt.branch, "auth", ".chief/worktrees/auth/") + bw.SetDialogContext(DialogWorktreePrompt) + bw.Reset() + + output := bw.Render() + if !strings.Contains(output, tt.branch) { + t.Errorf("expected rendered output to contain branch %q, got:\n%s", tt.branch, output) + } + if !strings.Contains(output, "Worktree Recommended") { + t.Errorf("expected rendered output to contain 'Worktree Recommended', got:\n%s", output) + } + }) + } +} + func TestBranchWarningGetDialogContext(t *testing.T) { bw := NewBranchWarning() bw.SetContext("main", "auth", ".chief/worktrees/auth/") - bw.SetDialogContext(DialogProtectedBranch) - if bw.GetDialogContext() != DialogProtectedBranch { - t.Error("expected DialogProtectedBranch") + bw.SetDialogContext(DialogWorktreePrompt) + if bw.GetDialogContext() != DialogWorktreePrompt { + t.Error("expected DialogWorktreePrompt") } bw.SetDialogContext(DialogAnotherPRDRunning) diff --git a/internal/update/update.go b/internal/update/update.go index ee21f1d7..6c28cab5 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -160,7 +160,7 @@ func PerformUpdate(currentVersion string, opts Options) (*CheckResult, error) { } // Make the new binary executable - if err := os.Chmod(tmpFile, 0o755); err != nil { + if err := os.Chmod(tmpFile, 0755); err != nil { return nil, fmt.Errorf("setting permissions on new binary: %w", err) } diff --git a/internal/update/update_test.go b/internal/update/update_test.go index 0c913e98..e4da6d2c 100644 --- a/internal/update/update_test.go +++ b/internal/update/update_test.go @@ -266,8 +266,8 @@ func TestCheckWritePermission_Success(t *testing.T) { func TestCheckWritePermission_Fail(t *testing.T) { dir := t.TempDir() - os.Chmod(dir, 0o555) - defer os.Chmod(dir, 0o755) // restore for cleanup + os.Chmod(dir, 0555) + defer os.Chmod(dir, 0755) // restore for cleanup if err := checkWritePermission(dir); err == nil { t.Error("expected write permission check to fail") @@ -320,7 +320,7 @@ func TestVerifyChecksum(t *testing.T) { dir := t.TempDir() filePath := filepath.Join(dir, "binary") content := []byte("test binary content") - os.WriteFile(filePath, content, 0o644) + os.WriteFile(filePath, content, 0644) // Calculate expected hash h := sha256.Sum256(content) @@ -340,7 +340,7 @@ func TestVerifyChecksum(t *testing.T) { func TestVerifyChecksum_Mismatch(t *testing.T) { dir := t.TempDir() filePath := filepath.Join(dir, "binary") - os.WriteFile(filePath, []byte("content"), 0o644) + os.WriteFile(filePath, []byte("content"), 0644) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, "0000000000000000000000000000000000000000000000000000000000000000 binary\n") @@ -372,7 +372,7 @@ func TestPerformUpdate_FullFlow(t *testing.T) { // Create a fake "current binary" dir := t.TempDir() binaryPath := filepath.Join(dir, "chief") - os.WriteFile(binaryPath, []byte("old binary"), 0o755) + os.WriteFile(binaryPath, []byte("old binary"), 0755) // New binary content newContent := []byte("new binary v0.6.0") From 133600a0525581f0e0eaeea1e718d9eed30bc0e6 Mon Sep 17 00:00:00 2001 From: Jeroen D Date: Wed, 29 Apr 2026 21:01:49 +0000 Subject: [PATCH 2/2] feat(tui): expose worktree prompt config in Settings overlay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add worktree.alwaysPrompt and worktree.promptBranchPattern to the Settings TUI overlay so users can edit them without leaving the app: - LoadFromConfig surfaces 5 items (was 3); the new worktree fields appear under the existing Worktree section. - ApplyToConfig copies both new keys back to Config. - ConfirmEdit now returns an error and validates regex input inline. On invalid pattern, the editor stays open with an inline "invalid regex: …" message; the previous value is preserved. Empty pattern is accepted (matches the documented opt-out semantics). - Editor app handler skips Save when ConfirmEdit returns an error, keeping the user in edit mode. DRY: extract config.ValidateBranchPattern as the single source of truth for the regex rule, used by both compilePromptRegex (load/save path) and ConfirmEdit (inline TUI validation). Includes a direct unit test covering empty / valid / invalid inputs so the contract is locked in. Docs updated: removed the "not yet exposed in the in-app Settings TUI" caveat and listed the two new editable items under the Worktree section. Co-Authored-By: Claude Opus 4.7 --- docs/reference/configuration.md | 4 +- internal/config/config.go | 16 ++- internal/config/config_test.go | 38 +++++++ internal/tui/app.go | 6 +- internal/tui/settings.go | 44 +++++++- internal/tui/settings_test.go | 192 ++++++++++++++++++++++++++------ 6 files changed, 251 insertions(+), 49 deletions(-) diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 7354dd8b..af30e658 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -37,7 +37,7 @@ onComplete: | `onComplete.push` | bool | `false` | Automatically push the branch to remote when a PRD completes | | `onComplete.createPR` | bool | `false` | Automatically create a pull request when a PRD completes (requires `gh` CLI) | -These settings are not yet exposed in the in-app Settings TUI; edit `.chief/config.yaml` directly. If you write `promptBranchPattern: ""` explicitly, Chief skips branch-name matching entirely; only `alwaysPrompt` will trigger the prompt. Default regex: `^(main|master)$`. The `\|` shown in the default column above is Markdown escaping for the table separator; the actual regex value uses an unescaped `|`. +If you write `promptBranchPattern: ""` explicitly, Chief skips branch-name matching entirely; only `alwaysPrompt` will trigger the prompt. Default regex: `^(main|master)$`. The `\|` shown in the default column above is Markdown escaping for the table separator; the actual regex value uses an unescaped `|`. ### Example Configurations @@ -81,7 +81,7 @@ Press `,` from any view in the TUI to open the Settings overlay. This provides a Settings are organized by section: -- **Worktree** — Setup command (string, editable inline) +- **Worktree** — Setup command (string, editable inline), Always prompt for worktree (toggle), Prompt branch pattern (regex, editable inline; invalid regex is rejected with an inline error so the editor stays open) - **On Complete** — Push to remote (toggle), Create pull request (toggle) Changes are saved immediately to `.chief/config.yaml` on every edit. diff --git a/internal/config/config.go b/internal/config/config.go index 61dd0e4c..49fb2ae7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -60,13 +60,19 @@ func (c *Config) Validate() error { return c.compilePromptRegex() } +// ValidateBranchPattern compiles pattern as a worktree prompt-branch regex. +// An empty pattern is valid and returns (nil, nil). The returned compile +// error is bare; callers add field-name context when surfacing it. +func ValidateBranchPattern(pattern string) (*regexp.Regexp, error) { + if pattern == "" { + return nil, nil + } + return regexp.Compile(pattern) +} + // compilePromptRegex compiles and caches the worktree prompt-branch regex. func (c *Config) compilePromptRegex() error { - if c.Worktree.PromptBranchPattern == "" { - c.promptBranchRegex = nil - return nil - } - re, err := regexp.Compile(c.Worktree.PromptBranchPattern) + re, err := ValidateBranchPattern(c.Worktree.PromptBranchPattern) if err != nil { return fmt.Errorf("invalid worktree.promptBranchPattern %q: %w", c.Worktree.PromptBranchPattern, err) } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c77ba53b..d74995fb 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -276,6 +276,44 @@ func TestValidateEmptyPatternIsValid(t *testing.T) { } } +func TestValidateBranchPattern(t *testing.T) { + t.Run("empty returns nil regex and nil error", func(t *testing.T) { + re, err := ValidateBranchPattern("") + if err != nil { + t.Fatalf("unexpected error for empty pattern: %v", err) + } + if re != nil { + t.Error("expected nil *Regexp for empty pattern") + } + }) + + t.Run("valid pattern returns compiled regex", func(t *testing.T) { + re, err := ValidateBranchPattern("^release/.*$") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if re == nil { + t.Fatal("expected non-nil *Regexp for valid pattern") + } + if !re.MatchString("release/v1") { + t.Error("expected compiled regex to match release/v1") + } + if re.MatchString("main") { + t.Error("expected compiled regex not to match main") + } + }) + + t.Run("invalid pattern returns error and nil regex", func(t *testing.T) { + re, err := ValidateBranchPattern("[unclosed") + if err == nil { + t.Fatal("expected error for invalid pattern, got nil") + } + if re != nil { + t.Error("expected nil *Regexp on validation failure") + } + }) +} + func TestLoadInvalidYAML(t *testing.T) { dir := t.TempDir() chiefDir := filepath.Join(dir, ".chief") diff --git a/internal/tui/app.go b/internal/tui/app.go index 8ac4ae0f..e995a362 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -1461,7 +1461,11 @@ func (a App) handleSettingsKeys(msg tea.KeyMsg) (tea.Model, tea.Cmd) { if a.settingsOverlay.IsEditing() { switch msg.String() { case "enter": - a.settingsOverlay.ConfirmEdit() + if err := a.settingsOverlay.ConfirmEdit(); err != nil { + // Validation rejected the value; stay in edit mode so the + // user can fix it. The overlay renders the error inline. + return a, nil + } a.settingsOverlay.ApplyToConfig(a.config) _ = config.Save(a.baseDir, a.config) return a, nil diff --git a/internal/tui/settings.go b/internal/tui/settings.go index 455a1042..b19b69f5 100644 --- a/internal/tui/settings.go +++ b/internal/tui/settings.go @@ -37,6 +37,7 @@ type SettingsOverlay struct { // Inline text editing editing bool editBuffer string + editError string // shown under the edit buffer when ConfirmEdit rejects input // GH CLI validation error ghError string @@ -58,12 +59,15 @@ func (s *SettingsOverlay) SetSize(width, height int) { func (s *SettingsOverlay) LoadFromConfig(cfg *config.Config) { s.items = []SettingsItem{ {Section: "Worktree", Label: "Setup command", Key: "worktree.setup", Type: SettingsItemString, StringVal: cfg.Worktree.Setup}, + {Section: "Worktree", Label: "Always prompt for worktree", Key: "worktree.alwaysPrompt", Type: SettingsItemBool, BoolVal: cfg.Worktree.AlwaysPrompt}, + {Section: "Worktree", Label: "Prompt branch pattern", Key: "worktree.promptBranchPattern", Type: SettingsItemString, StringVal: cfg.Worktree.PromptBranchPattern}, {Section: "On Complete", Label: "Push to remote", Key: "onComplete.push", Type: SettingsItemBool, BoolVal: cfg.OnComplete.Push}, {Section: "On Complete", Label: "Create pull request", Key: "onComplete.createPR", Type: SettingsItemBool, BoolVal: cfg.OnComplete.CreatePR}, } s.selectedIndex = 0 s.editing = false s.editBuffer = "" + s.editError = "" s.ghError = "" s.showGHError = false } @@ -74,6 +78,10 @@ func (s *SettingsOverlay) ApplyToConfig(cfg *config.Config) { switch item.Key { case "worktree.setup": cfg.Worktree.Setup = item.StringVal + case "worktree.alwaysPrompt": + cfg.Worktree.AlwaysPrompt = item.BoolVal + case "worktree.promptBranchPattern": + cfg.Worktree.PromptBranchPattern = item.StringVal case "onComplete.push": cfg.OnComplete.Push = item.BoolVal case "onComplete.createPR": @@ -106,27 +114,42 @@ func (s *SettingsOverlay) StartEditing() { if s.selectedIndex < len(s.items) && s.items[s.selectedIndex].Type == SettingsItemString { s.editing = true s.editBuffer = s.items[s.selectedIndex].StringVal + s.editError = "" } } -// ConfirmEdit saves the edit buffer to the selected item. -func (s *SettingsOverlay) ConfirmEdit() { - if s.editing && s.selectedIndex < len(s.items) { - s.items[s.selectedIndex].StringVal = s.editBuffer - s.editing = false - s.editBuffer = "" +// ConfirmEdit validates the edit buffer and, on success, writes it to the +// selected item and exits edit mode. On failure, returns the validation error +// and stays in edit mode so the user can fix the value without losing it. +func (s *SettingsOverlay) ConfirmEdit() error { + if !s.editing || s.selectedIndex >= len(s.items) { + return nil } + item := &s.items[s.selectedIndex] + if item.Key == "worktree.promptBranchPattern" { + if _, err := config.ValidateBranchPattern(s.editBuffer); err != nil { + s.editError = fmt.Sprintf("invalid regex: %v", err) + return err + } + } + item.StringVal = s.editBuffer + s.editing = false + s.editBuffer = "" + s.editError = "" + return nil } // CancelEdit discards the edit buffer. func (s *SettingsOverlay) CancelEdit() { s.editing = false s.editBuffer = "" + s.editError = "" } // AddEditChar adds a character to the edit buffer. func (s *SettingsOverlay) AddEditChar(ch rune) { s.editBuffer += string(ch) + s.editError = "" } // DeleteEditChar removes the last character from the edit buffer. @@ -135,6 +158,7 @@ func (s *SettingsOverlay) DeleteEditChar() { runes := []rune(s.editBuffer) s.editBuffer = string(runes[:len(runes)-1]) } + s.editError = "" } // ToggleBool toggles the selected boolean value. @@ -349,6 +373,14 @@ func (s *SettingsOverlay) renderItems(modalWidth int) string { result.WriteString(strings.Repeat(" ", padding)) result.WriteString(valueStr) result.WriteString("\n") + + // Inline edit error directly under the editing item. + if isSelected && s.editing && s.editError != "" { + errStyle := lipgloss.NewStyle().Foreground(ErrorColor) + result.WriteString(" ") + result.WriteString(errStyle.Render(s.editError)) + result.WriteString("\n") + } } return result.String() diff --git a/internal/tui/settings_test.go b/internal/tui/settings_test.go index 015ec8ec..c9946b7c 100644 --- a/internal/tui/settings_test.go +++ b/internal/tui/settings_test.go @@ -11,7 +11,9 @@ func TestSettingsOverlay_LoadFromConfig(t *testing.T) { s := NewSettingsOverlay() cfg := &config.Config{ Worktree: config.WorktreeConfig{ - Setup: "npm install", + Setup: "npm install", + AlwaysPrompt: false, + PromptBranchPattern: "^(main|master)$", }, OnComplete: config.OnCompleteConfig{ Push: true, @@ -20,17 +22,23 @@ func TestSettingsOverlay_LoadFromConfig(t *testing.T) { } s.LoadFromConfig(cfg) - if len(s.items) != 3 { - t.Fatalf("expected 3 items, got %d", len(s.items)) + if len(s.items) != 5 { + t.Fatalf("expected 5 items, got %d", len(s.items)) } if s.items[0].Key != "worktree.setup" || s.items[0].StringVal != "npm install" { t.Errorf("worktree.setup item: got key=%s val=%s", s.items[0].Key, s.items[0].StringVal) } - if s.items[1].Key != "onComplete.push" || !s.items[1].BoolVal { - t.Errorf("onComplete.push item: got key=%s val=%v", s.items[1].Key, s.items[1].BoolVal) + if s.items[1].Key != "worktree.alwaysPrompt" || s.items[1].BoolVal { + t.Errorf("worktree.alwaysPrompt item: got key=%s val=%v", s.items[1].Key, s.items[1].BoolVal) } - if s.items[2].Key != "onComplete.createPR" || s.items[2].BoolVal { - t.Errorf("onComplete.createPR item: got key=%s val=%v", s.items[2].Key, s.items[2].BoolVal) + if s.items[2].Key != "worktree.promptBranchPattern" || s.items[2].StringVal != "^(main|master)$" { + t.Errorf("worktree.promptBranchPattern item: got key=%s val=%s", s.items[2].Key, s.items[2].StringVal) + } + if s.items[3].Key != "onComplete.push" || !s.items[3].BoolVal { + t.Errorf("onComplete.push item: got key=%s val=%v", s.items[3].Key, s.items[3].BoolVal) + } + if s.items[4].Key != "onComplete.createPR" || s.items[4].BoolVal { + t.Errorf("onComplete.createPR item: got key=%s val=%v", s.items[4].Key, s.items[4].BoolVal) } if s.selectedIndex != 0 { t.Errorf("expected selectedIndex=0, got %d", s.selectedIndex) @@ -43,9 +51,11 @@ func TestSettingsOverlay_ApplyToConfig(t *testing.T) { s.LoadFromConfig(cfg) // Modify items - s.items[0].StringVal = "go mod download" - s.items[1].BoolVal = true - s.items[2].BoolVal = true + s.items[0].StringVal = "go mod download" // worktree.setup + s.items[1].BoolVal = true // worktree.alwaysPrompt + s.items[2].StringVal = "^release/.*$" // worktree.promptBranchPattern + s.items[3].BoolVal = true // onComplete.push + s.items[4].BoolVal = true // onComplete.createPR resultCfg := config.Default() s.ApplyToConfig(resultCfg) @@ -53,6 +63,12 @@ func TestSettingsOverlay_ApplyToConfig(t *testing.T) { if resultCfg.Worktree.Setup != "go mod download" { t.Errorf("expected setup='go mod download', got '%s'", resultCfg.Worktree.Setup) } + if !resultCfg.Worktree.AlwaysPrompt { + t.Error("expected alwaysPrompt=true") + } + if resultCfg.Worktree.PromptBranchPattern != "^release/.*$" { + t.Errorf("expected promptBranchPattern='^release/.*$', got '%s'", resultCfg.Worktree.PromptBranchPattern) + } if !resultCfg.OnComplete.Push { t.Error("expected push=true") } @@ -69,30 +85,29 @@ func TestSettingsOverlay_Navigation(t *testing.T) { t.Fatalf("expected initial index=0, got %d", s.selectedIndex) } - s.MoveDown() - if s.selectedIndex != 1 { - t.Errorf("expected index=1 after MoveDown, got %d", s.selectedIndex) - } - - s.MoveDown() - if s.selectedIndex != 2 { - t.Errorf("expected index=2 after second MoveDown, got %d", s.selectedIndex) + // Move down through every item + for i := 1; i <= 4; i++ { + s.MoveDown() + if s.selectedIndex != i { + t.Errorf("expected index=%d after MoveDown, got %d", i, s.selectedIndex) + } } // Can't go beyond last item s.MoveDown() - if s.selectedIndex != 2 { - t.Errorf("expected index=2 (clamped), got %d", s.selectedIndex) + if s.selectedIndex != 4 { + t.Errorf("expected index=4 (clamped), got %d", s.selectedIndex) } s.MoveUp() - if s.selectedIndex != 1 { - t.Errorf("expected index=1 after MoveUp, got %d", s.selectedIndex) + if s.selectedIndex != 3 { + t.Errorf("expected index=3 after MoveUp, got %d", s.selectedIndex) } // Can't go before first item - s.MoveUp() - s.MoveUp() + for i := 0; i < 10; i++ { + s.MoveUp() + } if s.selectedIndex != 0 { t.Errorf("expected index=0 (clamped), got %d", s.selectedIndex) } @@ -105,8 +120,10 @@ func TestSettingsOverlay_ToggleBool(t *testing.T) { } s.LoadFromConfig(cfg) - // Select "Push to remote" (index 1) - s.MoveDown() + // Navigate to "Push to remote" (index 3) + for i := 0; i < 3; i++ { + s.MoveDown() + } key, val := s.ToggleBool() if key != "onComplete.push" { @@ -117,11 +134,10 @@ func TestSettingsOverlay_ToggleBool(t *testing.T) { } // Toggle back - key, val = s.ToggleBool() + _, val = s.ToggleBool() if val { t.Error("expected val=false after second toggle") } - _ = key } func TestSettingsOverlay_ToggleBool_OnStringItem(t *testing.T) { @@ -142,14 +158,17 @@ func TestSettingsOverlay_RevertToggle(t *testing.T) { } s.LoadFromConfig(cfg) - s.MoveDown() // Select "Push to remote" + // Navigate to "Push to remote" (index 3) + for i := 0; i < 3; i++ { + s.MoveDown() + } s.ToggleBool() - if !s.items[1].BoolVal { + if !s.items[3].BoolVal { t.Fatal("expected true after toggle") } s.RevertToggle() - if s.items[1].BoolVal { + if s.items[3].BoolVal { t.Error("expected false after revert") } } @@ -183,7 +202,9 @@ func TestSettingsOverlay_StringEditing(t *testing.T) { t.Errorf("expected 'np' after delete, got '%s'", s.editBuffer) } - s.ConfirmEdit() + if err := s.ConfirmEdit(); err != nil { + t.Fatalf("ConfirmEdit unexpected error: %v", err) + } if s.IsEditing() { t.Fatal("should not be editing after ConfirmEdit") } @@ -214,7 +235,7 @@ func TestSettingsOverlay_CancelEdit(t *testing.T) { func TestSettingsOverlay_StartEditingOnBoolItem(t *testing.T) { s := NewSettingsOverlay() s.LoadFromConfig(config.Default()) - s.MoveDown() // Select "Push to remote" (bool) + s.MoveDown() // Select "Always prompt for worktree" (bool, index 1) s.StartEditing() if s.IsEditing() { @@ -222,6 +243,84 @@ func TestSettingsOverlay_StartEditingOnBoolItem(t *testing.T) { } } +func TestSettingsOverlay_ConfirmEdit_InvalidRegex(t *testing.T) { + s := NewSettingsOverlay() + s.LoadFromConfig(config.Default()) + + // Navigate to "Prompt branch pattern" (index 2) + s.MoveDown() + s.MoveDown() + + original := s.items[2].StringVal + s.StartEditing() + for s.editBuffer != "" { + s.DeleteEditChar() + } + for _, ch := range "[bad" { + s.AddEditChar(ch) + } + + err := s.ConfirmEdit() + if err == nil { + t.Fatal("expected error from ConfirmEdit on invalid regex, got nil") + } + if !s.IsEditing() { + t.Error("expected to remain in edit mode after rejection") + } + if s.items[2].StringVal != original { + t.Errorf("expected item value unchanged on rejection, got %q (was %q)", s.items[2].StringVal, original) + } + if !strings.Contains(s.editError, "invalid regex") { + t.Errorf("expected editError to mention invalid regex, got %q", s.editError) + } +} + +func TestSettingsOverlay_ConfirmEdit_ValidRegex(t *testing.T) { + s := NewSettingsOverlay() + s.LoadFromConfig(config.Default()) + + // Navigate to "Prompt branch pattern" (index 2) + s.MoveDown() + s.MoveDown() + s.StartEditing() + for s.editBuffer != "" { + s.DeleteEditChar() + } + for _, ch := range "^release/.*$" { + s.AddEditChar(ch) + } + + if err := s.ConfirmEdit(); err != nil { + t.Fatalf("ConfirmEdit unexpected error: %v", err) + } + if s.IsEditing() { + t.Error("expected editor to close after valid input") + } + if s.items[2].StringVal != "^release/.*$" { + t.Errorf("expected pattern saved, got %q", s.items[2].StringVal) + } +} + +func TestSettingsOverlay_ConfirmEdit_EmptyPatternIsValid(t *testing.T) { + s := NewSettingsOverlay() + s.LoadFromConfig(config.Default()) + + // Navigate to "Prompt branch pattern" (index 2) and clear it. + s.MoveDown() + s.MoveDown() + s.StartEditing() + for s.editBuffer != "" { + s.DeleteEditChar() + } + + if err := s.ConfirmEdit(); err != nil { + t.Fatalf("ConfirmEdit on empty pattern returned error: %v", err) + } + if s.items[2].StringVal != "" { + t.Errorf("expected empty pattern saved, got %q", s.items[2].StringVal) + } +} + func TestSettingsOverlay_GHError(t *testing.T) { s := NewSettingsOverlay() s.LoadFromConfig(config.Default()) @@ -288,6 +387,29 @@ func TestSettingsOverlay_Render(t *testing.T) { } } +func TestSettingsOverlay_RenderEditError(t *testing.T) { + s := NewSettingsOverlay() + s.LoadFromConfig(config.Default()) + s.SetSize(80, 24) + + // Navigate to promptBranchPattern, type a bad regex, attempt confirm. + s.MoveDown() + s.MoveDown() + s.StartEditing() + for s.editBuffer != "" { + s.DeleteEditChar() + } + for _, ch := range "[bad" { + s.AddEditChar(ch) + } + _ = s.ConfirmEdit() + + rendered := s.Render() + if !strings.Contains(rendered, "invalid regex") { + t.Error("expected rendered output to contain 'invalid regex' message") + } +} + func TestSettingsOverlay_RenderGHError(t *testing.T) { s := NewSettingsOverlay() s.LoadFromConfig(config.Default()) @@ -359,7 +481,7 @@ func TestSettingsOverlay_GetSelectedItem(t *testing.T) { s.MoveDown() item = s.GetSelectedItem() - if item.Key != "onComplete.push" { - t.Errorf("expected second item key='onComplete.push', got '%s'", item.Key) + if item.Key != "worktree.alwaysPrompt" { + t.Errorf("expected second item key='worktree.alwaysPrompt', got '%s'", item.Key) } }