Skip to content

Commit 5bdb783

Browse files
committed
fix(cli): carry config.email.persist to EmailServicePlugin (#5447)
`EmailServiceConfigSchema` declares `persist` and the generated reference documents it as "Persist to sys_email (default true)". `EmailServicePlugin` honours the constructor option — it builds no `EmailPersistence` when `persist === false`. The segment between them did not exist: `resolveEmailCapabilityArg` is the only reader `config.email` has in the repo, and it read every declared key except this one. A deployment that wrote `email: { persist: false }` to keep message bodies out of the database type-checked, parsed and read as configured, and went on writing every subject, body and recipient to `sys_email` — Prime Directive #10's declared != enforced, with a PII blast radius. ADR-0049 enforce-or-remove, answered "enforce". Resolution order, per setting, as the rest of this resolver: OS_EMAIL_PERSIST_ENABLED > config.email.persist > default (persist ON). The new env var reads the same truth table as OS_EMAIL_QUEUE_ENABLED, now a shared `envBooleanFlag` helper rather than two copies of the list. It is tri-state so an unset variable falls through to config instead of reading as false. Declaring neither source leaves the key out of the constructor options entirely, so the plugin default decides and existing deployments are unchanged. Tests boot the real plugin over the real resolver output and read back whether an EmailPersistence was built, rather than re-pinning the key name against itself. Reverse verification: deleting the wiring turns 8 of the 13 red; the 5 that stay green are the default-behaviour pins plus two completeness cases whose expectation coincides with the plugin default — annotated in the file so they are not misread as discriminating. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh
1 parent b375f08 commit 5bdb783

3 files changed

Lines changed: 286 additions & 3 deletions

File tree

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
'@objectstack/cli': patch
3+
---
4+
5+
`config.email.persist` now actually reaches the email plugin (#5447)
6+
7+
`EmailServiceConfigSchema` has always declared `persist` — the generated
8+
reference documents it as "Persist to sys_email (default true)" — and
9+
`EmailServicePlugin` has always honoured the constructor option, building no
10+
`EmailPersistence` when `persist === false`. What did not exist was the segment
11+
between them: `resolveEmailCapabilityArg` in `os serve` is the only reader
12+
`config.email` has, and it read every declared key except this one.
13+
14+
So a deployment that wrote `email: { persist: false }` to keep message bodies
15+
out of the database type-checked, parsed, and read as configured — and went on
16+
writing every subject, body and recipient to `sys_email`. Operators who
17+
switched persistence off for PII reasons were not getting what the contract
18+
promised. **If you rely on that row being written, no action is needed; if you
19+
had declared `persist: false` and audited on the assumption it took effect,
20+
those rows exist and are worth reviewing.**
21+
22+
Resolution order, per setting, matching the rest of this resolver:
23+
24+
OS_EMAIL_PERSIST_ENABLED > config.email.persist > default (persist ON)
25+
26+
`OS_EMAIL_PERSIST_ENABLED` is new, and reads the same truth table as
27+
`OS_EMAIL_QUEUE_ENABLED` (`1`/`true`/`yes`/`on`, case- and space-insensitive);
28+
that table is now one shared helper instead of two copies. Unlike the queue
29+
flag it is default-ON, because it does not enable a capability — it is the off
30+
switch for one that has always been on. A deployment that declares neither
31+
source is byte-for-byte unchanged: the key is left out of the plugin's
32+
constructor options entirely and the plugin's own default decides.
Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// framework#5447 — `config.email.persist` reaches `EmailServicePlugin`.
4+
//
5+
// `EmailServiceConfigSchema` has declared `persist` ("Persist to sys_email
6+
// (default true)") for as long as the plugin has honoured it, and the plugin
7+
// half was never in doubt: it builds no `EmailPersistence` when
8+
// `persist === false`, and says so in its own queue-delivery diagnostics. What
9+
// did not exist was the segment between them. `config.email` has exactly one
10+
// reader in the repo — `resolveEmailCapabilityArg`, whose return value IS the
11+
// plugin's constructor argument (see the `cap === 'email'` arm of the
12+
// capability loop) — and it read every declared key except this one.
13+
//
14+
// So the key was declared, typed, parsed, and documented, and a PII-sensitive
15+
// deployment that wrote `persist: false` to keep message bodies out of the
16+
// database kept writing every body to `sys_email`. That is Prime Directive
17+
// #10's declared != enforced with a privacy blast radius, and ADR-0049's
18+
// enforce-or-remove was answered "enforce".
19+
//
20+
// The end-to-end block below is the point of this file. Asserting only that
21+
// the resolver emits `persist: false` would re-pin the same declaration twice
22+
// (a resolver key name against a test's expectation of that key name) and
23+
// could not tell whether the plugin reads `persist` at all — which is exactly
24+
// the class of gap being closed. So it boots the REAL `EmailServicePlugin`
25+
// over the resolver's REAL output and asks the plugin what it built.
26+
27+
import { describe, it, expect, vi } from 'vitest';
28+
import { EmailServicePlugin } from '@objectstack/plugin-email';
29+
import { resolveEmailCapabilityArg } from './serve.js';
30+
31+
// ── resolver ───────────────────────────────────────────────────────────────
32+
33+
describe('resolveEmailCapabilityArg — sys_email persistence (#5447)', () => {
34+
it('leaves persist unset when neither config nor env declares it', () => {
35+
// The no-regression case: absent, not `false`. An option nobody wrote must
36+
// not appear in what the plugin is constructed with, because the plugin's
37+
// default (persist ON) is what every existing deployment is running and
38+
// emitting `persist: true` here would only look equivalent.
39+
expect(resolveEmailCapabilityArg({}, {})).not.toHaveProperty('options.persist');
40+
});
41+
42+
it('carries config.email.persist:false through to the constructor options', () => {
43+
const { options } = resolveEmailCapabilityArg({ persist: false }, {});
44+
expect(options.persist).toBe(false);
45+
});
46+
47+
it('carries an explicit config.email.persist:true too', () => {
48+
const { options } = resolveEmailCapabilityArg({ persist: true }, {});
49+
expect(options.persist).toBe(true);
50+
});
51+
52+
it('reads OS_EMAIL_PERSIST_ENABLED on the same truth table as OS_EMAIL_QUEUE_ENABLED', () => {
53+
// One truth table for both flags, which is why `envBooleanFlag` was
54+
// extracted instead of the list being written a second time: an operator
55+
// who learned `on` works for the queue flag must not find it silently
56+
// means "off" here.
57+
for (const on of ['1', 'true', 'TRUE', 'yes', 'on', ' On ']) {
58+
expect(resolveEmailCapabilityArg({}, { OS_EMAIL_PERSIST_ENABLED: on }).options.persist, on)
59+
.toBe(true);
60+
}
61+
for (const off of ['0', 'false', 'no', 'off', '']) {
62+
expect(resolveEmailCapabilityArg({}, { OS_EMAIL_PERSIST_ENABLED: off }).options.persist, off)
63+
.toBe(false);
64+
}
65+
});
66+
67+
it('lets env override config in BOTH directions', () => {
68+
// Both directions, because a one-way test passes just as well against a
69+
// resolver that ignores config entirely.
70+
expect(
71+
resolveEmailCapabilityArg({ persist: true }, { OS_EMAIL_PERSIST_ENABLED: 'false' })
72+
.options.persist,
73+
).toBe(false);
74+
expect(
75+
resolveEmailCapabilityArg({ persist: false }, { OS_EMAIL_PERSIST_ENABLED: 'true' })
76+
.options.persist,
77+
).toBe(true);
78+
});
79+
80+
it('does not let an UNSET env var read as false over a config that said true', () => {
81+
// The tri-state `envBooleanFlag` exists for this case: `undefined` must
82+
// fall through to config, not resolve to `false`.
83+
expect(
84+
resolveEmailCapabilityArg({ persist: true }, { OS_EMAIL_QUEUE_ENABLED: 'true' })
85+
.options.persist,
86+
).toBe(true);
87+
});
88+
89+
it('keeps the queue flag resolving exactly as it did before the extraction', () => {
90+
// `envBooleanFlag` replaced OS_EMAIL_QUEUE_ENABLED's inline list; these
91+
// pin that the refactor was behaviour-preserving, empty string included.
92+
expect(resolveEmailCapabilityArg({}, { OS_EMAIL_QUEUE_ENABLED: 'on' }).options.queueDelivery)
93+
.toBe(true);
94+
expect(resolveEmailCapabilityArg({ queueDelivery: true }, { OS_EMAIL_QUEUE_ENABLED: '' })
95+
.options.queueDelivery).toBe(false);
96+
expect(resolveEmailCapabilityArg({ queueDelivery: true }, {}).options.queueDelivery).toBe(true);
97+
expect(resolveEmailCapabilityArg({}, {})).not.toHaveProperty('options.queueDelivery');
98+
});
99+
100+
it('does not disturb the rest of the resolved options', () => {
101+
const { options } = resolveEmailCapabilityArg(
102+
{ provider: 'smtp', persist: false, options: { host: 'smtp.acme.test' } },
103+
{},
104+
);
105+
expect(options).toMatchObject({
106+
provider: 'smtp',
107+
persist: false,
108+
providerOptions: { host: 'smtp.acme.test' },
109+
});
110+
});
111+
});
112+
113+
// ── end to end: resolver output -> real plugin ─────────────────────────────
114+
115+
/**
116+
* Minimal ObjectQL stand-in. `start()`'s `kernel:ready` handler needs an
117+
* engine to exist before it decides anything about persistence, and the
118+
* stranded-outbox sweep reads `sys_email` on the persisting path.
119+
*/
120+
function fakeEngine() {
121+
return {
122+
async find() { return []; },
123+
async insert(_table: string, data: any) { return { id: data?.id }; },
124+
async update() { return {}; },
125+
async delete() { return { deleted: 0 }; },
126+
};
127+
}
128+
129+
/**
130+
* Boot the real plugin the way `os serve` does — through the resolver — and
131+
* report whether an `EmailPersistence` was built.
132+
*
133+
* `EmailService.setPersistence` is called only on the persisting branch, and
134+
* it is what puts `persistence` into the live service's options, so reading
135+
* that back is a direct observation of the branch the plugin took rather than
136+
* a restatement of the flag it was handed.
137+
*/
138+
async function bootThroughResolver(
139+
cfgEmail: Record<string, any>,
140+
env: NodeJS.ProcessEnv = {},
141+
): Promise<{ persistenceBuilt: boolean; constructedWith: Record<string, unknown> }> {
142+
const constructedWith = resolveEmailCapabilityArg(cfgEmail, env).options;
143+
144+
const services: Record<string, unknown> = { manifest: { register: () => {} }, objectql: fakeEngine() };
145+
const hooks: Record<string, Array<() => Promise<void> | void>> = {};
146+
const ctx: any = {
147+
logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() },
148+
getService: (name: string) => {
149+
if (!(name in services)) throw new Error(`service '${name}' not registered`);
150+
return services[name];
151+
},
152+
registerService: (name: string, svc: unknown) => { services[name] = svc; },
153+
hook: (name: string, fn: () => Promise<void> | void) => { (hooks[name] ??= []).push(fn); },
154+
};
155+
156+
const plugin = new EmailServicePlugin(constructedWith as any);
157+
await plugin.init(ctx);
158+
await plugin.start(ctx);
159+
for (const fn of hooks['kernel:ready'] ?? []) await fn();
160+
161+
const service = services.email as { options: { persistence?: unknown } };
162+
return { persistenceBuilt: service.options.persistence != null, constructedWith };
163+
}
164+
165+
describe('config.email.persist reaches EmailServicePlugin (#5447)', () => {
166+
it('builds NO EmailPersistence when the config says persist:false', async () => {
167+
const { persistenceBuilt, constructedWith } = await bootThroughResolver({ persist: false });
168+
expect(constructedWith.persist).toBe(false);
169+
expect(persistenceBuilt).toBe(false);
170+
}, 60_000);
171+
172+
it('still persists when the config says nothing — behaviour before #5447, unchanged', async () => {
173+
// The compatibility anchor. Every deployment that never wrote the key is
174+
// this case, and it must be indistinguishable from the pre-#5447 build.
175+
const { persistenceBuilt, constructedWith } = await bootThroughResolver({});
176+
expect(constructedWith).not.toHaveProperty('persist');
177+
expect(persistenceBuilt).toBe(true);
178+
}, 60_000);
179+
180+
// The two cases below are completeness pins, NOT discriminating ones, and
181+
// saying so is the point of this comment. Their expected outcome —
182+
// persistence built — is also what an unwired resolver produces, because the
183+
// plugin default is ON. Deleting the wiring leaves both GREEN (measured: 8
184+
// of these 13 go red, these two and the three default-behaviour pins do
185+
// not). They are worth keeping as the positive half of the matrix; they are
186+
// not evidence the carrier exists. The assertions that prove that are the
187+
// `persist:false` one above and the `OS_EMAIL_PERSIST_ENABLED=false` one
188+
// below — the two whose expectation DIFFERS from the default.
189+
it('persists on an explicit persist:true', async () => {
190+
const { persistenceBuilt } = await bootThroughResolver({ persist: true });
191+
expect(persistenceBuilt).toBe(true);
192+
}, 60_000);
193+
194+
it('lets OS_EMAIL_PERSIST_ENABLED=false switch persistence off over a config that said true',
195+
async () => {
196+
const { persistenceBuilt } = await bootThroughResolver(
197+
{ persist: true },
198+
{ OS_EMAIL_PERSIST_ENABLED: 'false' },
199+
);
200+
expect(persistenceBuilt).toBe(false);
201+
}, 60_000);
202+
203+
it('lets OS_EMAIL_PERSIST_ENABLED=true switch it back on over a config that said false',
204+
async () => {
205+
const { persistenceBuilt } = await bootThroughResolver(
206+
{ persist: false },
207+
{ OS_EMAIL_PERSIST_ENABLED: 'true' },
208+
);
209+
expect(persistenceBuilt).toBe(true);
210+
}, 60_000);
211+
});

packages/cli/src/commands/serve.ts

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2973,6 +2973,27 @@ export interface EmailCapabilityArg {
29732973
options: Record<string, unknown>;
29742974
}
29752975

2976+
/**
2977+
* The ONE truth table this file reads its `OS_EMAIL_*_ENABLED` booleans with
2978+
* (#5447).
2979+
*
2980+
* Extracted rather than restated: `OS_EMAIL_QUEUE_ENABLED` carried this list
2981+
* inline, and a second boolean flag written a second way is how one env var
2982+
* ends up accepting `on` while its neighbour does not — the operator-visible
2983+
* half of the "two literals describing one vocabulary" trap that split the
2984+
* settings dropdown from the transports (#5094).
2985+
*
2986+
* Tri-state on purpose: `undefined` means the variable is unset and the caller
2987+
* must fall through to config, which is what keeps an absent flag from
2988+
* silently reading as `false` and overriding a config that said `true`.
2989+
* An empty string is a SET variable and resolves to `false`, matching the
2990+
* behaviour `OS_EMAIL_QUEUE_ENABLED` already had.
2991+
*/
2992+
function envBooleanFlag(raw: string | undefined): boolean | undefined {
2993+
if (raw == null) return undefined;
2994+
return ['1', 'true', 'yes', 'on'].includes(String(raw).trim().toLowerCase());
2995+
}
2996+
29762997
/**
29772998
* Resolve what `EmailServicePlugin` is constructed with, from `config.email`
29782999
* plus `OS_EMAIL_*` env (env wins, so an operator can override per environment).
@@ -3007,6 +3028,18 @@ export interface EmailCapabilityArg {
30073028
* delivery from inline to the durable `sys_job_queue` path (#5160). It reuses
30083029
* `OS_EMAIL_RETRIES` as its attempt budget rather than adding a second retry
30093030
* knob — see `EmailServicePlugin.makeQueueDelivery`.
3031+
*
3032+
* `OS_EMAIL_PERSIST_ENABLED=false` (or `config.email.persist: false`) stops
3033+
* every delivery attempt being written to `sys_email` (#5447). The plugin
3034+
* option has been live since the plugin had one — it builds no
3035+
* `EmailPersistence` when `persist === false` — but nothing carried the
3036+
* declared `config.email.persist` here, so a PII-sensitive deployment that
3037+
* switched persistence off in `objectstack.config.ts` type-checked, parsed,
3038+
* read "Persist to sys_email (default true)" in the generated reference, and
3039+
* went on writing every message body to the database. Resolution order is this
3040+
* function's own, per setting: env > `config.email.persist` > the plugin
3041+
* default (persist ON) — so a config and an env that say nothing leave the
3042+
* option absent and the plugin's default untouched.
30103043
*/
30113044
export function resolveEmailCapabilityArg(
30123045
cfgEmail: Record<string, any> = {},
@@ -3033,9 +3066,15 @@ export function resolveEmailCapabilityArg(
30333066
// is not knowable here — no kernel exists yet — so the plugin asserts it on
30343067
// `kernel:ready`, where the service registry has settled, and fails the boot
30353068
// there if no durable queue showed up.
3036-
const queueDelivery = env.OS_EMAIL_QUEUE_ENABLED != null
3037-
? ['1', 'true', 'yes', 'on'].includes(String(env.OS_EMAIL_QUEUE_ENABLED).trim().toLowerCase())
3038-
: cfgEmail.queueDelivery;
3069+
const queueDelivery = envBooleanFlag(env.OS_EMAIL_QUEUE_ENABLED) ?? cfgEmail.queueDelivery;
3070+
// `OS_EMAIL_PERSIST_ENABLED` — the carrier `config.email.persist` never had
3071+
// (#5447). `_ENABLED` is Prime Directive #9's boolean-flag shape; unlike the
3072+
// queue flag it is default-ON rather than default-off, because it does not
3073+
// enable a new capability — it is the off switch for one that has always
3074+
// been on, and a deployment that says nothing must keep its `sys_email`
3075+
// audit trail. Absent from BOTH sources means the key is left out of the
3076+
// constructor options entirely, so the plugin's own default decides.
3077+
const persist = envBooleanFlag(env.OS_EMAIL_PERSIST_ENABLED) ?? cfgEmail.persist;
30393078
const defaultTemplateContext = {
30403079
appName: env.OS_APP_NAME || cfgEmail.appName || configAppName || 'ObjectStack',
30413080
...(cfgEmail.defaultTemplateContext || {}),
@@ -3070,6 +3109,7 @@ export function resolveEmailCapabilityArg(
30703109
defaultFrom,
30713110
...(retries != null && !Number.isNaN(retries) ? { retries } : {}),
30723111
...(queueDelivery != null ? { queueDelivery: !!queueDelivery } : {}),
3112+
...(persist != null ? { persist: !!persist } : {}),
30733113
defaultTemplateContext,
30743114
};
30753115

0 commit comments

Comments
 (0)