[teeohhem]

Summary

processAlert hard-codes threshold evaluation after its scheduling and chart-config setup. This adds an extension point for other detection strategies, and the threshold path is unchanged when none is registered.

• AlertEvaluator (tasks/checkAlerts/evaluators.ts): appliesTo(alert), evaluate(ctx), and an optional evaluationNow(alert, now) clock shift. The first registered evaluator that applies takes over; otherwise the threshold query runs as before.
• AlertEvaluationContext: what processAlert has already resolved (window, date range, unoptimized chart config, clients), plus the helpers the threshold path uses: getOrCreateHistory, trySendNotification, sendNotificationIfResolved, and a new persistHistories. Evaluators therefore get the same silencing, resolve and persistence behavior.
• evaluationNow is applied in both processAlert and getAlertWindowStart, so the scheduled window and the history fetched up front stay aligned.
• @/extensions: a registerExtensions() entry point, called once on the first evaluator lookup. It registers nothing in this repo.

Supporting changes:

• In processAlert, the notification helpers now come before the threshold query (both paths use them); otherwise unchanged.
• trySendNotification returns false when the send was suppressed (silenced) and true otherwise.
• alerts.evaluate records an alert_evaluator attribute: threshold, or the evaluator's name.

How to test locally or on Vercel

1. cd packages/api && yarn ci:unit src/tasks/checkAlerts: covers evaluator lookup, a registered evaluator bypassing the threshold query, the shifted clock, error recording, and the fallback to the threshold path.
2. make dev-int FILE=checkAlerts: the threshold path is unchanged (335 tests).

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated
hyperdx-oss	Ignored	Preview	Nov 9, 2026 7:10pm UTC
hyperdx-storybook	Skipped		Nov 9, 2026 7:10pm UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: b563d79

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@hyperdx/api	Patch
@hyperdx/app	Patch
@hyperdx/otel-collector	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

• Background tasks or delivery pipeline substantially modified — 430 lines (bar: 30):
  • packages/api/src/tasks/checkAlerts/evaluators.ts
  • packages/api/src/tasks/checkAlerts/index.ts

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats

• Production files changed: 3
• Production lines changed: 435 (+ 413 in test files, excluded from tier calculation)
• Critical-path lines changed: 430
• Branch: claude/alert-evaluator
• Author: teeohhem

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

Duplicate code analysis

No new duplicate code introduced by this PR.

---

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 110 in total (1.21%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[github-actions]

🔵 minor — persistHistories copies the threshold persist step, which still keeps its own two copies

The single-value branch (~1596-1606) and the time-series branch (~1830-1840) still do flush → attach analytics → updateAlertState by hand, and they already differ from persistHistories: they always attach analytics, while the helper skips analytics when the object is empty. Make both threshold exits call persistHistories(Array.from(histories.values())) so the 'same persistence behavior' the PR promises comes from one place.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[teeohhem]

Fixed in bf29ce8: both threshold exits now call persistHistories. Both already set backfilledBuckets, so the skip-empty-analytics branch never applies to them and their behavior is unchanged.

[github-actions]

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor

1 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor

• 🔵 packages/api/src/tasks/checkAlerts/index.ts:1220 — Materialized-view optimization still runs for alerts that a registered evaluator handles, and its result is thrown away → tryOptimizeConfigWithMaterializedView (which queries ClickHouse metadata) and clickHouseSettings are computed before the if (evaluator) branch. That branch passes the unoptimized chartConfig and tells evaluators to redo MV selection themselves, so every evaluator alert on a Log/Trace source with MVs pays for a lookup nobody uses. Move the mvSource/optimizedChartConfig/clickHouseSettings block below the evaluator early-return, next to the threshold queryChartConfig call.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[github-actions]

Deep Review

✅ No critical issues found. No P0/P1: this diff registers zero evaluators, so the threshold path is unchanged and the new extension-point machinery is latent. The findings below concern the contract the extension point establishes (which future evaluators will hit) plus one verified inefficiency in the shipped control flow.

🟡 P2 — recommended

• packages/api/src/tasks/checkAlerts/index.ts:454 — getAlertEvaluationNow's evaluator = findAlertEvaluator(alert) default fires on the explicitly-passed undefined that processAlert sends for threshold alerts, re-running the full lookup and every registered appliesTo a second (and, via the prefetch, third) time per alert each tick, defeating the single-lookup the prior commit introduced.
  • Fix: Make evaluator a required parameter and have getAlertWindowStart resolve it explicitly so a passed undefined means "no evaluator" rather than "look it up again."
  • _{correctness, kieran-typescript, adversarial, maintainability}
• packages/api/src/tasks/checkAlerts/index.ts:1488 — an evaluator that calls persistHistories (writing state=OK and clearing ERROR rows) and then throws falls to the outer catch, which re-stamps an ERROR history row at the same window start, so a window that succeeded renders as ERROR; the threshold path cannot hit this because persistHistories is its final statement.
  • Fix: Guard the catch against re-recording an ERROR row for an already-persisted window, or document that persistHistories must be the evaluator's last action.
  • _{adversarial, testing}
• packages/api/src/tasks/checkAlerts/index.ts:1488 — await evaluator.evaluate(ctx) has no timeout, unlike the threshold path bounded by clickhouseClient.requestTimeoutMs, so a hung evaluator on the shared PQueue can prevent onIdle() from resolving and stall the whole check-alerts tick.
  • Fix: Bound evaluator.evaluate with an enforced timeout or an abort signal exposed on the context.
  • _reliability
• packages/api/src/tasks/checkAlerts/index.ts:1893 — the new alert_evaluator attribute on the alerts.evaluate metric has no test asserting the evaluator name on the evaluator path or THRESHOLD_EVALUATOR_NAME on the fallback path, leaving a stated purpose of the change unverified.
  • Fix: Add assertions on the recorded alert_evaluator attribute for both the evaluator and threshold paths.
  • _testing

🔵 P3 nitpicks (4)

• packages/api/src/tasks/checkAlerts/index.ts:1893 — when findAlertEvaluator throws before evaluator is assigned, the finally labels the error outcome alert_evaluator: 'threshold', misattributing evaluator-lookup failures to the threshold path in the one metric meant to distinguish them.
  • Fix: Use a distinct label (e.g. lookup_failed) when the lookup did not resolve an evaluator.
  • _{adversarial, reliability}
• packages/api/src/tasks/checkAlerts/evaluators.ts:138 — clearAlertEvaluators sets extensionsRegistered = true, so once any test calls it no later test in the process can exercise the lazy first-use registerExtensions() path, and shared module state can leak between suites.
  • Fix: Reset extensionsRegistered = false in clearAlertEvaluators and document the shared-state coupling.
• packages/api/src/tasks/checkAlerts/evaluators.ts:561 — ensureExtensionsRegistered rolls back and rethrows without latching, so a persistently-throwing registerExtensions makes every findAlertEvaluator call fail, halting evaluation for all alerts (not just the extension's targets) each tick; the fail-closed choice is deliberate but has no retry bound.
  • Fix: Bound registration retries or fail closed only for alerts whose own evaluator failed.
  • _{adversarial, correctness}
• packages/api/src/tasks/checkAlerts/index.ts:2040 — the new Failed to compute alert window start for history prefetch error log emits no counter at its site, relying on processAlert re-running the lookup to count it, which departs from the CLAUDE.md "countable log events should also emit a metric" principle.
  • Fix: Emit a counter at the prefetch-failure site, or confirm the downstream alerts.evaluate error metric is the intended single source.

Previous-comments reviewer verified all six prior review threads (lookup-inside-try, registration latch/rollback, wall-clock history clamp, context-contract tests, per-alert prefetch containment, non-persisting-evaluator error writes) are genuinely resolved in the current diff, with no regressions.

---

Reviewers (11): correctness, adversarial, reliability, testing, maintainability, kieran-typescript, api-contract, project-standards, previous-comments, agent-native, learnings-researcher.

Testing gaps:

• No assertion on the new alert_evaluator metric attribute (evaluator name vs THRESHOLD_EVALUATOR_NAME).
• Evaluator that persists then throws (ERROR-over-OK double-write) is uncovered.
• No coverage for a hung/timed-out evaluator, for evaluationNow returning an invalid/NaN Date, or for a persistently-failing registerExtensions across a batch.
• Boundary case where the evaluator clock exactly equals the wall clock is not exercised.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

[github-actions]

E2E Test Results

✅ All tests passed • 462 passed • 1 skipped • 1270s

Status	Count
✅ Passed	462
❌ Failed	0
⚠️ Flaky	4
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

[teeohhem]

Deep Review findings:

1. Evaluator lookup outside the try: fixed in 48cc76b. The evaluator and clock are now resolved inside the try, so a throwing lookup is recorded on the alert and counted on alerts.evaluate. A failed lookup records an error instead of falling back to the threshold path. Evaluating an alert with a detector it wasn't written for would be wrong without any error.
2. Registration latch set before registerExtensions: fixed in 48cc76b. The latch is set only after success. A failed registration is rolled back and rethrown, then retried on the next lookup.
3. getPreviousAlertHistories uses the wall clock: that bound (createdAt <= wallClock) covers every window as long as the evaluation clock is not ahead of the wall clock. 48cc76b now enforces that: an evaluationNow later than the wall clock is ignored, and the docs say so.
4. Context contract untested: added in 48cc76b. The new tests cover:
   • suppressed vs attempted sends
   • a resolve notification sent through the context
   • merged persist errors and analytics attached only when measured
   • the up-front history window under a shifted or ahead-of-wall-clock clock
   • a failing lookup
   • registration rollback and retry
5. Narrow the context, add a stability marker and a consumer fixture: I added the marker: AlertEvaluationContext is documented as an internal extension point, not a stable API. I'm leaving the shape as is. The members are the helpers the threshold path already uses, and narrowing them now would mean guessing what future evaluators need. The unit tests compile against the context type, so a rename in processAlert fails type-checking here.

Unit tests (60) and make dev-int FILE=checkAlerts (335) pass after these commits.

[github-actions]

🔵 minor — findAlertEvaluator's doc comment is orphaned above ensureExtensionsRegistered

Two JSDoc blocks are stacked: "The registered evaluator for this alert, or undefined for the threshold path" sits directly above the ensureExtensionsRegistered doc. Editors and TypeDoc attach neither one to findAlertEvaluator. Fix: move that first comment onto findAlertEvaluator.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[teeohhem]

Fixed in 0d94bcf.

[teeohhem]

Round 2 addressed in 0d94bcf:

• History prefetch: an evaluator lookup failure is contained per alert, so it no longer rejects the whole batch.
• Evaluators that return without persisting still write their collected execution errors.
• Lint: removed three as any casts that put @hyperdx/api over its 302-warning ceiling.
• Added a changeset.
• Materialized-view optimization for evaluator alerts (outside the changed lines): not moved. It only wastes work, no evaluator is registered in this repo, and moving it would reorder the threshold path.

Unit tests (62) and make dev-int FILE=checkAlerts (335) pass. This is the second round of bot findings, so I am stopping here. A maintainer should review scope before more automated rounds.

[github-actions]

🔵 minor — A failed evaluator lookup is recorded on alerts.evaluate as a threshold error

If findAlertEvaluator throws (because registerExtensions or an appliesTo throws), evaluator is still undefined when the finally runs. The error is then labelled alert_evaluator: 'threshold', so extension failures inflate the threshold path's error rate. Do the lookup inside its own try block and record a distinct label (for example 'unknown') when it fails.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}