Skip to content

feat(api): accept and return multiple alert notification channels - #2846

Open
jordan-simonovski wants to merge 4 commits into
jordansimonovski/alerts-multi-channel-modelfrom
jordansimonovski/alerts-multi-channel-api
Open

feat(api): accept and return multiple alert notification channels#2846
jordan-simonovski wants to merge 4 commits into
jordansimonovski/alerts-multi-channel-modelfrom
jordansimonovski/alerts-multi-channel-api

Conversation

@jordan-simonovski

@jordan-simonovski jordan-simonovski commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Exposes multi-channel alerts through the v2 external API, the internal API, and the MCP clickstack_save_alert tool. Writes accept a channels array, and every alert response carries both channels and the legacy channel.

What changed

alertSchema (shared by /api/alerts and /api/v2/alerts) takes channel and/or channels, replacing its local channel definition with the shared one from common-utils.

validateAlertInput checks every referenced webhook exists and belongs to the team in a single query instead of one per channel. makeAlert persists the canonical channels array with channel mirrored to channels[0].

v2 responses gain channels; the internal API also returns channels but masks webhookId to {type}, matching the existing policy for channel. OpenAPI documents the new shape and the input rule.

The MCP tool accepts channels with hand-rolled cross-field validation, since the MCP SDK cannot serialise ZodEffects.

Key decisions

Updates replace rather than merge. A client that sends only the legacy channel when updating a multi-channel alert reduces it to that one target. Merging would make channels unremovable by legacy clients, so the behaviour stands and is documented in the changeset, the OpenAPI update schema, and the MCP tool description.

Bulk webhook check by count. countDocuments({_id: {$in: uniqueIds}, team}) compared against the deduplicated id count. A cross-team or non-existent id can only lower the count, so it still rejects — and ids are validated as ObjectIds before reaching $in.

Impact

Additive for existing consumers: channel is still accepted on input and present on every response. channel was removed from the required list of the create and update request schemas, since either field now satisfies the requirement.

Editing a multi-channel alert in the app still replaces channels with the single UI-selected channel, until the alert forms support the full list.

Implementation detail

The round-trip case is covered end to end: a response body is PUT back unchanged and the alert keeps both channels. Rejections are covered for a channel/channels mismatch, neither field, duplicates, exceeding the cap, and a cross-team webhook hidden among valid ones — on both the external and internal routes, and on the MCP tool.

Verification: 52 external-api, 43 internal, 31 MCP integration tests; yarn lint:openapi clean.

@changeset-bot

changeset-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 57f6d58

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 Minor
@hyperdx/app Minor
@hyperdx/otel-collector Minor

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

@vercel

vercel Bot commented Aug 9, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 10, 2026 1:36am
hyperdx-storybook Ready Ready Preview Aug 10, 2026 1:36am

Request Review

@greptile-apps

greptile-apps Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR exposes multi-channel alert configuration through the internal API, external v2 API, and MCP alert tool while retaining the singular compatibility field.

  • Accepts and validates one to ten webhook channels.
  • Persists a canonical channels array and mirrors its first entry into channel.
  • Adds multi-channel response translation, OpenAPI documentation, and integration coverage across all three interfaces.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains within the eligible follow-up scope.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/api/src/mcp/tools/alerts/saveAlert.ts Converts singular or plural MCP channel input into the canonical array before invoking shared alert controllers.
packages/api/src/mcp/tools/alerts/schemas.ts Extends the MCP schema with bounded channel arrays and runtime cross-field validation.
packages/api/src/routers/api/alerts.ts Adds masked plural channels to internal alert read responses while preserving the established response policy.
packages/api/src/routers/external-api/v2/alerts.ts Exposes plural channels through external alert requests, responses, and generated API documentation.
packages/api/src/utils/externalApi.ts Translates legacy and multi-channel alert documents into compatible external response shapes.
packages/api/openapi.json Documents channel-array constraints, compatibility behavior, and full-replacement update semantics.

Reviews (2): Last reviewed commit: "test(api): pin the legacy channel-only u..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This is a well-scoped, additive change with strong test coverage. The team-ownership check for referenced webhooks was independently verified as sound (deduped ids counted against a team-scoped query, so a cross-team or non-existent id always lowers the count and rejects), no cross-tenant exposure is introduced, the changeset is present and correctly bumped, and OpenAPI JSDoc and the generated openapi.json are in sync. The findings below are recommendations and nitpicks.

🟡 P2 -- recommended

  • packages/api/src/utils/externalApi.ts:340 -- A channel-less alert (a legacy/disabled document whose stored channel is {type: null}) serializes to channels: [] and channel: {type: null}, which violates the newly published AlertChannels (minItems: 1) and AlertChannel (webhook discriminator, no null type) schemas, so a schema-validating consumer rejects the response and cannot echo it back on PUT.
    • Fix: Reference an unconstrained channel array for the response Alert schema (keep minItems:1 only on the request), or omit both fields when no valid channel exists so responses stay within their own contract.
    • api-contract, correctness, kieran-typescript, adversarial
  • packages/api/src/mcp/tools/alerts/schemas.ts:434 -- validateSaveAlertInput hand-rolls the same neither/mismatch/duplicate cross-field rule that validateAlertChannelSelection already owns in common-utils, and the two already differ (MCP keys on webhookId only, common-utils on type:webhookId); they will drift as the rule evolves.
    • Fix: Refactor the shared rule into a pure predicate that both the Zod superRefine and the MCP runtime path call, so the logic has one definition.
    • maintainability, correctness, kieran-typescript
  • packages/api/src/mcp/__tests__/alerts.int.test.ts:513 -- The MCP multi-channel suite omits an over-cap (>10 channels) rejection case that both the internal and external API suites include, and the cap is enforced only by the SDK-level Zod .max() (not re-checked in validateSaveAlertInput), so a regression dropping SDK schema validation would go unnoticed.
    • Fix: Add an MCP test that submits 11 channels and asserts result.isError is true.
    • testing, agent-native
🔵 P3 nitpicks (7)
  • packages/api/src/mcp/tools/alerts/schemas.ts:24 -- mcpAlertChannelSchema.webhookId uses z.string() without .nonempty(), laxer than the common-utils zAlertChannel; an empty-string webhookId passes MCP validation and is only rejected later by validateObjectId.
    • Fix: Use z.string().nonempty() for webhookId to fail fast and match common-utils.
    • correctness, adversarial
  • packages/api/openapi.json:381 -- Removing channel from the required list leaves the "at least one of channel or channels" rule expressed only in prose, so generated clients/validators accept a request with neither field even though the runtime returns 400.
    • Fix: Express the rule machine-readably, e.g. anyOf: [{required:[channel]},{required:[channels]}] on the Create/Update request objects.
    • api-contract
  • packages/api/src/routers/api/alerts.ts:54 -- The internal response masks every channels[] webhookId to {type}, so a read-modify-write client cannot see or preserve channels 2..N; echoing the masked body back on PUT fails validation.
    • Fix: Provide an unmasked read path for internal edit flows, or document that internal callers must reconstruct channels rather than echo the masked response.
    • adversarial
  • packages/api/src/mcp/tools/alerts/schemas.ts:407 -- Exceeding the cap surfaces a raw Zod array-max error to the agent caller instead of the friendly mcpUserError used by the runtime validation path.
    • Fix: Add a custom .max() message so an agent gets a clear, consistent error.
    • agent-native
  • packages/api/src/mcp/__tests__/alerts.int.test.ts:572 -- Rejection tests assert exact human-readable substrings ('must match the first entry', 'Duplicate'), so rewording a message breaks the tests with no behavior change.
    • Fix: Assert only result.isError === true (or a stable error code) for these cases, as the API suites do.
    • testing
  • packages/api/src/utils/externalApi.ts:350 -- The legacy-mirror expression channel: channels[0] ?? { type: null } is duplicated in both the external translator and the controller write path.
    • Fix: Extract a small mirrorLegacyChannel(channels) helper beside getAlertChannels and reuse it in both places.
    • maintainability
  • packages/api/src/mcp/__tests__/alerts.int.test.ts:608 -- The MCP update path is never exercised with channels; the multi-channel suite only creates, and the existing update test uses a single legacy channel, so replace/collapse semantics via MCP are unverified.
    • Fix: Add an MCP test that creates a multi-channel alert then updates it (with id) using a different channels array and asserts the replacement.
    • testing, correctness

Reviewers (10): correctness, testing, maintainability, api-contract, security, kieran-typescript, adversarial, project-standards, agent-native, learnings-researcher.

Testing gaps:

  • The internal-API multi-channel PUT round-trip test resends webhookIds taken from the POST echo, not the masked GET/list body, so it never exercises the real read-modify-write path a client would use.
  • No test drives translateAlertDocumentToExternalAlert/formatAlertResponse for a channel-less alert (channels: [] + channel: {type: null}), leaving the fallback branch and its contract mismatch unverified.
  • MCP lacks tests for the over-cap (>10) case, an empty-string webhookId, and updating an existing alert with a new channels array.

v2 alert responses carry the full channels array with the legacy channel
field mirroring the first entry, so existing consumers are unaffected and
a response body can still be PUT back unchanged. OpenAPI components
document the new shape and the channel/channels input rule.
The internal /api/alerts route gained `channels` with no test coverage:
create with several channels, a legacy single-channel alert gaining
channels in the list response, a PUT round-trip, and the rejection cases
(neither field, channel/channels mismatch, duplicates, over the cap, and
a cross-team webhook hidden among valid ones).

Also document that updates replace rather than merge -- a client sending
only the legacy `channel` reduces a multi-channel alert to one target --
in the OpenAPI update schema, the MCP tool description an agent reads,
and the changeset.
Updates are a full replace, so a client that predates `channels` and
sends only `channel` reduces a multi-channel alert to that one target.
That behaviour is documented but was asserted nowhere -- the round-trip
test echoes both fields back, so it never exercised the collapse.
@jordan-simonovski
jordan-simonovski force-pushed the jordansimonovski/alerts-multi-channel-api branch from 2125c82 to 57f6d58 Compare August 10, 2026 01:32
@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

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

Why this tier:

  • Critical-path files (1) — tenancy, public API, or shipped database config:
    • packages/api/src/routers/external-api/v2/alerts.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: 6
  • Production lines changed: 153 (+ 436 in test files, excluded from tier calculation)
  • Critical-path lines changed: 44
  • Branch: jordansimonovski/alerts-multi-channel-api
  • Author: jordan-simonovski

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

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 276 passed • 1 skipped • 885s

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

Tests ran across 4 shards in parallel.

View full report →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant