From b4900ddaaf4e91ae3529b15f7af6a4e3653aed69 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:19:16 +0000 Subject: [PATCH 1/6] feat(events): Convert event automation identifiers to filters Each plugin can now turn its saved identifiers into the filters that select the same events (`identifierMatch` in plugin-api). Core uses it to save new and edited event automation triggers with no identifier, and the heartbeat converts saved rows one at a time through the locked, versioned save. A trigger converts only when every selected event gets the same filters, the filters agree with saved filters, and the result passes the catalog check. Otherwise the row keeps its identifier and keeps running, so no trigger gets broader. Slack channel automations now save a `channelId` filter for their destination channel. Sentry project identifiers do not convert for metric alert events, because those events can leave out `projects`. An omitted `match` on update now keeps the saved filters. Watches keep exact identifiers. The identifier column stays for slice D. Refs #2362 Co-Authored-By: David Cramer --- .../src/content/docs/concepts/automations.md | 2 + .../src/content/docs/extend/sentry-plugin.md | 6 + .../src/events/identifier-match.ts | 117 +++++++++++++++ packages/junior-github/src/plugin.ts | 2 + .../junior-github/tests/event-data.test.ts | 58 +------- .../src/events/identifier-match.ts | 32 ++++ packages/junior-linear/src/plugin.ts | 2 + packages/junior-linear/tests/webhook.test.ts | 19 +-- packages/junior-plugin-api/src/events.ts | 14 ++ .../src/events/identifier-match.ts | 65 +++++++++ packages/junior-sentry/src/index.ts | 2 + packages/junior-sentry/tests/webhook.test.ts | 37 ++--- .../src/events/identifier-match.ts | 43 ++++++ packages/junior-vercel/src/index.ts | 2 + packages/junior-vercel/tests/webhook.test.ts | 16 +- .../src/chat/agent-dispatch/heartbeat.ts | 10 ++ .../src/chat/event-automations/README.md | 30 +++- .../src/chat/event-automations/store.ts | 38 ++++- .../chat/event-automations/tool-support.ts | 12 +- .../event-automations/trigger-conversion.ts | 138 ++++++++++++++++++ packages/junior/src/chat/events/README.md | 15 +- packages/junior/src/chat/events/catalog.ts | 2 +- .../src/chat/slack/channel-message-events.ts | 39 +++-- .../src/chat/tools/update-event-automation.ts | 8 +- .../any-resource-event-automations.test.ts | 135 +++++++++++++++-- .../integration/api/automation-edits.test.ts | 15 +- .../slack/channel-message-automations.test.ts | 44 +++++- 27 files changed, 756 insertions(+), 147 deletions(-) create mode 100644 packages/junior-github/src/events/identifier-match.ts create mode 100644 packages/junior-linear/src/events/identifier-match.ts create mode 100644 packages/junior-sentry/src/events/identifier-match.ts create mode 100644 packages/junior-vercel/src/events/identifier-match.ts create mode 100644 packages/junior/src/chat/event-automations/trigger-conversion.ts diff --git a/packages/docs/src/content/docs/concepts/automations.md b/packages/docs/src/content/docs/concepts/automations.md index 919bced9a2..d3deb40463 100644 --- a/packages/docs/src/content/docs/concepts/automations.md +++ b/packages/docs/src/content/docs/concepts/automations.md @@ -61,6 +61,8 @@ whenever a pull request opens in getsentry/junior, review it Each plugin decides which events can run this way, and each plugin page lists them. Some events need a filter. For example, GitHub issue, pull request, and release events need a `repo` filter, and Linear issue events need a `teamKey` or `issueIdentifier` filter. Junior names the missing filter when it cannot save the automation. Junior saves filter values in the case that event data uses, so `getsentry/Junior` matches `getsentry/junior`. One event runs the automation once, even when the plugin publishes the event for more than one resource, such as each project of a metric alert. Slack channel-message automations always need their own channel. +When you name one exact resource, Junior saves it as the filters that select the same events. For example, one pull request becomes `repo` and `pullRequest` filters. Junior changes older automations the same way, and each change saves a new version. An automation keeps its exact resource when filters would miss events or match more events. When you update a trigger, leave out `match` to keep the saved filters. A new `match` replaces every saved filter. + A required filter limits how much an automation covers and how noisy it is. It is not access control. Junior does not check that the person who creates the automation can see the repository, project, or team in the filter. An automation with no exact resource posts what it receives to its destination channel. For example, a broad Sentry trigger or GitHub deployment trigger can post private project or repository names in that channel. Create broad automations only in channels where everyone can see those resources. A Slack channel-message automation runs for each new top-level message in the channel where you create it. It does not run for thread replies or for messages that mention Junior. It runs for a bot message, such as a Sentry alert, only when the automation names that bot's Slack user ID in `match.userId`. Junior replies in the thread of the message. Junior must be a member of the channel. The automation can watch only its own channel, and it always runs with Junior's own credentials, because anyone in the channel can write the message. Tell the channel that Junior reads every new message there. diff --git a/packages/docs/src/content/docs/extend/sentry-plugin.md b/packages/docs/src/content/docs/extend/sentry-plugin.md index b520d4df24..0854c4d02d 100644 --- a/packages/docs/src/content/docs/extend/sentry-plugin.md +++ b/packages/docs/src/content/docs/extend/sentry-plugin.md @@ -238,6 +238,12 @@ lowercase. Junior does not check that the creator can see the projects, so a broad automation can post private project names in its channel. See [Automations](/concepts/automations/). +An event automation for one issue, one metric alert, or the issues of one +project is saved as filters (`issueId`, `alertRuleId`, or `projects`). An +automation for the metric alerts of one project keeps its project identifier. +When an alert rule has too many projects to fit in event data, the event +leaves out `projects`, so a `projects` filter would miss that alert. + Create the watch or event automation before the webhook arrives. Junior does not replay earlier webhooks. ## Verify diff --git a/packages/junior-github/src/events/identifier-match.ts b/packages/junior-github/src/events/identifier-match.ts new file mode 100644 index 0000000000..8e9e837bb6 --- /dev/null +++ b/packages/junior-github/src/events/identifier-match.ts @@ -0,0 +1,117 @@ +import type { EventMatch } from "@sentry/junior-plugin-api"; +import { gitHubDeploymentSourceResource } from "./deployment.js"; +import { gitHubIssueResource } from "./issue.js"; +import { gitHubPullRequestResource } from "./pull-request.js"; +import { gitHubReleaseSourceResource } from "./release.js"; +import { gitHubRepositoryResource } from "./repository.js"; + +const REPO = String.raw`[^\s/:#]+\/[^\s/:#]+`; +const DEPLOYMENT_SOURCE = new RegExp( + String.raw`^deployment-source:(${REPO})(?::([^\s:]+))?:([0-9a-f]{40})$`, +); +const RELEASE_SOURCE = new RegExp( + String.raw`^release-source:(${REPO})(?::([^\s:]+))?$`, +); +const NUMBERED = new RegExp(String.raw`^(${REPO})#(\d+)$`); +const REPOSITORY = new RegExp(String.raw`^${REPO}$`); + +function decoded(value: string): string | undefined { + try { + return decodeURIComponent(value); + } catch { + return undefined; + } +} + +/** Filters for one identifier, before the check that the plugin builds it. */ +function identifierFilters( + identifier: string, + eventType: string, +): { match: EventMatch; built: string } | undefined { + const deployment = DEPLOYMENT_SOURCE.exec(identifier); + if (deployment) { + if (!eventType.startsWith("deployment.")) return undefined; + const [, repo, encodedEnvironment, commitSha] = deployment; + const environment = + encodedEnvironment === undefined + ? undefined + : decoded(encodedEnvironment); + if (encodedEnvironment !== undefined && !environment) return undefined; + return { + match: { + repo: repo!, + commitSha: commitSha!, + ...(environment ? { environment } : undefined), + }, + built: gitHubDeploymentSourceResource({ + repo: repo!, + commitSha: commitSha!, + environment, + }).identifier, + }; + } + const release = RELEASE_SOURCE.exec(identifier); + if (release) { + if (eventType !== "release.published") return undefined; + const [, repo, encodedTag] = release; + const tag = encodedTag === undefined ? undefined : decoded(encodedTag); + if (encodedTag !== undefined && !tag) return undefined; + return { + match: { repo: repo!, ...(tag ? { tag } : undefined) }, + built: gitHubReleaseSourceResource({ repo: repo!, tag }).identifier, + }; + } + const numbered = NUMBERED.exec(identifier); + if (numbered) { + const repo = numbered[1]!; + const number = Number(numbered[2]); + if (eventType.startsWith("issue.")) { + return { + match: { repo, issue: number }, + built: gitHubIssueResource({ repo, number }).identifier, + }; + } + if (eventType.startsWith("pull_request.")) { + return { + match: { repo, pullRequest: number }, + built: gitHubPullRequestResource({ repo, number }).identifier, + }; + } + return undefined; + } + if ( + REPOSITORY.test(identifier) && + (eventType.startsWith("deployment.") || + eventType.startsWith("issue.") || + eventType.startsWith("pull_request.")) + ) { + return { + match: { repo: identifier }, + built: gitHubRepositoryResource({ repo: identifier }).identifier, + }; + } + return undefined; +} + +/** + * Turn a saved GitHub identifier into the filters that select the same + * events of one type. Event data has the same lowercase values as + * identifiers. Environment and tag parts are URI-encoded in identifiers, so + * they are decoded. `owner/repo#n` names an issue on issue events and a pull + * request on the others. + * + * The filters must rebuild the same identifier with the builder that the + * webhooks use. Otherwise the identifier never got events, such as a tag that + * was not encoded, and filters could match events that it never got. + */ +export function gitHubIdentifierMatch(input: { + identifier: string; + eventType: string; +}): EventMatch | undefined { + const filters = identifierFilters(input.identifier, input.eventType); + // Saved identifiers went through the plugin's `normalizeIdentifier`. + if (!filters || filters.built.toLowerCase() !== input.identifier) { + return undefined; + } + return filters.match; +} diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index 6f486dc36c..9b53a46378 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -17,6 +17,7 @@ import { import { createGitHubTools } from "./tools.js"; import { createGitHubWebhookRoute } from "./webhooks/handler.js"; import { gitHubEventDeclarations } from "./events/declarations.js"; +import { gitHubIdentifierMatch } from "./events/identifier-match.js"; import { loadPullRequestCommitPages, loadRepositoryRole, @@ -203,6 +204,7 @@ export function githubPlugin( ), isEnabled: () => Boolean(readEnv("GITHUB_WEBHOOK_SECRET")), normalizeIdentifier: (identifier) => identifier.toLowerCase(), + identifierMatch: gitHubIdentifierMatch, }, manifest: { name: "github", diff --git a/packages/junior-github/tests/event-data.test.ts b/packages/junior-github/tests/event-data.test.ts index f140ebcaea..5cb5c4a355 100644 --- a/packages/junior-github/tests/event-data.test.ts +++ b/packages/junior-github/tests/event-data.test.ts @@ -1,8 +1,4 @@ -import { - eventMatches, - type EventInput, - type EventMatch, -} from "@sentry/junior-plugin-api"; +import { eventMatches, type EventInput } from "@sentry/junior-plugin-api"; import { filterEventMismatches, identifierMatchMismatches, @@ -257,51 +253,6 @@ async function publish( return events; } -/** - * Turn a saved GitHub identifier into filters, the way a saved trigger is - * converted. Saved identifiers are lowercase. The environment and tag parts are - * URI-encoded, so conversion decodes them. `owner/repo#n` names an issue on - * issue events and a pull request on the others. - */ -function gitHubIdentifierMatch(event: { - eventType: string; - identifier: string; -}): EventMatch | undefined { - const { eventType } = event; - // Saved identifiers went through the plugin's `normalizeIdentifier`. - const identifier = event.identifier.toLowerCase(); - const deployment = - /^deployment-source:([^:]+\/[^:]+)(?::([^:]+))?:([0-9a-f]{40})$/.exec( - identifier, - ); - if (deployment) { - const [, repo, environment, commitSha] = deployment; - return { - repo: repo!, - commitSha: commitSha!, - ...(environment - ? { environment: decodeURIComponent(environment) } - : undefined), - }; - } - const release = /^release-source:([^:]+\/[^:]+)(?::(.+))?$/.exec(identifier); - if (release) { - const [, repo, tag] = release; - return { - repo: repo!, - ...(tag ? { tag: decodeURIComponent(tag) } : undefined), - }; - } - const numbered = /^([^#]+\/[^#]+)#(\d+)$/.exec(identifier); - if (numbered) { - const key = eventType.startsWith("issue.") ? "issue" : "pullRequest"; - return { repo: numbered[1]!, [key]: Number(numbered[2]) }; - } - return /^[^/:#]+\/[^/:#]+$/.test(identifier) - ? { repo: identifier } - : undefined; -} - describe("GitHub event data", () => { it("declares filter events that match the data the webhooks set", async () => { const events = await publish(fixtureDeliveries(BASE_SCOPE), "delivery"); @@ -373,7 +324,12 @@ describe("GitHub event data", () => { resourceTypes: githubPlugin().events?.resourceTypes ?? [], events, matches: eventMatches, - toMatch: gitHubIdentifierMatch, + // Saved identifiers went through the plugin's `normalizeIdentifier`. + toMatch: ({ eventType, identifier }) => + githubPlugin().events?.identifierMatch?.({ + eventType, + identifier: identifier.toLowerCase(), + }), }), ).toEqual([]); // Tags keep their text: no URI encoding, and only ASCII letters change. diff --git a/packages/junior-linear/src/events/identifier-match.ts b/packages/junior-linear/src/events/identifier-match.ts new file mode 100644 index 0000000000..1b781752f2 --- /dev/null +++ b/packages/junior-linear/src/events/identifier-match.ts @@ -0,0 +1,32 @@ +import type { EventMatch } from "@sentry/junior-plugin-api"; +import { + LINEAR_ISSUE_EVENTS, + linearIssueResource, + linearTeamResource, +} from "./issue.js"; + +/** + * Turn a saved Linear identifier into the filters that select the same issue + * events. Issue and team identifiers share one form, so the shape decides: + * an issue identifier is `TEAM-123`, and a team key has no `-`. + */ +export function linearIdentifierMatch(input: { + identifier: string; + eventType: string; +}): EventMatch | undefined { + if (!(LINEAR_ISSUE_EVENTS as readonly string[]).includes(input.eventType)) + return undefined; + const { identifier } = input; + // Saved identifiers went through the plugin's `normalizeIdentifier`. + if (/^[^\s-]+-\d+$/.test(identifier)) { + return linearIssueResource({ identifier }).identifier === identifier + ? { issueIdentifier: identifier } + : undefined; + } + if (/^[^\s-]+$/.test(identifier)) { + return linearTeamResource({ teamKey: identifier }).identifier === identifier + ? { teamKey: identifier } + : undefined; + } + return undefined; +} diff --git a/packages/junior-linear/src/plugin.ts b/packages/junior-linear/src/plugin.ts index 716d7f5165..2bb5f8c2b6 100644 --- a/packages/junior-linear/src/plugin.ts +++ b/packages/junior-linear/src/plugin.ts @@ -9,6 +9,7 @@ import { LINEAR_ISSUE_EVENTS, LINEAR_ISSUE_MATCH_FIELDS, } from "./events/issue.js"; +import { linearIdentifierMatch } from "./events/identifier-match.js"; import { createLinearWebhookRoute } from "./webhooks/handler.js"; import { linearWebhookSecret } from "./webhooks/secret.js"; @@ -193,6 +194,7 @@ export function linearPlugin(): PluginRegistration { ), isEnabled: () => Boolean(linearWebhookSecret()), normalizeIdentifier: (identifier) => identifier.toUpperCase(), + identifierMatch: linearIdentifierMatch, }, manifest: { // Optional shared read account. It has no effect until an admin diff --git a/packages/junior-linear/tests/webhook.test.ts b/packages/junior-linear/tests/webhook.test.ts index bca1b120c2..96c9c5f199 100644 --- a/packages/junior-linear/tests/webhook.test.ts +++ b/packages/junior-linear/tests/webhook.test.ts @@ -1,9 +1,5 @@ import { createHmac } from "node:crypto"; -import { - eventMatches, - type EventInput, - type EventMatch, -} from "@sentry/junior-plugin-api"; +import { eventMatches, type EventInput } from "@sentry/junior-plugin-api"; import { filterEventMismatches, identifierMatchMismatches, @@ -117,13 +113,12 @@ describe("Linear webhook events", () => { body.data.team.key = teamKey!; return normalizeLinearEvents({ body, linearEvent: "Issue" }); }); - const toMatch = (event: { identifier: string }): EventMatch => { - // Saved identifiers went through the plugin's `normalizeIdentifier`. - const identifier = event.identifier.toUpperCase(); - return /-\d+$/.test(identifier) - ? { issueIdentifier: identifier } - : { teamKey: identifier }; - }; + // Saved identifiers went through the plugin's `normalizeIdentifier`. + const toMatch = (event: { eventType: string; identifier: string }) => + linearPlugin().events?.identifierMatch?.({ + eventType: event.eventType, + identifier: event.identifier.toUpperCase(), + }); expect( identifierMatchMismatches({ diff --git a/packages/junior-plugin-api/src/events.ts b/packages/junior-plugin-api/src/events.ts index c1e6d92070..6dc20b8315 100644 --- a/packages/junior-plugin-api/src/events.ts +++ b/packages/junior-plugin-api/src/events.ts @@ -430,6 +430,20 @@ export const pluginEventsSchema = z normalizeIdentifier: z .function({ input: [z.string()], output: z.string() }) .optional(), + /** + * Turn a normalized identifier into the filters that select the same + * events of one type: every event of that type that goes to the + * identifier, and no other. Return nothing when no filters can do that, + * for example when the event can leave out a scope value or does not go + * to this kind of identifier. Core then keeps the identifier, so a + * trigger never gets broader. + */ + identifierMatch: z + .function({ + input: [z.object({ identifier: z.string(), eventType: z.string() })], + output: eventMatchSchema.optional(), + }) + .optional(), }) .strict() .superRefine((registration, context) => { diff --git a/packages/junior-sentry/src/events/identifier-match.ts b/packages/junior-sentry/src/events/identifier-match.ts new file mode 100644 index 0000000000..088ea7f1cb --- /dev/null +++ b/packages/junior-sentry/src/events/identifier-match.ts @@ -0,0 +1,65 @@ +import type { EventMatch } from "@sentry/junior-plugin-api"; +import { sentryIssueResource, sentryProjectResource } from "./issue.js"; +import { sentryMetricAlertResource } from "./metric-alert.js"; + +const SLUG = String.raw`[^\s/:#]+`; +const METRIC_ALERT = new RegExp(String.raw`^metric-alert:(${SLUG})\/(\d+)$`); +const PROJECT = new RegExp(String.raw`^(${SLUG})\/(${SLUG})(?:#(\d+))?$`); + +/** Filters for one identifier, with the identifier that the plugin builds from them. */ +function identifierFilters( + identifier: string, + eventType: string, +): { match: EventMatch; built: string } | undefined { + const alert = METRIC_ALERT.exec(identifier); + if (alert) { + if (!eventType.startsWith("metric_alert.")) return undefined; + const [, org, alertRuleId] = alert; + return { + match: { alertRuleId: alertRuleId! }, + built: sentryMetricAlertResource({ org: org!, alertRuleId: alertRuleId! }) + .identifier, + }; + } + const scope = PROJECT.exec(identifier); + // Metric alert events leave out `projects` when the rule has too many + // projects to fit in event data. A project identifier still gets those + // events, so a `projects` filter would miss them. + if (!scope || eventType !== "issue.created") return undefined; + const [, org, project, issueId] = scope; + return issueId === undefined + ? { + match: { projects: project! }, + built: sentryProjectResource({ org: org!, project: project! }) + .identifier, + } + : { + match: { projects: project!, issueId }, + built: sentryIssueResource({ + org: org!, + project: project!, + issueId, + }).identifier, + }; +} + +/** + * Turn a saved Sentry identifier into the filters that select the same + * events of one type. One install is one Sentry org, so the org part of an + * identifier needs no filter. A project identifier converts only for issue + * events, because metric alert events can leave out `projects`. + * + * The filters must rebuild the same identifier with the builder that the + * webhooks use, so a malformed identifier keeps its identifier. + */ +export function sentryIdentifierMatch(input: { + identifier: string; + eventType: string; +}): EventMatch | undefined { + const filters = identifierFilters(input.identifier, input.eventType); + // Saved identifiers went through the plugin's `normalizeIdentifier`. + if (!filters || filters.built.toLowerCase() !== input.identifier) { + return undefined; + } + return filters.match; +} diff --git a/packages/junior-sentry/src/index.ts b/packages/junior-sentry/src/index.ts index d1e32ee6b3..a966420197 100644 --- a/packages/junior-sentry/src/index.ts +++ b/packages/junior-sentry/src/index.ts @@ -20,6 +20,7 @@ import { SENTRY_METRIC_ALERT_MATCH_FIELDS, SENTRY_METRIC_ALERT_SUGGESTED_EVENTS, } from "./events/metric-alert.js"; +import { sentryIdentifierMatch } from "./events/identifier-match.js"; import { SENTRY_API_DOMAINS, issueSentryCredential, @@ -74,6 +75,7 @@ export function sentryPlugin(): PluginRegistration { ), isEnabled: () => Boolean(sentryWebhookSecret()), normalizeIdentifier: (identifier) => identifier.toLowerCase(), + identifierMatch: sentryIdentifierMatch, }, manifest: { commandEnv: { diff --git a/packages/junior-sentry/tests/webhook.test.ts b/packages/junior-sentry/tests/webhook.test.ts index 8d7c2fa8eb..aec15d487c 100644 --- a/packages/junior-sentry/tests/webhook.test.ts +++ b/packages/junior-sentry/tests/webhook.test.ts @@ -1,9 +1,5 @@ import { createHmac } from "node:crypto"; -import { - eventMatches, - type EventInput, - type EventMatch, -} from "@sentry/junior-plugin-api"; +import { eventMatches, type EventInput } from "@sentry/junior-plugin-api"; import { filterEventMismatches, identifierMatchMismatches, @@ -166,22 +162,29 @@ describe("Sentry webhook events", () => { normalizeSentryEvents(delivery), ); - // The org part is dropped: the install already fixes the org. - const toMatch = (event: { identifier: string }): EventMatch | undefined => { - // Saved identifiers went through the plugin's `normalizeIdentifier`. - const identifier = event.identifier.toLowerCase(); - const alert = /^metric-alert:[^/]+\/(\d+)$/.exec(identifier); - if (alert) return { alertRuleId: alert[1]! }; - const scope = /^[^/]+\/([^/#]+)(?:#(\d+))?$/.exec(identifier); - if (!scope) return undefined; - const [, project, issueId] = scope; - return { projects: [project!], ...(issueId ? { issueId } : undefined) }; - }; + // The org part is dropped: the install already fixes the org. Saved + // identifiers went through the plugin's `normalizeIdentifier`. + const toMatch = (event: { eventType: string; identifier: string }) => + sentryPlugin().events?.identifierMatch?.({ + eventType: event.eventType, + identifier: event.identifier.toLowerCase(), + }); + // A metric alert can leave out `projects`, so its project copies keep + // their identifier. + const projectAlerts = events.filter( + (event) => + event.eventType.startsWith("metric_alert.") && + !event.identifier.startsWith("metric-alert:"), + ); + expect(projectAlerts.length).toBeGreaterThan(0); + expect(projectAlerts.map(toMatch)).toEqual( + projectAlerts.map(() => undefined), + ); expect( identifierMatchMismatches({ resourceTypes: sentryPlugin().events?.resourceTypes ?? [], - events, + events: events.filter((event) => !projectAlerts.includes(event)), matches: eventMatches, toMatch, }), diff --git a/packages/junior-vercel/src/events/identifier-match.ts b/packages/junior-vercel/src/events/identifier-match.ts new file mode 100644 index 0000000000..3bd3f1cee3 --- /dev/null +++ b/packages/junior-vercel/src/events/identifier-match.ts @@ -0,0 +1,43 @@ +import type { EventMatch } from "@sentry/junior-plugin-api"; +import { + VERCEL_DEPLOYMENT_EVENTS, + VERCEL_DEPLOYMENT_TARGETS, + vercelDeploymentResource, + type VercelDeploymentTarget, +} from "./deployment.js"; + +function isTarget(value: string): value is VercelDeploymentTarget { + return (VERCEL_DEPLOYMENT_TARGETS as readonly string[]).includes(value); +} + +/** + * Turn a saved Vercel identifier into the filters that select the same + * deployment events: `projectId`, then `target`, then `commitSha`, one for + * each part of `projectId[:target[:commitSha]]`. + * + * The filters must rebuild the same identifier with the builder that the + * webhooks use. Otherwise the identifier never got events, and filters could + * match events that it never got. + */ +export function vercelIdentifierMatch(input: { + identifier: string; + eventType: string; +}): EventMatch | undefined { + if ( + !(VERCEL_DEPLOYMENT_EVENTS as readonly string[]).includes(input.eventType) + ) + return undefined; + const [projectId, target, commitSha, ...rest] = input.identifier.split(":"); + if (!projectId || rest.length > 0) return undefined; + if (target !== undefined && !isTarget(target)) return undefined; + if (commitSha !== undefined && !/^[0-9a-f]{40}$/.test(commitSha)) { + return undefined; + } + const built = vercelDeploymentResource({ projectId, target, commitSha }); + if (built.identifier !== input.identifier) return undefined; + return { + projectId, + ...(target ? { target } : undefined), + ...(commitSha ? { commitSha } : undefined), + }; +} diff --git a/packages/junior-vercel/src/index.ts b/packages/junior-vercel/src/index.ts index ccc6b5f43d..2d44a3e953 100644 --- a/packages/junior-vercel/src/index.ts +++ b/packages/junior-vercel/src/index.ts @@ -17,6 +17,7 @@ import { VERCEL_DEPLOYMENT_SUGGESTED_EVENTS, } from "./events/deployment.js"; import { createVercelWebhookRoute } from "./webhooks/handler.js"; +import { vercelIdentifierMatch } from "./events/identifier-match.js"; import { vercelWebhookSecret } from "./webhooks/secret.js"; /** Register Vercel runtime metadata, tools, and signed webhook ingress. */ @@ -42,6 +43,7 @@ export function vercelPlugin(): PluginRegistration { ]), ), isEnabled: () => Boolean(vercelWebhookSecret()), + identifierMatch: vercelIdentifierMatch, }, manifest: { apiHeaders: { diff --git a/packages/junior-vercel/tests/webhook.test.ts b/packages/junior-vercel/tests/webhook.test.ts index 223548d67b..1b92dec760 100644 --- a/packages/junior-vercel/tests/webhook.test.ts +++ b/packages/junior-vercel/tests/webhook.test.ts @@ -1,9 +1,5 @@ import { createHmac } from "node:crypto"; -import { - eventMatches, - type EventInput, - type EventMatch, -} from "@sentry/junior-plugin-api"; +import { eventMatches, type EventInput } from "@sentry/junior-plugin-api"; import { filterEventMismatches, identifierMatchMismatches, @@ -115,14 +111,8 @@ describe("Vercel webhook events", () => { return normalizeVercelEvents({ body }); }), ); - const toMatch = ({ identifier }: { identifier: string }): EventMatch => { - const [projectId, target, commitSha] = identifier.split(":"); - return { - projectId: projectId!, - ...(target ? { target } : undefined), - ...(commitSha ? { commitSha } : undefined), - }; - }; + const toMatch = vercelPlugin().events?.identifierMatch; + if (!toMatch) throw new Error("Vercel identifiers must convert to filters"); expect( identifierMatchMismatches({ diff --git a/packages/junior/src/chat/agent-dispatch/heartbeat.ts b/packages/junior/src/chat/agent-dispatch/heartbeat.ts index 7e2110cbaf..12308953d9 100644 --- a/packages/junior/src/chat/agent-dispatch/heartbeat.ts +++ b/packages/junior/src/chat/agent-dispatch/heartbeat.ts @@ -1,3 +1,6 @@ +import { getDb } from "@/chat/db"; +import { convertSavedEventAutomationTriggers } from "@/chat/event-automations/trigger-conversion"; +import { getEventCatalog } from "@/chat/events/runtime-catalog"; import { runTimerWatchHeartbeat } from "@/chat/events/timers"; import { getPlugins } from "@/chat/plugins/agent-hooks"; import { logException, logInfo } from "@/chat/logging"; @@ -215,6 +218,13 @@ export async function runHeartbeat(args: { } catch (error) { logException(error, "scheduled_automations.heartbeat.failed"); } + try { + // Saved triggers with an identifier move to filters. Slice D of #2362 + // removes this step with the identifier column. + await convertSavedEventAutomationTriggers(getDb(), getEventCatalog()); + } catch (error) { + logException(error, "event_automation.trigger.conversion_failed"); + } try { await runTimerWatchHeartbeat({ nowMs: args.nowMs, queue }); } catch (error) { diff --git a/packages/junior/src/chat/event-automations/README.md b/packages/junior/src/chat/event-automations/README.md index b2913ba7e4..e7df110ee3 100644 --- a/packages/junior/src/chat/event-automations/README.md +++ b/packages/junior/src/chat/event-automations/README.md @@ -11,7 +11,9 @@ route handles. Thread replies and Slackbot posts do not publish. Other bot posts publish with `postedByBot`, but they run only automations whose `match` names the bot in `userId`. Bots can post often and would use up the run limit of every other automation in the channel. A Watch cannot select these events. An automation can watch only the channel where it posts, so -private channel text stays in that channel. Runs always use system credentials, +private channel text stays in that channel. Its trigger has no identifier +and a `channelId` filter for that channel. Ingest checks the channel of the +Event against the destination again. Runs always use system credentials, because anyone in the channel writes the Event text. Each run replies in the thread of the source message. @@ -34,8 +36,30 @@ identifier must have the same identifier in its row. A row with no identifier must have a trigger that the enabled catalog accepts, so a plugin change that narrows an event also stops old broad rows. Core logs and skips a row that does not parse or that the gate rejects, so one bad row cannot stop ingest for its -namespace. The update tool keeps the saved identifier and resource type when -they are left out. Only an explicit `null` removes them. +namespace. The update tool keeps the saved identifier, resource type, and +filters when they are left out. Only an explicit `null` removes the +identifier, and only an empty `match` removes the filters. + +## Triggers with an identifier + +Tools and the dashboard save a trigger with an identifier as a trigger with no +identifier when its plugin can turn the identifier into filters +(`identifierMatch`). For example, `getsentry/junior#7` on pull request events +becomes `repo` and `pullRequest` filters. `trigger-conversion.ts` owns the +rule. A trigger converts only when the plugin gives the same filters for every +selected event, those filters agree with the saved filters, and the result +passes the check for a new trigger. Otherwise it keeps its identifier and +keeps running. A conversion never makes a trigger broader. + +Each heartbeat converts saved rows that still have an identifier. It saves +each row with the revision that it read, under the row lock, and records a +version with no editor. A row that changed after the read is left for the next +heartbeat. Rows that cannot convert keep their identifier. Examples are a +Sentry project trigger for metric alerts, because a metric alert can leave +out `projects`, and a row whose plugin is not enabled. Slice D of #2362 +decides what happens to these rows before the identifier column goes away. +Saved versions keep their old triggers, and the version schema still reads +`identifier` and `resourceType`. `searchEventTypes` exposes the same enabled plugin catalog used by watches and event automations. Create and update tools accept only diff --git a/packages/junior/src/chat/event-automations/store.ts b/packages/junior/src/chat/event-automations/store.ts index 80aba23238..ae120ecd28 100644 --- a/packages/junior/src/chat/event-automations/store.ts +++ b/packages/junior/src/chat/event-automations/store.ts @@ -7,7 +7,18 @@ import { type EventMatch, type User, } from "@sentry/junior-plugin-api"; -import { and, asc, desc, eq, inArray, isNull, ne, or, sql } from "drizzle-orm"; +import { + and, + asc, + desc, + eq, + inArray, + isNotNull, + isNull, + ne, + or, + sql, +} from "drizzle-orm"; import type { JuniorDatabase } from "@/db/db"; import { juniorAutomations } from "@/db/schema/automations"; import { @@ -284,6 +295,31 @@ export async function listDeletedEventAutomationsCreatedBy( return rows.map(parseEventAutomationRow); } +/** + * List live event automations whose trigger still has an identifier. Skip a + * row that does not parse; ingest logs it when an event reaches it. + */ +export async function listEventAutomationsWithIdentifier( + db: JuniorDatabase, +): Promise { + const rows = await db + .select() + .from(juniorAutomations) + .where( + and( + activeEventAutomationWhere(), + isNotNull(juniorAutomations.identifier), + ), + ) + .orderBy(asc(juniorAutomations.createdAtMs), asc(juniorAutomations.id)); + return rows.flatMap((row) => { + const parsed = eventAutomationSchema.safeParse( + eventAutomationRowInput(row), + ); + return parsed.success ? [withRowFields(row, parsed.data)] : []; + }); +} + /** * Select live rows whose identifier is one of these, and rows with no * identifier. A row with no identifier runs for every resource of its diff --git a/packages/junior/src/chat/event-automations/tool-support.ts b/packages/junior/src/chat/event-automations/tool-support.ts index 6caca2fb6e..809e1bb1e8 100644 --- a/packages/junior/src/chat/event-automations/tool-support.ts +++ b/packages/junior/src/chat/event-automations/tool-support.ts @@ -28,6 +28,7 @@ import { requireValidEventTrigger, type EventCatalog, } from "@/chat/events/catalog"; +import { triggerWithoutIdentifier } from "@/chat/event-automations/trigger-conversion"; import { juniorToolOutputSchema } from "@/chat/tool-support/structured-result"; import { ToolInputError } from "@/chat/tools/execution/tool-input-error"; import type { ToolRuntimeContext } from "@/chat/tools/types"; @@ -108,7 +109,7 @@ export function registeredEventAutomationTriggerSchema(catalog: EventCatalog) { .max(EVENT_AUTOMATION_IDENTIFIER_MAX_LENGTH) .nullable() .describe( - 'Exact resource identifier. Use null, with a null resourceType, to run for every resource of the namespace. Only events whose searchEventTypes reach is "all" or { anyOf } can run that way, and match must set one of the anyOf filters. When creating, omit it to run for every resource. When updating, omit it to keep the saved identifier.', + 'Exact resource identifier. Use null, with a null resourceType, to run for every resource of the namespace. Only events whose searchEventTypes reach is "all" or { anyOf } can run that way, and match must set one of the anyOf filters. When creating, omit it to run for every resource. When updating, omit it to keep the saved identifier. When the plugin can, the identifier is saved as filters that select the same events, so the result shows identifier null and those filters.', ) .optional(), resourceType: registeredResourceTypeSchema(plugins) @@ -127,8 +128,9 @@ export function registeredEventAutomationTriggerSchema(catalog: EventCatalog) { /** * Check a new event-automation trigger with the enabled plugins and return * it as it is saved. It names a resource type and an identifier, or neither. - * The identifier gets its plugin's convention. Events and list values lose - * repeats. + * The identifier gets its plugin's convention. A trigger with an identifier is + * saved with no identifier when its plugin can turn the identifier into + * filters that select the same events. Events and list values lose repeats. */ export function requireEventAutomationTrigger( catalog: EventCatalog, @@ -154,7 +156,7 @@ export function requireEventAutomationTrigger( { namespace: input.namespace, resourceType, events, match: input.match }, "event_automation", ); - return { + const trigger: EventAutomation["trigger"] = { namespace: input.namespace, ...(identifier !== undefined && resourceType !== undefined ? { @@ -170,6 +172,8 @@ export function requireEventAutomationTrigger( events, ...(match ? { match } : undefined), }; + const converted = triggerWithoutIdentifier(catalog, trigger); + return "trigger" in converted ? converted.trigger : trigger; } /** Require the active Slack authority used for event-automation management. */ diff --git a/packages/junior/src/chat/event-automations/trigger-conversion.ts b/packages/junior/src/chat/event-automations/trigger-conversion.ts new file mode 100644 index 0000000000..4b5509fc7f --- /dev/null +++ b/packages/junior/src/chat/event-automations/trigger-conversion.ts @@ -0,0 +1,138 @@ +/** + * Turn event automation triggers with an identifier into triggers with no + * identifier that select the same events. + * + * Plugins own their identifier formats through `identifierMatch`. Core owns + * the rule: a trigger converts only when its plugin gives one set of filters + * for every selected event, the filters agree with the saved filters, and the + * result passes the same check as a new trigger. Otherwise the trigger keeps + * its identifier, so it never gets broader. + */ +import { + stableEventMatchKey, + type EventMatch, +} from "@sentry/junior-plugin-api"; +import { + AutomationConflictError, + automationRevision, +} from "@/chat/automations/revision"; +import { + automationEventCatalog, + registeredEventTriggerProblems, + type EventCatalog, +} from "@/chat/events/catalog"; +import { logException, logInfo } from "@/chat/logging"; +import type { JuniorDatabase } from "@/db/db"; +import { + listEventAutomationsWithIdentifier, + saveEventAutomation, +} from "./store"; +import type { EventAutomation } from "./types"; + +type EventAutomationTrigger = EventAutomation["trigger"]; + +/** A trigger with no identifier, or why the trigger keeps its identifier. */ +export type TriggerConversion = + | { trigger: EventAutomationTrigger } + | { reason: string }; + +function sameValue(left: EventMatch[string], right: EventMatch[string]) { + return ( + stableEventMatchKey({ value: left }) === + stableEventMatchKey({ value: right }) + ); +} + +/** + * Return the trigger with no identifier that selects the same events as this + * trigger, or why it cannot. A trigger with no identifier stays as it is. + */ +export function triggerWithoutIdentifier( + catalog: EventCatalog, + trigger: EventAutomationTrigger, +): TriggerConversion { + const { identifier, resourceType: _resourceType, ...rest } = trigger; + if (identifier === undefined) return { trigger }; + const plugins = automationEventCatalog(catalog); + const toMatch = plugins[trigger.namespace]?.identifierMatch; + if (!toMatch) { + return { + reason: `The ${trigger.namespace} source cannot turn identifiers into filters.`, + }; + } + let scope: EventMatch | undefined; + for (const eventType of trigger.events) { + const match = toMatch({ identifier, eventType }); + if (!match) { + return { + reason: `Identifier "${identifier}" cannot become filters for "${eventType}" without matching other events.`, + }; + } + if (scope && stableEventMatchKey(scope) !== stableEventMatchKey(match)) { + return { + reason: `Identifier "${identifier}" needs different filters for different events.`, + }; + } + scope = match; + } + for (const [key, value] of Object.entries(scope ?? {})) { + const saved = trigger.match?.[key]; + if (saved !== undefined && !sameValue(saved, value)) { + return { + reason: `Filter "${key}" already has a value that identifier "${identifier}" does not have.`, + }; + } + } + const match = { ...trigger.match, ...scope }; + const problems = registeredEventTriggerProblems(plugins, { + namespace: trigger.namespace, + events: trigger.events, + match, + }); + if (problems.length > 0) { + return { reason: problems.map((problem) => problem.message).join(" ") }; + } + return { trigger: { ...rest, match } }; +} + +/** + * Convert every saved event automation trigger that still has an identifier + * and that `triggerWithoutIdentifier` can convert. Each save locks the row, + * checks that the row did not change after it was read, and records a new + * version with no editor. A row that changed is left for the next run. Other + * rows keep their identifier and keep running. + */ +export async function convertSavedEventAutomationTriggers( + db: JuniorDatabase, + catalog: EventCatalog, +): Promise<{ converted: string[]; kept: { id: string; reason: string }[] }> { + const converted: string[] = []; + const kept: { id: string; reason: string }[] = []; + for (const task of await listEventAutomationsWithIdentifier(db)) { + try { + const conversion = triggerWithoutIdentifier(catalog, task.trigger); + if ("reason" in conversion) { + kept.push({ id: task.id, reason: conversion.reason }); + continue; + } + const saved = await saveEventAutomation( + db, + { ...task, trigger: conversion.trigger }, + automationRevision(task), + ); + if (!saved) continue; + converted.push(task.id); + logInfo("event_automation.trigger.converted", { + "app.task.id": task.id, + "app.task.type": "event", + }); + } catch (error) { + if (error instanceof AutomationConflictError) continue; + logException(error, "event_automation.trigger.conversion_failed", { + "app.task.id": task.id, + "app.task.type": "event", + }); + } + } + return { converted, kept }; +} diff --git a/packages/junior/src/chat/events/README.md b/packages/junior/src/chat/events/README.md index f68493dd15..3a2a2e3a83 100644 --- a/packages/junior/src/chat/events/README.md +++ b/packages/junior/src/chat/events/README.md @@ -121,6 +121,11 @@ Watches route events back into an existing conversation. automation trigger with no identifier can select only events with reach `"all"` or `{ anyOf }`, and must set one of the `anyOf` filters. Core merges the filters of every resource type once, when it builds the catalog. +- A source may declare `identifierMatch`. It turns one identifier and one + event type into the filters that select every event of that type that goes + to the identifier, and no other. It returns nothing when no filters can do + that. Core uses it to save event automation triggers with no identifier + (`event-automations/trigger-conversion.ts`). Watches keep their identifier. - A match field may declare `case: "lower"` or `case: "upper"`. Core changes a filter value to that case when it saves a trigger or a Watch, so a value such as `getsentry/Junior` still matches lowercase event data. The plugin decides @@ -182,10 +187,16 @@ Watches route events back into an existing conversation. filters that keep a trigger narrow when the event carries private text or makes Junior act. Use `"resource"` when a trigger must name one resource. Add `guidance` there when the agent needs a rule for this event. -7. When `data` needs a lookup, call `neededMatchKeys` with the values that the +7. Make `identifierMatch` return the filters for each identifier that gets + the event, and nothing for identifiers that do not get it. Build the + identifier again from the filters with the shared builder, and return + nothing when it differs. Use `identifierMatchMismatches` from + `@sentry/junior-testing` in the webhook test to prove that the filters + match the same events. +8. When `data` needs a lookup, call `neededMatchKeys` with the values that the plugin already has, such as `repo`. Core then skips triggers with no identifier whose filters on those values fail. -8. Extend the plugin's webhook test with a delivery for the new event. +9. Extend the plugin's webhook test with a delivery for the new event. ## Timer Watches diff --git a/packages/junior/src/chat/events/catalog.ts b/packages/junior/src/chat/events/catalog.ts index e9128fd20d..a7fddafaaa 100644 --- a/packages/junior/src/chat/events/catalog.ts +++ b/packages/junior/src/chat/events/catalog.ts @@ -38,7 +38,7 @@ export type EventResourceTypeLimits = { /** One source as a plugin or core registers it, before the catalog adds merged filters. */ export type EventRegistrationInput = Pick< PluginEvents, - "events" | "resourceTypes" | "normalizeIdentifier" + "events" | "resourceTypes" | "normalizeIdentifier" | "identifierMatch" > & { /** Name people see for the source, such as GitHub. Defaults to the namespace. */ displayName?: string; diff --git a/packages/junior/src/chat/slack/channel-message-events.ts b/packages/junior/src/chat/slack/channel-message-events.ts index f64b60b5d7..d50842e789 100644 --- a/packages/junior/src/chat/slack/channel-message-events.ts +++ b/packages/junior/src/chat/slack/channel-message-events.ts @@ -25,6 +25,11 @@ const MESSAGE_POSTED_EVENT = "message.posted"; const CHANNEL_ID_PATTERN = /^[CG][A-Z0-9]{2,}$/; const CHANNEL_MESSAGE_MATCH_FIELDS = eventMatchFieldsSchema.parse({ + channelId: { + kind: "string", + description: + "Slack channel ID of the message. A channel automation always names the channel where it posts.", + }, userId: { kind: "string", description: @@ -52,13 +57,18 @@ export function slackChannelMessageEvents(): EventRegistrationInput { ], events: { // A channel automation stays in its own channel, so it needs the - // channel identifier until a channel filter can replace it. + // channel filter. `requireSlackChannelMessageScope` sets it to the + // channel where the automation posts. [MESSAGE_POSTED_EVENT]: { - reach: "resource", + reach: { anyOf: ["channelId"] }, guidance: "A person or bot posted a new top-level message in this channel. A bot post runs the automation only when match names the bot in userId. A reply goes in that message's thread. If the instructions do not apply to this message, end with no action.", }, }, + identifierMatch: ({ identifier, eventType }) => + eventType === MESSAGE_POSTED_EVENT && CHANNEL_ID_PATTERN.test(identifier) + ? { channelId: identifier } + : undefined, limits: { [CHANNEL_RESOURCE_TYPE]: { watch: false, @@ -72,8 +82,10 @@ export function slackChannelMessageEvents(): EventRegistrationInput { /** * Bind a Slack channel-message automation to its own channel. * - * Resolve `current_channel` to the destination channel and reject other - * channels, so private channel text cannot reach another channel. + * The trigger names the channel with the identifier `current_channel`, the + * channel ID, or a `channelId` filter. Reject other channels, so private + * channel text cannot reach another channel. Save the trigger with no + * identifier and a `channelId` filter for the channel where it posts. */ export function requireSlackChannelMessageScope( automation: EventAutomation, @@ -85,11 +97,16 @@ export function requireSlackChannelMessageScope( "A Slack channel-message automation works only in a channel, not in a DM.", ); } - const identifier = - automation.trigger.identifier === SLACK_CURRENT_CHANNEL_IDENTIFIER - ? channelId - : automation.trigger.identifier; - if (identifier !== channelId) { + const { + identifier, + resourceType: _resourceType, + ...trigger + } = automation.trigger; + const named = [ + identifier === SLACK_CURRENT_CHANNEL_IDENTIFIER ? channelId : identifier, + trigger.match?.channelId, + ].filter((value) => value !== undefined); + if (named.length === 0 || named.some((value) => value !== channelId)) { throw new ToolInputError( `A Slack channel-message automation can watch only the channel where it posts. Use identifier "${SLACK_CURRENT_CHANNEL_IDENTIFIER}", or create the automation in the channel to watch.`, ); @@ -97,7 +114,7 @@ export function requireSlackChannelMessageScope( return { ...automation, credentialMode: "system", - trigger: { ...automation.trigger, identifier }, + trigger: { ...trigger, match: { ...trigger.match, channelId } }, }; } @@ -140,7 +157,7 @@ export function slackChannelMessageEvent(input: { * Decide whether a channel-message Event may run one automation. * * A message runs only automations that post in its own channel, so private - * channel text cannot reach another channel. The trigger identifier already + * channel text cannot reach another channel. The `channelId` filter already * names that channel, and this check keeps the rule when it does not. * A bot post runs only automations whose match names its author in `userId`. * Bots can post often and would use up the run limit of every other diff --git a/packages/junior/src/chat/tools/update-event-automation.ts b/packages/junior/src/chat/tools/update-event-automation.ts index 58c540e2f5..ece68ddaf3 100644 --- a/packages/junior/src/chat/tools/update-event-automation.ts +++ b/packages/junior/src/chat/tools/update-event-automation.ts @@ -42,7 +42,7 @@ export function createUpdateEventAutomationTool( }, executionMode: "sequential", description: - "Update the instruction, registered trigger, credential use, or status of an event automation.", + "Update the instruction, registered trigger, credential use, or status of an event automation. In trigger, an omitted identifier, resourceType, or match keeps the saved value. A match replaces every saved filter, so include saved filters such as repo, projects, or channelId that limit the trigger.", inputSchema: z .object({ automationId: z.string().min(1), @@ -139,11 +139,13 @@ export function createUpdateEventAutomationTool( { title: input.title, instruction: input.instruction ?? undefined, - // An omitted identifier or resource type keeps the saved one. - // Only an explicit null makes the trigger run for every resource. + // An omitted identifier, resource type, or match keeps the saved + // one, so a trigger cannot get broader by omission. Only an + // explicit null drops the identifier, and only `{}` drops filters. trigger: input.trigger ? { ...input.trigger, + match: input.trigger.match ?? current.trigger.match, identifier: input.trigger.identifier === undefined ? current.trigger.identifier diff --git a/packages/junior/tests/integration/any-resource-event-automations.test.ts b/packages/junior/tests/integration/any-resource-event-automations.test.ts index 1a479ef16c..2ead6a7731 100644 --- a/packages/junior/tests/integration/any-resource-event-automations.test.ts +++ b/packages/junior/tests/integration/any-resource-event-automations.test.ts @@ -1,4 +1,5 @@ import { githubPlugin } from "@sentry/junior-github"; +import type { EventMatch } from "@sentry/junior-plugin-api"; import { eq, sql } from "drizzle-orm"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createApp, defineJuniorPlugins } from "@/app"; @@ -6,6 +7,8 @@ import { getDispatchRecord } from "@/chat/agent-dispatch/store"; import { migrateSchema } from "@/chat/conversations/sql/migrations"; import { getConversationStore } from "@/chat/db"; import { ingestEventAutomations } from "@/chat/event-automations/ingest"; +import { convertSavedEventAutomationTriggers } from "@/chat/event-automations/trigger-conversion"; +import { listAutomationVersions } from "@/chat/automations/versions"; import { getEventCatalog } from "@/chat/events/runtime-catalog"; import { disconnectStateAdapter } from "@/chat/state/adapter"; import { createEventAutomationTool } from "@/chat/tools/create-event-automation"; @@ -201,8 +204,8 @@ describe("event automations with no identifier", () => { "About: New pull requests in getsentry/junior", ); - // An update that leaves out identifier and resourceType keeps them, - // so it cannot make a trigger broad by omission. + // A new trigger with an identifier is saved as the filters that select + // the same events, so the tools stop writing identifiers. const one = await create( { ...trigger, @@ -212,6 +215,15 @@ describe("event automations with no identifier", () => { }, "one-pull-request", ); + const onePullRequest = { + identifier: null, + resourceType: null, + match: { repo: "getsentry/junior", pullRequest: 7 }, + }; + expect(one.automation.trigger).toMatchObject(onePullRequest); + + // An update that leaves out identifier, resourceType, and match keeps + // them, so it cannot make a trigger broad by omission. const update = (next: Record, toolCallId: string) => execute( createUpdateEventAutomationTool(context(), catalog), @@ -221,27 +233,118 @@ describe("event automations with no identifier", () => { automation: { trigger: Record }; }>; const edited = { ...trigger, label: "Opened getsentry/junior#7" }; - await expect(update(edited, "keep-identifier")).resolves.toMatchObject({ - automation: { + await expect(update(edited, "keep-filters")).resolves.toMatchObject({ + automation: { trigger: onePullRequest }, + }); + // Only an explicit empty match drops filters, and then the trigger + // needs a repo. + await expect( + update({ ...edited, match: {} }, "drop-filters"), + ).rejects.toThrow('This trigger needs a "repo" filter (match.repo)'); + await expect( + update( + { ...edited, match: { repo: "getsentry/junior" } }, + "repo-filter", + ), + ).resolves.toMatchObject({ + automation: { trigger: { match: { repo: "getsentry/junior" } } }, + }); + + // Rows saved before this change still name one resource. The heartbeat + // step converts each one under its row lock and saves a version with + // no editor. A row that cannot convert without matching more events + // keeps its identifier and keeps running. + const savedPullRequest = (id: string, match?: EventMatch) => ({ + ...row!, + id, + identifier: "getsentry/junior#8", + record: { + ...record, trigger: { - identifier: "getsentry/junior#7", + ...record.trigger, + identifier: "getsentry/junior#8", resourceType: "pull_request", + match, }, }, }); - // Only explicit nulls drop them, and then the trigger needs a repo. - const broad = { ...edited, identifier: null, resourceType: null }; - await expect(update(broad, "drop-identifier")).rejects.toThrow( - 'This trigger needs a "repo" filter (match.repo)', + await fixture.sql + .db() + .insert(juniorAutomations) + .values([ + savedPullRequest("evt_saved_pull_request"), + savedPullRequest("evt_saved_list_filter", { pullRequest: [8, 9] }), + ]); + const conversion = await convertSavedEventAutomationTriggers( + fixture.sql.db(), + catalog, ); - await expect( - update( - { ...broad, match: { repo: "getsentry/junior" } }, - "drop-identifier-with-repo", + expect(conversion.converted).toEqual(["evt_saved_pull_request"]); + expect(conversion.kept).toEqual([ + { + id: "evt_saved_list_filter", + reason: + 'Filter "pullRequest" already has a value that identifier "getsentry/junior#8" does not have.', + }, + ]); + const [converted] = await fixture.sql + .db() + .select() + .from(juniorAutomations) + .where(eq(juniorAutomations.id, "evt_saved_pull_request")); + expect(converted?.identifier).toBeNull(); + expect( + await listAutomationVersions( + fixture.sql.db(), + "evt_saved_pull_request", + 5, ), - ).resolves.toMatchObject({ - automation: { trigger: { identifier: null, resourceType: null } }, - }); + ).toMatchObject([ + { + editedBy: null, + definition: { + trigger: { + events: ["pull_request.opened"], + match: { repo: "getsentry/junior", pullRequest: 8 }, + }, + }, + }, + ]); + // Both rows run once for a pull request event, and not for another + // pull request. + const dispatchedIds = async (pullRequest: number, eventKey: string) => { + const before = queue.sentRecords().length; + for (const identifier of [ + `getsentry/junior#${pullRequest}`, + "getsentry/junior", + ]) { + await ingestEventAutomations( + { + eventKey, + eventType: "pull_request.opened", + occurredAtMs: Date.now(), + namespace: "github", + identifier, + trustedSummary: "A pull request was opened.", + data: { pullRequest, repo: "getsentry/junior" }, + }, + { queue, teamId }, + ); + } + const ids = []; + for (const sentRecord of queue.sentRecords().slice(before)) { + const dispatched = await getDispatchRecord( + sentRecord.conversationId.replace(/^agent-dispatch:/, ""), + ); + ids.push(dispatched?.metadata?.eventAutomationId); + } + return ids.filter((id) => id?.startsWith("evt_saved_")).sort(); + }; + expect(await dispatchedIds(8, "github:opened-8")).toEqual([ + "evt_saved_list_filter", + "evt_saved_pull_request", + ]); + expect(await dispatchedIds(9, "github:opened-9")).toEqual([]); } finally { vi.unstubAllEnvs(); } diff --git a/packages/junior/tests/integration/api/automation-edits.test.ts b/packages/junior/tests/integration/api/automation-edits.test.ts index 15d44a763d..4365a2b063 100644 --- a/packages/junior/tests/integration/api/automation-edits.test.ts +++ b/packages/junior/tests/integration/api/automation-edits.test.ts @@ -899,9 +899,8 @@ describe("Automation edit API", () => { trigger, }); expect(response.status).toBe(200); - expect(await response.json()).toMatchObject({ - trigger: { ...trigger, identifier: "getsentry/junior#42" }, - }); + // The identifier is saved as filters that select the same events. + expect((await response.json()).trigger).not.toHaveProperty("identifier"); // Version history shows each saved trigger in the shared words. const { versions } = automationVersionListSchema.parse( await (await app.request(`${url}/versions`)).json(), @@ -918,7 +917,11 @@ describe("Automation edit API", () => { ).toEqual([ { events: ["pull request merged"], - filters: ["headBranch is main"], + filters: [ + "repo is getsentry/junior", + "headBranch is main", + "pullRequest is 42", + ], problems: [], }, saved, @@ -946,9 +949,9 @@ describe("Automation edit API", () => { expect(await summary.json()).toMatchObject({ trigger: { filters: ["authorUsername is dcramer"], - problems: [ + problems: expect.arrayContaining([ expect.stringContaining('Filter "authorUsername" does not exist'), - ], + ]), }, }); expect(await unavailableIds()).toEqual([id]); diff --git a/packages/junior/tests/integration/slack/channel-message-automations.test.ts b/packages/junior/tests/integration/slack/channel-message-automations.test.ts index 85bca193e4..e1156cf3ba 100644 --- a/packages/junior/tests/integration/slack/channel-message-automations.test.ts +++ b/packages/junior/tests/integration/slack/channel-message-automations.test.ts @@ -4,6 +4,7 @@ import { getDispatchRecord } from "@/chat/agent-dispatch/store"; import { getConversationStore } from "@/chat/db"; import { migrateSchema } from "@/chat/conversations/sql/migrations"; import { getEventAutomation } from "@/chat/event-automations/store"; +import { convertSavedEventAutomationTriggers } from "@/chat/event-automations/trigger-conversion"; import { getEventCatalog } from "@/chat/events/runtime-catalog"; import { disconnectStateAdapter, getStateAdapter } from "@/chat/state/adapter"; import { createEventAutomationTool } from "@/chat/tools/create-event-automation"; @@ -129,13 +130,34 @@ describe("Slack channel-message automations", () => { }, )) as { automation: { id: string } }; - // The trigger binds to the channel, and runs never get creator credentials. - expect( - await getEventAutomation(fixture.sql.db(), automation.id), - ).toMatchObject({ + // The trigger binds to the channel with a filter, and runs never get + // creator credentials. + const created = await getEventAutomation(fixture.sql.db(), automation.id); + expect(created).toMatchObject({ credentialMode: "system", - trigger: { identifier: "C123", namespace: "slack" }, + trigger: { namespace: "slack", match: { channelId: "C123" } }, }); + expect(created?.trigger.identifier).toBeUndefined(); + + // A row saved before channel filters names the channel with its + // identifier. The heartbeat step turns it into the same channel filter. + const { match: _match, ...unfiltered } = created!.trigger; + await fixture.sql + .db() + .update(juniorAutomations) + .set({ + identifier: "C123", + record: sql`jsonb_set(${juniorAutomations.record}, '{trigger}', ${JSON.stringify( + { ...unfiltered, identifier: "C123", resourceType: "channel" }, + )}::jsonb)`, + }) + .where(eq(juniorAutomations.id, automation.id)); + expect( + await convertSavedEventAutomationTriggers(fixture.sql.db(), catalog), + ).toEqual({ converted: [automation.id], kept: [] }); + expect( + (await getEventAutomation(fixture.sql.db(), automation.id))?.trigger, + ).toEqual(created?.trigger); // A thread reply, a bot post, and a message in another channel do not run it. expect( @@ -251,7 +273,7 @@ describe("Slack channel-message automations", () => { }), ).rejects.toThrow("can watch only the channel where it posts"); - // A trigger with no identifier would read every channel. + // A trigger with no identifier must name its own channel in a filter. const { identifier: _identifier, resourceType: _type, @@ -264,7 +286,15 @@ describe("Slack channel-message automations", () => { outcomes: [], trigger: anyChannel, }), - ).rejects.toThrow("This source cannot run for every resource."); + ).rejects.toThrow('This trigger needs a "channelId" filter'); + await expect( + execute(createEventAutomationTool(channelContext, catalog), { + instruction: "Summarize new messages.", + title: "Summarize another channel", + outcomes: [], + trigger: { ...anyChannel, match: { channelId: "C999" } }, + }), + ).rejects.toThrow("can watch only the channel where it posts"); // A message runs only automations that post in its own channel, even // when a saved row names that channel and posts somewhere else. From 5491278cb8fc33d64334a260d487b5f3f30c1886 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:32:43 +0000 Subject: [PATCH 2/6] feat(events): Save why a trigger keeps its identifier A trigger that cannot convert to filters now saves the reason in `identifierReason` through the same locked, versioned save. The heartbeat checks at most 50 rows per tick that have an identifier and no saved reason, so it checks each row once. It skips rows whose identifier column and saved trigger disagree, because a save would make them broad. Edits that change the trigger set the reason again. The trigger description shows the reason as a note, not a problem, in the dashboard details, Slack details, and listEventAutomations. Rows that keep their identifier stay active. The update tool description now says plainly that leaving out match keeps the saved filters, and only `{}` removes them. Refs #2362 Co-Authored-By: David Cramer --- .../pages/automations/AutomationDetails.tsx | 5 + .../src/mock-reporting/fixtures.ts | 4 + packages/junior/src/api/schema/automation.ts | 2 + .../src/chat/event-automations/README.md | 19 +-- .../src/chat/event-automations/store.ts | 15 ++- .../chat/event-automations/tool-support.ts | 33 ++++-- .../event-automations/trigger-conversion.ts | 109 ++++++++++-------- .../src/chat/event-automations/types.ts | 15 +++ .../src/chat/slack/automation-details.ts | 6 + .../src/chat/slack/channel-message-events.ts | 1 + .../src/chat/tools/list-event-automations.ts | 2 +- .../src/chat/tools/update-event-automation.ts | 2 +- .../any-resource-event-automations.test.ts | 93 ++++++++++----- .../integration/api/automation-edits.test.ts | 3 + .../tests/integration/api/automations.test.ts | 1 + 15 files changed, 211 insertions(+), 99 deletions(-) diff --git a/packages/junior-dashboard/src/client/pages/automations/AutomationDetails.tsx b/packages/junior-dashboard/src/client/pages/automations/AutomationDetails.tsx index b038522090..2fcb71d6fe 100644 --- a/packages/junior-dashboard/src/client/pages/automations/AutomationDetails.tsx +++ b/packages/junior-dashboard/src/client/pages/automations/AutomationDetails.tsx @@ -117,6 +117,11 @@ function AutomationDetailsContent({ {automation.trigger.events.join(", ")}

+ {automation.trigger.notes.map((note) => ( +

+ {note} +

+ ))} )} diff --git a/packages/junior-dashboard/src/mock-reporting/fixtures.ts b/packages/junior-dashboard/src/mock-reporting/fixtures.ts index 0fb1d93de2..2b3b9c9f43 100644 --- a/packages/junior-dashboard/src/mock-reporting/fixtures.ts +++ b/packages/junior-dashboard/src/mock-reporting/fixtures.ts @@ -3019,6 +3019,9 @@ function mockTasks(): AutomationSummary[] { events: ["issue closed"], filters: ["labels includes bug", "repository is api or web"], problems: [], + notes: [ + "Kept its exact issue because no filters select the same events.", + ], }, status: "active", }, @@ -3052,6 +3055,7 @@ function mockTasks(): AutomationSummary[] { events: ["incident updated"], filters: [], problems: ["The plugin for this source is not enabled."], + notes: [], }, status: "active", }, diff --git a/packages/junior/src/api/schema/automation.ts b/packages/junior/src/api/schema/automation.ts index b598bdac43..7781b3efee 100644 --- a/packages/junior/src/api/schema/automation.ts +++ b/packages/junior/src/api/schema/automation.ts @@ -78,6 +78,8 @@ export const eventTriggerDescriptionSchema = z filters: z.array(z.string().min(1)), /** Why the trigger cannot work. The trigger is available when empty. */ problems: z.array(z.string().min(1)), + /** Facts about a trigger that still works, such as a kept identifier. */ + notes: z.array(z.string().min(1)), }) .strict() satisfies z.ZodType; diff --git a/packages/junior/src/chat/event-automations/README.md b/packages/junior/src/chat/event-automations/README.md index e7df110ee3..bf84235bfa 100644 --- a/packages/junior/src/chat/event-automations/README.md +++ b/packages/junior/src/chat/event-automations/README.md @@ -49,16 +49,21 @@ becomes `repo` and `pullRequest` filters. `trigger-conversion.ts` owns the rule. A trigger converts only when the plugin gives the same filters for every selected event, those filters agree with the saved filters, and the result passes the check for a new trigger. Otherwise it keeps its identifier and -keeps running. A conversion never makes a trigger broader. +keeps running, and the trigger saves the reason in `identifierReason`. Each +edit that changes the trigger sets the reason again. A conversion never makes +a trigger broader. -Each heartbeat converts saved rows that still have an identifier. It saves +Each heartbeat checks at most 50 saved rows that have an identifier, no +saved reason, and the same identifier in the row and the trigger. It saves each row with the revision that it read, under the row lock, and records a version with no editor. A row that changed after the read is left for the next -heartbeat. Rows that cannot convert keep their identifier. Examples are a -Sentry project trigger for metric alerts, because a metric alert can leave -out `projects`, and a row whose plugin is not enabled. Slice D of #2362 -decides what happens to these rows before the identifier column goes away. -Saved versions keep their old triggers, and the version schema still reads +heartbeat. A row that cannot convert keeps its identifier, stays active, and +saves the reason, so later heartbeats skip it. Examples are a Sentry project +trigger for metric alerts, because a metric alert can leave out `projects`, +and a row whose plugin is not enabled. The trigger description shows the +reason as a note, not a problem, in the dashboard, Slack details, and +`listEventAutomations`. Slice D of #2362 decides what happens to these rows +before the identifier column goes away. Saved versions keep their old triggers, and the version schema still reads `identifier` and `resourceType`. `searchEventTypes` exposes the same enabled plugin catalog used by diff --git a/packages/junior/src/chat/event-automations/store.ts b/packages/junior/src/chat/event-automations/store.ts index ae120ecd28..63411c0e6e 100644 --- a/packages/junior/src/chat/event-automations/store.ts +++ b/packages/junior/src/chat/event-automations/store.ts @@ -296,11 +296,15 @@ export async function listDeletedEventAutomationsCreatedBy( } /** - * List live event automations whose trigger still has an identifier. Skip a - * row that does not parse; ingest logs it when an event reaches it. + * List up to `limit` live event automations whose trigger still has an + * identifier and no saved reason to keep it. Skip a row whose identifier + * column and saved trigger disagree, because ingest never runs it and a save + * could make it broad. Skip a row that does not parse; ingest logs it when an + * event reaches it. */ -export async function listEventAutomationsWithIdentifier( +export async function listEventAutomationsToConvert( db: JuniorDatabase, + limit: number, ): Promise { const rows = await db .select() @@ -309,9 +313,12 @@ export async function listEventAutomationsWithIdentifier( and( activeEventAutomationWhere(), isNotNull(juniorAutomations.identifier), + sql`${juniorAutomations.record}->'trigger'->>'identifier' = ${juniorAutomations.identifier}`, + sql`${juniorAutomations.record}->'trigger'->'identifierReason' is null`, ), ) - .orderBy(asc(juniorAutomations.createdAtMs), asc(juniorAutomations.id)); + .orderBy(asc(juniorAutomations.createdAtMs), asc(juniorAutomations.id)) + .limit(limit); return rows.flatMap((row) => { const parsed = eventAutomationSchema.safeParse( eventAutomationRowInput(row), diff --git a/packages/junior/src/chat/event-automations/tool-support.ts b/packages/junior/src/chat/event-automations/tool-support.ts index 809e1bb1e8..de7daef706 100644 --- a/packages/junior/src/chat/event-automations/tool-support.ts +++ b/packages/junior/src/chat/event-automations/tool-support.ts @@ -52,6 +52,8 @@ const compactEventAutomationResultSchema = z match: z.record(z.string(), z.unknown()).optional(), available: z.boolean(), problems: z.array(z.string().min(1)).min(1).optional(), + /** Facts about a trigger that still works, such as a kept identifier. */ + notes: z.array(z.string().min(1)).min(1).optional(), }) .strict(), credentialMode: z.enum(["system", "creator"]), @@ -130,7 +132,9 @@ export function registeredEventAutomationTriggerSchema(catalog: EventCatalog) { * it as it is saved. It names a resource type and an identifier, or neither. * The identifier gets its plugin's convention. A trigger with an identifier is * saved with no identifier when its plugin can turn the identifier into - * filters that select the same events. Events and list values lose repeats. + * filters that select the same events. Otherwise it is saved with the reason + * that it keeps the identifier, so each edit sets the reason again. Events + * and list values lose repeats. */ export function requireEventAutomationTrigger( catalog: EventCatalog, @@ -172,8 +176,7 @@ export function requireEventAutomationTrigger( events, ...(match ? { match } : undefined), }; - const converted = triggerWithoutIdentifier(catalog, trigger); - return "trigger" in converted ? converted.trigger : trigger; + return triggerWithoutIdentifier(catalog, trigger).trigger; } /** Require the active Slack authority used for event-automation management. */ @@ -247,20 +250,26 @@ export async function writableEventAutomation( return task; } +/** A saved trigger in words, with notes about how Junior saved it. */ +export type EventAutomationTriggerDescription = EventTriggerDescription & { + /** Plain facts about a trigger that still works, such as a kept identifier. */ + notes: string[]; +}; + /** * Describe a saved trigger with the enabled plugins. Its problems say why it * cannot work: a plugin can remove or rename a resource type, event, or filter * after the trigger was saved. Resource types with `automation: false` never - * dispatch event automations. + * dispatch event automations. Its notes do not stop it from working. */ export function describeEventAutomationTrigger( trigger: EventAutomation["trigger"], catalog: EventCatalog, -): EventTriggerDescription { - return describeRegisteredEventTrigger( - automationEventCatalog(catalog), - trigger, - ); +): EventAutomationTriggerDescription { + return { + ...describeRegisteredEventTrigger(automationEventCatalog(catalog), trigger), + notes: trigger.identifierReason ? [trigger.identifierReason] : [], + }; } /** Project an event automation into the bounded tool-result shape. */ @@ -269,7 +278,10 @@ export function compactEventAutomation( catalog: EventCatalog, requesterSlackUserId?: string, ) { - const { problems } = describeEventAutomationTrigger(task.trigger, catalog); + const { problems, notes } = describeEventAutomationTrigger( + task.trigger, + catalog, + ); return compactEventAutomationResultSchema.parse({ id: task.id, status: task.status, @@ -286,6 +298,7 @@ export function compactEventAutomation( ...(task.trigger.match ? { match: task.trigger.match } : undefined), available: problems.length === 0, ...(problems.length > 0 ? { problems } : undefined), + ...(notes.length > 0 ? { notes } : undefined), }, credentialMode: task.credentialMode, isCreator: task.createdBy.slackUserId === requesterSlackUserId, diff --git a/packages/junior/src/chat/event-automations/trigger-conversion.ts b/packages/junior/src/chat/event-automations/trigger-conversion.ts index 4b5509fc7f..f3f02c28fd 100644 --- a/packages/junior/src/chat/event-automations/trigger-conversion.ts +++ b/packages/junior/src/chat/event-automations/trigger-conversion.ts @@ -6,7 +6,7 @@ * the rule: a trigger converts only when its plugin gives one set of filters * for every selected event, the filters agree with the saved filters, and the * result passes the same check as a new trigger. Otherwise the trigger keeps - * its identifier, so it never gets broader. + * its identifier and a plain reason, so it never gets broader. */ import { stableEventMatchKey, @@ -23,18 +23,18 @@ import { } from "@/chat/events/catalog"; import { logException, logInfo } from "@/chat/logging"; import type { JuniorDatabase } from "@/db/db"; -import { - listEventAutomationsWithIdentifier, - saveEventAutomation, -} from "./store"; +import { listEventAutomationsToConvert, saveEventAutomation } from "./store"; import type { EventAutomation } from "./types"; type EventAutomationTrigger = EventAutomation["trigger"]; -/** A trigger with no identifier, or why the trigger keeps its identifier. */ +/** + * A trigger with no identifier, or the trigger with its identifier and the + * reason that it keeps it. + */ export type TriggerConversion = - | { trigger: EventAutomationTrigger } - | { reason: string }; + | { converted: true; trigger: EventAutomationTrigger } + | { converted: false; trigger: EventAutomationTrigger; reason: string }; function sameValue(left: EventMatch[string], right: EventMatch[string]) { return ( @@ -43,44 +43,54 @@ function sameValue(left: EventMatch[string], right: EventMatch[string]) { ); } +/** Why the trigger keeps its identifier, in words for the people who own it. */ +function keptReason(trigger: EventAutomationTrigger, why: string): string { + const resource = trigger.resourceType?.replaceAll("_", " ") ?? "resource"; + return `Kept its exact ${resource} because ${why}.`; +} + /** * Return the trigger with no identifier that selects the same events as this - * trigger, or why it cannot. A trigger with no identifier stays as it is. + * trigger. When no such trigger exists, return the trigger with its + * identifier and the reason. A trigger with no identifier stays as it is. */ export function triggerWithoutIdentifier( catalog: EventCatalog, trigger: EventAutomationTrigger, ): TriggerConversion { - const { identifier, resourceType: _resourceType, ...rest } = trigger; - if (identifier === undefined) return { trigger }; - const plugins = automationEventCatalog(catalog); - const toMatch = plugins[trigger.namespace]?.identifierMatch; - if (!toMatch) { + const { + identifier, + resourceType: _resourceType, + identifierReason: _identifierReason, + ...rest + } = trigger; + if (identifier === undefined) return { converted: true, trigger }; + const keep = (why: string): TriggerConversion => { + const reason = keptReason(trigger, why); return { - reason: `The ${trigger.namespace} source cannot turn identifiers into filters.`, + converted: false, + trigger: { ...trigger, identifierReason: reason }, + reason, }; - } + }; + const plugins = automationEventCatalog(catalog); + const source = plugins[trigger.namespace]; + if (!source) return keep("its plugin was not enabled when Junior checked it"); + const toMatch = source.identifierMatch; + if (!toMatch) return keep("its source cannot use filters in its place"); let scope: EventMatch | undefined; for (const eventType of trigger.events) { const match = toMatch({ identifier, eventType }); - if (!match) { - return { - reason: `Identifier "${identifier}" cannot become filters for "${eventType}" without matching other events.`, - }; - } + if (!match) return keep("no filters select the same events"); if (scope && stableEventMatchKey(scope) !== stableEventMatchKey(match)) { - return { - reason: `Identifier "${identifier}" needs different filters for different events.`, - }; + return keep("its events need different filters"); } scope = match; } for (const [key, value] of Object.entries(scope ?? {})) { const saved = trigger.match?.[key]; if (saved !== undefined && !sameValue(saved, value)) { - return { - reason: `Filter "${key}" already has a value that identifier "${identifier}" does not have.`, - }; + return keep(`its saved ${key} filter has a different value`); } } const match = { ...trigger.match, ...scope }; @@ -89,43 +99,48 @@ export function triggerWithoutIdentifier( events: trigger.events, match, }); - if (problems.length > 0) { - return { reason: problems.map((problem) => problem.message).join(" ") }; + if (problems.some((problem) => problem.reason === "identifier_required")) { + return keep("these events can only run for one resource"); } - return { trigger: { ...rest, match } }; + if (problems.length > 0) return keep("no filters select the same events"); + return { converted: true, trigger: { ...rest, match } }; } +/** Most saved triggers that one heartbeat checks. */ +const CONVERSION_BATCH_LIMIT = 50; + /** - * Convert every saved event automation trigger that still has an identifier - * and that `triggerWithoutIdentifier` can convert. Each save locks the row, - * checks that the row did not change after it was read, and records a new - * version with no editor. A row that changed is left for the next run. Other - * rows keep their identifier and keep running. + * Check saved event automation triggers that have an identifier and no saved + * reason, at most `CONVERSION_BATCH_LIMIT` in one call. Save each one with + * no identifier when `triggerWithoutIdentifier` can convert it. Otherwise + * save the reason, so later calls skip it and people can see why. Each save + * locks the row, checks that the row did not change after it was read, and + * records a new version with no editor. A row that changed is left for the + * next call. Every trigger stays active. */ export async function convertSavedEventAutomationTriggers( db: JuniorDatabase, catalog: EventCatalog, -): Promise<{ converted: string[]; kept: { id: string; reason: string }[] }> { +): Promise<{ converted: string[]; kept: string[] }> { const converted: string[] = []; - const kept: { id: string; reason: string }[] = []; - for (const task of await listEventAutomationsWithIdentifier(db)) { + const kept: string[] = []; + const tasks = await listEventAutomationsToConvert(db, CONVERSION_BATCH_LIMIT); + for (const task of tasks) { try { const conversion = triggerWithoutIdentifier(catalog, task.trigger); - if ("reason" in conversion) { - kept.push({ id: task.id, reason: conversion.reason }); - continue; - } const saved = await saveEventAutomation( db, { ...task, trigger: conversion.trigger }, automationRevision(task), ); if (!saved) continue; - converted.push(task.id); - logInfo("event_automation.trigger.converted", { - "app.task.id": task.id, - "app.task.type": "event", - }); + (conversion.converted ? converted : kept).push(task.id); + logInfo( + conversion.converted + ? "event_automation.trigger.converted" + : "event_automation.trigger.identifier_kept", + { "app.task.id": task.id, "app.task.type": "event" }, + ); } catch (error) { if (error instanceof AutomationConflictError) continue; logException(error, "event_automation.trigger.conversion_failed", { diff --git a/packages/junior/src/chat/event-automations/types.ts b/packages/junior/src/chat/event-automations/types.ts index fa5e3841f7..c5c6480d59 100644 --- a/packages/junior/src/chat/event-automations/types.ts +++ b/packages/junior/src/chat/event-automations/types.ts @@ -38,6 +38,12 @@ export const eventAutomationTriggerSchema = z .max(EVENT_AUTOMATION_IDENTIFIER_MAX_LENGTH) .optional(), resourceType: z.string().min(1).optional(), + /** + * Why the trigger keeps its identifier instead of filters, in plain + * words. Junior sets it when it cannot turn the identifier into filters + * that select the same events. A trigger with no identifier has none. + */ + identifierReason: z.string().min(1).max(500).optional(), }) .strict() .refine( @@ -49,6 +55,15 @@ export const eventAutomationTriggerSchema = z "Set both a resource type and an identifier, or leave out both to run for every resource.", path: ["identifier"], }, + ) + .refine( + (trigger) => + trigger.identifierReason === undefined || + trigger.identifier !== undefined, + { + message: "Only a trigger with an identifier can keep a reason for it.", + path: ["identifierReason"], + }, ); /** Event automations target a Slack channel or DM, never a message thread. */ diff --git a/packages/junior/src/chat/slack/automation-details.ts b/packages/junior/src/chat/slack/automation-details.ts index 9309bca798..9044b4e775 100644 --- a/packages/junior/src/chat/slack/automation-details.ts +++ b/packages/junior/src/chat/slack/automation-details.ts @@ -85,6 +85,12 @@ function renderAutomationDetails( long: true, }, ); + if (trigger.notes.length > 0) { + fields.push({ + ...textField("note", "Note", trigger.notes.join(" ")), + long: true, + }); + } if (trigger.problems.length > 0) { fields.push({ ...textField( diff --git a/packages/junior/src/chat/slack/channel-message-events.ts b/packages/junior/src/chat/slack/channel-message-events.ts index d50842e789..0a39810a84 100644 --- a/packages/junior/src/chat/slack/channel-message-events.ts +++ b/packages/junior/src/chat/slack/channel-message-events.ts @@ -100,6 +100,7 @@ export function requireSlackChannelMessageScope( const { identifier, resourceType: _resourceType, + identifierReason: _identifierReason, ...trigger } = automation.trigger; const named = [ diff --git a/packages/junior/src/chat/tools/list-event-automations.ts b/packages/junior/src/chat/tools/list-event-automations.ts index 8d970d950e..328115234b 100644 --- a/packages/junior/src/chat/tools/list-event-automations.ts +++ b/packages/junior/src/chat/tools/list-event-automations.ts @@ -26,7 +26,7 @@ export function createListEventAutomationsTool( readOnlyHint: true, }, description: - "List event automations for the current destination, not one thread. Each result says whether the requester is the creator (isCreator); only the creator can change its credential use. Warn when trigger.available is false and explain trigger.problems: the automation stays stored, but a plugin no longer supports its resource type, an event, or a filter. Update the trigger to fix it. Public automations can also be updated or deleted by id from another destination in the same workspace.", + "List event automations for the current destination, not one thread. Each result says whether the requester is the creator (isCreator); only the creator can change its credential use. Warn when trigger.available is false and explain trigger.problems: the automation stays stored, but a plugin no longer supports its resource type, an event, or a filter. Update the trigger to fix it. trigger.notes are facts about a trigger that still works, such as why it kept its exact identifier; mention them only when relevant. Public automations can also be updated or deleted by id from another destination in the same workspace.", inputSchema: z.object({}).strict(), outputSchema: eventAutomationListToolResultSchema, async execute() { diff --git a/packages/junior/src/chat/tools/update-event-automation.ts b/packages/junior/src/chat/tools/update-event-automation.ts index ece68ddaf3..6494b810a2 100644 --- a/packages/junior/src/chat/tools/update-event-automation.ts +++ b/packages/junior/src/chat/tools/update-event-automation.ts @@ -42,7 +42,7 @@ export function createUpdateEventAutomationTool( }, executionMode: "sequential", description: - "Update the instruction, registered trigger, credential use, or status of an event automation. In trigger, an omitted identifier, resourceType, or match keeps the saved value. A match replaces every saved filter, so include saved filters such as repo, projects, or channelId that limit the trigger.", + "Update the instruction, registered trigger, credential use, or status of an event automation. In trigger, leaving out identifier, resourceType, or match keeps the saved value. Only identifier null removes the identifier, and only match {} removes all filters. A new match replaces all saved filters, so repeat each saved filter to keep, such as repo, projects, or channelId.", inputSchema: z .object({ automationId: z.string().min(1), diff --git a/packages/junior/tests/integration/any-resource-event-automations.test.ts b/packages/junior/tests/integration/any-resource-event-automations.test.ts index 2ead6a7731..60f4d3c1f8 100644 --- a/packages/junior/tests/integration/any-resource-event-automations.test.ts +++ b/packages/junior/tests/integration/any-resource-event-automations.test.ts @@ -3,15 +3,16 @@ import type { EventMatch } from "@sentry/junior-plugin-api"; import { eq, sql } from "drizzle-orm"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createApp, defineJuniorPlugins } from "@/app"; +import { runHeartbeat } from "@/chat/agent-dispatch/heartbeat"; import { getDispatchRecord } from "@/chat/agent-dispatch/store"; import { migrateSchema } from "@/chat/conversations/sql/migrations"; import { getConversationStore } from "@/chat/db"; import { ingestEventAutomations } from "@/chat/event-automations/ingest"; -import { convertSavedEventAutomationTriggers } from "@/chat/event-automations/trigger-conversion"; import { listAutomationVersions } from "@/chat/automations/versions"; import { getEventCatalog } from "@/chat/events/runtime-catalog"; import { disconnectStateAdapter } from "@/chat/state/adapter"; import { createEventAutomationTool } from "@/chat/tools/create-event-automation"; +import { createListEventAutomationsTool } from "@/chat/tools/list-event-automations"; import { createUpdateEventAutomationTool } from "@/chat/tools/update-event-automation"; import type { EventAutomationRecord } from "@/chat/event-automations/types"; import { juniorAutomations } from "@/db/schema/automations"; @@ -251,9 +252,9 @@ describe("event automations with no identifier", () => { }); // Rows saved before this change still name one resource. The heartbeat - // step converts each one under its row lock and saves a version with - // no editor. A row that cannot convert without matching more events - // keeps its identifier and keeps running. + // converts each one under its row lock and saves a version with no + // editor. A row that cannot convert without matching more events keeps + // its identifier, saves the reason, and keeps running. const savedPullRequest = (id: string, match?: EventMatch) => ({ ...row!, id, @@ -274,32 +275,37 @@ describe("event automations with no identifier", () => { .values([ savedPullRequest("evt_saved_pull_request"), savedPullRequest("evt_saved_list_filter", { pullRequest: [8, 9] }), + // Its saved trigger has no identifier, so ingest never runs it. + // Conversion must not save it as a broad row. + { + ...savedPullRequest("evt_saved_column_only"), + record: { + ...record, + trigger: { + ...record.trigger, + match: { repo: "getsentry/junior" }, + }, + }, + }, ]); - const conversion = await convertSavedEventAutomationTriggers( - fixture.sql.db(), - catalog, - ); - expect(conversion.converted).toEqual(["evt_saved_pull_request"]); - expect(conversion.kept).toEqual([ - { - id: "evt_saved_list_filter", - reason: - 'Filter "pullRequest" already has a value that identifier "getsentry/junior#8" does not have.', - }, - ]); - const [converted] = await fixture.sql - .db() - .select() - .from(juniorAutomations) - .where(eq(juniorAutomations.id, "evt_saved_pull_request")); - expect(converted?.identifier).toBeNull(); - expect( - await listAutomationVersions( - fixture.sql.db(), - "evt_saved_pull_request", - 5, - ), - ).toMatchObject([ + const savedRows = async () => + Object.fromEntries( + ( + await fixture.sql + .db() + .select() + .from(juniorAutomations) + .where(sql`${juniorAutomations.id} like 'evt_saved_%'`) + ).map((saved) => [saved.id, saved]), + ); + const versions = (id: string) => + listAutomationVersions(fixture.sql.db(), id, 5); + await runHeartbeat({ nowMs: Date.now(), conversationWorkQueue: queue }); + const reason = + "Kept its exact pull request because its saved pullRequest filter has a different value."; + const rows = await savedRows(); + expect(rows.evt_saved_pull_request?.identifier).toBeNull(); + expect(await versions("evt_saved_pull_request")).toMatchObject([ { editedBy: null, definition: { @@ -310,6 +316,35 @@ describe("event automations with no identifier", () => { }, }, ]); + expect(rows.evt_saved_list_filter?.identifier).toBe("getsentry/junior#8"); + expect(rows.evt_saved_list_filter?.status).toBe("active"); + expect(await versions("evt_saved_list_filter")).toMatchObject([ + { + editedBy: null, + definition: { trigger: { identifierReason: reason } }, + }, + ]); + // The saved reason is a note on the trigger, not a problem. Listing + // reads every row of the team, so remove the row that does not parse. + await fixture.sql + .db() + .delete(juniorAutomations) + .where(eq(juniorAutomations.id, "evt_bad_record")); + const listed = (await execute( + createListEventAutomationsTool(context(), catalog), + {}, + "list-kept", + )) as { + automations: { id: string; trigger: Record }[]; + }; + expect( + listed.automations.find(({ id }) => id === "evt_saved_list_filter") + ?.trigger, + ).toMatchObject({ available: true, notes: [reason] }); + // The next heartbeat does not save the kept row again. + await runHeartbeat({ nowMs: Date.now(), conversationWorkQueue: queue }); + expect(await versions("evt_saved_list_filter")).toHaveLength(1); + expect(await versions("evt_saved_column_only")).toEqual([]); // Both rows run once for a pull request event, and not for another // pull request. const dispatchedIds = async (pullRequest: number, eventKey: string) => { diff --git a/packages/junior/tests/integration/api/automation-edits.test.ts b/packages/junior/tests/integration/api/automation-edits.test.ts index 4365a2b063..57b120bbd2 100644 --- a/packages/junior/tests/integration/api/automation-edits.test.ts +++ b/packages/junior/tests/integration/api/automation-edits.test.ts @@ -909,6 +909,7 @@ describe("Automation edit API", () => { events: ["issue closed"], filters: ["retainedCondition is one or two"], problems: ["The plugin for this source is not enabled."], + notes: [], }; expect( versions.map((version) => @@ -923,6 +924,7 @@ describe("Automation edit API", () => { "pullRequest is 42", ], problems: [], + notes: [], }, saved, saved, @@ -990,6 +992,7 @@ describe("Automation edit API", () => { events: ["pull request opened"], filters: ["repo is getsentry/junior"], problems: [], + notes: [], }, }); } finally { diff --git a/packages/junior/tests/integration/api/automations.test.ts b/packages/junior/tests/integration/api/automations.test.ts index b0e05f9101..23b3d2187b 100644 --- a/packages/junior/tests/integration/api/automations.test.ts +++ b/packages/junior/tests/integration/api/automations.test.ts @@ -407,6 +407,7 @@ describe("Automations API", () => { events: ["issue closed"], filters: ["action is closed or resolved"], problems: ["The plugin for this source is not enabled."], + notes: [], }, }), expect.objectContaining({ From 902ac382e8ff36ea6d3605062629506dc8c888b5 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:09:16 +0000 Subject: [PATCH 3/6] fix(events): Keep converted trigger scope on update and split backfill Updates now change match filters key by key. A left-out filter keeps its saved value and null removes it, so an update cannot drop the scope that a conversion moved into match. A write whose saved filter never matches its identifier is rejected instead of saved as a trigger that never runs. Move the heartbeat backfill of saved rows to a follow-up, because no release can read rows with no identifier yet. Drop the Sentry converter, because the plugin has no saved install org to check identifiers against. Pass the one pull request to the lazy GitHub gate, mark identifierMatch as temporary, and test identifiers that do not rebuild and events that need different filters. Refs #2362 Co-Authored-By: David Cramer --- .../src/content/docs/concepts/automations.md | 2 +- .../src/content/docs/extend/sentry-plugin.md | 6 - .../pages/automations/AutomationDetails.tsx | 2 +- .../src/mock-reporting/fixtures.ts | 1 - .../junior-github/src/webhooks/check-suite.ts | 21 +- .../src/webhooks/pull-request-match-data.ts | 12 +- .../junior-github/tests/event-data.test.ts | 28 +++ .../tests/webhook-outcomes.test.ts | 5 +- packages/junior-linear/tests/webhook.test.ts | 10 + packages/junior-plugin-api/src/events.ts | 4 + .../src/events/identifier-match.ts | 65 ------ packages/junior-sentry/src/index.ts | 2 - packages/junior-sentry/tests/webhook.test.ts | 37 ++- .../src/events/identifier-match.ts | 9 +- packages/junior-vercel/tests/webhook.test.ts | 11 + packages/junior/src/api/schema/automation.ts | 2 +- .../src/chat/agent-dispatch/heartbeat.ts | 10 - .../src/chat/event-automations/README.md | 40 ++-- .../src/chat/event-automations/store.ts | 45 +--- .../chat/event-automations/tool-support.ts | 26 ++- .../event-automations/trigger-conversion.ts | 94 +++----- packages/junior/src/chat/events/README.md | 4 +- .../src/chat/slack/automation-details.ts | 2 +- .../src/chat/tools/update-event-automation.ts | 38 +++- .../any-resource-event-automations.test.ts | 215 ++++++------------ .../integration/api/automation-edits.test.ts | 3 - .../tests/integration/api/automations.test.ts | 1 - .../integration/event-automations.test.ts | 9 +- .../slack/channel-message-automations.test.ts | 40 ++-- .../junior/tests/unit/events/catalog.test.ts | 50 ++++ 30 files changed, 362 insertions(+), 432 deletions(-) delete mode 100644 packages/junior-sentry/src/events/identifier-match.ts diff --git a/packages/docs/src/content/docs/concepts/automations.md b/packages/docs/src/content/docs/concepts/automations.md index d3deb40463..836a3f7149 100644 --- a/packages/docs/src/content/docs/concepts/automations.md +++ b/packages/docs/src/content/docs/concepts/automations.md @@ -61,7 +61,7 @@ whenever a pull request opens in getsentry/junior, review it Each plugin decides which events can run this way, and each plugin page lists them. Some events need a filter. For example, GitHub issue, pull request, and release events need a `repo` filter, and Linear issue events need a `teamKey` or `issueIdentifier` filter. Junior names the missing filter when it cannot save the automation. Junior saves filter values in the case that event data uses, so `getsentry/Junior` matches `getsentry/junior`. One event runs the automation once, even when the plugin publishes the event for more than one resource, such as each project of a metric alert. Slack channel-message automations always need their own channel. -When you name one exact resource, Junior saves it as the filters that select the same events. For example, one pull request becomes `repo` and `pullRequest` filters. Junior changes older automations the same way, and each change saves a new version. An automation keeps its exact resource when filters would miss events or match more events. When you update a trigger, leave out `match` to keep the saved filters. A new `match` replaces every saved filter. +When you name one exact resource, Junior saves it as the filters that select the same events. For example, one pull request becomes `repo` and `pullRequest` filters. An automation keeps its exact resource when filters would miss events or match more events, and its details say why. Junior does not save a trigger whose filter can never match its resource. When you update a trigger, `match` changes only the filters that it names: a value sets a filter, `null` removes it, and other filters keep their saved values. A required filter limits how much an automation covers and how noisy it is. It is not access control. Junior does not check that the person who creates the automation can see the repository, project, or team in the filter. An automation with no exact resource posts what it receives to its destination channel. For example, a broad Sentry trigger or GitHub deployment trigger can post private project or repository names in that channel. Create broad automations only in channels where everyone can see those resources. diff --git a/packages/docs/src/content/docs/extend/sentry-plugin.md b/packages/docs/src/content/docs/extend/sentry-plugin.md index 0854c4d02d..b520d4df24 100644 --- a/packages/docs/src/content/docs/extend/sentry-plugin.md +++ b/packages/docs/src/content/docs/extend/sentry-plugin.md @@ -238,12 +238,6 @@ lowercase. Junior does not check that the creator can see the projects, so a broad automation can post private project names in its channel. See [Automations](/concepts/automations/). -An event automation for one issue, one metric alert, or the issues of one -project is saved as filters (`issueId`, `alertRuleId`, or `projects`). An -automation for the metric alerts of one project keeps its project identifier. -When an alert rule has too many projects to fit in event data, the event -leaves out `projects`, so a `projects` filter would miss that alert. - Create the watch or event automation before the webhook arrives. Junior does not replay earlier webhooks. ## Verify diff --git a/packages/junior-dashboard/src/client/pages/automations/AutomationDetails.tsx b/packages/junior-dashboard/src/client/pages/automations/AutomationDetails.tsx index 2fcb71d6fe..18711d1e30 100644 --- a/packages/junior-dashboard/src/client/pages/automations/AutomationDetails.tsx +++ b/packages/junior-dashboard/src/client/pages/automations/AutomationDetails.tsx @@ -117,7 +117,7 @@ function AutomationDetailsContent({ {automation.trigger.events.join(", ")}

- {automation.trigger.notes.map((note) => ( + {automation.trigger.notes?.map((note) => (

{note}

diff --git a/packages/junior-dashboard/src/mock-reporting/fixtures.ts b/packages/junior-dashboard/src/mock-reporting/fixtures.ts index 2b3b9c9f43..1efd02ee29 100644 --- a/packages/junior-dashboard/src/mock-reporting/fixtures.ts +++ b/packages/junior-dashboard/src/mock-reporting/fixtures.ts @@ -3055,7 +3055,6 @@ function mockTasks(): AutomationSummary[] { events: ["incident updated"], filters: [], problems: ["The plugin for this source is not enabled."], - notes: [], }, status: "active", }, diff --git a/packages/junior-github/src/webhooks/check-suite.ts b/packages/junior-github/src/webhooks/check-suite.ts index f026d1a925..954929f935 100644 --- a/packages/junior-github/src/webhooks/check-suite.ts +++ b/packages/junior-github/src/webhooks/check-suite.ts @@ -451,7 +451,7 @@ export function normalizeCheckSuiteEvents( /** Identifiers and event types one completed check suite may publish. */ export function parseCheckSuitePublishTargets(body: unknown): | { - data: { repo: string }; + data: { repo: string; pullRequest?: number }; eventTypes: string[]; identifiers: string[]; } @@ -465,18 +465,25 @@ export function parseCheckSuitePublishTargets(body: unknown): const identifiers = new Set([ gitHubRepositoryResource({ repo }).identifier, ]); - for (const pullRequest of parsed.data.check_suite.pull_requests) { - if (!isCheckSuiteRepositoryPullRequest(pullRequest, repository.id)) - continue; + const pullRequests = parsed.data.check_suite.pull_requests.filter( + (pullRequest) => + isCheckSuiteRepositoryPullRequest(pullRequest, repository.id), + ); + for (const pullRequest of pullRequests) { identifiers.add( gitHubPullRequestResource({ number: pullRequest.number, repo }) .identifier, ); } return { - // Every copy sets this repo, so a broad trigger for another repo adds - // no lazy keys. - data: { repo: gitHubRepoDataSchema.parse(repo) }, + // Every copy sets this repo, and this pull request when there is one, + // so a trigger for another repo or pull request adds no lazy keys. + data: { + repo: gitHubRepoDataSchema.parse(repo), + ...(pullRequests.length === 1 + ? { pullRequest: pullRequests[0]!.number } + : undefined), + }, eventTypes: [eventType], identifiers: [...identifiers], }; diff --git a/packages/junior-github/src/webhooks/pull-request-match-data.ts b/packages/junior-github/src/webhooks/pull-request-match-data.ts index d5565f51be..ff40f512b0 100644 --- a/packages/junior-github/src/webhooks/pull-request-match-data.ts +++ b/packages/junior-github/src/webhooks/pull-request-match-data.ts @@ -257,11 +257,17 @@ export async function withGitHubPullRequestMatchData(args: { }; // A match decision needs every value that a filter uses. + const pullRequests = new Set(numbers.values()); const keys = (await args.neededMatchKeys?.({ - // Every copy sets this repo, so a broad trigger for another repo adds - // no lazy keys. - data: { repo: gitHubRepoDataSchema.parse(context.repositoryFullName) }, + // Every copy sets this repo, and this pull request when there is one, + // so a trigger for another repo or pull request adds no lazy keys. + data: { + repo: gitHubRepoDataSchema.parse(context.repositoryFullName), + ...(pullRequests.size === 1 + ? { pullRequest: [...pullRequests][0]! } + : undefined), + }, eventTypes: [...new Set(pullRequestEvents.map((e) => e.eventType))], identifiers: [...new Set(pullRequestEvents.map((e) => e.identifier))], })) ?? []; diff --git a/packages/junior-github/tests/event-data.test.ts b/packages/junior-github/tests/event-data.test.ts index 5cb5c4a355..00d28a3d78 100644 --- a/packages/junior-github/tests/event-data.test.ts +++ b/packages/junior-github/tests/event-data.test.ts @@ -248,6 +248,15 @@ async function publish( expect(event.data).toMatchObject(data); } } + // When every copy is for one pull request, core gets its number, so a + // trigger for another pull request skips the lookups. + const numbers = [ + ...new Set(published.map((event) => event.data?.pullRequest)), + ]; + const onePullRequest = numbers.length === 1 ? numbers[0] : undefined; + expect(known.map((data) => data.pullRequest)).toEqual( + known.map(() => onePullRequest), + ); events.push(...published); } return events; @@ -332,6 +341,25 @@ describe("GitHub event data", () => { }), }), ).toEqual([]); + // An identifier that the webhooks never build gets no filters, so a + // trigger that never ran does not start to run. + const sha = "a".repeat(40); + expect( + [ + ["release-source:getsentry/junior:release/1.0", "release.published"], + [ + `deployment-source:getsentry/junior:prod/eu:${sha}`, + "deployment.failed", + ], + ["getsentry/junior#007", "pull_request.opened"], + ["getsentry/junior#007", "issue.opened"], + ].map(([identifier, eventType]) => + githubPlugin().events?.identifierMatch?.({ + identifier: identifier!, + eventType: eventType!, + }), + ), + ).toEqual([undefined, undefined, undefined, undefined]); // Tags keep their text: no URI encoding, and only ASCII letters change. expect(new Set(events.flatMap((event) => event.data?.tag ?? []))).toEqual( new Set(["v1.2.0", "release/Ü1", "release/ü1"]), diff --git a/packages/junior-github/tests/webhook-outcomes.test.ts b/packages/junior-github/tests/webhook-outcomes.test.ts index 50420d0014..704fe38e38 100644 --- a/packages/junior-github/tests/webhook-outcomes.test.ts +++ b/packages/junior-github/tests/webhook-outcomes.test.ts @@ -1340,8 +1340,9 @@ describe("GitHub webhook events", () => { }, }), ).toEqual({ - // Core uses the repo to skip broad triggers for other repositories. - data: { repo: "getsentry/junior" }, + // Core uses the repo and the one pull request to skip triggers for + // other repositories or pull requests. + data: { repo: "getsentry/junior", pullRequest: 10 }, eventTypes: ["pull_request.checks.recovered"], identifiers: ["getsentry/junior", "getsentry/junior#10"], }); diff --git a/packages/junior-linear/tests/webhook.test.ts b/packages/junior-linear/tests/webhook.test.ts index 96c9c5f199..7a70c57500 100644 --- a/packages/junior-linear/tests/webhook.test.ts +++ b/packages/junior-linear/tests/webhook.test.ts @@ -128,6 +128,16 @@ describe("Linear webhook events", () => { toMatch, }), ).toEqual([]); + // Saved identifiers are uppercase. Another identifier never got events, + // so it gets no filters and does not start to run. + expect( + ["eng-7", "eng"].map((identifier) => + linearPlugin().events?.identifierMatch?.({ + eventType: "issue.created", + identifier, + }), + ), + ).toEqual([undefined, undefined]); }); it("normalizes a created issue for the issue and team", () => { diff --git a/packages/junior-plugin-api/src/events.ts b/packages/junior-plugin-api/src/events.ts index 6dc20b8315..0e63c80f13 100644 --- a/packages/junior-plugin-api/src/events.ts +++ b/packages/junior-plugin-api/src/events.ts @@ -437,6 +437,10 @@ export const pluginEventsSchema = z * for example when the event can leave out a scope value or does not go * to this kind of identifier. Core then keeps the identifier, so a * trigger never gets broader. + * + * Temporary: it exists only to move event automation triggers off + * identifiers. Slice D of getsentry/junior#2362 removes it with the + * identifier column. */ identifierMatch: z .function({ diff --git a/packages/junior-sentry/src/events/identifier-match.ts b/packages/junior-sentry/src/events/identifier-match.ts deleted file mode 100644 index 088ea7f1cb..0000000000 --- a/packages/junior-sentry/src/events/identifier-match.ts +++ /dev/null @@ -1,65 +0,0 @@ -import type { EventMatch } from "@sentry/junior-plugin-api"; -import { sentryIssueResource, sentryProjectResource } from "./issue.js"; -import { sentryMetricAlertResource } from "./metric-alert.js"; - -const SLUG = String.raw`[^\s/:#]+`; -const METRIC_ALERT = new RegExp(String.raw`^metric-alert:(${SLUG})\/(\d+)$`); -const PROJECT = new RegExp(String.raw`^(${SLUG})\/(${SLUG})(?:#(\d+))?$`); - -/** Filters for one identifier, with the identifier that the plugin builds from them. */ -function identifierFilters( - identifier: string, - eventType: string, -): { match: EventMatch; built: string } | undefined { - const alert = METRIC_ALERT.exec(identifier); - if (alert) { - if (!eventType.startsWith("metric_alert.")) return undefined; - const [, org, alertRuleId] = alert; - return { - match: { alertRuleId: alertRuleId! }, - built: sentryMetricAlertResource({ org: org!, alertRuleId: alertRuleId! }) - .identifier, - }; - } - const scope = PROJECT.exec(identifier); - // Metric alert events leave out `projects` when the rule has too many - // projects to fit in event data. A project identifier still gets those - // events, so a `projects` filter would miss them. - if (!scope || eventType !== "issue.created") return undefined; - const [, org, project, issueId] = scope; - return issueId === undefined - ? { - match: { projects: project! }, - built: sentryProjectResource({ org: org!, project: project! }) - .identifier, - } - : { - match: { projects: project!, issueId }, - built: sentryIssueResource({ - org: org!, - project: project!, - issueId, - }).identifier, - }; -} - -/** - * Turn a saved Sentry identifier into the filters that select the same - * events of one type. One install is one Sentry org, so the org part of an - * identifier needs no filter. A project identifier converts only for issue - * events, because metric alert events can leave out `projects`. - * - * The filters must rebuild the same identifier with the builder that the - * webhooks use, so a malformed identifier keeps its identifier. - */ -export function sentryIdentifierMatch(input: { - identifier: string; - eventType: string; -}): EventMatch | undefined { - const filters = identifierFilters(input.identifier, input.eventType); - // Saved identifiers went through the plugin's `normalizeIdentifier`. - if (!filters || filters.built.toLowerCase() !== input.identifier) { - return undefined; - } - return filters.match; -} diff --git a/packages/junior-sentry/src/index.ts b/packages/junior-sentry/src/index.ts index a966420197..d1e32ee6b3 100644 --- a/packages/junior-sentry/src/index.ts +++ b/packages/junior-sentry/src/index.ts @@ -20,7 +20,6 @@ import { SENTRY_METRIC_ALERT_MATCH_FIELDS, SENTRY_METRIC_ALERT_SUGGESTED_EVENTS, } from "./events/metric-alert.js"; -import { sentryIdentifierMatch } from "./events/identifier-match.js"; import { SENTRY_API_DOMAINS, issueSentryCredential, @@ -75,7 +74,6 @@ export function sentryPlugin(): PluginRegistration { ), isEnabled: () => Boolean(sentryWebhookSecret()), normalizeIdentifier: (identifier) => identifier.toLowerCase(), - identifierMatch: sentryIdentifierMatch, }, manifest: { commandEnv: { diff --git a/packages/junior-sentry/tests/webhook.test.ts b/packages/junior-sentry/tests/webhook.test.ts index aec15d487c..8d7c2fa8eb 100644 --- a/packages/junior-sentry/tests/webhook.test.ts +++ b/packages/junior-sentry/tests/webhook.test.ts @@ -1,5 +1,9 @@ import { createHmac } from "node:crypto"; -import { eventMatches, type EventInput } from "@sentry/junior-plugin-api"; +import { + eventMatches, + type EventInput, + type EventMatch, +} from "@sentry/junior-plugin-api"; import { filterEventMismatches, identifierMatchMismatches, @@ -162,29 +166,22 @@ describe("Sentry webhook events", () => { normalizeSentryEvents(delivery), ); - // The org part is dropped: the install already fixes the org. Saved - // identifiers went through the plugin's `normalizeIdentifier`. - const toMatch = (event: { eventType: string; identifier: string }) => - sentryPlugin().events?.identifierMatch?.({ - eventType: event.eventType, - identifier: event.identifier.toLowerCase(), - }); - // A metric alert can leave out `projects`, so its project copies keep - // their identifier. - const projectAlerts = events.filter( - (event) => - event.eventType.startsWith("metric_alert.") && - !event.identifier.startsWith("metric-alert:"), - ); - expect(projectAlerts.length).toBeGreaterThan(0); - expect(projectAlerts.map(toMatch)).toEqual( - projectAlerts.map(() => undefined), - ); + // The org part is dropped: the install already fixes the org. + const toMatch = (event: { identifier: string }): EventMatch | undefined => { + // Saved identifiers went through the plugin's `normalizeIdentifier`. + const identifier = event.identifier.toLowerCase(); + const alert = /^metric-alert:[^/]+\/(\d+)$/.exec(identifier); + if (alert) return { alertRuleId: alert[1]! }; + const scope = /^[^/]+\/([^/#]+)(?:#(\d+))?$/.exec(identifier); + if (!scope) return undefined; + const [, project, issueId] = scope; + return { projects: [project!], ...(issueId ? { issueId } : undefined) }; + }; expect( identifierMatchMismatches({ resourceTypes: sentryPlugin().events?.resourceTypes ?? [], - events: events.filter((event) => !projectAlerts.includes(event)), + events, matches: eventMatches, toMatch, }), diff --git a/packages/junior-vercel/src/events/identifier-match.ts b/packages/junior-vercel/src/events/identifier-match.ts index 3bd3f1cee3..e5c80c8b2c 100644 --- a/packages/junior-vercel/src/events/identifier-match.ts +++ b/packages/junior-vercel/src/events/identifier-match.ts @@ -2,7 +2,6 @@ import type { EventMatch } from "@sentry/junior-plugin-api"; import { VERCEL_DEPLOYMENT_EVENTS, VERCEL_DEPLOYMENT_TARGETS, - vercelDeploymentResource, type VercelDeploymentTarget, } from "./deployment.js"; @@ -15,9 +14,9 @@ function isTarget(value: string): value is VercelDeploymentTarget { * deployment events: `projectId`, then `target`, then `commitSha`, one for * each part of `projectId[:target[:commitSha]]`. * - * The filters must rebuild the same identifier with the builder that the - * webhooks use. Otherwise the identifier never got events, and filters could - * match events that it never got. + * Each part must have the form that `vercelDeploymentResource` builds: a + * known target and a full lowercase commit SHA. Otherwise the identifier + * never got events, and filters could match events that it never got. */ export function vercelIdentifierMatch(input: { identifier: string; @@ -33,8 +32,6 @@ export function vercelIdentifierMatch(input: { if (commitSha !== undefined && !/^[0-9a-f]{40}$/.test(commitSha)) { return undefined; } - const built = vercelDeploymentResource({ projectId, target, commitSha }); - if (built.identifier !== input.identifier) return undefined; return { projectId, ...(target ? { target } : undefined), diff --git a/packages/junior-vercel/tests/webhook.test.ts b/packages/junior-vercel/tests/webhook.test.ts index 1b92dec760..7cd6349c53 100644 --- a/packages/junior-vercel/tests/webhook.test.ts +++ b/packages/junior-vercel/tests/webhook.test.ts @@ -122,6 +122,17 @@ describe("Vercel webhook events", () => { toMatch, }), ).toEqual([]); + // An identifier that the webhooks never build gets no filters, so a + // trigger that never ran does not start to run. + expect( + [ + "prj_1:prod", + `prj_1:production:${"A".repeat(40)}`, + `prj_1:production:${"a".repeat(40)}:extra`, + ].map((identifier) => + toMatch({ eventType: "deployment.succeeded", identifier }), + ), + ).toEqual([undefined, undefined, undefined]); }); it.each([ diff --git a/packages/junior/src/api/schema/automation.ts b/packages/junior/src/api/schema/automation.ts index 7781b3efee..8ce8aeaeae 100644 --- a/packages/junior/src/api/schema/automation.ts +++ b/packages/junior/src/api/schema/automation.ts @@ -79,7 +79,7 @@ export const eventTriggerDescriptionSchema = z /** Why the trigger cannot work. The trigger is available when empty. */ problems: z.array(z.string().min(1)), /** Facts about a trigger that still works, such as a kept identifier. */ - notes: z.array(z.string().min(1)), + notes: z.array(z.string().min(1)).min(1).optional(), }) .strict() satisfies z.ZodType; diff --git a/packages/junior/src/chat/agent-dispatch/heartbeat.ts b/packages/junior/src/chat/agent-dispatch/heartbeat.ts index 12308953d9..7e2110cbaf 100644 --- a/packages/junior/src/chat/agent-dispatch/heartbeat.ts +++ b/packages/junior/src/chat/agent-dispatch/heartbeat.ts @@ -1,6 +1,3 @@ -import { getDb } from "@/chat/db"; -import { convertSavedEventAutomationTriggers } from "@/chat/event-automations/trigger-conversion"; -import { getEventCatalog } from "@/chat/events/runtime-catalog"; import { runTimerWatchHeartbeat } from "@/chat/events/timers"; import { getPlugins } from "@/chat/plugins/agent-hooks"; import { logException, logInfo } from "@/chat/logging"; @@ -218,13 +215,6 @@ export async function runHeartbeat(args: { } catch (error) { logException(error, "scheduled_automations.heartbeat.failed"); } - try { - // Saved triggers with an identifier move to filters. Slice D of #2362 - // removes this step with the identifier column. - await convertSavedEventAutomationTriggers(getDb(), getEventCatalog()); - } catch (error) { - logException(error, "event_automation.trigger.conversion_failed"); - } try { await runTimerWatchHeartbeat({ nowMs: args.nowMs, queue }); } catch (error) { diff --git a/packages/junior/src/chat/event-automations/README.md b/packages/junior/src/chat/event-automations/README.md index bf84235bfa..31e23f126c 100644 --- a/packages/junior/src/chat/event-automations/README.md +++ b/packages/junior/src/chat/event-automations/README.md @@ -47,23 +47,29 @@ identifier when its plugin can turn the identifier into filters (`identifierMatch`). For example, `getsentry/junior#7` on pull request events becomes `repo` and `pullRequest` filters. `trigger-conversion.ts` owns the rule. A trigger converts only when the plugin gives the same filters for every -selected event, those filters agree with the saved filters, and the result -passes the check for a new trigger. Otherwise it keeps its identifier and -keeps running, and the trigger saves the reason in `identifierReason`. Each -edit that changes the trigger sets the reason again. A conversion never makes -a trigger broader. - -Each heartbeat checks at most 50 saved rows that have an identifier, no -saved reason, and the same identifier in the row and the trigger. It saves -each row with the revision that it read, under the row lock, and records a -version with no editor. A row that changed after the read is left for the next -heartbeat. A row that cannot convert keeps its identifier, stays active, and -saves the reason, so later heartbeats skip it. Examples are a Sentry project -trigger for metric alerts, because a metric alert can leave out `projects`, -and a row whose plugin is not enabled. The trigger description shows the -reason as a note, not a problem, in the dashboard, Slack details, and -`listEventAutomations`. Slice D of #2362 decides what happens to these rows -before the identifier column goes away. Saved versions keep their old triggers, and the version schema still reads +selected event, each saved filter matches the identifier's value, and the +result passes the check for a new trigger. A saved list filter that has the +identifier's value becomes that one value. A conversion never makes a trigger +broader. + +A write is rejected when a saved filter never matches the identifier's value, +because the trigger could never run. In other cases, the trigger keeps its +identifier and saves the plain reason in `identifierReason`. An example is a +Sentry identifier. The Sentry plugin reads its org from each webhook URL and +has no saved org for the install, so it cannot check the org in an identifier. +A wrong org would make a dead trigger run, so the plugin gives no +`identifierMatch`. Each edit that changes the trigger sets +the reason again. The trigger description shows the reason as a note, not a +problem, in the dashboard, Slack details, and `listEventAutomations`. + +The update tool changes `match` key by key. A value sets a filter, `null` +removes it, and a filter that the update leaves out keeps its saved value. +After a conversion the scope lives in `match`, so replacing the whole object +would make the trigger broader. The dashboard sends the whole trigger from +its form, which shows every saved filter. + +Saved rows keep their identifiers until a later change converts them. Saved +versions keep their old triggers, and the version schema still reads `identifier` and `resourceType`. `searchEventTypes` exposes the same enabled plugin catalog used by diff --git a/packages/junior/src/chat/event-automations/store.ts b/packages/junior/src/chat/event-automations/store.ts index 63411c0e6e..80aba23238 100644 --- a/packages/junior/src/chat/event-automations/store.ts +++ b/packages/junior/src/chat/event-automations/store.ts @@ -7,18 +7,7 @@ import { type EventMatch, type User, } from "@sentry/junior-plugin-api"; -import { - and, - asc, - desc, - eq, - inArray, - isNotNull, - isNull, - ne, - or, - sql, -} from "drizzle-orm"; +import { and, asc, desc, eq, inArray, isNull, ne, or, sql } from "drizzle-orm"; import type { JuniorDatabase } from "@/db/db"; import { juniorAutomations } from "@/db/schema/automations"; import { @@ -295,38 +284,6 @@ export async function listDeletedEventAutomationsCreatedBy( return rows.map(parseEventAutomationRow); } -/** - * List up to `limit` live event automations whose trigger still has an - * identifier and no saved reason to keep it. Skip a row whose identifier - * column and saved trigger disagree, because ingest never runs it and a save - * could make it broad. Skip a row that does not parse; ingest logs it when an - * event reaches it. - */ -export async function listEventAutomationsToConvert( - db: JuniorDatabase, - limit: number, -): Promise { - const rows = await db - .select() - .from(juniorAutomations) - .where( - and( - activeEventAutomationWhere(), - isNotNull(juniorAutomations.identifier), - sql`${juniorAutomations.record}->'trigger'->>'identifier' = ${juniorAutomations.identifier}`, - sql`${juniorAutomations.record}->'trigger'->'identifierReason' is null`, - ), - ) - .orderBy(asc(juniorAutomations.createdAtMs), asc(juniorAutomations.id)) - .limit(limit); - return rows.flatMap((row) => { - const parsed = eventAutomationSchema.safeParse( - eventAutomationRowInput(row), - ); - return parsed.success ? [withRowFields(row, parsed.data)] : []; - }); -} - /** * Select live rows whose identifier is one of these, and rows with no * identifier. A row with no identifier runs for every resource of its diff --git a/packages/junior/src/chat/event-automations/tool-support.ts b/packages/junior/src/chat/event-automations/tool-support.ts index de7daef706..5d581dabd6 100644 --- a/packages/junior/src/chat/event-automations/tool-support.ts +++ b/packages/junior/src/chat/event-automations/tool-support.ts @@ -133,8 +133,9 @@ export function registeredEventAutomationTriggerSchema(catalog: EventCatalog) { * The identifier gets its plugin's convention. A trigger with an identifier is * saved with no identifier when its plugin can turn the identifier into * filters that select the same events. Otherwise it is saved with the reason - * that it keeps the identifier, so each edit sets the reason again. Events - * and list values lose repeats. + * that it keeps the identifier, so each edit sets the reason again. A trigger + * whose filter never matches the events of its identifier is rejected, + * because it would never run. Events and list values lose repeats. */ export function requireEventAutomationTrigger( catalog: EventCatalog, @@ -176,7 +177,13 @@ export function requireEventAutomationTrigger( events, ...(match ? { match } : undefined), }; - return triggerWithoutIdentifier(catalog, trigger).trigger; + const conversion = triggerWithoutIdentifier(catalog, trigger); + if (!conversion.converted && conversion.conflict) { + throw new ToolInputError( + `The "${conversion.conflict}" filter never matches the events of identifier "${trigger.identifier}", so this trigger would never run. Change or remove the "${conversion.conflict}" filter.`, + ); + } + return conversion.trigger; } /** Require the active Slack authority used for event-automation management. */ @@ -252,8 +259,11 @@ export async function writableEventAutomation( /** A saved trigger in words, with notes about how Junior saved it. */ export type EventAutomationTriggerDescription = EventTriggerDescription & { - /** Plain facts about a trigger that still works, such as a kept identifier. */ - notes: string[]; + /** + * Plain facts about a trigger that still works, such as a kept + * identifier. Missing when there are none. + */ + notes?: string[]; }; /** @@ -268,7 +278,9 @@ export function describeEventAutomationTrigger( ): EventAutomationTriggerDescription { return { ...describeRegisteredEventTrigger(automationEventCatalog(catalog), trigger), - notes: trigger.identifierReason ? [trigger.identifierReason] : [], + ...(trigger.identifierReason + ? { notes: [trigger.identifierReason] } + : undefined), }; } @@ -298,7 +310,7 @@ export function compactEventAutomation( ...(task.trigger.match ? { match: task.trigger.match } : undefined), available: problems.length === 0, ...(problems.length > 0 ? { problems } : undefined), - ...(notes.length > 0 ? { notes } : undefined), + ...(notes ? { notes } : undefined), }, credentialMode: task.credentialMode, isCreator: task.createdBy.slackUserId === requesterSlackUserId, diff --git a/packages/junior/src/chat/event-automations/trigger-conversion.ts b/packages/junior/src/chat/event-automations/trigger-conversion.ts index f3f02c28fd..3a08b656e9 100644 --- a/packages/junior/src/chat/event-automations/trigger-conversion.ts +++ b/packages/junior/src/chat/event-automations/trigger-conversion.ts @@ -12,34 +12,48 @@ import { stableEventMatchKey, type EventMatch, } from "@sentry/junior-plugin-api"; -import { - AutomationConflictError, - automationRevision, -} from "@/chat/automations/revision"; import { automationEventCatalog, registeredEventTriggerProblems, type EventCatalog, } from "@/chat/events/catalog"; -import { logException, logInfo } from "@/chat/logging"; -import type { JuniorDatabase } from "@/db/db"; -import { listEventAutomationsToConvert, saveEventAutomation } from "./store"; import type { EventAutomation } from "./types"; type EventAutomationTrigger = EventAutomation["trigger"]; /** * A trigger with no identifier, or the trigger with its identifier and the - * reason that it keeps it. + * reason that it keeps it. `conflict` names a saved filter that never matches + * the events of the identifier, so the trigger can never run. */ export type TriggerConversion = | { converted: true; trigger: EventAutomationTrigger } - | { converted: false; trigger: EventAutomationTrigger; reason: string }; + | { + converted: false; + trigger: EventAutomationTrigger; + reason: string; + conflict?: string; + }; -function sameValue(left: EventMatch[string], right: EventMatch[string]) { +/** + * Return whether every event of the identifier matches the saved filter + * value. The identifier gives one exact value. A saved list matches it when + * the list has that value. + */ +function savedValueMatches( + saved: EventMatch[string], + scope: EventMatch[string], +): boolean { + if ( + stableEventMatchKey({ value: saved }) === + stableEventMatchKey({ value: scope }) + ) { + return true; + } return ( - stableEventMatchKey({ value: left }) === - stableEventMatchKey({ value: right }) + Array.isArray(saved) && + !Array.isArray(scope) && + (saved as readonly unknown[]).includes(scope) ); } @@ -65,19 +79,21 @@ export function triggerWithoutIdentifier( ...rest } = trigger; if (identifier === undefined) return { converted: true, trigger }; - const keep = (why: string): TriggerConversion => { + const keep = (why: string, conflict?: string): TriggerConversion => { const reason = keptReason(trigger, why); return { converted: false, trigger: { ...trigger, identifierReason: reason }, reason, + ...(conflict ? { conflict } : undefined), }; }; const plugins = automationEventCatalog(catalog); const source = plugins[trigger.namespace]; if (!source) return keep("its plugin was not enabled when Junior checked it"); const toMatch = source.identifierMatch; - if (!toMatch) return keep("its source cannot use filters in its place"); + if (!toMatch) + return keep("its plugin cannot turn the identifier into filters"); let scope: EventMatch | undefined; for (const eventType of trigger.events) { const match = toMatch({ identifier, eventType }); @@ -89,8 +105,8 @@ export function triggerWithoutIdentifier( } for (const [key, value] of Object.entries(scope ?? {})) { const saved = trigger.match?.[key]; - if (saved !== undefined && !sameValue(saved, value)) { - return keep(`its saved ${key} filter has a different value`); + if (saved !== undefined && !savedValueMatches(saved, value)) { + return keep(`its ${key} filter never matches its events`, key); } } const match = { ...trigger.match, ...scope }; @@ -105,49 +121,3 @@ export function triggerWithoutIdentifier( if (problems.length > 0) return keep("no filters select the same events"); return { converted: true, trigger: { ...rest, match } }; } - -/** Most saved triggers that one heartbeat checks. */ -const CONVERSION_BATCH_LIMIT = 50; - -/** - * Check saved event automation triggers that have an identifier and no saved - * reason, at most `CONVERSION_BATCH_LIMIT` in one call. Save each one with - * no identifier when `triggerWithoutIdentifier` can convert it. Otherwise - * save the reason, so later calls skip it and people can see why. Each save - * locks the row, checks that the row did not change after it was read, and - * records a new version with no editor. A row that changed is left for the - * next call. Every trigger stays active. - */ -export async function convertSavedEventAutomationTriggers( - db: JuniorDatabase, - catalog: EventCatalog, -): Promise<{ converted: string[]; kept: string[] }> { - const converted: string[] = []; - const kept: string[] = []; - const tasks = await listEventAutomationsToConvert(db, CONVERSION_BATCH_LIMIT); - for (const task of tasks) { - try { - const conversion = triggerWithoutIdentifier(catalog, task.trigger); - const saved = await saveEventAutomation( - db, - { ...task, trigger: conversion.trigger }, - automationRevision(task), - ); - if (!saved) continue; - (conversion.converted ? converted : kept).push(task.id); - logInfo( - conversion.converted - ? "event_automation.trigger.converted" - : "event_automation.trigger.identifier_kept", - { "app.task.id": task.id, "app.task.type": "event" }, - ); - } catch (error) { - if (error instanceof AutomationConflictError) continue; - logException(error, "event_automation.trigger.conversion_failed", { - "app.task.id": task.id, - "app.task.type": "event", - }); - } - } - return { converted, kept }; -} diff --git a/packages/junior/src/chat/events/README.md b/packages/junior/src/chat/events/README.md index 3a2a2e3a83..fed63beb80 100644 --- a/packages/junior/src/chat/events/README.md +++ b/packages/junior/src/chat/events/README.md @@ -126,6 +126,7 @@ Watches route events back into an existing conversation. to the identifier, and no other. It returns nothing when no filters can do that. Core uses it to save event automation triggers with no identifier (`event-automations/trigger-conversion.ts`). Watches keep their identifier. + It is temporary. Slice D of #2362 removes it with the identifier column. - A match field may declare `case: "lower"` or `case: "upper"`. Core changes a filter value to that case when it saves a trigger or a Watch, so a value such as `getsentry/Junior` still matches lowercase event data. The plugin decides @@ -192,7 +193,8 @@ Watches route events back into an existing conversation. identifier again from the filters with the shared builder, and return nothing when it differs. Use `identifierMatchMismatches` from `@sentry/junior-testing` in the webhook test to prove that the filters - match the same events. + match the same events. Add identifiers that do not rebuild, such as a tag + that is not encoded, and check that they give no filters. 8. When `data` needs a lookup, call `neededMatchKeys` with the values that the plugin already has, such as `repo`. Core then skips triggers with no identifier whose filters on those values fail. diff --git a/packages/junior/src/chat/slack/automation-details.ts b/packages/junior/src/chat/slack/automation-details.ts index 9044b4e775..8866a572f9 100644 --- a/packages/junior/src/chat/slack/automation-details.ts +++ b/packages/junior/src/chat/slack/automation-details.ts @@ -85,7 +85,7 @@ function renderAutomationDetails( long: true, }, ); - if (trigger.notes.length > 0) { + if (trigger.notes) { fields.push({ ...textField("note", "Note", trigger.notes.join(" ")), long: true, diff --git a/packages/junior/src/chat/tools/update-event-automation.ts b/packages/junior/src/chat/tools/update-event-automation.ts index 6494b810a2..da9b391144 100644 --- a/packages/junior/src/chat/tools/update-event-automation.ts +++ b/packages/junior/src/chat/tools/update-event-automation.ts @@ -9,6 +9,7 @@ import { import { requireRequesterMention } from "@/chat/automations/requester-mention"; import { automationRevision } from "@/chat/automations/revision"; import { editEventAutomation } from "@/chat/event-automations/edit"; +import { eventMatchSchema, type EventMatch } from "@sentry/junior-plugin-api"; import { z } from "zod"; import { getDb } from "@/chat/db"; import { @@ -27,6 +28,31 @@ import { zodTool } from "@/chat/tool-support/zod-tool"; import { ToolInputError } from "@/chat/tools/execution/tool-input-error"; import type { ToolRuntimeContext } from "@/chat/tools/types"; +/** + * Filter changes for an update. A value sets the filter, and null removes + * it. A filter that is not named keeps its saved value, so an update cannot + * drop the filters that limit a trigger by leaving them out. + */ +const matchChangesSchema = z + .record(z.string(), z.union([eventMatchSchema.valueType, z.null()])) + .describe( + "Filter changes. A value sets the filter, null removes it, and a filter that is left out keeps its saved value. Use the matchFields of the resource type. A single value must equal the event data. A list means the event value may be any one entry.", + ); + +/** Apply filter changes to the saved filters, key by key. */ +function changedMatch( + saved: EventMatch | undefined, + changes: z.output | undefined, +): EventMatch | undefined { + if (changes === undefined) return saved; + const match: EventMatch = { ...saved }; + for (const [key, value] of Object.entries(changes)) { + if (value === null) delete match[key]; + else match[key] = value; + } + return Object.keys(match).length > 0 ? match : undefined; +} + /** Create the core tool that updates an event automation. */ export function createUpdateEventAutomationTool( context: ToolRuntimeContext, @@ -42,13 +68,14 @@ export function createUpdateEventAutomationTool( }, executionMode: "sequential", description: - "Update the instruction, registered trigger, credential use, or status of an event automation. In trigger, leaving out identifier, resourceType, or match keeps the saved value. Only identifier null removes the identifier, and only match {} removes all filters. A new match replaces all saved filters, so repeat each saved filter to keep, such as repo, projects, or channelId.", + "Update the instruction, registered trigger, credential use, or status of an event automation. In trigger, leaving out identifier, resourceType, or match keeps the saved value, and only null removes the identifier. match changes only the filters that it names: a value sets that filter, null removes it, and a filter that it leaves out keeps its saved value.", inputSchema: z .object({ automationId: z.string().min(1), title: automationTitleSchema.optional(), instruction: automationInstructionToolSchema.nullable().optional(), trigger: registeredEventAutomationTriggerSchema(catalog) + .extend({ match: matchChangesSchema.optional() }) .nullable() .optional(), outcomes: z @@ -139,13 +166,16 @@ export function createUpdateEventAutomationTool( { title: input.title, instruction: input.instruction ?? undefined, - // An omitted identifier, resource type, or match keeps the saved + // An omitted identifier, resource type, or filter keeps the saved // one, so a trigger cannot get broader by omission. Only an - // explicit null drops the identifier, and only `{}` drops filters. + // explicit null drops the identifier or a filter. trigger: input.trigger ? { ...input.trigger, - match: input.trigger.match ?? current.trigger.match, + match: changedMatch( + current.trigger.match, + input.trigger.match, + ), identifier: input.trigger.identifier === undefined ? current.trigger.identifier diff --git a/packages/junior/tests/integration/any-resource-event-automations.test.ts b/packages/junior/tests/integration/any-resource-event-automations.test.ts index 60f4d3c1f8..c693b1e2a2 100644 --- a/packages/junior/tests/integration/any-resource-event-automations.test.ts +++ b/packages/junior/tests/integration/any-resource-event-automations.test.ts @@ -1,18 +1,14 @@ import { githubPlugin } from "@sentry/junior-github"; -import type { EventMatch } from "@sentry/junior-plugin-api"; import { eq, sql } from "drizzle-orm"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createApp, defineJuniorPlugins } from "@/app"; -import { runHeartbeat } from "@/chat/agent-dispatch/heartbeat"; import { getDispatchRecord } from "@/chat/agent-dispatch/store"; import { migrateSchema } from "@/chat/conversations/sql/migrations"; import { getConversationStore } from "@/chat/db"; import { ingestEventAutomations } from "@/chat/event-automations/ingest"; -import { listAutomationVersions } from "@/chat/automations/versions"; import { getEventCatalog } from "@/chat/events/runtime-catalog"; import { disconnectStateAdapter } from "@/chat/state/adapter"; import { createEventAutomationTool } from "@/chat/tools/create-event-automation"; -import { createListEventAutomationsTool } from "@/chat/tools/list-event-automations"; import { createUpdateEventAutomationTool } from "@/chat/tools/update-event-automation"; import type { EventAutomationRecord } from "@/chat/event-automations/types"; import { juniorAutomations } from "@/db/schema/automations"; @@ -207,147 +203,40 @@ describe("event automations with no identifier", () => { // A new trigger with an identifier is saved as the filters that select // the same events, so the tools stop writing identifiers. - const one = await create( - { - ...trigger, - identifier: "getsentry/junior#7", - resourceType: "pull_request", - label: "getsentry/junior#7", - }, - "one-pull-request", - ); + const onePullRequestTrigger = { + ...trigger, + identifier: "getsentry/junior#7", + resourceType: "pull_request", + label: "getsentry/junior#7", + }; + const one = await create(onePullRequestTrigger, "one-pull-request"); const onePullRequest = { identifier: null, resourceType: null, match: { repo: "getsentry/junior", pullRequest: 7 }, }; expect(one.automation.trigger).toMatchObject(onePullRequest); - - // An update that leaves out identifier, resourceType, and match keeps - // them, so it cannot make a trigger broad by omission. - const update = (next: Record, toolCallId: string) => - execute( - createUpdateEventAutomationTool(context(), catalog), - { automationId: one.automation.id, trigger: next }, - toolCallId, - ) as Promise<{ - automation: { trigger: Record }; - }>; - const edited = { ...trigger, label: "Opened getsentry/junior#7" }; - await expect(update(edited, "keep-filters")).resolves.toMatchObject({ - automation: { trigger: onePullRequest }, - }); - // Only an explicit empty match drops filters, and then the trigger - // needs a repo. + // A saved list that has the identifier's value becomes that value. A + // filter that never matches the identifier would never run, so the + // write fails and names the filter. await expect( - update({ ...edited, match: {} }, "drop-filters"), - ).rejects.toThrow('This trigger needs a "repo" filter (match.repo)'); + create( + { ...onePullRequestTrigger, match: { pullRequest: [7, 8] } }, + "list-filter", + ), + ).resolves.toMatchObject({ automation: { trigger: onePullRequest } }); await expect( - update( - { ...edited, match: { repo: "getsentry/junior" } }, - "repo-filter", + create( + { ...onePullRequestTrigger, match: { pullRequest: 5 } }, + "conflicting-filter", ), - ).resolves.toMatchObject({ - automation: { trigger: { match: { repo: "getsentry/junior" } } }, - }); + ).rejects.toThrow( + 'The "pullRequest" filter never matches the events of identifier "getsentry/junior#7"', + ); - // Rows saved before this change still name one resource. The heartbeat - // converts each one under its row lock and saves a version with no - // editor. A row that cannot convert without matching more events keeps - // its identifier, saves the reason, and keeps running. - const savedPullRequest = (id: string, match?: EventMatch) => ({ - ...row!, - id, - identifier: "getsentry/junior#8", - record: { - ...record, - trigger: { - ...record.trigger, - identifier: "getsentry/junior#8", - resourceType: "pull_request", - match, - }, - }, - }); - await fixture.sql - .db() - .insert(juniorAutomations) - .values([ - savedPullRequest("evt_saved_pull_request"), - savedPullRequest("evt_saved_list_filter", { pullRequest: [8, 9] }), - // Its saved trigger has no identifier, so ingest never runs it. - // Conversion must not save it as a broad row. - { - ...savedPullRequest("evt_saved_column_only"), - record: { - ...record, - trigger: { - ...record.trigger, - match: { repo: "getsentry/junior" }, - }, - }, - }, - ]); - const savedRows = async () => - Object.fromEntries( - ( - await fixture.sql - .db() - .select() - .from(juniorAutomations) - .where(sql`${juniorAutomations.id} like 'evt_saved_%'`) - ).map((saved) => [saved.id, saved]), - ); - const versions = (id: string) => - listAutomationVersions(fixture.sql.db(), id, 5); - await runHeartbeat({ nowMs: Date.now(), conversationWorkQueue: queue }); - const reason = - "Kept its exact pull request because its saved pullRequest filter has a different value."; - const rows = await savedRows(); - expect(rows.evt_saved_pull_request?.identifier).toBeNull(); - expect(await versions("evt_saved_pull_request")).toMatchObject([ - { - editedBy: null, - definition: { - trigger: { - events: ["pull_request.opened"], - match: { repo: "getsentry/junior", pullRequest: 8 }, - }, - }, - }, - ]); - expect(rows.evt_saved_list_filter?.identifier).toBe("getsentry/junior#8"); - expect(rows.evt_saved_list_filter?.status).toBe("active"); - expect(await versions("evt_saved_list_filter")).toMatchObject([ - { - editedBy: null, - definition: { trigger: { identifierReason: reason } }, - }, - ]); - // The saved reason is a note on the trigger, not a problem. Listing - // reads every row of the team, so remove the row that does not parse. - await fixture.sql - .db() - .delete(juniorAutomations) - .where(eq(juniorAutomations.id, "evt_bad_record")); - const listed = (await execute( - createListEventAutomationsTool(context(), catalog), - {}, - "list-kept", - )) as { - automations: { id: string; trigger: Record }[]; - }; - expect( - listed.automations.find(({ id }) => id === "evt_saved_list_filter") - ?.trigger, - ).toMatchObject({ available: true, notes: [reason] }); - // The next heartbeat does not save the kept row again. - await runHeartbeat({ nowMs: Date.now(), conversationWorkQueue: queue }); - expect(await versions("evt_saved_list_filter")).toHaveLength(1); - expect(await versions("evt_saved_column_only")).toEqual([]); - // Both rows run once for a pull request event, and not for another - // pull request. - const dispatchedIds = async (pullRequest: number, eventKey: string) => { + // The converted trigger runs for its pull request and not for another + // one in the same repository. + const dispatchedFor = async (pullRequest: number) => { const before = queue.sentRecords().length; for (const identifier of [ `getsentry/junior#${pullRequest}`, @@ -355,7 +244,7 @@ describe("event automations with no identifier", () => { ]) { await ingestEventAutomations( { - eventKey, + eventKey: `github:one-pull-request-${pullRequest}`, eventType: "pull_request.opened", occurredAtMs: Date.now(), namespace: "github", @@ -366,22 +255,58 @@ describe("event automations with no identifier", () => { { queue, teamId }, ); } - const ids = []; + let count = 0; for (const sentRecord of queue.sentRecords().slice(before)) { const dispatched = await getDispatchRecord( sentRecord.conversationId.replace(/^agent-dispatch:/, ""), ); - ids.push(dispatched?.metadata?.eventAutomationId); + if (dispatched?.metadata?.eventAutomationId === one.automation.id) { + count++; + } } - return ids.filter((id) => id?.startsWith("evt_saved_")).sort(); + return count; }; - expect(await dispatchedIds(8, "github:opened-8")).toEqual([ - "evt_saved_list_filter", - "evt_saved_pull_request", - ]); - expect(await dispatchedIds(9, "github:opened-9")).toEqual([]); + expect(await dispatchedFor(7)).toBe(1); + expect(await dispatchedFor(8)).toBe(0); + + // An update changes only the filters that it names. A filter that it + // leaves out keeps its saved value, so the scope stays. Only null + // removes a filter. + const update = (next: Record, toolCallId: string) => + execute( + createUpdateEventAutomationTool(context(), catalog), + { automationId: one.automation.id, trigger: next }, + toolCallId, + ) as Promise<{ + automation: { trigger: Record }; + }>; + const edited = { ...trigger, label: "Opened getsentry/junior#7" }; + await expect(update(edited, "keep-filters")).resolves.toMatchObject({ + automation: { trigger: onePullRequest }, + }); + const notDraft = { ...onePullRequest.match, isDraft: false }; + await expect( + update({ ...edited, match: { isDraft: false } }, "add-filter"), + ).resolves.toMatchObject({ + automation: { trigger: { match: notDraft } }, + }); + await expect( + update({ ...edited, match: {} }, "no-filter-change"), + ).resolves.toMatchObject({ + automation: { trigger: { match: notDraft } }, + }); + await expect( + update({ ...edited, match: { repo: null } }, "drop-repo"), + ).rejects.toThrow('This trigger needs a "repo" filter (match.repo)'); + const repoOnly = await update( + { ...edited, match: { pullRequest: null, isDraft: null } }, + "drop-filters", + ); + expect(repoOnly.automation.trigger.match).toEqual({ + repo: "getsentry/junior", + }); } finally { vi.unstubAllEnvs(); } - }); + }, 20_000); }); diff --git a/packages/junior/tests/integration/api/automation-edits.test.ts b/packages/junior/tests/integration/api/automation-edits.test.ts index 57b120bbd2..4365a2b063 100644 --- a/packages/junior/tests/integration/api/automation-edits.test.ts +++ b/packages/junior/tests/integration/api/automation-edits.test.ts @@ -909,7 +909,6 @@ describe("Automation edit API", () => { events: ["issue closed"], filters: ["retainedCondition is one or two"], problems: ["The plugin for this source is not enabled."], - notes: [], }; expect( versions.map((version) => @@ -924,7 +923,6 @@ describe("Automation edit API", () => { "pullRequest is 42", ], problems: [], - notes: [], }, saved, saved, @@ -992,7 +990,6 @@ describe("Automation edit API", () => { events: ["pull request opened"], filters: ["repo is getsentry/junior"], problems: [], - notes: [], }, }); } finally { diff --git a/packages/junior/tests/integration/api/automations.test.ts b/packages/junior/tests/integration/api/automations.test.ts index 23b3d2187b..b0e05f9101 100644 --- a/packages/junior/tests/integration/api/automations.test.ts +++ b/packages/junior/tests/integration/api/automations.test.ts @@ -407,7 +407,6 @@ describe("Automations API", () => { events: ["issue closed"], filters: ["action is closed or resolved"], problems: ["The plugin for this source is not enabled."], - notes: [], }, }), expect.objectContaining({ diff --git a/packages/junior/tests/integration/event-automations.test.ts b/packages/junior/tests/integration/event-automations.test.ts index 71ae89495a..03dfcb09b5 100644 --- a/packages/junior/tests/integration/event-automations.test.ts +++ b/packages/junior/tests/integration/event-automations.test.ts @@ -725,8 +725,15 @@ describe("event automations", () => { expect(listed.automations.map((task) => task.id)).toEqual([ created.automation.id, ]); + // This source cannot turn identifiers into filters. The trigger still + // works, so the reason is a note and not a problem. expect(listed.automations[0]).toMatchObject({ - trigger: { available: true }, + trigger: { + available: true, + notes: [ + "Kept its exact pull request because its plugin cannot turn the identifier into filters.", + ], + }, }); const otherChannel = (await execute( createListEventAutomationsTool(context("U999", "COTHER"), EVENT_CATALOG), diff --git a/packages/junior/tests/integration/slack/channel-message-automations.test.ts b/packages/junior/tests/integration/slack/channel-message-automations.test.ts index e1156cf3ba..44e850439b 100644 --- a/packages/junior/tests/integration/slack/channel-message-automations.test.ts +++ b/packages/junior/tests/integration/slack/channel-message-automations.test.ts @@ -4,7 +4,6 @@ import { getDispatchRecord } from "@/chat/agent-dispatch/store"; import { getConversationStore } from "@/chat/db"; import { migrateSchema } from "@/chat/conversations/sql/migrations"; import { getEventAutomation } from "@/chat/event-automations/store"; -import { convertSavedEventAutomationTriggers } from "@/chat/event-automations/trigger-conversion"; import { getEventCatalog } from "@/chat/events/runtime-catalog"; import { disconnectStateAdapter, getStateAdapter } from "@/chat/state/adapter"; import { createEventAutomationTool } from "@/chat/tools/create-event-automation"; @@ -139,26 +138,6 @@ describe("Slack channel-message automations", () => { }); expect(created?.trigger.identifier).toBeUndefined(); - // A row saved before channel filters names the channel with its - // identifier. The heartbeat step turns it into the same channel filter. - const { match: _match, ...unfiltered } = created!.trigger; - await fixture.sql - .db() - .update(juniorAutomations) - .set({ - identifier: "C123", - record: sql`jsonb_set(${juniorAutomations.record}, '{trigger}', ${JSON.stringify( - { ...unfiltered, identifier: "C123", resourceType: "channel" }, - )}::jsonb)`, - }) - .where(eq(juniorAutomations.id, automation.id)); - expect( - await convertSavedEventAutomationTriggers(fixture.sql.db(), catalog), - ).toEqual({ converted: [automation.id], kept: [] }); - expect( - (await getEventAutomation(fixture.sql.db(), automation.id))?.trigger, - ).toEqual(created?.trigger); - // A thread reply, a bot post, and a message in another channel do not run it. expect( ( @@ -228,6 +207,25 @@ describe("Slack channel-message automations", () => { trigger: trigger("current_channel", { userId: "U777" }), }, )) as { automation: { id: string } }; + // A row saved before channel filters names the channel with its + // identifier, and it still runs for its own channel. + const created = await getEventAutomation(fixture.sql.db(), automation.id); + const { match, ...unfiltered } = created!.trigger; + await fixture.sql + .db() + .update(juniorAutomations) + .set({ + identifier: "C123", + record: sql`jsonb_set(${juniorAutomations.record}, '{trigger}', ${JSON.stringify( + { + ...unfiltered, + identifier: "C123", + resourceType: "channel", + match: { userId: match?.userId }, + }, + )}::jsonb)`, + }) + .where(eq(juniorAutomations.id, automation.id)); // A post from another bot does not run it. await postChannelMessage({ diff --git a/packages/junior/tests/unit/events/catalog.test.ts b/packages/junior/tests/unit/events/catalog.test.ts index 72e9b5f01c..19063f3c7f 100644 --- a/packages/junior/tests/unit/events/catalog.test.ts +++ b/packages/junior/tests/unit/events/catalog.test.ts @@ -1,3 +1,4 @@ +import type { EventMatch } from "@sentry/junior-plugin-api"; import { describe, expect, it } from "vitest"; import { CORE_EVENT_NAMESPACE, @@ -138,4 +139,53 @@ describe("event catalog", () => { ).problems, ).toEqual(["The plugin for this source is not enabled."]); }); + + it("converts an identifier only when every event gets the same filters", () => { + const fields = { kind: "string", description: "Item id." } as const; + const items: EventCatalog = { + demo: eventCatalogEntry({ + resourceTypes: [ + { + type: "item", + supportedEvents: ["first.opened", "second.opened"], + matchFields: { first: fields, second: fields }, + }, + ], + events: { + "first.opened": { reach: "all" }, + "second.opened": { reach: "all" }, + }, + identifierMatch: ({ identifier, eventType }): EventMatch => + eventType === "first.opened" + ? { first: identifier } + : { second: identifier }, + }), + }; + const trigger = (events: string[]) => + requireEventAutomationTrigger(items, { + namespace: "demo", + resourceType: "item", + identifier: "x", + label: "Item x", + events, + }); + + expect(trigger(["first.opened"])).toEqual({ + namespace: "demo", + label: "Item x", + events: ["first.opened"], + match: { first: "x" }, + }); + // One set of filters cannot select the same events for both, so the + // trigger keeps its identifier and says why. + expect(trigger(["first.opened", "second.opened"])).toEqual({ + namespace: "demo", + resourceType: "item", + identifier: "x", + identifierReason: + "Kept its exact item because its events need different filters.", + label: "Item x", + events: ["first.opened", "second.opened"], + }); + }); }); From 21e5642f5461d2965641dbf9924e04c126339c62 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:15:45 +0000 Subject: [PATCH 4/6] feat(automations): Read versions that Junior saved The version schema accepts editedBy "junior" and the dashboard shows Junior as the editor. Nothing writes it yet. The reader ships first, so the follow-up that converts saved triggers can roll back to this release and keep version history readable. Refs #2362 Co-Authored-By: David Cramer --- .../junior-dashboard/e2e/automation-editor.spec.ts | 6 +++++- .../pages/automations/AutomationVersionsPage.tsx | 13 ++++++++----- packages/junior/src/api/schema/automation.ts | 10 ++++++++-- .../junior/src/chat/event-automations/README.md | 5 ++++- .../tests/integration/api/automation-edits.test.ts | 10 +++++----- 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/packages/junior-dashboard/e2e/automation-editor.spec.ts b/packages/junior-dashboard/e2e/automation-editor.spec.ts index 31ac8afcd0..41466e6651 100644 --- a/packages/junior-dashboard/e2e/automation-editor.spec.ts +++ b/packages/junior-dashboard/e2e/automation-editor.spec.ts @@ -265,7 +265,8 @@ test("makes an earlier version active from version history", async ({ kind: "scheduled", version: 1, createdAt: "2026-07-28T16:00:00.000Z", - editedBy: null, + // A version that Junior saved names Junior as its editor. + editedBy: "junior", definition, }, ]; @@ -294,6 +295,9 @@ test("makes an earlier version active from version history", async ({ await page.getByRole("link", { name: "Version history" }).click(); await expect(page).toHaveURL(/\/automations\/scheduled-1\/versions/); await expect(page.getByText("Changed instruction")).toBeVisible(); + await expect( + page.getByRole("listitem", { name: "Version 1" }).getByText(/^Junior · /), + ).toBeVisible(); await screenshot(page, "automation-versions"); await page .getByRole("button", { name: "Make version 1 active", exact: true }) diff --git a/packages/junior-dashboard/src/client/pages/automations/AutomationVersionsPage.tsx b/packages/junior-dashboard/src/client/pages/automations/AutomationVersionsPage.tsx index 3fb4a3e625..d300acdb02 100644 --- a/packages/junior-dashboard/src/client/pages/automations/AutomationVersionsPage.tsx +++ b/packages/junior-dashboard/src/client/pages/automations/AutomationVersionsPage.tsx @@ -232,11 +232,14 @@ function VersionRow(props: { version: ListedAutomationVersion; }) { const { automation, version } = props; - const editor = version.editedBy - ? (version.editedBy.fullName ?? - version.editedBy.userName ?? - version.editedBy.slackUserId) - : "Editor unknown"; + const editor = + version.editedBy === "junior" + ? "Junior" + : version.editedBy + ? (version.editedBy.fullName ?? + version.editedBy.userName ?? + version.editedBy.slackUserId) + : "Editor unknown"; const changes = props.previous ? changedFields(props.previous, version) : undefined; diff --git a/packages/junior/src/api/schema/automation.ts b/packages/junior/src/api/schema/automation.ts index 8ce8aeaeae..a16e6a98fd 100644 --- a/packages/junior/src/api/schema/automation.ts +++ b/packages/junior/src/api/schema/automation.ts @@ -241,8 +241,14 @@ const automationDefinitionFields = { const automationVersionFields = { version: z.number().int().positive(), createdAt: z.string().datetime(), - /** Null for versions saved by the upgrade, because the editor is unknown. */ - editedBy: eventAutomationPrincipalSchema.nullable(), + /** + * The person who saved the version, or `"junior"` when Junior changed the + * trigger itself. Null for versions saved by the upgrade, because the + * editor is unknown. + */ + editedBy: z + .union([eventAutomationPrincipalSchema, z.literal("junior")]) + .nullable(), }; const scheduledAutomationVersionSchema = z diff --git a/packages/junior/src/chat/event-automations/README.md b/packages/junior/src/chat/event-automations/README.md index 31e23f126c..88e090dd05 100644 --- a/packages/junior/src/chat/event-automations/README.md +++ b/packages/junior/src/chat/event-automations/README.md @@ -70,7 +70,10 @@ its form, which shows every saved filter. Saved rows keep their identifiers until a later change converts them. Saved versions keep their old triggers, and the version schema still reads -`identifier` and `resourceType`. +`identifier` and `resourceType`. The version schema also reads +`editedBy: "junior"` for a version that Junior saves when it changes a trigger +itself. Nothing writes it yet. The reader comes first, so a release that +writes it can roll back to this one and keep version history readable. `searchEventTypes` exposes the same enabled plugin catalog used by watches and event automations. Create and update tools accept only diff --git a/packages/junior/tests/integration/api/automation-edits.test.ts b/packages/junior/tests/integration/api/automation-edits.test.ts index 4365a2b063..99a6e469da 100644 --- a/packages/junior/tests/integration/api/automation-edits.test.ts +++ b/packages/junior/tests/integration/api/automation-edits.test.ts @@ -544,7 +544,7 @@ describe("Automation edit API", () => { expect( versions.map(({ version, editedBy, definition }) => [ version, - editedBy?.slackUserId, + (editedBy as { slackUserId: string } | null)?.slackUserId, definition.instruction, definition.title, definition.credentialMode, @@ -635,10 +635,10 @@ describe("Automation edit API", () => { expect( afterReader.versions .slice(0, 2) - .map(({ version, editedBy }) => [version, editedBy?.slackUserId]), - ).toEqual([ - [9, "U456"], - [8, "U123"], + .map(({ version, editedBy }) => [version, editedBy]), + ).toMatchObject([ + [9, { slackUserId: "U456" }], + [8, { slackUserId: "U123" }], ]); expect(afterReader.activeVersion).toBe(9); expect( From e9711e4c3d3e27f77f0216adceda258263a7e68e Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:31:09 +0000 Subject: [PATCH 5/6] test(evals): Check the saved repo-wide trigger, not the call input A repository identifier and a repo filter now save the same trigger with no identifier, so the eval checks the saved trigger. Refs #2362 --- .../coding/event-automations/management.eval.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/junior-evals/evals/integration/coding/event-automations/management.eval.ts b/packages/junior-evals/evals/integration/coding/event-automations/management.eval.ts index 9935a980d4..b826ee5fa4 100644 --- a/packages/junior-evals/evals/integration/coding/event-automations/management.eval.ts +++ b/packages/junior-evals/evals/integration/coding/event-automations/management.eval.ts @@ -180,11 +180,15 @@ describe("Event automation management", () => { const creates = completedToolCalls("createEventAutomation", conversation); expect(creates).toHaveLength(1); - expect(creates[0]!.input).toMatchObject({ - trigger: { - namespace: "github", - identifier: "getsentry/junior", - resourceType: "repository", + // Check the saved trigger, not the call input. A repository identifier + // and a repo filter save the same trigger. + expect(toolOutput(creates[0]!)).toMatchObject({ + automation: { + trigger: { + namespace: "github", + identifier: null, + match: { repo: "getsentry/junior" }, + }, }, }); expect(new Set(triggerEvents(creates[0]!))).toEqual( From 9987344f5cc62c798a7f284d9a5d273c8be8d5a9 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:35:23 +0000 Subject: [PATCH 6/6] test(evals): Check the saved issue trigger after an update, not the call input --- .../event-automations/management.eval.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/junior-evals/evals/integration/coding/event-automations/management.eval.ts b/packages/junior-evals/evals/integration/coding/event-automations/management.eval.ts index b826ee5fa4..182694fec9 100644 --- a/packages/junior-evals/evals/integration/coding/event-automations/management.eval.ts +++ b/packages/junior-evals/evals/integration/coding/event-automations/management.eval.ts @@ -272,13 +272,18 @@ describe("Event automation management", () => { removal, ); expect(updates).toHaveLength(1); - expect(updates[0]!.input).toMatchObject({ - automationId: id, - trigger: { - events: ["issue.reopened"], - identifier: "getsentry/junior#208", - namespace: "github", - resourceType: "issue", + expect(updates[0]!.input).toMatchObject({ automationId: id }); + // Check the saved trigger, not the call input. The update may omit the + // identifier to keep it, and the issue identifier is saved as filters. + expect(toolOutput(updates[0]!)).toMatchObject({ + automation: { + id, + trigger: { + events: ["issue.reopened"], + identifier: null, + match: { repo: "getsentry/junior", issue: 208 }, + namespace: "github", + }, }, }); expect(