Phase 2 PR1: Regional state-change alerts from diff engine#2966
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR wires the existing
Confidence Score: 4/5Safe to merge after addressing the LPUSH rollback gap — the P1 reliability issue won't cause data loss or crashes, but will silently drop state-change alerts for 6h on any transient Upstash write error. One P1 finding: missing dedup key rollback on LPUSH failure diverges from the ais-relay.cjs reference and can cause alerts to be silently suppressed for a full cron cycle on transient errors. The rest of the implementation is sound — pure builders, correct best-effort layering, good test coverage, and clean integration into the seed loop. The P2 hash comment inaccuracy does not affect correctness today. scripts/regional-snapshot/alert-emitter.mjs — the defaultPublishEvent function needs a DEL rollback on LPUSH failure (lines 244–249). Important Files Changed
Sequence DiagramsequenceDiagram
participant Cron as Railway Cron (6h)
participant Seed as seed-regional-snapshots.mjs
participant Diff as diffRegionalSnapshot()
participant Emitter as alert-emitter.mjs
participant Redis as Upstash Redis
participant Relay as notification-relay.cjs
participant Sub as Pro Subscriber
Cron->>Seed: trigger
Seed->>Seed: computeSnapshot(region)
Seed->>Diff: diffRegionalSnapshot(prev, tentative)
Diff-->>Seed: SnapshotDiff
Seed->>Seed: persistSnapshot(snapshot)
Note over Seed: Only if result.persisted=true
Seed->>Emitter: emitRegionalAlerts(region, snapshot, diff)
Emitter->>Emitter: buildAlertEvents() → events[]
loop per event
Emitter->>Redis: SET NX wm:notif:scan-dedup:{type}:{hash} EX 21600
alt new event (SET NX → OK)
Emitter->>Redis: LPUSH wm:events:queue {eventType, payload, severity, publishedAt}
Redis-->>Emitter: list length (ok=true)
else dedup hit
Redis-->>Emitter: null (ok=false → skip)
end
end
Emitter-->>Seed: {enqueued, events}
Redis->>Relay: BRPOP wm:events:queue
Relay->>Sub: Telegram / email / Slack / Convex
Reviews (1): Last reviewed commit: "feat(intelligence): state-change alerts ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+244
to
+249
| if (ok) { | ||
| const title = String(event.payload?.title ?? ''); | ||
| console.log(`[alerts] queued ${event.severity} ${event.eventType}: ${title.slice(0, 60)}`); | ||
| } else { | ||
| console.warn(`[alerts] LPUSH failed for ${event.eventType}`); | ||
| } |
Contributor
There was a problem hiding this comment.
When upstashLpush returns false (transient Upstash error), the SET NX dedup key has already been written with a 6-hour TTL. Because there's no rollback, every subsequent cron cycle within that window will treat the event as a dedup hit and skip it — the alert is silently lost for the full 6h window. The reference implementation in ais-relay.cjs explicitly rolls back with upstashDel(dedupKey).catch(() => {}) for exactly this reason (line 397 of ais-relay.cjs).
Suggested change
| if (ok) { | |
| const title = String(event.payload?.title ?? ''); | |
| console.log(`[alerts] queued ${event.severity} ${event.eventType}: ${title.slice(0, 60)}`); | |
| } else { | |
| console.warn(`[alerts] LPUSH failed for ${event.eventType}`); | |
| } | |
| const ok = await upstashLpush(url, token, 'wm:events:queue', msg); | |
| if (ok) { | |
| const title = String(event.payload?.title ?? ''); | |
| console.log(`[alerts] queued ${event.severity} ${event.eventType}: ${title.slice(0, 60)}`); | |
| } else { | |
| console.warn(`[alerts] LPUSH failed for ${event.eventType} — rolling back dedup key`); | |
| fetch(`${url}/del/${encodeURIComponent(dedupKey)}`, { | |
| method: 'POST', | |
| headers: { Authorization: `Bearer ${token}` }, | |
| signal: AbortSignal.timeout(5_000), | |
| }).catch(() => {}); | |
| } |
Comment on lines
+173
to
+179
| export function simpleHash(str) { | ||
| let h = 0; | ||
| for (let i = 0; i < str.length; i += 1) { | ||
| h = ((h << 5) - h + str.charCodeAt(i)) | 0; | ||
| } | ||
| return (h >>> 0).toString(36); | ||
| } |
Contributor
There was a problem hiding this comment.
notifySimpleHash despite the compatibility comment
The comment says this matches notifySimpleHash in ais-relay.cjs, but the two functions produce different values for the same input whenever the accumulated hash h is negative: ais-relay.cjs uses Math.abs(h).toString(36) (magnitude of signed int), while this function uses (h >>> 0).toString(36) (unsigned 32-bit reinterpretation). For example, h = -1 yields '1' vs 'zzzzzzzz'. Since these are different event-type namespaces there's no dedup collision today, but the comment is misleading and the algorithm label "FNV-1a-ish" is also incorrect — the actual algorithm is djb2 (31*h + c).
Suggested change
| export function simpleHash(str) { | |
| let h = 0; | |
| for (let i = 0; i < str.length; i += 1) { | |
| h = ((h << 5) - h + str.charCodeAt(i)) | 0; | |
| } | |
| return (h >>> 0).toString(36); | |
| } | |
| export function simpleHash(str) { | |
| let h = 0; | |
| for (let i = 0; i < str.length; i += 1) { | |
| h = (Math.imul(31, h) + str.charCodeAt(i)) | 0; | |
| } | |
| return Math.abs(h).toString(36); | |
| } |
koala73
added a commit
that referenced
this pull request
Apr 11, 2026
…iew) P1 review finding on PR #2966. The default publisher used to SET NX the dedup key and then return false on LPUSH failure without removing it. Any transient Upstash or network hiccup therefore suppressed the alert for the full 6h dedup window even though nothing was enqueued — real state-change alerts could silently vanish for a quarter of a day. Fix: mirror the rollback path in scripts/ais-relay.cjs publishNotificationEvent() — when LPUSH fails after SET NX succeeded, DEL the dedup key so the next cron cycle can retry cleanly. ## Refactor for testability Extracted publishEventWithOps(event, ops) as the pure orchestrator with an injected Redis-ops interface ({ setNx, lpush, del }). Returns an outcome object { enqueued, dedupHit, rolledBack } so tests can assert exactly which branch fired without log scraping. Default publisher is now a thin wrapper that binds real Upstash REST calls and delegates. ## Tests — 6 new regression tests tests/regional-snapshot-alerts.test.mjs: - happy path: SET NX + LPUSH both succeed, nothing rolled back - dedup hit: dedupHit=true without touching the queue - LPUSH failure: rolledBack=true, DEL called, dedup key gone - retry-after-rollback: next call enqueues normally (the core bug) - LPUSH failure + DEL failure: still reports rolledBack=true (best-effort) - exceptions from setNx/lpush are swallowed, return non-enqueued outcome ## Verification - node --test tests/regional-snapshot-alerts.test.mjs: 31/31 pass - npm run test:data: 4383/4383 pass (was 4377; +6 regression tests) - npm run typecheck: clean - biome lint on touched files: clean
Phase 2 PR1 of the Regional Intelligence Model. Wires the existing
diffRegionalSnapshot output into the existing wm:events:queue so the
notification-relay on Railway can route state changes to Telegram / email /
Slack / Convex for Pro subscribers. No UI changes.
Single public entry point:
emitRegionalAlerts(region, snapshot, diff, { publishEvent? })
-> { enqueued: number, events: object[] }
Emits on 4 event types:
regional_regime_shift — diff.regime_changed set
regional_trigger_activation — one per entry in diff.trigger_activations
regional_corridor_break — one per entry in diff.corridor_breaks
regional_buffer_failure — one per entry in diff.buffer_failures
scenario_jumps and leverage_shifts are intentionally NOT emitted —
probability fluctuations are noisy and not actionable as push alerts.
Severity mapping:
- critical when regime shifts to escalation_ladder or fragmentation_risk
- critical for every corridor_break
- high for other regime shifts, trigger activations, buffer failures
- nothing below high is emitted
Dedup: 6h TTL on wm:notif:scan-dedup:{eventType}:{hash}, matching the cron
cadence and mirroring ais-relay.cjs publishNotificationEvent's path-based
Upstash REST calls.
emitRegionalAlerts NEVER throws. Three layers of guarding:
1. Null/undefined arguments short-circuit to { enqueued: 0, events: [] }
2. Each publisher call is individually try/catch'd so one failure can't
block other events in the same region
3. The seed writer wraps the call in its own try/catch so alert emission
can never block snapshot persist
This matches how ais-relay.cjs handles rss_alert events today.
After persistSnapshot succeeds in main(), the diff already computed in
step 15 is passed to emitRegionalAlerts. Logs per-region alert counts:
[mena] alerts: 3/3 enqueued
Zero impact on the persist path when the diff is empty (steady state
between state changes).
opts.publishEvent is the single injection point. Default is the Upstash
REST path-based publisher that walks SET NX -> LPUSH. Unit tests inject
a mock that captures events in an array, so the full diff -> events ->
dedup pipeline runs offline without touching Redis.
tests/regional-snapshot-alerts.test.mjs covers:
buildAlertEvents (13):
- empty diff -> []
- regime shift high vs critical (escalation_ladder / fragmentation_risk)
- 'none' fallback when previous regime is empty
- one event per trigger activation / corridor break / buffer failure
- scenario_jumps + leverage_shifts excluded
- stable output order: regime -> triggers -> corridors -> buffers
- null/undefined defensive returns
- region.label carried into titles
buildDedupKey + simpleHash (6):
- key shape matches ais-relay convention
- deterministic for same input
- different titles -> different hashes
- same title, different eventType -> different hashes
- missing payload.title handled safely
emitRegionalAlerts (6):
- no-op on empty diff (publisher never called)
- per-event enqueue through injected publisher
- publisher returning false only counts successes
- publisher throwing is swallowed, loop continues
- null/undefined arguments return empty result
- realistic escalation scenario enqueues all 6 event types correctly
- npm run test:data: 4377/4377 pass
- npm run typecheck: clean
- npm run typecheck:api: clean
- biome lint on touched files: clean
The relay already handles generic events with a `[SEVERITY] title` fallback
formatter. No relay-side changes are required — the new event types flow
through the same default path as rss_alert. If we later want custom
templates per regional event type, that's a follow-up to
scripts/notification-relay.cjs.
…iew) P1 review finding on PR #2966. The default publisher used to SET NX the dedup key and then return false on LPUSH failure without removing it. Any transient Upstash or network hiccup therefore suppressed the alert for the full 6h dedup window even though nothing was enqueued — real state-change alerts could silently vanish for a quarter of a day. Fix: mirror the rollback path in scripts/ais-relay.cjs publishNotificationEvent() — when LPUSH fails after SET NX succeeded, DEL the dedup key so the next cron cycle can retry cleanly. ## Refactor for testability Extracted publishEventWithOps(event, ops) as the pure orchestrator with an injected Redis-ops interface ({ setNx, lpush, del }). Returns an outcome object { enqueued, dedupHit, rolledBack } so tests can assert exactly which branch fired without log scraping. Default publisher is now a thin wrapper that binds real Upstash REST calls and delegates. ## Tests — 6 new regression tests tests/regional-snapshot-alerts.test.mjs: - happy path: SET NX + LPUSH both succeed, nothing rolled back - dedup hit: dedupHit=true without touching the queue - LPUSH failure: rolledBack=true, DEL called, dedup key gone - retry-after-rollback: next call enqueues normally (the core bug) - LPUSH failure + DEL failure: still reports rolledBack=true (best-effort) - exceptions from setNx/lpush are swallowed, return non-enqueued outcome ## Verification - node --test tests/regional-snapshot-alerts.test.mjs: 31/31 pass - npm run test:data: 4383/4383 pass (was 4377; +6 regression tests) - npm run typecheck: clean - biome lint on touched files: clean
koala73
force-pushed
the
feat/regional-intelligence-alerts
branch
from
April 11, 2026 20:20
ed913b8 to
ce2ac90
Compare
This was referenced Apr 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 PR1 of the Regional Intelligence Model. Wires the existing `diffRegionalSnapshot` output into the existing `wm:events:queue` so the notification relay on Railway routes state changes to Telegram / email / Slack / Convex for Pro subscribers. Server-only; no UI changes.
What landed
New module: `scripts/regional-snapshot/alert-emitter.mjs`
Single public entry point:
```js
emitRegionalAlerts(region, snapshot, diff, { publishEvent? })
-> { enqueued: number, events: object[] }
```
Emits on 4 event types:
`scenario_jumps` and `leverage_shifts` are intentionally NOT emitted — probability fluctuations are noisy and not actionable as push alerts.
Dedup
6h TTL on `wm:notif:scan-dedup:{eventType}:{hash}`, matching the cron cadence and mirroring `ais-relay.cjs` `publishNotificationEvent`'s path-based Upstash REST calls.
Best-effort contract
`emitRegionalAlerts` NEVER throws. Three layers of guarding:
This matches how `ais-relay.cjs` handles `rss_alert` events today.
`seed-regional-snapshots.mjs` hook
After `persistSnapshot` succeeds in `main()`, the diff already computed in step 15 is passed to `emitRegionalAlerts`. Logs per-region alert counts:
`[mena] alerts: 3/3 enqueued`
Zero impact on the persist path when the diff is empty (steady state between state changes).
Testability via dependency-injected publisher
`opts.publishEvent` is the single injection point. Default is the Upstash REST path-based publisher that walks SET NX → LPUSH. Unit tests inject a mock that captures events in an array, so the full diff → events → dedup pipeline runs offline without touching Redis.
Testing
25 new unit tests (`tests/regional-snapshot-alerts.test.mjs`), all running offline via the injected publisher:
`buildAlertEvents` (13): empty diff → [], regime shift high vs critical, 'none' fallback when previous regime is empty, one event per trigger/break/failure, `scenario_jumps` + `leverage_shifts` excluded, stable output order, defensive null/undefined, region.label carried into titles
`buildDedupKey` + `simpleHash` (6): key shape matches ais-relay convention, deterministic output, collision-free across titles and event types, missing-title safety
`emitRegionalAlerts` (6): no-op on empty diff, per-event enqueue, publisher-returns-false only counts successes, publisher-throws is swallowed + loop continues, null/undefined guard, end-to-end realistic escalation scenario enqueues all 6 event types with correct severities
`npm run test:data`: 4377/4377 pass
`npm run typecheck` + `typecheck:api`: clean
`biome lint` on touched files: clean
Dependency on notification-relay
The relay already handles generic events with a `[SEVERITY] title` fallback formatter. No relay-side changes are required — the new event types flow through the same default path as `rss_alert`. If we later want custom templates per regional event type, that's a follow-up to `scripts/notification-relay.cjs`.
Post-Deploy Monitoring & Validation
PR sequence