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>
237 lines
11 KiB
JavaScript
237 lines
11 KiB
JavaScript
// ── The five rules core ships, all of them OFF ─────────────────────────────
|
|
//
|
|
// ENGAGEMENT.md Phase 6, decision 3. Before this phase the Team pipeline mailed
|
|
// people with no operator configuration at all: the code decided who was mailed
|
|
// and about what, and the only knobs were per-user. Phase 6 moves that decision
|
|
// onto rules — which default `enabled = 0`, and of which core seeds none.
|
|
//
|
|
// **So a straight migration would have stopped Team email on every existing
|
|
// deployment, silently.** The org lead's decision was to honour the invariant
|
|
// rather than carve an exception into it: the rules are seeded, and they are
|
|
// seeded OFF. Team email resumes when an operator opens Admin → Engagement →
|
|
// Rules and switches one on, and until then the admin screen says so in as many
|
|
// words (`EngagementRules.jsx`). The release note names it.
|
|
//
|
|
// The alternative — seeding them enabled so nothing changes for anybody — was
|
|
// considered and refused. "Nothing is seeded, nothing is on by default" is what
|
|
// makes a rules table safe to restore, import or replicate, and an exception
|
|
// carved for the one pipeline that predates the engine is an exception that has
|
|
// to be re-argued every time somebody reads the invariant.
|
|
//
|
|
// **Seeded once, not ensured on every boot**, and the difference matters: an
|
|
// operator who deletes a rule must not find it back after a restart. The guard is
|
|
// a settings key, the same mechanism a one-shot migration uses, so a deployment
|
|
// that has seen this seed never sees it again — deleted rules stay deleted, and
|
|
// an enabled rule stays enabled rather than being reset to off.
|
|
|
|
// **Phase 11 added a fifth, for `news.post`, and it needed its OWN one-shot key
|
|
// rather than an entry in the list above.** The Team key is already stamped on
|
|
// every deployment that has booted since Phase 6, and the guard reads its
|
|
// presence — so appending to `RULES` would have seeded the news rule on fresh
|
|
// installs only, and on exactly the upgrades that need it, never. Those are the
|
|
// deployments where `pushDispatch.publish('news.post', …)` used to run and no
|
|
// longer does (§7.1 Q9): they would have lost news push with no rule to switch
|
|
// on and no way to tell why. One key per seed GROUP is the rule this establishes;
|
|
// a sixth rule for a new trigger takes a sixth key, and a rule added to an
|
|
// existing group is a rule that only fresh installs will ever see.
|
|
|
|
const rulesDb = require('../model/engagement/engagementRules.db')
|
|
const settingsDb = require('../model/settings/settings.db')
|
|
const log = require('../utils/logger')('engagement')
|
|
|
|
// The one-shot guard. Its VALUE is the timestamp, purely so an operator reading
|
|
// the settings table can tell when it ran; only its presence is read.
|
|
const SEEDED_KEY = 'engagement_team_rules_seeded'
|
|
|
|
// Phase 11's, and separate for the reason above. Same shape, same semantics.
|
|
const NEWS_SEEDED_KEY = 'engagement_news_rule_seeded'
|
|
|
|
const RULES = [
|
|
{
|
|
trigger_id: 'team.forum.post',
|
|
name: 'Team forum posts',
|
|
// `members`, which resolves to the recipient set the event carries — the
|
|
// access-checked list `teamNotify` has always computed. Not `authenticated`,
|
|
// and the trigger's own ceiling would refuse that anyway: a private Team's
|
|
// forum excerpt reaching the whole site is the failure G24 exists for.
|
|
audience: 'members',
|
|
channels: ['email'],
|
|
// `email` is the instant body; `digest` is what the digest worker renders.
|
|
// Two keys because they are two different messages — a template written for
|
|
// one post renders a day of them as a single missing variable.
|
|
template_keys: { email: 'notify.team-post', digest: 'notify.digest' },
|
|
// No cooldown. A busy thread is exactly what the per-user `email_mode` and
|
|
// the digest option are for, and a cooldown here would silently drop the
|
|
// second reply of a conversation rather than batching it.
|
|
cooldown_seconds: 0,
|
|
max_sends_per_hour: 500,
|
|
},
|
|
{
|
|
trigger_id: 'team.announcement',
|
|
name: 'Team announcements',
|
|
audience: 'members',
|
|
channels: ['email'],
|
|
// **The generic body, not `notify.team-post`, and the reason is a naming
|
|
// inconsistency in the Phase 2 declarations rather than a design choice
|
|
// here.** The two triggers describe the same underlying thing — a thread in a
|
|
// Team forum — but `team.forum.post` declares its title as `threadTitle` and
|
|
// `team.announcement` declares it as `title`. A template can only name one of
|
|
// them, so `notify.team-post`'s `{{threadTitle}}` renders empty for an
|
|
// announcement. `notify.event` + the structural projection gets it right
|
|
// (`title` is in the payload, `actionUrl` falls back to `postUrl`), and
|
|
// reconciling the two declarations is a version bump this phase did not take
|
|
// on its own authority.
|
|
template_keys: { email: 'notify.event', digest: 'notify.digest' },
|
|
cooldown_seconds: 0,
|
|
max_sends_per_hour: 500,
|
|
},
|
|
{
|
|
trigger_id: 'team.member.joined',
|
|
name: 'Team — new member',
|
|
audience: 'members',
|
|
channels: ['email'],
|
|
// The generic body: `notify.event` plus the structural projection renders it
|
|
// with no authoring (§4.6.1 property 1). A deployment that wants a better one
|
|
// duplicates the template and points this rule at the copy.
|
|
template_keys: { email: 'notify.event' },
|
|
// An hour, per user per Team. This is the rule §6.4 argued should not exist
|
|
// as a sink at all — a fifteen-minute sweep, already on the activity feed —
|
|
// and the cooldown is what makes it survivable for the operator who wants it
|
|
// anyway: a guild recruiting ten people in an afternoon sends one mail.
|
|
cooldown_seconds: 3600,
|
|
max_sends_per_hour: 200,
|
|
},
|
|
{
|
|
trigger_id: 'team.leadership.changed',
|
|
name: 'Team — leadership change',
|
|
audience: 'members',
|
|
channels: ['email'],
|
|
template_keys: { email: 'notify.event' },
|
|
cooldown_seconds: 3600,
|
|
max_sends_per_hour: 200,
|
|
},
|
|
]
|
|
|
|
// Phase 11's one rule, in its own list so it can carry its own one-shot key.
|
|
const NEWS_RULES = [
|
|
{
|
|
trigger_id: 'news.post',
|
|
name: 'News posts',
|
|
// `subscribers`, which is the trigger's declared default and the population
|
|
// `pushDispatch.publish('news.post', …)` used to reach directly: users who
|
|
// opted into this id on at least one channel. Not `authenticated`, even
|
|
// though the trigger's ceiling permits it — a news post is worth telling
|
|
// people who asked to be told, and mailing the whole user table on every
|
|
// publish is how a notification feature earns a spam complaint.
|
|
audience: 'subscribers',
|
|
// **All three channels, unlike the Team rules' `email` alone**, and that is
|
|
// the continuity half of §7.1 Q9's answer. Push is on this rule because push
|
|
// is what the raw tickle did; leaving it off would mean an operator who
|
|
// enabled the rule to restore news push got mail instead. In-app rides along
|
|
// because the inbox is the surface a tickle deep-links into (Phase 7).
|
|
channels: ['email', 'inapp', 'push'],
|
|
// The generic body plus the structural projection (§4.6.1 property 1):
|
|
// `news.post` declares its own `title` and `postUrl`, which the projection
|
|
// leaves exactly as emitted, so an unauthored mail already names the post and
|
|
// links it. `inapp.event` is the in-app renderer's; push carries no content
|
|
// by construction and needs no template.
|
|
template_keys: { email: 'notify.event', inapp: 'inapp.event', digest: 'notify.digest' },
|
|
// An hour, per USER — `news.post` declares no `subjectKey`, so the cooldown
|
|
// subject is the recipient. "Do not tell me about news more than once an
|
|
// hour" is the useful rule; keying it per post would make it a no-op, since
|
|
// every post is a new subject.
|
|
cooldown_seconds: 3600,
|
|
max_sends_per_hour: 1000,
|
|
},
|
|
]
|
|
|
|
/**
|
|
* Seed one group of rules, once, under its own guard key.
|
|
*
|
|
* Never throws: it is on the boot path beside `seedTemplates`, and a rule that
|
|
* failed to seed costs an operator one visit to the "new rule" form, not a
|
|
* deployment.
|
|
*
|
|
* @param {string} key the one-shot settings guard for THIS group
|
|
* @param {object[]} rules
|
|
* @param {string} note what the boot log should say when it inserts
|
|
*/
|
|
async function seedGroup(key, rules, note) {
|
|
const summary = { inserted: 0, skipped: 0 }
|
|
try {
|
|
// **Claimed BEFORE the loop, atomically**, and the stamp is the claim. A
|
|
// `get()` here with a `set()` after the inserts is not a guard when two
|
|
// instances boot together — both read "absent", both seed — and a duplicate
|
|
// rule is two mails per event. `claim()` is an `INSERT IGNORE` reporting its
|
|
// own `affectedRows`, so exactly one caller proceeds. See the note below on
|
|
// what a partial run costs: that trade is unchanged, only its ordering.
|
|
if (!(await settingsDb.claim(key, new Date().toISOString()))) {
|
|
return { ...summary, skipped: rules.length }
|
|
}
|
|
|
|
for (const rule of rules) {
|
|
try {
|
|
await rulesDb.insert({
|
|
audience_segment_id: null,
|
|
conditions: null,
|
|
// No delay and nothing cancels these. `delay_seconds` is the grace
|
|
// window a cancelling event needs, and nothing cancels "someone
|
|
// posted" — the post happened.
|
|
delay_seconds: 0,
|
|
cancel_on: [],
|
|
...rule,
|
|
enabled: 0,
|
|
updated_by: null,
|
|
})
|
|
summary.inserted += 1
|
|
} catch (err) {
|
|
log.error('team rule seed failed', { trigger: rule.trigger_id, message: err.message })
|
|
}
|
|
}
|
|
// Stamped even on a partial run — the claim above is the stamp. Re-running
|
|
// would duplicate the rules that did insert, and a duplicate rule is two
|
|
// mails per event, a worse outcome than the one missing rule an operator can
|
|
// add from the screen.
|
|
if (summary.inserted) {
|
|
log.info('seeded engagement rules, all disabled', { rules: summary.inserted, note })
|
|
}
|
|
} catch (err) {
|
|
log.error('rule seeding failed', { key, message: err.message })
|
|
}
|
|
return summary
|
|
}
|
|
|
|
/** The four Team rules (Phase 6). */
|
|
const seedTeamRules = () =>
|
|
seedGroup(SEEDED_KEY, RULES, 'Team email stays off until an operator enables one')
|
|
|
|
/** The one news rule (Phase 11). */
|
|
const seedNewsRule = () =>
|
|
seedGroup(NEWS_SEEDED_KEY, NEWS_RULES, 'News notifications stay off until an operator enables this rule')
|
|
|
|
/**
|
|
* Both groups, which is what the boot path calls.
|
|
*
|
|
* Sequential rather than concurrent, and not for correctness — each group has its
|
|
* own guard key and its own rows — but so the boot log reads in a fixed order and
|
|
* a failure names one group rather than an interleaving of two.
|
|
*/
|
|
async function seedCoreRules() {
|
|
const team = await seedTeamRules()
|
|
const news = await seedNewsRule()
|
|
return {
|
|
inserted: team.inserted + news.inserted,
|
|
skipped: team.skipped + news.skipped,
|
|
}
|
|
}
|
|
|
|
module.exports = {
|
|
seedCoreRules,
|
|
seedTeamRules,
|
|
seedNewsRule,
|
|
RULES,
|
|
NEWS_RULES,
|
|
SEEDED_KEY,
|
|
NEWS_SEEDED_KEY,
|
|
}
|