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..38b592f --- /dev/null +++ b/internal/tui/screen/recipe_editor.go @@ -0,0 +1,261 @@ +package screen + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + + 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 +// 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 + } + // 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 := strings.TrimSpace(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) + 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} + }) +} + +// 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")) + // 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, body, 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. +// +// 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 { + 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 { + 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..bfa35b9 --- /dev/null +++ b/internal/tui/screen/recipe_editor_test.go @@ -0,0 +1,535 @@ +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) + } +} + +// 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") + 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..609da59 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. @@ -50,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() @@ -126,7 +145,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 +160,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 +256,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 +334,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")