Repository navigation
Conversation
processAlert hard-codes threshold evaluation after its scheduling and config setup. Add an AlertEvaluator interface: the first registered evaluator whose appliesTo matches evaluates the alert through an AlertEvaluationContext (the same history, notification, and persistence helpers the threshold path uses); otherwise the threshold path runs unchanged. An evaluator can also shift the evaluation clock, applied in processAlert and in getAlertWindowStart so window and history stay aligned. Evaluators are registered from @/extensions, called once on first lookup. It registers none here. The evaluator name is recorded on alerts.evaluate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches 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:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| }; | ||
|
|
||
| const persistHistories = async ( |
There was a problem hiding this comment.
🔵 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
There was a problem hiding this comment.
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.
| teamWebhooksById: Map<string, IWebhook>, | ||
| ) => { | ||
| const { alert, previousMap, recentHistoryMap } = details; | ||
| const now = getAlertEvaluationNow(alert, wallClockNow); |
There was a problem hiding this comment.
🔵 minor — Evaluator looked up twice per processAlert; the clock and the evaluator can come from different evaluators
getAlertEvaluationNow calls findAlertEvaluator internally, and the next line calls it again. If an appliesTo depends on mutable state, the clock shift and the evaluator that runs can belong to different evaluators. Look it up once (const evaluator = findAlertEvaluator(alert); const now = evaluator?.evaluationNow?.(alert, wallClockNow) ?? wallClockNow;), and give getAlertEvaluationNow the evaluator as a parameter.
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
There was a problem hiding this comment.
Fixed in bf29ce8: processAlert looks the evaluator up once and passes it to getAlertEvaluationNow.
| expect(ctx?.nowInMinsRoundDown).toEqual(new Date('2025-01-01T00:00:00Z')); | ||
| }); | ||
|
|
||
| it('records an error thrown by the evaluator on the alert', async () => { |
There was a problem hiding this comment.
🔵 minor — Nothing tests that the history prefetch window follows evaluationNow
The PR says evaluationNow keeps getAlertWindowStart (used by getConsecutiveWindowHistories) in line with processAlert, but the clock test only checks processAlert's nowInMinsRoundDown. Dropping the getAlertEvaluationNow call from getAlertWindowStart would still pass. Add a case with numConsecutiveWindows > 1 and a shifted clock that checks the createdAt range getConsecutiveWindowHistories queries (or export getAlertWindowStart and test it directly).
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
There was a problem hiding this comment.
Fixed in 48cc76b: getAlertWindowStart is exported and tested directly, for both a shifted clock and one that runs ahead of the wall clock (ignored).
PR Review
4 finding(s): 🔴 0 critical · 🟠 2 major · 🔵 2 minor 3 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep ReviewScope: The threshold-path refactor is clean: 🟡 P2 — recommended
🔵 P3 nitpicks (3)
Reviewers (11): correctness, reliability, testing, maintainability, project-standards, kieran-typescript, api-contract, adversarial, previous-comments, agent-native, learnings-researcher. Testing gaps: Confirm the test suite asserts that a registered evaluator actually bypasses the ClickHouse query (threshold path not taken), and that an evaluator throwing records the error while the 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 |
E2E Test Results✅ All tests passed • 462 passed • 1 skipped • 1698s
Tests ran across 4 shards in parallel. |
…clock - Resolve the evaluator and clock inside processAlert's try, so a throwing appliesTo/evaluationNow/registerExtensions is recorded on the alert and counted on alerts.evaluate instead of escaping. - Mark extensions registered only after registerExtensions succeeds; roll back a partial registration and retry on the next lookup, rather than silently routing alerts to the threshold path. - Ignore an evaluationNow later than the wall clock: previous-state lookups bound history by the wall clock, which only covers every window when the evaluation clock is not ahead of it. - Test the context contract: suppressed vs attempted sends, resolve via the context, merged persist errors, conditional analytics, and the up-front history window under a shifted or clamped clock. - Note that AlertEvaluationContext is not a stable API. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… results through persistHistories Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| alert: IAlert, | ||
| wallClockNow: Date, | ||
| ): Date => { | ||
| const now = getAlertEvaluationNow(alert, wallClockNow); |
There was a problem hiding this comment.
🟠 major — An evaluator lookup failure in getAlertWindowStart stops every alert from loading, not just the faulty one
getConsecutiveWindowHistories now calls findAlertEvaluator through getAlertWindowStart. That call runs registerExtensions and each evaluator's appliesTo/evaluationNow, so any throw rejects the Promise.all in DefaultAlertProvider.getAlertTasks (providers/default.ts:435-438). It only takes one multi-window alert hitting a throwing appliesTo to fail getAlertTasks for the whole cycle. The containment added to processAlert does not cover this path. Fix: catch the lookup per alert in getConsecutiveWindowHistories (fall back to the wall clock or skip that alert's history), and add a test for it next to 'records a failing evaluator lookup on the alert'.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| }; | ||
| try { | ||
| evaluator = findAlertEvaluator(alert); | ||
| const now = getAlertEvaluationNow(alert, wallClockNow); |
There was a problem hiding this comment.
🔵 minor — Every evaluation looks up the evaluator twice
processAlert calls findAlertEvaluator(alert), and then getAlertEvaluationNow calls it again, so every appliesTo runs twice per evaluation. A non-deterministic appliesTo could return a clock from a different evaluator than the one that runs evaluate. Fix: change getAlertEvaluationNow to take the already-resolved evaluator (e.g. (evaluator, alert, now)) and pass it in from both callers.
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
|
Deep Review findings:
Unit tests (60) and |
| alert: IAlert, | ||
| wallClockNow: Date, | ||
| ): Date => { | ||
| const now = getAlertEvaluationNow(alert, wallClockNow); |
There was a problem hiding this comment.
🟠 major — An evaluator lookup that throws in getAlertWindowStart aborts the whole alert run, not just one alert
getAlertWindowStart now calls findAlertEvaluator and evaluationNow, and nothing guards either call. getConsecutiveWindowHistories runs them inside one Promise.all, and that is awaited unguarded in DefaultAlertProvider.getAlertTasks (providers/default.ts:435). So if registerExtensions or one evaluator's appliesTo/evaluationNow throws for any multi-window alert, the whole tick returns no tasks and no alert gets evaluated. Commit 48cc76b contains this failure only inside processAlert. Fix: wrap the lookup here (or per alert in getConsecutiveWindowHistories) in a try/catch that falls back to the wall clock or skips that alert, and add a test where appliesTo throws in getConsecutiveWindowHistories.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| sendNotificationIfResolved, | ||
| persistHistories, | ||
| }; | ||
| await evaluator.evaluate(ctx); |
There was a problem hiding this comment.
🟠 major — If an evaluator returns without persisting, its errors are silently dropped and the run counts as success
After await evaluator.evaluate(ctx); return;, nothing checks whether persistHistories ran. If an evaluator returns early (for example, no data yet) after trySendNotification has pushed webhook failures onto executionErrors, those errors are never written. evalOutcome also stays 'success', and evaluators cannot set it to 'error' or 'skipped', so the alerts.evaluate SLI misreports them. Fix: track whether persistHistories (or recordAlertErrors) was called and flush executionErrors if it wasn't, and give evaluators a way to set the outcome, e.g. a returned status or a ctx.setOutcome setter.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| evaluators.push(evaluator); | ||
| }; | ||
|
|
||
| /** The registered evaluator for this alert, or undefined for the threshold |
There was a problem hiding this comment.
🔵 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
Summary
processAlerthard-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 optionalevaluationNow(alert, now)clock shift. The first registered evaluator that applies takes over; otherwise the threshold query runs as before.AlertEvaluationContext: whatprocessAlerthas already resolved (window, date range, unoptimized chart config, clients), plus the helpers the threshold path uses:getOrCreateHistory,trySendNotification,sendNotificationIfResolved, and a newpersistHistories. Evaluators therefore get the same silencing, resolve and persistence behavior.evaluationNowis applied in bothprocessAlertandgetAlertWindowStart, so the scheduled window and the history fetched up front stay aligned.@/extensions: aregisterExtensions()entry point, called once on the first evaluator lookup. It registers nothing in this repo.Supporting changes:
processAlert, the notification helpers now come before the threshold query (both paths use them); otherwise unchanged.trySendNotificationreturnsfalsewhen the send was suppressed (silenced) andtrueotherwise.alerts.evaluaterecords analert_evaluatorattribute:threshold, or the evaluator's name.How to test locally or on Vercel
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.make dev-int FILE=checkAlerts: the threshold path is unchanged (335 tests).🤖 Generated with Claude Code