8453762e3b1d292fba9f5a139047a0cd154797a1
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
| 82a50e5e04 |
fix(engagement): the trigger manifest was stale, and its check was crying wolf
Two defects, and the second is why the first survived a whole phase. The manifest is stale. `engagement-triggers.json` embeds `moduleApiVersion` deliberately -- "a stale manifest needs to know which API's rules produced it" -- and Phase 7 (website#189) bumped MODULE_API_VERSION to 1.10.0 without regenerating it. The committed file has said 1.9.0 ever since. One line, and regenerating is the whole fix. The check could not be believed. Both the test and the `--check` CI gate compared bytes, and this repo is developed on Windows under core.autocrlf=true, so git checks the committed LF blob out as CRLF and the comparison then calls an unchanged manifest stale. That failure fires on every Windows checkout, says "a trigger declaration changed", and is "fixed" by regenerating a file whose content was already correct. So the one check that exists to be believed had been failing for a reason everyone had learned to write off as environmental -- including me, twice: the Phase 7 PR recorded it as a pre-existing CRLF failure, and the Phase 8 PR repeated the claim. It was neither pre-existing nor CRLF. A check that cries wolf is a check nobody reads, and the genuine staleness underneath it went unnoticed for exactly that reason. Line endings are now normalised on both sides, which is the convention routeManifest.js and routeManifest.test.js already use one file along -- that pair had clearly hit this and been fixed; the engagement pair never was. What is being asserted is that the committed manifest describes the same declarations, and a line ending is not a declaration. Policing the encoding is .gitattributes' job, not this check's. Verify - The check is still LIVE, proved by breaking it deliberately: with the content changed the gate exits 1; with only the line endings changed it exits 0. That is the whole point of the fix, so it is not taken on trust. - `npm test` under the TAP reporter: 1950 tests, 1877 pass, 0 fail. That is pristine `edge`'s 1950/1876 plus the one test this repairs. A harness note, disclosed rather than buried The default (spec) reporter intermittently reports a FILE-level failure with all of that file's subtests passing, no assertion, and no diagnostic beyond 'test failed'. It named a different unrelated file on each of four runs (requireInternalKey, routeManifest, eventAuthorize, totp) and the TAP reporter shows zero failures over the same suite. It appears to be a reporter artifact under concurrency rather than a failing test, but it correlates with this branch (4/4) against pristine edge (0/2) on the same machine state, which I could not explain and am not claiming to have. Worth its own look; it does not indicate a product defect and no assertion fails. Co-Authored-By: Claude <noreply@anthropic.com> |
|||
| 563199a096 |
feat(modules): event triggers, audiences and the ceiling lattice (engagement Phase 2)
The contract half of the engagement system: a module (and core) can DECLARE an
event with a payload contract and fire it. Nothing delivers yet — `emit`
validates, logs and stops, and Phase 4 replaces that log line with the engine.
`api.registerEventTriggers` and `api.registerAudiences` ride the existing
stage()/apply() validate-then-commit discipline, so a registrant that throws
halfway leaves nothing behind. `ctx.events.emit` is fire-and-forget and binds
the owner from the calling module — a module fires its own triggers and no one
else's. `ctx.inbox.push` is present and throws until Phase 7, the shape 1.6.0
settled on for a member that arrives a phase late.
MODULE_API_VERSION 1.7.0 on both halves. Additions only; module-uo's
`coreApi: "^1.3.0"` still resolves.
Three design decisions, approved by the org lead before any code:
ONE NAMESPACE for trigger ids and notification-stream ids (ENGAGEMENT.md §7.2,
against the recommendation in the text). A trigger is a payload contract
attached to an id that may also carry a subscription toggle, so an id has
exactly one owner across both facets, checked in both directions. Core's five
trigger ids ARE its five stream ids, so the same-owner upgrade case is
exercised on every boot rather than only by a module. It keeps
notification_channel_prefs single-keyed in Phase 3, where two namespaces would
have forced a `kind` discriminator into its primary key.
Two knock-on effects appeared only once it was implemented. The id grammar had
to be RELAXED to admit `_` inside a segment — §4.3's own worked example is
`uo.house.idoc_warning`, and two grammars over one namespace would mean an id
legal as a trigger and illegal as the stream it is the same event as. And the
seven grandfathered `uo.*` ids had to share their legacy allowlist with
triggers, because under one namespace `idoc.warning` is a single id. The push
catalog is untouched either way: allStreams() still serves the stream facet
only, so the shipped Android client sees exactly what it saw before.
THE CEILING LATTICE (G24), which the plan named everywhere and defined nowhere.
It is containment, not size: everyone ⊃ authenticated ⊃ {subscribers, members,
staff, owner}, with the four leaves mutually incomparable. The flat total order
the plan's wording invites would let a `staff`-ceilinged trigger be given an
`owner` audience — a rule that mails cheat detection to the player it detected.
Fewer people is not less exposure. Two incomparable ceilings have no meet at
all, so a composition is refused rather than guessed; union-widens is the
intuitive implementation and it is the wrong one.
`kind: 'event' | 'scheduled'` is declarable now and no evaluator exists (§7.1
Q6). Registration accepts `scheduled` and emit refuses to fire one, so `kind`
means something from the moment it can be written rather than from the moment
it is honoured.
Also: `GET /admin/engagement/{triggers,audiences}`, served from the registries
rather than a table so an uninstalled module simply stops appearing;
`npm run engagement:manifest` plus its CI `--check`, the twin of the route
manifest, because renaming a variable breaks stored templates silently, at send
time, in mail someone already received.
Co-Authored-By: Claude <noreply@anthropic.com>
|
