From d7d25aa72bdafcbc41e2b3b1a800e14704b013a3 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:40:59 +0000 Subject: [PATCH 1/2] ref(cli): Parse alert rule positionals before the command runs Move the nine allow-listed alert commands off in-func target parsing: - alert issues view/delete/edit and alert metrics view/delete/edit take their rule argument through issueRuleParameter/metricRuleParameter, a boundPositional over the existing parseIssueRuleArg/parseMetricRuleArg splitters. Rule names may contain spaces, so they keep their own syntax rather than an ID_KINDS entry. - alert issues create and alert metrics create use orgProjectTarget with their existing resolvers; alert metrics list uses listTarget. A pasted URL's server is applied on resolve, after the auth guard. Drop the commands from UNBOUND_TARGET_PARSING_ALLOWED, register AlertRuleArgument with require-target-resolution, and cover the commands in argument-conventions.test.ts. Part of #1564 Co-Authored-By: BYK --- packages/cli/AGENTS.md | 7 + packages/cli/lint-rules/cli-oxlint-plugin.js | 12 +- packages/cli/src/commands/alert/arguments.ts | 87 ++++++++++++ .../cli/src/commands/alert/issues/create.ts | 73 ++++++---- .../cli/src/commands/alert/issues/delete.ts | 19 ++- .../cli/src/commands/alert/issues/edit.ts | 19 ++- .../src/commands/alert/issues/rule-resolve.ts | 16 +++ .../cli/src/commands/alert/issues/view.ts | 19 ++- .../cli/src/commands/alert/metrics/create.ts | 26 ++-- .../cli/src/commands/alert/metrics/delete.ts | 32 ++--- .../cli/src/commands/alert/metrics/edit.ts | 25 ++-- .../cli/src/commands/alert/metrics/list.ts | 23 +++- .../commands/alert/metrics/rule-resolve.ts | 16 +++ .../cli/src/commands/alert/metrics/view.ts | 27 ++-- .../test/commands/alert/issues/create.test.ts | 1 + .../commands/alert/metrics/create.test.ts | 1 + .../commands/alert/metrics/delete.test.ts | 4 +- .../cli/test/lib/argument-conventions.test.ts | 125 +++++++++++++++++- 18 files changed, 397 insertions(+), 135 deletions(-) create mode 100644 packages/cli/src/commands/alert/arguments.ts diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index 24595dcd5..75a0d5599 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -319,6 +319,13 @@ inside a tuple positional. `func` takes a `ReleaseTarget` and gets `{ version, org, detectedFrom }` from `await target.resolved`; a blank version fails the parse. +**Alert rule positionals:** `//` (issue +alerts) and `/` (metric alerts) use +`issueRuleParameter` / `metricRuleParameter` from the alert `rule-resolve.ts` +files inside a tuple positional. Rule names may contain spaces, so they keep +their own syntax instead of `ID_KINDS`. `func` takes an `AlertRuleArgument` +and gets `{ ref, target }` from `await rule.resolved`. + List commands take their target from `listTargetPositional()` in `src/lib/list-command.ts` (`LIST_TARGET_POSITIONAL`, `LIST_ORG_TARGET_POSITIONAL`, or `listTargetPositional({ orgScoped, names, diff --git a/packages/cli/lint-rules/cli-oxlint-plugin.js b/packages/cli/lint-rules/cli-oxlint-plugin.js index 6b6e4252c..1f50a1971 100644 --- a/packages/cli/lint-rules/cli-oxlint-plugin.js +++ b/packages/cli/lint-rules/cli-oxlint-plugin.js @@ -66,6 +66,7 @@ const BOUND_TARGET_TYPES = new Set([ "OrgProjectTarget", "ProjectBoundTarget", "ReleaseTarget", + "AlertRuleArgument", "BoundArguments", "DashboardArguments", "DashboardListArguments", @@ -205,17 +206,6 @@ const UNBOUND_TARGET_PARSING = new Set([ /** Commands still allowed to, each with its follow-up; a ratchet too. */ const UNBOUND_TARGET_PARSING_ALLOWED = new Map([ // Follow-ups (#1564): - // `[/[/]]`, resolving to every matching project - ["/src/commands/alert/issues/create.ts", "#1564: alert rule target"], - ["/src/commands/alert/issues/delete.ts", "#1564: alert rule target"], - ["/src/commands/alert/issues/edit.ts", "#1564: alert rule target"], - ["/src/commands/alert/issues/view.ts", "#1564: alert rule target"], - // `[/]`, org-scoped - ["/src/commands/alert/metrics/create.ts", "#1564: alert rule target"], - ["/src/commands/alert/metrics/delete.ts", "#1564: alert rule target"], - ["/src/commands/alert/metrics/edit.ts", "#1564: alert rule target"], - ["/src/commands/alert/metrics/list.ts", "#1564: alert rule target"], - ["/src/commands/alert/metrics/view.ts", "#1564: alert rule target"], // `[/]` with a non-hex ID, like `releaseParameter` ["/src/commands/agent-conversation/view.ts", "#1564: org-scoped ID"], ["/src/commands/integration/delete.ts", "#1564: org-scoped ID"], diff --git a/packages/cli/src/commands/alert/arguments.ts b/packages/cli/src/commands/alert/arguments.ts new file mode 100644 index 000000000..927d76ca6 --- /dev/null +++ b/packages/cli/src/commands/alert/arguments.ts @@ -0,0 +1,87 @@ +/** + * Alert rule positionals, parsed before `func` runs. + * + * {@link alertRuleParameter} takes a rule ID or name and where to find it: + * `//` for issue alerts, `/` for metric + * alerts. Rule names may hold spaces, so these keep their own syntax + * (`parseIssueRuleArg()`, `parseMetricRuleArg()`) rather than the ID + * grammar of `id-arguments.ts`. + * + * The parse has no side effects; the server of a pasted Sentry URL is + * applied when the value resolves, after the auth guard. + */ + +import { + type ParsedOrgProject, + parseOrgProjectInput, +} from "../../lib/arg-parsing.js"; +import type { + ORG_PROJECT_TARGET, + ORG_TARGET, +} from "../../lib/completion-target.js"; +import { + type BoundPositional, + boundPositional, +} from "../../lib/positional-parse.js"; +import { applySentryUrlContext } from "../../lib/sentry-url-parser.js"; + +/** A rule ID or name and the target to find it in. */ +export type AlertRuleRef = { + /** The rule's numeric ID or name, as typed */ + readonly ref: string; + /** Where to look the rule up */ + readonly target: ParsedOrgProject; +}; + +/** + * An alert rule positional. `await rule.resolved` applies a pasted URL's + * server and yields the rule reference; the command then looks the rule up + * in the target's projects or org. + */ +export type AlertRuleArgument = BoundPositional; + +/** Splits a rule argument into the rule reference and the target argument. */ +type RuleArgSplitter = ( + arg: string, + usageHint: string, +) => { ref: string; targetArg: string | undefined }; + +/** + * The rule parameter of an alert command's tuple positional. + * + * @param options.split - The rule syntax: `parseIssueRuleArg` or + * `parseMetricRuleArg`; throws a `ValidationError` for an invalid rule + * @param options.kind - Shell completion for the target part + */ +export function alertRuleParameter(options: { + readonly placeholder: string; + readonly brief: string; + readonly kind: typeof ORG_TARGET | typeof ORG_PROJECT_TARGET; + readonly split: RuleArgSplitter; + readonly usageHint: string; +}) { + const { placeholder, brief, kind, split, usageHint } = options; + const parse = (raw: string | undefined) => { + const { ref, targetArg } = split(raw ?? "", usageHint); + return { ref, ...parseOrgProjectInput(targetArg) }; + }; + return { + placeholder, + brief, + parse: boundPositional( + kind, + (raw): AlertRuleRef => { + const { ref, parsed } = parse(raw); + return { ref, target: parsed }; + }, + async (rule: AlertRuleRef, _context: unknown, raw) => { + // Parsing again is cheap and pure; it recovers the URL's server + const { baseUrl } = parse(raw); + if (baseUrl) { + applySentryUrlContext(baseUrl); + } + return rule; + }, + ), + } as const; +} diff --git a/packages/cli/src/commands/alert/issues/create.ts b/packages/cli/src/commands/alert/issues/create.ts index 39627f29d..878abbdb7 100644 --- a/packages/cli/src/commands/alert/issues/create.ts +++ b/packages/cli/src/commands/alert/issues/create.ts @@ -9,7 +9,7 @@ import { createIssueAlertRule, resolveErrorDetectorId, } from "../../../lib/api-client.js"; -import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; +import type { ParsedOrgProject } from "../../../lib/arg-parsing.js"; import { buildCommand, numberParser } from "../../../lib/command.js"; import { ContextError, @@ -18,7 +18,14 @@ import { } from "../../../lib/errors.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../../lib/mutate-command.js"; -import { resolveProjectBoundTargets } from "../../../lib/resolve-target.js"; +import { + type ResolvedTarget, + resolveProjectBoundTargets, +} from "../../../lib/resolve-target.js"; +import { + type OrgProjectTarget, + orgProjectTarget, +} from "../../../lib/target-positional.js"; import { matchToLogicType, parseJsonObjectList, @@ -61,6 +68,33 @@ function formatCreated(result: CreateResult): string { return `Created issue alert rule ${result.id ?? "(unknown id)"} in ${result.org}/${result.project}: ${result.name} (${result.status ?? "active"}).`; } +/** + * Resolve the create target to exactly one project. + * + * @throws {ContextError} When no project is found + * @throws {ValidationError} When the target matches several projects + */ +async function resolveCreateTarget( + parsed: ParsedOrgProject, + cwd: string, +): Promise { + const { targets } = await resolveProjectBoundTargets(parsed, { + cwd, + usageHint: USAGE_HINT, + }); + const [target, ...others] = targets; + if (!target) { + throw new ContextError("Organization and project", USAGE_HINT); + } + if (others.length > 0) { + throw new ValidationError( + "Provide a target that resolves to exactly one project for create.", + "target", + ); + } + return target; +} + export const createCommand = buildCommand({ docs: { brief: "Create an issue alert rule", @@ -106,7 +140,9 @@ export const createCommand = buildCommand({ placeholder: "target", brief: "/, auto-detected project, or (search)", - parse: String, + parse: orgProjectTarget((parsed, { cwd }) => + resolveCreateTarget(parsed, cwd), + ), optional: true as const, }, ], @@ -174,9 +210,8 @@ export const createCommand = buildCommand({ async *func( this: SentryContext, flags: CreateFlags, - arg: string | undefined, + target: OrgProjectTarget, ) { - const { cwd } = this; if (!flags.name.trim()) { throw new ValidationError("Rule name cannot be empty.", "name"); } @@ -193,24 +228,10 @@ export const createCommand = buildCommand({ validateIssueRuleArrays(conditions, actions, "conditions"); validateIssueRuleArrays(conditions, actions, "actions"); - const parsed = parseOrgProjectArg(arg); - const { targets } = await resolveProjectBoundTargets(parsed, { - cwd, - usageHint: USAGE_HINT, - }); - if (targets.length === 0) { - throw new ContextError("Organization and project", USAGE_HINT); - } - if (targets.length !== 1) { - throw new ValidationError( - "Provide a target that resolves to exactly one project for create.", - "target", - ); - } - const target = targets[0] as (typeof targets)[number]; + const { org, project } = await target.resolved; // Issue alerts fire on a project's error detector; connect via detector_ids. - const detectorId = await resolveErrorDetectorId(target.org, target.project); + const detectorId = await resolveErrorDetectorId(org, project); // Assemble the workflow-shaped body. The user supplies workflow-native // conditions/actions; the CLI only builds the envelope (triggers + @@ -240,8 +261,8 @@ export const createCommand = buildCommand({ if (flags["dry-run"]) { yield new CommandOutput({ - org: target.org, - project: target.project, + org, + project, name: flags.name, dryRun: true, body, @@ -249,10 +270,10 @@ export const createCommand = buildCommand({ return { hint: "Dry run - no issue alert rule was created." }; } - const created = await createIssueAlertRule(target.org, body); + const created = await createIssueAlertRule(org, body); yield new CommandOutput({ - org: target.org, - project: target.project, + org, + project, id: stringifyUnknown(created.id ?? ""), name: stringifyUnknown(created.name ?? flags.name), status: stringifyUnknown( diff --git a/packages/cli/src/commands/alert/issues/delete.ts b/packages/cli/src/commands/alert/issues/delete.ts index 04796b36c..35a4ea080 100644 --- a/packages/cli/src/commands/alert/issues/delete.ts +++ b/packages/cli/src/commands/alert/issues/delete.ts @@ -6,7 +6,6 @@ import type { SentryContext } from "../../../context.js"; import { deleteIssueAlertRule } from "../../../lib/api-client.js"; -import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; import { ContextError, ValidationError } from "../../../lib/errors.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; import { logger } from "../../../lib/logger.js"; @@ -17,7 +16,8 @@ import { requireExplicitTarget, } from "../../../lib/mutate-command.js"; import { resolveProjectBoundTargets } from "../../../lib/resolve-target.js"; -import { parseIssueRuleArg, resolveIssueAlertRule } from "./rule-resolve.js"; +import type { AlertRuleArgument } from "../arguments.js"; +import { issueRuleParameter, resolveIssueAlertRule } from "./rule-resolve.js"; const USAGE_HINT = "sentry alert issues delete //"; @@ -90,18 +90,17 @@ export const deleteCommand = buildDeleteCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/project/rule-id-or-name", - brief: "Rule id or name (same as view)", - parse: String, - }, + issueRuleParameter(USAGE_HINT, "Rule id or name (same as view)"), ], }, }, - async *func(this: SentryContext, flags: DeleteFlags, arg: string) { + async *func( + this: SentryContext, + flags: DeleteFlags, + ruleArg: AlertRuleArgument, + ) { const { cwd } = this; - const { ref, targetArg } = parseIssueRuleArg(arg, USAGE_HINT); - const parsed = parseOrgProjectArg(targetArg); + const { ref, target: parsed } = await ruleArg.resolved; requireExplicitTarget(parsed, "Issue alert target", USAGE_HINT); if (parsed.type !== "explicit") { throw new ValidationError( diff --git a/packages/cli/src/commands/alert/issues/edit.ts b/packages/cli/src/commands/alert/issues/edit.ts index 0bb7ec6df..0456820cf 100644 --- a/packages/cli/src/commands/alert/issues/edit.ts +++ b/packages/cli/src/commands/alert/issues/edit.ts @@ -10,7 +10,6 @@ import { getIssueAlertWorkflowDocument, updateIssueAlertRule, } from "../../../lib/api-client.js"; -import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; import { buildCommand, numberParser } from "../../../lib/command.js"; import { ContextError, @@ -27,7 +26,8 @@ import { triggerLogicType, validateIssueRuleArrays, } from "../mutation-utils.js"; -import { parseIssueRuleArg, resolveIssueAlertRule } from "./rule-resolve.js"; +import type { AlertRuleArgument } from "../arguments.js"; +import { issueRuleParameter, resolveIssueAlertRule } from "./rule-resolve.js"; const USAGE_HINT = "sentry alert issues edit // --name | --status active|disabled"; @@ -204,11 +204,7 @@ export const editCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/project/rule-id-or-name", - brief: "Rule id or name (same as view)", - parse: String, - }, + issueRuleParameter(USAGE_HINT, "Rule id or name (same as view)"), ], }, flags: { @@ -276,12 +272,15 @@ export const editCommand = buildCommand({ m: "filter-match", }, }, - async *func(this: SentryContext, flags: EditFlags, arg: string) { + async *func( + this: SentryContext, + flags: EditFlags, + ruleArg: AlertRuleArgument, + ) { const { cwd } = this; validateIssueEditFlags(flags); - const { ref, targetArg } = parseIssueRuleArg(arg, USAGE_HINT); - const parsed = parseOrgProjectArg(targetArg); + const { ref, target: parsed } = await ruleArg.resolved; const { targets } = await resolveProjectBoundTargets(parsed, { cwd, usageHint: USAGE_HINT, diff --git a/packages/cli/src/commands/alert/issues/rule-resolve.ts b/packages/cli/src/commands/alert/issues/rule-resolve.ts index db5312a92..e7da9789b 100644 --- a/packages/cli/src/commands/alert/issues/rule-resolve.ts +++ b/packages/cli/src/commands/alert/issues/rule-resolve.ts @@ -10,11 +10,13 @@ import { isNotFoundApiError, listIssueAlertsPaginated, } from "../../../lib/api-client.js"; +import { ORG_PROJECT_TARGET } from "../../../lib/completion-target.js"; import { ResolutionError, ValidationError } from "../../../lib/errors.js"; import { fuzzyMatch } from "../../../lib/fuzzy.js"; import { logger } from "../../../lib/logger.js"; import type { ResolvedTarget } from "../../../lib/resolve-target.js"; import { countChar, isAllDigits } from "../../../lib/utils.js"; +import { alertRuleParameter } from "../arguments.js"; const log = logger.withTag("alert.issues"); @@ -67,6 +69,20 @@ export function parseIssueRuleArg( return { ref, targetArg: targetPart || undefined }; } +/** + * The `//` parameter of an issue alert + * command, parsed with {@link parseIssueRuleArg} before the command runs. + */ +export function issueRuleParameter(usageHint: string, brief: string) { + return alertRuleParameter({ + placeholder: "org/project/rule-id-or-name", + brief, + kind: ORG_PROJECT_TARGET, + split: parseIssueRuleArg, + usageHint, + }); +} + /** List all issue alert rules for a project (paginated). */ export async function listAllIssueRulesForTarget( target: ResolvedTarget, diff --git a/packages/cli/src/commands/alert/issues/view.ts b/packages/cli/src/commands/alert/issues/view.ts index 140ccb24c..92216112c 100644 --- a/packages/cli/src/commands/alert/issues/view.ts +++ b/packages/cli/src/commands/alert/issues/view.ts @@ -1,14 +1,14 @@ import type { SentryContext } from "../../../context.js"; -import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; import { openInBrowser } from "../../../lib/browser.js"; import { buildCommand } from "../../../lib/command.js"; import { ContextError } from "../../../lib/errors.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; import { resolveProjectBoundTargets } from "../../../lib/resolve-target.js"; import { buildIssueAlertsUrl } from "../../../lib/sentry-web-urls.js"; +import type { AlertRuleArgument } from "../arguments.js"; import { type IssueRuleResolution, - parseIssueRuleArg, + issueRuleParameter, resolveIssueAlertRule, } from "./rule-resolve.js"; @@ -71,11 +71,7 @@ export const viewCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/project/rule-id-or-name", - brief: "Issue alert rule ID or name", - parse: String, - }, + issueRuleParameter(USAGE_HINT, "Issue alert rule ID or name"), ], }, flags: { @@ -87,10 +83,13 @@ export const viewCommand = buildCommand({ }, aliases: { w: "web" }, }, - async *func(this: SentryContext, flags: ViewFlags, arg: string) { + async *func( + this: SentryContext, + flags: ViewFlags, + ruleArg: AlertRuleArgument, + ) { const { cwd } = this; - const { ref, targetArg } = parseIssueRuleArg(arg, USAGE_HINT); - const parsed = parseOrgProjectArg(targetArg); + const { ref, target: parsed } = await ruleArg.resolved; if (flags.web && parsed.type === "explicit") { await openInBrowser( diff --git a/packages/cli/src/commands/alert/metrics/create.ts b/packages/cli/src/commands/alert/metrics/create.ts index 9516ba514..ef6d1e390 100644 --- a/packages/cli/src/commands/alert/metrics/create.ts +++ b/packages/cli/src/commands/alert/metrics/create.ts @@ -6,12 +6,17 @@ import type { SentryContext } from "../../../context.js"; import { createMetricAlertRule } from "../../../lib/api-client.js"; +import type { ParsedOrgProject } from "../../../lib/arg-parsing.js"; import { buildCommand, numberParser } from "../../../lib/command.js"; import { stringifyUnknown, ValidationError } from "../../../lib/errors.js"; import { warning } from "../../../lib/formatters/colors.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../../lib/mutate-command.js"; -import { resolveOrgOnlyFromArg } from "../../../lib/resolve-target.js"; +import { resolveOrgOnlyTarget } from "../../../lib/resolve-target.js"; +import { + type OrgProjectTarget, + orgProjectTarget, +} from "../../../lib/target-positional.js"; import { METRIC_ALERT_DATASET_BRIEF, parseMetricTriggers, @@ -59,11 +64,11 @@ function formatCreated(result: CreateResult): string { } function resolveMetricCreateOrg( - arg: string | undefined, + parsed: ParsedOrgProject, cwd: string, ): Promise { - return resolveOrgOnlyFromArg( - arg, + return resolveOrgOnlyTarget( + parsed, cwd, "alert metrics create", "sentry alert metrics create /", @@ -110,7 +115,9 @@ export const createCommand = buildCommand({ { placeholder: "target", brief: "Target project or organization", - parse: String, + parse: orgProjectTarget((parsed, { cwd }) => + resolveMetricCreateOrg(parsed, cwd), + ), }, ], }, @@ -177,8 +184,11 @@ export const createCommand = buildCommand({ }, aliases: { ...DRY_RUN_ALIASES, t: "trigger", p: "project" }, }, - async *func(this: SentryContext, flags: CreateFlags, arg: string) { - const { cwd } = this; + async *func( + this: SentryContext, + flags: CreateFlags, + target: OrgProjectTarget, + ) { if (!flags.name.trim()) { throw new ValidationError("Rule name cannot be empty.", "name"); } @@ -211,7 +221,7 @@ export const createCommand = buildCommand({ ); } - const orgSlug = await resolveMetricCreateOrg(arg, cwd); + const orgSlug = await target.resolved; const body: Record = { name: flags.name, diff --git a/packages/cli/src/commands/alert/metrics/delete.ts b/packages/cli/src/commands/alert/metrics/delete.ts index 42b9b93ec..9bdeac9fd 100644 --- a/packages/cli/src/commands/alert/metrics/delete.ts +++ b/packages/cli/src/commands/alert/metrics/delete.ts @@ -6,7 +6,6 @@ import type { SentryContext } from "../../../context.js"; import { deleteMetricAlertRule } from "../../../lib/api-client.js"; -import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; import { logger } from "../../../lib/logger.js"; import { @@ -15,8 +14,9 @@ import { isConfirmationBypassed, requireExplicitTarget, } from "../../../lib/mutate-command.js"; -import { resolveOrgOnlyFromArg } from "../../../lib/resolve-target.js"; -import { parseMetricRuleArg, resolveMetricAlertRule } from "./rule-resolve.js"; +import { resolveOrgOnlyTarget } from "../../../lib/resolve-target.js"; +import type { AlertRuleArgument } from "../arguments.js"; +import { metricRuleParameter, resolveMetricAlertRule } from "./rule-resolve.js"; const USAGE_HINT = "sentry alert metrics delete /"; @@ -70,27 +70,19 @@ export const deleteCommand = buildDeleteCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/rule-id-or-name", - brief: "Rule id or name (same as view)", - parse: String, - }, + metricRuleParameter(USAGE_HINT, "Rule id or name (same as view)"), ], }, }, - async *func(this: SentryContext, flags: DeleteFlags, arg: string) { + async *func( + this: SentryContext, + flags: DeleteFlags, + ruleArg: AlertRuleArgument, + ) { const { cwd } = this; - const { ref, targetArg } = parseMetricRuleArg(arg, USAGE_HINT); - requireExplicitTarget( - parseOrgProjectArg(targetArg), - "Metric alert target", - USAGE_HINT, - ); - const org = await resolveOrgOnlyFromArg( - targetArg, - cwd, - "alert metrics delete", - ); + const { ref, target } = await ruleArg.resolved; + requireExplicitTarget(target, "Metric alert target", USAGE_HINT); + const org = await resolveOrgOnlyTarget(target, cwd, "alert metrics delete"); const orgSlugs = [org]; const { orgSlug, rule } = await resolveMetricAlertRule( diff --git a/packages/cli/src/commands/alert/metrics/edit.ts b/packages/cli/src/commands/alert/metrics/edit.ts index 798727707..f29e345e9 100644 --- a/packages/cli/src/commands/alert/metrics/edit.ts +++ b/packages/cli/src/commands/alert/metrics/edit.ts @@ -13,7 +13,7 @@ import { buildCommand, numberParser } from "../../../lib/command.js"; import { stringifyUnknown, ValidationError } from "../../../lib/errors.js"; import { warning } from "../../../lib/formatters/colors.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; -import { resolveOrgOnlyFromArg } from "../../../lib/resolve-target.js"; +import { resolveOrgOnlyTarget } from "../../../lib/resolve-target.js"; import { METRIC_ALERT_DATASET_BRIEF, parseMetricTriggers, @@ -29,7 +29,8 @@ import { validateMetricDataset, validateMetricTimeWindow, } from "../mutation-utils.js"; -import { parseMetricRuleArg, resolveMetricAlertRule } from "./rule-resolve.js"; +import type { AlertRuleArgument } from "../arguments.js"; +import { metricRuleParameter, resolveMetricAlertRule } from "./rule-resolve.js"; import { metricAlertStatusLabel } from "./status.js"; const USAGE_HINT = @@ -207,11 +208,7 @@ export const editCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/rule-id-or-name", - brief: "Rule id or name (same as view)", - parse: String, - }, + metricRuleParameter(USAGE_HINT, "Rule id or name (same as view)"), ], }, flags: { @@ -291,15 +288,15 @@ export const editCommand = buildCommand({ p: "project", }, }, - async *func(this: SentryContext, flags: EditFlags, arg: string) { + async *func( + this: SentryContext, + flags: EditFlags, + ruleArg: AlertRuleArgument, + ) { const { cwd } = this; validateMetricEditFlags(flags); - const { ref, targetArg } = parseMetricRuleArg(arg, USAGE_HINT); - const org = await resolveOrgOnlyFromArg( - targetArg, - cwd, - "alert metrics edit", - ); + const { ref, target } = await ruleArg.resolved; + const org = await resolveOrgOnlyTarget(target, cwd, "alert metrics edit"); const orgSlugs = [org]; const { orgSlug, rule } = await resolveMetricAlertRule( orgSlugs, diff --git a/packages/cli/src/commands/alert/metrics/list.ts b/packages/cli/src/commands/alert/metrics/list.ts index a8e90a208..c0aa78df9 100644 --- a/packages/cli/src/commands/alert/metrics/list.ts +++ b/packages/cli/src/commands/alert/metrics/list.ts @@ -14,8 +14,9 @@ import type { SentryContext } from "../../../context.js"; import type { MetricAlertRule } from "../../../lib/api/alerts.js"; import { paginate } from "../../../lib/api/infrastructure.js"; import { listMetricAlertsPaginated } from "../../../lib/api-client.js"; -import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; +import type { ParsedOrgProject } from "../../../lib/arg-parsing.js"; import { openInBrowser } from "../../../lib/browser.js"; +import { ORG_PROJECT_TARGET } from "../../../lib/completion-target.js"; import { advancePaginationState, buildMultiOrgContextKey, @@ -62,6 +63,10 @@ import { resolveProjectBoundTargets, } from "../../../lib/resolve-target.js"; import { buildMetricAlertsUrl } from "../../../lib/sentry-web-urls.js"; +import { + type ListCommandTarget, + listTarget, +} from "../../../lib/target-positional.js"; import type { Writer } from "../../../types/index.js"; import { assertAlertListLimit, @@ -82,7 +87,7 @@ const METRIC_ALERT_TARGET_POSITIONAL = { { placeholder: "target", brief: "/, / (project ignored), or (search)", - parse: String, + parse: listTarget(ORG_PROJECT_TARGET), optional: true as const, }, ], @@ -161,7 +166,7 @@ async function fetchRulesForOrg( // Mode handlers type ResolvedOrgsOptions = { - parsed: ReturnType; + parsed: ParsedOrgProject; flags: ListFlags; cwd: string; projectSearchResolution?: ProjectSearchTargetResolution; @@ -176,7 +181,7 @@ type ResolvedOrgsOptions = { * detection / cross-org project search) to discover the org(s). */ async function resolveOrgs( - parsed: ReturnType, + parsed: ParsedOrgProject, cwd: string, projectSearchResolution?: ProjectSearchTargetResolution, ): Promise<{ orgs: string[]; footer?: string }> { @@ -195,7 +200,7 @@ async function resolveOrgs( } async function resolveWebUrl( - parsed: ReturnType, + parsed: ParsedOrgProject, cwd: string, ): Promise { let projectSearchResolution: ProjectSearchTargetResolution | undefined; @@ -544,9 +549,13 @@ export const listCommand = buildListCommand("alert metrics", { }, aliases: { ...LIST_BASE_ALIASES, w: "web", q: "query" }, }, - async *func(this: SentryContext, flags: ListFlags, target?: string) { + async *func( + this: SentryContext, + flags: ListFlags, + target: ListCommandTarget, + ) { const { cwd } = this; - const parsed = parseOrgProjectArg(target); + const { parsed } = await target.resolved; assertAlertListLimit(flags.limit); if (flags.web) { diff --git a/packages/cli/src/commands/alert/metrics/rule-resolve.ts b/packages/cli/src/commands/alert/metrics/rule-resolve.ts index 9d4dd35ee..60714ff62 100644 --- a/packages/cli/src/commands/alert/metrics/rule-resolve.ts +++ b/packages/cli/src/commands/alert/metrics/rule-resolve.ts @@ -10,10 +10,12 @@ import { listMetricAlertsPaginated, type MetricAlertRule, } from "../../../lib/api-client.js"; +import { ORG_TARGET } from "../../../lib/completion-target.js"; import { ResolutionError, ValidationError } from "../../../lib/errors.js"; import { fuzzyMatch } from "../../../lib/fuzzy.js"; import { logger } from "../../../lib/logger.js"; import { countChar, isAllDigits } from "../../../lib/utils.js"; +import { alertRuleParameter } from "../arguments.js"; const log = logger.withTag("alert.metrics"); @@ -65,6 +67,20 @@ export function parseMetricRuleArg( return { ref, targetArg: targetPart ? `${targetPart}/` : undefined }; } +/** + * The `/` parameter of a metric alert command, + * parsed with {@link parseMetricRuleArg} before the command runs. + */ +export function metricRuleParameter(usageHint: string, brief: string) { + return alertRuleParameter({ + placeholder: "org/rule-id-or-name", + brief, + kind: ORG_TARGET, + split: parseMetricRuleArg, + usageHint, + }); +} + export async function listAllMetricRulesForOrg( orgSlug: string, ): Promise { diff --git a/packages/cli/src/commands/alert/metrics/view.ts b/packages/cli/src/commands/alert/metrics/view.ts index 2f5da8155..78915aeb2 100644 --- a/packages/cli/src/commands/alert/metrics/view.ts +++ b/packages/cli/src/commands/alert/metrics/view.ts @@ -3,7 +3,6 @@ import { getIssueAlertWorkflowDocument, getMetricAlertRule, } from "../../../lib/api-client.js"; -import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; import { openInBrowser } from "../../../lib/browser.js"; import { buildCommand } from "../../../lib/command.js"; import { ApiError } from "../../../lib/errors.js"; @@ -13,10 +12,11 @@ import { } from "../../../lib/formatters/alerts.js"; import { filterFields } from "../../../lib/formatters/json.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; -import { resolveOrgOnlyFromArg } from "../../../lib/resolve-target.js"; +import { resolveOrgOnlyTarget } from "../../../lib/resolve-target.js"; import { buildMetricAlertsUrl } from "../../../lib/sentry-web-urls.js"; import { isAllDigits } from "../../../lib/utils.js"; -import { parseMetricRuleArg, resolveMetricAlertRule } from "./rule-resolve.js"; +import type { AlertRuleArgument } from "../arguments.js"; +import { metricRuleParameter, resolveMetricAlertRule } from "./rule-resolve.js"; const USAGE_HINT = "sentry alert metrics view /"; @@ -71,11 +71,7 @@ export const viewCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/rule-id-or-name", - brief: "Metric alert rule ID or name", - parse: String, - }, + metricRuleParameter(USAGE_HINT, "Metric alert rule ID or name"), ], }, flags: { @@ -87,10 +83,13 @@ export const viewCommand = buildCommand({ }, aliases: { w: "web" }, }, - async *func(this: SentryContext, flags: ViewFlags, arg: string) { + async *func( + this: SentryContext, + flags: ViewFlags, + ruleArg: AlertRuleArgument, + ) { const { cwd } = this; - const { ref, targetArg } = parseMetricRuleArg(arg, USAGE_HINT); - const parsed = parseOrgProjectArg(targetArg); + const { ref, target: parsed } = await ruleArg.resolved; if (flags.web && parsed.type === "org-all") { await openInBrowser( @@ -100,11 +99,7 @@ export const viewCommand = buildCommand({ return; } - const org = await resolveOrgOnlyFromArg( - targetArg, - cwd, - "alert metrics view", - ); + const org = await resolveOrgOnlyTarget(parsed, cwd, "alert metrics view"); if (flags.web) { await openInBrowser(buildMetricAlertsUrl(org), "metric alert rules"); diff --git a/packages/cli/test/commands/alert/issues/create.test.ts b/packages/cli/test/commands/alert/issues/create.test.ts index d3b8df0c7..69cd1face 100644 --- a/packages/cli/test/commands/alert/issues/create.test.ts +++ b/packages/cli/test/commands/alert/issues/create.test.ts @@ -38,6 +38,7 @@ function createContext() { stdout: { write: stdoutWrite }, stderr: { write: vi.fn(() => true) }, cwd: getConfigDir(), + commandPrefix: ["sentry", "alert", "issues", "create"], stdoutWrite, }; } diff --git a/packages/cli/test/commands/alert/metrics/create.test.ts b/packages/cli/test/commands/alert/metrics/create.test.ts index 5d2f11484..0de2357e2 100644 --- a/packages/cli/test/commands/alert/metrics/create.test.ts +++ b/packages/cli/test/commands/alert/metrics/create.test.ts @@ -31,6 +31,7 @@ function createContext() { stdout: { write: stdoutWrite }, stderr: { write: vi.fn(() => true) }, cwd: getConfigDir(), + commandPrefix: ["sentry", "alert", "metrics", "create"], stdoutWrite, }; } diff --git a/packages/cli/test/commands/alert/metrics/delete.test.ts b/packages/cli/test/commands/alert/metrics/delete.test.ts index 6899e1682..2543781bf 100644 --- a/packages/cli/test/commands/alert/metrics/delete.test.ts +++ b/packages/cli/test/commands/alert/metrics/delete.test.ts @@ -52,7 +52,7 @@ describe("alert metrics delete", () => { beforeEach(() => { getRuleSpy = vi.spyOn(apiClient, "getMetricAlertRule"); deleteRuleSpy = vi.spyOn(apiClient, "deleteMetricAlertRule"); - resolveSpy = vi.spyOn(resolveTarget, "resolveOrgOnlyFromArg"); + resolveSpy = vi.spyOn(resolveTarget, "resolveOrgOnlyTarget"); getRuleSpy.mockResolvedValue(sampleRule); deleteRuleSpy.mockResolvedValue(undefined); @@ -76,7 +76,7 @@ describe("alert metrics delete", () => { ); expect(resolveSpy).toHaveBeenCalledWith( - "test-org/", + { type: "org-all", org: "test-org" }, getConfigDir(), "alert metrics delete", ); diff --git a/packages/cli/test/lib/argument-conventions.test.ts b/packages/cli/test/lib/argument-conventions.test.ts index 4c9fb2b69..3cd906565 100644 --- a/packages/cli/test/lib/argument-conventions.test.ts +++ b/packages/cli/test/lib/argument-conventions.test.ts @@ -10,6 +10,15 @@ import { ArgumentParseError } from "@stricli/core"; import { beforeEach, describe, expect, test, vi } from "vitest"; +import { createCommand as alertIssuesCreateCommand } from "../../src/commands/alert/issues/create.js"; +import { deleteCommand as alertIssuesDeleteCommand } from "../../src/commands/alert/issues/delete.js"; +import { editCommand as alertIssuesEditCommand } from "../../src/commands/alert/issues/edit.js"; +import { viewCommand as alertIssuesViewCommand } from "../../src/commands/alert/issues/view.js"; +import { createCommand as alertMetricsCreateCommand } from "../../src/commands/alert/metrics/create.js"; +import { deleteCommand as alertMetricsDeleteCommand } from "../../src/commands/alert/metrics/delete.js"; +import { editCommand as alertMetricsEditCommand } from "../../src/commands/alert/metrics/edit.js"; +import { listCommand as alertMetricsListCommand } from "../../src/commands/alert/metrics/list.js"; +import { viewCommand as alertMetricsViewCommand } from "../../src/commands/alert/metrics/view.js"; import { listCommand as dashboardListCommand } from "../../src/commands/dashboard/list.js"; import { restoreCommand as dashboardRestoreCommand } from "../../src/commands/dashboard/restore.js"; import { revisionsCommand as dashboardRevisionsCommand } from "../../src/commands/dashboard/revisions.js"; @@ -48,7 +57,7 @@ import { listCommand as spanListCommand } from "../../src/commands/span/list.js" import { viewCommand as spanViewCommand } from "../../src/commands/span/view.js"; import { logsCommand as traceLogsCommand } from "../../src/commands/trace/logs.js"; import { viewCommand as traceViewCommand } from "../../src/commands/trace/view.js"; -import { ContextError } from "../../src/lib/errors.js"; +import { ContextError, ValidationError } from "../../src/lib/errors.js"; import { applyBoundPositionals, BoundPositional, @@ -795,3 +804,117 @@ describe.each(RELEASE_COMMANDS)("%s", (_name, command) => { expect(() => bind(command, [" "])).toThrow(ContextError); }); }); + +/** Commands that take an `//` issue alert. */ +const ISSUE_RULE_COMMANDS: readonly (readonly [string, unknown])[] = [ + ["alert issues view", alertIssuesViewCommand], + ["alert issues delete", alertIssuesDeleteCommand], + ["alert issues edit", alertIssuesEditCommand], +]; + +/** Commands that take an `/` metric alert. */ +const METRIC_RULE_COMMANDS: readonly (readonly [string, unknown])[] = [ + ["alert metrics view", alertMetricsViewCommand], + ["alert metrics delete", alertMetricsDeleteCommand], + ["alert metrics edit", alertMetricsEditCommand], +]; + +/** Alert commands that take only an `[/][]` target. */ +const ALERT_TARGET_COMMANDS: readonly (readonly [string, unknown])[] = [ + ["alert issues create", alertIssuesCreateCommand], + ["alert metrics create", alertMetricsCreateCommand], + ["alert metrics list", alertMetricsListCommand], +]; + +const PROJECT_SETTINGS_URL = `${SERVER}/settings/acme/projects/web/`; + +/** The parsed value of a command's first positional. */ +function parsedOf(command: unknown, input: string): unknown { + const [bound] = bind(command, [input]); + expect(bound).toBeInstanceOf(BoundPositional); + return (bound as BoundPositional).parsed; +} + +describe.each(ISSUE_RULE_COMMANDS)("%s", (_name, command) => { + test.each([ + ["42", { ref: "42", target: { type: "auto-detect" } }], + [ + "my-org/web/High error rate", + { + ref: "High error rate", + target: { type: "explicit", org: "my-org", project: "web" }, + }, + ], + [ + `${PROJECT_SETTINGS_URL}42`, + { + ref: "42", + target: { type: "explicit", org: "acme", project: "web" }, + }, + ], + ])("parses %s before the command runs", (input, expected) => { + expect(parsedOf(command, input)).toEqual(expected); + }); + + test.each(["my-org/web", " "])("%s fails to parse", (input) => { + expect(() => bind(command, [input])).toThrow(ValidationError); + }); + + test("applies a pasted URL's server on resolve, not parse", async () => { + const [bound] = bind(command, [`${PROJECT_SETTINGS_URL}42`]); + expect(applyUrl).not.toHaveBeenCalled(); + await expect( + (bound as BoundPositional).resolved, + ).rejects.toBeInstanceOf(UrlApplied); + expect(applyUrl).toHaveBeenCalledWith(SERVER); + }); +}); + +describe.each(METRIC_RULE_COMMANDS)("%s", (_name, command) => { + test.each([ + ["42", { ref: "42", target: { type: "auto-detect" } }], + [ + "my-org/High error rate", + { ref: "High error rate", target: { type: "org-all", org: "my-org" } }, + ], + ])("parses %s before the command runs", (input, expected) => { + expect(parsedOf(command, input)).toEqual(expected); + }); + + test.each(["my-org/", "my-org/web/42", " "])( + "%s fails to parse", + (input) => { + expect(() => bind(command, [input])).toThrow(ValidationError); + }, + ); +}); + +describe.each(ALERT_TARGET_COMMANDS)("%s", (name, command) => { + test.each([ + ["my-org/", { type: "org-all", org: "my-org" }], + ["my-org/web", { type: "explicit", org: "my-org", project: "web" }], + ["web", { type: "project-search", projectSlug: "web" }], + ])("parses the target %s before the command runs", (input, expected) => { + const parsed = parsedOf(command, input); + // List targets wrap the parsed target with the name filter + const target = + command === alertMetricsListCommand + ? (parsed as { parsed: unknown }).parsed + : parsed; + expect(target).toEqual(expected); + }); + + test("applies a pasted URL's server on resolve, not parse", async () => { + // Target resolvers name the running command in usage hints + const [bound] = applyBoundPositionals( + [PROJECT_SETTINGS_URL], + positionalOf(command), + { ...CONTEXT, commandPrefix: ["sentry", ...name.split(" ")] }, + ); + expect(applyUrl).not.toHaveBeenCalled(); + await expect( + (bound as BoundPositional).resolved, + ).rejects.toBeInstanceOf(UrlApplied); + expect(applyUrl).toHaveBeenCalledWith(SERVER); + }); +}); From bc0e3294847b2b94f2a3825b833a184e13347c81 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 23:53:58 +0000 Subject: [PATCH 2/2] ref(cli): Drop the unbound target parsing allow-list UNBOUND_TARGET_PARSING_ALLOWED is empty now that every command parses its positionals before run. Delete it with ratchetedNames and allowedFollowUp, and have require-target-resolution report every banned reference in command files through bannedNames. Update AGENTS.md: neither positional rule has an allow-list. Part of #1564 Co-Authored-By: BYK --- packages/cli/AGENTS.md | 13 +++--- packages/cli/lint-rules/cli-oxlint-plugin.js | 43 +------------------- 2 files changed, 6 insertions(+), 50 deletions(-) diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index 1d994dc77..19d40661a 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -313,12 +313,9 @@ itself, and an omitted target arrives as auto-detect. The one of these types and never reads `.resolved`. It also rejects, in commands, what leaves a target for `func` to parse: the completion-only parsers `parseOrgTarget` and `parseOrgProjectTarget`, `parseOrgProjectArg`, and the -`resolve*FromArg` helpers. Commands not migrated yet are on -`UNBOUND_TARGET_PARSING_ALLOWED` in `lint-rules/cli-oxlint-plugin.js`, each -with its follow-up; the list is a ratchet, so a file that stops using them -fails until its entry is removed. `sentry-cli/no-ad-hoc-id-arguments` has no -allow-list: no command may parse IDs by hand, and the completion-only -`parseIssueTarget` and `parseDashboardTarget` are rejected too. +`resolve*FromArg` helpers. Neither rule has an allow-list: no command may +parse a target or an ID by hand. `sentry-cli/no-ad-hoc-id-arguments` rejects +the completion-only `parseIssueTarget` and `parseDashboardTarget` too. **Release positionals:** `[/]` uses `releaseParameter(USAGE_HINT, brief)` from `src/commands/release/parse.ts` @@ -395,8 +392,8 @@ dashboard are tagged with a completion kind from `src/lib/completion-target.ts` the `target-positional.ts` parsers behind `listTargetPositional`, `issuePositional`, and `releaseParameter` complete the right values. The completion-only parsers (`parseOrgProjectTarget`, `parseOrgTarget`, -`parseIssueTarget`) remain for the allow-listed -commands that still parse in `func`; new commands use a bound parser. Shared value +`parseIssueTarget`) are rejected in commands by the lint rules above; commands +use a bound parser. Shared value parsers are not tagged where they live; `PARSER_TARGETS` in `completion-target.ts` lists them instead, so every flag that parses with `sanitizeIssueQuery` completes issue search terms diff --git a/packages/cli/lint-rules/cli-oxlint-plugin.js b/packages/cli/lint-rules/cli-oxlint-plugin.js index e32a91a3d..872edec55 100644 --- a/packages/cli/lint-rules/cli-oxlint-plugin.js +++ b/packages/cli/lint-rules/cli-oxlint-plugin.js @@ -200,22 +200,6 @@ const UNBOUND_TARGET_PARSING = new Set([ "resolveProjectBoundFromArg", ]); -/** - * Commands still allowed to, each with its follow-up. A ratchet: an entry - * whose file no longer uses them is reported, so the list only shrinks. - */ -const UNBOUND_TARGET_PARSING_ALLOWED = new Map([ - // Follow-ups (#1564): -]); - -/** The follow-up of `file` in an allow-list, if it is on it. */ -function allowedFollowUp(allowed, file) { - for (const [path, followUp] of allowed) { - if (file.endsWith(path)) return followUp; - } - return undefined; -} - /** Whether `node` uses one of `names`; imports and `typeof` do not. */ function usesName(node, names) { return ( @@ -235,30 +219,6 @@ function bannedNames(file, names, report, message) { }; } -/** - * Like {@link bannedNames}, except in files on `allowed`; those are reported - * instead when they no longer use any, so the allow-list only shrinks. - */ -function ratchetedNames(file, names, allowed, report, message) { - if (!file.includes("/src/commands/")) return {}; - const followUp = allowedFollowUp(allowed, file); - let used = false; - return { - Identifier(node) { - if (!usesName(node, names)) return; - used = true; - if (followUp === undefined) report(node, message); - }, - "Program:exit"(node) { - if (followUp !== undefined && !used) - report( - node, - `Remove this file from the allow-list (${followUp}): it no longer parses its positional by hand.`, - ); - }, - }; -} - const ID_ARGUMENT_PARSERS = new Set(["parseIdArguments", "parseIdArgument"]); /** Whether `node` is `ID_KINDS.`. */ @@ -435,10 +395,9 @@ const rules = { FunctionDeclaration: check, FunctionExpression: check, ArrowFunctionExpression: check, - ...ratchetedNames( + ...bannedNames( file, UNBOUND_TARGET_PARSING, - UNBOUND_TARGET_PARSING_ALLOWED, report, "Take the target as a bound positional (src/lib/target-positional.ts, boundArguments) instead of parsing it in func.", ),