docs(website): record the org lead's answers to Q1/Q3/Q5/Q7, and add Phase 1b
Four of the eight open questions in ENGAGEMENT.md §7.1 were answered on 2026-08-28. Q1's answer turned out to carry a whole phase with it. - Q5: document all three SMTP postures, lead with a relay, name Gmail-app-password as the migration path off OAuth2. - Q3: rules stay operator-editable data, but `enabled` defaults to 0 and every rule carries a hard per-hour send ceiling. Adds max_sends_per_hour to engagement_rules — the ceiling is what makes "data" safe to choose over "code". - Q7: no campaigns surface at all. Lists DO exist, but only module-declared and powered by module data, on a surface core exposes to every module. Operators may compose them; composition must NARROW, never widen. Adds §5.1a, api.registerAudiences and engagement_audience_segments. - Q1: opt-in only, users.email becomes UNIQUE, and the verification gate is an admin setting (on for fresh installs, off for upgrades). New §0.6 records why the UNIQUE index is not a one-line ALTER, verified in the tree rather than assumed: - ensureSchema() runs the ALTER block on every boot, so ADD UNIQUE INDEX against a table holding duplicates stops the site from starting. - isDuplicateUsername() tests only ER_DUP_ENTRY/1062 and never which index collided, so register would answer "that username is already taken" for a duplicate email, and provisionSsoPlayer would retry usernames for an email conflict until it exhausts PROVISION_MAX_TRIES and fails opaquely. - SSO auto-provisioning manufactures those duplicates and marks addresses verified merely for existing — which is also why dedupe is oldest-wins rather than verified-wins. CLAUDE.md's "identities are never auto-provisioned" is stale. - No route lets a user change their own address, so a verification gate has no flow to gate; Phase 1b builds one. New Phase 1b sequences the fix before the index, dedupes oldest-wins with an admin report, and keeps the collision error generic, rate-limited and out of the bot scorer. Phase 9 loses the verification flow to it and therefore no longer blocks Phase 11. Also corrects §6.0a: android-app DOES have an edge (1 behind main), so Phase -1 is six fast-forwards and two branch creations, not five and three. Assisted-By: Claude Code (Opus 5) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -1,12 +1,14 @@
|
||||
# The Engagement System — findings and plan
|
||||
|
||||
**Status:** design of record for the next workstream. No code written yet. The five scope decisions below
|
||||
are settled; the eight questions in §7.1 are open and none of them block Phase 1 or Phase 2. Per CLAUDE.md
|
||||
§ Conventions, no implementation starts without the org lead's approval of the phase it belongs to.
|
||||
**Status:** design of record for the next workstream. No code written yet. The scope decisions below are
|
||||
settled; **four of the eight questions in §7.1 were answered by the org lead on 2026-08-28** — Q1, Q3, Q5
|
||||
and Q7, and Q1's answer added a whole phase (**Phase 1b**, unique email addresses). Q2, Q4, Q6 and
|
||||
Q8 remain open and block Phases 4, 5b, 2 and 8 respectively. Per CLAUDE.md § Conventions, no implementation
|
||||
starts without the org lead's approval of the phase it belongs to.
|
||||
|
||||
**Branching:** every phase lands on **`edge`** in its repo; `main` is touched once, by the cutover
|
||||
(Phase 13). §6.0a records the blocking precondition — every existing `edge` is stale and three repos
|
||||
have none.
|
||||
(Phase 13). §6.0a records the blocking precondition — six `edge` branches are stale and two repos have
|
||||
none.
|
||||
|
||||
**Scope decisions, settled by the org lead (2026-08-28):**
|
||||
|
||||
@@ -19,9 +21,25 @@ have none.
|
||||
5. **The system ships with a seeded set of working templates and an editor**, so a fresh deployment
|
||||
sends correctly-branded mail before anyone opens the editor. See §4.6.
|
||||
|
||||
**Four further decisions, settled 2026-08-28** (recorded in full at §7.1, with the findings that shaped
|
||||
them in §0.6):
|
||||
|
||||
6. **Engagement mail is opt-in only, and `users.email` becomes UNIQUE.** Standard marketing-email practice
|
||||
applies: explicit opt-in, working unsubscribe, suppression. Uniqueness is not a detail — it is
|
||||
**Phase 1b**, because the column is nullable-and-duplicated by design today and three code paths break
|
||||
the moment an index is added. Whether an *unverified* address may receive opt-in mail is an **admin
|
||||
setting**, defaulting **on for fresh installs and off for upgrades**.
|
||||
7. **There is no campaigns surface.** No operator-authored send screen, no free-form list building. An
|
||||
admin's expressive power lives in trigger conditions and rules.
|
||||
8. **Audiences are module-declared and operator-composable.** A module registers named, queryable
|
||||
audiences over *its own* data (`uo.team.members`, `uo.governors`); core exposes the same registration
|
||||
surface to every module and learns no game vocabulary. An operator may compose declared audiences with
|
||||
and/or/not into a saved segment — and the composed result is still bounded by the trigger's G24
|
||||
audience ceiling. See §5.1a.
|
||||
|
||||
---
|
||||
|
||||
## Part 0 — Five findings that contradict the brief
|
||||
## Part 0 — Six findings that contradict the brief
|
||||
|
||||
Stated up front because the rest of the document is shaped by them.
|
||||
|
||||
@@ -129,6 +147,54 @@ So the engagement additions take **1.7.0** — additions only (`api.registerEven
|
||||
|
||||
---
|
||||
|
||||
### 0.6 A UNIQUE email is not a one-line ALTER — it breaks three paths and can stop a boot
|
||||
|
||||
Decision 6 (opt-in only, unique addresses) reads like a schema tweak. It is not. `users.email` is
|
||||
`VARCHAR(255) NULL` with **no** unique index, and `schema.sql:24` says so deliberately: *"Optional
|
||||
contact email (players). Not unique — SSO emails may repeat."* Adding the index touches registration,
|
||||
SSO provisioning and the upgrade path. All four findings below were read out of the tree on 2026-08-28.
|
||||
|
||||
**1. The boot-time `ALTER` is how schema reaches a deployment, and it would fail loudly.** Upgrades ride
|
||||
the idempotent `ALTER TABLE … IF NOT EXISTS` block at `schema.sql:1409+`, executed by `ensureSchema()`
|
||||
on **every** boot (`server/src/server.js:66`). `ADD UNIQUE INDEX` against a table that already holds
|
||||
duplicate addresses errors, `ensureSchema()` throws, and **the site does not start**. A de-duplication
|
||||
step must run before the index, in the same release — see Phase 1b.
|
||||
|
||||
**2. `isDuplicateUsername()` does not inspect which index collided.** `users.model.js:24` is
|
||||
`err.code === 'ER_DUP_ENTRY' || err.errno === 1062` and nothing more. Two callers misread an email
|
||||
collision as a username collision the instant the index exists:
|
||||
|
||||
| Site | Today | After `UNIQUE(email)`, unfixed |
|
||||
| --- | --- | --- |
|
||||
| `auth.controller.js:137` (register) | 409 *"That username is already taken."* on a genuine username race | Same message for a duplicate **email** — wrong, and it misattributes the conflict to the one field the user did not collide on |
|
||||
| `sso.controller.js:197` (`provisionSsoPlayer`) | Retries the next username suffix on collision | Retries **usernames** for an **email** conflict, which can never clear; burns `PROVISION_MAX_TRIES` and returns `null`, so SSO sign-up fails opaquely with the log blaming usernames |
|
||||
|
||||
The fix is to distinguish the constraint (read the index name off the driver error) before Phase 1b adds
|
||||
the index — not after.
|
||||
|
||||
**3. SSO auto-provisioning is the source of the duplicates, and CLAUDE.md is stale about it.** CLAUDE.md
|
||||
states *"identities are never auto-provisioned"*. `provisionSsoPlayer` (`sso.controller.js:197`) does
|
||||
exactly that whenever `player_registration ∈ {sso, both}`, writing `profile.email` straight into
|
||||
`users.email`. It also sets `emailVerified: Boolean(profile.email)` — **verified because an address was
|
||||
present, not because the IdP asserted `email_verified`**. That matters twice over: it manufactures the
|
||||
duplicates Phase 1b must clean up, and it makes `email_verified` too weak a signal to arbitrate *which*
|
||||
duplicate wins (which is why §7.1 Q1's answer is oldest-wins, not verified-wins).
|
||||
|
||||
**4. There is no self-serve email flow at all.** No route lets a user set or change their own address
|
||||
after signup — `router/v1/player/account.router.js` has none, and `users.model.js:72`'s `update()` is
|
||||
reached only by admin user management. An address is captured once, at registration or SSO provisioning,
|
||||
and is thereafter unchangeable by its owner. A verification gate presupposes a change-and-verify flow,
|
||||
so Phase 1b builds one; it is not an add-on to an existing screen.
|
||||
|
||||
**One consequence for the error surface.** A unique constraint needs a user-facing failure, and the
|
||||
obvious wording (*"that email is already registered"*) makes account existence queryable — a step back
|
||||
from a posture the codebase holds deliberately elsewhere (`passwordReset.controller.js` answers a generic
|
||||
200 *"to avoid account enumeration"*). §7.1 Q1 settles it: the message stays generic, the real reason is
|
||||
logged not returned, the endpoint stays rate-limited, and the failure is **not** fed to the bot scorer —
|
||||
an honest typo on a taken address must not push a legitimate user toward an IP ban.
|
||||
|
||||
---
|
||||
|
||||
## Part 1 — Current-state map
|
||||
|
||||
### 1.1 Notification system, end to end
|
||||
@@ -746,6 +812,11 @@ CREATE TABLE IF NOT EXISTS engagement_rules (
|
||||
name VARCHAR(160) NOT NULL,
|
||||
enabled TINYINT(1) NOT NULL DEFAULT 0, -- OFF by default; an operator turns it on
|
||||
audience VARCHAR(32) NOT NULL DEFAULT 'owner',
|
||||
audience_segment_id INT NULL, -- a composed segment (§5.1a); NULL = the plain audience above
|
||||
-- §7.1 Q3: the hard stop that makes operator-editable rules safe to choose over
|
||||
-- code-registered ones. Counted in engagement_sends, enforced before the outbox
|
||||
-- row is written, never overridable from the rule editor beyond this column.
|
||||
max_sends_per_hour INT NOT NULL DEFAULT 100,
|
||||
channels JSON NOT NULL, -- ['email','inapp'] — a rule may span channels
|
||||
template_keys JSON NOT NULL, -- { email: 'idoc-warning', inapp: 'idoc-warning-short' }
|
||||
conditions JSON NULL, -- declared-variable predicates, e.g. decayStatus in [Greatly, IDOC]
|
||||
@@ -759,6 +830,22 @@ CREATE TABLE IF NOT EXISTS engagement_rules (
|
||||
INDEX idx_engr_trigger (trigger_id, enabled)
|
||||
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
|
||||
|
||||
-- §5.1a: an operator-composed segment over module-declared audiences. Stored as a
|
||||
-- boolean tree of audience ids + params; `ceiling` is DERIVED at save time as the
|
||||
-- NARROWEST ceiling in the tree and re-checked against the trigger's own ceiling,
|
||||
-- so composition can never widen. It is a column rather than a runtime computation
|
||||
-- so an audit can read what a rule was allowed to reach without re-resolving it.
|
||||
CREATE TABLE IF NOT EXISTS engagement_audience_segments (
|
||||
id INT AUTO_INCREMENT PRIMARY KEY,
|
||||
name VARCHAR(160) NOT NULL,
|
||||
expression JSON NOT NULL, -- { op: 'and'|'or'|'not', nodes: [...] | { audienceId, params } }
|
||||
ceiling VARCHAR(32) NOT NULL, -- derived, never operator-typed
|
||||
updated_by INT NULL,
|
||||
created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
|
||||
updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
|
||||
CONSTRAINT fk_engseg_user FOREIGN KEY (updated_by) REFERENCES users(id) ON DELETE SET NULL
|
||||
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
|
||||
|
||||
-- G8/G9: the channel dimension notification_subscriptions lacks.
|
||||
CREATE TABLE IF NOT EXISTS notification_channel_prefs (
|
||||
user_id INT NOT NULL,
|
||||
@@ -921,7 +1008,7 @@ supply markup.
|
||||
|
||||
## Part 5 — The module registration mechanism
|
||||
|
||||
### 5.1 Two additions to the contract, both modelled on what already works
|
||||
### 5.1 Three additions to the contract, all modelled on what already works
|
||||
|
||||
```js
|
||||
// api — what the module registers (MODULE_API.md §2.4). Modelled on
|
||||
@@ -936,6 +1023,10 @@ ctx.events.emit(triggerId, { subject, data, ownerUserId?, dedupeKey?, occurredAt
|
||||
// ctx — the in-app sink, for a module that wants to write the inbox directly
|
||||
// without a rule. Optional; most modules will only emit.
|
||||
ctx.inbox.push(userId, { triggerId, title, body, url, dedupeKey })
|
||||
|
||||
// api — the audiences a module can resolve over its own data (decision 8).
|
||||
// Same registration discipline as the triggers above; see §5.1a.
|
||||
api.registerAudiences([{ id, label, description, ceiling, resolve }])
|
||||
```
|
||||
|
||||
**Why a new surface rather than extending `registerNotificationStreams`.** A stream entry is a
|
||||
@@ -956,6 +1047,46 @@ streams and the Discord leg. That is not ceremony — `registries.js`'s header s
|
||||
only core's hardcoded base bypasses is a registry whose first real exercise is a module, which is the drift
|
||||
this PR exists to prevent."
|
||||
|
||||
### 5.1a Audiences — module-declared, operator-composable *(decision 8)*
|
||||
|
||||
The org lead's correction to Q7 is precise and worth stating exactly: **there is no campaign surface, but
|
||||
lists exist — powered by game data, through the module, on a surface every module shares.** "Team X's
|
||||
members" and "the governors" are legitimate audiences; "everyone who opened the last mail" is not, and
|
||||
nothing here builds it.
|
||||
|
||||
```js
|
||||
api.registerAudiences([{
|
||||
id: 'team.members', // namespaced() prefixes it → 'uo.team.members'
|
||||
label: 'Members of a team',
|
||||
params: [{ id: 'teamId', type: 'int', required: true }],
|
||||
ceiling: 'members', // the widest this audience can EVER resolve to (G24)
|
||||
resolve: async (params, ctx) => [/* user ids */],
|
||||
}])
|
||||
```
|
||||
|
||||
**Four rules, each of which exists because of something already in the tree:**
|
||||
|
||||
1. **Core learns no game vocabulary.** Core never knows what a governor is; it knows an id, a label and
|
||||
a `resolve` it may call. This is the same boundary `registerNotificationStreams` holds, and
|
||||
`check:modules` already proves core's own ids name no game concept.
|
||||
2. **The resolver returns user ids and nothing else.** It is not handed a template, a channel or an
|
||||
address, and it cannot enumerate them — a module still cannot send mail (§1.2), and this must not
|
||||
become the back door that lets it. The engine maps ids to addresses on core's side, after
|
||||
preferences, suppression and the verification gate.
|
||||
3. **A composed segment is bounded by the *narrowest* ceiling it contains, not the widest.** Operators
|
||||
may combine declared audiences with and/or/not into a saved segment. That is real power and it is the
|
||||
part with a security edge: composition must never *widen*. `A OR B` takes the tighter of the two
|
||||
ceilings, and the result is still checked against the trigger's own G24 ceiling before a rule using it
|
||||
can be saved. Union-widens is the intuitive implementation and it is the wrong one.
|
||||
4. **An audience whose module is uninstalled goes dormant, exactly as a rule does** (§7.3). It resolves
|
||||
to the empty set and the rule referring to it shows as dormant — never an error, never auto-deleted,
|
||||
never a silent send to a *different* set of people because the id stopped resolving.
|
||||
|
||||
**Where it lands.** The registration surface and the ceiling arithmetic belong in **Phase 2**, with the
|
||||
trigger declaration — G24's reasoning applies unchanged, and both are cheap now and expensive to retrofit
|
||||
into a rule model that already has rows in it. The composition UI belongs with the rules screen in
|
||||
**Phase 4**. `module-uo`'s first real audiences come in **Phase 11**.
|
||||
|
||||
### 5.2 The seam, end to end
|
||||
|
||||
```
|
||||
@@ -992,8 +1123,8 @@ learn. The module resolves and passes `ownerUserId`; core never sees `ownerAcct`
|
||||
|
||||
### 5.3 What this costs the contract
|
||||
|
||||
`MODULE_API_VERSION` 1.6.0 → **1.7.0**. Additions only (`registerEventTriggers`, `ctx.events.emit`,
|
||||
`ctx.inbox.push`), no removal, no changed signature ⇒ minor by §1.1's table. `module-uo`'s
|
||||
`MODULE_API_VERSION` 1.6.0 → **1.7.0**. Additions only (`registerEventTriggers`, `registerAudiences`,
|
||||
`ctx.events.emit`, `ctx.inbox.push`), no removal, no changed signature ⇒ minor by §1.1's table. `module-uo`'s
|
||||
`coreApi: "^1.3.0"` still resolves, so no module is broken by the bump.
|
||||
|
||||
Knock-on obligations:
|
||||
@@ -1017,7 +1148,7 @@ regenerated when a route changes, `npm run routes:manifest -- --check` clean, `n
|
||||
clean, **the documentation edits §6.0b assigns it**, Conventional Commits, the AI-disclosure trailer,
|
||||
and a branch cut from a freshly-pulled base.
|
||||
|
||||
**Stage A (1–2) is prerequisite. Stage B (3–6) is the engagement system. Stage C (7–8) is the in-app
|
||||
**Stage A (1–1b–2) is prerequisite. Stage B (3–6) is the engagement system. Stage C (7–8) is the in-app
|
||||
channel. Stage D (9) is deliverability. Stage E (10–11) is the shard enrichment and runs in parallel
|
||||
from day one. Stage F (12–13) is the public site and the cutover.**
|
||||
|
||||
@@ -1037,8 +1168,14 @@ different days. `main` must never hold a half-applied set of them.
|
||||
refreshed after merging. **Fast-forward each `edge` to `main` before the first phase PR** — it is
|
||||
lossless (0 ahead), and skipping it means the cutover diff carries stale content or conflicts that
|
||||
have nothing to do with this workstream.
|
||||
2. **Three repos have no `edge` at all** and need one cut from `main`: `android-app` (its M12 branch
|
||||
was deleted after that cutover), `runicgateway.com`, and `Integration-kit`.
|
||||
2. **Two repos have no `edge` at all** and need one cut from `main`: `runicgateway.com` and
|
||||
`Integration-kit`. *(Corrected 2026-08-28: `android-app` **does** have an `edge`, 1 behind `main`, so
|
||||
it is a fast-forward like the rest. The claim that its M12 branch was deleted after that cutover was
|
||||
wrong.)*
|
||||
|
||||
**Phase -1 as actually measured, 2026-08-28.** Six fast-forwards — `module-uo` 9 behind, `installer` 7,
|
||||
`servuo-plugins` 7, `website` 5, `link` 3, `android-app` 1 — and two branch creations. `docs`' `edge` is
|
||||
already done and is 3 ahead of `main`. Every one is 0 ahead, so all eight are lossless.
|
||||
|
||||
**Android CI does not run on `edge`.** `android-app/.gitea/workflows/pr-checks.yml` triggers only on
|
||||
PRs into `main`, so every Phase 8 PR lands with **zero CI** and the cutover is the first real run. That
|
||||
@@ -1058,6 +1195,7 @@ change is not complete until `docs/` reflects it" — is the floor; this table i
|
||||
| Phase | `docs/` | Other repos |
|
||||
| --- | --- | --- |
|
||||
| **1** Remove Gmail OAuth2, SMTP | `website/BACKEND_DESIGN.md` §7 **rewritten** (not amended — it documents Gmail OAuth2 as *the* mechanism); route tables lose `/admin/email/connect/*` | `website/README.md` + `.env.example` wherever they point at Connect Gmail · **`runicgateway.com`**: `notifications-and-email.mdx` (its "There is no SMTP option" aside is now false), `configuration.mdx:62`, `troubleshooting.mdx:101`, `system-architecture.mdx:117` · a release note |
|
||||
| **1b** Unique email | `website/BACKEND_DESIGN.md` — the `users` table (the "not unique" note is now false), the new change/verify routes, and the de-dupe migration as an operator-visible upgrade step | `website/README.md` upgrade notes · a release note naming the admin report and the verification-gate default |
|
||||
| **2** Trigger registry | `website/MODULE_API.md` §1.1 (**1.7.0** + correct the stale "1.6.0 has only ever been on `edge`" paragraph), §2.3 (`ctx.events`, `ctx.inbox`), §2.4 (`registerEventTriggers`), §7.3's dormant-rule note · `website/ENGAGEMENT.md` §4.3 kept true | `Integration-kit`: `ci/core-ref.json` re-pinned (the equality check goes red **on purpose**) + chapter 2 gains a "registering a trigger" section · **`runicgateway.com`**: `platform.json.moduleApi` → 1.7.0 |
|
||||
| **3** Channel preferences | `website/BACKEND_DESIGN.md` route table · `android/PLAN.md` §11 | — |
|
||||
| **4** Engine | `website/ENGAGEMENT.md` (rules/cooldown/outbox as built) · `BACKEND_DESIGN.md` table inventory | — |
|
||||
@@ -1065,7 +1203,7 @@ change is not complete until `docs/` reflects it" — is the floor; this table i
|
||||
| **6** Email channel + Teams migration | `website/TEAMS.md` §6.3/§6.4 **rewritten** — the Team pipeline it describes no longer exists as its own thing | **`runicgateway.com`**: `administration/teams.mdx` notification section |
|
||||
| **7** In-app channel (core+web) | `website/BACKEND_DESIGN.md` routes + tables · `website/ENGAGEMENT.md` | **`runicgateway.com`**: `notifications-and-email.mdx` gains the in-app channel |
|
||||
| **8** In-app (Android) | `android/PLAN.md` | `android-app/README.md` |
|
||||
| **9** Deliverability | `website/BACKEND_DESIGN.md` §7 · a suppression/bounce operator section | **`runicgateway.com`**: `troubleshooting.mdx` gains bounce/suppression · **`PLAY_DATA_SAFETY.md` + `/privacy`** — see Phase 12 |
|
||||
| **9** Deliverability | `website/BACKEND_DESIGN.md` §7 · a suppression/bounce operator section (the verification flow is Phase 1b's) | **`runicgateway.com`**: `troubleshooting.mdx` gains bounce/suppression · **`PLAY_DATA_SAFETY.md` + `/privacy`** — see Phase 12 |
|
||||
| **10** Protocol bump | `link/INTEGRATION.md` §Housing (table + example) · `link/PLAN.md` §5/§7 · a `link/v5.md` if the bump earns its own design doc, as v3 and v4 did | `servuo-plugins/overlay.toml` · **`runicgateway.com`**: `platform.json.protocol` → 5, `bundle.*`, `architecture/protocol-versions.mdx` |
|
||||
| **11** module-uo triggers | `modules/uo/API.md` · `modules/uo/README.md` | `module-uo/README.md` |
|
||||
| **12** Public site | — | **`runicgateway.com`**, in full — see the phase |
|
||||
@@ -1084,8 +1222,9 @@ This document, landed as `docs/website/ENGAGEMENT.md` with the five settled deci
|
||||
top. No code.
|
||||
|
||||
**Acceptance:** merged into `docs/`; §7.1's open questions each answered or explicitly deferred before
|
||||
the phase that depends on them starts — Q5 before Phase 1, Q1/Q3/Q6/Q7 before Phase 2, Q2 before
|
||||
Phase 4, Q4 before Phase 5b.
|
||||
the phase that depends on them starts. **Q1, Q3, Q5 and Q7 were answered on 2026-08-28** (§7.1), which
|
||||
unblocked Phases 1, 2 and 4 and added Phase 1b. Still outstanding: **Q6 before Phase 2**, **Q2 before
|
||||
Phase 4**, **Q4 before Phase 5b**, **Q8 before Phase 8**.
|
||||
|
||||
---
|
||||
|
||||
@@ -1116,6 +1255,55 @@ deployment does today — contact form falls back to `mailto`, invites surface t
|
||||
|
||||
---
|
||||
|
||||
### Phase 1b — Unique, changeable, verifiable email addresses *(decision 6)*
|
||||
|
||||
**Lands alone, between 1 and 2, and before any engagement mail exists.** It touches registration, SSO
|
||||
provisioning and the boot-time schema path — three security-sensitive surfaces — and retrofitting
|
||||
uniqueness *after* a send log and a suppression list hold rows is strictly worse than doing it now.
|
||||
§0.6 is the finding this phase discharges.
|
||||
|
||||
**Four pieces, in this order within the PR:**
|
||||
|
||||
1. **Distinguish the constraint before adding one.** Replace `isDuplicateUsername()`'s bare
|
||||
`ER_DUP_ENTRY`/`1062` test with a check that reads the violated index off the driver error, and give
|
||||
its two callers (`auth.controller.js:137`, `sso.controller.js:197`) separate branches. **This must be
|
||||
in the tree before the index is**, or the register path starts lying and SSO sign-up starts failing
|
||||
opaquely the moment the ALTER runs.
|
||||
2. **De-duplicate, then index.** A migration step that runs *before* the `ALTER`: for each duplicated
|
||||
address, the **earliest-created** account keeps it; every later duplicate has `email` set to `NULL`
|
||||
and `email_verified` to `0`. Multiple `NULL`s are legal under a UNIQUE index, so nobody loses an
|
||||
account and nothing cascades. The affected accounts are written to an admin-visible report — *who*
|
||||
was cleared and *what* address they lost — because they are exactly the users who must be contacted.
|
||||
Then `ALTER TABLE users ADD UNIQUE INDEX IF NOT EXISTS uq_users_email (email)`, pinned to a
|
||||
case-insensitive collation for the same reason `username` was (`schema.sql:24`): `Foo@x.com` and
|
||||
`foo@x.com` are one mailbox everywhere that matters, and folding must happen in the index rather than
|
||||
in application code that can be bypassed.
|
||||
3. **A self-serve change-and-verify flow**, which does not exist today (§0.6 finding 4). Set/change
|
||||
address, a signed time-boxed verification link, `email_verified` set only on link use. SSO's
|
||||
`emailVerified: Boolean(profile.email)` is corrected at the same time to honour the IdP's actual
|
||||
`email_verified` claim rather than the mere presence of an address.
|
||||
4. **The verification gate as an admin setting** — `on` for fresh installs, `off` for upgrades, so the
|
||||
live deployment does not silently stop mailing its existing opted-in users on the day it upgrades.
|
||||
The asymmetry is deliberate and is the same lesson as G22: a safe default must not be applied
|
||||
retroactively to a running system without telling anyone.
|
||||
|
||||
**The error surface stays anti-enumeration.** A collision returns a generic failure, the real reason is
|
||||
logged and not returned, the endpoint keeps its rate limit, and the failure is **not** scored by the bot
|
||||
detector — a legitimate user typing a colleague's address must not be pushed toward an IP ban for it.
|
||||
|
||||
**Acceptance:** a DB seeded with three accounts sharing an address boots clean, keeps the oldest, nulls
|
||||
two, and lists both in the admin report; registering with a taken address returns the generic failure,
|
||||
logs the specific one, increments no bot score, and does **not** say "username"; SSO sign-up with an
|
||||
address already held by another account fails with a distinguishable reason rather than exhausting
|
||||
`PROVISION_MAX_TRIES`; a user can change their address and it stays `email_verified = 0` until the link
|
||||
is used; `Foo@x.com` collides with `foo@x.com`; an upgraded install has the gate `off` and a fresh one
|
||||
`on`.
|
||||
**Guardrails:** swagger regen + `routes:manifest --check` (the change/verify routes are new); the
|
||||
migration is idempotent and re-running `ensureSchema()` is a no-op; **no destructive DDL** — the
|
||||
de-dupe nulls a column, it never deletes a row.
|
||||
|
||||
---
|
||||
|
||||
### Phase 2 — The trigger registry and the variable contract
|
||||
|
||||
`api.registerEventTriggers` + `ctx.events.emit` in `modules/registries.js` and `modules/loader.js`;
|
||||
@@ -1127,8 +1315,13 @@ A trigger declaration also carries its **audience ceiling** (G24) — the widest
|
||||
give it — and its `kind` (`event` now, `scheduled` reserved for G25). Both are cheap here and expensive
|
||||
to retrofit into the rule model later.
|
||||
|
||||
Phase 2 also lands `api.registerAudiences` and the ceiling arithmetic (§5.1a) — the same registration
|
||||
discipline, and the same "cheap now, expensive later" argument G24 makes for the trigger ceiling.
|
||||
|
||||
**Acceptance:** core's triggers appear in `GET /admin/engagement/triggers`; a module registering an
|
||||
un-namespaced trigger fails to load with the holder named; a payload missing a `required` variable
|
||||
un-namespaced trigger or audience fails to load with the holder named; **an `A OR B` composition takes
|
||||
the narrower of the two ceilings, not the wider**; an audience whose module is uninstalled resolves
|
||||
empty and shows dormant rather than erroring; a payload missing a `required` variable
|
||||
throws in dev and is dropped+logged in prod; **a rule cannot be saved with an audience wider than its
|
||||
trigger's ceiling**; `engagement-triggers.json` diffs zero in CI.
|
||||
**Guardrails:** the new manifest `--check` (this is where the "manifest-style guardrail" the brief asks
|
||||
@@ -1238,16 +1431,20 @@ branch get **no CI** — the cutover PR is the first real run. Plan for that.
|
||||
|
||||
---
|
||||
|
||||
### Phase 9 — Deliverability: suppression, bounces, verification
|
||||
### Phase 9 — Deliverability: suppression and bounces
|
||||
|
||||
`engagement_suppressions`, bounce/complaint capture per transport (SMTP has none — this is where the
|
||||
API-based providers earn their place), and an email-verification flow so `users.email_verified` finally
|
||||
has a writer for self-registered addresses. Policy decision needed: **may an unverified address receive
|
||||
engagement mail at all?** (Recommendation: transactional yes, engagement no.)
|
||||
`engagement_suppressions` and bounce/complaint capture per transport (SMTP has none — this is where the
|
||||
API-based providers earn their place).
|
||||
|
||||
**The verification flow is no longer part of this phase.** §7.1 Q1's answer moved it forward into
|
||||
**Phase 1b**, along with the admin gate setting that decides whether an unverified address is excluded.
|
||||
Phase 9 therefore *consumes* `email_verified` rather than introducing a writer for it, and — the reason
|
||||
this reshuffle is worth it — **Phase 9 no longer blocks Phase 11.** The first real rule can ship on the
|
||||
verification mechanism 1b already built, with bounces following.
|
||||
|
||||
**Acceptance:** a suppressed address is skipped with `status='suppressed'` in `engagement_sends` and no
|
||||
transport call; a hard bounce suppresses the address; an unverified address is excluded from engagement
|
||||
rules but still receives password resets.
|
||||
transport call; a hard bounce suppresses the address; with the Phase 1b gate `on`, an unverified address
|
||||
is excluded from engagement rules but still receives password resets; with it `off`, it receives both.
|
||||
|
||||
---
|
||||
|
||||
@@ -1379,7 +1576,7 @@ is then fast-forwarded to `main` again so the next workstream starts from a clea
|
||||
Phase -1 fast-forward every edge to main; create edge in android-app,
|
||||
runicgateway.com and Integration-kit ← blocking, §6.0a
|
||||
|
||||
Stage A 1 ── 2
|
||||
Stage A 1 ── 1b ── 2
|
||||
Stage B └─ 3 ── 4 ── 5a ── 5b ── 6
|
||||
Stage C └─ 7 ── 8 (8 = app-store cadence)
|
||||
Stage D └─ 9
|
||||
@@ -1393,7 +1590,9 @@ Stage F 12 ── 13 (12 written before
|
||||
**Phase -1 is blocking and takes minutes.** Every `edge` is 0 ahead / 3–16 behind `main` (§6.0a), so
|
||||
the fast-forward is lossless; skipping it means the cutover diff carries other workstreams' leftovers.
|
||||
|
||||
Phases 1, 2 and 10 can start immediately and in parallel. **Phase 1 and Phase 6 are the two that touch
|
||||
Phases 1, 1b and 10 can start immediately and in parallel; **Phase 2 now follows 1b** rather than 1,
|
||||
because a trigger's audience resolves to users and the identity those users are mailed at should be
|
||||
unique and verifiable before anything resolves an audience over it. **Phase 1 and Phase 6 are the two that touch
|
||||
mail people actually receive** and should each land alone: Phase 1 because it can silently stop email
|
||||
for the live deployment (§1.2a), Phase 6 because it rewrites the pipeline behind notifications going
|
||||
out today. Both get the local rig exercised before merge, not only tests.
|
||||
@@ -1412,31 +1611,53 @@ day it ships.
|
||||
|
||||
## Part 7 — Open questions and forward-compat notes
|
||||
|
||||
### 7.1 Questions for the org lead (not blocking Phase 1 or 2)
|
||||
### 7.1 Questions for the org lead — five answered 2026-08-28, four still open
|
||||
|
||||
1. **May unverified addresses receive engagement mail?** Recommendation: no — transactional only. This
|
||||
decides whether Phase 9 blocks Phase 11 or merely follows it.
|
||||
1. ✅ **ANSWERED — may unverified addresses receive engagement mail?** *"Emails need to be unique and
|
||||
verification blocking sending is an admin setting."* Combined with the opt-in answer, this settles
|
||||
three things at once and creates **Phase 1b**:
|
||||
- **Opt-in only**, following standard marketing-email practice — explicit consent, working
|
||||
unsubscribe, suppression. This matches the `team_notification_prefs` pattern already in use.
|
||||
- **`users.email` becomes UNIQUE.** Not a schema tweak — see §0.6 for the three code paths it breaks
|
||||
and the boot it can stop, and Phase 1b for the work. Duplicates are resolved **oldest-wins**: the
|
||||
earliest account keeps the address, later ones are nulled and listed in an admin report.
|
||||
- **The verification gate is an admin setting**, default **on for fresh installs, off for upgrades**,
|
||||
so a running deployment does not silently stop mailing its opted-in users at cutover.
|
||||
- The collision error stays **generic and anti-enumeration**, rate-limited but **not bot-scored**.
|
||||
|
||||
*Consequence for ordering:* Phase 9 no longer blocks Phase 11 — the verification *mechanism* moves
|
||||
forward into 1b, and Phase 9 keeps only bounces and suppression.
|
||||
2. **Multi-instance.** Do we commit to single-instance (status quo) and document it, or add
|
||||
`SELECT … FOR UPDATE SKIP LOCKED` to the outbox sweep in Phase 4? Recommendation: add it in Phase 4 —
|
||||
it is cheap there and expensive to retrofit after mail has doubled once.
|
||||
3. **Rules as data vs. rules as code.** The plan makes rules operator-editable rows. The alternative is
|
||||
rules registered in code by whoever owns the trigger, with only enable/disable in the DB. Data is more
|
||||
flexible; code is far easier to test and impossible to misconfigure into a mail storm.
|
||||
Recommendation: data, but with `enabled` defaulting to 0 and a hard per-rule hourly send ceiling.
|
||||
3. ✅ **ANSWERED — rules as data vs. rules as code.** Data, as recommended: rules are operator-editable
|
||||
rows, `enabled` defaults to `0`, and every rule carries a **hard per-rule hourly send ceiling**. The
|
||||
ceiling is not a nicety — it is the thing that keeps a misconfigured rule from becoming a mail storm,
|
||||
and it is what makes "data" safe enough to choose over "code". Phase 4 builds both.
|
||||
4. **Does the engagement admin surface belong under Admin → Settings, or its own top-level section?**
|
||||
It is three screens (Triggers, Rules, Templates) plus a send log.
|
||||
5. **Which SMTP posture is the documented default for operators?** Their own mail server, a relay
|
||||
(Mailgun/SES/Postmark) over SMTP, or Gmail-with-an-app-password. Recommendation: document all three,
|
||||
lead with a relay, and name Gmail-app-password explicitly as the migration path off OAuth2 for the
|
||||
existing deployment (§1.2a).
|
||||
5. ✅ **ANSWERED — which SMTP posture is the documented default?** Document all three; **lead with a
|
||||
relay** (Mailgun/SES/Postmark); name Gmail-with-an-app-password (`smtp.gmail.com:587`) explicitly as
|
||||
the migration path off OAuth2 for the existing deployment (§1.2a). Phase 1 owes all three in
|
||||
`runicgateway.com`'s `notifications-and-email.mdx` as well as `BACKEND_DESIGN.md` §7.
|
||||
6. **Time-based triggers (G25) — design now, build when?** Recommendation: design the declaration in
|
||||
Phase 2 so `kind: 'scheduled'` exists in the contract, build the evaluator after Phase 9. The
|
||||
lifecycle uses in §8.5 are the highest-value non-game triggers on the list and the first thing anyone
|
||||
will ask for after the IDOC mail works.
|
||||
7. **Manual/operator-authored sends** (§8.5, last paragraph) — in or out? It is the one campaign-shaped
|
||||
feature, it is genuinely useful, and it is also the thin end of the wedge the brief rules out.
|
||||
Recommendation: in, but as a `scheduled` trigger with an explicit staff audience and no list
|
||||
building — never a separate "campaigns" surface.
|
||||
7. ✅ **ANSWERED — manual/operator-authored sends.** *"There is no campaign in the normal sense of email
|
||||
marketing. But admins can create all sorts of trigger conditions"*, and separately: *"lists can be
|
||||
built if they are powered by game data — say team X members or governors or whatever — thru the uo
|
||||
module; same surface will be exposed to all modules."*
|
||||
|
||||
So: **no campaigns surface, no operator-authored send screen, no free-form list building.** An
|
||||
admin's expressive power lives in trigger conditions and rules. `kind: 'scheduled'` therefore stays in
|
||||
the Phase 2 contract as an admin-definable *condition*, not as a campaign.
|
||||
|
||||
**Lists do exist, module-declared.** A module registers named audiences over its own data on a surface
|
||||
core exposes to every module, and an operator may compose them with and/or/not into a saved segment.
|
||||
Composition must **narrow, never widen** — the segment takes the tightest ceiling it contains and is
|
||||
still checked against the trigger's G24 ceiling. §5.1a is the design; Phase 2 owns the surface,
|
||||
Phase 4 the composition UI, Phase 11 `module-uo`'s first real audiences.
|
||||
8. **Android CI on `edge`** (§6.0a). `android-app/.gitea/workflows/pr-checks.yml` triggers only on PRs
|
||||
into `main`, so Phase 8 lands with zero CI and Phase 13 is its first real build — as happened to all
|
||||
nine M12 phase PRs. Fix the trigger as Phase 8's first commit, or accept it deliberately?
|
||||
|
||||
Reference in New Issue
Block a user