Files
website/server/src/model/engagement/engagementRules.model.js
wtclaude eec7dbf785
All checks were successful
PR Checks / client-build (pull_request) Successful in 35s
PR Checks / server-tests (pull_request) Successful in 5m29s
PR Checks / bot-tests (pull_request) Successful in 8m30s
fix(engagement): claim the seed guard atomically, and let a rule name a digest body
Both defects came out of Phase 13's acceptance walk, and neither was visible to
any test.

**1. The one-shot rule-group guard was not a guard.** `seedRuleGroup` and
`coreRules.seedGroup` both read their settings stamp, inserted the whole group,
and wrote the stamp AFTER the loop. Two instances booting in the same moment both
read "absent" and both insert -- the walk ended up with 52 module rules where the
module ships 26. `docker compose up --scale app=2` and a rolling restart both
start two instances on purpose, so this is ordinary rather than exotic.

`settings.db` gains `claim(key, value)`: the same `INSERT IGNORE` as
`seedDefault`, reporting its own `affectedRows`, so exactly one caller can win a
key. The atomicity is the PRIMARY KEY's -- no transaction, no lock, the same
bargain `engagementWorker`'s row claim already makes. Both seeders claim before
inserting.

The trade the code already documented is unchanged, only its order: a process
that dies mid-loop leaves the group stamped and partly seeded, and the missing
rules are an operator's visit to the "new rule" form. A duplicate rule is two
mails per event, for every rule in the group, which both functions' own comments
already call the worse outcome.

**Why no test caught it:** the stubs supplied `get` and `set` over a Map, which
cannot race, so they agreed with the bug -- Phase 4a's `foundRows` finding in a
different costume. The fake is now `claim`-shaped and decides without yielding,
exactly as the table does, and both suites gained a test that runs two seeders
with `Promise.all` and asserts one insert each. Verified by reverting the fix:
the module test then reports every rule inserted twice.

**2. A rule that names a `digest` body could not be saved.** `registries.js`
`checkSeedRule` permits `digest` in as many words -- it is the body
`teamDigestWorker` renders for a rule whose email channel an individual set to
digest mode, so it never appears in `channels` and never could -- and
MODULE_API.md 2.4 tells modules they may point `template_keys` at
`notify.digest`. `engagementRules.model.js` then rejected any key that was not
one of the rule's channels.

So every rule shipping a digest body answered an operator who opened it and
pressed Save with a 400 naming a key they had never typed: core's own Team and
news rules, and sixteen of module-uo's. The only way to save was to delete the
digest body, silently dropping digest support from that rule. The two validators
now agree; anything that is neither a channel nor `digest` is still refused, with
a test for each direction.

No MODULE_API bump: no member is added, removed or changed, and the documented
contract is what the code now honours rather than something new.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-09-01 14:29:21 -05:00

330 lines
15 KiB
JavaScript

// ── Engagement rules — the save path ───────────────────────────────────────
//
// ENGAGEMENT.md §4.5 / §7.1 Q3, Phase 4a. A rule is **operator-editable data**,
// not code, and that was a deliberate choice with a condition attached: it is
// safe to choose only because `enabled` defaults to 0 and every rule carries a
// hard per-hour send ceiling. Both of those live in this file's validation, not
// in the screen that calls it - Phase 4b builds a form over this, and a rule that
// arrives by any other route (a restore, a fixture, a future import) gets the
// same answer.
//
// **Every check here is a boundary, not a convenience.** The rule editor will
// re-implement some of them for the sake of a good error message, and that
// second copy is expected to drift - so this one is the one that decides.
//
// The check with teeth is the ceiling (G24): an operator may narrow a rule's
// audience as much as they like and may never widen it past what the trigger
// declared. `ceilings.permits` is that arithmetic, `segments.validate` derives
// it for a composed audience, and the engine re-runs the same check at SEND
// time in case a module upgrade narrowed the declaration underneath a saved rule.
const db = require('./engagementRules.db')
const segmentsDb = require('./engagementSegments.db')
const registries = require('../../modules/registries')
const ceilings = require('../../modules/ceilings')
const channels = require('../../engagement/channels')
const segmentExpressions = require('../../engagement/segments')
const conditions = require('../../engagement/conditions')
const templates = require('../../engagement/templates')
// A day. Longer than this and "cooldown" is really "send once", which a rule
// expresses by being disabled rather than by a decade-long interval.
const MAX_COOLDOWN_SECONDS = 86_400
// The grace window (§4.2a). A delay longer than a day outlives the thing it is
// about - and, more practically, a queue row that sits for a week is a row whose
// payload no longer describes the world.
const MAX_DELAY_SECONDS = 86_400
// The upper bound on the operator-set hourly ceiling. It is not "unlimited by
// another name": the number exists so that a misconfiguration is a bad hour
// rather than an unbounded one, and a ceiling nobody can raise past a bound is
// what makes rules-as-data safe (§7.1 Q3).
const MAX_SENDS_PER_HOUR = 10_000
// The one key in `template_keys` that is not a delivery channel. `teamDigestWorker`
// renders it for a rule whose email channel an individual has set to digest mode,
// so it belongs to a MODE rather than to the rule's channel list and can never
// appear there.
const DIGEST_SLOT = 'digest'
const isPlainObject = (v) => v !== null && typeof v === 'object' && !Array.isArray(v)
/**
* Validate a rule against the registries and the lattice.
*
* Returns `{ ok: true, rule }` with a normalised row ready for insert/update, or
* `{ ok: false, errors }` listing every problem.
*
* `triggerId` may name a trigger nobody currently registers ONLY on an update of
* an existing rule - a dormant rule must stay editable (its module can come
* back), and refusing to save it would make an uninstall destructive after the
* fact. A NEW rule must name a live trigger, because there is nothing to
* preserve and a typo should be caught now.
*/
async function validate(input, { existing = null } = {}) {
const errors = []
const raw = isPlainObject(input) ? input : {}
const triggerId = typeof raw.triggerId === 'string' ? raw.triggerId : existing?.trigger_id
const declaration = triggerId ? registries.eventTrigger(triggerId) : null
if (!triggerId) errors.push('triggerId is required')
else if (!declaration && !existing) errors.push(`no trigger "${triggerId}" is registered`)
const name = typeof raw.name === 'string' ? raw.name.trim() : ''
if (!name) errors.push('name is required')
else if (name.length > 160) errors.push('name is longer than 160 characters')
// Channels are stored as data and checked against the registry, so a rule
// cannot name a sink that does not exist. Phase 4b's form offers the registered
// set; this is what makes that an affordance rather than the rule.
const wanted = Array.isArray(raw.channels) ? [...new Set(raw.channels)] : []
if (!wanted.length) errors.push('at least one channel is required')
for (const c of wanted) if (!channels.has(c)) errors.push(`no channel "${c}" is registered`)
// `template_keys` is { channel: templateKey }. Phase 5 owns templates, so the
// KEYS are checked for shape and not for existence - a rule may legitimately
// name a template that has not been authored yet, and Phase 5's editor is where
// that becomes resolvable.
//
// **The shape check was wrong until Phase 5b, and wrong in the way that matters:**
// it required `/^[a-z0-9][a-z0-9-]{0,63}$/`, which has no dot, while every
// template key that exists is dotted (`notify.event`, `auth.password-reset`).
// Written before templates existed, it could not match one, so no rule could name
// any real template - which is precisely the workflow S4.6.2's duplicate action
// exists to serve. It now uses the templates model's own pattern, so the two
// cannot disagree about what a key is.
const templateKeys = {}
if (raw.templateKeys !== undefined && !isPlainObject(raw.templateKeys)) {
errors.push('templateKeys must be an object of { channel: templateKey }')
} else {
for (const [channel, key] of Object.entries(raw.templateKeys || {})) {
// **`digest` is a template SLOT, not a channel**, and it is legal here for
// exactly the reason `registries.js` `checkSeedRule` says it is: it names
// the body `teamDigestWorker` renders for a rule whose email channel an
// individual has set to digest mode, so it never appears in `channels` and
// never could. Rejecting it made every rule that ships one unsaveable from
// the Rules screen — core's own team and news rules included, and sixteen
// of module-uo's — with a 400 naming a key the operator never typed, whose
// only remedy was deleting the digest body and silently dropping digest
// support. Found by Phase 13's acceptance walk; the two validators now
// agree about what `digest` is.
if (channel !== DIGEST_SLOT && !wanted.includes(channel)) {
errors.push(`templateKeys names "${channel}", which is not one of this rule's channels`)
continue
}
if (typeof key !== 'string' || key.length > templates.MAX_KEY || !templates.KEY_RE.test(key)) {
errors.push(`templateKeys.${channel} is not a valid template key`)
continue
}
templateKeys[channel] = key
}
}
const numbers = [
['cooldownSeconds', 'cooldown_seconds', MAX_COOLDOWN_SECONDS, 0],
['delaySeconds', 'delay_seconds', MAX_DELAY_SECONDS, 0],
['maxSendsPerHour', 'max_sends_per_hour', MAX_SENDS_PER_HOUR, 1],
]
const scalars = {}
for (const [key, column, max, min] of numbers) {
const supplied = raw[key]
const fallback = existing ? existing[column] : column === 'max_sends_per_hour' ? 100 : 0
const value = supplied === undefined || supplied === null ? fallback : Number(supplied)
if (!Number.isInteger(value) || value < min || value > max) {
errors.push(`${key} must be an integer between ${min} and ${max}`)
} else scalars[column] = value
}
// `cancel_on` names trigger ids, and they are NOT checked for registration for
// the dormancy reason (§7.3): a resolving event whose module is temporarily
// absent should stop cancelling, not make the rule unsaveable.
const cancelOn = Array.isArray(raw.cancelOn) ? [...new Set(raw.cancelOn.filter((t) => typeof t === 'string'))] : []
if (cancelOn.length && !scalars.delay_seconds) {
// Not an error - it is a rule that will never cancel anything, because there
// is no window in which to do it. Worth saying out loud rather than silently
// accepting a setting that cannot take effect.
errors.push('cancelOn has no effect without a delaySeconds grace window')
}
const checked = conditions.validate(declaration, raw.conditions === undefined ? existing?.conditions : raw.conditions)
if (!checked.ok) errors.push(...checked.errors)
// ── The audience, and the one check that is a security boundary ──────────
let audience = typeof raw.audience === 'string' ? raw.audience : existing?.audience || declaration?.audience
let segmentId = raw.audienceSegmentId === undefined ? existing?.audience_segment_id ?? null : raw.audienceSegmentId
segmentId = segmentId === null || segmentId === '' ? null : Number(segmentId)
let effectiveCeiling = null
if (segmentId !== null) {
if (!Number.isInteger(segmentId)) errors.push('audienceSegmentId must be an integer')
else {
const segment = await segmentsDb.getById(segmentId)
if (!segment) errors.push(`no audience segment ${segmentId} exists`)
else {
// The segment's STORED ceiling, derived when it was saved by
// `segments.validate` from the narrowest audience it contains. A rule
// pointing at a segment takes that as its reach; the `audience` column
// is retained for display and is not what the engine resolves.
effectiveCeiling = segment.ceiling
audience = segment.ceiling
}
}
} else if (!ceilings.isCeiling(audience)) {
errors.push(`audience must be one of ${ceilings.CEILINGS.join(', ')}`)
} else {
effectiveCeiling = audience
}
if (declaration && effectiveCeiling && !ceilings.permits(declaration.ceiling, effectiveCeiling)) {
errors.push(
`audience "${effectiveCeiling}" is wider than trigger "${triggerId}" permits (ceiling "${declaration.ceiling}")`,
)
}
if (errors.length) return { ok: false, errors }
return {
ok: true,
rule: {
trigger_id: triggerId,
name,
enabled: raw.enabled === undefined ? Boolean(existing?.enabled) : Boolean(raw.enabled),
audience,
audience_segment_id: segmentId,
max_sends_per_hour: scalars.max_sends_per_hour,
channels: wanted,
template_keys: templateKeys,
conditions: checked.conditions,
cooldown_seconds: scalars.cooldown_seconds,
delay_seconds: scalars.delay_seconds,
cancel_on: cancelOn,
updated_by: Number.isInteger(raw.updatedBy) ? raw.updatedBy : null,
},
}
}
async function create(input) {
const checked = await validate(input)
if (!checked.ok) return checked
const id = await db.insert(checked.rule)
return { ok: true, rule: await db.getById(id) }
}
async function update(id, input) {
const existing = await db.getById(id)
if (!existing) return { ok: false, errors: [`no rule ${id} exists`], notFound: true }
const checked = await validate(input, { existing })
if (!checked.ok) return checked
await db.update(id, checked.rule)
return { ok: true, rule: await db.getById(id) }
}
/**
* Why this rule cannot currently fire, as a list of sentences. Empty = it can.
*
* **Three ways, not two.** A rule can be dormant because its trigger is gone,
* because a channel it names is gone, or because its AUDIENCE is gone - and the
* audience case has two shapes that a screen must not collapse into one:
*
* • the segment row was deleted out from under it (§7.3), or
* • the segment still exists and every audience in it belongs to a module that
* has been uninstalled (§5.1a rule 4).
*
* Both leave the rule reaching nobody. Only the first leaves nothing behind, and
* a check that asks only "does the row exist" reports the first and misses the
* second - which shows an enabled, healthy-looking rule that cannot fire. Found
* by uninstalling a module under a live rule while building Phase 4b's screen.
*
* @param {Map<number, {expression: object}>} segments every segment, by id
*/
function dormancyReasons(rule, segments) {
const reasons = []
if (!registries.eventTrigger(rule.trigger_id)) reasons.push(`trigger "${rule.trigger_id}" is not registered`)
if (rule.audience_segment_id) {
const segment = segments.get(rule.audience_segment_id)
if (!segment) reasons.push('its audience segment no longer exists')
else {
const missing = segmentExpressions.missingAudiences(segment.expression)
if (missing.length) {
reasons.push(`its audience "${segment.name}" uses ${missing.join(', ')}, which nothing registers`)
}
}
}
for (const c of rule.channels || []) if (!channels.has(c)) reasons.push(`channel "${c}" is not registered`)
return reasons
}
const annotate = (rule, segments) => {
const reasons = dormancyReasons(rule, segments)
return { ...rule, dormant: reasons.length > 0, dormantReasons: reasons }
}
const segmentsById = async () => new Map((await segmentsDb.list()).map((s) => [s.id, s]))
/**
* List every rule, each annotated with whether it can currently fire.
*
* Dormancy is computed rather than stored (§7.3): a rule whose trigger or
* segment is not registered right now is listed, flagged, and left alone. The
* alternative - deleting or disabling it on uninstall - destroys an operator's
* configuration on the strength of a module being temporarily absent.
*/
async function listAnnotated() {
const rows = await db.list()
const segments = await segmentsById()
return rows.map((rule) => annotate(rule, segments))
}
/** One rule with the same dormancy annotation the list carries, or null. */
async function getAnnotated(id) {
const rule = await db.getById(id)
if (!rule) return null
return annotate(rule, await segmentsById())
}
/**
* Turn one rule on or off, writing that column and no other (Phase 4b).
*
* This is the one write path that does NOT go through `validate`, and the
* asymmetry is deliberate. Switching a rule OFF must always be possible - a rule
* whose module has been uninstalled, or whose trigger has since narrowed its
* ceiling under a saved audience, is exactly the rule an operator most urgently
* wants stopped, and it is exactly the rule `validate` would now refuse. The
* full editor still re-validates on save, and the engine re-checks the ceiling at
* send time, so nothing is loosened by having a switch that is only a switch.
*/
async function setEnabled(id, enabled, updatedBy = null) {
const existing = await db.getById(id)
if (!existing) return { ok: false, errors: [`no rule ${id} exists`], notFound: true }
await db.setEnabled(id, enabled, updatedBy)
return { ok: true, rule: await getAnnotated(id) }
}
/**
* Delete a rule.
*
* Its cooldown rows and any still-pending outbox rows go with it (both carry an
* ON DELETE CASCADE), and that is the right blast radius: neither means anything
* without the rule. `engagement_sends` deliberately does NOT — its `rule_id`
* carries no foreign key — so the send log outlives the rule and the record of
* what was actually mailed survives an operator tidying up.
*/
async function remove(id) {
const existing = await db.getById(id)
if (!existing) return { ok: false, errors: [`no rule ${id} exists`], notFound: true }
await db.remove(id)
return { ok: true }
}
module.exports = {
validate,
create,
update,
setEnabled,
remove,
listAnnotated,
getAnnotated,
MAX_COOLDOWN_SECONDS,
MAX_DELAY_SECONDS,
MAX_SENDS_PER_HOUR,
}