Skip to content

feat(mail): manifest-declared mail.env value maps for enum-style SMTP config (#302) - #314

Open
bogdanpydev wants to merge 1 commit into
mainfrom
backend/302-mail-enum-maps
Open

feat(mail): manifest-declared mail.env value maps for enum-style SMTP config (#302)#314
bogdanpydev wants to merge 1 commit into
mainfrom
backend/302-mail-enum-maps

Conversation

@bogdanpydev

Copy link
Copy Markdown
Collaborator

Closes #302.

Generalizes BYO-mail injection so an app whose mail config is a boot-validated enum can be auto-wired, where compose environment: interpolation can only substitute / default (:-) / test-presence (:+) and cannot remap one enum's tokens onto another's. Two shipped apps are degraded purely by this — twenty's EMAIL_DRIVER (logger|smtp, boot-rejects the empty string ${MALMO_MAIL_HOST:+smtp} expands to on an unbound box) and vaultwarden's SMTP_SECURITY (off|starttls|force_tls, a value remap off malmo's none|starttls|tls).

Mechanism

The manifest declares, per app-owned mail env var, the token for each mail state; the brain resolves the final token in Go and stamps it into the .env under the app's own name — no MALMO_ indirection, the same direct-stamp convention config: uses.

mail:
  optional: true
  env:
    EMAIL_DRIVER:                                    # twenty — is a provider bound?
      from: bound                                    # domain: bound | unbound
      map: { bound: smtp, unbound: logger }
    SMTP_SECURITY:                                   # vaultwarden — the provider's mode
      from: encryption                               # domain: none | starttls | tls (unbound ⇒ none)
      map: { none: "off", starttls: starttls, tls: force_tls }
  • internal/manifestMail gains Env map[string]MailEnvMap; validateMail enforces the env name like a config app_env (uppercase, never MALMO_/loader var) and requires each map cover its from domain exactly.
  • internal/lifecycle — new mailAppEnvLines resolves each declared var; writeEnv and rewriteEnvMail stamp them in both bound and unbound states (the load-bearing change — unbound resolves the none/unbound tokens so the enum boots valid instead of rejecting on an empty value), stripping the declared vars by name on rebind so no duplicate lines are left behind.
  • DocsAPP_MANIFEST.md #D3, SERVICE_PROVISIONING.md #BYO outgoing mail, gap ledger mail-enable-enum — twenty → implemented, capabilities.yml id mail-enable-enum (version 1→2), and a progress entry.

Scope

Brain mechanism only. The catalog/ tree was cut over to the store repo, so catalog/twenty (EMAIL_DRIVER) and the store repo's apps/vaultwarden (SMTP_SECURITY) manifest wiring + the live send checks are the coordinated follow-up against this mechanism (documented in the progress entry's Known gaps).

Verification

  • make check green.
  • New-code coverage: mailAppEnvLines 100%, validateMail 100%.
  • Tests cover: both domains parse; validateMail rejects unknown from, incomplete/extra/misspelled domain keys, empty tokens, MALMO_-prefixed / lowercase / reserved names; install unbound stamps logger/off with no MALMO_MAIL_*; install bound stamps smtp/force_tls alongside MALMO_MAIL_*; rebind bound→unbound→bound re-stamps the right tokens each way with exactly one line per var; and mailAppEnvLines across all three encryption modes (guarding the manifest ↔ store.MailEncryption* alignment).

… config (#302)

Generalize BYO-mail injection so an app whose mail config is a boot-validated
enum auto-wires where compose interpolation can't remap tokens. The manifest
declares, per app-owned mail env var, the token for each mail state
(from: bound|encryption); the brain resolves the final token in Go and stamps
it under the app's own name, emitted in both bound and unbound states so the
enum is present-and-valid unbound instead of boot-rejecting on an empty value.

Closes #302
@bogdanpydev bogdanpydev self-assigned this Jul 8, 2026
@bogdanpydev
bogdanpydev requested a review from onel July 8, 2026 20:36
@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

Safe to merge; the one finding is a missing default branch in a switch that is already fully guarded by manifest validation at parse time.

The core mechanism is correct and well-tested across all mail states and rebind cycles. The only gap is that mailAppEnvLines has no default case in its em.From switch — if that branch were ever reached (e.g., via direct struct construction or a future domain added without updating the switch), it would silently produce VAR= rather than failing loudly, which is exactly the empty-value boot-reject this PR was designed to prevent. Validation makes it unreachable today, but the invariant is not self-documenting in the code.

internal/lifecycle/mail.go — the mailAppEnvLines switch is the one spot worth a second look; everything else is straightforward.

Important Files Changed

Filename Overview
internal/lifecycle/mail.go Adds mailAppEnvLines to resolve manifest-declared mail.env vars in both bound and unbound states; extends rewriteEnvMail to strip and re-stamp declared vars by name. Logic is correct, but the switch lacks a default branch — an unknown From silently produces an empty token.
internal/manifest/manifest.go Adds MailEnvMap, exported domain constants, and mailEnvDomains; extends validateMail to enforce exact domain coverage, uppercase naming, and no MALMO_/reserved-var collisions. Validation is thorough and reuses the established configEnvName / reservedConfigEnv guards.
internal/lifecycle/lifecycle.go Threads mail *manifest.Mail through writeEnv and rewriteEnvMail; correctly passes the already-loaded man.Mail at both call sites with no extra manifest reads.
internal/lifecycle/lifecycle_mail_test.go New integration tests cover install-unbound, install-bound, and three-step rebind cycle with duplicate-line and exact-token assertions; unit test for mailAppEnvLines guards all three encryption modes and the store ↔ manifest vocabulary alignment.
internal/manifest/manifest_test.go Parse roundtrip and eight rejection cases for validateMail — covers unknown from, incomplete/extra/misspelled domain keys, empty tokens, MALMO_-prefixed names, lowercase names, and reserved vars.
docs/specs/APP_MANIFEST.md New D3 subsection with working YAML examples for bound and encryption domains; clearly explains the compose-interpolation limitation that motivates the feature.
docs/specs/SERVICE_PROVISIONING.md One-paragraph addition in the BYO outgoing mail section cross-referencing APP_MANIFEST.md D3 and noting that declared vars are emitted in both states.
docs/dev/capabilities.yml Version bump 1→2 and new mail-enable-enum capability entry; matches the mechanism shipped here.

Reviews (1): Last reviewed commit: "feat(mail): manifest-declared mail.env v..." | Re-trigger Greptile

Comment on lines +63 to +79
for _, name := range names {
em := mail.Env[name]
var domainValue string
switch em.From {
case manifest.MailFromEncryption:
domainValue = store.MailEncryptionNone // unbound ⇒ no encryption
if bound != nil {
domainValue = bound.Encryption
}
case manifest.MailFromBound:
domainValue = manifest.MailUnbound
if bound != nil {
domainValue = manifest.MailBound
}
}
lines = append(lines, name+"="+em.Map[domainValue])
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent empty-token fallthrough in mailAppEnvLines. If em.From is neither MailFromEncryption nor MailFromBound, the switch falls through without setting domainValue, leaving it as "". em.Map[""] then returns the zero value (""), and the function appends VAR= — exactly the empty-value that causes a boot-reject enum app to fail. Validation via validateMail prevents this at parse time for manifests loaded with Parse(), but if a Mail struct is ever constructed directly or a new domain is added without updating this switch, the failure mode is silent rather than loud.

Suggested change
for _, name := range names {
em := mail.Env[name]
var domainValue string
switch em.From {
case manifest.MailFromEncryption:
domainValue = store.MailEncryptionNone // unbound ⇒ no encryption
if bound != nil {
domainValue = bound.Encryption
}
case manifest.MailFromBound:
domainValue = manifest.MailUnbound
if bound != nil {
domainValue = manifest.MailBound
}
}
lines = append(lines, name+"="+em.Map[domainValue])
}
for _, name := range names {
em := mail.Env[name]
var domainValue string
switch em.From {
case manifest.MailFromEncryption:
domainValue = store.MailEncryptionNone // unbound ⇒ no encryption
if bound != nil {
domainValue = bound.Encryption
}
case manifest.MailFromBound:
domainValue = manifest.MailUnbound
if bound != nil {
domainValue = manifest.MailBound
}
default:
// validateMail rejects unknown From values at parse time; this
// branch guards against direct struct construction or a future
// domain added without updating this switch.
panic("mailAppEnvLines: unhandled mail domain " + em.From)
}
lines = append(lines, name+"="+em.Map[domainValue])
}

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.

Mail: manifest-declared value maps for enum-style SMTP config (retire mail-enable-enum gap)

1 participant