Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 37 additions & 10 deletions packages/api/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,6 @@
],
"description": "Current alert state."
},
"AlertChannelType": {
"type": "string",
"enum": [
"webhook"
],
"description": "Channel type."
},
"AlertErrorType": {
"type": "string",
"enum": [
Expand Down Expand Up @@ -250,8 +243,12 @@
],
"properties": {
"type": {
"$ref": "#/components/schemas/AlertChannelType",
"description": "Channel type. Must be \"webhook\" for webhook alerts."
"type": "string",
"enum": [
"webhook"
],
"description": "Channel type. Must be \"webhook\" for webhook alerts.",
"example": "webhook"
},
"webhookId": {
"type": "string",
Expand All @@ -260,14 +257,44 @@
}
}
},
"AlertChannelAgent": {
"type": "object",
"description": "AI agent notification target. A firing alert starts an investigation session on the referenced managed agent. Only available on deployments with managed agents enabled.",
"required": [
"type",
"agentId"
],
"properties": {
"type": {
"type": "string",
"enum": [
"agent"
],
"description": "Channel type. Must be \"agent\" for agent alerts.",
"example": "agent"
},
"agentId": {
"type": "string",
"description": "Managed agent ID.",
"example": "65f5e4a3b9e77c001a789014"
}
}
},
"AlertChannel": {
"oneOf": [
{
"$ref": "#/components/schemas/AlertChannelWebhook"
},
{
"$ref": "#/components/schemas/AlertChannelAgent"
}
],
"discriminator": {
Comment thread
jordan-simonovski marked this conversation as resolved.
Resolved
Comment thread
jordan-simonovski marked this conversation as resolved.
Resolved
"propertyName": "type"
"propertyName": "type",
"mapping": {
"webhook": "#/components/schemas/AlertChannelWebhook",
"agent": "#/components/schemas/AlertChannelAgent"
}
}
},
"AlertChannels": {
Expand Down
55 changes: 42 additions & 13 deletions packages/api/src/controllers/alerts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { groupBy } from 'lodash';
import { Types } from 'mongoose';
import { z } from 'zod';

import { IS_MANAGED_AGENTS_ENABLED } from '@/config';
import type { ObjectId } from '@/models';
import Alert, {
AlertChannel,
Expand All @@ -19,6 +20,7 @@ import Alert, {
} from '@/models/alert';
import Connection from '@/models/connection';
import Dashboard, { IDashboard } from '@/models/dashboard';
import ManagedAgent from '@/models/managedAgent';
import { ISavedSearch, SavedSearch } from '@/models/savedSearch';
import { Source } from '@/models/source';
import { IUser } from '@/models/user';
Expand Down Expand Up @@ -217,19 +219,46 @@ export const validateAlertInput = async (
throw new Api400Error('At least one notification channel is required');
}

const webhookIds = channels
.filter(c => c.type === 'webhook')
.map(c => c.webhookId);
for (const webhookId of webhookIds) {
validateObjectId(webhookId, 'Invalid webhook ID');
}
const uniqueIds = [...new Set(webhookIds)];
const found = await Webhook.countDocuments({
_id: { $in: uniqueIds },
team: teamId,
});
if (found !== uniqueIds.length) {
throw new Api400Error('Webhook not found');
// Each channel kind names a row in a different collection; the check is the
// same either way — the ids parse, and every distinct one belongs to this
// team. Kept as one pass so a third channel kind cannot arrive with a
// subtly different rule.
const assertChannelRefsExist = async (
ids: string[],
model: { countDocuments: (f: object) => PromiseLike<number> },
label: string,
) => {
for (const id of ids) {
validateObjectId(id, `Invalid ${label} ID`);
}
const unique = [...new Set(ids)];
const found = await model.countDocuments({
_id: { $in: unique },
team: teamId,
});
if (found !== unique.length) {
throw new Api400Error(
`${label[0].toUpperCase()}${label.slice(1)} not found`,
);
}
};

await assertChannelRefsExist(
channels.filter(c => c.type === 'webhook').map(c => c.webhookId),
Webhook,
'webhook',
);

const agentIds = channels.filter(c => c.type === 'agent').map(c => c.agentId);
Comment thread
jordan-simonovski marked this conversation as resolved.
Resolved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — Dashboard tile alerts never reach validateAlertInput, so an agent channel bypasses both the feature flag and team scoping

POST/PATCH /dashboards persists tile alerts through controllers/dashboard.ts:93 → createOrUpdateDashboardAlerts → makeAlert, with no call to validateAlertInput anywhere on that path — the only validation is DashboardWithoutIdSchema → TileSchema.config → AlertBaseChannelCheckedSchema, which now accepts {type:'agent', agentId} because zAlertChannel widened. So a body with tiles[0].config.alert.channels = [{type:'agent', agentId:'<another team's id>'}] (or any nonempty string — zAgentAlertChannel only requires nonempty, not an ObjectId) saves cleanly even with HDX_MANAGED_AGENTS_ENABLED unset, defeating both new rules. The webhook equivalent is pre-existing but self-limiting, because dispatch resolves webhooks from the team-scoped teamWebhooksById; an agent id has no such backstop once the dispatch PR lands. Call the channel portion of validateAlertInput from syncDashboardAlerts/createDashboard, or move the flag + ref check into makeAlert so every writer gets it.

if (agentIds.length > 0) {
// The alert editor is flag-gated too; this rejects an agent channel saved
// via the API on a deployment where dispatch would silently do nothing.
if (!IS_MANAGED_AGENTS_ENABLED) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Disabling the flag makes an existing agent-channel alert impossible to edit or repair

validateAlertInput runs on update as well as create, so after HDX_MANAGED_AGENTS_ENABLED is turned off, every PUT on an alert that already stores an agent channel fails with 400 — including a UI save that only changes the threshold, because alertToFormValues/toFormAlertChannels echo the agent channel back untouched. The user cannot edit their way out either: AlertChannelForm skips any row whose type is not webhook (packages/app/src/components/Alerts.tsx:92-95), so the agent row is invisible and has no remove button. Scope the flag check to agent ids that are not already on the stored alert (or let the update pass when the channel set is unchanged), so an operator disabling the feature does not strand the alerts it created.

throw new Api400Error(
'AI agent notification channels are not enabled on this deployment (set HDX_MANAGED_AGENTS_ENABLED)',
);
}
await assertChannelRefsExist(agentIds, ManagedAgent, 'agent');
}

return refs;
Expand Down
37 changes: 37 additions & 0 deletions packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,3 +209,40 @@ describe('mcpSaveAlertSchema isDelta dialect bridge', () => {
},
);
});

describe('validateSaveAlertInput per-type channel id pairing', () => {
const ag = (id: string) => ({ type: 'agent' as const, agentId: id });

it('accepts a webhook channel with a webhookId and an agent channel with an agentId', () => {
expect(
validateSaveAlertInput({ ...baseInput, channels: [wh('a'), ag('b')] }),
).toBeNull();
});

it('rejects a webhook channel missing webhookId', () => {
expect(
validateSaveAlertInput({
...baseInput,
channel: foreignChannel({ type: 'webhook' }),
}),
).toBe('webhookId is required for a webhook channel');
});

it('rejects an agent channel missing agentId', () => {
expect(
validateSaveAlertInput({
...baseInput,
channel: foreignChannel({ type: 'agent' }),
}),
).toBe('agentId is required for an agent channel');
});

it('rejects when any entry in channels misses its per-type id', () => {
expect(
validateSaveAlertInput({
...baseInput,
channels: [wh('a'), foreignChannel({ type: 'agent' })],
}),
).toBe('agentId is required for an agent channel');
});
});
30 changes: 23 additions & 7 deletions packages/api/src/mcp/tools/alerts/saveAlert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
mcpUserError,
validateObjectId,
} from '@/mcp/utils/errors';
import { AlertSource } from '@/models/alert';
import { AlertChannel, AlertSource } from '@/models/alert';
import {
convertExternalAlertChartConfigToInternal,
translateAlertDocumentToExternalAlertWithChartConfig,
Expand Down Expand Up @@ -48,11 +48,13 @@ export function registerSaveAlert({
'Alerts monitor a saved search, a dashboard tile, or an inline chart ' +
'config (source "inline" + chartConfig — no saved search or dashboard ' +
'needed) and fire when the metric crosses a threshold. At least one ' +
'webhook notification channel ' +
'is required: pass "channels" for 1-10 targets, or the legacy singular ' +
'"channel" for one. Updates replace the alert configuration rather than ' +
'merging it, so read the alert first and resend its full "channels" ' +
'array to avoid dropping channels you did not mean to remove.',
'notification channel is required — a webhook ' +
'({type:"webhook", webhookId}) or, on deployments with managed agents ' +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — MCP tool now asks for an agentId that no MCP tool can discover

The clickstack_save_alert description instructs callers to pass {type:"agent", agentId}, but the alerts tool group registers clickstack_get_webhook (packages/api/src/mcp/tools/alerts/getWebhook.ts) and nothing equivalent for managed agents — packages/api/src/mcp/tools/ has no agent listing tool, and MCP.md:147 documents only the webhook one. An MCP client has no way to obtain a valid agent id, so it will guess and get Agent not found. Either add a list-agents tool alongside the webhook one, or hold the agent wording in this description until that exists.

'enabled, an AI agent investigation ({type:"agent", agentId}). Pass ' +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — The MCP tool and the v2 OpenAPI now ask for an agentId that neither surface can supply

MCP exposes clickstack_get_webhook to discover webhook ids, and external v2 exposes routers/external-api/v2/webhooks.ts; there is no equivalent for managed agents on either — /managed-agents is mounted only on the session-authenticated internal app (api-app.ts:106). So an agent following this tool description, or a client reading the new AlertChannelAgent schema in the v2 spec, has no documented way to obtain the id the field requires. Either add an agent-listing tool/v2 endpoint alongside this, or say in both descriptions that agent ids come from the app UI only.

'"channels" for 1-10 targets, or the legacy singular "channel" for ' +
'one. Updates replace the alert configuration rather than merging it, ' +
'so read the alert first and resend its full "channels" array to ' +
'avoid dropping channels you did not mean to remove.',
inputSchema: mcpSaveAlertSchema,
},
async input => {
Expand Down Expand Up @@ -95,10 +97,24 @@ export function registerSaveAlert({

// Build the alert input matching the shape expected by controllers.
const source = MCP_SOURCE_TO_ALERT_SOURCE[input.source];
// The MCP schema keeps webhookId/agentId both optional on one object
// shape (the per-type pairing is enforced in validateSaveAlertInput), so
// narrow into the discriminated AlertChannel here.
const toAlertChannel = (c: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — The MCP narrowing that decides what gets persisted has no test

toAlertChannel is the only code turning MCP input into the stored AlertChannel, and the new tests only cover validateSaveAlertInput; swapping its two branches (emitting a webhook channel for type:'agent') would pass every test added here, since mcp/__tests__/alerts.int.test.ts never sends an agent channel. Add an MCP integration case that saves an alert with {type:'agent', agentId} and asserts the persisted channel/channels, mirroring the internal-API case at routers/api/__tests__/alerts.int.test.ts:408.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — No test covers an agent channel through the MCP tool, including the new narrowing

toAlertChannel is the only new mapping logic on the MCP path and is defined inside the request handler, so it is unreachable from unit tests; the added cases in mcp/tools/alerts/__tests__/schemas.test.ts only assert validateSaveAlertInput's return value, and mcp/__tests__/alerts.int.test.ts has no agent case. A regression returning {type:'webhook', webhookId:''} for an agent input would pass every added test. Add an int case to mcp/__tests__/alerts.int.test.ts that saves an agent channel and asserts the persisted/returned channel keeps agentId (mirroring the internal-API cases in routers/api/__tests__/alerts.int.test.ts).

type: 'webhook' | 'agent';
webhookId?: string;
agentId?: string;
}): AlertChannel =>
c.type === 'agent'
? { type: 'agent', agentId: c.agentId ?? '' }
: { type: 'webhook', webhookId: c.webhookId ?? '' };

const alertInput: AlertInput = {
source,
// `channel` is omitted; makeAlert mirrors it from channels[0].
channels: input.channels ?? (input.channel ? [input.channel] : []),
channels: (
input.channels ?? (input.channel ? [input.channel] : [])
).map(toAlertChannel),
interval: input.interval,
threshold: input.threshold,
thresholdType: input.thresholdType as AlertThresholdType,
Expand Down
21 changes: 20 additions & 1 deletion packages/api/src/mcp/tools/alerts/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,18 @@ const mcpAlertChartConfigSchema = z
const mcpAlertChannelSchema = z

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — The MCP channel schema accepts webhookId and agentId together and the off-variant id is silently dropped

{type:'webhook', webhookId:'w1', agentId:'a1'} passes validateSaveAlertInput (the webhook branch only checks webhookId), and toAlertChannel in saveAlert.ts:108 then discards agentId with no error — an LLM caller that fills in both optional fields gets an alert missing the agent target it asked for. Reject the mismatched id in the same loop that enforces the pairing (schemas.ts:257), e.g. return an error when a webhook channel carries agentId or an agent channel carries webhookId.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — MCP's flat channel shape is hashed before narrowing, so the duplicate/mismatch check disagrees with what gets persisted

checkAlertChannelSelection hashes the raw MCP objects (alertChannelKey = objectHash of the whole object), but toAlertChannel (saveAlert.ts:103) then drops the id that doesn't belong to the type. channels: [{type:'webhook',webhookId:'a'}, {type:'webhook',webhookId:'a',agentId:'x'}] hashes as two distinct entries, passes the duplicate rule, and persists as two identical webhook channels that every other write path rejects; symmetrically, channel:{type:'webhook',webhookId:'a'} with channels:[{type:'webhook',webhookId:'a',agentId:'b'}] is rejected as a mismatch although both narrow to the same channel. Narrow with toAlertChannel first and run checkAlertChannelSelection on the narrowed channels, or reject a cross-type id in validateSaveAlertInput instead of silently discarding it.

.object({
type: z
.literal('webhook')
.enum(['webhook', 'agent'])
.describe('Channel type for alert notifications.'),
webhookId: z

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — MCP channel schema admits both ids at once; the extra one is silently dropped and defeats duplicate detection

With webhookId and agentId both optional on one object, {type:'webhook', webhookId:'w1', agentId:'a1'} passes validateSaveAlertInput (it only checks the id matching the type) and toAlertChannel (saveAlert.ts:103) drops agentId without comment. Because checkAlertChannelSelection hashes the whole raw object, channels: [{type:'webhook',webhookId:'w1'}, {type:'webhook',webhookId:'w1',agentId:'x'}] is not seen as a duplicate yet persists two identical webhook channels. Make the nested channel/channels field a z.discriminatedUnion('type', [...]) of the two variants — MCP input schemas allow a union on a nested property — which removes the hand-rolled pairing check and the unreachable ?? '' fallbacks too.

.string()
.optional()
.describe('Webhook destination ID (required for webhook channel).'),
agentId: z
.string()
.optional()
.describe(
'Managed agent ID (required for agent channel; a firing alert starts an AI investigation on this agent).',
),
})
.describe('Alert notification channel configuration.');

Expand Down Expand Up @@ -245,6 +252,18 @@ export function validateSaveAlertInput(data: McpSaveAlertInput): string | null {
}
}

// Per-type required channel id (the Zod schema keeps both ids optional so
// one object shape serves both types; enforce the pairing here).
for (const channel of [data.channel, ...(data.channels ?? [])]) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — MCP duplicate/mismatch channel rules run before per-type narrowing, so a stray foreign id defeats them

checkAlertChannelSelection hashes the whole channel object (alertChannelKey = objectHash), and mcpAlertChannelSchema now declares both webhookId and agentId as known optional fields, so they survive the parse. channels: [{type:'webhook',webhookId:'A'}, {type:'webhook',webhookId:'A',agentId:'X'}] hashes to two distinct values, passes the duplicate rule, and toAlertChannel (saveAlert.ts:103-110) then collapses both to {type:'webhook',webhookId:'A'} — a duplicate channel set that the internal and external v2 schemas both reject. The mirror case is a false rejection: channel:{type:'webhook',webhookId:'A'} with channels:[{type:'webhook',webhookId:'A',agentId:'X'}] is reported as a mismatch although the two are identical after narrowing. Previously an extra key was an unknown key and zod stripped it, so this hole is new. Reject the foreign id in the pairing loop (agentId on a webhook channel, webhookId on an agent channel), or run toAlertChannel over the channels before calling checkAlertChannelSelection.

if (!channel) continue;
if (channel.type === 'webhook' && !channel.webhookId) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — MCP channel shape accepts an agentId on a webhook channel, which defeats duplicate detection

alertChannelKey hashes the whole object (packages/common-utils/src/types.ts:789), so channels: [{type:'webhook',webhookId:'a'}, {type:'webhook',webhookId:'a',agentId:'x'}] passes the duplicate check, then toAlertChannel collapses both to the same webhook channel and the alert persists the same target twice — after which any read-modify-write (GET then PUT/save_alert with the returned channels) is rejected as Duplicate notification channels are not allowed. Extend the pairing loop to also reject the foreign id (agentId on a webhook channel, webhookId on an agent channel) instead of silently dropping it.

return 'webhookId is required for a webhook channel';
}
if (channel.type === 'agent' && !channel.agentId) {
return 'agentId is required for an agent channel';
}
}

// Source-specific required fields
if (data.source === 'tile') {
if (!data.dashboardId) {
Expand Down
6 changes: 6 additions & 0 deletions packages/api/src/models/alert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ export type AlertChannel =
type: 'webhook';
webhookId: string;
}
| {
// A firing alert starts an investigation session on the referenced
// managed agent (see models/managedAgent.ts) instead of POSTing a payload.
type: 'agent';
agentId: string;
}
| {
type: null;
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import mongoose from 'mongoose';

import {
getLoggedInAgent,
getServer,
makeAlertInput,
makeTile,
} from '@/fixtures';
import ManagedAgent from '@/models/managedAgent';

// The rest of the agent-channel suites run with the flag on (.env.test); this
// file mocks it off to pin the two flag-off behaviours: alert saves carrying an
// agent channel are rejected, and the managed-agents surface 404s.
jest.mock('@/config', () => ({
...jest.requireActual('@/config'),
IS_MANAGED_AGENTS_ENABLED: false,
}));

const MOCK_TILES = [makeTile()];
const MOCK_DASHBOARD = {
id: new mongoose.Types.ObjectId().toString(),
name: 'Test Dashboard',
tiles: MOCK_TILES,
tags: ['test'],
};

describe('managed agents disabled', () => {
const server = getServer();
let agent: Awaited<ReturnType<typeof getLoggedInAgent>>['agent'];
let team: Awaited<ReturnType<typeof getLoggedInAgent>>['team'];

beforeAll(async () => {
await server.start();
});

beforeEach(async () => {
const result = await getLoggedInAgent(server);
agent = result.agent;
team = result.team;
});

afterEach(async () => {
await server.clearDBs();
});

afterAll(async () => {
await server.stop();
});

it('rejects saving an alert with an agent channel', async () => {
const dashboard = await agent
.post('/dashboards')
.send(MOCK_DASHBOARD)
.expect(200);
const managedAgent = await ManagedAgent.create({
team: team._id,
name: 'SRE Responder',
model: 'claude-opus-4-8',
anthropicAgentId: 'agent_1',
vaultId: 'vlt_1',
environmentId: 'env_1',
mcpServerUrl: 'https://mcp.example.test/api/mcp',
});

const resp = await agent
.post('/alerts')
.send({
...makeAlertInput({
dashboardId: dashboard.body.id,
tileId: dashboard.body.tiles[0].id,
}),
channel: { type: 'agent', agentId: managedAgent._id.toString() },
})
.expect(400);
expect(resp.text).toContain('not enabled');
});

it('404s the managed-agents surface', async () => {
await agent.get('/managed-agents').expect(404);
});
});
Loading
Loading