OMNIS-38: Write TOML escapes instead of Go escapes for config values

CHANGELOG.md

@@ -21,6 +21,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
   output. `list --json` was not valid JSON on hosts where the package manager
   writes to stdout. `apply`, `doctor --fix`, `remove` and `uninstall` still show
   installation and hook output.
+- `omnishell set` no longer writes a `config.toml` that no command can load
+  when a value contains a BEL or vertical-tab character: control characters are
+  now written as `\u00XX`, the escape TOML defines, instead of Go's `\a`, `\v`
+  and `\x..`. Values that were not valid UTF-8 used to come back as different
+  characters; they are now rejected with exit code 2 and the file is left
+  untouched. Values that worked before are written exactly as before.
 
 ## [0.6.0] - 2026-10-07
 

internal/cli/set_toml_test.go

@@ -0,0 +1,67 @@
+package cli_test
+
+import (
+	"bytes"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/JtheGunner/omnishell/internal/cli"
+	"github.com/JtheGunner/omnishell/internal/config"
+)
+
+// A control character in a value used to be written with a Go-only escape
+// (BEL as \a, VT as \v), after which no command could load config.toml.
+func TestSetWithControlCharactersKeepsTheConfigLoadable(t *testing.T) {
+	cases := map[string]string{"BEL": "a\x07b", "VT": "a\x0bb", "ESC": "a\x1bb", "DEL": "a\x7fb", "NUL": "a\x00b"}
+	for name, value := range cases {
+		t.Run(name, func(t *testing.T) {
+			cfgPath := setTestInit(t)
+
+			var out, errb bytes.Buffer
+			if code := cli.Execute([]string{"set", "direnv.log_format", value}, &out, &errb); code != 0 {
+				t.Fatalf("set exit %d: %s", code, errb.String())
+			}
+
+			out.Reset()
+			errb.Reset()
+			if code := cli.Execute([]string{"list"}, &out, &errb); code != 0 {
+				t.Fatalf("the next command cannot load the config: exit %d: %s", code, errb.String())
+			}
+			c, err := config.Load(cfgPath)
+			if err != nil {
+				t.Fatalf("Load: %v", err)
+			}
+			if got := c.Modules["direnv"].Options["log_format"]; got != value {
+				t.Fatalf("log_format = %q, want %q", got, value)
+			}
+		})
+	}
+}
+
+// A value that is not valid UTF-8 cannot be written to a TOML file; it must be
+// refused up front instead of coming back as different characters.
+func TestSetWithInvalidUTF8IsRejectedAndLeavesTheConfigUntouched(t *testing.T) {
+	cfgPath := setTestInit(t)
+	before, err := os.ReadFile(cfgPath)
+	if err != nil {
+		t.Fatalf("read config: %v", err)
+	}
+
+	var out, errb bytes.Buffer
+	code := cli.Execute([]string{"set", "direnv.log_format", "a\xffb"}, &out, &errb)
+
+	if code != 2 {
+		t.Fatalf("exit = %d, want 2 (stderr: %s)", code, errb.String())
+	}
+	if !strings.Contains(errb.String(), "direnv.log_format") || !strings.Contains(errb.String(), "not valid UTF-8") {
+		t.Fatalf("stderr should name the option and the problem, got: %s", errb.String())
+	}
+	if out.Len() != 0 {
+		t.Fatalf("nothing to stdout on error, got: %s", out.String())
+	}
+	after, err := os.ReadFile(cfgPath)
+	if err != nil || !bytes.Equal(before, after) {
+		t.Fatalf("config.toml must be untouched (err=%v)", err)
+	}
+}

internal/config/edit.go

@@ -32,7 +32,7 @@ func SetEnabled(path, moduleID string, enabled bool) error {
 func SetOption(path, moduleID, key string, value any) error {
 	lit, err := tomlLiteral(value)
 	if err != nil {
-		return err
+		return Error{Path: path, Msg: fmt.Sprintf("invalid value for %s.%s: %v", moduleID, key, err)}
 	}
 	return editTable(path, moduleID, "options", key, lit)
 }
@@ -46,11 +46,15 @@ func tomlLiteral(value any) (string, error) {
 	case int64:
 		return strconv.FormatInt(v, 10), nil
 	case string:
-		return strconv.Quote(v), nil
+		return quoteTOML(v)
 	case []string:
 		quoted := make([]string, len(v))
 		for i, s := range v {
-			quoted[i] = strconv.Quote(s)
+			q, err := quoteTOML(s)
+			if err != nil {
+				return "", err
+			}
+			quoted[i] = q
 		}
 		return "[" + strings.Join(quoted, ", ") + "]", nil
 	default:

internal/config/literal_test.go

@@ -0,0 +1,253 @@
+package config_test
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/JtheGunner/omnishell/internal/config"
+)
+
+// newConfigFile writes the default config into a temp dir and returns its path.
+func newConfigFile(t *testing.T) string {
+	t.Helper()
+	p := filepath.Join(t.TempDir(), "config.toml")
+	if err := os.WriteFile(p, config.RenderDefault(), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	return p
+}
+
+// writtenLine returns the line `key = ...` that SetOption wrote into the file.
+func writtenLine(t *testing.T, path, key string) string {
+	t.Helper()
+	src, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, line := range strings.Split(string(src), "\n") {
+		if strings.HasPrefix(line, key+" = ") {
+			return strings.TrimPrefix(line, key+" = ")
+		}
+	}
+	t.Fatalf("no line for %q in:\n%s", key, src)
+	return ""
+}
+
+// escapesIn returns the character after every backslash in a TOML basic string
+// literal, skipping the character that a backslash itself escapes.
+func escapesIn(literal string) []byte {
+	var out []byte
+	for i := 0; i < len(literal); i++ {
+		if literal[i] == '\\' && i+1 < len(literal) {
+			out = append(out, literal[i+1])
+			i++
+		}
+	}
+	return out
+}
+
+// toml10Escapes are the only escape sequences TOML 1.0 defines for basic
+// strings: \b \t \n \f \r \" \\ and the two Unicode forms.
+const toml10Escapes = `btnfr"\uU`
+
+func assertOnlyTOMLEscapes(t *testing.T, input, literal string) {
+	t.Helper()
+	for _, c := range escapesIn(literal) {
+		if !strings.ContainsRune(toml10Escapes, rune(c)) {
+			t.Fatalf("input %q was written as %s, which uses the escape \\%c that TOML 1.0 does not have", input, literal, c)
+		}
+	}
+}
+
+// Every ASCII character, in the middle of a value: the file must load again,
+// the value must come back unchanged, and only TOML escapes may be written.
+func TestSetOptionRoundTripsEveryAsciiCharacter(t *testing.T) {
+	for r := rune(0); r <= 0x7f; r++ {
+		input := "a" + string(r) + "b"
+		p := newConfigFile(t)
+
+		if err := config.SetOption(p, "m", "k", input); err != nil {
+			t.Fatalf("%q: SetOption: %v", input, err)
+		}
+		c, err := config.Load(p)
+		if err != nil {
+			t.Fatalf("%q: the written file does not load: %v\nwritten: %s", input, err, writtenLine(t, p, "k"))
+		}
+		if got := c.Modules["m"].Options["k"]; got != input {
+			t.Fatalf("%q: came back as %q", input, got)
+		}
+		assertOnlyTOMLEscapes(t, input, writtenLine(t, p, "k"))
+	}
+}
+
+func TestSetOptionRoundTripsOtherAwkwardCharacters(t *testing.T) {
+	for _, r := range []rune{0x80, 0x85, 0x9f, 0xa0, 0xad, 0x200b, 0x2028, 0x2029, 0x202e, 0xfeff, 0xfffd, 0x1f600, 0x10ffff} {
+		input := "a" + string(r) + "b"
+		p := newConfigFile(t)
+
+		if err := config.SetOption(p, "m", "k", input); err != nil {
+			t.Fatalf("U+%04X: SetOption: %v", r, err)
+		}
+		c, err := config.Load(p)
+		if err != nil {
+			t.Fatalf("U+%04X: the written file does not load: %v", r, err)
+		}
+		if got := c.Modules["m"].Options["k"]; got != input {
+			t.Fatalf("U+%04X: came back as %q", r, got)
+		}
+		assertOnlyTOMLEscapes(t, input, writtenLine(t, p, "k"))
+	}
+}
+
+func TestSetOptionRoundTripsListElementsWithControlCharacters(t *testing.T) {
+	p := newConfigFile(t)
+	in := []string{"a\x07b", "c\x0bd", "e\x01f", "plain", `q"uo\te`}
+
+	if err := config.SetOption(p, "m", "k", in); err != nil {
+		t.Fatalf("SetOption: %v", err)
+	}
+	c, err := config.Load(p)
+	if err != nil {
+		t.Fatalf("the written file does not load: %v\nwritten: %s", err, writtenLine(t, p, "k"))
+	}
+
+	got, ok := c.Modules["m"].Options["k"].([]any)
+	if !ok || len(got) != len(in) {
+		t.Fatalf("list = %#v, want %d elements", c.Modules["m"].Options["k"], len(in))
+	}
+	for i, want := range in {
+		if got[i] != want {
+			t.Fatalf("element %d = %q, want %q", i, got[i], want)
+		}
+	}
+	assertOnlyTOMLEscapes(t, fmt.Sprint(in), writtenLine(t, p, "k"))
+}
+
+// What worked before must be written exactly as before, so existing files and
+// diffs do not change; only the Go-only escapes are replaced.
+func TestSetOptionWritesTheSameLiteralsAsBefore(t *testing.T) {
+	cases := []struct{ input, literal string }{
+		{"--height 40%", `"--height 40%"`},
+		{"a\"b\\c\nd\te", `"a\"b\\c\nd\te"`},
+		{"\b\f\r", `"\b\f\r"`},
+		{"é", `"é"`},
+		{"日本語", `"日本語"`},
+		{"😀", `"😀"`},
+		{"\u200b", `"\u200b"`},
+		{"\u2028", `"\u2028"`},
+		{"\ufeff", `"\ufeff"`},
+		{"\u0085", `"\u0085"`},
+		{"\U0010ffff", `"\U0010ffff"`},
+		{"", `""`},
+	}
+	for _, c := range cases {
+		p := newConfigFile(t)
+		if err := config.SetOption(p, "m", "k", c.input); err != nil {
+			t.Fatalf("%q: SetOption: %v", c.input, err)
+		}
+		if got := writtenLine(t, p, "k"); got != c.literal {
+			t.Fatalf("%q written as %s, want %s", c.input, got, c.literal)
+		}
+	}
+}
+
+// The characters Go writes with escapes TOML lacks become \u00XX.
+func TestSetOptionWritesGoOnlyEscapesAsUnicodeEscapes(t *testing.T) {
+	cases := []struct{ input, literal string }{
+		{"\x07", `"\u0007"`}, // BEL, Go: \a
+		{"\x0b", `"\u000b"`}, // VT, Go: \v
+		{"\x00", `"\u0000"`},
+		{"\x01", `"\u0001"`}, // Go: \x01
+		{"\x1b", `"\u001b"`}, // ESC
+		{"\x1f", `"\u001f"`},
+		{"\x7f", `"\u007f"`}, // DEL, Go: \x7f
+	}
+	for _, c := range cases {
+		p := newConfigFile(t)
+		if err := config.SetOption(p, "m", "k", c.input); err != nil {
+			t.Fatalf("%q: SetOption: %v", c.input, err)
+		}
+		if got := writtenLine(t, p, "k"); got != c.literal {
+			t.Fatalf("%q written as %s, want %s", c.input, got, c.literal)
+		}
+	}
+}
+
+// TOML text must be valid UTF-8; a value that is not cannot be written, and
+// silently turning each stray byte into another character would change it.
+func TestSetOptionRejectsInvalidUTF8AndLeavesTheFileUntouched(t *testing.T) {
+	for _, bad := range []string{"a\xffb", "a\xc3b", "\xe2\x82"} {
+		p := newConfigFile(t)
+		before, err := os.ReadFile(p)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		err = config.SetOption(p, "m", "k", bad)
+
+		var cfgErr config.Error
+		if !errors.As(err, &cfgErr) || !strings.Contains(cfgErr.Msg, "m.k") || !strings.Contains(cfgErr.Msg, "not valid UTF-8") {
+			t.Fatalf("%q: err = %v, want a config.Error naming m.k and the UTF-8 problem", bad, err)
+		}
+		after, err := os.ReadFile(p)
+		if err != nil || string(before) != string(after) {
+			t.Fatalf("%q: the file must be untouched (err=%v)", bad, err)
+		}
+	}
+}
+
+func TestSetOptionRejectsInvalidUTF8InAListElementAndWritesNothing(t *testing.T) {
+	p := newConfigFile(t)
+	before, _ := os.ReadFile(p)
+
+	err := config.SetOption(p, "m", "k", []string{"fine", "bad\xff"})
+
+	var cfgErr config.Error
+	if !errors.As(err, &cfgErr) || !strings.Contains(cfgErr.Msg, "not valid UTF-8") {
+		t.Fatalf("err = %v, want a config.Error about UTF-8", err)
+	}
+	if after, _ := os.ReadFile(p); string(before) != string(after) {
+		t.Fatal("the file must be untouched")
+	}
+}
+
+// Writing a value with a control character must not disturb the rest of the
+// file: comments, other tables and other keys stay as they were.
+func TestSetOptionWithAControlCharacterLeavesTheRestOfTheFileAlone(t *testing.T) {
+	p := filepath.Join(t.TempDir(), "config.toml")
+	original := `# my omnishell config
+[omnishell]
+version = 1
+
+[modules.direnv]
+# keep this comment
+enabled = true
+
+[modules.direnv.options]
+whitelist = ["a", "b"]
+`
+	if err := os.WriteFile(p, []byte(original), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := config.SetOption(p, "direnv", "log_format", "x\x07y"); err != nil {
+		t.Fatalf("SetOption: %v", err)
+	}
+
+	got, _ := os.ReadFile(p)
+	for _, line := range strings.Split(strings.TrimRight(original, "\n"), "\n") {
+		if !strings.Contains(string(got), line) {
+			t.Fatalf("line %q is gone:\n%s", line, got)
+		}
+	}
+	if !strings.Contains(string(got), `log_format = "x\u0007y"`) {
+		t.Fatalf("the new option is missing:\n%s", got)
+	}
+	if _, err := config.Load(p); err != nil {
+		t.Fatalf("the file must still load: %v", err)
+	}
+}