fix(engagement): two defects the Phase 11b live walk found in core
All checks were successful
PR Checks / bot-tests (pull_request) Successful in 37s
PR Checks / client-build (pull_request) Successful in 45s
PR Checks / server-tests (pull_request) Successful in 5m30s

Both are invisible to a fixture and loud on a real database, which is why the
walk is the phase's acceptance rather than a formality.

1. registerEngagementSeeds validated `max_sends_per_hour` and then dropped it
   from the normalized rule. The column is NOT NULL, so every one of the 25
   module-seeded rules failed to insert at boot. The registry test asserted the
   REJECTION of a bad ceiling and never that a good one survives; it now asserts
   the normalized rule against `engagementRules.db.insert`'s own column list, so
   the next field added is covered the day it is added.

2. The cooldown claim runs inside the engine's per-channel loop and its key was
   (rule, user, subject). So the first channel of a rule claimed the cooldown and
   every later one was reported as cooled -- and `inapp` is ranked first
   deliberately, so a rule naming email + in-app delivered the inbox item and
   silently never the mail. Core's own `news.post` rule has that shape. Phase
   11b's decision 8 requires the letter and the inbox item to fire together.

   `channel` joins the PRIMARY KEY (the org lead's decision 12: a cooldown is per
   delivery, not per occasion). Migrated in place behind an information_schema
   guard, because MariaDB has no conditional form of a key change and replaying
   schema.sql would otherwise fail on every boot after the first.

1551 core tests green; both new tests verified by reverting each fix in turn.

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
2026-09-01 07:12:09 -05:00
parent c3783f56f1
commit c8d45733b6
6 changed files with 118 additions and 17 deletions

View File

@@ -98,8 +98,11 @@ function installStubs() {
// the pair is still cooling. `engagementEngineSql.test.js` is what proves the
// SQL itself - a stub can only ever agree with whoever wrote it, and in this
// case the first version of both was wrong together.
cooldownsDb.claim = async (ruleId, userId, subjectKey, cooldownSeconds, now) => {
const key = `${ruleId}|${userId}|${subjectKey}`
cooldownsDb.claim = async (ruleId, userId, subjectKey, channel, cooldownSeconds, now) => {
// `channel` is in the key, exactly as the PRIMARY KEY is: a rule naming two
// channels must deliver on both, and a stub that dropped the channel would
// agree with the engine bug Phase 11b's live walk found.
const key = `${ruleId}|${userId}|${subjectKey}|${channel}`
const row = store.cooldowns.get(key)
if (!row) {
store.cooldowns.set(key, { last_fired_at: now, fire_count: 1 })
@@ -327,6 +330,26 @@ test('a cooldown that has expired lets the same subject through again', async ()
assert.equal(outboxRows().length, 2)
})
test('a cooldown does not stop a rule delivering on its OTHER channels', async () => {
// Phase 11b's live walk. The claim runs inside the per-channel loop, so a key
// without the channel let the FIRST channel claim the cooldown and reported
// every later one as cooled — and `inapp` is ranked ahead of `email` on
// purpose, so a two-channel rule delivered the inbox item and silently never
// the mail. Decision 8 requires both, and this is the assertion that says so.
addUser(10)
optIn(10, 'uo.house.idoc_warning', 'email')
optIn(10, 'uo.house.idoc_warning', 'inapp')
addRule({ cooldown_seconds: 86_400, channels: ['email', 'inapp'] })
await engine.dispatch(event(), T0)
assert.deepEqual(outboxRows().map((r) => r.channel).sort(), ['email', 'inapp'])
// …and the cooldown still holds, on both channels, for a second event about
// the same house inside the day. Per-delivery, not per-channel-forever.
await engine.dispatch(event(), later(60_000))
assert.equal(outboxRows().length, 2)
})
test('two rules on one trigger each get their own cooldown', async () => {
addRule({ cooldown_seconds: 3600 })
addRule({ cooldown_seconds: 3600 })