diff --git a/apps/cli-docs/src/content/docs/agent-guidance.md b/apps/cli-docs/src/content/docs/agent-guidance.md index 2c7b2c504..c016145f4 100644 --- a/apps/cli-docs/src/content/docs/agent-guidance.md +++ b/apps/cli-docs/src/content/docs/agent-guidance.md @@ -54,6 +54,7 @@ The CLI uses semantic exit codes. Key ranges for agents: | 40–49 | Feature unavailable | Inform user about plan/settings | | 50–59 | Operation error | Report to user | | 60–69 | Command-specific | Check stderr for details | +| 252 | Invalid argument | Check the command's `--help` and fix the argument | See [Exit Codes](/exit-codes/) for the complete reference. diff --git a/apps/cli-docs/src/content/docs/exit-codes.md b/apps/cli-docs/src/content/docs/exit-codes.md index 715268c69..a7c56051b 100644 --- a/apps/cli-docs/src/content/docs/exit-codes.md +++ b/apps/cli-docs/src/content/docs/exit-codes.md @@ -30,7 +30,7 @@ react to failure categories without parsing stderr. | 12 | Token Invalid | Auth token rejected by the server | | 13 | Host Scope | Request blocked — credentials don't match the target host | | 20 | Config Error | Configuration or DSN problem | -| 21 | Validation Error | Invalid input (malformed ID, bad flag value, etc.) | +| 21 | Validation Error | Invalid input found while the command runs (malformed ID, unknown value, etc.) | | 22 | Missing Context | Required context (org, project) could not be determined | | 23 | Not Found | A user-provided identifier could not be resolved | | 30 | API Error | Sentry API returned an error response | @@ -45,6 +45,7 @@ react to failure categories without parsing stderr. | 62 | Wizard Deps | Wizard dependency installation failed | | 63 | Wizard Codemod | Wizard codemod plan or apply failed | | 64 | Wizard Verify | User stopped wizard after verification step | +| 252 | Invalid Argument | An argument could not be parsed — see [Argument errors](#argument-errors) | ## Scripting Examples @@ -83,6 +84,18 @@ elif 40 <= result.returncode <= 49: print("Feature not available") ``` +## Argument errors + +Arguments are parsed before the command runs. When one can't be parsed, the +CLI prints `Failed to parse "" for : ` and exits with +252, whatever the reason. This covers: + +- unknown flags and missing required arguments +- invalid flag values, such as `--period bogus` or `--limit abc` +- invalid `/` targets, such as `sentry trace list @latest` + +Run the command with `--help` to see the arguments it accepts. + ## Notes - Exit codes below 128 are safe from collision with Unix signal exits (128+N). @@ -97,4 +110,5 @@ elif 40 <= result.returncode <= 49: verification stopped → 64 (Wizard Verify), other → 61 (Wizard). - [Stricli](https://bloomberg.github.io/stricli/) (the CLI framework) uses negative exit codes (-5 to -1) for framework-level errors like unknown - commands or invalid arguments. These appear as 251–255 in unsigned form. + commands (251) or [invalid arguments](#argument-errors) (252). These + appear as 251–255 in unsigned form. diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index 5fb0eb025..ac6537f4d 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -254,17 +254,19 @@ one of these types and never reads `.resolved`. Resolvers need no command name: they read it from `this.commandPrefix`, which Stricli's `forCommand` and the SDK invoker set (test mock contexts set it too). In a destructive command (`buildDeleteCommand`, or `buildCommand` with -`destructive: true`), the target refuses to auto-detect, so those commands -don't call `requireExplicitTarget` for it. - -Both steps are lazy (`lazyPositional` in `src/lib/positional-parse.ts`): the -parse runs on the first `parsed` read and the resolver on the first -`resolved` read, both inside `func` after the auth guard. Invalid input still -throws the parser's `CliError` with its exit code. `target.raw` is what the -user typed; shared code that handles any positional uses -`rawPositional(value)`. `buildCommand` runs these parsers on -positionals that arrive as strings or are omitted, so the SDK invoke path and -tests that call `command.loader()` keep passing raw strings. Positionals whose +`destructive: true`), an auto-detected target fails before `func` runs, so +those commands don't call `requireExplicitTarget` for it. + +`boundPositional` in `src/lib/positional-parse.ts` builds these parsers. +Stricli runs the parse in its argument scanner, so invalid input fails like +any other argument: `Failed to parse "" for : …` and exit +code 252. The parse must have no side effects; anything that changes state +(such as applying a pasted Sentry URL's server) goes in the resolver, which +runs on the first `resolved` read, inside `func` after the auth guard. +`target.raw` is what the user typed; shared code that handles any positional +uses `rawPositional(value)`. `buildCommand` runs these parsers on positionals +that arrive as strings or are omitted, so the SDK invoke path and tests that +call `command.loader()` keep passing raw strings. Positionals whose meaning depends on how many arguments were given, such as `[//]`, keep parsing in `func`. diff --git a/packages/cli/lint-rules/cli-oxlint-plugin.js b/packages/cli/lint-rules/cli-oxlint-plugin.js index bcc220a4d..409dd1f97 100644 --- a/packages/cli/lint-rules/cli-oxlint-plugin.js +++ b/packages/cli/lint-rules/cli-oxlint-plugin.js @@ -294,7 +294,7 @@ const rules = { : {}, ), "require-target-resolution": rule( - "Await `.resolved`: it runs the target's resolver and destructive-command guard.", + "Await `.resolved`: it runs the target's resolver.", (_context, file, report) => { if (!file.includes("/src/commands/")) return {}; const check = (node) => { diff --git a/packages/cli/plugins/sentry-cli/skills/sentry-cli/SKILL.md b/packages/cli/plugins/sentry-cli/skills/sentry-cli/SKILL.md index f5536126b..4841380bb 100644 --- a/packages/cli/plugins/sentry-cli/skills/sentry-cli/SKILL.md +++ b/packages/cli/plugins/sentry-cli/skills/sentry-cli/SKILL.md @@ -64,6 +64,7 @@ The CLI uses semantic exit codes. Key ranges for agents: | 40–49 | Feature unavailable | Inform user about plan/settings | | 50–59 | Operation error | Report to user | | 60–69 | Command-specific | Check stderr for details | +| 252 | Invalid argument | Check the command's `--help` and fix the argument | See [Exit Codes](/exit-codes/) for the complete reference. diff --git a/packages/cli/src/app.ts b/packages/cli/src/app.ts index 49002ebcc..21a69c3a6 100644 --- a/packages/cli/src/app.ts +++ b/packages/cli/src/app.ts @@ -74,7 +74,7 @@ import { } from "./lib/command-suggestions.js"; import { CLI_VERSION } from "./lib/constants.js"; import { redactCredentialText } from "./lib/credential-redaction.js"; -import { reportCliError } from "./lib/error-reporting.js"; +import { reportArgumentError, reportCliError } from "./lib/error-reporting.js"; import { ApiError, AuthError, @@ -85,6 +85,7 @@ import { WizardError, } from "./lib/errors.js"; import { error as errorColor, warning } from "./lib/formatters/colors.js"; +import { stripAnsi } from "./lib/formatters/plain-detect.js"; import { buildTopLevelFlags } from "./lib/global-flags.js"; import { renderJsonHelp } from "./lib/help.js"; import { isRouteMap, type RouteMap } from "./lib/introspect.js"; @@ -312,6 +313,38 @@ function escapesToOuterMiddleware(exc: unknown): boolean { return exc instanceof ApiError && (exc.status === 401 || exc.status === 403); } +/** Message for an argument that failed to parse, with our hints. */ +function formatArgumentError(exc: unknown, ansiColor: boolean): string { + // Case A: bare route group with no subcommand (e.g., `sentry issue`) + if (exc instanceof UnsatisfiedPositionalError) { + const bareHint = detectBareRouteGroup(ansiColor); + if (bareHint) { + return bareHint; + } + } + + // Case B + plural alias: extra args that Stricli can't consume + if (exc instanceof UnexpectedPositionalError) { + const pluralHint = detectPluralAliasMisuse(ansiColor); + if (pluralHint) { + return `${text_en.exceptionWhileParsingArguments(exc, ansiColor)}${pluralHint}`; + } + + // With defaultCommand: "view", unknown tokens like "metrics" fill the + // positional slot, then extra args (e.g., CLI-AB) trigger this error. + // Check if the first non-route token is a known synonym. + const synonymHint = getSynonymSuggestionFromArgv(); + if (synonymHint) { + const tip = ansiColor + ? warning(`\nTip: ${synonymHint}`) + : `\nTip: ${synonymHint}`; + return `${text_en.exceptionWhileParsingArguments(exc, ansiColor)}${tip}`; + } + } + + return text_en.exceptionWhileParsingArguments(exc, ansiColor); +} + /** * Custom error formatting for CLI errors. * @@ -325,34 +358,10 @@ const customText: ApplicationText = { exc: unknown, ansiColor: boolean, ): string => { - // Case A: bare route group with no subcommand (e.g., `sentry issue`) - if (exc instanceof UnsatisfiedPositionalError) { - const bareHint = detectBareRouteGroup(ansiColor); - if (bareHint) { - return bareHint; - } - } - - // Case B + plural alias: extra args that Stricli can't consume - if (exc instanceof UnexpectedPositionalError) { - const pluralHint = detectPluralAliasMisuse(ansiColor); - if (pluralHint) { - return `${text_en.exceptionWhileParsingArguments(exc, ansiColor)}${pluralHint}`; - } - - // With defaultCommand: "view", unknown tokens like "metrics" fill the - // positional slot, then extra args (e.g., CLI-AB) trigger this error. - // Check if the first non-route token is a known synonym. - const synonymHint = getSynonymSuggestionFromArgv(); - if (synonymHint) { - const tip = ansiColor - ? warning(`\nTip: ${synonymHint}`) - : `\nTip: ${synonymHint}`; - return `${text_en.exceptionWhileParsingArguments(exc, ansiColor)}${tip}`; - } - } - - return text_en.exceptionWhileParsingArguments(exc, ansiColor); + const message = formatArgumentError(exc, ansiColor); + // Exit 252 never reaches reportCliError; record it for UX research + reportArgumentError(exc, stripAnsi(message)); + return message; }, noCommandRegisteredForInput: ({ input, corrections, ansiColor }): string => { // Default error message from Stricli (e.g., "No command registered for `info`") diff --git a/packages/cli/src/commands/project/delete.ts b/packages/cli/src/commands/project/delete.ts index 90d1cba16..949e2524a 100644 --- a/packages/cli/src/commands/project/delete.ts +++ b/packages/cli/src/commands/project/delete.ts @@ -14,8 +14,8 @@ * Safety measures: * - Uses `buildDeleteCommand` — auto-injects `--yes`/`--force`/`--dry-run` * flags and enforces the non-interactive guard before `func()` runs - * - No auto-detect mode: `buildDeleteCommand` marks the target destructive, so - * `target.resolved` rejects an omitted target + * - No auto-detect mode: `buildDeleteCommand` marks the command destructive, + * so an omitted target fails before `func()` runs * - Type-out confirmation via `confirmByTyping` (unless --yes/--force) */ diff --git a/packages/cli/src/lib/arg-parsing.ts b/packages/cli/src/lib/arg-parsing.ts index 0e6d50792..1d5dd2d2d 100644 --- a/packages/cli/src/lib/arg-parsing.ts +++ b/packages/cli/src/lib/arg-parsing.ts @@ -754,28 +754,56 @@ export function parseOrgProjectArg( arg: string | undefined, options: { multi?: boolean } = {}, ): ParsedOrgProject { + const { parsed, baseUrl } = parseOrgProjectInput(arg, options); + if (baseUrl) { + applySentryUrlContext(baseUrl); + } + return parsed; +} + +/** A parsed org/project argument and the server of a pasted Sentry URL. */ +export type OrgProjectInput = { + readonly parsed: ParsedOrgProject; + /** Base URL of a pasted Sentry web URL, for `applySentryUrlContext` */ + readonly baseUrl?: string; +}; + +/** + * Parse an org/project argument like {@link parseOrgProjectArg}, without + * side effects: the server of a pasted Sentry URL is returned as `baseUrl` + * instead of applied, so the caller applies it with `applySentryUrlContext` + * when it is safe to. + */ +export function parseOrgProjectInput( + arg: string | undefined, + options: { multi?: boolean } = {}, +): OrgProjectInput { // Keep only the first non-blank line: agents often pass targets with // appended output or notes after a newline (CLI-1RA). const trimmed = arg ? firstNonBlankLine(arg) : ""; if (!trimmed) { - return { type: "auto-detect" }; + return { parsed: { type: "auto-detect" } }; } // URL detection — extract org/project from Sentry web URLs const urlParsed = parseSentryUrl(trimmed); if (urlParsed) { - applySentryUrlContext(urlParsed.baseUrl); - return orgProjectFromUrl(urlParsed); + return { + parsed: orgProjectFromUrl(urlParsed), + baseUrl: urlParsed.baseUrl, + }; } - return parseTarget(trimmed, { - multi: options.multi === true, - checkSlug: validateResourceId, - rejectAt: rejectAtSelector, - invalid: (message, field) => { - throw new ValidationError(message, field); - }, - }); + return { + parsed: parseTarget(trimmed, { + multi: options.multi === true, + checkSlug: validateResourceId, + rejectAt: rejectAtSelector, + invalid: (message, field) => { + throw new ValidationError(message, field); + }, + }), + }; } /** diff --git a/packages/cli/src/lib/command.ts b/packages/cli/src/lib/command.ts index c3df5aafc..88535c601 100644 --- a/packages/cli/src/lib/command.ts +++ b/packages/cli/src/lib/command.ts @@ -66,8 +66,8 @@ import { setLogLevel, } from "./logger.js"; import { - applyLazyPositionals, - markDestructive, + applyBoundPositionals, + checkDestructive, rawPositional, } from "./positional-parse.js"; import { setArgsContext, setFlagContext, withTracing } from "./telemetry.js"; @@ -201,7 +201,7 @@ type LocalCommandBuilderArguments< * Mark the command as destructive or irreversible. Defaults to `false`. * * Destructive commands are listed in the agent Safety Rules (generated from - * this flag), so agents ask the user before running them, and their lazy + * this flag), so agents ask the user before running them, and their bound * target positionals refuse an auto-detected target. * `buildDeleteCommand` sets this automatically, and `check:destructive` * fails CI when a command that sends an HTTP DELETE lacks it. Set it @@ -730,17 +730,13 @@ export function buildCommand< ...positionalArgs: unknown[] ) { // Callers that skip Stricli's scanner (SDK invoke, tests) pass raw - // strings, and omitted optional positionals are undefined; give lazily - // parsed positionals the value Stricli would. - const args = applyLazyPositionals( + // strings, and omitted optional positionals are undefined; give bound + // positionals the value Stricli would. + const args = applyBoundPositionals( positionalArgs, existingParams.positional, this, ); - // Lazy target positionals of destructive commands refuse to auto-detect - if (builderArgs.destructive) { - markDestructive(args); - } applyLoggingFlags( flags[LOG_LEVEL_KEY] as LogLevelName | undefined, @@ -835,6 +831,12 @@ export function buildCommand< await assertRcUrlTrusted(this.cwd as string); } + // Bound target positionals of destructive commands refuse to + // auto-detect, whether `func` reads `parsed` or `resolved` + if (builderArgs.destructive) { + checkDestructive(args); + } + // Execution phase: core command logic, API calls, org/project resolution const returned = await withTracing( "exec", diff --git a/packages/cli/src/lib/error-reporting.ts b/packages/cli/src/lib/error-reporting.ts index 529b4589f..fc8be2699 100644 --- a/packages/cli/src/lib/error-reporting.ts +++ b/packages/cli/src/lib/error-reporting.ts @@ -27,6 +27,17 @@ // oxlint-disable-next-line sentry-cli/no-namespace-import -- Sentry SDK recommends namespace import import * as Sentry from "@sentry/node-core/light"; +import { + AliasNotFoundError, + ArgumentParseError, + EnumValidationError, + FlagNotFoundError, + InvalidNegatedFlagSyntaxError, + UnexpectedFlagError, + UnexpectedPositionalError, + UnsatisfiedFlagError, + UnsatisfiedPositionalError, +} from "@stricli/core"; import { redactCredentialText } from "./credential-redaction.js"; import { ApiError, @@ -437,6 +448,66 @@ export function reportCliError(error: unknown): void { }); } +/** The class name of a thrown value, for event context. */ +function errorClassName(error: unknown): string { + return error instanceof Error ? error.name : typeof error; +} + +/** + * Stricli's argument scanner errors by name. They don't set `name`, and the + * minified bundle renames classes, so match them by identity. + */ +const ARGUMENT_ERROR_CLASSES = { + AliasNotFoundError, + ArgumentParseError, + EnumValidationError, + FlagNotFoundError, + InvalidNegatedFlagSyntaxError, + UnexpectedFlagError, + UnexpectedPositionalError, + UnsatisfiedFlagError, + UnsatisfiedPositionalError, +}; + +/** The Stricli class name of an argument scanner error. */ +function argumentErrorClassName(error: unknown): string { + for (const [name, errorClass] of Object.entries(ARGUMENT_ERROR_CLASSES)) { + if (error instanceof errorClass) { + return name; + } + } + return errorClassName(error); +} + +/** + * Report an argument that Stricli could not parse (exit 252) for UX research. + * + * These are user mistakes rather than CLI bugs, so like `unknown_command` + * they go to Sentry as one fixed `info` message that groups into a single + * issue. The `invalid_argument` context holds the per-event detail. + * + * @param error - The error Stricli passed to `exceptionWhileParsingArguments` + * @param message - What the user saw, without ANSI color codes + */ +export function reportArgumentError(error: unknown, message: string): void { + const errorClass = argumentErrorClassName(error); + Sentry.captureMessage("invalid_argument", { + level: "info", + tags: { "argument_error.class": errorClass }, + contexts: { + invalid_argument: { + error_class: errorClass, + // What the argument's parser threw, e.g. a ValidationError + cause_class: + error instanceof ArgumentParseError + ? errorClassName(error.exception) + : null, + message: redactCredentialText(message), + }, + }, + }); +} + /** * Enrich an error event with `cli_error.*` tags for server-side fingerprinting. * diff --git a/packages/cli/src/lib/list-command.ts b/packages/cli/src/lib/list-command.ts index 305bf9016..b50bd0de3 100644 --- a/packages/cli/src/lib/list-command.ts +++ b/packages/cli/src/lib/list-command.ts @@ -44,7 +44,7 @@ import { type ListResult, type OrgListConfig, } from "./org-list.js"; -import { applyLazyPositionals, rawPositional } from "./positional-parse.js"; +import { applyBoundPositionals, rawPositional } from "./positional-parse.js"; import { disableResponseCache } from "./response-cache.js"; import { PERIOD_BRIEF, parsePeriod } from "./time-range.js"; import { shellQuote } from "./utils.js"; @@ -641,8 +641,8 @@ export function buildListCommand< args[0] = undefined; } } - // A dropped lazy target becomes an omitted one (auto-detect) - const finalArgs = applyLazyPositionals(args, params.positional, this); + // A dropped bound target becomes an omitted one (auto-detect) + const finalArgs = applyBoundPositionals(args, params.positional, this); return originalFunc.call(this, flags, ...(finalArgs as unknown as ARGS)); } as typeof originalFunc; diff --git a/packages/cli/src/lib/positional-parse.ts b/packages/cli/src/lib/positional-parse.ts index aec33f5c8..d4908d7da 100644 --- a/packages/cli/src/lib/positional-parse.ts +++ b/packages/cli/src/lib/positional-parse.ts @@ -1,37 +1,29 @@ /** - * Positionals whose value parses and resolves lazily. + * Positionals that Stricli parses and the command resolves. * - * A parser made with {@link lazyPositional} gives the command a - * {@link LazyPositional}: + * A parser made with {@link boundPositional} gives the command a + * {@link BoundPositional}: * - * - `target.parsed` runs the parse on first access, inside `func`. + * - `target.parsed` is the parsed value. Stricli runs the parse in its + * argument scanner, so invalid input fails like any other argument: + * `Failed to parse "" for : …` and Stricli's + * `InvalidArgument` exit code (-4, shown as 252). * - `target.resolved` starts the positional's own resolver on first access * and returns its promise, so `func` awaits it instead of picking and - * calling a resolver itself. A parse error rejects it. + * calling a resolver itself. It runs inside `func`, after the auth guard + * and rc URL check. * - `target.raw` is what the user typed (`undefined` if omitted). * - * Until `func` reads it, the value does nothing, so nothing can fail and - * nothing runs early: - * - * - Stricli's argument scanner never sees an error, so invalid input still - * throws the parser's `CliError` from the command, with its exit code, - * telemetry, and `--json` error output, instead of Stricli's generic - * "Failed to parse" (exit -4). - * - Parser side effects, such as `parseOrgProjectArg` applying the host of a - * pasted Sentry URL, run after the auth guard and rc URL check, as they did - * when `func` called the parser itself. + * The parse must have no side effects: it runs before the auth guard, and + * also when a later argument fails to parse. Work that changes state, such + * as applying the host of a pasted Sentry URL, belongs in the resolver. * * `buildCommand` also runs these parsers on positionals that reach the * command as plain strings (the SDK invoke path and tests calling the loaded * func) and on omitted optional ones (parsed from `undefined`), so `func` - * always gets a {@link LazyPositional} and every caller keeps passing what - * the user typed. - * - * Shared code that handles any positional (plural alias interception, `help` - * recovery, telemetry) reads the raw input with {@link rawPositional}. - * `buildCommand` calls {@link markDestructive} on the positionals of - * destructive commands (`destructive: true`, which `buildDeleteCommand` sets), - * so resolvers see `destructive: true` and can refuse to auto-detect. + * always gets a {@link BoundPositional}. Shared code that handles any + * positional (plural alias interception, `help` recovery, telemetry) reads + * the raw input with {@link rawPositional}. */ import { @@ -39,77 +31,69 @@ import { withCompletionTarget, } from "./completion-target.js"; -/** What the running command tells a positional's resolver. */ -export type ResolveOptions = { - /** The command is destructive (see {@link markDestructive}). */ - readonly destructive: boolean; -}; +/** Destructive-command checks of {@link boundPositional} values. */ +const destructiveChecks = new WeakMap void>(); -/** Values whose command is destructive (see {@link markDestructive}). */ -const destructiveValues = new WeakSet(); - -/** Parsers made by {@link lazyPositional}. */ -const lazyParsers = new WeakSet(); +/** Parsers made by {@link boundPositional}. */ +const boundParsers = new WeakSet(); /** - * A positional value that parses on first `parsed` read and resolves on - * first `resolved` read. Each step runs once. + * A parsed positional value bound to its resolver, which runs once, on the + * first `resolved` read. */ -export class LazyPositional { - #parsed: { value: T } | undefined; +export class BoundPositional { #resolved: Promise | undefined; - readonly #parse: (raw: string | undefined) => T; - readonly #resolve: (value: T, options: ResolveOptions) => Promise; + readonly #resolve: (value: T) => Promise; /** What the user typed; `undefined` for an omitted positional. */ readonly raw: string | undefined; + /** The parsed value. */ + readonly parsed: T; constructor( raw: string | undefined, - parse: (raw: string | undefined) => T, - resolve: (value: T, options: ResolveOptions) => Promise, + parsed: T, + resolve: (value: T) => Promise, ) { this.raw = raw; - this.#parse = parse; + this.parsed = parsed; this.#resolve = resolve; } - /** The parsed value. Throws the parser's error. */ - get parsed(): T { - this.#parsed ??= { value: this.#parse(this.raw) }; - return this.#parsed.value; - } - - /** The resolved value; rejects on a parse or resolve error. */ + /** The resolved value; rejects on a resolve error. */ get resolved(): Promise { - this.#resolved ??= (async () => - await this.#resolve(this.parsed, { - destructive: destructiveValues.has(this), - }))(); + this.#resolved ??= this.#resolve(this.parsed); return this.#resolved; } } /** - * Build a positional parser that hands the command a {@link LazyPositional}. + * Build a positional parser that hands the command a {@link BoundPositional}. * * @param kind - Shell completion kind for the positional - * @param parse - Parses the raw input (`undefined` when omitted); may throw a - * `CliError` + * @param parse - Parses the raw input (`undefined` when omitted); may throw. + * Must have no side effects * @param resolve - Resolves the parsed value; gets the command context that - * Stricli passes to parsers as `this`, and {@link ResolveOptions} + * Stricli passes to parsers as `this`, and the raw input + * @param checkDestructive - Throws when the parsed value is not allowed in a + * destructive command (see {@link checkDestructive}) */ -export function lazyPositional( +export function boundPositional( kind: CompletionTarget, parse: (raw: string | undefined) => T, - resolve: (value: T, context: C, options: ResolveOptions) => Promise, -): (this: C, input?: string) => LazyPositional { + resolve: (value: T, context: C, raw: string | undefined) => Promise, + checkDestructive?: (value: T, context: C) => void, +): (this: C, input?: string) => BoundPositional { const parser = withCompletionTarget(kind, function (this: C, input?: string) { - return new LazyPositional(input, parse, (value: T, options) => - resolve(value, this, options), + const value = new BoundPositional(input, parse(input), (parsed: T) => + resolve(parsed, this, input), ); + if (checkDestructive) { + destructiveChecks.set(value, () => checkDestructive(value.parsed, this)); + } + return value; }); - lazyParsers.add(parser); + boundParsers.add(parser); return parser; } @@ -118,30 +102,30 @@ type PositionalDef = | { kind: "tuple"; parameters: readonly { parse?: unknown }[] } | { kind: "array"; parameter: { parse?: unknown } }; -/** The {@link lazyPositional} parser at a positional index, if any. */ -function lazyParserAt( +/** The {@link boundPositional} parser at a positional index, if any. */ +function boundParserAt( def: PositionalDef, index: number, ): ((this: unknown, input?: string) => unknown) | undefined { const parse = def.kind === "array" ? def.parameter.parse : def.parameters[index]?.parse; - return typeof parse === "function" && lazyParsers.has(parse) + return typeof parse === "function" && boundParsers.has(parse) ? (parse as (this: unknown, input?: string) => unknown) : undefined; } /** - * Give every {@link lazyPositional} positional its stand-in. + * Give every {@link boundPositional} positional its value. * * Stricli already ran the parsers for typed input. This covers callers that * skip its scanner (the SDK invoke path, tests calling the loaded func), * which pass raw strings, and omitted optional positionals, which Stricli - * leaves `undefined`. Lazy parsers never throw and have no side effects, so - * this is safe to run up front. Other positionals are left as they are. + * leaves `undefined`. Other positionals are left as they are. * * @param context - Passed to each parser as `this`, as Stricli does + * @throws The parser's error for an invalid raw string */ -export function applyLazyPositionals( +export function applyBoundPositionals( args: readonly unknown[], positional: unknown, context: unknown, @@ -156,7 +140,7 @@ export function applyLazyPositionals( : args.length; return Array.from({ length: count }, (_, index) => { const arg = args[index]; - const parse = lazyParserAt(def, index); + const parse = boundParserAt(def, index); const missing = arg === undefined && def.kind === "tuple"; return parse && (typeof arg === "string" || missing) ? parse.call(context, arg) @@ -165,22 +149,24 @@ export function applyLazyPositionals( } /** - * Tell the resolvers of these positionals that the command is destructive. - * Must run before `func` reads `resolved`. Other values are ignored. + * Run the destructive-command checks of these positionals, such as a target + * refusing to auto-detect. Other values are ignored. + * + * @throws The first failing check's error */ -export function markDestructive(args: readonly unknown[]): void { +export function checkDestructive(args: readonly unknown[]): void { for (const arg of args) { - if (arg instanceof LazyPositional) { - destructiveValues.add(arg); + if (arg instanceof BoundPositional) { + destructiveChecks.get(arg)?.(); } } } /** - * What the user typed for a positional value. Never triggers a parse. + * What the user typed for a positional value. * - * @returns `value.raw` for a {@link LazyPositional}, otherwise `value` + * @returns `value.raw` for a {@link BoundPositional}, otherwise `value` */ export function rawPositional(value: unknown): unknown { - return value instanceof LazyPositional ? value.raw : value; + return value instanceof BoundPositional ? value.raw : value; } diff --git a/packages/cli/src/lib/target-positional.ts b/packages/cli/src/lib/target-positional.ts index 47ce9a6cc..1b34c4d25 100644 --- a/packages/cli/src/lib/target-positional.ts +++ b/packages/cli/src/lib/target-positional.ts @@ -2,8 +2,8 @@ * `[/][]` target positionals. * * Each parser here hands `func` an {@link OrgProjectTarget}: the - * {@link ParsedOrgProject} as `target.parsed` (parsed on first read) plus a - * `resolved` promise bound to the resolver the positional declares. A + * {@link ParsedOrgProject} as `target.parsed` plus a `resolved` promise bound + * to the resolver the positional declares. A * command reads `target.parsed.type` when it needs the parsed form and awaits * `target.resolved` for the org and project, without choosing or calling a * parser or resolver itself. An omitted target arrives as auto-detect. See @@ -12,22 +12,24 @@ * Resolution gets the command path from `commandPrefix`, which Stricli's * `forCommand` (and the SDK invoker) set on the context, so usage hints need * no hand-written command name. Destructive commands (`destructive: true`, - * which `buildDeleteCommand` sets) refuse an auto-detected target. + * which `buildDeleteCommand` sets) refuse an auto-detected target before + * `func` runs. */ -import { type ParsedOrgProject, parseOrgProjectArg } from "./arg-parsing.js"; +import { type ParsedOrgProject, parseOrgProjectInput } from "./arg-parsing.js"; import { ORG_PROJECT_TARGET } from "./completion-target.js"; import { requireExplicitTarget } from "./mutate-command.js"; -import { type LazyPositional, lazyPositional } from "./positional-parse.js"; +import { type BoundPositional, boundPositional } from "./positional-parse.js"; import { type ResolvedOrgOptionalTarget, type ResolvedProjectBoundTarget, resolveOrgOptionalTarget, resolveProjectBoundTarget, } from "./resolve-target.js"; +import { applySentryUrlContext } from "./sentry-url-parser.js"; /** A parsed `[/][]` target that resolves to `R`. */ -export type OrgProjectTarget = LazyPositional; +export type OrgProjectTarget = BoundPositional; /** What a target resolver gets from the command context. */ export type TargetContext = { @@ -61,8 +63,10 @@ function commandNameOf(context: TargetContext): string { /** * Positional parser for an `[/][]` target with its own * resolver. Completes like `parseOrgProjectTarget`. In a destructive command - * an auto-detected target fails `requireExplicitTarget` before `resolve` - * runs. + * an auto-detected target fails `requireExplicitTarget` before `func` runs. + * + * The parse has no side effects; the server of a pasted Sentry URL is + * applied when the target resolves, after the auth guard and rc URL check. * * @param resolve - Turns the parsed target into what `target.resolved` * yields @@ -73,20 +77,24 @@ export function orgProjectTarget( context: TargetResolveContext, ) => Promise, ): (this: TargetContext, input?: string) => OrgProjectTarget { - return lazyPositional( + return boundPositional( ORG_PROJECT_TARGET, - (raw) => parseOrgProjectArg(raw), - async (parsed, context: TargetContext, { destructive }) => { + (raw) => parseOrgProjectInput(raw).parsed, + async (parsed, context: TargetContext, raw) => { const commandName = commandNameOf(context); - if (destructive) { - requireExplicitTarget( - parsed, - "Project target", - `sentry ${commandName} /`, - ); + // Parsing again is cheap and pure; it recovers the URL's server + const { baseUrl } = parseOrgProjectInput(raw); + if (baseUrl) { + applySentryUrlContext(baseUrl); } return await resolve(parsed, { ...context, commandName }); }, + (parsed, context: TargetContext) => + requireExplicitTarget( + parsed, + "Project target", + `sentry ${commandNameOf(context)} /`, + ), ); } diff --git a/packages/cli/test/lib/error-reporting.test.ts b/packages/cli/test/lib/error-reporting.test.ts index a7cb7ac17..a673192e7 100644 --- a/packages/cli/test/lib/error-reporting.test.ts +++ b/packages/cli/test/lib/error-reporting.test.ts @@ -10,6 +10,7 @@ // oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking import * as Sentry from "@sentry/node-core/light"; +import { ArgumentParseError, FlagNotFoundError } from "@stricli/core"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { formatAuthHeader } from "../../src/lib/auth-header.js"; import { @@ -18,6 +19,7 @@ import { extractMessagePrefix, extractResourceKind, normalizeEndpoint, + reportArgumentError, reportCliError, } from "../../src/lib/error-reporting.js"; import { @@ -827,3 +829,53 @@ describe("reportCliError integration", () => { expect(metricSpy).not.toHaveBeenCalled(); }); }); + +describe("reportArgumentError", () => { + let messageSpy: ReturnType; + + beforeEach(() => { + messageSpy = vi.spyOn(Sentry, "captureMessage"); + }); + + afterEach(() => { + messageSpy.mockRestore(); + }); + + test("captures one info message with the parser's error class", () => { + const error = new ArgumentParseError( + "period", + "bogus", + new ValidationError("Invalid period 'bogus'", "period"), + ); + reportArgumentError(error, 'Failed to parse "bogus" for period'); + expect(messageSpy).toHaveBeenCalledWith("invalid_argument", { + level: "info", + tags: { "argument_error.class": "ArgumentParseError" }, + contexts: { + invalid_argument: { + error_class: "ArgumentParseError", + cause_class: "ValidationError", + message: 'Failed to parse "bogus" for period', + }, + }, + }); + }); + + test("reports scanner errors that wrap no parser error", () => { + reportArgumentError( + new FlagNotFoundError("nope", []), + "No flag registered for --nope", + ); + expect(messageSpy).toHaveBeenCalledWith( + "invalid_argument", + expect.objectContaining({ + contexts: { + invalid_argument: expect.objectContaining({ + error_class: "FlagNotFoundError", + cause_class: null, + }), + }, + }), + ); + }); +}); diff --git a/packages/cli/test/lib/positional-parse.test.ts b/packages/cli/test/lib/positional-parse.test.ts index 3899744f9..fe957bde3 100644 --- a/packages/cli/test/lib/positional-parse.test.ts +++ b/packages/cli/test/lib/positional-parse.test.ts @@ -1,12 +1,17 @@ /** - * Lazily parsed and resolved target positionals (src/lib/positional-parse.ts, + * Bound target positionals (src/lib/positional-parse.ts, * src/lib/target-positional.ts). * * Runs a test command through Stricli's `run()` so the positional parser, * the command wrapper, and exit-code handling are exercised together. */ -import { buildApplication, type CommandContext, run } from "@stricli/core"; +import { + buildApplication, + type CommandContext, + ExitCode, + run, +} from "@stricli/core"; import { describe, expect, test, vi } from "vitest"; import type { ParsedOrgProject } from "../../src/lib/arg-parsing.js"; import { buildCommand } from "../../src/lib/command.js"; @@ -18,10 +23,11 @@ import { ContextError, EXIT, getExitCode, + HostScopeError, ValidationError, } from "../../src/lib/errors.js"; import { - markDestructive, + checkDestructive, rawPositional, } from "../../src/lib/positional-parse.js"; import { buildRouteMap } from "../../src/lib/route-map.js"; @@ -29,6 +35,9 @@ import { type OrgProjectTarget, orgProjectTarget, } from "../../src/lib/target-positional.js"; +import { useTestConfigDir } from "../helpers.js"; + +useTestConfigDir("positional-parse-"); type TestContext = CommandContext & { cwd: string; @@ -51,7 +60,7 @@ type EchoTarget = OrgProjectTarget>>; /** Run `test cmd ...args`, recording what `target.resolved` yields. */ async function runWithTarget( args: string[], - { destructive = false } = {}, + { destructive = false, readResolved = true } = {}, ): Promise<{ resolved: unknown[]; stderr: string; @@ -81,7 +90,8 @@ async function runWithTarget( }, // oxlint-disable-next-line require-yield -- test command only records the resolved target async *func(_flags, target) { - resolved.push(await target.resolved); + // Reading only `parsed` still goes through the destructive check + resolved.push(readResolved ? await target.resolved : target.parsed); }, }); const app = buildApplication( @@ -140,13 +150,21 @@ describe("orgProjectTarget", () => { expect(stderr).toContain("Auto-detection is disabled"); }); - test("invalid input fails inside the command with the CliError exit code", async () => { + test("a destructive command refuses an auto-detected target it only reads as parsed", async () => { + const { resolved, exitCode } = await runWithTarget([], { + destructive: true, + readResolved: false, + }); + expect(resolved).toEqual([]); + expect(exitCode).toBe(EXIT.CONTEXT_MISSING); + }); + + test("invalid input fails as an argument parse error", async () => { const { resolved, stderr, exitCode } = await runWithTarget(["@latest"]); expect(resolved).toEqual([]); - expect(exitCode).toBe(EXIT.VALIDATION); + expect(exitCode).toBe(ExitCode.InvalidArgument); + expect(stderr).toContain('Failed to parse "@latest"'); expect(stderr).toContain("is an issue selector"); - // Not Stricli's argument-parse error, which would exit with -4. - expect(stderr).not.toContain("Failed to parse"); }); test("keeps completing org/project slugs", () => { @@ -154,17 +172,11 @@ describe("orgProjectTarget", () => { }); }); -describe("lazy parsing and resolution", () => { +describe("parsing and resolution", () => { const context = { cwd: "/work", commandPrefix: ["sentry", "thing", "list"] }; - test("parses on first read of parsed", () => { - const target = echoTarget.call(context, "@latest"); - expect(() => target.parsed).toThrow(ValidationError); - }); - - test("rejects resolved with the parse error", async () => { - const target = echoTarget.call(context, "@latest"); - await expect(target.resolved).rejects.toThrow(ValidationError); + test("parses when the parser runs", () => { + expect(() => echoTarget.call(context, "@latest")).toThrow(ValidationError); }); test("keeps the raw input and the parsed value", () => { @@ -188,22 +200,27 @@ describe("lazy parsing and resolution", () => { expect(echoResolver).toHaveBeenCalledTimes(1); }); - test("a destructive command refuses an auto-detected target", async () => { - echoResolver.mockClear(); + test("the destructive check refuses only an auto-detected target", () => { const omitted = echoTarget.call(context, undefined); - markDestructive([omitted]); - const error = await omitted.resolved.catch((e: unknown) => e); - expect(error).toBeInstanceOf(ContextError); - expect((error as Error).message).toContain( + expect(() => checkDestructive([omitted])).toThrow(ContextError); + expect(() => checkDestructive([omitted])).toThrow( "sentry thing list /", ); - expect(echoResolver).not.toHaveBeenCalled(); const explicit = echoTarget.call(context, "acme/web"); - markDestructive([explicit]); - await expect(explicit.resolved).resolves.toMatchObject({ - commandName: "thing list", - }); + expect(() => checkDestructive([explicit, "plain"])).not.toThrow(); + }); + + test("applies a pasted URL's server when resolving, not when parsing", async () => { + echoResolver.mockClear(); + // No account for this server: applying it throws HostScopeError + const target = echoTarget.call( + context, + "https://sentry.example.com/organizations/acme/projects/web/", + ); + expect(target.parsed).toMatchObject({ org: "acme" }); + await expect(target.resolved).rejects.toThrow(HostScopeError); + expect(echoResolver).not.toHaveBeenCalled(); }); test("needs the command path on the context", async () => { @@ -211,19 +228,20 @@ describe("lazy parsing and resolution", () => { await expect(target.resolved).rejects.toThrow("commandPrefix"); }); - test("is not a thenable, so awaiting it does not parse", async () => { - // Stricli awaits parser results; a parse here would fail in its scanner. - const target = echoTarget.call(context, "@latest"); + test("is not a thenable, so awaiting it does not resolve", async () => { + // Stricli awaits parser results; resolving there would skip the auth guard. + echoResolver.mockClear(); + const target = echoTarget.call(context, "acme/web"); await expect(Promise.resolve(target)).resolves.toBe(target); + expect(echoResolver).not.toHaveBeenCalled(); }); }); describe("rawPositional", () => { const context = { cwd: "/work" }; - test("returns what the user typed without parsing", () => { + test("returns what the user typed", () => { expect(rawPositional(echoTarget.call(context, "acme/"))).toBe("acme/"); - expect(rawPositional(echoTarget.call(context, "@latest"))).toBe("@latest"); expect(rawPositional(echoTarget.call(context, undefined))).toBeUndefined(); expect(rawPositional("plain")).toBe("plain"); expect(rawPositional(undefined)).toBeUndefined(); diff --git a/packages/cli/test/lib/scanner-flags.integration.test.ts b/packages/cli/test/lib/scanner-flags.integration.test.ts index 8571d7fb0..885ad9c64 100644 --- a/packages/cli/test/lib/scanner-flags.integration.test.ts +++ b/packages/cli/test/lib/scanner-flags.integration.test.ts @@ -20,8 +20,10 @@ import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { run } from "@stricli/core"; -import { describe, expect, test } from "vitest"; +// oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking +import * as Sentry from "@sentry/node-core/light"; +import { ExitCode, run } from "@stricli/core"; +import { describe, expect, test, vi } from "vitest"; import { app } from "../../src/app.js"; import type { SentryContext } from "../../src/context.js"; import { CLI_VERSION } from "../../src/lib/constants.js"; @@ -317,3 +319,48 @@ describe("--help --json renders structured help via renderHelp hook", () => { expect(parsed.error).toContain("nope"); }); }); + +describe("arguments that fail to parse", () => { + // Stricli parses flags and positionals before the command runs; any parse + // failure, including a CliError from our parsers, exits InvalidArgument. + + test("an invalid flag value", async () => { + const { stderr, exitCode } = await runApp([ + "issue", + "list", + "--period", + "bogus", + ]); + expect(exitCode).toBe(ExitCode.InvalidArgument); + expect(stderr).toContain('Failed to parse "bogus" for period'); + }); + + test("is reported to Sentry for UX research", async () => { + const messageSpy = vi.spyOn(Sentry, "captureMessage"); + try { + await runApp(["issue", "list", "--period", "bogus"]); + expect(messageSpy).toHaveBeenCalledWith( + "invalid_argument", + expect.objectContaining({ + contexts: { + invalid_argument: expect.objectContaining({ + cause_class: "ValidationError", + message: expect.stringContaining( + 'Failed to parse "bogus" for period', + ), + }), + }, + }), + ); + } finally { + messageSpy.mockRestore(); + } + }); + + test("an invalid org/project target", async () => { + const { stderr, exitCode } = await runApp(["trace", "list", "@latest"]); + expect(exitCode).toBe(ExitCode.InvalidArgument); + expect(stderr).toContain('Failed to parse "@latest"'); + expect(stderr).toContain("is an issue selector"); + }); +});