From 810b72ac4f6f549acf8abedae02092470332d6c1 Mon Sep 17 00:00:00 2001 From: Juergen Klaassen Date: Fri, 22 May 2026 16:00:14 -0600 Subject: [PATCH 1/3] feat(tui): recipe edit via $EDITOR handoff from list screen (KLA-399) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes KLA-399 with a smaller, more durable approach than the ticket's original multi-pane Bubbletea editor. Two new keys in the recipe list screen: - n: write a starter YAML to RecipesDir() (collision-safe filename) and open it in $EDITOR. After the editor returns, re-parse and validate via recipe.Validate(); reload the list so the new recipe appears. Invalid YAML / failed validation leaves the file on disk so the operator's work is never lost. - e: edit the highlighted recipe. User-authored recipes open in place; built-ins prompt "Save 'X' as user copy before editing?" with an inline y/N confirm so the operator stays in the list. Confirming runs MarshalYAML → write to RecipesDir() → edit the copy. Why this over a multi-pane Bubbletea editor: - Preserves YAML comments and ordering, addressing the trade-off the ticket explicitly called out about yaml.Marshal round-tripping. - ~10x less code to maintain than the multi-pane editor would have been (3 files vs ~10, no form-state machine, no live YAML preview pane to keep in sync). - $VISUAL/$EDITOR is what TUI users already have configured for every other YAML they touch — meets them where they are. Editor resolution mirrors git's convention: $VISUAL > $EDITOR > platform default (vi / notepad). Whitespace-splitting on the editor string honors common "wait" wrappers like 'code -w' and 'subl -w'. Tests (16 new): - slugForFilename / uniquePath unit cases - writeStarterRecipe produces parseable, validating YAML; collision-safe - saveBuiltinAsUserCopy round-trips through MarshalYAML - validateEditedFile classifies parse vs validate failures - n / e keys; builtin confirm flow (y vs n); editor-finished reload + flash - resolveEditor precedence and fallback execEditor is a package-level var so tests can swap it for a function that mutates the file without actually launching a child process. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/QUICKSTART.md | 2 + internal/tui/screen/recipe_editor.go | 232 ++++++++++++ internal/tui/screen/recipe_editor_test.go | 421 ++++++++++++++++++++++ internal/tui/screen/recipe_list.go | 110 +++++- 4 files changed, 764 insertions(+), 1 deletion(-) create mode 100644 internal/tui/screen/recipe_editor.go create mode 100644 internal/tui/screen/recipe_editor_test.go diff --git a/docs/QUICKSTART.md b/docs/QUICKSTART.md index 8323a11..5aa4e0b 100644 --- a/docs/QUICKSTART.md +++ b/docs/QUICKSTART.md @@ -146,6 +146,8 @@ jc recipe run onboard-user \ jc recipe run onboard-user --plan # Preview only ``` +Author recipes from the TUI without leaving your terminal: `jc tui` → Recipes → press **`n`** for a new recipe (opens a starter YAML in `$EDITOR`) or **`e`** to edit the highlighted recipe. Built-in recipes prompt to save a user copy first. On save, jc re-parses and validates the file; invalid recipes stay on disk so your work is never lost. `$VISUAL` is honored ahead of `$EDITOR`; falls back to `vi` (or `notepad` on Windows). + ### MCP Server (for AI Assistants) Add to Claude Desktop config (`claude_desktop_config.json`): diff --git a/internal/tui/screen/recipe_editor.go b/internal/tui/screen/recipe_editor.go new file mode 100644 index 0000000..5e55de6 --- /dev/null +++ b/internal/tui/screen/recipe_editor.go @@ -0,0 +1,232 @@ +package screen + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + + tea "github.com/charmbracelet/bubbletea" + "github.com/klaassen-consulting/jc/internal/recipe" +) + +// editorFinishedMsg is delivered after $EDITOR returns. path is the file +// the editor was pointed at; err is non-nil only when the child process +// itself failed to launch or returned a non-zero exit. Parse/validate +// errors live in the post-load handling — this message just signals +// "editor session ended, time to re-check the file." +type editorFinishedMsg struct { + path string + err error +} + +// recipeStarterTemplate is the YAML body written to a freshly-created +// recipe file. Intentionally minimal so the user's first edit replaces +// most of it. Uses YAML comments to point at reference material — those +// stick around because we're handing the file off to $EDITOR untouched, +// not round-tripping it through yaml.Marshal. +const recipeStarterTemplate = `# New jc recipe. Edit, save, and exit your editor to validate. +# Reference: built-in recipes embedded in jc (jc tui → Recipes → list). +# Spec: docs/QUICKSTART.md → Recipes. + +name: untitled +description: "" +author: "" +version: 0.1.0 +tags: [] + +parameters: + # - name: username + # description: Account to operate on + # type: string + # required: true + +steps: + - name: example + command: jc users list --filter "activated:eq:false" --ids + # when: '{{ .username }}' + # capture: user_ids + # continue_on_error: false +` + +// resolveEditor picks the command to launch for $EDITOR handoff. Order +// matches the de-facto convention `git commit` uses: $VISUAL beats +// $EDITOR (full-screen editors typically set VISUAL), then a platform +// default. We deliberately don't error when neither var is set — most +// systems have `vi` (or notepad on Windows), and a clear "could not +// launch editor" error from the OS is more actionable than a refusal +// here would be. +// +// editorOverride lets tests inject a known-good binary (e.g. a shell +// script that mutates the file) without touching env vars. +var editorOverride string + +func resolveEditor() string { + if editorOverride != "" { + return editorOverride + } + if v := os.Getenv("VISUAL"); v != "" { + return v + } + if v := os.Getenv("EDITOR"); v != "" { + return v + } + if runtime.GOOS == "windows" { + return "notepad" + } + return "vi" +} + +// execEditor is overridable for tests. The default runs the resolved +// editor against path via tea.ExecProcess, which suspends the bubbletea +// runtime so the editor takes over the controlling terminal. +var execEditor = func(path string) tea.Cmd { + editor := resolveEditor() + // Some editors (VS Code's `code -w`, sublime's `subl -w`) need a + // "wait" flag to block; we honor whatever the user put in $EDITOR + // verbatim by splitting on whitespace — same convention git uses. + fields := strings.Fields(editor) + cmd := exec.Command(fields[0], append(fields[1:], path)...) + return tea.ExecProcess(cmd, func(err error) tea.Msg { + return editorFinishedMsg{path: path, err: err} + }) +} + +// userRecipeFile returns the path on disk of a user recipe with the +// given name, or "" if not found. Scans RecipesDir() and parses each +// candidate just enough to match the internal recipe.Name. Necessary +// because recipe.Recipe doesn't carry its source path through Load*. +func userRecipeFile(name string) string { + dir := recipe.RecipesDir() + entries, err := os.ReadDir(dir) + if err != nil { + return "" + } + for _, e := range entries { + if e.IsDir() { + continue + } + if !strings.HasSuffix(strings.ToLower(e.Name()), ".yaml") && + !strings.HasSuffix(strings.ToLower(e.Name()), ".yml") { + continue + } + path := filepath.Join(dir, e.Name()) + r, err := recipe.ParseFile(path) + if err != nil { + continue + } + if r.Name == name { + return path + } + } + return "" +} + +// slugForFilename converts a recipe name into a filesystem-safe slug. +// Used when writing new files (e.g. saving a builtin as a user copy or +// renaming an "untitled" file after the user picked a real name in +// their editor). Keep it conservative: lowercase, letters / digits / +// hyphens / underscores only. +func slugForFilename(name string) string { + var b strings.Builder + prev := byte(0) + for i := 0; i < len(name); i++ { + c := name[i] + switch { + case c >= 'A' && c <= 'Z': + c += 32 + fallthrough + case (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '-' || c == '_': + b.WriteByte(c) + default: + // Coalesce any run of non-safe chars into a single hyphen. + if prev != '-' { + b.WriteByte('-') + c = '-' + } else { + continue + } + } + prev = c + } + out := strings.Trim(b.String(), "-_") + if out == "" { + return "untitled" + } + return out +} + +// uniquePath returns base if it doesn't exist; otherwise appends -2, +// -3, ... before the extension until it finds a free slot. Avoids +// silently clobbering an existing file when the operator hits `n` +// twice and we'd otherwise overwrite their first attempt. +func uniquePath(base string) string { + if _, err := os.Stat(base); os.IsNotExist(err) { + return base + } + ext := filepath.Ext(base) + stem := strings.TrimSuffix(base, ext) + for i := 2; i < 1000; i++ { + candidate := fmt.Sprintf("%s-%d%s", stem, i, ext) + if _, err := os.Stat(candidate); os.IsNotExist(err) { + return candidate + } + } + // Astonishingly unlikely; defer to caller with the un-incremented + // path and let os.WriteFile fail loudly if it really is taken. + return base +} + +// writeStarterRecipe creates a new YAML file under RecipesDir() with +// the starter template. Returns the actual path written (may differ +// from desired if a unique-suffix was applied). Caller is responsible +// for opening it in the editor. +func writeStarterRecipe() (string, error) { + dir := recipe.RecipesDir() + if err := os.MkdirAll(dir, 0o700); err != nil { + return "", fmt.Errorf("creating recipes dir: %w", err) + } + path := uniquePath(filepath.Join(dir, "untitled.yaml")) + if err := os.WriteFile(path, []byte(recipeStarterTemplate), 0o600); err != nil { + return "", fmt.Errorf("writing starter recipe: %w", err) + } + return path, nil +} + +// saveBuiltinAsUserCopy serializes the named builtin to RecipesDir() +// so the operator can edit a copy without touching the embedded +// original. Returns the path of the new file. The MarshalYAML +// reformatting cost is documented on the prompt — operators who want +// pristine YAML start a new file with the starter template instead. +func saveBuiltinAsUserCopy(r *recipe.Recipe) (string, error) { + dir := recipe.RecipesDir() + if err := os.MkdirAll(dir, 0o700); err != nil { + return "", fmt.Errorf("creating recipes dir: %w", err) + } + path := uniquePath(filepath.Join(dir, slugForFilename(r.Name)+".yaml")) + yaml, err := recipe.MarshalYAML(r) + if err != nil { + return "", fmt.Errorf("marshaling builtin recipe: %w", err) + } + if err := os.WriteFile(path, yaml, 0o600); err != nil { + return "", fmt.Errorf("writing user copy: %w", err) + } + return path, nil +} + +// validateEditedFile re-parses + re-validates a file after the editor +// returned. Returns a user-facing message — empty string when valid. +// Doesn't delete the file on invalid YAML; the operator's work is +// preserved on disk so they can re-open and fix it. +func validateEditedFile(path string) string { + r, err := recipe.ParseFile(path) + if err != nil { + return fmt.Sprintf("Saved %s but YAML parse failed: %v", filepath.Base(path), err) + } + if err := r.Validate(); err != nil { + return fmt.Sprintf("Saved %s but recipe is invalid: %v", filepath.Base(path), err) + } + return "" +} diff --git a/internal/tui/screen/recipe_editor_test.go b/internal/tui/screen/recipe_editor_test.go new file mode 100644 index 0000000..fa9708b --- /dev/null +++ b/internal/tui/screen/recipe_editor_test.go @@ -0,0 +1,421 @@ +package screen + +import ( + "os" + "path/filepath" + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" + "github.com/klaassen-consulting/jc/internal/recipe" + "github.com/klaassen-consulting/jc/internal/tui" +) + +// withTempRecipesDir points recipe.RecipesDir at a per-test tmpdir so +// edit-flow tests don't touch the operator's real ~/.config/jc/recipes. +func withTempRecipesDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + orig := recipe.RecipesDir + recipe.RecipesDir = func() string { return dir } + t.Cleanup(func() { recipe.RecipesDir = orig }) + return dir +} + +// withFakeEditor installs an execEditor that mutates the file before +// completing — letting tests drive the after-edit path without actually +// suspending bubbletea or invoking an external program. +func withFakeEditor(t *testing.T, mutate func(path string)) { + t.Helper() + orig := execEditor + execEditor = func(path string) tea.Cmd { + return func() tea.Msg { + if mutate != nil { + mutate(path) + } + return editorFinishedMsg{path: path} + } + } + t.Cleanup(func() { execEditor = orig }) +} + +func TestSlugForFilename(t *testing.T) { + cases := []struct{ in, want string }{ + {"simple", "simple"}, + {"Mixed Case", "mixed-case"}, + {"with.dots and/slashes", "with-dots-and-slashes"}, + {"hyphenated-name", "hyphenated-name"}, + {"___underscores___", "underscores"}, + {"!!!", "untitled"}, + {"", "untitled"}, + {" spaces ", "spaces"}, + {"123-numeric", "123-numeric"}, + } + for _, tc := range cases { + got := slugForFilename(tc.in) + if got != tc.want { + t.Errorf("slugForFilename(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +func TestUniquePath_AppendsSuffixOnCollision(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "rec.yaml") + + // No collision → returns the original path. + if got := uniquePath(base); got != base { + t.Errorf("uniquePath(non-existent) = %q, want %q", got, base) + } + + // First collision → -2 suffix. + if err := os.WriteFile(base, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + first := uniquePath(base) + want := filepath.Join(dir, "rec-2.yaml") + if first != want { + t.Errorf("first collision = %q, want %q", first, want) + } + + // Second collision → -3 suffix. + if err := os.WriteFile(first, []byte("y"), 0o600); err != nil { + t.Fatal(err) + } + second := uniquePath(base) + want = filepath.Join(dir, "rec-3.yaml") + if second != want { + t.Errorf("second collision = %q, want %q", second, want) + } +} + +func TestWriteStarterRecipe_ProducesParseableSkeleton(t *testing.T) { + dir := withTempRecipesDir(t) + + path, err := writeStarterRecipe() + if err != nil { + t.Fatalf("writeStarterRecipe: %v", err) + } + if !strings.HasPrefix(path, dir) { + t.Errorf("starter path %q not under RecipesDir %q", path, dir) + } + r, err := recipe.ParseFile(path) + if err != nil { + t.Fatalf("starter YAML must parse: %v", err) + } + if err := r.Validate(); err != nil { + t.Errorf("starter recipe must validate: %v", err) + } +} + +func TestWriteStarterRecipe_AvoidsClobberingExisting(t *testing.T) { + withTempRecipesDir(t) + + first, err := writeStarterRecipe() + if err != nil { + t.Fatal(err) + } + second, err := writeStarterRecipe() + if err != nil { + t.Fatal(err) + } + if first == second { + t.Errorf("second writeStarterRecipe overwrote the first at %q", first) + } +} + +func TestSaveBuiltinAsUserCopy_WritesValidYAML(t *testing.T) { + dir := withTempRecipesDir(t) + r := &recipe.Recipe{ + Name: "Audit MFA", + Description: "Check MFA coverage", + Steps: []recipe.Step{{Name: "list", Command: "jc users list --filter mfa:eq:false"}}, + } + + path, err := saveBuiltinAsUserCopy(r) + if err != nil { + t.Fatalf("saveBuiltinAsUserCopy: %v", err) + } + if filepath.Dir(path) != dir { + t.Errorf("copy written to %q, want under %q", path, dir) + } + if !strings.HasSuffix(path, "audit-mfa.yaml") { + t.Errorf("copy filename = %q, want slugified", path) + } + loaded, err := recipe.ParseFile(path) + if err != nil { + t.Fatalf("user copy must parse: %v", err) + } + if loaded.Name != "Audit MFA" { + t.Errorf("loaded.Name = %q, want %q", loaded.Name, "Audit MFA") + } +} + +func TestValidateEditedFile(t *testing.T) { + dir := t.TempDir() + + // Valid recipe → empty string (no error message). + good := filepath.Join(dir, "good.yaml") + if err := os.WriteFile(good, []byte("name: ok\nsteps:\n - name: s\n command: jc users list\n"), 0o600); err != nil { + t.Fatal(err) + } + if msg := validateEditedFile(good); msg != "" { + t.Errorf("valid recipe should produce empty message; got %q", msg) + } + + // Malformed YAML → "YAML parse failed" message. + bad := filepath.Join(dir, "bad.yaml") + if err := os.WriteFile(bad, []byte("name: : :\n"), 0o600); err != nil { + t.Fatal(err) + } + if msg := validateEditedFile(bad); !strings.Contains(msg, "parse failed") { + t.Errorf("malformed YAML should mention parse failure; got %q", msg) + } + + // Parseable but invalid (no steps) → "recipe is invalid" message. + invalid := filepath.Join(dir, "invalid.yaml") + if err := os.WriteFile(invalid, []byte("name: empty\nsteps: []\n"), 0o600); err != nil { + t.Fatal(err) + } + if msg := validateEditedFile(invalid); !strings.Contains(msg, "invalid") { + t.Errorf("validation failure should mention invalid; got %q", msg) + } +} + +func TestRecipeListScreen_NewKeyOpensEditorOnStarter(t *testing.T) { + withTempRecipesDir(t) + withRecipeLoaders(t, sampleRecipes(), sampleRecipes()) + + var calledPath string + withFakeEditor(t, func(path string) { calledPath = path }) + + s := NewRecipeListScreen() + s.Init() + s.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + + _, cmd := s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("n")}) + if cmd == nil { + t.Fatal("n should produce a command") + } + msg := cmd() + if _, ok := msg.(editorFinishedMsg); !ok { + t.Fatalf("n should fire editor; cmd returned %T", msg) + } + if !strings.HasSuffix(calledPath, ".yaml") { + t.Errorf("editor invoked with %q, want a .yaml file", calledPath) + } +} + +func TestRecipeListScreen_EditUserRecipeOpensEditor(t *testing.T) { + dir := withTempRecipesDir(t) + + // Write a user recipe to disk and have the loader return it as user-authored. + userPath := filepath.Join(dir, "onboard-user.yaml") + yaml := "name: onboard-user\nsteps:\n - name: create\n command: jc users create\n" + if err := os.WriteFile(userPath, []byte(yaml), 0o600); err != nil { + t.Fatal(err) + } + user, err := recipe.ParseFile(userPath) + if err != nil { + t.Fatal(err) + } + withRecipeLoaders(t, []*recipe.Recipe{user}, nil) // empty builtin → user-authored + + var calledPath string + withFakeEditor(t, func(path string) { calledPath = path }) + + s := NewRecipeListScreen() + s.Init() + s.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + + _, cmd := s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("e")}) + if cmd == nil { + t.Fatal("e should produce a command on a user recipe") + } + msg := cmd() + if _, ok := msg.(editorFinishedMsg); !ok { + t.Fatalf("e should fire editor; cmd returned %T", msg) + } + if calledPath != userPath { + t.Errorf("editor invoked with %q, want %q", calledPath, userPath) + } +} + +func TestRecipeListScreen_EditBuiltinPromptsForCopy(t *testing.T) { + withTempRecipesDir(t) + // Same recipe in both 'all' and 'builtin' loaders → flagged builtin. + all := sampleRecipes() + withRecipeLoaders(t, all, all) + withFakeEditor(t, nil) + + s := NewRecipeListScreen() + s.Init() + s.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + + // `e` on a builtin should set the confirm prompt rather than + // immediately launching the editor. + _, cmd := s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("e")}) + if cmd != nil { + t.Errorf("e on builtin should defer editor until confirm; got cmd %T", cmd()) + } + if s.confirmPrompt == "" { + t.Error("e on builtin should set a confirm prompt") + } + if !strings.Contains(s.confirmPrompt, "user copy") { + t.Errorf("confirm prompt should mention 'user copy'; got %q", s.confirmPrompt) + } +} + +func TestRecipeListScreen_ConfirmYTriggersCopyAndEdit(t *testing.T) { + dir := withTempRecipesDir(t) + all := sampleRecipes() + withRecipeLoaders(t, all, all) + + var calledPath string + withFakeEditor(t, func(path string) { calledPath = path }) + + s := NewRecipeListScreen() + s.Init() + s.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + + // Trigger the prompt, then accept. + s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("e")}) + _, cmd := s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("y")}) + if cmd == nil { + t.Fatal("y on confirm should launch editor") + } + if _, ok := cmd().(editorFinishedMsg); !ok { + t.Fatalf("y on confirm should fire editor; got %T", cmd()) + } + if !strings.HasPrefix(calledPath, dir) { + t.Errorf("editor path %q not under user RecipesDir %q", calledPath, dir) + } + if s.confirmPrompt != "" { + t.Errorf("confirm prompt should clear after y; still %q", s.confirmPrompt) + } +} + +func TestRecipeListScreen_ConfirmNCancelsWithoutEditing(t *testing.T) { + withTempRecipesDir(t) + all := sampleRecipes() + withRecipeLoaders(t, all, all) + + editorFired := false + withFakeEditor(t, func(string) { editorFired = true }) + + s := NewRecipeListScreen() + s.Init() + s.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + + s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("e")}) + _, cmd := s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("n")}) + if cmd != nil { + // A nil cmd is fine; if non-nil, draining it must NOT fire the editor. + msg := cmd() + if _, ok := msg.(editorFinishedMsg); ok { + t.Errorf("n on confirm should NOT trigger editor; got editorFinishedMsg") + } + } + if editorFired { + t.Error("n on confirm should leave editor unfired") + } + if s.confirmPrompt != "" { + t.Errorf("confirm prompt should clear after n; still %q", s.confirmPrompt) + } +} + +func TestRecipeListScreen_EditorFinishedReloadsAndFlashes(t *testing.T) { + dir := withTempRecipesDir(t) + + user := &recipe.Recipe{ + Name: "before-edit", + Steps: []recipe.Step{{Name: "s", Command: "jc users list"}}, + } + withRecipeLoaders(t, []*recipe.Recipe{user}, nil) + + s := NewRecipeListScreen() + s.Init() + s.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + + // Now swap the loader so the post-edit reload sees a different + // recipe — simulating the editor having created/renamed something. + after := &recipe.Recipe{ + Name: "after-edit", + Steps: []recipe.Step{{Name: "s", Command: "jc users list"}}, + } + recipeLoader = func() ([]*recipe.Recipe, error) { return []*recipe.Recipe{after}, nil } + + // Drop a valid file at the path the editor "edited" so + // validateEditedFile returns the success message. + finished := editorFinishedMsg{path: filepath.Join(dir, "x.yaml")} + if err := os.WriteFile(finished.path, + []byte("name: ok\nsteps:\n - name: s\n command: jc users list\n"), 0o600); err != nil { + t.Fatal(err) + } + + _, cmd := s.Update(finished) + if cmd == nil { + t.Fatal("editorFinishedMsg should yield a flash command") + } + if _, ok := cmd().(tui.FlashMsg); !ok { + t.Fatalf("expected FlashMsg, got %T", cmd()) + } + // Reload must have picked up the new loader's recipe. + view := s.View() + if !strings.Contains(view, "after-edit") { + t.Errorf("view after reload missing 'after-edit'; got:\n%s", view) + } +} + +func TestRecipeListScreen_EditorFinishedSurfacesValidationError(t *testing.T) { + dir := withTempRecipesDir(t) + withRecipeLoaders(t, nil, nil) + + s := NewRecipeListScreen() + s.Init() + s.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + + bad := filepath.Join(dir, "broken.yaml") + if err := os.WriteFile(bad, []byte("name: : :\n"), 0o600); err != nil { + t.Fatal(err) + } + + _, cmd := s.Update(editorFinishedMsg{path: bad}) + if cmd == nil { + t.Fatal("expected flash command") + } + flash, ok := cmd().(tui.FlashMsg) + if !ok { + t.Fatalf("expected FlashMsg, got %T", cmd()) + } + if !strings.Contains(flash.Text, "parse failed") { + t.Errorf("flash should describe parse failure; got %q", flash.Text) + } +} + +func TestResolveEditor_VisualBeatsEditor(t *testing.T) { + t.Setenv("VISUAL", "code --wait") + t.Setenv("EDITOR", "vi") + if got := resolveEditor(); got != "code --wait" { + t.Errorf("resolveEditor() = %q, want %q (VISUAL wins)", got, "code --wait") + } +} + +func TestResolveEditor_EditorWhenNoVisual(t *testing.T) { + t.Setenv("VISUAL", "") + t.Setenv("EDITOR", "nano") + if got := resolveEditor(); got != "nano" { + t.Errorf("resolveEditor() = %q, want nano", got) + } +} + +func TestResolveEditor_FallsBackToPlatformDefault(t *testing.T) { + t.Setenv("VISUAL", "") + t.Setenv("EDITOR", "") + got := resolveEditor() + // Default is "vi" on unix, "notepad" on windows. Both are + // acceptable — the contract is just "non-empty, predictable". + if got != "vi" && got != "notepad" { + t.Errorf("resolveEditor() = %q, want vi or notepad", got) + } +} diff --git a/internal/tui/screen/recipe_list.go b/internal/tui/screen/recipe_list.go index 951e0c6..8fe32aa 100644 --- a/internal/tui/screen/recipe_list.go +++ b/internal/tui/screen/recipe_list.go @@ -2,6 +2,7 @@ package screen import ( "fmt" + "path/filepath" "sort" "strings" @@ -35,6 +36,14 @@ type RecipeListScreen struct { height int err string loaded bool + + // confirmPrompt is non-empty when the screen is asking the operator + // to confirm a one-off action (currently: "save builtin X as user + // copy before editing?"). y/Y triggers confirmAction; anything else + // dismisses without running it. Inline state instead of a sub-screen + // keeps the `e` flow on builtins to a single keypress. + confirmPrompt string + confirmAction func() (tea.Model, tea.Cmd) } // NewRecipeListScreen creates a recipe list screen. @@ -126,7 +135,13 @@ func (s *RecipeListScreen) Update(msg tea.Msg) (tea.Model, tea.Cmd) { s.height = msg.Height return s, nil + case editorFinishedMsg: + return s.handleEditorFinished(msg) + case tea.KeyMsg: + if s.confirmPrompt != "" { + return s.updateConfirmMode(msg) + } if s.filtering { return s.updateFilterMode(msg) } @@ -135,6 +150,44 @@ func (s *RecipeListScreen) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return s, nil } +// updateConfirmMode handles the y/n response to a confirmation prompt. +// y/Y runs the deferred action; everything else dismisses without action. +func (s *RecipeListScreen) updateConfirmMode(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch msg.String() { + case "y", "Y": + action := s.confirmAction + s.confirmPrompt = "" + s.confirmAction = nil + if action != nil { + return action() + } + return s, nil + default: + s.confirmPrompt = "" + s.confirmAction = nil + return s, nil + } +} + +// handleEditorFinished re-validates the edited file, reloads the +// catalog so the list reflects the new recipe (or the now-valid edits), +// and surfaces parse/validation errors as a flash. The file is left on +// disk in both success and failure paths — the operator's work is never +// deleted just because they introduced a typo. +func (s *RecipeListScreen) handleEditorFinished(msg editorFinishedMsg) (tea.Model, tea.Cmd) { + if msg.err != nil { + return s, func() tea.Msg { + return tui.FlashMsg{Text: fmt.Sprintf("Editor exited with error: %v", msg.err)} + } + } + flashText := validateEditedFile(msg.path) + if flashText == "" { + flashText = fmt.Sprintf("Saved %s", filepath.Base(msg.path)) + } + s.loadRecipes() + return s, func() tea.Msg { return tui.FlashMsg{Text: flashText} } +} + func (s *RecipeListScreen) updateFilterMode(msg tea.KeyMsg) (tea.Model, tea.Cmd) { switch msg.String() { case "esc": @@ -193,6 +246,58 @@ func (s *RecipeListScreen) updateBrowseMode(msg tea.KeyMsg) (tea.Model, tea.Cmd) case "r": s.loadRecipes() return s, func() tea.Msg { return tui.FlashMsg{Text: "Recipes reloaded"} } + case "n": + return s.startNewRecipe() + case "e": + return s.editSelected() + } + return s, nil +} + +// startNewRecipe writes the starter template to RecipesDir() under a +// unique untitled-*.yaml filename and hands the file to $EDITOR. After +// the editor returns, handleEditorFinished re-parses, validates, and +// reloads the catalog so the new recipe appears in the list. +func (s *RecipeListScreen) startNewRecipe() (tea.Model, tea.Cmd) { + path, err := writeStarterRecipe() + if err != nil { + return s, func() tea.Msg { + return tui.FlashMsg{Text: fmt.Sprintf("Could not create recipe: %v", err)} + } + } + return s, execEditor(path) +} + +// editSelected opens the currently-highlighted recipe in $EDITOR. For +// user-authored recipes it edits the file in place; for built-ins, we +// can't edit the embedded source, so prompt to save a user copy first +// — the operator's choice keeps the path explicit instead of silently +// shadowing a builtin. +func (s *RecipeListScreen) editSelected() (tea.Model, tea.Cmd) { + if s.cursor < 0 || s.cursor >= len(s.filtered) { + return s, nil + } + selected := s.filtered[s.cursor] + if s.userRecipes[selected.Name] { + path := userRecipeFile(selected.Name) + if path == "" { + return s, func() tea.Msg { + return tui.FlashMsg{Text: fmt.Sprintf("Could not locate %q in %s", selected.Name, recipe.RecipesDir())} + } + } + return s, execEditor(path) + } + // Builtin: queue a save-as-copy confirmation. The deferred action + // runs MarshalYAML → write → edit when the operator presses 'y'. + s.confirmPrompt = fmt.Sprintf("Save %q as user copy before editing? [y/N]", selected.Name) + s.confirmAction = func() (tea.Model, tea.Cmd) { + path, err := saveBuiltinAsUserCopy(selected) + if err != nil { + return s, func() tea.Msg { + return tui.FlashMsg{Text: fmt.Sprintf("Could not save user copy: %v", err)} + } + } + return s, execEditor(path) } return s, nil } @@ -219,7 +324,10 @@ func (s *RecipeListScreen) View() string { return sb.String() } - if s.filtering { + if s.confirmPrompt != "" { + sb.WriteString(style.FilterInput.Render(s.confirmPrompt)) + sb.WriteString("\n") + } else if s.filtering { s.filter.Width = s.width - 4 sb.WriteString(style.FilterInput.Render(s.filter.View())) sb.WriteString("\n") From 9210fe7e23906808ae0e53eb6d10398cd29cae56 Mon Sep 17 00:00:00 2001 From: Juergen Klaassen Date: Fri, 22 May 2026 18:25:33 -0600 Subject: [PATCH 2/3] fix(tui): Bugbot findings on recipe edit handoff (KLA-399) Three findings on PR #35, all real: 1. **Validation errors mislabeled as YAML parse failures (Medium).** recipe.ParseFile already runs Validate() internally, so validateEditedFile's first err branch caught structural failures (no steps, unnamed step, missing command) and labeled them as "YAML parse failed". The second Validate() call was dead. Fix: split the read + unmarshal + validate steps inline so each branch reports the right cause. Pinned by TestValidateEditedFile_ClassifiesParseVsValidateDistinctly. 2. **Confirm mode didn't suppress app-level 'q' quit (Medium).** TextInputActive returned only s.filtering, so pressing 'q' while the builtin-edit confirm prompt was visible quit the entire TUI instead of dismissing the prompt via updateConfirmMode's default branch. Fix: return s.filtering || s.confirmPrompt != "" so the global Quit shortcut is suppressed while a prompt is awaiting a y/N response. Pinned by TestRecipeListScreen_TextInputActiveCoversConfirmPrompt. 3. **Whitespace-only $VISUAL/$EDITOR caused index panic (Low).** resolveEditor's v != "" check let a " " value through; execEditor's fields[0] then panicked. Fix: TrimSpace the env reads (so whitespace-only is treated as unset and we fall through to the platform default), and add a defensive len(fields) == 0 guard in execEditor that surfaces a friendly error-bearing editorFinishedMsg instead of panicking. Pinned by TestResolveEditor_WhitespaceOnlyTreatedAsUnset and TestExecEditor_EmptyResolutionSurfacesError. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/tui/screen/recipe_editor.go | 31 +++++- internal/tui/screen/recipe_editor_test.go | 114 ++++++++++++++++++++++ internal/tui/screen/recipe_list.go | 12 ++- 3 files changed, 153 insertions(+), 4 deletions(-) diff --git a/internal/tui/screen/recipe_editor.go b/internal/tui/screen/recipe_editor.go index 5e55de6..042ccbe 100644 --- a/internal/tui/screen/recipe_editor.go +++ b/internal/tui/screen/recipe_editor.go @@ -10,6 +10,7 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/klaassen-consulting/jc/internal/recipe" + "go.yaml.in/yaml/v3" ) // editorFinishedMsg is delivered after $EDITOR returns. path is the file @@ -67,10 +68,15 @@ func resolveEditor() string { if editorOverride != "" { return editorOverride } - if v := os.Getenv("VISUAL"); v != "" { + // TrimSpace on the env-var values: a whitespace-only $VISUAL (e.g. + // " ") used to pass the empty check, then strings.Fields would + // return an empty slice in execEditor and fields[0] panicked. Treat + // whitespace-only the same as unset so we fall through to the + // platform default. + if v := strings.TrimSpace(os.Getenv("VISUAL")); v != "" { return v } - if v := os.Getenv("EDITOR"); v != "" { + if v := strings.TrimSpace(os.Getenv("EDITOR")); v != "" { return v } if runtime.GOOS == "windows" { @@ -88,6 +94,15 @@ var execEditor = func(path string) tea.Cmd { // "wait" flag to block; we honor whatever the user put in $EDITOR // verbatim by splitting on whitespace — same convention git uses. fields := strings.Fields(editor) + if len(fields) == 0 { + // resolveEditor's TrimSpace + platform default should make this + // unreachable, but belt-and-suspenders: surface an editorFinishedMsg + // with err set so handleEditorFinished produces a sensible flash + // instead of panicking the bubbletea program. + return func() tea.Msg { + return editorFinishedMsg{path: path, err: fmt.Errorf("no editor configured ($VISUAL/$EDITOR empty)")} + } + } cmd := exec.Command(fields[0], append(fields[1:], path)...) return tea.ExecProcess(cmd, func(err error) tea.Msg { return editorFinishedMsg{path: path, err: err} @@ -220,9 +235,19 @@ func saveBuiltinAsUserCopy(r *recipe.Recipe) (string, error) { // returned. Returns a user-facing message — empty string when valid. // Doesn't delete the file on invalid YAML; the operator's work is // preserved on disk so they can re-open and fix it. +// +// Why this doesn't call recipe.ParseFile: that helper folds yaml +// unmarshal + Validate() into a single error, so a structural failure +// (no steps, unnamed step) would surface here as the YAML-parse branch +// — Bugbot flagged the mislabel on the first version. Splitting the +// two steps keeps the message accurate. func validateEditedFile(path string) string { - r, err := recipe.ParseFile(path) + data, err := os.ReadFile(path) if err != nil { + return fmt.Sprintf("Could not read %s: %v", filepath.Base(path), err) + } + var r recipe.Recipe + if err := yaml.Unmarshal(data, &r); err != nil { return fmt.Sprintf("Saved %s but YAML parse failed: %v", filepath.Base(path), err) } if err := r.Validate(); err != nil { diff --git a/internal/tui/screen/recipe_editor_test.go b/internal/tui/screen/recipe_editor_test.go index fa9708b..bfa35b9 100644 --- a/internal/tui/screen/recipe_editor_test.go +++ b/internal/tui/screen/recipe_editor_test.go @@ -393,6 +393,120 @@ func TestRecipeListScreen_EditorFinishedSurfacesValidationError(t *testing.T) { } } +// Bugbot finding on PR #35: validateEditedFile used recipe.ParseFile +// which already calls Validate() internally, so a parseable-but-invalid +// recipe (no steps, unnamed step, ...) surfaced as the YAML-parse +// branch instead of the validate branch. Pin the distinction by hand +// so we don't regress. +func TestValidateEditedFile_ClassifiesParseVsValidateDistinctly(t *testing.T) { + dir := t.TempDir() + + // Malformed YAML: 'name: : :' is a syntax error → parse branch. + bad := filepath.Join(dir, "bad.yaml") + if err := os.WriteFile(bad, []byte("name: : :\n"), 0o600); err != nil { + t.Fatal(err) + } + parseMsg := validateEditedFile(bad) + if !strings.Contains(parseMsg, "YAML parse failed") { + t.Errorf("malformed YAML should hit the parse branch; got %q", parseMsg) + } + if strings.Contains(parseMsg, "recipe is invalid") { + t.Errorf("malformed YAML must NOT label as 'recipe is invalid'; got %q", parseMsg) + } + + // Valid YAML but no steps → structural failure → validate branch. + noSteps := filepath.Join(dir, "no-steps.yaml") + if err := os.WriteFile(noSteps, []byte("name: thing\nsteps: []\n"), 0o600); err != nil { + t.Fatal(err) + } + validateMsg := validateEditedFile(noSteps) + if !strings.Contains(validateMsg, "recipe is invalid") { + t.Errorf("no-steps YAML should hit the validate branch; got %q", validateMsg) + } + if strings.Contains(validateMsg, "YAML parse failed") { + t.Errorf("structurally-invalid YAML must NOT label as parse failure; got %q", validateMsg) + } + + // Valid YAML, named recipe, but the step itself is invalid (missing + // command) → validate branch as well. + noCmd := filepath.Join(dir, "no-cmd.yaml") + if err := os.WriteFile(noCmd, []byte("name: t\nsteps:\n - name: s\n"), 0o600); err != nil { + t.Fatal(err) + } + stepMsg := validateEditedFile(noCmd) + if !strings.Contains(stepMsg, "recipe is invalid") { + t.Errorf("missing-command step should hit the validate branch; got %q", stepMsg) + } +} + +// Bugbot finding on PR #35: TextInputActive ignored confirm-prompt +// state, so pressing 'q' while the builtin-edit prompt was up quit +// the entire TUI instead of dismissing the prompt. +func TestRecipeListScreen_TextInputActiveCoversConfirmPrompt(t *testing.T) { + withTempRecipesDir(t) + all := sampleRecipes() + withRecipeLoaders(t, all, all) + withFakeEditor(t, nil) + + s := NewRecipeListScreen() + s.Init() + s.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + + if s.TextInputActive() { + t.Fatal("TextInputActive should be false in idle browse mode") + } + + // Trigger the builtin-edit confirm prompt. + s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("e")}) + if s.confirmPrompt == "" { + t.Fatal("e on builtin should set confirm prompt (test setup precondition)") + } + if !s.TextInputActive() { + t.Error("TextInputActive must return true while confirm prompt is visible so global 'q' is suppressed") + } + + // Dismiss with anything-but-y. + s.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("n")}) + if s.TextInputActive() { + t.Error("TextInputActive should return false after the prompt is dismissed") + } +} + +// Bugbot finding on PR #35: execEditor used fields[0] without +// checking len(fields), so a whitespace-only $VISUAL/$EDITOR caused +// an index panic. +func TestResolveEditor_WhitespaceOnlyTreatedAsUnset(t *testing.T) { + t.Setenv("VISUAL", " ") + t.Setenv("EDITOR", "\t \n") + got := resolveEditor() + if got != "vi" && got != "notepad" { + t.Errorf("whitespace-only $VISUAL/$EDITOR should fall through to platform default; got %q", got) + } +} + +func TestExecEditor_EmptyResolutionSurfacesError(t *testing.T) { + // Force the resolver to return whitespace (which strings.Fields + // will reduce to an empty slice). The defensive len check in + // execEditor must produce a friendly error-bearing + // editorFinishedMsg rather than panicking. + origOverride := editorOverride + editorOverride = " " + t.Cleanup(func() { editorOverride = origOverride }) + + cmd := execEditor("/tmp/whatever.yaml") + if cmd == nil { + t.Fatal("execEditor with empty editor should still produce a tea.Cmd") + } + msg := cmd() + finished, ok := msg.(editorFinishedMsg) + if !ok { + t.Fatalf("expected editorFinishedMsg, got %T", msg) + } + if finished.err == nil { + t.Error("editorFinishedMsg.err should describe the empty-editor failure") + } +} + func TestResolveEditor_VisualBeatsEditor(t *testing.T) { t.Setenv("VISUAL", "code --wait") t.Setenv("EDITOR", "vi") diff --git a/internal/tui/screen/recipe_list.go b/internal/tui/screen/recipe_list.go index 8fe32aa..609da59 100644 --- a/internal/tui/screen/recipe_list.go +++ b/internal/tui/screen/recipe_list.go @@ -59,7 +59,17 @@ func NewRecipeListScreen() *RecipeListScreen { func (s *RecipeListScreen) Title() string { return "Recipes" } -func (s *RecipeListScreen) TextInputActive() bool { return s.filtering } +// TextInputActive includes both filter mode and the confirm prompt: +// the app uses this to decide whether to swallow single-key globals +// like 'q' (quit) before the screen sees them. Filter mode obviously +// needs that suppression; the confirm prompt does too, because 'q' +// while the prompt is up should dismiss the prompt (via the default +// branch in updateConfirmMode) rather than quit the whole TUI. +// Bugbot caught the regression on PR #35 — the original commit only +// returned s.filtering and confirm-mode 'q' quit the app. +func (s *RecipeListScreen) TextInputActive() bool { + return s.filtering || s.confirmPrompt != "" +} func (s *RecipeListScreen) Init() tea.Cmd { s.loadRecipes() From 281596c037827dd4d23699a7bea50cedf126a099 Mon Sep 17 00:00:00 2001 From: Juergen Klaassen Date: Fri, 22 May 2026 18:34:54 -0600 Subject: [PATCH 3/3] fix(tui): avoid yaml package shadowing in saveBuiltinAsUserCopy (Bugbot KLA-399) The previous Bugbot fix added `go.yaml.in/yaml/v3` to the file-level imports (for validateEditedFile's split parse/validate path), at which point the existing `yaml` local variable in saveBuiltinAsUserCopy started shadowing the package. Renamed to `body` so a future yaml.Unmarshal addition inside the function doesn't fail with a confusing "yaml is not a package" compile error. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/tui/screen/recipe_editor.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/tui/screen/recipe_editor.go b/internal/tui/screen/recipe_editor.go index 042ccbe..38b592f 100644 --- a/internal/tui/screen/recipe_editor.go +++ b/internal/tui/screen/recipe_editor.go @@ -221,11 +221,15 @@ func saveBuiltinAsUserCopy(r *recipe.Recipe) (string, error) { return "", fmt.Errorf("creating recipes dir: %w", err) } path := uniquePath(filepath.Join(dir, slugForFilename(r.Name)+".yaml")) - yaml, err := recipe.MarshalYAML(r) + // Don't name this `yaml` — that shadows the package-level import + // of go.yaml.in/yaml/v3 used by validateEditedFile, and anyone + // later adding a yaml.Unmarshal call in this function would get a + // confusing "yaml is not a package" compile error. + body, err := recipe.MarshalYAML(r) if err != nil { return "", fmt.Errorf("marshaling builtin recipe: %w", err) } - if err := os.WriteFile(path, yaml, 0o600); err != nil { + if err := os.WriteFile(path, body, 0o600); err != nil { return "", fmt.Errorf("writing user copy: %w", err) } return path, nil