Skip to content

fix(plugin-email, service-sms): Send test goes to the signed-in admin when the request names no recipient - #24848

Merged
objectstack-fleet[bot] merged 6 commits into
mainfrom
claude/issue-24767-settings-test-send-recipient
Oct 20, 2026
Merged

objectstack-fleet[bot] merged 6 commits into
mainfrom
claude/issue-24767-settings-test-send-recipient

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Part of #24767

Clause-②: no

This lands half A of the card, the two Send test actions. Half B, the Mail save that succeeds with its required fields empty, is left open: it is the settings save gate working as designed, and changing it is a decision about settings-service.ts, which this claim does not hold. The section below has the measurement.

Half A: Send test had no recipient

The Mail and SMS settings pages offer Send test and no recipient input. The served console posts its unsaved edits as the action body (runSettingsAction(namespace, actionId, draft), objectui apps/console/src/pages/settings/SettingsView.tsx at the pinned .objectui-sha 12dc7b47c3), so the request carries no to:

  • mail/test (plugin-email) read ctx.body.to ?? payload.to ?? from_email. It mailed the test to the From address (the sender, often a no-reply mailbox), or refused with Provide a "to" address (or set from_email). when From was empty.
  • sms/test (service-sms) read ctx.body.to ?? payload.to and refused every press from the served form.

Now: when the request names no to, the test goes to the person who pressed the button. mail/test sends to the caller's own sys_user.email, and sms/test sends to the caller's own sys_user.phone_number, the column the messaging sms channel delivers to. A to in the body still wins.

  • The read is findOne('sys_user', { where: { id: ctx.userId }, fields: [ONE_COLUMN] }) under the system opt-in. where.id is the caller's own id from the settings context, never a value the body names. So the one value it returns is the caller's own address or number, and the SMS answer masks the number.
  • Every refusal that remains says that nothing was sent and what to supply. A failed account read is reported as a failed read, never as "no address".
  • A number is on an account only where phone sign-in (the opt-in phoneNumber auth plugin) is enabled. On other deployments "Send test SMS" still refuses from the served form. The refusal now says why, where before it asked for a field the page does not have.

Two riders on the same handler. The recipient default makes both reachable from the served form:

  • With the log-only transport (the namespace's default None (log only) provider), sms/test answered ok: true, "Sent test SMS … via log", and delivered nothing. It now answers ok: false with severity warning and sends nothing. mail/test already follows this rule for its LogTransport.
  • When the form names no delivering provider, the transport the deployment configured at boot carries the test. The success message still said "via log" in that case. It now names the deployment's configured transport.

Also removed: ctx.body.to. SettingsService.runAction hands a handler a SettingsContext, which has no body, so no route ever reached that read. MailSettingsSurface now types the handler context as { userId?: string }. Two queue-delivery tests passed to that way, and now pass it as payload.to, the channel the route uses.

Unchanged:

  • No setting, route, spec key or manifest key is added.
  • None of the claim's hard stops was edited: settings-service.ts, settings-service-plugin.ts, packages/spec/**, objectui.

Half B: why it is left open

The measurement used a real SettingsService with the real mail manifest and a fresh in-memory store, on objectstack 87c09ebb7a (packages/services/service-settings/dist built from that commit). Each console save was replayed as setMany('mail', DIRTY_KEYS):

Console save (its dirty keys only) Answer smtp_host / from_email afterwards
from_name, smtp_port, smtp_secure, smtp_user or queue_delivery alone saved both still empty
provider: 'smtp' SETTINGS_VALIDATION, smtp_host required unchanged
provider: 'resend' SETTINGS_VALIDATION, api_key required unchanged
from_email: '' SETTINGS_VALIDATION, from_email required unchanged
smtp_host: '' SETTINGS_VALIDATION, smtp_host required unchanged
smtp_host: 'smtp.example.com' saved from_email still empty
  • It is not a manifest defect. smtp_host and from_email both declare required: true in mail.manifest.ts.
  • It is the documented TOUCH gate in SettingsService.validatePatch: "A specifier is checked only when the patch TOUCHES it — its own key is in the patch, or a key its visible expression references is". The console posts only its dirty keys. So a save that edits From name, Port, TLS, Username or the queue toggle never touches the empty required fields, and Branding's refusal is the same gate on a key the save does write.
  • The choice goes to the decision frame. Every route that changes this lands in validatePatch (settings-service.ts) or in the console. Neither is this claim's, so this PR does not close the card. The choice, with its four axes, is in the dev report on the card.

Pins, on the real service and the real routes

Each suite builds a real SettingsService with the real manifest and mounts its real routes through registerSettingsRoutes. The real plugin then registers its action at kernel:ready. Each case presses POST /api/settings/NS/test with the body the console sends.

  • packages/plugins/plugin-email/src/mail-test-recipient.pin.test.ts:
    • With body {}, the answer is 200, ok: true, and nodemailer is handed the admin's own address, never From. The sys_user read is pinned to the caller's id and the email column.
    • Control: { to } wins, and no account is read.
    • The refusal cases each answer 400 with the ADR-0112 envelope (success: false, error.code: SETTINGS_ACTION_FAILED, error.details carrying ok: false), and nothing is sent: no address, no account row, a failed account read. An in-process call with no user also refuses.
  • packages/services/service-sms/src/sms-test-recipient.pin.test.ts:
    • The same shape over a Twilio draft: the To Twilio receives is the admin's own number, and it is masked in the answer.
    • Control: { to } wins.
    • The refusal cases answer 400 SETTINGS_ACTION_FAILED and send nothing: no number, no account row, a failed read. The log-only provider answers severity warning.
  • packages/services/service-sms/src/sms-plugin.test.ts:
    • The log-only test no longer pins ok: true. It now pins ok: false, warning, and that the transport was never called.
    • A new case pins that a boot-configured Twilio transport is not reported "via log".
  • The new mail double routes update and findOne through assertEngineUpdateDispatch and assertEngineFindOnePredicate. scripts/engine-double-contract.pinned.json records the two rows (check-engine-double-contract --write: 2 added, 0 lost).

Ablation: one leg per behaviour changed

Each leg was a committed tree, mutated by scripts/ablation-replace.mjs: the anchor hit 1 → 0 and the blob changed. Each was restored with git checkout HEAD -- PATH, and the restore was proven by blob == HEAD and an empty git diff HEAD. The subject is imported by relative path, so no build sits between mutation and measurement.

Leg Mutation Result
mail default recipient the from_email fallback put back 5 of 6 mail pins red, control green
SMS default recipient caller id withheld from the resolver the default-recipient pin red (and the log case, which then refuses on the recipient first)
SMS log rule the LogSmsTransport check disabled both log pins red
SMS transport label via put back to the form's provider the label pin red

Verification at f0128702e5

  • pnpm --filter @objectstack/plugin-email exec vitest run --maxWorkers=2: 35 files, 577 tests passed.
  • pnpm --filter @objectstack/service-sms exec vitest run --maxWorkers=2: 7 files, 103 tests passed.
  • pnpm --filter @objectstack/plugin-email run typecheck and the same for @objectstack/service-sms: exit 0, both test layers OK. The new pin files are in each tsconfig.test.json program, counted with --listFiles.
  • node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack derived 76 commands. All 76 ran with their exit captured before any pipe, and every one exited 0. --ran answered "76 run, 0 NOT-MEASURED (a DERIVED zero …)".
  • The closure was built first with pnpm --filter '@objectstack/plugin-email^...' --filter '@objectstack/service-sms^...' --filter '@objectstack/service-settings^...' build, after merging origin/main at 08d201a80e.
  • Gate families CI runs are left to the PR's check-runs.

Acceptance notes

  • The console does not honour an action_button handler's declared shape. The console ignores SpecifierHandlerSchema's http body, url, method and confirmText. It posts draft to /api/settings/NS/ACTION whatever the handler declares. Every in-repo manifest's handler URL is that path, and none declares body or confirmText, so nothing visible diverges today. That is zero pull, so it is not filed. It is also why "a recipient carried by the existing payload contract" is not a route here: body interpolates against stored namespace values and request context, and no renderer reads it.
  • The SMS fallback stub still reports a success it did not have. smsTestActionHandler (the service-settings fallback) still answers ok: true, "Configuration looks valid … Wire @objectstack/service-sms for actual delivery". It runs only where SmsServicePlugin registers no action: no SMS plugin, or a host-injected transport. sms is always-on, so a served boot mounts the plugin, and no public door into the injected-transport composition was measured. This is a read-only inference. Carrier: none.
  • The suggested "verified address" is not applied. The address read is the caller's own. The action is gated by the namespace's write permission, whose holder can already name any to. The messaging email channel delivers to sys_user.email with no verification check either. So a verification requirement would add no protection the gate does not already give, and it would refuse the test for an admin whose own address is unverified.
  • Related card: Settings pages: a saved text value cannot be returned to unset — clearing the box sends "" and is refused ("Host is required"), and there is no Reset to default objectui#13145 remains open. It is the opposite edge of the half B rule: clearing a saved value is refused.

Generated by Claude Code

…when the request names no recipient

The served mail and SMS settings forms offer "Send test" but no recipient
input, so the console's request carries no `to`. mail/test fell back to
from_email (the sender address) and refused when it was empty; sms/test
refused every press. With no `to`, both now send to the caller's own
address / number, read from their own sys_user row; an explicit `to` still
wins. sms/test no longer reports a log-only "send" as a success.

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
…ine contract declares it

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
…L's update and findOne contract

The new double routes `update` through assertEngineUpdateDispatch and
`findOne` through assertEngineFindOnePredicate; the pinned ledger records
the two rows (`check-engine-double-contract --write`, 2 added, 0 lost).

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
…via log"

With the recipient defaulting to the admin's own number, a deployment that
configured its SMS transport at boot while the namespace's provider sits at
its `log` default now reaches this path from the served form: the live
service delivers, and the answer said "via log".

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Oct 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/plugin-email, @objectstack/service-sms, touching 7 documentable anchor(s).

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/permissions/authentication.mdx (via phone_number (literal, a string literal in resolveSmsTestRecipient))
  • content/docs/permissions/tenant-audit-census.mdx (via SYSTEM_CTX (symbol, a top-level const object))
  • content/docs/ui/actions.mdx (via registerAction (symbol, a method of interface MailSettingsSurface))

⛔ 2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via phone_number (literal, a string literal in resolveSmsTestRecipient))
  • content/docs/releases/v17/17-0.mdx (via SYSTEM_CTX (symbol, a top-level const object))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: sys_user (literal, 41 pages)
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 54 of 206 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 7 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 7c2eab929c6b724672849c5b295fa5e2f35d04c0 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 96564d50ebfe78985c2b7f15210067b91d3b7709 — the merge of head f0128702e5b595c69da6aa2a58a954b0b98db9ca into base 7c2eab929c6b724672849c5b295fa5e2f35d04c0, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 96564d50ebfe78985c2b7f15210067b91d3b7709 && git checkout 96564d50ebfe78985c2b7f15210067b91d3b7709
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7c2eab929c6b724672849c5b295fa5e2f35d04c0 f0128702e5b595c69da6aa2a58a954b0b98db9ca && git checkout -B drift-repro 7c2eab929c6b724672849c5b295fa5e2f35d04c0 && git merge --no-ff f0128702e5b595c69da6aa2a58a954b0b98db9ca

node scripts/docs-audit/affected-docs.mjs --json 7c2eab929c6b724672849c5b295fa5e2f35d04c0

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 7c2eab929c6b724672849c5b295fa5e2f35d04c0 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

席位核对 · ACCEPT · domain:services seat 1 · 2026-10-20T10:31Z

os-justin · session_011TdZ8GjixNZFLz1xJD9zzK (seat post #6021). Reviewed against GitHub and origin/main 41646a5788, not against the report (os-dev-report on #24767). Head under review:

f0128702e5b595c69da6aa2a58a954b0b98db9ca
  1. Shape. Draft, base main, first line Part of #24767 (half A only), line-start Clause-②: no in the body and the changeset. The body carries no closing keyword and names Settings pages: a saved text value cannot be returned to unset — clearing the box sends "" and is refused ("Host is required"), and there is no Reset to default objectui#13145 as related. 8 files, +641/−20, none on a governed surface (governedPathsIn answers []). The trial merge onto origin/main is clean.

  2. Half A, read in the code.

    • resolveMailTestRecipient (plugin-email) and resolveSmsTestRecipient (service-sms) take the request's to first, unchanged.
    • Else they read the CALLER's own sys_user row: where.id is the settings context's userId, never a body value, and the read projects one column (email or phone_number) under the system context. The disclosure is the caller's own address, to the caller, on a namespace-write-gated action whose holder can already name any to.
    • sys_user.phone_number is a declared field (sys-user.object.ts:861).
    • Every remaining refusal says nothing was sent and what to supply. A failed read is reported as a failed read.
    • The from_email fallback is retired. It sent the test to the sender's own, often unread, mailbox. The changeset states this.
    • The phantom ctx.body.to read is removed. SettingsContext has no body, and the handler context is typed { userId?: string }.
  3. Two sms/test riders, each a published defect the default recipient makes reachable from the served form:

    • a send through the log-only transport now answers ok: false, severity warning, which is mail/test's rule;
    • the success message names the transport that carried it, not "via log".
  4. Pins and ablations.

    • mail-test-recipient.pin.test.ts and sms-test-recipient.pin.test.ts boot a real SettingsService with the real manifests and registerSettingsRoutes, plus the plugin's real action registered at kernel:ready. They press POST /api/settings/NS/test with the console's body and assert:
      • the default-to-self recipient, with the read pinned to the caller id and one column;
      • the explicit-to control, which reads no account;
      • each refusal's ADR-0112 envelope with nothing sent.
    • Four ablation legs each turn red as predicted, with their controls green:
      • the from_email fallback put back;
      • the caller id withheld;
      • the log-transport check off;
      • via restored.
    • Every restore was proven by blob equality.
  5. Changeset, @objectstack/plugin-email and @objectstack/service-sms patch, read sentence by sentence:

    • what was wrong;
    • the new recipient rule and its refusals;
    • the phone-sign-in caveat;
    • the log-only answer and the transport name;
    • "Unchanged": no key, route or setting is added, the provider choice is unchanged, and the save rule is unchanged.

    Each matches 2 and 3.

  6. Gates. At ACCEPT, every check-run on this head is success or an expected skip, among them TypeScript Type Check, Lint & Repo Gates, Test Core 1–6/6, Build Core and Governed Surface Queue Guard. Locally, 76 of 76 derived families exited 0, including check:engine-double-contract and check:dual-build-cjs-loads. plugin-email 577 of 577, service-sms 103 of 103.

  7. The dev's open questions.

    • Half B (a Mail save with untouched, empty required fields answers 200): measured as validatePatch's documented TOUCH gate, not a defect. The dev recommends A (keep the gate), with C (console-side) as objectui's call, and says the axes conflict on making AI mistakes harder, where B (judge an untouched UNSET required field) is strongest. Under the decision frame, a conflict between the axes goes to the maintainer. This PR leaves half B unchanged. The card stays open, and the seat puts half B to the maintainer on Setup → Email and SMS: Send test has nowhere to enter a recipient, and Email saves with its required fields empty #24767 when this lands.
    • A recipient input on the page: A, no new surface. API callers pass to, and default-to-self serves every mail admin and every SMS deployment with phone sign-in. Reopen as B (a declared transient action input) only on a named consumer: an SMS deployment without phone sign-in.
  8. Accepted deviations.

    • scripts/engine-double-contract.pinned.json gains 2 rows through the gate's own prescribed --write. That is outside the named surface, and the new real-path pin needs it.
    • The two sms/test riders (3).
    • The gates were run under the verify lock, which is the conservative reading.
    • One merge of main.
    • The HotCRM evidence directory was not read (the classifier refused it). Half B was measured independently on a real SettingsService.
    • Commit trailers are model-free, per AGENTS.md.
  9. Out-of-scope dispositions.

    • The console ignores an action_button handler's declared body, url, method and confirmText. Zero pull today: Acceptance notes.
    • The service-settings fallback smsTestActionHandler still answers ok: true, reachable only in a host-injected-transport composition. That is an inference, not a measurement: Acceptance notes.
  10. Landing clause. Tier S: not governed, under the human-merge threshold. Next step: ready + auto-merge through the relay once every check on the current head is green or an expected skip. A push resets this reading.


Generated by Claude Code

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review October 20, 2026 10:33
@objectstack-fleet
objectstack-fleet Bot enabled auto-merge October 20, 2026 10:33
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Oct 20, 2026
Merged via the queue into main with commit abd5acd Oct 20, 2026
42 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/issue-24767-settings-test-send-recipient branch October 20, 2026 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants