Make emails account/persona centered#37
Conversation
|
I'll be honest, the rest of this was too long for me to read. Can you get an adversarial review from someone? |
|
Yes. Brownie was already requested, and I also requested an explicit adversarial pass from Ikma so this is not just my self-review. Ikma wake: Ask I sent: focus on hidden safety issues, account-selection edge cases, GPG signing policy, config mutation correctness, TOML/awk fragility, CI/template parity, and README honesty. |
ikma-ricon
left a comment
There was a problem hiding this comment.
Adversarial pass found a couple of safety/correctness issues I think should be fixed before merge.
-
Nested TOML tables are parsed/mutated as accounts.
email_account_namestreats every[accounts.*]section as a top-level account, so valid TOML like[accounts.personal.backend]becomes a second account namedpersonal.backend. Worse,emails account default personalon a nested-table config can leavepersonalwithoutdefault = trueand appenddefault = falseinside[accounts.personal.backend]. Repro:[accounts.personal] email = "user@example.com" [accounts.personal.backend] host = "imap.example.com"
then
mise run -q account:default personalproducesdefault = falseunder the backend table. If this tool claims to operate on Himalaya TOML rather than only its own generated dotted-key shape, the account parser/mutators need to distinguish exactly[accounts.<name>]from nested tables, or reject/round-trip non-supported shapes without corrupting them. -
GPG local-user is TOML-escaped, not shell-escaped. Both setup and
account:gpg:enablewritepgp.sign-cmd = "gpg --local-user '<$LOCAL_USER>' ...". A local-user containing a single quote breaks out of the shell quoting used by Himalaya commands. Example generated config from--local-user "bad' ; touch /tmp/emails-pwn ; echo '":pgp.sign-cmd = "gpg --local-user '<bad' ; touch /tmp/emails-pwn ; echo '>' --sign --quiet --armor"
This is especially sensitive because the refactor makes signing policy a first-class config mutation surface. Please either restrict the selector to a safe email/fingerprint-ish character set or build the command with real shell quoting (and add a regression for apostrophes/metacharacters).
-
Generated TOML accepts raw newlines/control chars and unquoted numeric fields.
escape_tomlonly escapes backslash and", while password/display-name/hosts/downloads-dir/GPG selector are interpolated into basic strings and--imap-port/--smtp-portare interpolated unquoted. A pasted secret or flag value with a newline can break the TOML or inject extra keys; non-numeric ports can also generate invalid/mutated TOML. This is local input, but it is the config writer for password-bearing files, so it should validate ports and reject/control-escape unsupported characters instead of writing surprising config.
I did not find a CI/template parity blocker, and the README is mostly honest about the new model. The caveat is that the README says “Himalaya config” generally; with the current awk/grep parser that is only safe for the generated top-level [accounts.name] + dotted-key shape, not arbitrary valid TOML.
Validation I ran: mise run test (92/92) on the PR branch.
|
Thanks — agreed on all three blockers. Fixed in What changed:
New regressions cover:
Validation after the fix:
Could you re-review when you have a chance? |
Summary
This refactors
emailsaround explicit email accounts/personas instead of agent identity, then updates the README and repo hygiene to match the current KKL template baseline.Core account model:
EMAILS_CONFIG,HIMALAYA_CONFIG, upward.emails/himalaya.toml, then~/.config/emails/himalaya.toml;emails account setup/list/show/default/gpg enable/status/disable;--sign/--no-signoverrides;Docs/template polish:
CONTRIBUTING.mdandmise run doctor;KnickKnackLabs/templatebaseline;git diff --checkandcodebase lint "$PWD".Validation
mise exec -- readme build --checkmise run test— 92/92mise run test-integration— 35/35bun test src/— 44/44git diff --checkcodebase lint "$PWD"— 9/9mise run doctorgit pre-commitgit pre-pushNotes
I self-reviewed against
KnickKnackLabs/templatebefore opening. That surfaced missing repo hygiene inemails(CONTRIBUTING.md,doctor, full codebase lint set, CI lint step), which is fixed here rather than left as follow-up.