Skip to content

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

Merged
JtheGunner merged 1 commit into
mainfrom
fix/OMNIS-38-valid-toml-for-control-chars
Oct 7, 2026
Merged

JtheGunner merged 1 commit into
mainfrom
fix/OMNIS-38-valid-toml-for-control-chars

Conversation

@JtheGunner

Copy link
Copy Markdown
Owner

YouTrack: https://youtrack.xn--wrm-hoa.li/issue/OMNIS-38

omnishell set <module>.<key> <value> could write a config.toml that no command could load afterwards, when the value contained a BEL (0x07) or vertical-tab (0x0b) character. Every command then failed until the file was fixed by hand.

Cause

config.tomlLiteral rendered strings with strconv.Quote, which writes a Go literal. Go's escapes are not TOML's: Go spells BEL as \a, VT as \v and other control characters (and invalid UTF-8 bytes) as \xHH. TOML 1.0 has none of these. Measured over 140 inputs (every ASCII character plus awkward Unicode and invalid UTF-8):

  • BEL and VT: the written file does not load (invalid escape).
  • Other control characters and DEL: the file loads, but only because the parser in use also accepts \xHH, which is not TOML 1.0.
  • Invalid UTF-8 ($'\xff'): the file loads, but the value silently comes back as a different character (ÿ).
  • The remaining 136 inputs already round-tripped.

Changes

  • New internal/config/quote.go with quoteTOML. For everything that is valid in both languages it writes exactly what strconv.Quote wrote (\", \\, \n, \t, \r, \b, \f, printable characters as they are, other non-printable characters as \uXXXX / \UXXXXXXXX), so existing files and diffs do not change. Every other control character, and DEL, is written as \u00XX.
  • A value that is not valid UTF-8 cannot be written to a TOML file and is rejected with a config.Error (exit code 2, invalid value for <module>.<key>: value is not valid UTF-8); the file is left untouched.
  • tomlLiteral uses it for strings and for list elements. SetOption reports a literal error as a config.Error, like the other value errors.
  • CHANGELOG.md: entry under Fixed.

Decision on the open question in the issue: escape rather than reject. Newline and tab are already accepted and written as \n / \t, so rejecting all control characters would break values that work today; escaping loses nothing and is valid TOML. Only what TOML cannot represent (invalid UTF-8) is rejected.

Commits

  • fix: write TOML escapes instead of Go escapes for config values

Test plan

  • make vet and make test (race detector) pass locally.
  • golangci-lint v2.13.2 (the CI version, via go run) reports 0 issues.
  • CGO_ENABLED=0 go build ./cmd/omnishell succeeds for linux/amd64, linux/arm64, linux/armv6, linux/armv7, darwin/amd64 and darwin/arm64; go.mod and go.sum are unchanged.
  • New tests in internal/config/literal_test.go: round trip SetOption then Load for every ASCII character and for awkward Unicode (only TOML 1.0 escapes may appear in the written line), list elements, the exact literal for everything that worked before, \u00XX for the Go-only escapes, rejection of invalid UTF-8 for strings and list elements with the file left untouched, and comments and other lines left alone.
  • New CLI tests in internal/cli/set_toml_test.go: omnishell set with BEL, VT, ESC, DEL and NUL leaves a config the next command can load; invalid UTF-8 gives exit code 2 and an untouched file. They fail against the old edit.go for BEL, VT and invalid UTF-8.
  • With real binaries in a throwaway HOME: the old build writes "a\ab" and omnishell validate then reports a TOML error; this branch writes "a\u0007b" and validate reports the file as valid; a\xffb is rejected with exit 2.
  • CI: vet, race tests, golangci-lint, govulncheck and the build matrix pass.
  • Reviewer: in a scratch HOME (never a real config), run omnishell init, then omnishell set direnv.log_format $'a\ab' and omnishell validate.

Known gaps (not in this PR)

  • Files that were already broken by the old behaviour are not repaired; the parser's message names the line and key.
  • Existing files that contain \xHH escapes keep loading with omnishell's parser (no migration); another TOML 1.0 parser would reject them. Only values written from now on are TOML 1.0 clean.

strconv.Quote writes Go literals: BEL as \a, VT as \v, other control characters as \xHH. TOML 1.0 has none of these, so a config.toml holding a BEL or VT no longer loaded, and an invalid UTF-8 byte written as \xff came back as a different character. quoteTOML writes the same text as before for everything valid in both languages and \u00XX for the rest, and rejects invalid UTF-8.
@JtheGunner
JtheGunner merged commit e757633 into main Oct 7, 2026
4 checks passed
@JtheGunner
JtheGunner deleted the fix/OMNIS-38-valid-toml-for-control-chars branch October 7, 2026 13:38
@JtheGunner JtheGunner mentioned this pull request Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant