OMNIS-36: Polish the TUI after the first reviews

.gitattributes

@@ -0,0 +1,3 @@
+# The TUI golden files compare rendered screens, whose lines end in spaces.
+# Keep editors and `git diff --check` from treating them as errors.
+internal/tui/testdata/*.golden -whitespace

CHANGELOG.md

@@ -9,13 +9,19 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
   homepage, package status, platforms, shells) with a `/` filter. Space enables
   or disables the selected module in `config.toml`, like `enable` / `disable`.
   `o` edits the module's options (bool, enum, string and int), validated like
-  `omnishell set`. `a` previews the plan; confirming it closes the UI and runs `omnishell apply`,
-  which still asks before it changes anything. Modules that cannot run on this
-  host are dimmed with the reason shown. It needs an interactive terminal. It
-  brings the first runtime dependencies, Bubble Tea and Lip Gloss, so the
-  binary grows by about 1.5 MiB.
+  `omnishell set`; text fields have a movable cursor and accept pasted text.
+  `a` previews the plan; confirming it closes the UI and runs
+  `omnishell apply`, which still asks before it changes anything. Modules that
+  cannot run on this host are dimmed with the reason shown; one that only lacks
+  OS support can still be enabled, but `apply` skips it. It needs an
+  interactive terminal. It brings the first runtime dependencies, Bubble Tea
+  and Lip Gloss, so the binary grows by about 1.5 MiB.
+- `apply` now says "Nothing to apply" when it finds nothing to do, instead of
+  printing nothing.
 
 ### Fixed
+- `list` no longer prints control characters from a module manifest's text to
+  the terminal.
 - `list`, `diff` / `apply --dry-run` and `doctor` no longer print the package
   manager's answers (for example `brew list --versions fzf`) into their own
   output. `list --json` was not valid JSON on hosts where the package manager

CLAUDE.md

@@ -163,16 +163,18 @@ files matters more than any single file:
    change on one key press, string and int options are typed and validated on
    enter (a rejected value keeps the typed text), list options are shown but not
    editable. The header's change count includes options whose value differs from
-   the first value the screen showed. The plan screen (`a`) asks `Backend.Plan`, which is
-   `Engine.Preview`: the plan text plus apply's own "nothing to do" rule, so the
-   screen never promises a change apply would not make. `y` only sets
-   `Result.ApplyRequested`; once the UI has closed, `internal/cli/tui.go` runs
-   the real apply command (`handOffToApply`), so its prompt, sudo and exit codes
+   the first value the screen showed. The plan screen (`a`) asks `Backend.Plan`,
+   which is `Engine.Preview`: the plan text plus apply's own "nothing to do"
+   rule, so the screen never promises a change apply would not make. `y` or
+   Enter only sets `Result.ApplyRequested` (and only while the plan is on
+   screen); once the UI has closed, `internal/cli/tui.go` runs the real apply
+   command (`handOffToApply`), so its prompt, sudo and exit codes
    stay the gate. The UI's own engine is built with `io.Discard` writers,
    because the runner copies package-manager output to them and the UI owns the
    terminal.
    Any text that comes from a backend or a manifest (module fields, plan text,
-   error messages) must go through `sanitize` before it is drawn. Rendering
+   error messages) must go through `sanitize` (`modedit.Clean`, which `omnishell
+   list` uses too) before it is drawn. Rendering
    tests compare `View().Content` (styling stripped) with golden files in
    `internal/tui/testdata`; regenerate them with
    `go test ./internal/tui -run Golden -update` and review the diff by eye.

README.md

@@ -103,7 +103,7 @@ your system until you run `omnishell apply`.
 |----------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | `omnishell init`                       | Create the omnishell config and hook it into your shells. Idempotent.                                                                                                                                                                                                                                      | —                                                                                                                                                                                                                                                                                                               |
 | `omnishell list`                       | List known modules and their status (enabled/disabled, packages, platforms, shells, source, description).                                                                                                                                                                                                  | `--json` (emit a JSON array instead of the table; also includes each module's `homepage`)                                                                                                                                                                                                                       |
-| `omnishell tui`                        | Browse and toggle modules in a full-screen terminal UI: a module list with description, homepage, package status, platforms and shells, a `/` filter, and Space to enable or disable the selected module (written to `config.toml` at once, like `enable` / `disable`). `o` edits the module's options (bool, enum, string and int; each change is written at once and validated like `omnishell set`). `a` previews the plan; confirming it closes the UI and runs `omnishell apply`, which still asks before it changes anything. Modules that cannot run on this host are dimmed. Needs an interactive terminal (exit 2 otherwise).                                                                                          | —                                                                                                                                                                                                                                                                                                               |
+| `omnishell tui`                        | Browse and toggle modules in a full-screen terminal UI: a module list with description, homepage, package status, platforms and shells, a `/` filter, and Space to enable or disable the selected module (written to `config.toml` at once, like `enable` / `disable`). `o` edits the module's options (bool, enum, string and int; each change is written at once and validated like `omnishell set`). `a` previews the plan; confirming it closes the UI and runs `omnishell apply`, which still asks before it changes anything. Modules that cannot run on this host are dimmed with the reason; a module that only lacks OS support can still be enabled, but `apply` skips it. Needs an interactive terminal (exit 2 otherwise).                                                                                          | —                                                                                                                                                                                                                                                                                                               |
 | `omnishell enable <module>`            | Enable a module in the config. Does not apply.                                                                                                                                                                                                                                                             | —                                                                                                                                                                                                                                                                                                               |
 | `omnishell disable <module>`           | Disable a module in the config. Does not apply.                                                                                                                                                                                                                                                            | —                                                                                                                                                                                                                                                                                                               |
 | `omnishell set <module>.<key> <value>` | Set a module option in the config, validated against that module's option schema.                                                                                                                                                                                                                          | —                                                                                                                                                                                                                                                                                                               |

internal/cli/apply.go

@@ -99,6 +99,9 @@ func runApply(cmd *cobra.Command, forceDryRun bool) error {
 	}
 
 	printApplySummary(out, res)
+	if res.NothingToDo {
+		_, _ = fmt.Fprintln(out, "Nothing to apply: no module or package changes are planned.")
+	}
 
 	// --reload re-execs the login shell, but only after a fully successful,
 	// non-dry-run apply and only in an interactive session.

internal/cli/list.go

@@ -49,14 +49,16 @@ PACKAGES is one of:
 
 			rows := make([]listRow, 0, len(views))
 			for _, v := range views {
+				// Manifest text can come from a stranger's module: it must not
+				// reach the terminal as a control sequence.
 				rows = append(rows, listRow{
-					Module:      v.ID,
+					Module:      modedit.Clean(v.ID),
 					Status:      string(v.Status),
-					Description: v.Description,
-					Homepage:    v.Homepage,
+					Description: modedit.Clean(v.Description),
+					Homepage:    modedit.Clean(v.Homepage),
 					Packages:    string(v.Packages),
-					Platforms:   strings.Join(v.Platforms, ","),
-					Shells:      strings.Join(v.Shells, ","),
+					Platforms:   modedit.Clean(strings.Join(v.Platforms, ",")),
+					Shells:      modedit.Clean(strings.Join(v.Shells, ",")),
 					Src:         string(v.Origin),
 				})
 			}

internal/cli/list_test.go

@@ -117,3 +117,53 @@ func findRow(table, id string) string {
 	}
 	return ""
 }
+
+// A module manifest can come from a stranger; `list` writes its text to a
+// terminal, so control sequences in it must not survive.
+func TestListNeutralisesControlSequencesInManifestText(t *testing.T) {
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config"))
+	configDir := filepath.Join(home, ".config", "omnishell")
+	installFixtureModules(t, configDir)
+
+	manifest := filepath.Join(configDir, "modules", "completion", "manifest.toml")
+	data, err := os.ReadFile(manifest)
+	if err != nil {
+		t.Fatal(err)
+	}
+	hostile := strings.Replace(string(data), `description = "compinit"`,
+		`description = "compinit\u001b[2J\u0007\u202eevil"`, 1)
+	hostile = strings.Replace(hostile, `version     = "1.0.0"`,
+		"version     = \"1.0.0\"\nhomepage    = \"https://example.com/\\u001b]0;pwned\\u0007\"", 1)
+	if hostile == string(data) {
+		t.Fatal("the fixture manifest changed shape; adapt the test")
+	}
+	if err := os.WriteFile(manifest, []byte(hostile), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	cli.SetLookPathForTest(func(string) (string, error) { return "", os.ErrNotExist })
+	defer cli.SetLookPathForTest(nil)
+	cli.SetRunnerForTest(&pkgmgr.MockRunner{})
+	defer cli.SetRunnerForTest(nil)
+	var initOut, initErr bytes.Buffer
+	if code := cli.Execute([]string{"init"}, &initOut, &initErr); code != 0 {
+		t.Fatalf("init exit %d: %s", code, initErr.String())
+	}
+
+	for _, args := range [][]string{{"list"}, {"list", "--json"}} {
+		var out, errOut bytes.Buffer
+		if code := cli.Execute(args, &out, &errOut); code != 0 {
+			t.Fatalf("%v exit %d: %s", args, code, errOut.String())
+		}
+		for _, bad := range []string{"\x1b", "\x07", "\u202e"} {
+			if strings.Contains(out.String(), bad) {
+				t.Fatalf("%v output contains %q:\n%q", args, bad, out.String())
+			}
+		}
+		if !strings.Contains(out.String(), "compinit") {
+			t.Fatalf("%v lost the description:\n%s", args, out.String())
+		}
+	}
+}

internal/cli/tui.go

@@ -105,11 +105,12 @@ func newTUICmd() *cobra.Command {
 		Long: `Browse modules in a full-screen terminal UI.
 
 Shows every known module with its description, homepage, package status,
-platforms and shells. Move with the arrow keys, press space to enable or disable
-the selected module, press o to edit its options, type / to filter, q to quit. Space writes config.toml at
-once, exactly like 'omnishell enable' and 'disable'; it never touches your
-shells. Press a to preview the plan; confirming it closes the UI and runs
-'omnishell apply', which still asks before it changes anything.
+platforms and shells. Move with the arrow keys, press space to enable or
+disable the selected module, press o to edit its options, type / to filter,
+q to quit. Space writes config.toml at once, exactly like 'omnishell enable'
+and 'disable'; it never touches your shells. Press a to preview the plan;
+confirming it closes the UI and runs 'omnishell apply', which still asks
+before it changes anything.
 
 Needs an interactive terminal; in scripts use 'omnishell list'.`,
 		Args: cobra.NoArgs,

internal/cli/tui_test.go

@@ -11,6 +11,7 @@ import (
 	"sort"
 	"strings"
 	"testing"
+	"unicode/utf8"
 
 	"github.com/JtheGunner/omnishell/internal/cli"
 	"github.com/JtheGunner/omnishell/internal/config"
@@ -532,3 +533,42 @@ func TestTUIBackendOptionsOfAnUnknownModuleIsAnError(t *testing.T) {
 		t.Fatalf("err = %v, want unknown module", err)
 	}
 }
+
+// The plan said there was something to apply, but the config changed in
+// between and apply finds nothing to do: that must be said, not left silent.
+func TestTUIHandoffSaysWhenApplyFindsNothingToDo(t *testing.T) {
+	zshHostWithCompletion(t)
+	handoffRun(t, tui.Result{ApplyRequested: true}, nil)
+	askedQuestions(t, true)
+
+	var out, errb bytes.Buffer
+	if code := cli.Execute([]string{"tui"}, &out, &errb); code != 0 {
+		t.Fatalf("first run: exit = %d (stderr: %s)", code, errb.String())
+	}
+	if strings.Contains(out.String(), "Nothing to apply") {
+		t.Fatalf("the first apply had work to do, but said otherwise:\n%s", out.String())
+	}
+
+	out.Reset()
+	errb.Reset()
+	if code := cli.Execute([]string{"tui"}, &out, &errb); code != 0 {
+		t.Fatalf("second run: exit = %d (stderr: %s)", code, errb.String())
+	}
+	if !strings.Contains(out.String(), "Nothing to apply: no module or package changes are planned.") {
+		t.Fatalf("a hand-off that finds nothing to do must say so; stdout:\n%s", out.String())
+	}
+}
+
+// A help line longer than a classic 80-column terminal wraps in the middle of a
+// word.
+func TestTUIHelpFitsAnEightyColumnTerminal(t *testing.T) {
+	var out, errb bytes.Buffer
+	if code := cli.Execute([]string{"tui", "--help"}, &out, &errb); code != 0 {
+		t.Fatalf("exit = %d (stderr: %s)", code, errb.String())
+	}
+	for _, line := range strings.Split(out.String(), "\n") {
+		if n := utf8.RuneCountInString(line); n > 80 {
+			t.Errorf("help line is %d characters wide: %q", n, line)
+		}
+	}
+}

internal/engine/apply.go

@@ -52,6 +52,9 @@ type Result struct {
 	InitFilesWritten []string
 	BackupDir        string
 	Changed          bool
+	// NothingToDo is true when apply took its idempotent no-op path: no planned
+	// changes and init and rc files that still match the lockfile.
+	NothingToDo bool
 }
 
 // Apply brings the system to the desired state described by cfg. It loads the
@@ -89,6 +92,7 @@ func (e Engine) Apply(cfg config.Config, cfgPath, lockPath string, opts ApplyOpt
 			res.Modules = summarise(plan, pd)
 			return res, ErrDegraded
 		}
+		res.NothingToDo = true
 		return res, nil
 	}
 

internal/modedit/clean.go

@@ -0,0 +1,23 @@
+package modedit
+
+import (
+	"strings"
+	"unicode"
+)
+
+// Clean replaces control characters (ESC, BEL, CR, newline, tab, C1) and bidi
+// override characters with a space, so text that comes from a module manifest
+// can neither act as a terminal command nor reorder the text around it. Every
+// front end that prints manifest text to a terminal runs it through Clean.
+func Clean(s string) string {
+	return strings.Map(func(r rune) rune {
+		if unicode.IsControl(r) || isBidiControl(r) {
+			return ' '
+		}
+		return r
+	}, s)
+}
+
+func isBidiControl(r rune) bool {
+	return (r >= 0x202A && r <= 0x202E) || (r >= 0x2066 && r <= 0x2069)
+}

internal/modedit/clean_test.go

@@ -0,0 +1,20 @@
+package modedit
+
+import "testing"
+
+func TestCleanReplacesControlAndBidiCharactersWithSpaces(t *testing.T) {
+	cases := map[string]string{
+		"plain text":              "plain text",
+		"ä日 ok":                   "ä日 ok",
+		"a\x1b]0;x\x07b":          "a ]0;x b",
+		"a\x1b[2Jb":               "a [2Jb",
+		"one\ntwo\r\tthree":       "one two  three",
+		"a\u202etxt\u2066b\u2069": "a txt b ",
+		"c1\u009bcontrol":         "c1 control",
+	}
+	for in, want := range cases {
+		if got := Clean(in); got != want {
+			t.Errorf("Clean(%q) = %q, want %q", in, got, want)
+		}
+	}
+}

internal/modedit/views.go

@@ -177,7 +177,8 @@ func (ed Editor) packageState(mf module.Manifest) PackageState {
 func (ed Editor) unavailableReason(cfg config.Config, mf module.Manifest) string {
 	osName := string(ed.Engine.Platform.OS)
 	if !contains(mf.Platforms, osName) {
-		return fmt.Sprintf("not supported on %s (module supports %s)", osName, strings.Join(mf.Platforms, ", "))
+		return fmt.Sprintf("not supported on %s (module supports %s); you can enable it, but apply skips it",
+			osName, strings.Join(mf.Platforms, ", "))
 	}
 	managed := engine.ManagedShells(cfg, ed.Engine.Platform)
 	if !sharesAny(mf.Shells, managed) {

internal/modedit/views_test.go

@@ -224,7 +224,7 @@ func TestViewsExplainAModuleThatNeedsAnotherOS(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Views: %v", err)
 	}
-	want := "not supported on linux (module supports macos)"
+	want := "not supported on linux (module supports macos); you can enable it, but apply skips it"
 	if got := viewByID(t, views, "macosonly").Unavailable; got != want {
 		t.Fatalf("Unavailable = %q, want %q", got, want)
 	}

internal/tui/helpers_test.go

@@ -201,6 +201,20 @@ func key(name string) tea.KeyPressMsg {
 		return tea.KeyPressMsg{Code: tea.KeySpace, Text: " "}
 	case "backspace":
 		return tea.KeyPressMsg{Code: tea.KeyBackspace}
+	case "left":
+		return tea.KeyPressMsg{Code: tea.KeyLeft}
+	case "right":
+		return tea.KeyPressMsg{Code: tea.KeyRight}
+	case "home":
+		return tea.KeyPressMsg{Code: tea.KeyHome}
+	case "end":
+		return tea.KeyPressMsg{Code: tea.KeyEnd}
+	case "delete":
+		return tea.KeyPressMsg{Code: tea.KeyDelete}
+	case "ctrl+a":
+		return tea.KeyPressMsg{Code: 'a', Mod: tea.ModCtrl}
+	case "ctrl+e":
+		return tea.KeyPressMsg{Code: 'e', Mod: tea.ModCtrl}
 	case "ctrl+c":
 		return tea.KeyPressMsg{Code: 'c', Mod: tea.ModCtrl}
 	default:
@@ -219,6 +233,12 @@ func press(t *testing.T, m Model, names ...string) Model {
 	return m
 }
 
+// paste delivers text the way a terminal with bracketed paste does.
+func paste(m Model, text string) Model {
+	next, _ := m.Update(tea.PasteMsg{Content: text})
+	return next.(Model)
+}
+
 // sized returns m after a terminal of w x h cells has been reported.
 func sized(m Model, w, h int) Model {
 	next, _ := m.Update(tea.WindowSizeMsg{Width: w, Height: h})