diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index aa642f2f5..8dc037ca5 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -250,6 +250,10 @@ the `sentry-cli/no-ad-hoc-id-arguments` lint rule rejects hand-written ID parsing (`parseSlashSeparatedArg`, `HEX_ID_RE`, ...) in commands and kinds defined outside `ID_KINDS`. Issue IDs keep their own syntax; `event view`, which also takes them, checks for an issue first (`parseEventViewInput`). +`test/lib/argument-conventions.test.ts` feeds the same inputs to every +command that takes IDs, issues, or releases, through the command's own +positional; add a new command to its tables, and encode a legitimate +difference (such as `needsProject`) in its row. **Issue positionals:** issue IDs, short IDs, selectors, and issue URLs are parsed by `parseIssueArgument` (`src/lib/issue-arguments.ts`), whose forms @@ -284,7 +288,21 @@ project), or `orgProjectTarget(resolve)` for a command-specific resolver (see with `await target.resolved`. It never calls `parseOrgProjectArg` or a resolver itself, and an omitted target arrives as auto-detect. The `sentry-cli/require-target-resolution` lint rule fails a function that takes -one of these types and never reads `.resolved`. +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. `AD_HOC_ID_PARSING_ALLOWED` does the same +for ID positionals, including the completion-only `parseIssueTarget` and +`parseDashboardTarget`. + +**Release positionals:** `[/]` uses +`releaseParameter(USAGE_HINT, brief)` from `src/commands/release/parse.ts` +inside a tuple positional. `func` takes a `ReleaseTarget` and gets +`{ version, org, detectedFrom }` from `await target.resolved`; a blank version +fails the parse. List commands take their target from `listTargetPositional()` in `src/lib/list-command.ts` (`LIST_TARGET_POSITIONAL`, @@ -340,13 +358,16 @@ ID looks like, so the syntax cannot drift between commands. It accepts any input; errors come from `parseIdArguments`, never Peggy's own syntax errors. **Shell completion:** Target positionals that take an org, project, issue, or -dashboard use a completion-aware parser from `src/lib/completion-target.ts` -instead of `String`: `parseOrgProjectTarget` completes `org/`, `org/project`, -and project aliases; `parseOrgTarget` completes org slugs only; -`parseIssueTarget` adds cached issue short IDs; `parseDashboardTarget` -completes cached dashboards. `issueIdsPositional` already uses them. A -parser tagged with `withCompletionTarget`, such as the `target-positional.ts` -parsers behind `listTargetPositional`, completes the same way. Shared value +dashboard are tagged with a completion kind from `src/lib/completion-target.ts` +(`ORG_PROJECT_TARGET` completes `org/`, `org/project`, and project aliases; +`ORG_TARGET` org slugs only; `ISSUE_TARGET` adds cached issue short IDs; +`DASHBOARD_TARGET` cached dashboards). The bound parsers above +(`boundPositional(kind, …)`, `boundArguments({ kind, … })`) carry the kind, so +the `target-positional.ts` parsers behind `listTargetPositional`, +`issuePositional`, and `releaseParameter` complete the right values. The +completion-only parsers (`parseOrgProjectTarget`, `parseOrgTarget`, +`parseIssueTarget`, `parseDashboardTarget`) remain for the allow-listed +commands that still parse in `func`; new 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 8ac95a893..a37fff22e 100644 --- a/packages/cli/lint-rules/cli-oxlint-plugin.js +++ b/packages/cli/lint-rules/cli-oxlint-plugin.js @@ -20,8 +20,8 @@ function rule(message, visitors) { create(context) { const filename = context.filename.replaceAll("\\", "/"); if (!filename.includes("/packages/cli/")) return {}; - return visitors(context, filename, (node) => - context.report({ node, message }), + return visitors(context, filename, (node, override = message) => + context.report({ node, message: override }), ); }, }; @@ -65,6 +65,7 @@ const BOUND_TARGET_TYPES = new Set([ "OrgOptionalTarget", "OrgProjectTarget", "ProjectBoundTarget", + "ReleaseTarget", "BoundArguments", "EventViewArguments", "LogListArguments", @@ -157,15 +158,127 @@ function regexSource(node) { /** * Helpers that parse ID positionals by hand. Commands parse IDs with * `parseIdArguments()` (src/lib/id-arguments.ts), so every command takes - * the same forms. + * the same forms. `parseIssueTarget` and `parseDashboardTarget` only tag a + * positional for completion, leaving the parsing to `func`; issues have + * their own grammar (`issuePositional`, src/lib/issue-arguments.ts). */ const AD_HOC_ID_PARSING = new Set([ "parseSlashSeparatedArg", "detectSwappedViewArgs", "HEX_ID_RE", "SPAN_ID_RE", + "parseIssueTarget", + "parseDashboardTarget", ]); +/** + * 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 AD_HOC_ID_PARSING_ALLOWED = new Map([ + // #1730 parses dashboard arguments with their own grammar + ["/src/commands/dashboard/restore.ts", "#1730"], + ["/src/commands/dashboard/revisions.ts", "#1730"], + ["/src/commands/dashboard/view.ts", "#1730"], + ["/src/commands/dashboard/widget/delete.ts", "#1730"], + ["/src/commands/dashboard/widget/edit.ts", "#1730"], + // Follow-up (#1564): several issues per command (`issue view`, + // `issue explain`, `issue merge`) need an array issue positional + ["/src/commands/issue/utils.ts", "#1564: array issue positional"], + ["/src/commands/issue/merge.ts", "#1564: array issue positional"], +]); + +/** + * Helpers that leave a target positional for `func` to parse: + * `parseOrgTarget` and `parseOrgProjectTarget` only tag it for completion, + * and the others parse a raw string (applying a pasted URL's server as they + * go). Commands take targets as bound positionals instead + * (src/lib/target-positional.ts, `boundArguments`), which parse before the + * command runs and resolve on `target.resolved`. + */ +const UNBOUND_TARGET_PARSING = new Set([ + "parseOrgTarget", + "parseOrgProjectTarget", + "parseOrgProjectArg", + "resolveOrgOnlyFromArg", + "resolveOrgOptionalFromArg", + "resolveProjectBoundFromArg", +]); + +/** Commands still allowed to, each with its follow-up; a ratchet too. */ +const UNBOUND_TARGET_PARSING_ALLOWED = new Map([ + // #1730 parses dashboard arguments with their own grammar + ["/src/commands/dashboard/list.ts", "#1730"], + ["/src/commands/dashboard/restore.ts", "#1730"], + ["/src/commands/dashboard/revisions.ts", "#1730"], + ["/src/commands/dashboard/view.ts", "#1730"], + ["/src/commands/dashboard/widget/add.ts", "#1730"], + ["/src/commands/dashboard/widget/delete.ts", "#1730"], + ["/src/commands/dashboard/widget/edit.ts", "#1730"], + // 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"], + // An org slug alone + ["/src/commands/org/view.ts", "#1564: org positional"], + ["/src/commands/trial/list.ts", "#1564: org positional"], + ["/src/commands/trial/start.ts", "#1564: org positional"], + // A target followed by other positionals + ["/src/commands/dashboard/create.ts", "#1564: target and title"], + ["/src/commands/init.ts", "#1564: target and directory"], + ["/src/commands/project/create.ts", "#1564: name and platform"], +]); + +/** 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; +} + +/** + * Visitors reporting each reference to one of `names` in a command file, + * except in files on `allowed`; those are reported instead when they no + * longer reference any, so the allow-list only shrinks. Imports and + * `typeof` references are not uses. + */ +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 ( + !names.has(node.name) || + node.parent?.type === "ImportSpecifier" || + node.parent?.type === "TSTypeQuery" + ) + 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.`. */ @@ -342,6 +455,13 @@ const rules = { FunctionDeclaration: check, FunctionExpression: check, ArrowFunctionExpression: check, + ...ratchetedNames( + 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.", + ), }; }, ), @@ -350,16 +470,13 @@ const rules = { (_context, file, report) => { if (!file.includes("/src/") || file.endsWith("/lib/id-arguments.ts")) return {}; - const commands = file.includes("/src/commands/"); return { - Identifier(node) { - if ( - commands && - AD_HOC_ID_PARSING.has(node.name) && - node.parent?.type !== "ImportSpecifier" - ) - report(node); - }, + ...ratchetedNames( + file, + AD_HOC_ID_PARSING, + AD_HOC_ID_PARSING_ALLOWED, + report, + ), CallExpression(node) { if ( ID_ARGUMENT_PARSERS.has(node.callee.name) && diff --git a/packages/cli/src/commands/release/archive.ts b/packages/cli/src/commands/release/archive.ts index 44faac56a..55794bf5f 100644 --- a/packages/cli/src/commands/release/archive.ts +++ b/packages/cli/src/commands/release/archive.ts @@ -9,7 +9,6 @@ import type { SentryContext } from "../../context.js"; import { updateRelease } from "../../lib/api-client.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { colorTag, escapeMarkdownInline, @@ -20,7 +19,7 @@ import { import { CommandOutput } from "../../lib/formatters/output.js"; import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../lib/mutate-command.js"; import type { SentryRelease } from "../../types/index.js"; -import { resolveReleaseTarget } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const USAGE_HINT = "sentry release archive [/]"; @@ -76,11 +75,10 @@ export const archiveCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version to archive", - parse: parseOrgTarget, - }, + releaseParameter( + USAGE_HINT, + "[/] - Release version to archive", + ), ], }, flags: { @@ -95,15 +93,9 @@ export const archiveCommand = buildCommand({ readonly json: boolean; readonly fields?: string[]; }, - target: string, + target: ReleaseTarget, ) { - const { cwd } = this; - - const { version, org, detectedFrom } = await resolveReleaseTarget( - target, - USAGE_HINT, - cwd, - ); + const { version, org, detectedFrom } = await target.resolved; if (flags["dry-run"]) { yield new CommandOutput({ dryRun: true, version, org }); diff --git a/packages/cli/src/commands/release/create.ts b/packages/cli/src/commands/release/create.ts index eb3c0e93c..126ba4a7c 100644 --- a/packages/cli/src/commands/release/create.ts +++ b/packages/cli/src/commands/release/create.ts @@ -7,7 +7,6 @@ import type { SentryContext } from "../../context.js"; import { createRelease } from "../../lib/api-client.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { colorTag, escapeMarkdownInline, @@ -18,7 +17,7 @@ import { import { CommandOutput } from "../../lib/formatters/output.js"; import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../lib/mutate-command.js"; import type { SentryRelease } from "../../types/index.js"; -import { resolveReleaseTarget } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const USAGE_HINT = "sentry release create [/]"; @@ -112,11 +111,10 @@ export const createCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version to create", - parse: parseOrgTarget, - }, + releaseParameter( + USAGE_HINT, + "[/] - Release version to create", + ), ], }, flags: { @@ -158,15 +156,9 @@ export const createCommand = buildCommand({ readonly json: boolean; readonly fields?: string[]; }, - target: string, + target: ReleaseTarget, ) { - const { cwd } = this; - - const { version, org } = await resolveReleaseTarget( - target, - USAGE_HINT, - cwd, - ); + const { version, org } = await target.resolved; const body: Parameters[1] = { version }; if (flags.project) { diff --git a/packages/cli/src/commands/release/delete.ts b/packages/cli/src/commands/release/delete.ts index 5a28df3b6..7907494cf 100644 --- a/packages/cli/src/commands/release/delete.ts +++ b/packages/cli/src/commands/release/delete.ts @@ -9,7 +9,6 @@ import type { SentryContext } from "../../context.js"; import { deleteRelease, getRelease } from "../../lib/api-client.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { ApiError } from "../../lib/errors.js"; import { renderMarkdown, safeCodeSpan } from "../../lib/formatters/markdown.js"; import { CommandOutput } from "../../lib/formatters/output.js"; @@ -19,7 +18,7 @@ import { isConfirmationBypassed, } from "../../lib/mutate-command.js"; import { buildReleaseUrl } from "../../lib/sentry-web-urls.js"; -import { resolveReleaseTarget } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const USAGE_HINT = "sentry release delete [/]"; @@ -105,22 +104,15 @@ export const deleteCommand = buildDeleteCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version to delete", - parse: parseOrgTarget, - }, + releaseParameter( + USAGE_HINT, + "[/] - Release version to delete", + ), ], }, }, - async *func(this: SentryContext, flags: DeleteFlags, target: string) { - const { cwd } = this; - - const { version, org } = await resolveReleaseTarget( - target, - USAGE_HINT, - cwd, - ); + async *func(this: SentryContext, flags: DeleteFlags, target: ReleaseTarget) { + const { version, org } = await target.resolved; // Verify the release exists before prompting for confirmation const release = await getRelease(org, version); diff --git a/packages/cli/src/commands/release/deploy.ts b/packages/cli/src/commands/release/deploy.ts index 93bc22d26..6685c4509 100644 --- a/packages/cli/src/commands/release/deploy.ts +++ b/packages/cli/src/commands/release/deploy.ts @@ -8,7 +8,6 @@ import type { SentryContext } from "../../context.js"; import { createReleaseDeploy } from "../../lib/api-client.js"; import { buildCommand, numberParser } from "../../lib/command.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { ContextError, ValidationError } from "../../lib/errors.js"; import { mdKvTable, @@ -18,9 +17,8 @@ import { import { CommandOutput } from "../../lib/formatters/output.js"; import { formatRelativeTime } from "../../lib/formatters/time-utils.js"; import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../lib/mutate-command.js"; -import { resolveOrg } from "../../lib/resolve-target.js"; import type { SentryDeploy } from "../../types/index.js"; -import { parseReleaseArg } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const USAGE_HINT = "sentry release deploy [/] [name]"; @@ -53,34 +51,24 @@ function formatDeployCreated(data: Record): string { } /** - * Parse the deploy positionals: `[org/]version`, `environment`, optional `name`. + * Check the deploy positionals after the release: the required + * `environment` and an optional `name`. * - * The first arg is parsed as the release target (version with optional org prefix). - * The second arg is the required environment. The third is an optional deploy - * name — multi-word names must be quoted (e.g. `"Deploy #42"`), matching standard - * CLI conventions and avoiding the silent `args.join(" ")` defect. + * The deploy name is a single positional — multi-word names must be quoted + * (e.g. `"Deploy #42"`), matching standard CLI conventions and avoiding the + * silent `args.join(" ")` defect. * - * @param target - The `[/]` release target * @param environment - The deploy environment * @param name - Optional deploy name */ function parseDeployArgs( - target: string, - environment: string, + environment: string | undefined, name?: string, -): { - version: string; - orgSlug?: string; - environment: string; - name?: string; -} { - if (!(target && environment)) { - throw new ContextError("Release version and environment", USAGE_HINT, []); +): { environment: string; name?: string } { + if (!environment) { + throw new ContextError("Environment", USAGE_HINT, []); } - - const { version, orgSlug } = parseReleaseArg(target, USAGE_HINT); - - return { version, orgSlug, environment, name: name || undefined }; + return { environment, name: name || undefined }; } export const deployCommand = buildCommand({ @@ -119,11 +107,7 @@ export const deployCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version", - parse: parseOrgTarget, - }, + releaseParameter(USAGE_HINT, "[/] - Release version"), { placeholder: "environment", brief: "Deploy environment (e.g. production)", @@ -179,21 +163,12 @@ export const deployCommand = buildCommand({ readonly json: boolean; readonly fields?: string[]; }, - target: string, + target: ReleaseTarget, environmentArg: string, nameArg?: string, ) { - const { cwd } = this; - - const { version, orgSlug, environment, name } = parseDeployArgs( - target, - environmentArg, - nameArg, - ); - const resolved = await resolveOrg({ org: orgSlug, cwd }); - if (!resolved) { - throw new ContextError("Organization", USAGE_HINT); - } + const { environment, name } = parseDeployArgs(environmentArg, nameArg); + const { version, org } = await target.resolved; const body: Parameters[2] = { environment }; if (name) { @@ -237,7 +212,7 @@ export const deployCommand = buildCommand({ return { hint: "Dry run — no deploy was created." }; } - const deploy = await createReleaseDeploy(resolved.org, version, body); + const deploy = await createReleaseDeploy(org, version, body); yield new CommandOutput(deploy); }, }); diff --git a/packages/cli/src/commands/release/deploys.ts b/packages/cli/src/commands/release/deploys.ts index 33c4884ee..2f8f706ff 100644 --- a/packages/cli/src/commands/release/deploys.ts +++ b/packages/cli/src/commands/release/deploys.ts @@ -7,12 +7,11 @@ import type { SentryContext } from "../../context.js"; import { listReleaseDeploys } from "../../lib/api-client.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { type Column, formatTable } from "../../lib/formatters/table.js"; import { formatRelativeTime } from "../../lib/formatters/time-utils.js"; import type { SentryDeploy } from "../../types/index.js"; -import { resolveReleaseTarget } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const USAGE_HINT = "sentry release deploys [/]"; @@ -58,26 +57,16 @@ export const deploysCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version", - parse: parseOrgTarget, - }, + releaseParameter(USAGE_HINT, "[/] - Release version"), ], }, }, async *func( this: SentryContext, _flags: { readonly json: boolean; readonly fields?: string[] }, - target: string, + target: ReleaseTarget, ) { - const { cwd } = this; - - const { version, org } = await resolveReleaseTarget( - target, - USAGE_HINT, - cwd, - ); + const { version, org } = await target.resolved; const deploys = await listReleaseDeploys(org, version); yield new CommandOutput(deploys); diff --git a/packages/cli/src/commands/release/finalize.ts b/packages/cli/src/commands/release/finalize.ts index 3d2b625a7..111880e73 100644 --- a/packages/cli/src/commands/release/finalize.ts +++ b/packages/cli/src/commands/release/finalize.ts @@ -7,7 +7,6 @@ import type { SentryContext } from "../../context.js"; import { updateRelease } from "../../lib/api-client.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { colorTag, escapeMarkdownInline, @@ -18,7 +17,7 @@ import { import { CommandOutput } from "../../lib/formatters/output.js"; import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../lib/mutate-command.js"; import type { SentryRelease } from "../../types/index.js"; -import { resolveReleaseTarget } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const USAGE_HINT = "sentry release finalize [/]"; @@ -71,11 +70,10 @@ export const finalizeCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version to finalize", - parse: parseOrgTarget, - }, + releaseParameter( + USAGE_HINT, + "[/] - Release version to finalize", + ), ], }, flags: { @@ -104,15 +102,9 @@ export const finalizeCommand = buildCommand({ readonly json: boolean; readonly fields?: string[]; }, - target: string, + target: ReleaseTarget, ) { - const { cwd } = this; - - const { version, org, detectedFrom } = await resolveReleaseTarget( - target, - USAGE_HINT, - cwd, - ); + const { version, org, detectedFrom } = await target.resolved; if (flags["dry-run"]) { yield new CommandOutput({ diff --git a/packages/cli/src/commands/release/parse.ts b/packages/cli/src/commands/release/parse.ts index f6ccfd852..5788ad2d9 100644 --- a/packages/cli/src/commands/release/parse.ts +++ b/packages/cli/src/commands/release/parse.ts @@ -6,12 +6,20 @@ * slug separators. This module provides version-aware parsing. */ +import { ORG_TARGET } from "../../lib/completion-target.js"; import { ContextError, ValidationError } from "../../lib/errors.js"; +import { + type BoundPositional, + boundPositional, +} from "../../lib/positional-parse.js"; import { resolveOrg } from "../../lib/resolve-target.js"; /** Slug pattern: lowercase alphanumeric + hyphens, no leading/trailing hyphen */ const SLUG_RE = /^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$/; +/** A parsed `[/]` argument. */ +type ParsedReleaseArg = { version: string; orgSlug?: string }; + /** * Parse a release positional argument: `[/]`. * @@ -31,7 +39,7 @@ const SLUG_RE = /^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$/; export function parseReleaseArg( arg: string, usageHint: string, -): { version: string; orgSlug?: string } { +): ParsedReleaseArg { const firstSlash = arg.indexOf("/"); if (firstSlash > 0) { @@ -65,39 +73,62 @@ export type ResolvedReleaseTarget = { detectedFrom?: string; }; +/** A parsed `[/]` positional, bound to its org resolver. */ +export type ReleaseTarget = BoundPositional< + ParsedReleaseArg, + ResolvedReleaseTarget +>; + +/** What the release resolver needs from the command context. */ +type ReleaseTargetContext = { readonly cwd: string }; + /** - * Resolve a single release positional argument into a concrete `{ version, org }`. - * - * Centralizes the version-only release-command boilerplate: it parses the - * `[/]` target, resolves the organization (from the parsed prefix - * or the surrounding project context), and raises consistent {@link ContextError}s - * when the version is missing or no organization can be determined. + * Parse a `[/]` positional, before the command runs. * - * Commands pass a single required positional (Stricli `kind: "tuple"`), so callers - * never need to join variadic args — preventing the silent - * `args.join(" ")` defect where extra positionals were swallowed into the version. - * - * @param target - The raw positional argument (e.g. `1.0.0` or `my-org/1.0.0`) - * @param usageHint - Single-line usage example for error messages - * @param cwd - Current working directory, used for context-based org detection - * @returns The resolved version and organization - * @throws {ContextError} If the version is empty or the org cannot be resolved + * @throws {ContextError} When the version is missing or blank */ -export async function resolveReleaseTarget( - target: string | undefined, +function parseReleaseInput( + raw: string | undefined, usageHint: string, - cwd: string, -): Promise { - const trimmed = target?.trim(); +): ParsedReleaseArg { + const trimmed = raw?.trim(); if (!trimmed) { throw new ContextError("Release version", usageHint, []); } + return parseReleaseArg(trimmed, usageHint); +} - const { version, orgSlug } = parseReleaseArg(trimmed, usageHint); - const resolved = await resolveOrg({ org: orgSlug, cwd }); - if (!resolved) { - throw new ContextError("Organization", usageHint); - } - - return { version, org: resolved.org, detectedFrom: resolved.detectedFrom }; +/** + * The `[/]` parameter of a release command's tuple positional. + * + * The argument is parsed before the command runs, so a blank version fails + * like any other argument. `target.resolved` finds the org (from the prefix, + * or the surrounding project context) inside `func`, after the auth guard. + * + * @param usageHint - Single-line usage example for error messages + * @param brief - Help text of the parameter + */ +export function releaseParameter(usageHint: string, brief: string) { + return { + placeholder: "org/version", + brief, + parse: boundPositional( + ORG_TARGET, + (raw) => parseReleaseInput(raw, usageHint), + async ( + { version, orgSlug }, + context: ReleaseTargetContext, + ): Promise => { + const resolved = await resolveOrg({ org: orgSlug, cwd: context.cwd }); + if (!resolved) { + throw new ContextError("Organization", usageHint); + } + return { + version, + org: resolved.org, + detectedFrom: resolved.detectedFrom, + }; + }, + ), + } as const; } diff --git a/packages/cli/src/commands/release/restore.ts b/packages/cli/src/commands/release/restore.ts index d354d0396..7fe05cc24 100644 --- a/packages/cli/src/commands/release/restore.ts +++ b/packages/cli/src/commands/release/restore.ts @@ -8,7 +8,6 @@ import type { SentryContext } from "../../context.js"; import { updateRelease } from "../../lib/api-client.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { colorTag, escapeMarkdownInline, @@ -19,7 +18,7 @@ import { import { CommandOutput } from "../../lib/formatters/output.js"; import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../lib/mutate-command.js"; import type { SentryRelease } from "../../types/index.js"; -import { resolveReleaseTarget } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const USAGE_HINT = "sentry release restore [/]"; @@ -73,11 +72,10 @@ export const restoreCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version to restore", - parse: parseOrgTarget, - }, + releaseParameter( + USAGE_HINT, + "[/] - Release version to restore", + ), ], }, flags: { @@ -92,15 +90,9 @@ export const restoreCommand = buildCommand({ readonly json: boolean; readonly fields?: string[]; }, - target: string, + target: ReleaseTarget, ) { - const { cwd } = this; - - const { version, org, detectedFrom } = await resolveReleaseTarget( - target, - USAGE_HINT, - cwd, - ); + const { version, org, detectedFrom } = await target.resolved; if (flags["dry-run"]) { yield new CommandOutput({ dryRun: true, version, org }); diff --git a/packages/cli/src/commands/release/set-commits.ts b/packages/cli/src/commands/release/set-commits.ts index fea3bb954..fab4935e3 100644 --- a/packages/cli/src/commands/release/set-commits.ts +++ b/packages/cli/src/commands/release/set-commits.ts @@ -12,7 +12,6 @@ import { setCommitsWithRefs, } from "../../lib/api-client.js"; import { buildCommand, numberParser } from "../../lib/command.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { getDatabase } from "../../lib/db/index.js"; import { clearMetadata, getMetadata, setMetadata } from "../../lib/db/utils.js"; import { @@ -34,7 +33,7 @@ import { } from "../../lib/git.js"; import { logger } from "../../lib/logger.js"; import type { SentryRelease } from "../../types/index.js"; -import { resolveReleaseTarget } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const log = logger.withTag("release.set-commits"); @@ -401,11 +400,7 @@ export const setCommitsCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version", - parse: parseOrgTarget, - }, + releaseParameter(USAGE_HINT, "[/] - Release version"), ], }, flags: { @@ -467,15 +462,11 @@ export const setCommitsCommand = buildCommand({ readonly json: boolean; readonly fields?: string[]; }, - target: string, + target: ReleaseTarget, ) { const { cwd } = this; - const { version, org } = await resolveReleaseTarget( - target, - USAGE_HINT, - cwd, - ); + const { version, org } = await target.resolved; // Clear mode: remove all commits regardless of other flags. // The server only clears commits when it receives an explicitly-empty diff --git a/packages/cli/src/commands/release/view.ts b/packages/cli/src/commands/release/view.ts index 4214806ad..89acbade4 100644 --- a/packages/cli/src/commands/release/view.ts +++ b/packages/cli/src/commands/release/view.ts @@ -8,7 +8,6 @@ import type { SentryContext } from "../../context.js"; import { getRelease } from "../../lib/api-client.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgTarget } from "../../lib/completion-target.js"; import { colorTag, escapeMarkdownCell, @@ -27,7 +26,7 @@ import { FRESH_FLAG, } from "../../lib/list-command.js"; import type { SentryRelease } from "../../types/index.js"; -import { resolveReleaseTarget } from "./parse.js"; +import { type ReleaseTarget, releaseParameter } from "./parse.js"; const USAGE_HINT = "sentry release view [/]"; @@ -189,11 +188,10 @@ export const viewCommand = buildCommand({ positional: { kind: "tuple", parameters: [ - { - placeholder: "org/version", - brief: "[/] - Release version to view", - parse: parseOrgTarget, - }, + releaseParameter( + USAGE_HINT, + "[/] - Release version to view", + ), ], }, flags: { @@ -208,16 +206,10 @@ export const viewCommand = buildCommand({ readonly json: boolean; readonly fields?: string[]; }, - target: string, + target: ReleaseTarget, ) { applyFreshFlag(flags); - const { cwd } = this; - - const { version, org, detectedFrom } = await resolveReleaseTarget( - target, - USAGE_HINT, - cwd, - ); + const { version, org, detectedFrom } = await target.resolved; const release = await getRelease(org, version, { health: true, diff --git a/packages/cli/test/commands/release/deploy.test.ts b/packages/cli/test/commands/release/deploy.test.ts index 3ccb843a9..c731cf716 100644 --- a/packages/cli/test/commands/release/deploy.test.ts +++ b/packages/cli/test/commands/release/deploy.test.ts @@ -137,7 +137,7 @@ describe("release deploy", () => { const func = await deployCommand.loader(); await expect(func.call(context, { json: false }, "1.0.0")).rejects.toThrow( - "Release version and environment", + "Environment is required", ); }); @@ -146,7 +146,7 @@ describe("release deploy", () => { const func = await deployCommand.loader(); await expect(func.call(context, { json: false })).rejects.toThrow( - "Release version and environment", + "Release version is required", ); }); }); diff --git a/packages/cli/test/lib/argument-conventions.test.ts b/packages/cli/test/lib/argument-conventions.test.ts new file mode 100644 index 000000000..9fb66aed4 --- /dev/null +++ b/packages/cli/test/lib/argument-conventions.test.ts @@ -0,0 +1,551 @@ +/** + * Argument conventions across commands. + * + * `id-arguments.test.ts` checks the shared grammar for each kind of ID. This + * file checks the commands built on it: the same input goes through each + * command's own positional, as Stricli parses it, and every command must + * read it the same way. Where a command legitimately differs, its row in + * {@link ID_COMMANDS} says how, so a difference is a decision, not drift. + */ + +import { ArgumentParseError } from "@stricli/core"; +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { listCommand as eventListCommand } from "../../src/commands/event/list.js"; +import { viewCommand as eventViewCommand } from "../../src/commands/event/view.js"; +import { resolveCommand as feedbackResolveCommand } from "../../src/commands/feedback/resolve.js"; +import { spamCommand as feedbackSpamCommand } from "../../src/commands/feedback/spam.js"; +import { unresolveCommand as feedbackUnresolveCommand } from "../../src/commands/feedback/unresolve.js"; +import { viewCommand as feedbackViewCommand } from "../../src/commands/feedback/view.js"; +import { archiveCommand } from "../../src/commands/issue/archive.js"; +import { eventsCommand as issueEventsCommand } from "../../src/commands/issue/events.js"; +import { linkCommand as issueLinkCommand } from "../../src/commands/issue/link.js"; +import { planCommand as issuePlanCommand } from "../../src/commands/issue/plan.js"; +import { resolveCommand as issueResolveCommand } from "../../src/commands/issue/resolve.js"; +import { tagsCommand as issueTagsCommand } from "../../src/commands/issue/tags.js"; +import { unlinkCommand as issueUnlinkCommand } from "../../src/commands/issue/unlink.js"; +import { unresolveCommand as issueUnresolveCommand } from "../../src/commands/issue/unresolve.js"; +import { listCommand as logListCommand } from "../../src/commands/log/list.js"; +import { viewCommand as logViewCommand } from "../../src/commands/log/view.js"; +import { archiveCommand as releaseArchiveCommand } from "../../src/commands/release/archive.js"; +import { createCommand as releaseCreateCommand } from "../../src/commands/release/create.js"; +import { deleteCommand as releaseDeleteCommand } from "../../src/commands/release/delete.js"; +import { deployCommand as releaseDeployCommand } from "../../src/commands/release/deploy.js"; +import { deploysCommand as releaseDeploysCommand } from "../../src/commands/release/deploys.js"; +import { finalizeCommand as releaseFinalizeCommand } from "../../src/commands/release/finalize.js"; +import { restoreCommand as releaseRestoreCommand } from "../../src/commands/release/restore.js"; +import { setCommitsCommand as releaseSetCommitsCommand } from "../../src/commands/release/set-commits.js"; +import { viewCommand as releaseViewCommand } from "../../src/commands/release/view.js"; +import { downloadCommand as replayDownloadCommand } from "../../src/commands/replay/download.js"; +import { viewCommand as replayViewCommand } from "../../src/commands/replay/view.js"; +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 { + applyBoundPositionals, + BoundPositional, +} from "../../src/lib/positional-parse.js"; +// oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking +import * as sentryUrlParser from "../../src/lib/sentry-url-parser.js"; + +const ID = "aaaa1111bbbb2222cccc3333dddd4444"; +const ID2 = "bbbb2222cccc3333dddd4444eeee5555"; +const SPAN_ID = "aaaa1111bbbb2222"; +const SERVER = "https://sentry.example.com"; +const TRACE_URL = `${SERVER}/organizations/acme/explore/traces/trace/${ID}/`; +const REPLAY_URL = `${SERVER}/organizations/acme/explore/replays/${ID}/`; +const EVENT_URL = `${SERVER}/organizations/acme/issues/123/events/${ID}/`; + +const REVERSED = /^Arguments appear reversed/; +const EXTRA = /^Extra arguments ignored/; + +/** How a command read its arguments, the same way for every command. */ +type Reading = { + /** + * `auto`, `org:`, `project:` (found in any org), or + * `/` + */ + readonly target: string; + readonly ids: readonly string[]; + readonly warnings: readonly string[]; + readonly baseUrl?: string; +}; + +/** A parsed `[/][]` target, as the parsers return it. */ +type ParsedTarget = { + readonly type: string; + readonly org?: string; + readonly project?: string; + readonly projectSlug?: string; +}; + +/** A parsed trace target, from `parseTraceArgumentsInput` and friends. */ +type TraceInput = { + readonly parsed: ParsedTarget & { readonly traceId: string }; + readonly warnings: readonly string[]; + readonly baseUrl?: string; +}; + +/** A `log list` or `span list` parse: a project's items, or a trace's. */ +type DualInput = + | { readonly mode: "trace"; readonly trace: TraceInput } + | { readonly mode: "project"; readonly target: ParsedTarget }; + +function describeTarget(target: ParsedTarget): string { + switch (target.type) { + case "auto-detect": + return "auto"; + case "explicit": + return `${target.org}/${target.project}`; + case "project-search": + return `project:${target.projectSlug}`; + default: + return `org:${target.org}`; + } +} + +function readTrace(input: TraceInput, extraIds: readonly string[] = []) { + return { + target: describeTarget(input.parsed), + ids: [input.parsed.traceId, ...extraIds], + warnings: input.warnings, + ...(input.baseUrl ? { baseUrl: input.baseUrl } : {}), + }; +} + +function readDual(input: DualInput): Reading { + return input.mode === "trace" + ? readTrace(input.trace) + : { target: describeTarget(input.target), ids: [], warnings: [] }; +} + +function readReplay(input: { + readonly replayId: string; + readonly target: ParsedTarget; + readonly warnings: readonly string[]; + readonly baseUrl?: string; +}): Reading { + return { + target: describeTarget(input.target), + ids: [input.replayId], + warnings: input.warnings, + ...(input.baseUrl ? { baseUrl: input.baseUrl } : {}), + }; +} + +/** A command that takes `[/[/]]`, and how it differs. */ +type IdCommand = { + readonly name: string; + readonly command: unknown; + readonly read: (parsed: unknown) => Reading; + /** + * What `/` names: org ``, or project `` for commands that + * need a project + */ + readonly slashTarget: "org" | "project"; + /** Whether more IDs are taken, not ignored with a warning */ + readonly multiple: boolean; + /** Whether the ID may be left out, to list a project's items instead */ + readonly optional: boolean; + /** A Sentry URL with this command's ID, if the command takes one */ + readonly url?: string; + /** Arguments every input gets after the ID, such as `span view`'s span ID */ + readonly trailing?: readonly string[]; + /** + * Whether `/ ` works. `span view` reads its first + * argument as the trace, so the target must be joined to it + */ + readonly separateTarget: boolean; +}; + +const ID_COMMANDS: readonly IdCommand[] = [ + { + name: "trace view", + command: traceViewCommand, + read: (parsed) => readTrace((parsed as { trace: TraceInput }).trace), + slashTarget: "org", + multiple: false, + optional: false, + url: TRACE_URL, + separateTarget: true, + }, + { + name: "trace logs", + command: traceLogsCommand, + read: (parsed) => readTrace(parsed as TraceInput), + slashTarget: "org", + multiple: false, + optional: false, + url: TRACE_URL, + separateTarget: true, + }, + { + name: "span view", + command: spanViewCommand, + read: (parsed) => { + const input = parsed as { + trace: TraceInput; + rawSpanIds: readonly string[]; + }; + return readTrace(input.trace, input.rawSpanIds); + }, + slashTarget: "project", + multiple: true, + optional: false, + url: TRACE_URL, + trailing: [SPAN_ID], + separateTarget: false, + }, + { + name: "span list", + command: spanListCommand, + read: (parsed) => readDual(parsed as DualInput), + slashTarget: "project", + multiple: false, + optional: true, + url: TRACE_URL, + separateTarget: true, + }, + { + name: "log list", + command: logListCommand, + read: (parsed) => readDual(parsed as DualInput), + slashTarget: "org", + multiple: false, + optional: true, + url: TRACE_URL, + separateTarget: true, + }, + { + name: "log view", + command: logViewCommand, + read: (parsed) => { + const input = parsed as { + rawLogIds: readonly string[]; + warnings: readonly string[]; + target: { parsed: ParsedTarget; baseUrl?: string }; + }; + return { + target: describeTarget(input.target.parsed), + ids: input.rawLogIds, + warnings: input.warnings, + ...(input.target.baseUrl ? { baseUrl: input.target.baseUrl } : {}), + }; + }, + slashTarget: "project", + multiple: true, + optional: false, + // Logs have no URL of their own + separateTarget: true, + }, + { + name: "event view", + command: eventViewCommand, + read: (parsed) => { + const input = parsed as { + eventId: string; + target: ParsedTarget; + extraEventIds?: readonly string[]; + warnings: readonly string[]; + baseUrl?: string; + }; + return { + target: describeTarget(input.target), + ids: [input.eventId, ...(input.extraEventIds ?? [])], + warnings: input.warnings, + ...(input.baseUrl ? { baseUrl: input.baseUrl } : {}), + }; + }, + slashTarget: "project", + multiple: true, + optional: false, + url: EVENT_URL, + separateTarget: true, + }, + { + name: "replay view", + command: replayViewCommand, + read: (parsed) => readReplay(parsed as Parameters[0]), + slashTarget: "org", + multiple: false, + optional: false, + url: REPLAY_URL, + separateTarget: true, + }, + { + name: "replay download", + command: replayDownloadCommand, + read: (parsed) => readReplay(parsed as Parameters[0]), + slashTarget: "org", + multiple: false, + optional: false, + url: REPLAY_URL, + separateTarget: true, + }, +]; + +/** Context Stricli passes to the parsers and resolvers. */ +const CONTEXT = { cwd: "/tmp", commandPrefix: ["sentry"] }; + +function positionalOf(command: unknown): unknown { + return (command as { parameters: { positional: unknown } }).parameters + .positional; +} + +/** Parse `args` with the command's positional, as Stricli would. */ +function bind(command: unknown, args: readonly string[]): unknown[] { + return applyBoundPositionals(args, positionalOf(command), CONTEXT); +} + +function parseWith(entry: IdCommand, args: readonly string[]): Reading { + const [bound] = bind(entry.command, [...args, ...(entry.trailing ?? [])]); + expect(bound).toBeInstanceOf(BoundPositional); + return entry.read((bound as BoundPositional).parsed); +} + +function parseError(command: unknown, args: readonly string[]): unknown { + try { + bind(command, args); + } catch (error) { + return error; + } + throw new Error(`Expected ${args.join(" ")} to fail to parse`); +} + +/** The IDs a command reads for `ids`, plus its trailing arguments. */ +function idsOf(entry: IdCommand, ...ids: string[]): string[] { + return [...ids, ...(entry.trailing ?? [])]; +} + +/** Thrown by the stubbed `applySentryUrlContext`, to stop resolving there. */ +class UrlApplied extends Error {} + +let applyUrl: ReturnType; + +beforeEach(() => { + applyUrl = vi + .spyOn(sentryUrlParser, "applySentryUrlContext") + .mockImplementation(() => { + throw new UrlApplied(); + }); + return () => applyUrl.mockRestore(); +}); + +describe.each(ID_COMMANDS)("$name", (entry) => { + test("a bare ID auto-detects the target", () => { + expect(parseWith(entry, [ID])).toEqual({ + target: "auto", + ids: idsOf(entry, ID), + warnings: [], + }); + }); + + test("/ names the org, or the project when one is needed", () => { + expect(parseWith(entry, [`my-org/${ID}`]).target).toBe( + entry.slashTarget === "org" ? "org:my-org" : "project:my-org", + ); + }); + + test("// names both", () => { + expect(parseWith(entry, [`my-org/web/${ID}`])).toEqual({ + target: "my-org/web", + ids: idsOf(entry, ID), + warnings: [], + }); + }); + + test.runIf(entry.separateTarget)( + "/ reads the same as //", + () => { + expect(parseWith(entry, ["my-org/web", ID])).toEqual( + parseWith(entry, [`my-org/web/${ID}`]), + ); + }, + ); + + test.runIf(!entry.separateTarget)( + "/ is not a target: the first argument is the ID", + () => { + const error = parseError(entry.command, ["my-org/web", ID, SPAN_ID]); + expect(error).toBeInstanceOf(ArgumentParseError); + expect((error as ArgumentParseError).exception).toBeInstanceOf( + ContextError, + ); + }, + ); + + test.runIf(entry.separateTarget)( + "reversed arguments are swapped back, with a warning", + () => { + const reading = parseWith(entry, [ID, "my-org/web"]); + expect(reading.target).toBe("my-org/web"); + expect(reading.ids).toEqual(idsOf(entry, ID)); + expect(reading.warnings).toEqual([expect.stringMatching(REVERSED)]); + }, + ); + + test( + entry.multiple + ? "takes more than one ID" + : "ignores extra arguments, with a warning", + () => { + const reading = parseWith(entry, [ID, ID2]); + if (entry.multiple) { + expect(reading.ids).toEqual([ID, ID2, ...(entry.trailing ?? [])]); + expect(reading.warnings).toEqual([]); + } else { + expect(reading.ids).toEqual([ID]); + expect(reading.warnings).toEqual([expect.stringMatching(EXTRA)]); + } + }, + ); + + test( + entry.optional + ? "no ID lists the auto-detected project's items" + : "a missing ID fails to parse, before the command runs", + () => { + if (entry.optional) { + expect(parseWith({ ...entry, trailing: [] }, [])).toEqual({ + target: "auto", + ids: [], + warnings: [], + }); + return; + } + const error = parseError(entry.command, []); + expect(error).toBeInstanceOf(ArgumentParseError); + expect((error as ArgumentParseError).exception).toBeInstanceOf( + ContextError, + ); + }, + ); + + test.runIf(entry.url !== undefined)( + "a pasted URL names the org; its server is applied on resolve, not parse", + async () => { + const url = entry.url ?? ""; + const [bound] = bind(entry.command, [url, ...(entry.trailing ?? [])]); + const reading = entry.read( + (bound as BoundPositional).parsed, + ); + expect(reading).toMatchObject({ + target: "org:acme", + ids: idsOf(entry, ID), + baseUrl: SERVER, + }); + expect(applyUrl).not.toHaveBeenCalled(); + + await expect( + (bound as BoundPositional).resolved, + ).rejects.toBeInstanceOf(UrlApplied); + expect(applyUrl).toHaveBeenCalledWith(SERVER); + }, + ); + + test.runIf(entry.url === undefined)( + "a pasted URL without this kind of ID fails to parse", + () => { + const error = parseError(entry.command, [TRACE_URL]); + expect(error).toBeInstanceOf(ArgumentParseError); + }, + ); +}); + +/** Commands that take one issue, through `issuePositional` or alike. */ +const ISSUE_COMMANDS: readonly (readonly [string, unknown])[] = [ + ["issue resolve", issueResolveCommand], + ["issue unresolve", issueUnresolveCommand], + ["issue archive", archiveCommand], + ["issue events", issueEventsCommand], + ["issue plan", issuePlanCommand], + ["issue tags", issueTagsCommand], + ["issue link", issueLinkCommand], + ["issue unlink", issueUnlinkCommand], + ["event list", eventListCommand], + ["feedback view", feedbackViewCommand], + ["feedback resolve", feedbackResolveCommand], + ["feedback spam", feedbackSpamCommand], + ["feedback unresolve", feedbackUnresolveCommand], +]; + +const ISSUE_INPUTS = [ + "CLI-K9", + "my-org/CLI-K9", + "my-org/web/CLI-K9", + "123456", + "my-org/123456", + "@latest", + "my-org/@most_frequent", + `${SERVER}/organizations/acme/issues/123456/`, +]; + +/** The issue argument a command parsed, from its first positional. */ +function parseIssue(command: unknown, input: string) { + const [bound] = bind(command, [input]); + expect(bound).toBeInstanceOf(BoundPositional); + return bound as BoundPositional; +} + +describe("issue arguments", () => { + const [[, reference], ...others] = ISSUE_COMMANDS; + + test.each( + ISSUE_INPUTS.flatMap((input) => + others.map(([name, command]) => [input, name, command] as const), + ), + )("%s reads the same in %s as in issue resolve", (input, _name, command) => { + expect(parseIssue(command, input).parsed).toEqual( + parseIssue(reference, input).parsed, + ); + }); + + test.each(ISSUE_COMMANDS)( + "%s applies a pasted issue URL's server on resolve, not parse", + async (_name, command) => { + const bound = parseIssue( + command, + `${SERVER}/organizations/acme/issues/123456/`, + ); + expect(bound.parsed).toMatchObject({ baseUrl: SERVER }); + expect(applyUrl).not.toHaveBeenCalled(); + + await expect(bound.resolved).rejects.toBeInstanceOf(UrlApplied); + expect(applyUrl).toHaveBeenCalledWith(SERVER); + }, + ); +}); + +/** Commands that take a `[/]` release. */ +const RELEASE_COMMANDS: readonly (readonly [string, unknown])[] = [ + ["release view", releaseViewCommand], + ["release archive", releaseArchiveCommand], + ["release create", releaseCreateCommand], + ["release delete", releaseDeleteCommand], + ["release deploy", releaseDeployCommand], + ["release deploys", releaseDeploysCommand], + ["release finalize", releaseFinalizeCommand], + ["release restore", releaseRestoreCommand], + ["release set-commits", releaseSetCommitsCommand], +]; + +describe.each(RELEASE_COMMANDS)("%s", (_name, command) => { + test.each([ + ["1.0.0", { version: "1.0.0" }], + ["my-org/1.0.0", { orgSlug: "my-org", version: "1.0.0" }], + // A prefix that is not a slug is part of the version + ["sentry-cli@2.0.0", { version: "sentry-cli@2.0.0" }], + ["my-org/feature/x", { orgSlug: "my-org", version: "feature/x" }], + ])("parses %s before the command runs", (input, expected) => { + const [bound] = bind(command, [input]); + expect(bound).toBeInstanceOf(BoundPositional); + expect((bound as BoundPositional).parsed).toEqual( + expected, + ); + }); + + test("a blank version fails to parse", () => { + expect(() => bind(command, [" "])).toThrow(ContextError); + }); +}); diff --git a/packages/cli/test/lib/release-parse.property.test.ts b/packages/cli/test/lib/release-parse.property.test.ts index 0aba30d7c..02b4928bd 100644 --- a/packages/cli/test/lib/release-parse.property.test.ts +++ b/packages/cli/test/lib/release-parse.property.test.ts @@ -2,7 +2,8 @@ import { array, constantFrom, assert as fcAssert, property } from "fast-check"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { parseReleaseArg, - resolveReleaseTarget, + type ReleaseTarget, + releaseParameter, } from "../../src/commands/release/parse.js"; import { ContextError, ValidationError } from "../../src/lib/errors.js"; import { DEFAULT_NUM_RUNS } from "../model-based/helpers.js"; @@ -133,7 +134,7 @@ describe("unit: parseReleaseArg edge cases", () => { }); }); -describe("unit: resolveReleaseTarget", () => { +describe("unit: releaseParameter", () => { let resolveOrgSpy: ReturnType; beforeEach(() => { @@ -145,36 +146,41 @@ describe("unit: resolveReleaseTarget", () => { }); const USAGE = "sentry release archive [/]"; + const { parse } = releaseParameter(USAGE, "Release version"); + const bind = (raw?: string): ReleaseTarget => + parse.call({ cwd: "/tmp" }, raw); + + test("parses the version and org without resolving", () => { + expect(bind("my-org/1.0.0").parsed).toEqual({ + orgSlug: "my-org", + version: "1.0.0", + }); + expect(resolveOrgSpy).not.toHaveBeenCalled(); + }); test("resolves version and org from a target", async () => { resolveOrgSpy.mockResolvedValue({ org: "my-org" }); - const result = await resolveReleaseTarget("my-org/1.0.0", USAGE, "/tmp"); + const result = await bind("my-org/1.0.0").resolved; expect(result).toEqual({ version: "1.0.0", org: "my-org" }); expect(resolveOrgSpy).toHaveBeenCalledWith({ org: "my-org", cwd: "/tmp" }); }); test("forwards detectedFrom from the resolved org", async () => { resolveOrgSpy.mockResolvedValue({ org: "my-org", detectedFrom: "DSN" }); - const result = await resolveReleaseTarget("1.0.0", USAGE, "/tmp"); + const result = await bind("1.0.0").resolved; expect(result.detectedFrom).toBe("DSN"); }); - test("throws ContextError when target is undefined", async () => { - await expect( - resolveReleaseTarget(undefined, USAGE, "/tmp"), - ).rejects.toThrow(ContextError); + test("throws ContextError when parsing an omitted target", () => { + expect(() => bind(undefined)).toThrow(ContextError); }); - test("throws ContextError when target is whitespace", async () => { - await expect(resolveReleaseTarget(" ", USAGE, "/tmp")).rejects.toThrow( - "Release version", - ); + test("throws ContextError when parsing a whitespace target", () => { + expect(() => bind(" ")).toThrow("Release version"); }); test("throws ContextError when org cannot be resolved", async () => { resolveOrgSpy.mockResolvedValue(null); - await expect(resolveReleaseTarget("1.0.0", USAGE, "/tmp")).rejects.toThrow( - ContextError, - ); + await expect(bind("1.0.0").resolved).rejects.toThrow(ContextError); }); }); diff --git a/scripts/cli-lint-rules.test.mjs b/scripts/cli-lint-rules.test.mjs index 5184b826d..1b1919cfa 100644 --- a/scripts/cli-lint-rules.test.mjs +++ b/scripts/cli-lint-rules.test.mjs @@ -153,6 +153,23 @@ tester.run("no-ad-hoc-id-arguments", plugin.rules["no-ad-hoc-id-arguments"], { errors: 1, }, { filename: commands, code: "HEX_ID_RE.test(value);", errors: 1 }, + { + // Only tags the positional for completion; func would parse it + filename: commands, + code: "const issue = { placeholder: 'issue', parse: parseIssueTarget };", + errors: 1, + }, + { + filename: commands, + code: "const dashboard = { parse: parseDashboardTarget };", + errors: 1, + }, + { + // A ratchet: an allow-listed file that stopped parsing IDs by hand + filename: "/repo/packages/cli/src/commands/issue/merge.ts", + code: "const issues = issuePositional(USAGE_HINT);", + errors: [{ message: /Remove this file from the allow-list \(#1564/ }], + }, { filename: source, code: "parseIdArguments(args, { label: 'Thing ID', isId }, options);", @@ -161,6 +178,66 @@ tester.run("no-ad-hoc-id-arguments", plugin.rules["no-ad-hoc-id-arguments"], { ], }); +tester.run( + "require-target-resolution", + plugin.rules["require-target-resolution"], + { + valid: [ + { + filename: commands, + code: "async function* run(flags, target: ReleaseTarget) { const { org } = await target.resolved; }", + }, + { + filename: commands, + code: "import { parseOrgProjectArg } from './arg-parsing.js'; type Parsed = ReturnType;", + }, + { + // Libraries implement the bound positionals + filename: source, + code: "const parsed = parseOrgProjectArg(target);", + }, + { + // On the allow-list, with its follow-up + filename: "/repo/packages/cli/src/commands/org/view.ts", + code: "const org = { placeholder: 'org', parse: parseOrgTarget };", + }, + ], + invalid: [ + { + filename: commands, + code: "async function* run(flags, target: ReleaseTarget) { return target.parsed; }", + errors: 1, + }, + { + filename: commands, + code: "const org = { placeholder: 'org', parse: parseOrgTarget };", + errors: 1, + }, + { + filename: commands, + code: "const target = { parse: parseOrgProjectTarget };", + errors: 1, + }, + { + filename: commands, + code: "async function* run(flags, arg) { const parsed = parseOrgProjectArg(arg); }", + errors: 1, + }, + { + filename: commands, + code: "const org = await resolveOrgOnlyFromArg(arg, cwd, 'alert metrics view');", + errors: 1, + }, + { + // A ratchet: an allow-listed file that takes a bound target now + filename: "/repo/packages/cli/src/commands/org/view.ts", + code: "const org = { parse: orgOptionalTarget };", + errors: [{ message: /Remove this file from the allow-list/ }], + }, + ], + }, +); + tester.run("no-raw-metadata-queries", plugin.rules["no-raw-metadata-queries"], { valid: [ {