Skip to content

Commit 0ec4c5a

Browse files
fix(plugin-email): saving mail provider "None (log only)" stops delivery instead of keeping the live SMTP transport (#25497)
<!-- claude-projects-attribution: {"github_login":"os-sales"} --> _Requested by **Sales** · [project thread](https://claude.ai/code/project/chan_01D5bWFY3rFzgJcytD5gD4Uu?thread=cmsg_01D5bWFY3rFzgJcytD5gD4UuACdPWFJTLDcTrByFCHLzqb)_ Fixes #25450 Clause-②: no Metadata-protocol: none ## What was wrong Setup → Email offers the provider `log` as **"None (log only — no real delivery)"**. When `log` was saved, `applyMailSettings` (`packages/plugins/plugin-email/src/email-plugin.ts`) returned without swapping the transport. It logged "transport unchanged — messages are logged and recorded in sys_email, never delivered". After a boot with SMTP, every message kept going out over SMTP. The live-status banner (landed with #25448) truthfully said SMTP was live. "Send test email" sent a real message and answered "Sent test email to …". The page label and the log line both said nothing was delivered. ## Measurement: was "transport unchanged" deliberate? No. Shape (A). - `git log -S"transport unchanged"` finds the line's origin in `fc9b204fe8` ("Add global scope + reactive settings client"). There `smtp` and `log` shared ONE no-op branch, with the comment "No SMTP transport ships in core; settings-only edits become a no-op for transport". For both providers, nothing could be swapped in. - `41c3b48f1d` shipped `SmtpTransport` (ADR-0012) and split the branch. `smtp` got a real swap. `log` kept the no-op, and the docstring describing it says "`provider = 'log'` keeps the LogTransport". That line assumes the live transport is already the LogTransport. Neither commit, nor ADR-0012, names a boot-built transport (`OS_EMAIL_SMTP_*` / `providerOptions`) or a host-supplied one that a saved `log` must not replace. - The code already decides precedence the other way. A saved SMTP host replaces a boot-built or host-supplied transport, and so does a saved Resend/Postmark key. The only place the boot SMTP is kept is the `smtp`-with-no-host branch. That is the out-of-the-box namespace, where the page has stated nothing. Env precedence over the page lives one layer up, in the settings env lock (`OS_MAIL_*`, `SettingsService.get`), not in keeping the transport. - The manifest's own contract text (`service-settings/src/manifests/mail.manifest.ts`, the `PROVIDER_OPTIONS` comment) says of `log`: "the label says so, the runtime honours it". After this PR, that is true. So nothing deliberate protects the old transport, and this PR takes **(A)**. A saved `log` installs the `LogTransport` and replaces whatever was live, the same way every other selected provider does. A host-supplied transport is not exempt. The settings page already replaces it for every other provider, so an exemption for `log` alone would bring back the mismatch between the label and what the transport does. ## The change - `applyMailSettings`, `provider === 'log'`: if the live transport is not already a `LogTransport`, install a new `LogTransport` and drop the `liveSmtp` handle. This matches the Resend/Postmark branch: the old SMTP pool is not closed under a possible in-flight send. - The info line now says what happened, and names the transport it replaced: `the LogTransport now replaces the previous live transport (was: "Mail is delivered through SMTP (host:port)") — messages are logged and recorded in sys_email, never delivered.` When nothing delivered before, it says `the LogTransport stays live — …`. - Docstring updated. - Landing: the file the claim names. `live-transport-status.ts` and the manifest/translations are unchanged. The banner and the label were already true statements; the transport was what disagreed with them. ### Why `Metadata-protocol: none` This does change what a saved value DOES: a saved `log` now stops mail. But `mail.provider` is a settings value that an administrator stores in `sys_setting` through Setup. It is not metadata an author writes, and the change adds, narrows or re-means no key, value or type in the metadata protocol. The changeset carries the operator-facing consequence and the one-line remedy. ## Tests New `packages/plugins/plugin-email/src/mail-log-provider.pin.test.ts`. Each pin reads all four answers from one save: the live transport and `isConfigured()`; the page statement (`registerLiveStatus`); the re-apply's log line; and the "Send test email" answer, with `nodemailer.sendMail` never called. - After a boot with SMTP **saved on the page**, saving `log` gives: LogTransport, banner "No mail leaves this machine", a line naming the replaced `SMTP (smtp.saved.test:587)`, and Send test `ok: false` / `warning`. - After a boot with SMTP **built from the boot options** (`OS_EMAIL_SMTP_*` reaches the plugin as `providerOptions`): the same. - After a boot with a **host-supplied** transport: the same. The host transport is not called. - A **restart** with `log` stored and SMTP in the boot options boots onto the LogTransport. - **CONTROL**, a boot with no SMTP: the boot LogTransport stays live (same instance), the banner warns, the line says "stays live", and Send test answers `ok: false`. Fixture re-judged: `email-plugin.queue-delivery.test.ts` used a stored `provider: 'log'` with a host transport, which relied on exactly the "transport unchanged" behaviour this PR removes. Its default namespace is now the out-of-the-box one: `smtp` at its manifest default, no host. That branch keeps the harness transport, as before. Runs (head `fa561813a8`): - `pnpm --filter @objectstack/plugin-email test` gives Test Files 38 passed (38), Tests 685 passed (685). - `pnpm --filter @objectstack/plugin-email typecheck` gives exit 0 (`tsc --noEmit` plus `check:test-typecheck: OK`). **Ablation** (head `fa561813a8`, through `scripts/ablation-replace.mjs`): the anchor `this.service.setTransport(new LogTransport(ctx.logger));` was replaced by a no-op. The tool reported "ok mutation landed: anchor 1 -> 0, blob 6e5d437 -> 0a9522b85240". The pin went RED, with Tests 4 failed | 1 passed (5): all four SMTP/host legs failed and the CONTROL held. The restore reported "ok restored: blob == HEAD (6e5d437) and `git diff HEAD` is empty". The test imports `./email-plugin.js` from `src`, so no `dist` leg applies. ## Gates `node scripts/pm/dispatch-gates.mjs --commands` at `fa561813a8` derives 67 commands. All 67 ran locally and exited 0. `--ran`, with an exit code recorded for each, says: "67 derived famil(ies) accounted for — 67 run, 0 NOT-MEASURED (a DERIVED zero …)". - `check:engine-double-contract` was red on the first pass, because the new pin's fake engine declared an `update()`. The fake now answers reads only (`persist: false`), and the gate is green. - The CI-only families (Test Core, Build Core, type-check lanes, artifact rosters) are read from this PR's check-runs. ## Acceptance notes - Found while measuring, and not changed here: with **"None" picked on the form but not saved**, "Send test email" still sends through the live SMTP and answers "Sent test email to admin@… (id=…)". The handler's `provider === 'log'` path tests the live service rather than the form's choice. A throwaway probe at `fa561813a8` (not committed) showed `ok: true`, with `sendMail` called once. Reported to the seat as a separate finding. - When the transport is swapped, the previous SMTP pool is not closed. This holds on every swap branch, not only `log`. It is pre-existing, and nothing in this PR changes it. --- _Generated by [Claude Code](https://claude.ai/code/session_016g7nYP9wdNx1GDa5XDr7Uy)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016g7nYP9wdNx1GDa5XDr7Uy --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a285ff2 commit 0ec4c5a

4 files changed

Lines changed: 218 additions & 4 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/plugin-email': patch
3+
---
4+
5+
fix(plugin-email): saving mail provider "None (log only — no real delivery)" stops delivery
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
Setup → Email offers the provider `log` as "None (log only — no real delivery)". Saving it used to leave the live transport in place, so after a boot with SMTP (saved on the page, or built from `OS_EMAIL_SMTP_*` / `providerOptions`) every message kept going out over SMTP while the page label and the info log both said nothing was delivered. A saved `log` now installs the `LogTransport`, replacing whatever was live, as a saved SMTP host or API key already replaces it. The live-status banner, the re-apply's log line (which names the transport it replaced) and "Send test email" all report that no mail leaves the machine.
11+
12+
If a deployment saved `log` on the settings page while relying on SMTP from `OS_EMAIL_SMTP_*` or on a transport the host supplies, mail now stops at boot. To deliver again, select SMTP (or Resend / Postmark) in Setup → Email and save. Alternatively, pin the provider with `OS_MAIL_PROVIDER`.

‎packages/plugins/plugin-email/src/email-plugin.queue-delivery.test.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,11 @@ function fakeCtx(services: Record<string, unknown>) {
140140
/** Last element — the package's tsconfig lib predates Array.prototype.at. */
141141
function last<T>(arr: T[]): T | undefined { return arr[arr.length - 1]; }
142142

143+
// The out-of-the-box `mail` namespace (provider `smtp` at its manifest
144+
// default, no host): the re-apply keeps the transport this harness hands the
145+
// plugin. A saved `log` would not — it installs the LogTransport.
143146
const MAIL_DEFAULTS: Record<string, Resolved> = {
144-
provider: { value: 'log', source: 'global' },
147+
provider: { value: 'smtp', source: 'default' },
145148
from_email: { value: 'no-reply@example.test', source: 'global' },
146149
from_name: { value: 'ObjectStack', source: 'default' },
147150
queue_delivery: { value: false, source: 'default' },

‎packages/plugins/plugin-email/src/email-plugin.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1249,8 +1249,12 @@ export class EmailServicePlugin implements Plugin {
12491249
* - `provider = 'smtp'` builds a real {@link SmtpTransport} from
12501250
* `smtp_host` / `smtp_port` / `smtp_secure` / `smtp_user` /
12511251
* `smtp_password` and swaps it in (ADR-0012 — SMTP ships in core).
1252-
* - `provider = 'log'` keeps the LogTransport. The from-address is
1253-
* still applied.
1252+
* - `provider = 'log'` installs the LogTransport, replacing whatever was
1253+
* live — a boot-built SMTP (`OS_EMAIL_SMTP_*` / `providerOptions`) or a
1254+
* host-supplied transport included, as every other selected provider
1255+
* replaces it. The page offers it as "None (log only — no real
1256+
* delivery)", so saving it stops delivery. The from-address is still
1257+
* applied.
12541258
* - `provider = 'resend' | 'postmark'` rebuilds the transport using
12551259
* `api_key` from settings.
12561260
* - anything else — including `sendgrid` / `ses`, which the settings page
@@ -1384,9 +1388,25 @@ export class EmailServicePlugin implements Plugin {
13841388
}
13851389

13861390
if (provider === 'log') {
1391+
// "None (log only — no real delivery)" is a stop, and it stops: the
1392+
// LogTransport replaces whatever was live, exactly as a saved SMTP host
1393+
// or API key replaces it. Keeping the previous transport here (a
1394+
// leftover from when `smtp` and `log` were both transport no-ops) went
1395+
// on delivering over a boot-built SMTP while the page label and this
1396+
// line both said nothing was delivered.
1397+
const replaced = this.service.options.transport instanceof LogTransport
1398+
? undefined
1399+
: describeLiveMailTransport(this.service).label;
1400+
if (replaced) {
1401+
this.service.setTransport(new LogTransport(ctx.logger));
1402+
this.liveSmtp = undefined;
1403+
}
13871404
ctx.logger.info(
13881405
`EmailServicePlugin: mail settings applied (provider=log, from=${appliedFrom ?? '∅'}); `
1389-
+ 'transport unchanged — messages are logged and recorded in sys_email, never delivered.',
1406+
+ (replaced
1407+
? `the LogTransport now replaces the previous live transport (was: "${replaced}") — `
1408+
: 'the LogTransport stays live — ')
1409+
+ 'messages are logged and recorded in sys_email, never delivered.',
13901410
);
13911411
return;
13921412
}
Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// Setup → Email offers the provider `log` as "None (log only — no real
4+
// delivery)". Saving it after a boot with SMTP used to keep the SMTP transport
5+
// live: the re-apply logged "transport unchanged — … never delivered" while
6+
// every message kept going out over SMTP. These pins hold the four readings of
7+
// one save to one answer — the live transport, the page's live-status
8+
// statement, the re-apply's log line and the "Send test email" answer — after
9+
// a boot with SMTP from the settings page, from the boot options
10+
// (`OS_EMAIL_SMTP_*` reaches the plugin as `providerOptions`), and from the
11+
// host. The CONTROL is a boot with no SMTP at all.
12+
13+
import { describe, it, expect, vi, beforeEach } from 'vitest';
14+
import { EmailServicePlugin, type EmailServicePluginOptions } from './email-plugin.js';
15+
import { EmailService, LogTransport } from './email-service.js';
16+
import { SmtpTransport } from './transports/smtp.js';
17+
import type { LiveMailTransportStatus } from './live-transport-status.js';
18+
19+
const nm = vi.hoisted(() => ({ createTransport: vi.fn(), sendMail: vi.fn() }));
20+
vi.mock('nodemailer', () => ({
21+
default: { createTransport: nm.createTransport },
22+
createTransport: nm.createTransport,
23+
}));
24+
25+
interface Resolved { value: unknown; source?: string }
26+
27+
function fakeSettings(values: Record<string, Resolved>) {
28+
const listeners: Array<() => void> = [];
29+
const actions = new Map<string, (a: any) => Promise<any>>();
30+
let liveStatus: (() => Promise<LiveMailTransportStatus | undefined>) | undefined;
31+
return {
32+
values,
33+
createClient: () => ({}),
34+
getNamespace: async () => ({ values }),
35+
subscribe: (_ns: string, cb: () => void) => { listeners.push(cb); },
36+
registerAction: (ns: string, id: string, fn: (a: any) => Promise<any>) => { actions.set(`${ns}/${id}`, fn); },
37+
registerLiveStatus: (ns: string, fn: () => Promise<LiveMailTransportStatus | undefined>) => {
38+
if (ns === 'mail') liveStatus = fn;
39+
},
40+
/** Patch the snapshot and emit settings:changed, as a page save does. */
41+
save(patch: Record<string, Resolved>) {
42+
Object.assign(values, patch);
43+
for (const l of listeners) l();
44+
},
45+
/** What the page states, as the settings GET route asks for it (waits for a running re-apply). */
46+
statement: () => liveStatus!(),
47+
/** The "Send test email" answer for the stored values, with nothing edited on the form. */
48+
sendTest: () => actions.get('mail/test')!({
49+
values: Object.fromEntries(Object.entries(values).map(([k, v]) => [k, v.value])),
50+
payload: { to: 'admin@example.test' },
51+
}),
52+
};
53+
}
54+
55+
async function boot(mail: Record<string, Resolved>, opts: EmailServicePluginOptions = {}) {
56+
const settings = fakeSettings({
57+
provider: { value: 'smtp', source: 'default' },
58+
smtp_port: { value: 587, source: 'default' },
59+
from_email: { value: 'no-reply@example.test', source: 'global' },
60+
...mail,
61+
});
62+
// The settings binding waits for an engine; nothing here writes a row
63+
// (`persist: false` below), so it answers reads only.
64+
const engine = { async find() { return []; } };
65+
const services: Record<string, unknown> = { manifest: { register: () => {} }, objectql: engine, settings };
66+
const hooks: Record<string, Array<() => Promise<void> | void>> = {};
67+
const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() };
68+
const ctx = {
69+
logger,
70+
getService: <T,>(name: string): T => {
71+
if (!(name in services)) throw new Error(`service '${name}' not registered`);
72+
return services[name] as T;
73+
},
74+
registerService: (name: string, svc: unknown) => { services[name] = svc; },
75+
hook: (name: string, fn: () => Promise<void> | void) => { (hooks[name] ??= []).push(fn); },
76+
};
77+
const plugin = new EmailServicePlugin({ persist: false, ...opts });
78+
await plugin.init(ctx as never);
79+
await plugin.start(ctx as never);
80+
for (const fn of hooks['kernel:ready'] ?? []) await fn();
81+
return { settings, logger, service: services.email as EmailService };
82+
}
83+
84+
/** The re-apply's line for a `log` save — the last info line naming provider=log. */
85+
function logLine(logger: { info: ReturnType<typeof vi.fn> }): string {
86+
const lines = logger.info.mock.calls.map((c) => String(c[0])).filter((l) => l.includes('provider=log'));
87+
expect(lines.length).toBeGreaterThan(0);
88+
return lines[lines.length - 1];
89+
}
90+
91+
const SAVE_LOG = { provider: { value: 'log', source: 'global' } };
92+
93+
/** Every reading of a `log` save that replaced a delivering transport says mail stopped — and it did. */
94+
async function expectMailStopped(h: Awaited<ReturnType<typeof boot>>) {
95+
const statement = await h.settings.statement();
96+
// The transport.
97+
expect(h.service.options.transport).toBeInstanceOf(LogTransport);
98+
expect(h.service.isConfigured()).toBe(false);
99+
// The page.
100+
expect(statement?.label).toBe('No mail leaves this machine');
101+
expect(statement?.bannerSeverity).toBe('warning');
102+
// The log line.
103+
const line = logLine(h.logger);
104+
expect(line).toMatch(/replaces the previous live transport/);
105+
expect(line).toMatch(/never delivered/);
106+
// The answer.
107+
nm.sendMail.mockClear();
108+
const answer = await h.settings.sendTest();
109+
expect(answer).toMatchObject({ ok: false, severity: 'warning' });
110+
expect(answer.message).toMatch(/only logged/);
111+
expect(nm.sendMail).not.toHaveBeenCalled();
112+
}
113+
114+
beforeEach(() => {
115+
nm.createTransport.mockReset();
116+
nm.sendMail.mockReset();
117+
nm.createTransport.mockImplementation(() => ({ sendMail: nm.sendMail, close: vi.fn() }));
118+
nm.sendMail.mockResolvedValue({ messageId: '<wire-1@smtp>', response: '250 2.0.0 Ok' });
119+
});
120+
121+
describe('saving provider "log" after a boot with SMTP stops delivery', () => {
122+
it('SMTP saved on the settings page: the save installs the LogTransport', async () => {
123+
const h = await boot({
124+
provider: { value: 'smtp', source: 'global' },
125+
smtp_host: { value: 'smtp.saved.test', source: 'global' },
126+
});
127+
expect(h.service.options.transport).toBeInstanceOf(SmtpTransport);
128+
expect((await h.settings.statement())?.label).toBe('Mail is delivered through SMTP (smtp.saved.test:587)');
129+
130+
h.settings.save(SAVE_LOG);
131+
await expectMailStopped(h);
132+
expect(logLine(h.logger)).toMatch(/was: "Mail is delivered through SMTP \(smtp\.saved\.test:587\)"/);
133+
});
134+
135+
it('SMTP built at boot from the boot options (OS_EMAIL_SMTP_*): the save installs the LogTransport', async () => {
136+
const h = await boot({}, { provider: 'smtp', providerOptions: { host: 'smtp.env.test', port: 2525 } });
137+
// The out-of-the-box namespace (no host) keeps the boot-built SMTP.
138+
expect(h.service.options.transport).toBeInstanceOf(SmtpTransport);
139+
expect((await h.settings.statement())?.label).toBe('Mail is delivered through SMTP (smtp.env.test:2525)');
140+
141+
h.settings.save(SAVE_LOG);
142+
await expectMailStopped(h);
143+
});
144+
145+
it('a transport the host supplied: the save installs the LogTransport', async () => {
146+
const send = vi.fn(async () => ({ messageId: '<host@x>' }));
147+
const h = await boot({}, { transport: { send } as never });
148+
expect(h.service.isConfigured()).toBe(true);
149+
150+
h.settings.save(SAVE_LOG);
151+
await expectMailStopped(h);
152+
expect(send).not.toHaveBeenCalled();
153+
});
154+
155+
it('a restart with "log" stored and SMTP in the boot options boots onto the LogTransport', async () => {
156+
const h = await boot(SAVE_LOG, { provider: 'smtp', providerOptions: { host: 'smtp.env.test' } });
157+
await expectMailStopped(h);
158+
});
159+
});
160+
161+
describe('CONTROL — saving provider "log" after a boot with no SMTP', () => {
162+
it('the LogTransport stays live, and every reading says nothing is delivered', async () => {
163+
const h = await boot({});
164+
const bootTransport = h.service.options.transport;
165+
expect(bootTransport).toBeInstanceOf(LogTransport);
166+
167+
h.settings.save(SAVE_LOG);
168+
const statement = await h.settings.statement();
169+
expect(h.service.options.transport).toBe(bootTransport);
170+
expect(h.service.isConfigured()).toBe(false);
171+
expect(statement?.label).toBe('No mail leaves this machine');
172+
const line = logLine(h.logger);
173+
expect(line).toMatch(/the LogTransport stays live/);
174+
expect(line).toMatch(/never delivered/);
175+
const answer = await h.settings.sendTest();
176+
expect(answer).toMatchObject({ ok: false, severity: 'warning' });
177+
expect(nm.sendMail).not.toHaveBeenCalled();
178+
});
179+
});

0 commit comments

Comments
 (0)