feat(alert): add alert list/view commands with pagination fixes#579
Open
betegon wants to merge 25 commits into
Open
feat(alert): add alert list/view commands with pagination fixes#579betegon wants to merge 25 commits into
betegon wants to merge 25 commits into
Conversation
Adds two new list commands under `sentry alert`: - `sentry alert issues list` — list issue alert rules for one or more projects, with full multi-target resolution (DSN auto-detect, org-all, project-search, explicit org/project), compound cursor pagination, Phase 1 + Phase 2 budget redistribution, and graceful partial failure handling - `sentry alert metrics list` — list metric alert rules for an org Both commands support --json, --limit, --web, and -c next/prev pagination.
…ed lib Moves duplicated code from issue/list.ts and alert/issues/list.ts into shared modules so both commands use the same implementation: - lib/db/pagination.ts: exports CURSOR_SEP, encodeCompoundCursor, decodeCompoundCursor, buildMultiTargetContextKey - lib/resolve-target.ts: exports resolveTargetsFromParsedArg — handles all four target modes (auto-detect, explicit, org-all, project-search) with options for enrichProjectIds and checkIssueShortId (issue-list-specific) issue/list.ts drops ~260 lines of local duplicates; alert/issues/list.ts never needs to define them in the first place.
Contributor
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Dashboard
Init
Other
Bug Fixes 🐛Dashboard
Event
Other
Documentation 📚
Internal Changes 🔧Coverage
Event
Other
Other
🤖 This preview updates automatically when you update the PR. |
Contributor
|
Contributor
Codecov Results 📊❌ Patch coverage is 60.20%. Project has 4669 uncovered lines. Files with missing lines (21)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 81.96% 81.08% -0.88%
==========================================
Files 329 347 +18
Lines 23749 24673 +924
Branches 15502 16162 +660
==========================================
+ Hits 19463 20004 +541
- Misses 4286 4669 +383
- Partials 1643 1781 +138Generated by Codecov Action |
…List Both alert/issues/list and alert/metrics/list were implementing their own 4-mode target dispatch and compound cursor machinery from scratch, bypassing the shared dispatchOrgScopedList infrastructure every other list command uses. Replace with the standard pattern: - dispatchOrgScopedList with ListCommandMeta + 4 mode handler overrides - Simple parallel fetch-all for auto-detect/explicit/project-search modes (no compound cursor — alert rule lists are small datasets) - Single-cursor pagination for org-all mode (metrics: listMetricAlertsPaginated with cursor; issues: resolveTargetsFromParsedArg for project list + fetch all) Removes ~320 lines of custom dispatch and compound cursor logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ucture - Use jsonTransformListResult (shared) instead of custom JSON transforms - Separate raw rules into items (for JSON) and displayRows (for human output) - Add --query/-q flag for client-side name filtering on both alert commands - Restore api-client.ts alerts export to original shape Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ssue list Full structural alignment with sentry issue list: - FetchResult success/failure wrapping per target - fetchWithBudget with two-phase surplus redistribution - Compound cursor pagination (encodeCompoundCursor/decodeCompoundCursor) - buildProjectAliasMap with buildOrgAwareAliases + setProjectAliases - trimWithProjectGuarantee for per-project representation - One handleResolvedTargets for all 4 modes (alert issues have no org-level API) - allowCursorInModes for all modes including org-all - parseCursorFlag explicitly in cursor flag definition - logger.warn for partial failures - IssueAlertListResult with title/footerMode/moreHint/footer - jsonTransformListResult (shared JSON transform) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…issue list Full structural alignment with sentry issue list and alert issues list: - FetchResult success/failure wrapping per org with withAuthGuard - fetchWithBudget with two-phase surplus redistribution (per-org) - Compound cursor pagination across multiple orgs (auto-detect/explicit/project-search) - trimWithOrgGuarantee for per-org representation - handleResolvedOrgs for multi-org modes; handleOrgAllMetricAlerts for single-org cursor - allowCursorInModes for auto-detect/explicit/project-search - parseCursorFlag explicitly in cursor flag definition - logger.warn for partial org failures - MetricAlertListResult with title/footerMode/moreHint/footer - jsonTransformListResult (shared JSON transform) - buildMultiOrgContextKey isolates cursors per unique org set + query Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…list Removes the separate handleOrgAllMetricAlerts handler and routes all 4 modes (including org-all) through handleResolvedOrgs. org-all resolves all projects in the specified org, deduplicates to unique orgs (just the one), then uses fetchWithBudget with compound cursor — same path as auto-detect/explicit/project-search. This makes the structure identical to alert issues list and supports multi-org results from DSN detection consistently across all modes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract FetchResult<T>, trimWithGroupGuarantee<T>, buildProjectAliasMap<T>, and buildMultiOrgContextKey into lib/ so all three list commands (issue, alert issues, alert metrics) share the same implementations. Each command retains a thin domain-specific wrapper where needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
For metric alerts (org-scoped), explicit and org-all modes already provide the org slug in the parsed arg. Routing them through resolveTargetsFromParsedArg caused an unnecessary listProjects API call just to re-derive the org slug. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Made-with: Cursor # Conflicts: # docs/public/.well-known/skills/index.json # plugins/sentry-cli/skills/sentry-cli/SKILL.md # src/commands/issue/list.ts # src/lib/sentry-urls.ts
Correct hasMore computation in alert issue/metric list flows so exact-limit pages without a next cursor don't report false positives, and filtered-empty pages still retain next-page availability. Add regression tests for both commands and clean up a leftover issue list merge marker/imports that blocked command loading. Made-with: Cursor
Add alert rule detail fetch endpoints and new `alert issues view` / `alert metrics view` commands with ID-or-name resolution across resolved targets, plus docs/skill reference updates for the new subcommands. Made-with: Cursor
Switch alert route wiring to the telemetry-aware route-map wrapper and add the missing alert command fragment so generated docs validation passes in CI. Made-with: Cursor
MathurAditya724
changed the title
- View: fail fast on malformed org/project, suppress only 404 in numeric lookup, name match exact then suggestions (no auto fuzzy pick) - List: preserve hasMore from phase-1 when phase-2 is skipped; drop dead footerMode; remove unused isMultiOrg - Add view tests and phase1HasMore list regression test Made-with: Cursor
- API: delete/put helpers, apiRequestToRegionNoContent for empty responses - Shared rule-resolve modules for view/delete/edit (parse + resolve) - issues|metrics delete: buildDeleteCommand, type org/project/id to confirm - issues|metrics edit: --name and --status (active|disabled) - Fix project-structure doc: dedupe subcommand label for nested routes (alert) - Regenerate SKILL + references/alert only; other skill refs left at HEAD to avoid unrelated example-date churn from generate:skill Made-with: Cursor
- Add isNotFoundApiError in lib/api/error-guards (re-exported from api-client) - DRY fetchIssueAlertRuleJson / fetchMetricAlertRuleJson for get*Rule and get*Document - Re-exported from api-client, used by both rule-resolve modules Made-with: Cursor
Implement full alert CRUD coverage for issue and metric rules by adding create commands, broadening merge-based edit support, and updating API helpers, tests, and generated command docs. Made-with: Cursor
Tighten alert create/edit typing to satisfy strict TypeScript and lint checks, and regenerate related skill reference docs so CI passes consistently. Made-with: Cursor
Compute month-boundary period examples in UTC so generated skill/docs output does not drift between local environments and CI. Made-with: Cursor
Resolve conflicts: - contributing.md: keep both main's new commands (local, replay, archive, import, revisions, restore) and branch's alert entry - api-client.ts: keep both discover exports (main) and error-guards + apiRequestToRegionNoContent exports (branch) - infrastructure.ts: keep main's throwRawApiError refactor and branch's apiRequestToRegionNoContent (updated to use throwRawApiError) - resolve-target.ts: keep both branch's resolveTargetsFromParsedArg and main's resolveOrgOptionalProjectTarget
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d4838b5. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
- Replace bun:test imports with vitest in all 8 alert test files - Fix empty-page pagination hint being dropped in issues/list and metrics/list (move moreHint build before the empty-page early return) - Fix edit command validating existing rule conditions/actions instead of only user-provided flags - Fix parseMetricRuleArg misrouting bare org slugs to project-search by appending trailing slash for org-all mode - Fix safety-cap path in fetchRulesForOrg returning hasMore:false and dropping the cursor - Fix transient fetch failure permanently excluding org from pagination by using start-of-list cursor as fallback
Comment on lines
+134
to
+144
| validateMetricTimeWindow(Number(body.timeWindow ?? 0)); | ||
| if (typeof body.query !== "string" || body.query.trim() === "") { | ||
| throw new ValidationError("query must be present and non-empty.", "query"); | ||
| } | ||
| if (typeof body.aggregate !== "string" || body.aggregate.trim() === "") { | ||
| throw new ValidationError( | ||
| "aggregate must be present and non-empty.", | ||
| "aggregate" | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: The validation for editing metric alerts uses an invalid fallback value of 0 for timeWindow. If the API response lacks this field, the edit operation will always fail.
Severity: MEDIUM
Suggested Fix
The validation logic in validateMetricBody should be adjusted for edit operations. Instead of validating the entire merged document with strict requirements, it should only validate the fields explicitly provided by the user via flags. Alternatively, if validating the whole document is intended, remove the invalid fallbacks like ?? 0 for timeWindow and handle missing fields from the API response more gracefully, perhaps by not validating them if they weren't part of the user's update.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/commands/alert/metrics/edit.ts#L129-L144
Potential issue: When editing a metric alert rule, the code fetches the existing rule's
data, merges it with user-provided flags, and then validates the merged object. The
validation for `timeWindow` uses a fallback of `0` (i.e., `body.timeWindow ?? 0`).
However, `0` is not a valid value according to `validateMetricTimeWindow`. If the API
response for an existing rule is missing the `timeWindow` field for any reason, the
validation will use the invalid fallback `0` and throw a `ValidationError`, causing the
edit operation to fail. A similar issue exists for `triggers`, which would fail
validation if missing from the API response.
Comment on lines
+148
to
+156
| }, | ||
| async *func(this: SentryContext, flags: CreateFlags, arg: string) { | ||
| const { cwd } = this; | ||
| if (!flags.name.trim()) { | ||
| throw new ValidationError("Rule name cannot be empty.", "name"); | ||
| } | ||
| if (!flags.query.trim()) { | ||
| throw new ValidationError("query cannot be empty.", "query"); | ||
| } |
There was a problem hiding this comment.
Bare org slug resolves as project-search, silently targeting the wrong organization
Passing my-org (no trailing slash) to sentry alert metrics create my-org ... causes parseOrgProjectArg to return { type: "project-search", projectSlug: "my-org" }. If a project named my-org exists in a different organization, resolveTargetsFromParsedArg returns that org's slug, and the metric alert is created in the wrong organization without any error. The usage hint and docs examples show <org> but the correct form is <org>/ to trigger org-all parsing.
Evidence
parseOrgProjectArgdocs (arg-parsing.ts:568-569) show"cli"→{ type: "project-search", projectSlug: "cli" }; only"sentry/"(trailing slash) yields{ type: "org-all", org: "sentry" }.resolveTargetsFromParsedArgproject-searchbranch callsfindProjectsBySlug(parsed.projectSlug)and maps results to{ org: m.orgSlug, ... }(resolve-target.ts:1912-1915).- The metrics
createcommand then doestargets.map(t => t.org)— if a project namedacme-corplives inother-org,orgSlugsbecomes["other-org"]and the alert is POSTed to the wrong org. - The fallback
isOrgerror check (resolve-target.ts:1878) only fires whenfindProjectsBySlugreturns zero matches and the slug is an org; a coincidental project name match bypasses it entirely. - The docs and USAGE_HINT on line 23 both show
<org>not<org>/, so users will consistently trigger this path.
Also found at 1 additional location
src/app.ts:88
Identified by Warden find-bugs · LMM-4VA
Comment on lines
+94
to
+121
| if (isAllDigits(ref)) { | ||
| const hits: IssueRuleResolution[] = []; | ||
| for (const target of targets) { | ||
| try { | ||
| const rule = await getIssueAlertRule(target.org, target.project, ref); | ||
| hits.push({ target, rule }); | ||
| } catch (e) { | ||
| if (isNotFoundApiError(e)) { | ||
| continue; | ||
| } | ||
| throw e; | ||
| } | ||
| } | ||
|
|
||
| if (hits.length === 1) { | ||
| return hits[0] as IssueRuleResolution; | ||
| } | ||
| if (hits.length > 1) { | ||
| throw new ValidationError( | ||
| `Alert rule ID '${ref}' matched multiple projects.\n` + | ||
| "Use an explicit target: sentry alert issues <command> <org>/<project>/<rule-id>" | ||
| ); | ||
| } | ||
| throw new ResolutionError( | ||
| `Issue alert rule '${ref}'`, | ||
| "not found", | ||
| usageHint | ||
| ); |
There was a problem hiding this comment.
All-digit rule names silently resolve to a different rule by ID, or fail to resolve at all
When ref is all digits, resolveIssueAlertRule only performs an ID-based API lookup and never falls back to name-based search. A rule whose name happens to be all digits (e.g. "42") will either silently return a different rule (the one with numeric ID 42), or return "not found" if no rule with that ID exists — even though the named rule does exist.
Evidence
isAllDigits(ref)at line 94 branches unconditionally: thetruepath callsgetIssueAlertRule(target.org, target.project, ref)(an ID lookup) for every target and returns early or throws without ever reaching the name-based loop at line 122.getIssueAlertRuleinsrc/lib/api/alerts.ts:102forwardsruleIddirectly to the API endpoint — it treats any digit string as a numeric ID, not a name.- If a project has both rule ID 42 and a distinct rule named
"42", the caller supplying"42"always gets rule ID 42, silently bypassing the intended target. - The same design exists in the metrics sibling (
src/commands/alert/metrics/rule-resolve.ts:87) but is out of this hunk's scope.
Suggested fix: After the ID-based search finds zero matches, fall back to the name-based loop instead of immediately throwing ResolutionError.
Suggested change
| if (isAllDigits(ref)) { | |
| const hits: IssueRuleResolution[] = []; | |
| for (const target of targets) { | |
| try { | |
| const rule = await getIssueAlertRule(target.org, target.project, ref); | |
| hits.push({ target, rule }); | |
| } catch (e) { | |
| if (isNotFoundApiError(e)) { | |
| continue; | |
| } | |
| throw e; | |
| } | |
| } | |
| if (hits.length === 1) { | |
| return hits[0] as IssueRuleResolution; | |
| } | |
| if (hits.length > 1) { | |
| throw new ValidationError( | |
| `Alert rule ID '${ref}' matched multiple projects.\n` + | |
| "Use an explicit target: sentry alert issues <command> <org>/<project>/<rule-id>" | |
| ); | |
| } | |
| throw new ResolutionError( | |
| `Issue alert rule '${ref}'`, | |
| "not found", | |
| usageHint | |
| ); | |
| // No ID match — fall through to name-based resolution below |
Identified by Warden find-bugs · 75B-EM3
Comment on lines
+211
to
+213
| return { hint: "Dry run - no metric alert rule was created." }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Status check defaults to 'disabled' when API omits status field on newly created metric alert
When created.status is undefined (absent from the API response), created.status === 0 || created.status === "0" evaluates to false, so the rule is reported as "disabled" even though a freshly created rule should be "active". Invert the check to === 1 to default to "active" for unknown values, matching the MetricAlertRule type comment (0 = active, 1 = disabled) and the pattern used in issues/create.ts.
Evidence
createMetricAlertRulereturnsPromise<Record<string, unknown>>, socreated.statusis typed asunknownand may be absent.MetricAlertRuletype (alerts.ts:42) documents0 = active, 1 = disabled; the only truthy-disabled value is1, not any non-zero.- The sibling
issues/create.tsusesString(created.status ?? "active"), correctly defaulting absent status to"active". metrics/edit.tsuses the identical inverted check, confirming the same bug is present there too.- If the Sentry API omits
statusin the POST response, every created rule is silently shown as disabled.
Identified by Warden find-bugs · 5ZS-L6G
Comment on lines
+75
to
+83
| brief: "Open metric alert rules page in browser", | ||
| default: false, | ||
| }, | ||
| }, | ||
| aliases: { w: "web" }, | ||
| }, | ||
| async *func(this: SentryContext, flags: ViewFlags, arg: string) { | ||
| const { cwd } = this; | ||
| const { ref, targetArg } = parseMetricRuleArg(arg, USAGE_HINT); |
There was a problem hiding this comment.
resolveTargetsFromParsedArg throws misleading 'no accessible projects' error for org-scoped metric alert view
When the user runs sentry alert metrics view my-org/12345, parseMetricRuleArg appends a trailing slash to produce targetArg = "my-org/", which parseOrgProjectArg converts to { type: "org-all" }. resolveTargetsFromParsedArg then calls listProjects("my-org") and throws ResolutionError: Organization 'my-org' has no accessible projects if the org has no accessible projects — even though metric alerts are org-scoped and require no project at all.
Evidence
parseMetricRuleArg("my-org/12345")setstargetPart = "my-org"and appends/, yieldingtargetArg = "my-org/"(rule-resolve.tsline 57).parseOrgProjectArg("my-org/")returns{ type: "org-all", org: "my-org" }(arg-parsing.ts).resolveTargetsFromParsedArgfororg-allcallslistProjects(parsed.org)and throwsResolutionError: Organization 'my-org' has no accessible projectswhen the project list is empty (resolve-target.tslines 1835–1849).- Metric alert rules are org-scoped (no project needed), so the
listProjectscall is unnecessary and the resulting error is misleading. - The same pattern exists in
metrics/delete.tsandmetrics/edit.ts, so the fix should apply consistently across those files too.
Identified by Warden find-bugs · GJT-644
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #578
Adds full
sentry alertCRUD coverage for both issue alerts (project-scoped) and metric alerts (org-scoped), plus pagination correctness fixes and generated docs/CI follow-ups.What this PR includes
Alert list commands
sentry alert issues list(project-scoped issue alert rules)sentry alert metrics list(org-scoped metric alert rules)Both support target resolution, pagination,
--json,--web, and query filtering.Alert view commands
sentry alert issues view <org/project/rule-id-or-name>sentry alert metrics view <org/rule-id-or-name>Both support ID-or-name lookup and
--web.Alert create commands
sentry alert issues create <org/project>with required--name,--condition,--action,--action-match, plus optional--frequency,--environment,--filter,--filter-match,--owner, and--dry-run.sentry alert metrics create <org>with required--name,--query,--aggregate,--dataset,--time-window,--trigger, plus optional--project,--environment,--owner, and--dry-run.Alert edit commands
sentry alert issues edit <org/project/rule-id-or-name>now supports merge-based updates beyond name/status, including conditions/actions/match modes/frequency/environment/filters/owner.sentry alert metrics edit <org/rule-id-or-name>now supports merge-based updates beyond name/status, including query/aggregate/dataset/time-window/triggers/projects/environment/owner.Both edit flows resolve by ID-or-name, fetch the current rule, merge only provided flags, validate the merged payload, and PUT the full document.
Alert delete commands
sentry alert issues delete <org/project/rule-id-or-name>sentry alert metrics delete <org/rule-id-or-name>Both use interactive confirmation by default with
--yes/--forcebypass and--dry-runpreview.API + resolver improvements
createIssueAlertRuleandcreateMetricAlertRuleAPI helpers and exports.isNotFoundApiErrorfor alert rule resolution paths.get*Ruleandget*Documentnow share a single GET per resource type via shared fetch helpers.Pagination correctness fixes
hasMorehandling for exact-limit pages without a next cursor.hasMorewhen client-side filtering yields an empty display page but upstream pages still exist.Follow-up fixes from CI
alertroute import to telemetry-awarebuildRouteMapwrapper.docs/src/fragments/commands/alert.mdfragment.Test plan