diff --git a/.oxlintrc.json b/.oxlintrc.json index 695f7bab6..15b8df462 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -68,6 +68,7 @@ "sentry-cli/no-args-join-in-release": "error", "sentry-cli/no-direct-target-resolution": "error", "sentry-cli/require-target-resolution": "error", + "sentry-cli/no-ad-hoc-id-arguments": "error", "sentry-cli/no-generic-is-record": "error", "sentry-cli/prefer-paginate-helper": "error", "sentry-cli/no-manual-pagination": "error", diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index 1f8f79fb4..64b4d505c 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -227,18 +227,42 @@ export const myRoute = buildRouteMap({ ### Positional Arguments -Use `parseSlashSeparatedArg` from `src/lib/arg-parsing.ts` for the standard `[//]` pattern. Required identifiers (trace IDs, span IDs) should be **positional args**, not flags. +Required identifiers (trace IDs, span IDs) should be **positional args**, not +flags. Every command that takes trace, span, replay, or log IDs parses them +with `parseIdArguments` (or `parseIdArgument` for exactly one) from +`src/lib/id-arguments.ts`, so they all take the same forms: + +- ``, `/`, `//`, ` `, a Sentry URL +- ` /`, reversed, with a warning +- more IDs after any of those (with `multiple`), each maybe repeating the + target; one argument may hold several lines, or IDs after a space +- extra arguments are ignored with a warning; slashes pasted after an ID are + ignored + +The command passes its kind of ID from `ID_KINDS` (what the ID looks like, +how to validate it, which part of a URL holds one) and how it takes IDs: +`optional` when it can list a project's items instead (`log list`, +`span list`), `needsProject` when `/` should name project `` +rather than org ``, `validate: false` to recover malformed IDs once the +target is known. The result is a `target` string for `parseOrgProjectInput` +and the IDs. Add a kind to `ID_KINDS` rather than validating IDs in a command; +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 (and `event view`, which takes them) +keep their own syntax. ```typescript -import { parseSlashSeparatedArg, parseOrgProjectArg } from "../../lib/arg-parsing.js"; +import { ID_KINDS, parseIdArgument } from "../../lib/id-arguments.js"; -// "my-org/my-project/abc123" → { id: "abc123", targetArg: "my-org/my-project" } -const { id, targetArg } = parseSlashSeparatedArg(first, "Trace ID", USAGE_HINT); -const parsed = parseOrgProjectArg(targetArg); -// parsed.type: "auto-detect" | "explicit" | "project-search" | "org-all" +// ["my-org/my-project/abc123..."] → { target: "my-org/my-project", id: "abc123...", warnings: [] } +const { target, id, baseUrl, warnings } = parseIdArgument(args, ID_KINDS.replay, { + usageHint: USAGE_HINT, +}); +const parsed = parseOrgProjectInput(target); ``` -Reference: `span/list.ts`, `trace/view.ts`, `event/view.ts` +Reference: `replay/view.ts`, `span/view.ts`, `log/view.ts`, and +`trace-target.ts` for trace targets. **Target positionals:** A single `[/][]` positional uses a parser from `src/lib/target-positional.ts`, which binds the resolver: @@ -267,9 +291,34 @@ 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`. +call `command.loader()` keep passing raw strings. + +**Arity-dependent positionals:** when what an argument means depends on how +many were given (`[/] `, `log list`'s optional +trailing trace ID), declare the positional with `boundArguments` from +`src/lib/positional-parse.ts` (see `replayArguments` in `replay/view.ts`, +`log list`, and `trace view`). It is an array positional whose `parse` sees every argument at +once and whose `resolve` binds like a target positional's: `func` takes one +`BoundArguments` (alias it per command and add the alias to +the lint rule's type list), reads `target.parsed`, and awaits +`target.resolved`. A Stricli parser sees one argument at a time, so +`buildCommand` runs the parse before `func` (and before the auth guard). A +parse error becomes Stricli's `ArgumentParseError`, which prints and exits +252 like a scanner error; next to a `help` argument it turns into the +command's help instead. The same purity rule applies: return a pasted URL's server +(`baseUrl`) and any warnings from the parse, and apply them in `resolve` +(`parseTraceTargetInput` and `parseDualModeInput` in `trace-target.ts` do +this for trace targets). Commands that recover a malformed ID (`trace view`, +`span view`, `log view`) parse it unvalidated and validate or recover it in +`resolve`, which can look the prefix up: `parseTraceArgumentsInput` and +`resolveTraceArguments` do this for trace IDs. A missing ID or a bad target +still fails the parse. + +Which argument is the ID and which is its target is decided by +`grammar/id-arguments.pegjs`, not by counting arguments in TypeScript. The +grammar knows no kind of ID: the command's `ID_KINDS` entry tells it what an +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` diff --git a/packages/cli/grammar/id-arguments.d.ts b/packages/cli/grammar/id-arguments.d.ts new file mode 100644 index 000000000..e891f0631 --- /dev/null +++ b/packages/cli/grammar/id-arguments.d.ts @@ -0,0 +1,60 @@ +// Types of the parser generated from id-arguments.pegjs. The generator +// copies this file to src/generated/id-arguments-grammar.d.ts and rewrites +// relative imports to resolve from there. + +/** An ID a Sentry URL carries, with the URL's org and server. */ +export type IdUrl = { + readonly id: string; + readonly org?: string; + readonly baseUrl?: string; +}; + +/** + * What the command parses: the grammar itself does not know any kind of ID. + * See `parseIdArguments()`. + */ +export type IdArgumentsGrammarOptions = { + /** Whether the arguments may name no ID (then a lone token is a target). */ + optional: boolean; + /** Whether `value` is an ID of the kind the command takes. */ + isId: (value: string) => boolean; + /** Whether `value`, not an ID, still looks like a malformed one. */ + mayBeId: (value: string) => boolean; + /** The ID in a Sentry URL, or `null` for anything else. */ + idUrl: (value: string) => IdUrl | null; +}; + +/** A token that names an ID, maybe after a target: `[/]`. */ +export type IdToken = { + /** The token as typed */ + readonly text: string; + /** What came before the ID's slash, if anything */ + readonly target?: string; + /** The ID, not validated */ + readonly id: string; +}; + +/** Which tokens are the target and which are IDs. */ +export type IdArgumentsSyntax = { + /** The target, as typed: a separate argument, a path prefix, or `/` */ + readonly target?: string; + /** Whether `target` came before the first ID in one path */ + readonly path?: boolean; + readonly ids: readonly IdToken[]; + /** Server of a pasted Sentry URL */ + readonly baseUrl?: string; + /** Whether the ID came before the target */ + readonly reversed?: boolean; +}; + +export declare class SyntaxError extends Error { + expected: unknown; + found: string | null; + location: unknown; +} + +/** Parse NUL-prefixed tokens into a target and IDs. */ +export declare function parse( + input: string, + options: IdArgumentsGrammarOptions, +): IdArgumentsSyntax; diff --git a/packages/cli/grammar/id-arguments.pegjs b/packages/cli/grammar/id-arguments.pegjs new file mode 100644 index 000000000..8866e6c86 --- /dev/null +++ b/packages/cli/grammar/id-arguments.pegjs @@ -0,0 +1,117 @@ +// Grammar for positionals that name IDs and the org/project to look them up +// in. It knows nothing about what kind of ID it parses (trace, span, replay, +// log...): the command says which, through the `isId` and `idUrl` hooks in +// `options`, so every ID-taking command shares one syntax: +// +// the org/project is auto-detected +// / like a URL: the org, then the ID (the +// caller may read `` as a project) +// // +// any org/project target, as everywhere else +// / reversed; the caller warns +// a Sentry URL with the ID in it +// +// More IDs may follow any of these, each repeating the target or not +// (`/`). The ID is always the last path segment, and slashes +// pasted after it are ignored. +// +// `pnpm run generate:grammar` compiles it, with the types in +// id-arguments.d.ts, into the gitignored src/generated/; the build, +// typecheck, and test scripts run it. +// +// `parseIdArguments()` (src/lib/id-arguments.ts) splits the arguments into +// tokens first and puts a NUL, which no argument contains, before each one. +// No token is empty. The grammar only decides which token is the target and +// which are IDs; it accepts any input and never throws. The caller counts +// the IDs, validates them, and parses the target like any org/project target. + +{ + const { optional, isId, mayBeId, idUrl } = options; + + // `[/[/]]` before an ID; the caller makes it a target + function pathTarget(prefix) { + if (prefix === "") { + return {}; + } + return { target: prefix, path: true }; + } +} + +// Peggy runs a rule's action as soon as that rule matches, even when the +// enclosing alternative fails later and the parser backtracks. Every action +// here only builds values, and the hooks are pure, so that is harmless. + +arguments + = end { return { ids: [] }; } + // A Sentry URL with the ID in it + / sep u:urlWithId x:more { + return { + ...(u.org && { target: `${u.org}/` }), + ids: [{ text: u.id, id: u.id }, ...x], + ...(u.baseUrl && { baseUrl: u.baseUrl }), + }; + } + // Any other URL is a target + / sep t:url x:more { return { target: t, ids: x }; } + // ` /`: the arguments are reversed. When IDs are + // optional, only a real ID can come first. + / sep i:segment &{ return !optional || isId(i); } sep !idPath + t:$([^/\0]* "/" [^\0]*) end + { return { target: t, ids: [{ text: i, id: i }], reversed: true }; } + // `[/[/]] [...]` + / sep p:idPath x:more { + return { + ...pathTarget(p.target ?? ""), + ids: [{ text: p.text, id: p.id }, ...x], + }; + } + // One token that does not end in an ID. Unless IDs are optional, it still + // names one, so it can be reported or recovered: the last segment after + // two slashes (`//`), after one when it looks like a + // malformed ID (`/`), or the whole token. Otherwise, + // `/` is a target without an ID. + / sep t:segment "/" i:segment end &{ return !optional && mayBeId(i); } + { return { ...pathTarget(t), ids: [{ text: text().slice(1), id: i }] }; } + / sep !"/" t:$(segment "/" segment "/" [^\0]*) end &{ return !optional; } { + const slash = t.lastIndexOf("/"); + return { + ...pathTarget(t.slice(0, slash)), + ids: [{ text: t, id: t.slice(slash + 1) }], + }; + } + / sep i:segment end &{ return !optional; } { return { ids: [{ text: i, id: i }] }; } + // ` [...]` + / sep t:token x:more { return { target: t, ids: x }; } + +// The IDs after the first token. Each may repeat the target: +// `/`. +more + = (sep @(idPath / t:token { return { text: t, id: t }; }))* + +// `[/]` in one token: the last segment is an ID, maybe with +// slashes pasted after it. +idPath + = t:$(!("/" idSegment argEnd) [^\0])* "/" i:idSegment argEnd + { return { text: text(), target: t, id: i }; } + / i:idSegment argEnd { return { text: text(), id: i }; } + +// An ID, and the slashes after it +idSegment + = i:segment "/"* &{ return isId(i); } { return i; } + +// A Sentry URL carrying the ID +urlWithId + = u:token &{ return idUrl(u) !== null; } { return idUrl(u); } + +url + = $("http" "s"? "://" [^\0]*) + +token = $[^\0]+ + +segment = $[^/\0]+ + +sep = "\0" + +argEnd = &sep / end + +end = !. diff --git a/packages/cli/lint-rules/cli-oxlint-plugin.js b/packages/cli/lint-rules/cli-oxlint-plugin.js index 409dd1f97..d3cfcfecd 100644 --- a/packages/cli/lint-rules/cli-oxlint-plugin.js +++ b/packages/cli/lint-rules/cli-oxlint-plugin.js @@ -56,11 +56,20 @@ function readsProperty(node, object, property) { ); } -/** Target positional types from `src/lib/target-positional.ts`. */ +/** + * Target positional types from `src/lib/target-positional.ts`, and + * `boundArguments` positionals (`BoundArguments` and the commands' aliases). + */ const BOUND_TARGET_TYPES = new Set([ "OrgOptionalTarget", "OrgProjectTarget", "ProjectBoundTarget", + "BoundArguments", + "LogListArguments", + "LogViewArguments", + "ReplayArguments", + "SpanViewArguments", + "TraceViewArguments", ]); function boundTargetParams(fn) { @@ -141,6 +150,35 @@ function regexSource(node) { return ""; } +/** + * Helpers that parse ID positionals by hand. Commands parse IDs with + * `parseIdArguments()` (src/lib/id-arguments.ts), so every command takes + * the same forms. + */ +const AD_HOC_ID_PARSING = new Set([ + "parseSlashSeparatedArg", + "detectSwappedViewArgs", + "HEX_ID_RE", + "SPAN_ID_RE", +]); + +/** + * Commands still allowed to: `event view` also takes issue IDs, which are + * not hex IDs and have their own syntax. + */ +const AD_HOC_ID_PARSING_ALLOWED = ["/src/commands/event/view.ts"]; + +const ID_ARGUMENT_PARSERS = new Set(["parseIdArguments", "parseIdArgument"]); + +/** Whether `node` is `ID_KINDS.`. */ +function isIdKind(node) { + return ( + node?.type === "MemberExpression" && + node.object.type === "Identifier" && + node.object.name === "ID_KINDS" + ); +} + const rules = { "no-silent-catch": rule( "Handle or report caught errors instead of discarding them.", @@ -309,6 +347,33 @@ const rules = { }; }, ), + "no-ad-hoc-id-arguments": rule( + "Parse ID positionals with parseIdArguments() and an ID_KINDS kind.", + (_context, file, report) => { + if (!file.includes("/src/") || file.endsWith("/lib/id-arguments.ts")) + return {}; + const commands = + file.includes("/src/commands/") && + !AD_HOC_ID_PARSING_ALLOWED.some((allowed) => file.endsWith(allowed)); + return { + Identifier(node) { + if ( + commands && + AD_HOC_ID_PARSING.has(node.name) && + node.parent?.type !== "ImportSpecifier" + ) + report(node); + }, + CallExpression(node) { + if ( + ID_ARGUMENT_PARSERS.has(node.callee.name) && + !isIdKind(node.arguments[1]) + ) + report(node); + }, + }; + }, + ), "no-generic-is-record": rule( "Use a shape-specific guard instead of isRecord().", (_context, file, report) => diff --git a/packages/cli/src/app.ts b/packages/cli/src/app.ts index 7b87688ba..5359ae2de 100644 --- a/packages/cli/src/app.ts +++ b/packages/cli/src/app.ts @@ -1,5 +1,6 @@ import { type ApplicationText, + ArgumentScannerError, buildApplication, text_en, UnexpectedPositionalError, @@ -400,6 +401,12 @@ const customText: ApplicationText = { return base; }, exceptionWhileRunningCommand: (exc: unknown, ansiColor: boolean): string => { + // `boundArguments` positionals parse in the command wrapper, after + // Stricli's scanner; their errors still read as argument errors + if (exc instanceof ArgumentScannerError) { + return customText.exceptionWhileParsingArguments(exc, ansiColor); + } + // These errors are handled outside Stricli: OutputError has already been // rendered, while auth errors may trigger login and a single retry. if (escapesToOuterMiddleware(exc)) { diff --git a/packages/cli/src/commands/log/list.ts b/packages/cli/src/commands/log/list.ts index 0c5f6a5d6..5a23fe479 100644 --- a/packages/cli/src/commands/log/list.ts +++ b/packages/cli/src/commands/log/list.ts @@ -16,10 +16,12 @@ import { } from "../../lib/api-client.js"; import { buildProjectQuery, + type ParsedOrgProject, parseLogSort, + parseOrgProjectInput, validateLimit, } from "../../lib/arg-parsing.js"; -import { parseOrgProjectTarget } from "../../lib/completion-target.js"; +import { ORG_PROJECT_TARGET } from "../../lib/completion-target.js"; import { AuthError, stringifyUnknown, @@ -50,9 +52,13 @@ import { } from "../../lib/list-command.js"; import { logger } from "../../lib/logger.js"; import { withProgress } from "../../lib/polling.js"; +import { + type BoundArguments, + boundArguments, +} from "../../lib/positional-parse.js"; import { resolveLogProjectId, - resolveProjectBoundFromArg, + resolveProjectBoundTarget, } from "../../lib/resolve-target.js"; import { sanitizeQuery } from "../../lib/search-query.js"; import { @@ -63,9 +69,12 @@ import { type TimeRange, timeRangeToApiParams, } from "../../lib/time-range.js"; +import { applySentryUrlContext } from "../../lib/sentry-url-parser.js"; import { - parseDualModeArgs, + applyTraceTargetInput, + parseDualModeInput, resolveTraceOrg, + type TraceTargetInput, warnIfNormalized, } from "../../lib/trace-target.js"; import { getUpdateNotification } from "../../lib/version-check.js"; @@ -155,16 +164,71 @@ type FetchResult = { // Positional argument disambiguation // --------------------------------------------------------------------------- +/** What `log list` lists, as parsed: a project's logs or a trace's. */ +type LogListInput = + | { + readonly mode: "project"; + readonly target: ParsedOrgProject; + /** Server of a pasted Sentry URL, for `applySentryUrlContext` */ + readonly baseUrl?: string; + } + | { readonly mode: "trace"; readonly trace: TraceTargetInput }; + +/** What `log list` lists, resolved. */ +type LogListTarget = + | { readonly mode: "project"; readonly org: string; readonly project: string } + | { + readonly mode: "trace"; + readonly org: string; + readonly traceId: string; + /** Project the user named, to filter the trace's logs by */ + readonly projectFilter?: string; + }; + +/** The arguments of `log list`. */ +type LogListArguments = BoundArguments; + /** - * Disambiguate log list positional arguments. - * - * Thin wrapper around {@link parseDualModeArgs} that binds the - * trace-mode usage hint for log list. + * Disambiguate log list positional arguments with {@link parseDualModeInput} + * (trace mode when the arguments end in a trace ID). */ -function parseLogListArgs( - args: string[], -): ReturnType { - return parseDualModeArgs(args, TRACE_USAGE_HINT); +function parseLogListInput(args: readonly string[]): LogListInput { + const input = parseDualModeInput(args, TRACE_USAGE_HINT); + if (input.mode === "trace") { + return input; + } + const { parsed, baseUrl } = parseOrgProjectInput(input.target); + return { mode: "project", target: parsed, baseUrl }; +} + +async function resolveLogListTarget( + input: LogListInput, + context: SentryContext, +): Promise { + if (input.mode === "trace") { + const parsed = applyTraceTargetInput(input.trace); + warnIfNormalized(parsed, "log.list"); + const { traceId, org } = await resolveTraceOrg( + parsed, + context.cwd, + TRACE_USAGE_HINT, + ); + return { + mode: "trace", + org, + traceId, + projectFilter: parsed.type === "explicit" ? parsed.project : undefined, + }; + } + if (input.baseUrl) { + applySentryUrlContext(input.baseUrl); + } + const { org, project } = await resolveProjectBoundTarget( + input.target, + context.cwd, + COMMAND_NAME, + ); + return { mode: "project", org, project }; } /** @@ -758,15 +822,13 @@ export const listCommand = buildListCommand( schema: SentryLogSchema, }, parameters: { - positional: { - kind: "array", - parameter: { - placeholder: "org/project-or-trace-id", - brief: - "[/[/]], /, or ", - parse: parseOrgProjectTarget, - }, - }, + positional: boundArguments({ + placeholder: "org/project-or-trace-id", + brief: "[/[/]], /, or ", + kind: ORG_PROJECT_TARGET, + parse: parseLogListInput, + resolve: resolveLogListTarget, + }), flags: { limit: { kind: "parsed", @@ -809,17 +871,17 @@ export const listCommand = buildListCommand( s: "sort", }, }, - async *func(this: SentryContext, flags: ListFlags, ...args: string[]) { + async *func( + this: SentryContext, + flags: ListFlags, + target: LogListArguments, + ) { validateFollowFlags(flags); - const { cwd } = this; - - const parsed = parseLogListArgs(args); - // Resolve mode-dependent default period const timeRange = flags.period ?? - (parsed.mode === "trace" ? TIME_RANGE_14D : TIME_RANGE_30D); + (target.parsed.mode === "trace" ? TIME_RANGE_14D : TIME_RANGE_30D); // Follow mode streams live events via short polling intervals — // absolute date ranges are silently ignored, so reject them entirely. @@ -831,14 +893,10 @@ export const listCommand = buildListCommand( ); } - if (parsed.mode === "trace") { + const resolved = await target.resolved; + if (resolved.mode === "trace") { // Trace mode: use the org-scoped trace-logs endpoint. - warnIfNormalized(parsed.parsed, "log.list"); - const { traceId, org } = await resolveTraceOrg( - parsed.parsed, - cwd, - TRACE_USAGE_HINT, - ); + const { traceId, org, projectFilter } = resolved; // Warn if --fields was passed — the trace-logs endpoint has a fixed // field set and doesn't support arbitrary extra fields. @@ -848,10 +906,6 @@ export const listCommand = buildListCommand( ); } - // Capture explicit project for API-level filtering - const projectFilter = - parsed.parsed.type === "explicit" ? parsed.parsed.project : undefined; - // Prepend project filter to the query when user explicitly specified a project const traceQuery = buildProjectQuery(flags.query, projectFilter); @@ -927,11 +981,7 @@ export const listCommand = buildListCommand( // Standard project-scoped mode { - const { org, project } = await resolveProjectBoundFromArg( - parsed.target, - cwd, - COMMAND_NAME, - ); + const { org, project } = resolved; // Resolve the slug to a numeric project ID so the Events query scopes // via the `project` param. The `project:` filter only matches // actively-selected projects and can otherwise return no logs (#1317). diff --git a/packages/cli/src/commands/log/view.ts b/packages/cli/src/commands/log/view.ts index 38cab4a19..7115d767f 100644 --- a/packages/cli/src/commands/log/view.ts +++ b/packages/cli/src/commands/log/view.ts @@ -11,15 +11,14 @@ import pLimit from "p-limit"; import type { SentryContext } from "../../context.js"; import { getLogItemDetail, getLogs } from "../../lib/api-client.js"; import { - detectSwappedViewArgs, looksLikeIssueShortId, - parseOrgProjectArg, - parseSlashSeparatedArg, - splitNewlineArg, + type OrgProjectInput, + type ParsedOrgProject, + parseOrgProjectInput, } from "../../lib/arg-parsing.js"; import { openInBrowser } from "../../lib/browser.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgProjectTarget } from "../../lib/completion-target.js"; +import { ORG_PROJECT_TARGET } from "../../lib/completion-target.js"; import { ContextError, ResolutionError, @@ -28,28 +27,29 @@ import { import { formatLogDetails } from "../../lib/formatters/index.js"; import { filterFields } from "../../lib/formatters/json.js"; import { CommandOutput } from "../../lib/formatters/output.js"; -import { - ageInDaysFromUuidV7, - HEX_ID_RE, - normalizeHexId, - validateHexId, -} from "../../lib/hex-id.js"; +import { ageInDaysFromUuidV7, validateHexId } from "../../lib/hex-id.js"; import { handleRecoveryResult, recoverHexId, } from "../../lib/hex-id-recovery.js"; +import { ID_KINDS, parseIdArguments } from "../../lib/id-arguments.js"; import { applyFreshFlag, FRESH_ALIASES, FRESH_FLAG, } from "../../lib/list-command.js"; import { logger } from "../../lib/logger.js"; +import { + type BoundArguments, + boundArguments, +} from "../../lib/positional-parse.js"; import { resolveLogProjectId, resolveOrgAndProject, resolveProjectBoundSlug, } from "../../lib/resolve-target.js"; import { RETENTION_DAYS } from "../../lib/retention.js"; +import { applySentryUrlContext } from "../../lib/sentry-url-parser.js"; import { buildLogsUrl } from "../../lib/sentry-web-urls.js"; import { setOrgProjectContext } from "../../lib/telemetry.js"; import { isAllDigits } from "../../lib/utils.js"; @@ -70,211 +70,49 @@ type ViewFlags = { /** Usage hint for ContextError messages */ const USAGE_HINT = "sentry log view / [...]"; -/** - * Resolve a single-slash positional arg (`before/after`) as `org/log-id`, or - * return `null` when `after` is not a valid 32-char hex log ID and should fall - * through to `parseSlashSeparatedArg`. - * - * This guard must run before `parseSlashSeparatedArg` because that function - * throws `ContextError` for any single-slash arg, treating it as `org/project` - * with a missing log ID (CLI-1AK). - */ -function parseSingleSlashLogArg( - beforeSlash: string, - afterSlash: string, -): { rawLogIds: string[]; targetArg: string | undefined } | null { - // "org/LOG-ID" or "project/LOG-ID" → treat beforeSlash as target, afterSlash as log ID. - if (afterSlash && HEX_ID_RE.test(normalizeHexId(afterSlash))) { - return { - rawLogIds: [normalizeHexId(afterSlash)], - targetArg: beforeSlash || undefined, - }; - } - return null; -} - -/** - * Split a fully-qualified `//` arg into its target and - * log ID, or return `null` when the arg isn't in that shape (fewer than two - * slashes, or the trailing segment isn't a 32-char hex ID). - * - * Agents often pass several fully-qualified paths to one `log view` call - * (CLI-1XC). Without this, the first path was used verbatim as the target - * (yielding project `"/"`) and the rest failed hex validation. - */ -function splitFullLogPath( - arg: string, -): { target: string; logId: string } | null { - const trimmed = arg.trim(); - const firstSlash = trimmed.indexOf("/"); - const lastSlash = trimmed.lastIndexOf("/"); - if (firstSlash === -1 || firstSlash === lastSlash) { - return null; - } - const logId = normalizeHexId(trimmed.slice(lastSlash + 1)); - if (!HEX_ID_RE.test(logId)) { - return null; - } - return { target: trimmed.slice(0, lastSlash), logId }; -} - -/** - * Parse multiple args where the first is a fully-qualified log path. - * Every later arg may be a fully-qualified path or a bare log ID; all paths - * must share the same org/project target. - * - * @throws {ValidationError} If the paths reference different targets - */ -function parseMultipleFullPaths( - args: string[], - target: string, -): { rawLogIds: string[]; targetArg: string } { - const rawLogIds: string[] = []; - for (const arg of args) { - const full = splitFullLogPath(arg); - if (full) { - if (full.target !== target) { - throw new ValidationError( - `Log IDs reference different projects ('${target}' and '${full.target}'). ` + - `View logs from one project per call: ${USAGE_HINT}`, - ); - } - rawLogIds.push(full.logId); - } else { - rawLogIds.push(...splitNewlineArg(arg)); - } - } - return { rawLogIds, targetArg: target }; -} - -/** - * Strip a redundant `/` prefix from a log ID arg, so - * `sentry log view org/proj org/proj/` resolves `` (CLI-1XC). - */ -function stripTargetPrefix(rawId: string, target: string): string { - const prefix = `${target.trim()}/`; - return rawId.startsWith(prefix) ? rawId.slice(prefix.length) : rawId; -} - /** * Parse positional arguments for log view. - * Handles: - * - `` — single log ID (auto-detect org/project) - * - ` [...]` — explicit target + one or more log IDs - * - `//` — single slash-separated arg - * - * When two or more args are provided, the first is always treated as the - * target (org/project specifier) and the rest as log IDs. * - * Arguments containing newlines are split into multiple IDs. + * Log IDs take the forms every ID-taking command does (see + * `id-arguments.ts`): ``, `/`, + * `//`, ` `, reversed, and more log + * IDs after any of those, each maybe repeating the target. An argument may + * hold several lines, one log ID each. * * Returns **raw** log IDs without running {@link validateHexId} — validation * is deferred to the main command so {@link recoverHexId} can use the * resolved org/project context for fuzzy prefix lookups. * * @param args - Positional arguments from CLI - * @returns Parsed raw log IDs and optional target arg - * @throws {ContextError} If no arguments provided + * @returns Parsed raw log IDs, optional target arg, and warnings + * @throws {ContextError} If no log ID is given + * @throws {ValidationError} If the log IDs name different projects */ -// positional arg parsing has many format branches by design -export function parsePositionalArgs(args: string[]): { +export function parsePositionalArgs(args: readonly string[]): { rawLogIds: string[]; targetArg: string | undefined; + /** Warnings to log, such as for reversed arguments */ + warnings: readonly string[]; /** Suggestion when first arg looks like an issue short ID */ suggestion?: string; } { - if (args.length === 0) { - throw new ContextError("Log ID", USAGE_HINT, []); - } - - const first = args[0]; - if (first === undefined) { - throw new ContextError("Log ID", USAGE_HINT, []); - } - - if (args.length === 1) { - // Single arg — could be slash-separated org/project/logId, org/logId, or a plain ID - // (possibly containing newlines). - // Guard: detect exactly-one-slash args before parseSlashSeparatedArg, which - // throws ContextError for "org/logId" thinking it's "org/project" with no ID. - const trimmedFirst = first.trim(); - const slashIdx = trimmedFirst.indexOf("/"); - if (slashIdx !== -1 && trimmedFirst.indexOf("/", slashIdx + 1) === -1) { - const singleSlash = parseSingleSlashLogArg( - trimmedFirst.slice(0, slashIdx), - trimmedFirst.slice(slashIdx + 1), - ); - if (singleSlash) { - if (singleSlash.rawLogIds.length === 0) { - throw new ContextError("Log ID", USAGE_HINT, []); - } - return singleSlash; - } - } - - const { id, targetArg } = parseSlashSeparatedArg( - first, - "Log ID", - USAGE_HINT, - ); - const rawLogIds = splitNewlineArg(id); - if (rawLogIds.length === 0) { - throw new ContextError("Log ID", USAGE_HINT, []); - } - return { rawLogIds, targetArg }; - } - - // Two or more args — first is target, rest are log IDs. - // Each arg may contain newlines (split them). - - // Check issue short ID first — it takes precedence over swap detection - // because `detectSwappedViewArgs` also fires for `CAM-82X my-org/project` - // (first has no "/", second has "/"), but the user's intent is clearly - // to view an issue, not to swap log-view arguments. - if (looksLikeIssueShortId(first)) { - const rawLogIds = args.slice(1).flatMap(splitNewlineArg); - if (rawLogIds.length === 0) { - throw new ContextError("Log ID", USAGE_HINT, []); - } - return { - rawLogIds, - targetArg: first, - suggestion: `Did you mean: sentry issue view ${first}`, - }; - } - - // oxlint-disable-next-line typescript/no-non-null-assertion -- length >= 2 guarantees index 1 exists - const second = args[1]!; - - // Detect swapped args: exactly two args where first has no "/" and second - // does (e.g., `sentry log view ace106b2 myorg/myproject`). Only fires for - // the two-arg case to avoid silently dropping extra args in multi-arg use. - if (args.length === 2) { - const swapWarning = detectSwappedViewArgs(first, second); - if (swapWarning) { - return { - rawLogIds: splitNewlineArg(first), - targetArg: second, - suggestion: swapWarning, - }; - } - } - - // Every arg is (or starts with) a fully-qualified `org/project/log-id`. - const firstFull = splitFullLogPath(first); - if (firstFull) { - return parseMultipleFullPaths(args, firstFull.target); - } - - const rawLogIds = args - .slice(1) - .flatMap(splitNewlineArg) - .map((id) => stripTargetPrefix(id, first)); - if (rawLogIds.length === 0) { - throw new ContextError("Log ID", USAGE_HINT, []); - } - - return { rawLogIds, targetArg: first }; + const { target, ids, warnings } = parseIdArguments(args, ID_KINDS.log, { + usageHint: USAGE_HINT, + multiple: true, + needsProject: true, + validate: false, + }); + const [first] = args; + return { + rawLogIds: [...ids], + targetArg: target, + warnings, + ...(args.length > 1 && + first && + looksLikeIssueShortId(first) && { + suggestion: `Did you mean: sentry issue view ${first}`, + }), + }; } /** @@ -319,14 +157,14 @@ export type ResolvedLogTarget = { /** * Resolve the target org/project from the parsed arg. * - * @param parsed - Result of `parseOrgProjectArg` + * @param parsed - The parsed target argument * @param rawLogIds - Raw log IDs (used for usage hints; not yet validated) * @param cwd - Current working directory * @returns Resolved target, or null if resolution produced nothing * @throws {ContextError} If org-all mode is used (requires specific project) */ async function resolveTarget( - parsed: ReturnType, + parsed: ParsedOrgProject, rawLogIds: string[], cwd: string, ): Promise { @@ -369,6 +207,58 @@ async function resolveTarget( } } +/** The arguments of `log view`: the log IDs as typed, and the target. */ +type LogViewInput = ReturnType & { + readonly target: OrgProjectInput; +}; + +/** The target of `log view`, and its log IDs, validated or recovered. */ +type ResolvedLogs = ResolvedLogTarget & { readonly logIds: string[] }; + +/** The arguments of `log view`. */ +type LogViewArguments = BoundArguments; + +/** Parse `log view` arguments, without side effects. */ +function parseLogViewInput(args: readonly string[]): LogViewInput { + const input = parsePositionalArgs(args); + return { ...input, target: parseOrgProjectInput(input.targetArg) }; +} + +/** + * Resolve `log view` arguments: find the org and project, then validate + * (or recover) the log IDs in that project. + */ +async function resolveLogViewInput( + input: LogViewInput, + context: SentryContext, +): Promise { + if (input.target.baseUrl) { + applySentryUrlContext(input.target.baseUrl); + } + const cmdLog = logger.withTag("log.view"); + for (const warning of input.warnings) { + cmdLog.warn(warning); + } + if (input.suggestion) { + cmdLog.warn(input.suggestion); + } + const target = await resolveTarget( + input.target.parsed, + input.rawLogIds, + context.cwd, + ); + if (!target) { + throw new ContextError("Organization and project", USAGE_HINT); + } + // `log.warn` preserves emission order regardless of await order, and a + // throwing recovery (multi-match ResolutionError) exits via + // `Promise.all` with the first-thrown error. + const logIds = await Promise.all( + input.rawLogIds.map((raw) => validateAndRecoverLogId(raw, target)), + ); + return { ...target, logIds }; +} + /** * Format a list of log IDs as a markdown bullet list. * @@ -596,15 +486,14 @@ export const viewCommand = buildCommand({ : data.logs, }, parameters: { - positional: { - kind: "array", - parameter: { - placeholder: "org/project/log-id", - brief: - "[/] [...] - Target (optional) and one or more log IDs", - parse: parseOrgProjectTarget, - }, - }, + positional: boundArguments({ + placeholder: "org/project/log-id", + brief: + "[/] [...] - Target (optional) and one or more log IDs", + kind: ORG_PROJECT_TARGET, + parse: parseLogViewInput, + resolve: resolveLogViewInput, + }), flags: { web: { kind: "boolean", @@ -615,32 +504,10 @@ export const viewCommand = buildCommand({ }, aliases: { ...FRESH_ALIASES, w: "web" }, }, - async *func(this: SentryContext, flags: ViewFlags, ...args: string[]) { + async *func(this: SentryContext, flags: ViewFlags, args: LogViewArguments) { applyFreshFlag(flags); - const { cwd } = this; const cmdLog = logger.withTag("log.view"); - - // Parse positional args (raw — validation is deferred to after target - // resolution so we can run fuzzy recovery with org/project context). - const { rawLogIds, targetArg, suggestion } = parsePositionalArgs(args); - if (suggestion) { - cmdLog.warn(suggestion); - } - const parsed = parseOrgProjectArg(targetArg); - - const target = await resolveTarget(parsed, rawLogIds, cwd); - - if (!target) { - throw new ContextError("Organization and project", USAGE_HINT); - } - - // Validate + recover each log ID in parallel. `log.warn` preserves - // emission order regardless of await order, and a throwing recovery - // (multi-match ResolutionError) exits via `Promise.all` with the - // first-thrown error — same UX as the old sequential loop. - const logIds = await Promise.all( - rawLogIds.map((raw) => validateAndRecoverLogId(raw, target)), - ); + const { logIds, ...target } = await args.resolved; if (flags.web) { await handleWebOpen(target.org, logIds); diff --git a/packages/cli/src/commands/replay/download.ts b/packages/cli/src/commands/replay/download.ts index 996e30840..6637115b2 100644 --- a/packages/cli/src/commands/replay/download.ts +++ b/packages/cli/src/commands/replay/download.ts @@ -19,7 +19,6 @@ import { formatReplayDownloadResult, type ReplayDownloadData, } from "../../lib/formatters/replay.js"; -import { validateHexId } from "../../lib/hex-id.js"; import { applyFreshFlag, FRESH_ALIASES, @@ -31,9 +30,12 @@ import { rrwebDurationMs, toRRWebEvents, } from "../../lib/replay-rrweb.js"; -import { resolveOrgOptionalFromArg } from "../../lib/resolve-target.js"; import type { ReplayDetails } from "../../types/index.js"; -import { parsePositionalArgs, validateReplayProjectScope } from "./view.js"; +import { + type ReplayArguments, + replayArguments, + validateReplayProjectScope, +} from "./view.js"; type DownloadFlags = { readonly output?: string; @@ -81,14 +83,7 @@ export const downloadCommand = buildCommand({ human: formatReplayDownloadResult, }, parameters: { - positional: { - kind: "array", - parameter: { - placeholder: "replay-id-or-url", - brief: "[/] or ", - parse: String, - }, - }, + positional: replayArguments("download", USAGE_HINT), flags: { output: { kind: "parsed", @@ -101,21 +96,16 @@ export const downloadCommand = buildCommand({ }, aliases: { ...FRESH_ALIASES, o: "output" }, }, - async *func(this: SentryContext, flags: DownloadFlags, ...args: string[]) { + async *func( + this: SentryContext, + flags: DownloadFlags, + target: ReplayArguments, + ) { applyFreshFlag(flags); const { cwd } = this; - const parsedArgs = parsePositionalArgs(args, USAGE_HINT); - if (parsedArgs.warning) { - log.warn(parsedArgs.warning); - } - - const replayId = validateHexId(parsedArgs.replayId, "replay ID"); - const resolved = await resolveOrgOptionalFromArg( - parsedArgs.targetArg, - cwd, - "replay download", - ); + const resolved = await target.resolved; + const { replayId } = resolved; let replay: ReplayDetails; try { diff --git a/packages/cli/src/commands/replay/view.ts b/packages/cli/src/commands/replay/view.ts index ee2e5132e..8111fb690 100644 --- a/packages/cli/src/commands/replay/view.ts +++ b/packages/cli/src/commands/replay/view.ts @@ -13,18 +13,13 @@ import { resolveReplay, } from "../../lib/api-client.js"; import { - detectSwappedViewArgs, - parseSlashSeparatedArg, + type ParsedOrgProject, + parseOrgProjectInput, } from "../../lib/arg-parsing.js"; import { openInBrowser } from "../../lib/browser.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgProjectTarget } from "../../lib/completion-target.js"; -import { - ApiError, - ContextError, - ResolutionError, - ValidationError, -} from "../../lib/errors.js"; +import { ORG_PROJECT_TARGET } from "../../lib/completion-target.js"; +import { ApiError, ResolutionError } from "../../lib/errors.js"; import { filterFields } from "../../lib/formatters/json.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { @@ -33,18 +28,22 @@ import { type ReplayViewData, replayHint, } from "../../lib/formatters/replay.js"; -import { tryNormalizeHexId, validateHexId } from "../../lib/hex-id.js"; +import { ID_KINDS, parseIdArgument } from "../../lib/id-arguments.js"; import { applyFreshFlag, FRESH_ALIASES, FRESH_FLAG, } from "../../lib/list-command.js"; import { logger } from "../../lib/logger.js"; -import { resolveOrgOptionalFromArg } from "../../lib/resolve-target.js"; import { - applySentryUrlContext, - parseSentryUrl, -} from "../../lib/sentry-url-parser.js"; + type BoundArguments, + boundArguments, +} from "../../lib/positional-parse.js"; +import { + type ResolvedOrgOptionalTarget, + resolveOrgOptionalTarget, +} from "../../lib/resolve-target.js"; +import { applySentryUrlContext } from "../../lib/sentry-url-parser.js"; import { buildReplayUrl } from "../../lib/sentry-web-urls.js"; import type { ReplayActivityEvent, @@ -61,12 +60,6 @@ type ViewFlags = { readonly fields?: string[]; }; -type ParsedPositionalArgs = { - replayId: string; - targetArg: string | undefined; - warning?: string; -}; - const USAGE_HINT = "sentry replay view [//] | "; const MAX_ACTIVITY_EVENTS = 6; @@ -76,106 +69,84 @@ const MAX_RELATED_TRACES = 2; const log = logger.withTag("replay.view"); /** - * Parse a single positional argument as a replay target. - * - * Handles bare replay IDs, `/`, `//`, - * and Sentry replay URLs. The single-slash case (`org/id`) needs special - * handling because 32-char hex replay IDs look valid to the generic - * `parseSlashSeparatedArg` which would misinterpret the org as a project. - */ -function parseSingleArg(arg: string, usageHint: string): ParsedPositionalArgs { - const trimmed = arg.trim(); - if (!trimmed) { - throw new ContextError("Replay ID", usageHint, []); - } - - // Handle / shorthand — must check before parseSlashSeparatedArg - // because replay IDs are 32-char hex strings that look valid to the generic - // slash parser's ID extraction, but with only one slash the "project" segment - // would be wrongly treated as the ID. - const slashIdx = trimmed.indexOf("/"); - if (slashIdx !== -1 && trimmed.indexOf("/", slashIdx + 1) === -1) { - const org = trimmed.slice(0, slashIdx); - const replaySegment = trimmed.slice(slashIdx + 1); - const normalizedReplayId = - replaySegment && tryNormalizeHexId(replaySegment); - if (!normalizedReplayId) { - throw new ContextError("Replay ID", usageHint, []); - } - return { replayId: normalizedReplayId, targetArg: `${org}/` }; - } - - const { id: replayId, targetArg } = parseSlashSeparatedArg( - trimmed, - "Replay ID", - usageHint, - ); - return { replayId, targetArg }; -} - -/** - * Parse replay view positional arguments. - * - * Supports: - * - `` - * - `/` - * - `//` - * - ` ` - * - `` + * Parse `replay view` and `replay download` arguments: a replay ID in the + * forms every ID-taking command takes (see `id-arguments.ts`), or a replay + * URL. * * `usageHint` lets sibling replay commands show their own usage in errors. + * Has no side effects: a pasted URL's server is returned as `baseUrl`. */ -export function parsePositionalArgs( - args: string[], +export function parseReplayArguments( + args: readonly string[], usageHint = USAGE_HINT, -): ParsedPositionalArgs { - if (args.length === 0) { - throw new ContextError("Replay ID", usageHint, []); - } - if (args.length > 2) { - throw new ValidationError( - `Too many positional arguments (got ${args.length}, expected at most 2).\n\nUsage: ${usageHint}`, - "positional", - ); - } - - const first = args[0]; - if (!first) { - throw new ContextError("Replay ID", usageHint, []); - } - - const urlParsed = parseSentryUrl(first); - if (urlParsed) { - applySentryUrlContext(urlParsed.baseUrl); - if (urlParsed.replayId && urlParsed.org) { - return { replayId: urlParsed.replayId, targetArg: `${urlParsed.org}/` }; - } - throw new ContextError("Replay ID", usageHint, [ - "Pass a replay URL: https://sentry.io/organizations/{org}/explore/replays/{replayId}/", - ]); - } +): ReplayArgumentsInput { + const { target, id, baseUrl, warnings } = parseIdArgument( + args, + ID_KINDS.replay, + { usageHint }, + ); + const parsed = parseOrgProjectInput(target); + return { + replayId: id, + target: parsed.parsed, + baseUrl: baseUrl ?? parsed.baseUrl, + warnings, + }; +} - if (args.length === 1) { - return parseSingleArg(first, usageHint); - } +/** Parsed `replay view` and `replay download` arguments. */ +export type ReplayArgumentsInput = { + readonly replayId: string; + readonly target: ParsedOrgProject; + /** Server of a pasted Sentry URL, for `applySentryUrlContext` */ + readonly baseUrl?: string; + /** Warnings to log, such as for reversed arguments */ + readonly warnings: readonly string[]; +}; - const second = args[1]; - if (!second) { - throw new ContextError("Replay ID", usageHint, []); - } +/** A replay ID and the org (and optional project) to look it up in. */ +export type ResolvedReplayTarget = ResolvedOrgOptionalTarget & { + readonly replayId: string; +}; - const warning = - args.length === 2 ? detectSwappedViewArgs(first, second) : null; - if (warning) { - const normalizedReplayId = tryNormalizeHexId(first) ?? first; - return { - replayId: normalizedReplayId, - targetArg: second, - warning, - }; - } +/** The arguments of `replay view` and `replay download`. */ +export type ReplayArguments = BoundArguments< + ReplayArgumentsInput, + ResolvedReplayTarget +>; - return { replayId: second, targetArg: first }; +/** + * Positional for `[/] ` or a replay URL, shared by + * `replay view` and `replay download`. + */ +export function replayArguments( + command: "view" | "download", + usageHint: string, +) { + const commandLog = logger.withTag(`replay.${command}`); + return boundArguments({ + placeholder: "replay-id-or-url", + brief: "[/] or ", + kind: ORG_PROJECT_TARGET, + parse: (raw) => parseReplayArguments(raw, usageHint), + resolve: async ( + input, + context: SentryContext, + ): Promise => { + if (input.baseUrl) { + applySentryUrlContext(input.baseUrl); + } + for (const warning of input.warnings) { + commandLog.warn(warning); + } + const resolved = await resolveOrgOptionalTarget( + input.target, + context.cwd, + `replay ${command}`, + ); + return { ...resolved, replayId: input.replayId }; + }, + }); } type ReplayProjectScope = { @@ -397,14 +368,7 @@ export const viewCommand = buildCommand({ schema: ReplayViewOutputSchema, }, parameters: { - positional: { - kind: "array", - parameter: { - placeholder: "replay-id-or-url", - brief: "[/] or ", - parse: parseOrgProjectTarget, - }, - }, + positional: replayArguments("view", USAGE_HINT), flags: { web: { kind: "boolean", @@ -415,21 +379,11 @@ export const viewCommand = buildCommand({ }, aliases: { ...FRESH_ALIASES, w: "web" }, }, - async *func(this: SentryContext, flags: ViewFlags, ...args: string[]) { + async *func(this: SentryContext, flags: ViewFlags, target: ReplayArguments) { applyFreshFlag(flags); - const { cwd } = this; - const parsedArgs = parsePositionalArgs(args); - if (parsedArgs.warning) { - log.warn(parsedArgs.warning); - } - - const replayId = validateHexId(parsedArgs.replayId, "replay ID"); - const resolved = await resolveOrgOptionalFromArg( - parsedArgs.targetArg, - cwd, - "replay view", - ); + const resolved = await target.resolved; + const { replayId } = resolved; let replay: ReplayDetails; try { diff --git a/packages/cli/src/commands/span/list.ts b/packages/cli/src/commands/span/list.ts index 6b73a079a..2266b7e6c 100644 --- a/packages/cli/src/commands/span/list.ts +++ b/packages/cli/src/commands/span/list.ts @@ -197,7 +197,7 @@ export function parseSort(value: string): SpanSortValue { export function parseSpanListArgs( args: string[], ): ReturnType { - return parseDualModeArgs(args, TRACE_USAGE_HINT); + return parseDualModeArgs(args, TRACE_USAGE_HINT, { needsProject: true }); } // --------------------------------------------------------------------------- diff --git a/packages/cli/src/commands/span/view.ts b/packages/cli/src/commands/span/view.ts index a3ed686d7..30aff9040 100644 --- a/packages/cli/src/commands/span/view.ts +++ b/packages/cli/src/commands/span/view.ts @@ -13,7 +13,7 @@ import { } from "../../lib/api-client.js"; import { spansFlag } from "../../lib/arg-parsing.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgProjectTarget } from "../../lib/completion-target.js"; +import { ORG_PROJECT_TARGET } from "../../lib/completion-target.js"; import { ContextError, ResolutionError, @@ -28,11 +28,12 @@ import { import { filterFields } from "../../lib/formatters/json.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { computeSpanDurationMs } from "../../lib/formatters/time-utils.js"; -import { HEX_ID_RE, SPAN_ID_RE, validateSpanId } from "../../lib/hex-id.js"; +import { validateSpanId } from "../../lib/hex-id.js"; import { handleRecoveryResult, recoverHexId, } from "../../lib/hex-id-recovery.js"; +import { ID_KINDS, parseIdArguments } from "../../lib/id-arguments.js"; import { applyFreshFlag, FRESH_ALIASES, @@ -40,9 +41,15 @@ import { } from "../../lib/list-command.js"; import { logger } from "../../lib/logger.js"; import { - parseSlashSeparatedTraceTarget, - parseTraceTargetWithRecovery, + type BoundArguments, + boundArguments, +} from "../../lib/positional-parse.js"; +import { + parseTraceArgumentsInput, + type ResolvedTraceOrgProject, + resolveTraceArguments, resolveTraceOrgProject, + type TraceArgumentsInput, warnIfNormalized, } from "../../lib/trace-target.js"; @@ -59,195 +66,119 @@ type ViewFlags = { const USAGE_HINT = "sentry span view [//] [...]"; -/** Result of the initial positional-arg parse for `span view`. */ -type SpanViewArgs = - | { - /** Args are already resolved (auto-split `/`). */ - kind: "resolved"; - traceTarget: ReturnType; - rawSpanIds: string[]; - } - | { - /** - * First arg is the raw trace target (needs async recovery); the rest - * are raw span IDs. Command layer calls `parseTraceTargetWithRecovery`. - */ - kind: "deferred"; - rawTraceArg: string; - rawSpanIds: string[]; - }; +/** The arguments of `span view`, parsed but with the IDs as typed. */ +export type SpanViewInput = { + /** The trace, validated (or recovered) when the arguments resolve */ + readonly trace: TraceArgumentsInput; + readonly rawSpanIds: readonly string[]; +}; -/** - * Parse positional arguments for span view. - * - * Handles a few sync edge cases (single-arg `/` slash split, - * bare span ID without a trace) and returns either a fully-resolved - * target or a deferred form. In the deferred case the command layer calls - * {@link parseTraceTargetWithRecovery} on `rawTraceArg` so a malformed - * trace ID gets the full recovery treatment (matching `trace view` - * behavior). - * - * @throws {ContextError} If insufficient arguments or a single bare span ID - */ +/** The trace and the span IDs of `span view`, validated or recovered. */ +type ResolvedSpans = ResolvedTraceOrgProject & { + readonly spanIds: string[]; +}; + +/** The arguments of `span view`. */ +type SpanViewArguments = BoundArguments; /** - * Try to auto-split a single arg of the form `/`. + * Parse `span view` arguments, without side effects. * - * Handles one-slash (`/`) and multi-slash - * (`org/project//`) forms. Returns null when the arg - * cannot be unambiguously split (e.g. left side is not a hex trace ID in - * the single-slash case). Throws a trace-focused {@link ContextError} when - * the arg clearly intends the `/` form but the - * trace-target portion is invalid (e.g. `org/project/`). + * Span IDs take the forms every ID-taking command does (see + * `id-arguments.ts`), with the trace as their target: + * `[//] ...`, or + * `[//]/` in one argument (as `span list` + * prints them), with more span IDs after it. * - * @throws {ContextError} When the trace-target prefix fails validation. + * IDs are kept as typed: {@link resolveSpanViewInput} validates the trace + * and then the spans once it knows the trace, so malformed IDs can be + * recovered. + * + * @throws {ContextError} If the trace or the span IDs are missing */ -function tryAutoSplitSpanArg(arg: string): SpanViewArgs | null { - const lastSlashIdx = arg.lastIndexOf("/"); - if (lastSlashIdx === -1) { - return null; - } - - const tracePrefix = arg.slice(0, lastSlashIdx); - const possibleSpanId = arg - .slice(lastSlashIdx + 1) - .trim() - .toLowerCase() - .replace(/-/g, ""); - - if (!SPAN_ID_RE.test(possibleSpanId) || tracePrefix.length === 0) { - return null; - } - - const hasMultipleSlashes = tracePrefix.includes("/"); - const normalizedPrefix = tracePrefix.trim().toLowerCase().replace(/-/g, ""); +export function parseSpanViewInput(rawArgs: readonly string[]): SpanViewInput { + // A lone argument that does not end in a span ID is the trace + const { target, ids } = parseIdArguments(rawArgs, ID_KINDS.span, { + usageHint: USAGE_HINT, + multiple: true, + optional: true, + needsProject: true, + validate: false, + }); + const [first] = ids; - // For single-slash form the prefix must be a hex trace ID. - // For multi-slash (org/project/trace-id) always attempt the split. - if (!(hasMultipleSlashes || HEX_ID_RE.test(normalizedPrefix))) { - return null; + if (target === undefined) { + // Span IDs without a trace: say so, rather than "invalid trace ID" + // (CLI-SC) + throw new ContextError( + "Trace ID and span ID", + USAGE_HINT, + first && ID_KINDS.span.isId(first) + ? [ + `'${first}' looks like a span ID (16 characters), not a trace ID`, + `Provide the trace ID first: sentry span view ${validateSpanId(first)}`, + `Use 'sentry trace list' to find trace IDs`, + ] + : [], + ); } - let traceTarget: ReturnType; - try { - traceTarget = parseSlashSeparatedTraceTarget(tracePrefix, USAGE_HINT); - } catch (_error) { - // We already confirmed the last segment is a span ID and the prefix - // looked like a trace target, so a validation failure here means the - // trace-target portion is bad — e.g. `org/project/` (missing - // the trace ID) or `org/project//`. Surface a - // trace-focused ContextError that preserves the underlying detail - // rather than falling through to the misleading "missing span ID" path. - const suggestion = HEX_ID_RE.test(tracePrefix.split("/").pop() || "") - ? `Provide the correct trace ID: sentry span view ${tracePrefix} ${possibleSpanId}` - : `Provide the trace ID: sentry span view ${tracePrefix}/ ${possibleSpanId}`; - throw new ContextError("Trace ID", USAGE_HINT, [ - `'${arg}' is missing a valid trace ID before the span ID '${possibleSpanId}'`, - suggestion, + if (first === undefined) { + throw new ContextError("Span ID", USAGE_HINT, [ + `Use 'sentry span list ${target}' to find span IDs within this trace`, ]); } - log.warn( - `Interpreting '${arg}' as /. ` + - `Use separate arguments: sentry span view ${tracePrefix} ${possibleSpanId}`, - ); - return { kind: "resolved", traceTarget, rawSpanIds: [possibleSpanId] }; -} -/** Splits a string on any run of whitespace (spaces, tabs, newlines). */ -const WHITESPACE_SPLIT_RE = /\s+/; + return { trace: parseTrace(target, first), rawSpanIds: ids }; +} /** - * Split positional args on whitespace and drop empty tokens. + * Parse the target of the span IDs as the trace, with a trace-focused error + * when it has no trace ID, as in `//` (CLI-1GP). A + * malformed trace ID is left for {@link resolveTraceArguments}. * - * AI agents and scripts frequently pass the whole positional list as a - * single quoted argument — e.g. `"org/project/ "` or - * `"/\n"`. The shell never splits those, so - * without this normalization the span IDs are invisible to the parser and - * the command fails with "Span ID is required" (CLI-1BD). Trace IDs, span - * IDs, and org/project slugs never contain whitespace, so splitting is safe. + * @throws {ContextError} When the trace ID is missing */ -function splitWhitespaceArgs(args: string[]): string[] { - return args - .flatMap((arg) => arg.split(WHITESPACE_SPLIT_RE)) - .filter((token) => token.length > 0); -} - -export function parsePositionalArgs(rawArgs: string[]): SpanViewArgs { - const args = splitWhitespaceArgs(rawArgs); - const wasSplit = - args.length !== rawArgs.length || - args.some((token, i) => token !== rawArgs[i]); - if (wasSplit && args.length > 0) { - log.warn( - `Split whitespace-separated arguments into: ${args.join(" ")}. ` + - `Pass each ID as a separate argument: ${USAGE_HINT}`, - ); - } - - if (args.length === 0) { - throw new ContextError("Trace ID and span ID", USAGE_HINT, []); - } - - const first = args[0]; - if (first === undefined) { - throw new ContextError("Trace ID and span ID", USAGE_HINT, []); - } - - // Auto-detect single-arg forms where the last slash-segment is a span ID. - // This handles: - // / (one slash) - // [/][/]/ (two+ slashes, e.g. copy-paste from `span list`) - // Without this check, parseSlashSeparatedTraceTarget treats the span - // ID as a trace ID and fails validation (CLI-G6 / CLI-1BD). - if (args.length === 1) { - const resolved = tryAutoSplitSpanArg(first); - if (resolved) { - return resolved; - } - } - - // `/ ...` — the first token carries the - // first span ID and the remaining tokens are additional span IDs (e.g. a - // newline-separated list pasted after the slash form). - if (args.length > 1) { - let resolved: SpanViewArgs | null = null; - try { - resolved = tryAutoSplitSpanArg(first); - } catch { - // Not a valid `/` token; keep the standard - // ` ...` interpretation below. - resolved = null; - } - if (resolved) { - return { - ...resolved, - rawSpanIds: [...resolved.rawSpanIds, ...args.slice(1)], - }; - } - } - - // Single bare arg that looks like a span ID (16-char hex, no slashes): - // the user forgot the trace ID. Give a targeted ContextError instead - // of the confusing "Invalid trace ID" from validateTraceId() (CLI-SC). - if (args.length === 1 && !first.includes("/")) { - const normalized = first.trim().toLowerCase().replace(/-/g, ""); - if (SPAN_ID_RE.test(normalized)) { - throw new ContextError("Trace ID and span ID", USAGE_HINT, [ - `'${first}' looks like a span ID (16 characters), not a trace ID`, - `Provide the trace ID first: sentry span view ${normalized}`, - `Use 'sentry trace list' to find trace IDs`, - ]); +function parseTrace(traceArg: string, spanId: string): TraceArgumentsInput { + try { + return parseTraceArgumentsInput([traceArg], USAGE_HINT, { + needsProject: true, + }); + } catch (error) { + if (!(error instanceof ContextError)) { + throw error; } - } - - const rawSpanIds = args.slice(1); - if (rawSpanIds.length === 0) { - throw new ContextError("Span ID", USAGE_HINT, [ - `Use 'sentry span list ${first}' to find span IDs within this trace`, + throw new ContextError("Trace ID", USAGE_HINT, [ + `'${traceArg}' is missing a trace ID before the span ID '${spanId}'`, + `Provide the trace ID: sentry span view ${traceArg}/ ${spanId}`, ]); } +} - return { kind: "deferred", rawTraceArg: first, rawSpanIds }; +/** + * Resolve `span view` arguments: validate (or recover) the trace ID, find + * the trace's org and project, then validate (or recover) the span IDs in + * that trace. + */ +async function resolveSpanViewInput( + input: SpanViewInput, + context: SentryContext, +): Promise { + const traceTarget = await resolveTraceArguments(input.trace); + warnIfNormalized(traceTarget, "span.view"); + const { traceId, org, project } = await resolveTraceOrgProject( + traceTarget, + context.cwd, + USAGE_HINT, + ); + // Span IDs are unique only within a trace, so recovery scopes its fuzzy + // lookup via `ctx.traceId`. + const spanIds = await Promise.all( + input.rawSpanIds.map((raw) => + validateAndRecoverSpanId(raw, { org, project, traceId }), + ), + ); + return { traceId, org, project, spanIds }; } /** @@ -445,52 +376,28 @@ export const viewCommand = buildCommand({ jsonTransform: jsonTransformSpanView, }, parameters: { - positional: { - kind: "array", - parameter: { - placeholder: "trace-id/span-id", - brief: - "[//] [...] - Trace ID and one or more span IDs", - parse: parseOrgProjectTarget, - }, - }, + positional: boundArguments({ + placeholder: "trace-id/span-id", + brief: + "[//] [...] - Trace ID and one or more span IDs", + kind: ORG_PROJECT_TARGET, + parse: parseSpanViewInput, + resolve: resolveSpanViewInput, + }), flags: { ...spansFlag, fresh: FRESH_FLAG, }, aliases: { ...FRESH_ALIASES }, }, - async *func(this: SentryContext, flags: ViewFlags, ...args: string[]) { + async *func( + this: SentryContext, + flags: ViewFlags, + target: SpanViewArguments, + ) { applyFreshFlag(flags); - const { cwd } = this; - - // Parse positional args. Either we get a resolved trace target (single-arg - // / slash form) or a deferred trace arg that routes through - // `parseTraceTargetWithRecovery` for async trace-ID recovery — same - // contract as `trace view`. - const parsed = parsePositionalArgs(args); - const rawSpanIds = parsed.rawSpanIds; - const traceTarget = - parsed.kind === "resolved" - ? parsed.traceTarget - : await parseTraceTargetWithRecovery([parsed.rawTraceArg], USAGE_HINT); - warnIfNormalized(traceTarget, "span.view"); - - // Resolve org/project - const { traceId, org, project } = await resolveTraceOrgProject( - traceTarget, - cwd, - USAGE_HINT, - ); + const { traceId, org, project, spanIds } = await target.resolved; - // Validate + recover span IDs now that trace context is available. - // Span IDs are unique only within a trace, so recovery scopes its - // fuzzy lookup via `ctx.traceId`. - const spanIds = await Promise.all( - rawSpanIds.map((raw) => - validateAndRecoverSpanId(raw, { org, project, traceId }), - ), - ); // Fetch trace data (single fetch for all span lookups) const spans = await getDetailedTrace(org, traceId); diff --git a/packages/cli/src/commands/trace/view.ts b/packages/cli/src/commands/trace/view.ts index 3aa370eff..a256a927a 100644 --- a/packages/cli/src/commands/trace/view.ts +++ b/packages/cli/src/commands/trace/view.ts @@ -14,14 +14,10 @@ import { getProject, type TraceItemDetail, } from "../../lib/api-client.js"; -import { - detectSwappedViewArgs, - looksLikeIssueShortId, - spansFlag, -} from "../../lib/arg-parsing.js"; +import { looksLikeIssueShortId, spansFlag } from "../../lib/arg-parsing.js"; import { openInBrowser } from "../../lib/browser.js"; import { buildCommand } from "../../lib/command.js"; -import { parseOrgProjectTarget } from "../../lib/completion-target.js"; +import { ORG_PROJECT_TARGET } from "../../lib/completion-target.js"; import { ContextError, ResolutionError, @@ -44,12 +40,18 @@ import { FRESH_FLAG, } from "../../lib/list-command.js"; import { logger } from "../../lib/logger.js"; +import { + type BoundArguments, + boundArguments, +} from "../../lib/positional-parse.js"; import { resolveOrg, toNumericId } from "../../lib/resolve-target.js"; import { buildTraceUrl } from "../../lib/sentry-web-urls.js"; import { setOrgProjectContext } from "../../lib/telemetry.js"; import { - parseTraceTargetWithRecovery, + parseTraceArgumentsInput, + resolveTraceArguments, resolveTraceOrgOptionalProject, + type TraceArgumentsInput, warnIfNormalized, } from "../../lib/trace-target.js"; import type { TraceSpan } from "../../types/index.js"; @@ -87,68 +89,71 @@ function buildViewHint( } /** - * Detect UX issues in raw positional args before trace-target parsing. - * - * - **Single-arg issue short ID**: first arg looks like `CAM-82X` with no - * second arg → sets `issueShortId` for auto-recovery (resolve issue → trace). - * - **Swapped args**: user typed ` /` instead of - * `/ `. If detected, swaps them silently and warns. - * - **Two-arg issue short ID**: first arg looks like `CAM-82X` with a second - * arg → suggests `sentry issue view` (ambiguous intent, no auto-recovery). + * The arguments of `trace view`, parsed: an issue short ID to find the + * trace of, or the trace ID in the forms every ID-taking command takes + * (see `id-arguments.ts`), as typed until it resolves. + */ +export type TraceViewInput = + | { readonly issueShortId: string } + | { + readonly trace: TraceArgumentsInput; + /** Suggestion when the first argument looks like an issue short ID */ + readonly suggestion?: string; + }; + +/** + * Parse `trace view` arguments, without side effects. * - * Returns corrected args and optional warnings to emit. + * - **Single-arg issue short ID**: `CAM-82X` alone is looked up as an issue + * (resolve issue → trace). + * - **Two-arg issue short ID**: `CAM-82X` with a second argument suggests + * `sentry issue view` (ambiguous intent, no lookup). * * @internal Exported for testing */ -export function preProcessArgs(args: string[]): { - correctedArgs: string[]; - warning?: string; - suggestion?: string; - /** Issue short ID detected for auto-recovery (single-arg only) */ - issueShortId?: string; -} { - if (args.length === 0) { - return { correctedArgs: args }; - } - - const first = args[0]; - if (!first) { - return { correctedArgs: args }; - } - - // Single-arg issue short ID → auto-recover by resolving issue → trace - if (args.length === 1 && looksLikeIssueShortId(first)) { - return { - correctedArgs: args, - issueShortId: first, - }; +export function parseTraceViewInput(args: readonly string[]): TraceViewInput { + const [first] = args; + const shortId = first !== undefined && looksLikeIssueShortId(first); + if (shortId && args.length === 1) { + return { issueShortId: first }; } + return { + trace: parseTraceArgumentsInput(args, USAGE_HINT), + ...(shortId && { suggestion: `Did you mean: sentry issue view ${first}` }), + }; +} - if (args.length < 2) { - return { correctedArgs: args }; - } +/** The arguments of `trace view`. */ +type TraceViewArguments = BoundArguments; - const second = args[1]; - if (!second) { - return { correctedArgs: args }; +/** + * Resolve `trace view` arguments to the trace and the org (and project) to + * fetch it from, recovering a malformed trace ID. + */ +async function resolveTraceViewInput( + input: TraceViewInput, + context: SentryContext, +): Promise { + if ("issueShortId" in input) { + return await resolveTraceFromIssue(input.issueShortId, context.cwd); } - - // Detect swapped args: user put ID first and target second - const swapWarning = detectSwappedViewArgs(first, second); - if (swapWarning) { - // Swap them: put target first, trace ID second - return { - correctedArgs: [second, first, ...args.slice(2)], - warning: swapWarning, - }; + if (input.suggestion) { + logger.withTag("trace.view").warn(input.suggestion); } - - // Detect issue short ID passed as first arg (two-arg case — ambiguous) - const suggestion = looksLikeIssueShortId(first) - ? `Did you mean: sentry issue view ${first}` - : undefined; - - return { correctedArgs: args, suggestion }; + const parsed = await resolveTraceArguments(input.trace); + warnIfNormalized(parsed, "trace.view"); + const target = await resolveTraceOrgOptionalProject( + parsed, + context.cwd, + USAGE_HINT, + ); + return { + ...target, + projectFilter: + parsed.type === "explicit" + ? (target as { project?: string }).project + : undefined, + }; } /** @@ -492,15 +497,14 @@ export const viewCommand = buildCommand({ jsonTransform: jsonTransformTraceView, }, parameters: { - positional: { - kind: "array", - parameter: { - placeholder: "org/project/trace-id", - brief: - "[//] - Target (optional) and trace ID (required)", - parse: parseOrgProjectTarget, - }, - }, + positional: boundArguments({ + placeholder: "org/project/trace-id", + brief: + "[//] - Target (optional) and trace ID (required)", + kind: ORG_PROJECT_TARGET, + parse: parseTraceViewInput, + resolve: resolveTraceViewInput, + }), flags: { web: { kind: "boolean", @@ -517,44 +521,13 @@ export const viewCommand = buildCommand({ }, aliases: { ...FRESH_ALIASES, w: "web" }, }, - async *func(this: SentryContext, flags: ViewFlags, ...args: string[]) { + async *func( + this: SentryContext, + flags: ViewFlags, + target: TraceViewArguments, + ) { applyFreshFlag(flags); - const { cwd } = this; - const log = logger.withTag("trace.view"); - - // Pre-process: detect swapped args and issue short IDs - const { correctedArgs, warning, suggestion, issueShortId } = - preProcessArgs(args); - if (warning) { - log.warn(warning); - } - if (suggestion) { - log.warn(suggestion); - } - - let resolved: ResolvedTrace; - if (issueShortId) { - resolved = await resolveTraceFromIssue(issueShortId, cwd); - } else { - const parsed = await parseTraceTargetWithRecovery( - correctedArgs, - USAGE_HINT, - ); - warnIfNormalized(parsed, "trace.view"); - const target = await resolveTraceOrgOptionalProject( - parsed, - cwd, - USAGE_HINT, - ); - resolved = { - ...target, - projectFilter: - parsed.type === "explicit" - ? (target as { project?: string }).project - : undefined, - }; - } - const { traceId, org, project, projectFilter } = resolved; + const { traceId, org, project, projectFilter } = await target.resolved; if (flags.web) { await openInBrowser(buildTraceUrl(org, traceId), "trace"); diff --git a/packages/cli/src/lib/command.ts b/packages/cli/src/lib/command.ts index bc073dd4b..840300dcf 100644 --- a/packages/cli/src/lib/command.ts +++ b/packages/cli/src/lib/command.ts @@ -31,6 +31,7 @@ */ import { + ArgumentScannerError, type Command, type CommandContext, buildCommand as stricliCommand, @@ -69,6 +70,7 @@ import { import { applyBoundPositionals, checkDestructive, + isHelpArgument, rawPositional, } from "./positional-parse.js"; import { setArgsContext, setFlagContext, withTracing } from "./telemetry.js"; @@ -692,16 +694,13 @@ export function buildCommand< }, args: unknown[], ): Promise { - if (!(err instanceof CliError) || err instanceof OutputError) { + const recoverable = + (err instanceof CliError && !(err instanceof OutputError)) || + err instanceof ArgumentScannerError; + if (!recoverable) { return false; } - if ( - args.length === 0 || - !args.some((a) => { - const raw = rawPositional(a); - return raw === "help" || raw === "--h" || raw === "-help"; - }) - ) { + if (!args.flatMap(rawPositional).some(isHelpArgument)) { return false; } if (!ctx.commandPrefix || ctx.directInvoke) { @@ -733,12 +732,29 @@ export function buildCommand< ) { // Callers that skip Stricli's scanner (SDK invoke, tests) pass raw // strings, and omitted optional positionals are undefined; give bound - // positionals the value Stricli would. - const args = applyBoundPositionals( - positionalArgs, - existingParams.positional, - this, - ); + // positionals the value Stricli would. `boundArguments` positionals are + // parsed here for every caller. + let args: unknown[]; + let argumentError: ArgumentScannerError | undefined; + try { + args = applyBoundPositionals( + positionalArgs, + existingParams.positional, + this, + ); + } catch (error) { + // Next to a `help` argument, fail where it can turn into the help + if ( + !( + error instanceof ArgumentScannerError && + positionalArgs.some(isHelpArgument) + ) + ) { + throw error; + } + argumentError = error; + args = positionalArgs; + } applyLoggingFlags( flags[LOG_LEVEL_KEY] as LogLevelName | undefined, @@ -757,7 +773,7 @@ export function buildCommand< const cleanFlags = cleanRawFlags(flags as Record); setFlagContext(cleanFlags); if (args.length > 0) { - setArgsContext(args.map(rawPositional)); + setArgsContext(args.flatMap(rawPositional)); } // Environment-based JSON mode (used by library entry point) @@ -820,6 +836,10 @@ export function buildCommand< // `sentry issue list help`). Without this, the auth prompt would fire // before the help-recovery path could show the command's help text. try { + if (argumentError) { + throw argumentError; + } + // A usable stored account defers the check: the command's target may // select it even when the account in effect is logged out. if (requiresAuth && !getAuthConfig() && !hasUsableStoredAccount()) { diff --git a/packages/cli/src/lib/errors.ts b/packages/cli/src/lib/errors.ts index 71fb08bd2..d20a84e1c 100644 --- a/packages/cli/src/lib/errors.ts +++ b/packages/cli/src/lib/errors.ts @@ -23,6 +23,7 @@ * @see https://cli.sentry.dev/exit-codes/ for full reference */ +import { ArgumentScannerError, ExitCode } from "@stricli/core"; import { redactCredentialText } from "./credential-redaction.js"; import { buildBillingUrl, @@ -845,12 +846,18 @@ export function formatError(error: unknown): string { * Get process exit code for an error. * * @param error - Any thrown value - * @returns Exit code (from CliError.exitCode or 1 for other errors) + * @returns Exit code (from CliError.exitCode, Stricli's `InvalidArgument` + * for an argument error the command wrapper threw, or 1 for other errors) */ export function getExitCode(error: unknown): number { if (error instanceof CliError) { return error.exitCode; } + // `boundArguments` positionals parse in the command wrapper, after + // Stricli's scanner; their errors exit like the scanner's + if (error instanceof ArgumentScannerError) { + return ExitCode.InvalidArgument; + } return 1; } diff --git a/packages/cli/src/lib/hex-id-recovery.ts b/packages/cli/src/lib/hex-id-recovery.ts index e6d7dadfe..8dd9a95a0 100644 --- a/packages/cli/src/lib/hex-id-recovery.ts +++ b/packages/cli/src/lib/hex-id-recovery.ts @@ -196,6 +196,27 @@ const SLUG_REDIRECT_COMMAND: Record = { // Pure helpers (exported for tests) // --------------------------------------------------------------------------- +/** + * Strip URL fragment prefixes (`span-`, `trace-`, ...) from a lowercased + * input. Repeatedly, so it is idempotent: a pathological double-prefixed + * input like `span-span-abc` would otherwise keep its inner prefix. + */ +function stripUrlFragmentPrefixes(lowered: string): string { + let cleaned = lowered; + let stripped = true; + while (stripped) { + stripped = false; + for (const prefix of URL_FRAGMENT_PREFIXES) { + if (cleaned.startsWith(prefix)) { + cleaned = cleaned.slice(prefix.length); + stripped = true; + break; + } + } + } + return cleaned; +} + /** * Pre-normalize a raw input before further analysis. Pipeline: * @@ -216,24 +237,7 @@ export function preNormalize(input: string): { cleaned: string; sentinel?: string; } { - const lowered = input.trim().toLowerCase(); - - // Strip URL fragment prefixes repeatedly so the function is idempotent. - // A pathological double-prefixed input like `span-span-abc` would otherwise - // strip only the outer `span-` and leave the inner prefix in place, which - // breaks `preNormalize(preNormalize(x)) === preNormalize(x)`. - let cleaned = lowered; - let stripped = true; - while (stripped) { - stripped = false; - for (const prefix of URL_FRAGMENT_PREFIXES) { - if (cleaned.startsWith(prefix)) { - cleaned = cleaned.slice(prefix.length); - stripped = true; - break; - } - } - } + let cleaned = stripUrlFragmentPrefixes(input.trim().toLowerCase()); // Sentinel check runs AFTER URL-prefix stripping so inputs like // `span-null` (shell variable leak through a URL-builder) surface as @@ -343,6 +347,43 @@ export function looksLikeSlug(input: string): boolean { return !(PURE_HEX_RE.test(dashless) && dashless.length >= MIN_FUZZY_PREFIX); } +/** + * {@link looksLikeSlug} after URL fragment prefixes are stripped: a prefix + * (`trace-`) has a dash and letters, but does not make the input a slug. + */ +function looksLikeUnprefixedSlug(input: string): boolean { + return looksLikeSlug(stripUrlFragmentPrefixes(input.trim().toLowerCase())); +} + +/** + * Whether `input`, not a valid ID, still has enough of one for + * {@link recoverHexId} to look it up: a hex prefix or ellipsized ID of at + * least {@link MIN_FUZZY_PREFIX} chars that is not a slug. The ID argument + * grammar uses it to read `/` as an ID to recover, not as a + * project. + */ +export function looksLikeHexIdFragment(input: string): boolean { + if (looksLikeUnprefixedSlug(input)) { + return false; + } + const { cleaned, sentinel } = preNormalize(input); + if (sentinel) { + return false; + } + return isLongEnough(extractHexCandidate(cleaned)); +} + +/** Whether `candidate` has enough hex chars for a fuzzy lookup */ +function isLongEnough( + candidate: HexCandidate | null, +): candidate is HexCandidate { + return ( + candidate !== null && + (candidate.prefix.length >= MIN_FUZZY_PREFIX || + (candidate.suffix?.length ?? 0) >= MIN_FUZZY_PREFIX) + ); +} + /** Returns true for paths with 4+ slash-separated non-empty segments. */ export function isOverNestedPath(input: string): boolean { return input.split("/").filter(Boolean).length >= OVER_NESTED_MIN_SEGMENTS; @@ -781,11 +822,12 @@ async function recoverHexIdInternal( return redirect; } - // Slug-shape check on the RAW input before fuzzy lookup. Slug-like - // inputs where the dashless form happens to start with enough hex - // chars (e.g. `cafe-babe-app` → `cafebabeapp`) would otherwise reach + // Slug-shape check on the input (without URL fragment prefixes, but + // before partial-UUID dashes are stripped) before fuzzy lookup. + // Slug-like inputs where the dashless form happens to start with enough + // hex chars (e.g. `cafe-babe-app` → `cafebabeapp`) would otherwise reach // runFuzzyLookup and waste an API call on a clearly-mistaken input. - if (looksLikeSlug(input)) { + if (looksLikeUnprefixedSlug(input)) { return { kind: "failed", original: input, @@ -795,11 +837,7 @@ async function recoverHexIdInternal( } const candidate = extractHexCandidate(cleaned); - const longEnough = - candidate !== null && - (candidate.prefix.length >= MIN_FUZZY_PREFIX || - (candidate.suffix?.length ?? 0) >= MIN_FUZZY_PREFIX); - if (!(longEnough && candidate)) { + if (!isLongEnough(candidate)) { // Raw-input slug check already ran above; here it's either a short // pure-hex prefix or a short non-hex blob — neither is a slug. return { diff --git a/packages/cli/src/lib/hex-id.ts b/packages/cli/src/lib/hex-id.ts index fd530028b..3047b418c 100644 --- a/packages/cli/src/lib/hex-id.ts +++ b/packages/cli/src/lib/hex-id.ts @@ -182,6 +182,13 @@ export function validateHexId(value: string, label: string): string { return normalized; } +/** Non-throwing check: whether {@link validateSpanId} accepts `value`. */ +export function isSpanId(value: string): boolean { + return SPAN_ID_RE.test( + value.trim().toLowerCase().replace(DASH_GLOBAL_RE, ""), + ); +} + /** * Validate that a string is a 16-character hexadecimal span ID. * Trims whitespace and normalizes to lowercase before validation. diff --git a/packages/cli/src/lib/id-arguments.ts b/packages/cli/src/lib/id-arguments.ts new file mode 100644 index 000000000..d9072972e --- /dev/null +++ b/packages/cli/src/lib/id-arguments.ts @@ -0,0 +1,329 @@ +/** + * Positionals that name IDs and the org/project to look them up in, parsed + * by `grammar/id-arguments.pegjs`. + * + * Every command that takes trace, span, replay, or log IDs shares one + * syntax, whatever the kind of ID: + * + * - ``: the org/project is auto-detected + * - `/` and `//`, like a URL (`/` + * for commands that need a project) + * - ` `, where `` is any org/project target + * - ` /`, reversed, with a warning + * - a Sentry URL with the ID in it + * + * and, for commands that take several, more IDs after any of those, each + * repeating the target or not. An argument may hold several lines, and IDs + * after a space: `"/ "` is two arguments. + * + * The command picks an {@link ID_KINDS} entry, which says what its IDs look + * like, how to validate them, and which part of a URL holds one. The grammar + * never guesses a kind from the shape of an ID: trace, replay, and log IDs + * all look alike. `sentry-cli/no-ad-hoc-id-arguments` keeps + * commands from parsing these forms on their own. + * + * Parsing has no side effects: a pasted URL's server comes back as `baseUrl` + * and anything to tell the user as `warnings`, for the caller to apply. + */ + +import { parse } from "../generated/id-arguments-grammar.js"; +import { ContextError, ValidationError } from "./errors.js"; +import { + isSpanId, + tryNormalizeHexId, + validateHexId, + validateSpanId, +} from "./hex-id.js"; +import { looksLikeHexIdFragment } from "./hex-id-recovery.js"; +import { type ParsedSentryUrl, parseSentryUrl } from "./sentry-url-parser.js"; + +/** A kind of ID a command takes. */ +export type IdKind = { + /** Name in messages, e.g. `"Trace ID"` */ + readonly label: string; + /** Whether `value` is an ID of this kind (after normalizing) */ + readonly isId: (value: string) => boolean; + /** + * Whether `value`, not an ID, still looks like a malformed one, so + * `/` names an ID to report or recover, not a project + */ + readonly mayBeId: (value: string) => boolean; + /** + * Normalize an ID. + * + * @throws {ValidationError} When `value` is not one + */ + readonly validate: (value: string) => string; + /** The ID of this kind in a parsed Sentry URL, if it has one */ + readonly fromUrl: (url: ParsedSentryUrl) => string | undefined; + /** A URL with this kind of ID, suggested when a pasted URL has none */ + readonly urlExample?: string; +}; + +function hexKind( + label: string, + fromUrl: IdKind["fromUrl"], + urlExample?: string, +): IdKind { + const lower = label.charAt(0).toLowerCase() + label.slice(1); + return { + label, + isId: (value) => tryNormalizeHexId(value) !== undefined, + mayBeId: looksLikeHexIdFragment, + validate: (value) => validateHexId(value, lower), + fromUrl, + urlExample, + }; +} + +/** The kinds of ID commands take. Commands use these instead of their own. */ +export const ID_KINDS = { + trace: hexKind("Trace ID", (url) => url.traceId), + // Replays are looked up in an org, so a replay URL needs one + replay: hexKind( + "Replay ID", + (url) => (url.org ? url.replayId : undefined), + "https://sentry.io/organizations/{org}/explore/replays/{replayId}/", + ), + log: hexKind("Log ID", () => undefined), + span: { + label: "Span ID", + isId: isSpanId, + mayBeId: looksLikeHexIdFragment, + validate: validateSpanId, + fromUrl: () => undefined, + }, +} as const satisfies Record; + +/** How a command takes its IDs. */ +export type IdArgumentsOptions = { + /** Usage shown in errors and warnings */ + readonly usageHint: string; + /** Whether the command takes more than one ID */ + readonly multiple?: boolean; + /** + * Whether the command may get no ID, like a list of a project's items or + * a trace's. Then a lone argument that does not end in an ID is a target. + */ + readonly optional?: boolean; + /** + * Whether the command needs a project, not just an org. Then `/` + * names project `` (found in any org), like ` ` does. + */ + readonly needsProject?: boolean; + /** + * Whether to validate the IDs. Commands that recover malformed IDs once + * they know the target turn it off, and validate later. + */ + readonly validate?: boolean; +}; + +/** ID arguments, parsed. */ +export type IdArguments = { + /** + * The org/project target argument, for `parseOrgProjectInput`; + * `undefined` to auto-detect. `/` gives `/` (the org), or `` + * when the command needs a project. + */ + readonly target?: string; + /** The IDs: normalized, or as typed when not validated */ + readonly ids: readonly string[]; + /** Server of a pasted Sentry URL, for `applySentryUrlContext` */ + readonly baseUrl?: string; + /** Warnings to log, such as for ignored or reversed arguments */ + readonly warnings: readonly string[]; +}; + +/** + * Parse ID arguments. + * + * @throws {ContextError} When the IDs are missing and not optional + * @throws {ValidationError} When an ID is invalid (unless not validated), a + * path has an empty org or project, or IDs name different targets + */ +export function parseIdArguments( + args: readonly string[], + kind: IdKind, + options: IdArgumentsOptions, +): IdArguments { + const { + usageHint, + multiple = false, + optional = false, + needsProject = false, + } = options; + const tokens = tokenize(args, kind.isId); + const syntax = parse(tokens.map((token) => `\0${token}`).join(""), { + optional, + isId: kind.isId, + mayBeId: kind.mayBeId, + idUrl: (value) => idUrl(value, kind), + }); + const warnings: string[] = []; + + const pathTarget = (prefix: string, text: string) => + pathTargetArg(prefix, text, { usageHint, needsProject }); + let target = syntax.path + ? pathTarget(syntax.target ?? "", syntax.ids[0]?.text ?? "") + : syntax.target; + const typed: { text: string; id: string }[] = []; + for (const token of syntax.ids) { + if (token.target) { + const own = pathTarget(token.target, token.text); + if (target === undefined) { + target = own; + } else if (own !== target && token.target !== target) { + throw new ValidationError( + `${kind.label}s reference different projects ('${target}' and '${own}'). ` + + `Pass IDs from one project per call: ${usageHint}`, + "positional", + ); + } + } + if (token.id !== "") { + typed.push(token); + } + } + + if (syntax.reversed && target !== undefined && typed[0]) { + warnings.push( + `Arguments appear reversed. Interpreting as: ${target} ${typed[0].text}`, + ); + } + + if (typed.length === 0) { + if (optional) { + return { target, ids: [], baseUrl: syntax.baseUrl, warnings }; + } + throw new ContextError( + kind.label, + usageHint, + missingSuggestions(tokens, kind), + ); + } + + let chosen = typed; + if (!multiple && typed.length > 1) { + // Keep the first real ID; the rest are extra + const first = typed.find((token) => kind.isId(token.id)) ?? typed[0]; + chosen = first ? [first] : []; + const extra = typed.filter((token) => token !== first); + warnings.push( + `Extra arguments ignored: ${extra.map((token) => token.text).join(" ")}. Expected: ${usageHint}`, + ); + } + + const validate = options.validate ?? true; + return { + target, + ids: chosen.map((token) => (validate ? kind.validate(token.id) : token.id)), + baseUrl: syntax.baseUrl, + warnings, + }; +} + +/** Arguments that name exactly one ID, parsed. */ +export type IdArgument = Omit & { + /** The ID: normalized, or as typed when not validated */ + readonly id: string; +}; + +/** + * Parse arguments that name one ID, like {@link parseIdArguments} for a + * command that takes neither several IDs nor none. + */ +export function parseIdArgument( + args: readonly string[], + kind: IdKind, + options: Omit, +): IdArgument { + const { ids, ...rest } = parseIdArguments(args, kind, options); + const [id] = ids; + if (id === undefined) { + throw new ContextError(kind.label, options.usageHint); + } + return { ...rest, id }; +} + +/** Line breaks between IDs pasted into one argument */ +const LINE_BREAK_RE = /\r?\n/; +const WHITESPACE_RE = /\s+/; + +/** + * The grammar's tokens: every line of every argument, trimmed, and with the + * IDs at the end of a line split off at spaces. A line's first word stays + * whole, so a project's display name keeps its spaces. Empty lines are + * dropped: an empty target means none. + */ +function tokenize( + args: readonly string[], + isId: (value: string) => boolean, +): string[] { + const tokens: string[] = []; + for (const arg of args) { + if (arg.includes("\0")) { + throw new ValidationError( + "Arguments cannot contain NUL characters.", + "positional", + ); + } + for (const line of arg.split(LINE_BREAK_RE)) { + const words = line.trim().split(WHITESPACE_RE); + const ids: string[] = []; + while (words.length > 1 && isId(words.at(-1) ?? "")) { + ids.unshift(words.pop() ?? ""); + } + tokens.push(...[words.join(" "), ...ids].filter((token) => token)); + } + } + return tokens; +} + +/** + * The target argument for the `[/[/]]` before an ID in a path + * (`text`): `/` with both; with one, `/`, or the project + * when the command needs one. + * + * @throws {ValidationError} When the org or the project is empty + */ +function pathTargetArg( + prefix: string, + text: string, + options: { usageHint: string; needsProject: boolean }, +): string { + const { usageHint, needsProject } = options; + const slash = prefix.indexOf("/"); + if (slash === -1) { + return needsProject ? prefix : `${prefix}/`; + } + if (slash === 0 || slash === prefix.length - 1) { + throw new ValidationError( + `Invalid format: "${text}" needs an organization and a project before the ID.\n\nUsage: ${usageHint}`, + "positional", + ); + } + return prefix; +} + +/** The ID of `kind` in a Sentry URL, or `null` for anything else. */ +function idUrl(value: string, kind: IdKind) { + const url = parseSentryUrl(value); + const id = url ? kind.fromUrl(url) : undefined; + if (!(url && id)) { + return null; + } + return { + id, + ...(url.org && { org: url.org }), + baseUrl: url.baseUrl, + }; +} + +function missingSuggestions(tokens: readonly string[], kind: IdKind): string[] { + const pastedUrl = tokens.some((token) => parseSentryUrl(token) !== null); + return pastedUrl && kind.urlExample + ? [ + `Pass a ${kind.label.replace(" ID", "").toLowerCase()} URL: ${kind.urlExample}`, + ] + : []; +} diff --git a/packages/cli/src/lib/list-command.ts b/packages/cli/src/lib/list-command.ts index 663f4beb7..19ac91ace 100644 --- a/packages/cli/src/lib/list-command.ts +++ b/packages/cli/src/lib/list-command.ts @@ -44,7 +44,12 @@ import { type ListResult, type OrgListConfig, } from "./org-list.js"; -import { applyBoundPositionals, rawPositional } from "./positional-parse.js"; +import { + applyBoundPositionals, + rawPositional, + skipLeadingArgument, + skippedArgument, +} from "./positional-parse.js"; import { disableResponseCache } from "./response-cache.js"; import { PERIOD_BRIEF, parsePeriod } from "./time-range.js"; import { shellQuote } from "./utils.js"; @@ -533,19 +538,21 @@ export function interceptSubcommand( stderr: { write(s: string): void }, routeName: string, ): string | undefined { - if (!target) { + if (!(target && isSubcommandName(target, routeName))) { return target; } - const trimmed = target.trim(); - if (trimmed && getSubcommandsForRoute(routeName).has(trimmed)) { - stderr.write( - warning( - `Tip: "${trimmed}" is a subcommand. Running: sentry ${routeName} ${trimmed}\n`, - ), - ); - return; - } - return target; + stderr.write( + warning( + `Tip: "${target.trim()}" is a subcommand. Running: sentry ${routeName} ${target.trim()}\n`, + ), + ); + return; +} + +/** Whether `arg` names a subcommand of `routeName` (see {@link interceptSubcommand}). */ +function isSubcommandName(arg: string, routeName: string): boolean { + const trimmed = arg.trim(); + return trimmed !== "" && getSubcommandsForRoute(routeName).has(trimmed); } // --------------------------------------------------------------------------- @@ -652,8 +659,15 @@ export function buildListCommand< mergedAliases.c = "cursor"; } + // `boundArguments` positionals parse every argument before this wrapper + // runs, so a subcommand name is left out of that parse, before it changes + // what the other arguments mean + const positional = skipLeadingArgument(params.positional, (arg) => + isSubcommandName(arg, routeName), + ); const mergedParams = { ...params, + ...(positional ? { positional } : {}), flags: mergedFlags, aliases: mergedAliases, }; @@ -665,17 +679,22 @@ export function buildListCommand< // The first positional arg is always the target (org/project pattern), // either a raw string or a parsed value. Intercept it to handle plural - // alias confusion. + // alias confusion. All list commands use SentryContext which has stderr + // at top level + const ctx = this as unknown as { stderr: { write(s: string): void } }; + const skipped = skippedArgument(args[0]); const rawTarget = rawPositional(args[0]); - if (typeof rawTarget === "string") { - // All list commands use SentryContext which has stderr at top level - const ctx = this as unknown as { stderr: { write(s: string): void } }; - if (interceptSubcommand(rawTarget, ctx.stderr, routeName) === undefined) { - args[0] = undefined; - } + if (skipped !== undefined) { + // The scanner already parsed the other arguments without it + interceptSubcommand(skipped, ctx.stderr, routeName); + } else if ( + typeof rawTarget === "string" && + interceptSubcommand(rawTarget, ctx.stderr, routeName) === undefined + ) { + args[0] = undefined; } // A dropped bound target becomes an omitted one (auto-detect) - const finalArgs = applyBoundPositionals(args, params.positional, this); + const finalArgs = applyBoundPositionals(args, 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 d4908d7da..f9977e4ac 100644 --- a/packages/cli/src/lib/positional-parse.ts +++ b/packages/cli/src/lib/positional-parse.ts @@ -24,8 +24,15 @@ * always gets a {@link BoundPositional}. Shared code that handles any * positional (plural alias interception, `help` recovery, telemetry) reads * the raw input with {@link rawPositional}. + * + * {@link boundArguments} does the same for positionals whose meaning depends + * on how many arguments were given, such as `[/] `. A + * Stricli parser sees one argument at a time, so `buildCommand` parses all + * of them at once before `func` runs and hands `func` one + * {@link BoundArguments}. */ +import { ArgumentParseError } from "@stricli/core"; import { type CompletionTarget, withCompletionTarget, @@ -37,24 +44,43 @@ const destructiveChecks = new WeakMap void>(); /** Parsers made by {@link boundPositional}. */ const boundParsers = new WeakSet(); +/** + * Parses every argument of a {@link boundArguments} positional at once, into + * a {@link BoundArguments}. + */ +type ArgumentsParser = (this: unknown, raw: readonly string[]) => object; + +/** Whole-argument parsers of {@link boundArguments} positionals. */ +const argumentParsers = new WeakMap(); + +/** Leading arguments {@link skipLeadingArgument} left out, by bound value. */ +const skippedArguments = new WeakMap(); + +/** + * Whether the user typed `help` (or a mistyped help flag) as an argument, + * which `buildCommand` answers with the command's help. + */ +export function isHelpArgument(raw: unknown): boolean { + return raw === "help" || raw === "--h" || raw === "-help"; +} + /** * A parsed positional value bound to its resolver, which runs once, on the * first `resolved` read. */ -export class BoundPositional { +export class BoundPositional { #resolved: Promise | undefined; readonly #resolve: (value: T) => Promise; - /** What the user typed; `undefined` for an omitted positional. */ - readonly raw: string | undefined; + /** + * What the user typed; `undefined` for an omitted positional, every + * argument for {@link boundArguments}. + */ + readonly raw: RAW; /** The parsed value. */ readonly parsed: T; - constructor( - raw: string | undefined, - parsed: T, - resolve: (value: T) => Promise, - ) { + constructor(raw: RAW, parsed: T, resolve: (value: T) => Promise) { this.raw = raw; this.parsed = parsed; this.#resolve = resolve; @@ -97,6 +123,109 @@ export function boundPositional( return parser; } +/** Every argument of a {@link boundArguments} positional, parsed and bound. */ +export type BoundArguments = BoundPositional; + +/** + * Build an array positional whose arguments parse as a whole, for syntaxes + * where what an argument means depends on how many were given (e.g. + * `[/] `, or a trailing trace ID). `func` gets one + * {@link BoundArguments} instead of `...args: string[]`. + * + * Stricli only tags each argument for completion as `kind`. `buildCommand` + * then parses them all before `func` runs. A parse error becomes Stricli's + * {@link ArgumentParseError}, which fails like any other argument (exit 252), + * unless an argument is `help`: then it turns into the command's help. + * + * @param options.parse - Parses all the arguments, possibly none; may + * throw. Must have no side effects + * @param options.resolve - Resolves the parsed value; gets the command + * context and the raw arguments + */ +export function boundArguments(options: { + readonly placeholder: string; + readonly brief: string; + readonly kind: CompletionTarget; + readonly parse: (raw: readonly string[]) => T; + readonly resolve: ( + value: T, + context: C, + raw: readonly string[], + ) => Promise; +}): { + readonly kind: "array"; + readonly parameter: { + readonly placeholder: string; + readonly brief: string; + readonly parse: (input: string) => string; + }; +} { + const positional = { + kind: "array", + parameter: { + placeholder: options.placeholder, + brief: options.brief, + parse: withCompletionTarget(options.kind, (input: string) => input), + }, + } as const; + argumentParsers.set(positional, function (this: unknown, values) { + const raw = [...values]; + let parsed: T; + try { + parsed = options.parse(raw); + } catch (error) { + throw new ArgumentParseError( + options.placeholder as ConstructorParameters< + typeof ArgumentParseError + >[0], + raw.join(" "), + error, + ); + } + return new BoundPositional(raw, parsed, (value: T) => + options.resolve(value, this as C, raw), + ); + }); + return positional; +} + +/** + * Wrap a {@link boundArguments} positional so a leading argument that + * `skip` matches is left out before the arguments are parsed; + * {@link skippedArgument} returns it. Other positionals are returned as + * they are. + */ +export function skipLeadingArgument

( + positional: P, + skip: (arg: string) => boolean, +): P { + const parseAll = + typeof positional === "object" && positional !== null + ? argumentParsers.get(positional as object) + : undefined; + if (!parseAll) { + return positional; + } + const skipping = { ...(positional as object) } as P & object; + argumentParsers.set(skipping, function (this: unknown, values) { + const [first, ...rest] = values; + if (first === undefined || !skip(first)) { + return parseAll.call(this, values); + } + const bound = parseAll.call(this, rest); + skippedArguments.set(bound, first); + return bound; + }); + return skipping; +} + +/** The argument {@link skipLeadingArgument} left out of `value`, if any. */ +export function skippedArgument(value: unknown): string | undefined { + return value instanceof BoundPositional + ? skippedArguments.get(value) + : undefined; +} + /** A Stricli positional definition — the subset that carries the parsers. */ type PositionalDef = | { kind: "tuple"; parameters: readonly { parse?: unknown }[] } @@ -115,15 +244,19 @@ function boundParserAt( } /** - * Give every {@link boundPositional} positional its value. + * Give every {@link boundPositional} positional its value, and parse the + * arguments of a {@link boundArguments} positional into one + * {@link BoundArguments}. * - * 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`. Other positionals are left as they are. + * Stricli already ran the {@link boundPositional} 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`. 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 + * @throws The parser's error for an invalid raw string, or an + * {@link ArgumentParseError} for {@link boundArguments} */ export function applyBoundPositionals( args: readonly unknown[], @@ -134,6 +267,14 @@ export function applyBoundPositionals( if (!def) { return [...args]; } + const parseAll = argumentParsers.get(def); + if (parseAll) { + // Already parsed when a wrapper applies them again + if (args.length === 1 && args[0] instanceof BoundPositional) { + return [...args]; + } + return [parseAll.call(context, args as readonly string[])]; + } const count = def.kind === "tuple" ? Math.max(args.length, def.parameters.length) @@ -165,7 +306,8 @@ export function checkDestructive(args: readonly unknown[]): void { /** * What the user typed for a positional value. * - * @returns `value.raw` for a {@link BoundPositional}, otherwise `value` + * @returns `value.raw` for a {@link BoundPositional} (every argument for + * {@link BoundArguments}), otherwise `value` */ export function rawPositional(value: unknown): unknown { return value instanceof BoundPositional ? value.raw : value; diff --git a/packages/cli/src/lib/trace-target.ts b/packages/cli/src/lib/trace-target.ts index 1801a93e3..ab0efd3b7 100644 --- a/packages/cli/src/lib/trace-target.ts +++ b/packages/cli/src/lib/trace-target.ts @@ -1,28 +1,32 @@ /** * Shared Trace-Target Parsing & Resolution * - * Provides a unified abstraction for commands that accept trace IDs - * with optional org/project context. Trace IDs are globally unique, - * so these formats are all supported: + * Trace commands take their trace ID like every ID-taking command (see + * `id-arguments.ts`): ``, `/`, + * `//`, ` `, or a trace URL. + * This module turns that into a {@link ParsedTraceTarget} and resolves it + * to the org (and project) the command's API needs. * - * - `` — auto-detect org/project from DSN/config - * - `/` — org-scoped (for org-only APIs like trace-logs) - * - `//` — fully explicit - * - * Also handles two-arg forms: - * - `/ ` — target as first arg, trace ID as second - * - ` ` — project-first bare target with org fallback - * - * Used by: span list, span view, trace view, trace logs. + * Used by: span list, span view, trace view, trace logs, log list. */ -import { normalizeSlug, parseOrgProjectArg } from "./arg-parsing.js"; +import { + type ParsedOrgProject, + parseOrgProjectArg, + parseOrgProjectInput, +} from "./arg-parsing.js"; import { ContextError, ValidationError } from "./errors.js"; import { handleRecoveryResult, recoverHexId, resolveRecoveryOrg, } from "./hex-id-recovery.js"; +import { + ID_KINDS, + type IdArguments, + parseIdArgument, + parseIdArguments, +} from "./id-arguments.js"; import { logger } from "./logger.js"; import { guideOrgProjectFailure, @@ -32,9 +36,8 @@ import { resolveOrgOptionalTarget, resolveProjectBoundTarget, } from "./resolve-target.js"; -import { applySentryUrlContext, parseSentryUrl } from "./sentry-url-parser.js"; +import { applySentryUrlContext } from "./sentry-url-parser.js"; import { setOrgProjectContext } from "./telemetry.js"; -import { isTraceId, validateTraceId } from "./trace-id.js"; const log = logger.withTag("trace-target"); @@ -86,6 +89,30 @@ export type ParsedTraceTarget = traceId: string; }; +/** + * A {@link ParsedTraceTarget} parsed without side effects, plus what the + * caller applies when it runs: the server of a pasted Sentry URL (for + * `applySentryUrlContext`) and warnings to log. + */ +export type TraceTargetInput = { + readonly parsed: ParsedTraceTarget; + readonly baseUrl?: string; + readonly warnings: readonly string[]; +}; + +/** Apply what parsing a trace target deferred, and return the target. */ +export function applyTraceTargetInput( + input: TraceTargetInput, +): ParsedTraceTarget { + if (input.baseUrl) { + applySentryUrlContext(input.baseUrl); + } + for (const warning of input.warnings) { + log.warn(warning); + } + return input.parsed; +} + /** Resolved trace target with both org and project. */ export type ResolvedTraceOrgProject = { traceId: string; @@ -106,17 +133,10 @@ export type ResolvedTraceOrg = { /** * Parse trace-related positional arguments into a {@link ParsedTraceTarget}. * - * **Single argument (slash-separated):** - * - `` → auto-detect - * - `/` → org-scoped - * - `//` → explicit - * - * **Two arguments (space-separated):** - * - `/ ` → explicit - * - ` ` → project-first bare target with org fallback - * - * Extra positional arguments beyond the first two are ignored with a - * warning, matching the established pattern across CLI commands. + * Takes the forms every ID-taking command does (see `id-arguments.ts`): + * ``, `/`, `//`, + * ` `, ` `, or a trace URL. Extra + * arguments are ignored with a warning. * * @param args - Positional arguments from CLI * @param usageHint - Usage example for error messages @@ -125,177 +145,62 @@ export type ResolvedTraceOrg = { * @throws {ValidationError} If the trace ID format is invalid */ export function parseTraceTarget( - args: string[], + args: readonly string[], usageHint: string, + options: TraceArgumentsOptions = {}, ): ParsedTraceTarget { - if (args.length === 0) { - throw new ContextError("Trace ID", usageHint, []); - } - - const first = args[0]; - if (first === undefined) { - throw new ContextError("Trace ID", usageHint, []); - } - - // Sentry trace URLs (often copy-pasted with a trailing slash) carry the - // trace ID and org in the path; slash-splitting them yields an empty or - // bogus trace ID segment (CLI-13F). - const urlTarget = traceTargetFromUrl(first); - if (urlTarget) { - return urlTarget; - } - - // Warn about extra positional args that will be ignored - if (args.length > 2) { - log.warn( - `Extra arguments ignored: ${args.slice(2).join(" ")}. Expected: ${usageHint}`, - ); - } - - if (args.length === 1) { - return parseSlashSeparatedTraceTarget(first, usageHint); - } - - // Two+ args: first is target context, second is trace ID - const second = args[1]; - if (second === undefined) { - return parseSlashSeparatedTraceTarget(first, usageHint); - } - - const traceId = validateTraceId(second); - return targetArgToTraceTarget(first, traceId); + return applyTraceTargetInput(parseTraceTargetInput(args, usageHint, options)); } -/** - * Build a trace target from a Sentry trace detail URL. - * - * Returns `null` when the input is not a Sentry URL or the URL has no trace - * ID. Side effect: configures `SENTRY_URL` for self-hosted hosts via - * {@link applySentryUrlContext} (throws `HostScopeError` for untrusted hosts). - * - * @internal Exported for testing - */ -export function traceTargetFromUrl(input: string): ParsedTraceTarget | null { - const urlParsed = parseSentryUrl(input.trim()); - if (!urlParsed?.traceId) { - return null; - } - applySentryUrlContext(urlParsed.baseUrl); - const traceId = validateTraceId(urlParsed.traceId); - if (urlParsed.org) { - return { type: "org-scoped", traceId, org: urlParsed.org }; - } - return { type: "auto-detect", traceId }; -} +/** How a trace command takes its trace ID. */ +export type TraceArgumentsOptions = { + /** + * Whether the command needs a project (`span list`, `span view`): then + * `/` names project `` instead of org ``. + */ + readonly needsProject?: boolean; +}; /** - * Remove trailing `/` characters when the segment before them is a trace ID, - * so `org/project//` splits the same as `org/project/`. - * - * Inputs like `org/project/` are left untouched: the trailing slash there - * means the trace ID was omitted, which must stay a "Trace ID is required" - * error rather than being misread as `org/`. - * - * @internal Exported for testing + * Parse trace-related positional arguments like {@link parseTraceTarget}, + * without side effects: the server of a pasted Sentry URL and the warnings + * are returned for the caller to apply. */ -export function stripTrailingSlashesAfterTraceId(input: string): string { - let end = input.length; - while (end > 0 && input[end - 1] === "/") { - end -= 1; - } - if (end === input.length) { - return input; - } - const stripped = input.slice(0, end); - const tail = stripped.slice(stripped.lastIndexOf("/") + 1); - return isTraceId(tail) ? stripped : input; +export function parseTraceTargetInput( + args: readonly string[], + usageHint: string, + options: TraceArgumentsOptions = {}, +): TraceTargetInput { + const { id, ...rest } = parseIdArgument(args, ID_KINDS.trace, { + usageHint, + ...options, + }); + return traceTargetInput({ ...rest, ids: [id] }); } /** - * Parse a single slash-separated argument into a trace target. - * - * - No slashes → auto-detect (bare trace ID) - * - One slash → `/` (org-scoped) - * - Two+ slashes → split on last `/` → `//` (explicit) - * - * @internal Exported for testing + * Turn {@link IdArguments} with one trace ID into a trace target. The target + * parses like any org/project target. */ -export function parseSlashSeparatedTraceTarget( - rawInput: string, - usageHint: string, -): ParsedTraceTarget { - const input = stripTrailingSlashesAfterTraceId(rawInput); - const lastSlash = input.lastIndexOf("/"); - - if (lastSlash === -1) { - // No slashes — bare trace ID - return { type: "auto-detect", traceId: validateTraceId(input) }; - } - - const prefix = input.slice(0, lastSlash); - const rawTraceId = input.slice(lastSlash + 1); - - if (!rawTraceId) { - throw new ContextError("Trace ID", usageHint, []); - } - - const traceId = validateTraceId(rawTraceId); - - if (!prefix) { - // "/" — leading slash, treat as auto-detect - return { type: "auto-detect", traceId }; - } - - const innerSlash = prefix.indexOf("/"); - - if (innerSlash === -1) { - // "org/" — org-scoped - const ns = normalizeSlug(prefix); - return { - type: "org-scoped", - traceId, - org: ns.slug, - ...(ns.normalized && { normalized: true }), - }; - } - - // "org/project/" — explicit - const rawOrg = prefix.slice(0, innerSlash); - const rawProject = prefix.slice(innerSlash + 1); - - if (!(rawOrg && rawProject)) { - throw new ContextError("Trace ID", usageHint, []); - } - - const no = normalizeSlug(rawOrg); - const np = normalizeSlug(rawProject); - const normalized = no.normalized || np.normalized; - +function traceTargetInput({ + target, + ids, + baseUrl, + warnings, +}: IdArguments): TraceTargetInput { + const traceId = ids[0] ?? ""; + const input = parseOrgProjectInput(target); return { - type: "explicit", - traceId, - org: no.slug, - project: np.slug, - ...(normalized && { normalized: true }), + parsed: orgProjectToTraceTarget(input.parsed, traceId), + baseUrl: baseUrl ?? input.baseUrl, + warnings, }; } -/** - * Convert a target argument + validated trace ID into a trace target. - * Delegates org/project parsing to {@link parseOrgProjectArg}. - * - * Note: `parseOrgProjectArg` already emits slug normalization warnings - * internally, so `normalized` is NOT propagated here to avoid double - * warnings when callers also check via {@link warnIfNormalized}. - * - * @internal Exported for testing - */ -export function targetArgToTraceTarget( - targetArg: string, +function orgProjectToTraceTarget( + parsed: ParsedOrgProject, traceId: string, ): ParsedTraceTarget { - const parsed = parseOrgProjectArg(targetArg); - switch (parsed.type) { case "explicit": return { @@ -335,52 +240,44 @@ export function targetArgToTraceTarget( } /** - * Extract the raw trace ID slot from a positional-arg invocation **without - * validating** it. Returns `{ rawTraceId, targetArg? }` where: - * - * - Single arg → the part after the last `/` is the trace ID candidate, - * and anything before the last `/` is the target (org/project[/]). - * - Two+ args → the second arg is the raw trace ID, the first is the target. + * Trace arguments parsed like {@link parseTraceTargetInput}, but with the + * trace ID as typed: {@link resolveTraceArguments} validates it, or + * recovers a malformed one, when the command runs. + */ +export type TraceArgumentsInput = TraceTargetInput & { + /** The target argument as typed, for the recovery lookup */ + readonly targetArg?: string; +}; + +/** + * Parse trace arguments without side effects and without validating the + * trace ID, so a malformed one can be recovered once the command runs. * - * Used by {@link parseTraceTargetWithRecovery} to peek at the raw input - * before `parseTraceTarget` runs its strict validation. Returns null when - * there isn't a usable trace ID slot (e.g., zero args). + * @throws {ContextError} When the trace ID is missing + * @throws {ValidationError} When the target is invalid */ -export function extractRawTraceId( - args: string[], -): { rawTraceId: string; targetArg?: string } | null { - if (args.length === 0) { - return null; - } - if (args.length === 1) { - const rawFirst = args[0]; - if (!rawFirst) { - return null; - } - const first = stripTrailingSlashesAfterTraceId(rawFirst); - const lastSlash = first.lastIndexOf("/"); - if (lastSlash === -1) { - return { rawTraceId: first }; - } - const rawTraceId = first.slice(lastSlash + 1); - const prefix = first.slice(0, lastSlash); - return rawTraceId - ? { rawTraceId, targetArg: prefix || undefined } - : { rawTraceId: "" }; - } - const second = args[1]; - const first = args[0]; - if (second === undefined) { - return null; - } - return { rawTraceId: second, targetArg: first }; +export function parseTraceArgumentsInput( + args: readonly string[], + usageHint: string, + options: TraceArgumentsOptions = {}, +): TraceArgumentsInput { + const { id, ...rest } = parseIdArgument(args, ID_KINDS.trace, { + usageHint, + ...options, + validate: false, + }); + return { + ...traceTargetInput({ ...rest, ids: [id] }), + targetArg: rest.target, + }; } /** - * Parse trace-target args, falling back to {@link recoverHexId} on an - * invalid trace ID. Returns a {@link ParsedTraceTarget} with a validated - * or recovered trace ID, or throws the original {@link ValidationError} - * when recovery can't proceed. + * Apply what {@link parseTraceArgumentsInput} deferred and validate the + * trace ID, falling back to {@link recoverHexId} on an invalid one. + * Returns a {@link ParsedTraceTarget} with a validated or recovered trace + * ID, or throws the original {@link ValidationError} when recovery can't + * proceed. * * The recovery path runs the cheap classifications (sentinel, slug, etc.) * locally regardless of context. Fuzzy prefix lookup additionally requires @@ -388,32 +285,41 @@ export function extractRawTraceId( * NOT attempt DSN/config auto-detection during recovery (it's expensive * and the adapter returns empty without a project anyway). */ -export async function parseTraceTargetWithRecovery( - args: string[], - usageHint: string, +export async function resolveTraceArguments( + input: TraceArgumentsInput, ): Promise { + const parsed = applyTraceTargetInput(input); try { - return parseTraceTarget(args, usageHint); + return { ...parsed, traceId: ID_KINDS.trace.validate(parsed.traceId) }; } catch (err) { if (!(err instanceof ValidationError)) { throw err; } - const raw = extractRawTraceId(args); - if (!raw?.rawTraceId) { - throw err; - } - const ctx = await recoveryContextFromTargetArg(raw.targetArg); - const result = await recoverHexId(raw.rawTraceId, "trace", ctx); - const recoveredTraceId = handleRecoveryResult(result, err, { - entityType: "trace", - canonicalCommand: `sentry trace view ${ctx.org || ""}/${ctx.project ?? ""}/`, - logTag: "trace-target", - }); - const newArgs = substituteTraceId(args, recoveredTraceId); - return parseTraceTarget(newArgs, usageHint); + const traceId = await recoverTraceId(parsed.traceId, input.targetArg, err); + return { ...parsed, traceId }; } } +/** + * Recover a malformed trace ID with {@link recoverHexId}, in the context of + * the target argument, if any. + * + * @throws The original `failure` when recovery finds nothing + */ +async function recoverTraceId( + rawTraceId: string, + targetArg: string | undefined, + failure: ValidationError, +): Promise { + const ctx = await recoveryContextFromTargetArg(targetArg); + const result = await recoverHexId(rawTraceId, "trace", ctx); + return handleRecoveryResult(result, failure, { + entityType: "trace", + canonicalCommand: `sentry trace view ${ctx.org || ""}/${ctx.project ?? ""}/`, + logTag: "trace-target", + }); +} + /** * Best-effort recovery context derived from a raw target arg. * @@ -444,26 +350,6 @@ async function recoveryContextFromTargetArg( ); } -/** - * Rebuild the positional args with the recovered trace ID in the correct - * slot. Mirrors the logic in {@link extractRawTraceId}. - */ -function substituteTraceId(args: string[], recoveredTraceId: string): string[] { - if (args.length === 1) { - const first = args[0]; - if (!first) { - return [recoveredTraceId]; - } - const lastSlash = first.lastIndexOf("/"); - if (lastSlash === -1) { - return [recoveredTraceId]; - } - const prefix = first.slice(0, lastSlash); - return [`${prefix}/${recoveredTraceId}`]; - } - return [args[0] ?? "", recoveredTraceId, ...args.slice(2)]; -} - /** * Emit a slug normalization warning if applicable. * Shared by all trace commands to avoid duplicating the check. @@ -666,63 +552,49 @@ export type ParsedDualModeArgs = /** * Disambiguate positional arguments for dual-mode list commands. * - * Detects trace mode by checking whether any argument segment looks like - * a 32-char hex trace ID via {@link isTraceId}: - * - * - **No args**: project mode (auto-detect org/project) - * - **Two+ args**: checks the last positional. If it's a trace ID → trace - * mode (space-separated form like ` `). - * - **Single arg**: checks the tail segment (last part after `/`). If it - * looks like a trace ID → trace mode. Otherwise → project target. - * - * When trace mode is detected, delegates to {@link parseTraceTarget} for - * full parsing and validation. + * Trace mode when the arguments name a trace ID in any of the forms + * {@link parseTraceTarget} takes; otherwise project mode, with the first + * argument as the target. * * @param args - Positional arguments from CLI * @param traceUsageHint - Usage hint for trace-mode error messages * @returns Parsed args with mode discrimination */ export function parseDualModeArgs( - args: string[], + args: readonly string[], traceUsageHint: string, + options: TraceArgumentsOptions = {}, ): ParsedDualModeArgs { - if (args.length === 0) { - return { mode: "project" }; - } - - const first = args[0]; - if (first === undefined) { - return { mode: "project" }; - } - - // Two+ args: check if the last arg is a trace ID (space-separated form) - if (args.length >= 2) { - const last = args.at(-1); - if (last && isTraceId(last)) { - return { - mode: "trace", - parsed: parseTraceTarget(args, traceUsageHint), - }; - } - } - - // Trace detail URL → trace mode (the URL itself carries the trace ID) - const urlTarget = traceTargetFromUrl(first); - if (urlTarget) { - return { mode: "trace", parsed: urlTarget }; - } + const input = parseDualModeInput(args, traceUsageHint, options); + return input.mode === "trace" + ? { mode: "trace", parsed: applyTraceTargetInput(input.trace) } + : input; +} - // Single arg: check the tail segment (last part after "/", or entire arg) - const stripped = stripTrailingSlashesAfterTraceId(first); - const lastSlash = stripped.lastIndexOf("/"); - const tail = lastSlash === -1 ? stripped : stripped.slice(lastSlash + 1); - if (isTraceId(tail)) { - return { - mode: "trace", - parsed: parseTraceTarget(args, traceUsageHint), - }; - } +/** {@link ParsedDualModeArgs} parsed without side effects. */ +export type DualModeInput = + | { mode: "project"; target?: string } + | { mode: "trace"; trace: TraceTargetInput }; - // Not a trace ID → treat as project target - return { mode: "project", target: first }; +/** + * Disambiguate dual-mode arguments like {@link parseDualModeArgs}, without + * side effects: trace mode returns a {@link TraceTargetInput} for the caller + * to apply. + */ +export function parseDualModeInput( + args: readonly string[], + traceUsageHint: string, + options: TraceArgumentsOptions = {}, +): DualModeInput { + const input = parseIdArguments(args, ID_KINDS.trace, { + ...options, + usageHint: traceUsageHint, + optional: true, + }); + if (input.ids.length > 0) { + return { mode: "trace", trace: traceTargetInput(input) }; + } + return input.target === undefined + ? { mode: "project" } + : { mode: "project", target: input.target }; } diff --git a/packages/cli/test/commands/log/list.test.ts b/packages/cli/test/commands/log/list.test.ts index edafd7370..0a4ff21e5 100644 --- a/packages/cli/test/commands/log/list.test.ts +++ b/packages/cli/test/commands/log/list.test.ts @@ -348,10 +348,7 @@ describe("listCommand.func — standard mode", () => { beforeEach(() => { listLogsSpy = vi.spyOn(apiClient, "listLogs"); - resolveOrgProjectSpy = vi.spyOn( - resolveTarget, - "resolveProjectBoundFromArg", - ); + resolveOrgProjectSpy = vi.spyOn(resolveTarget, "resolveProjectBoundTarget"); vi.spyOn(resolveTarget, "resolveLogProjectId").mockResolvedValue(undefined); withProgressSpy = vi .spyOn(polling, "withProgress") @@ -760,7 +757,7 @@ describe("listCommand.func — positional disambiguation", () => { .spyOn(apiClient, "listTraceLogs") .mockResolvedValue([]); resolveOrgProjectSpy = vi - .spyOn(resolveTarget, "resolveProjectBoundFromArg") + .spyOn(resolveTarget, "resolveProjectBoundTarget") .mockResolvedValue({ org: ORG, project: PROJECT }); resolveTraceOrgSpy = vi .spyOn(traceTarget, "resolveTraceOrg") @@ -1003,7 +1000,7 @@ describe("listCommand.func — flag validation", () => { // downstream resolution, not flag validation. Mock resolution to reject // with a non-ValidationError so we can verify flag validation passed. const resolveOrgProjectSpy = vi - .spyOn(resolveTarget, "resolveProjectBoundFromArg") + .spyOn(resolveTarget, "resolveProjectBoundTarget") .mockRejectedValueOnce( new ContextError("Organization", "sentry log list"), ); @@ -1035,10 +1032,7 @@ describe("listCommand.func — follow mode (standard)", () => { beforeEach(() => { sigint = interceptSigint(); listLogsSpy = vi.spyOn(apiClient, "listLogs"); - resolveOrgProjectSpy = vi.spyOn( - resolveTarget, - "resolveProjectBoundFromArg", - ); + resolveOrgProjectSpy = vi.spyOn(resolveTarget, "resolveProjectBoundTarget"); vi.spyOn(resolveTarget, "resolveLogProjectId").mockResolvedValue(undefined); isPlainSpy = vi.spyOn(formatters, "isPlainOutput").mockReturnValue(true); updateNotifSpy = vi diff --git a/packages/cli/test/commands/log/view.property.test.ts b/packages/cli/test/commands/log/view.property.test.ts index 2488c9cab..e4a44abb1 100644 --- a/packages/cli/test/commands/log/view.property.test.ts +++ b/packages/cli/test/commands/log/view.property.test.ts @@ -15,6 +15,7 @@ import { import { describe, expect, test } from "vitest"; import { parsePositionalArgs } from "../../../src/commands/log/view.js"; import { ContextError } from "../../../src/lib/errors.js"; +import { looksLikeHexIdFragment } from "../../../src/lib/hex-id-recovery.js"; import { DEFAULT_NUM_RUNS } from "../../model-based/helpers.js"; /** Valid log IDs (32-char hex) */ @@ -23,6 +24,16 @@ const logIdArb = stringMatching(/^[a-f0-9]{32}$/); /** Valid org/project slugs */ const slugArb = stringMatching(/^[a-z][a-z0-9-]{1,20}[a-z0-9]$/); +/** + * Project slugs that cannot be read as a partial log ID. `/` is + * parsed as an ID to recover when `x` has enough hex for a fuzzy lookup + * (e.g. `aa000a00`), so only these slugs make `/` a target. + */ +const projectSlugArb = slugArb.filter((s) => !looksLikeHexIdFragment(s)); + +/** Partial log IDs: a hex prefix long enough for a fuzzy lookup */ +const partialLogIdArb = stringMatching(/^[a-f0-9]{8,31}$/); + describe("parsePositionalArgs properties", () => { test("single valid log ID: returns it in rawLogIds with undefined targetArg", async () => { fcAssert( @@ -49,7 +60,7 @@ describe("parsePositionalArgs properties", () => { test("single arg with one slash: throws ContextError (missing log ID)", async () => { fcAssert( - property(tuple(slugArb, slugArb), ([org, project]) => { + property(tuple(slugArb, projectSlugArb), ([org, project]) => { expect(() => parsePositionalArgs([`${org}/${project}`])).toThrow( ContextError, ); @@ -58,6 +69,17 @@ describe("parsePositionalArgs properties", () => { ); }); + test("single arg org/partial-id: org is targetArg, partial ID is in rawLogIds", async () => { + fcAssert( + property(tuple(slugArb, partialLogIdArb), ([org, partialId]) => { + const result = parsePositionalArgs([`${org}/${partialId}`]); + expect(result.targetArg).toBe(org); + expect(result.rawLogIds).toEqual([partialId]); + }), + { numRuns: DEFAULT_NUM_RUNS }, + ); + }); + test("two args (target + logId): first is targetArg, second is in rawLogIds", async () => { fcAssert( property(tuple(slugArb, logIdArb), ([slug, logId]) => { diff --git a/packages/cli/test/commands/log/view.test.ts b/packages/cli/test/commands/log/view.test.ts index be524cbc5..502c34aae 100644 --- a/packages/cli/test/commands/log/view.test.ts +++ b/packages/cli/test/commands/log/view.test.ts @@ -279,7 +279,7 @@ describe("parsePositionalArgs", () => { // Auto-swap: org/project becomes target, hex ID becomes the log ID expect(result.targetArg).toBe("my-org/my-project"); expect(result.rawLogIds).toEqual(["968c763c740cfda8b6728f27fb9e9b01"]); - expect(result.suggestion).toContain("reversed"); + expect(result.warnings).toEqual([expect.stringContaining("reversed")]); }); test("returns suggestion when first arg looks like issue short ID", () => { diff --git a/packages/cli/test/commands/replay/download.test.ts b/packages/cli/test/commands/replay/download.test.ts index 6d9d6f492..0763e2553 100644 --- a/packages/cli/test/commands/replay/download.test.ts +++ b/packages/cli/test/commands/replay/download.test.ts @@ -8,6 +8,7 @@ import { mkdtemp, readFile, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { ArgumentParseError } from "@stricli/core"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { downloadCommand } from "../../../src/commands/replay/download.js"; @@ -112,7 +113,7 @@ describe("replay download", () => { vi.clearAllMocks(); tmpDir = await mkdtemp(join(tmpdir(), "replay-dl-")); resolveTargetSpy = vi - .spyOn(resolveTarget, "resolveOrgOptionalFromArg") + .spyOn(resolveTarget, "resolveOrgOptionalTarget") .mockResolvedValue({ org: "test-org" }); resolveReplaySpy = vi .spyOn(apiClient, "resolveReplay") @@ -189,7 +190,7 @@ describe("replay download", () => { ); expect(resolveTargetSpy).toHaveBeenCalledWith( - "url-org/", + { type: "org-all", org: "url-org" }, tmpDir, "replay download", ); @@ -265,7 +266,12 @@ describe("replay download", () => { .call(createContext().context, { fresh: false }) .catch((caught: unknown) => caught); - expect(error).toBeInstanceOf(ContextError); - expect((error as ContextError).message).toContain("sentry replay download"); + // An argument error, as the CLI reports it, wrapping the parser's error + expect(error).toBeInstanceOf(ArgumentParseError); + const { exception } = error as ArgumentParseError; + expect(exception).toBeInstanceOf(ContextError); + expect((exception as ContextError).message).toContain( + "sentry replay download", + ); }); }); diff --git a/packages/cli/test/commands/replay/view.test.ts b/packages/cli/test/commands/replay/view.test.ts index 38bd70435..dc7c87db5 100644 --- a/packages/cli/test/commands/replay/view.test.ts +++ b/packages/cli/test/commands/replay/view.test.ts @@ -2,9 +2,12 @@ * Replay View Command Tests */ +import type { ArgumentParseError } from "@stricli/core"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { - parsePositionalArgs, + parseReplayArguments, + type ReplayArguments, + replayArguments, viewCommand, } from "../../../src/commands/replay/view.js"; @@ -38,6 +41,7 @@ import * as browser from "../../../src/lib/browser.js"; import { ApiError, ContextError, + HostScopeError, ResolutionError, ValidationError, } from "../../../src/lib/errors.js"; @@ -53,6 +57,7 @@ vi.mock("../../../src/lib/resolve-target.js", async (importOriginal) => { ); }); +import { applyBoundPositionals } from "../../../src/lib/positional-parse.js"; // oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking import * as resolveTarget from "../../../src/lib/resolve-target.js"; import type { ReplayDetails } from "../../../src/types/index.js"; @@ -79,64 +84,116 @@ function sampleReplay(overrides: Partial = {}): ReplayDetails { }; } -describe("parsePositionalArgs", () => { +describe("parseReplayArguments", () => { + const ORG_ALL = { type: "org-all", org: "test-org" }; + const EXPLICIT = { type: "explicit", org: "test-org", project: "cli" }; + test("parses replay ID only", () => { - const result = parsePositionalArgs([REPLAY_ID]); + const result = parseReplayArguments([REPLAY_ID]); expect(result.replayId).toBe(REPLAY_ID); - expect(result.targetArg).toBeUndefined(); + expect(result.target).toEqual({ type: "auto-detect" }); }); test("parses org/replay-id shorthand", () => { - const result = parsePositionalArgs([`test-org/${REPLAY_ID}`]); + const result = parseReplayArguments([`test-org/${REPLAY_ID}`]); expect(result.replayId).toBe(REPLAY_ID); - expect(result.targetArg).toBe("test-org/"); + expect(result.target).toEqual(ORG_ALL); }); test("normalizes dashed org/replay-id shorthand", () => { - const result = parsePositionalArgs([ + const result = parseReplayArguments([ "test-org/346789a7-03f6-4543-84f1-de473b8b9fcc", ]); expect(result.replayId).toBe(REPLAY_ID); - expect(result.targetArg).toBe("test-org/"); + expect(result.target).toEqual(ORG_ALL); }); test("parses org/project/replay-id form", () => { - const result = parsePositionalArgs([`test-org/cli/${REPLAY_ID}`]); + const result = parseReplayArguments([`test-org/cli/${REPLAY_ID}`]); + expect(result.replayId).toBe(REPLAY_ID); + expect(result.target).toEqual(EXPLICIT); + }); + + test("ignores a slash pasted after the replay ID", () => { + const result = parseReplayArguments([`test-org/cli/${REPLAY_ID}/`]); expect(result.replayId).toBe(REPLAY_ID); - expect(result.targetArg).toBe("test-org/cli"); + expect(result.target).toEqual(EXPLICIT); + }); + + test("parses target and replay ID as two arguments", () => { + const result = parseReplayArguments(["test-org/cli", REPLAY_ID]); + expect(result.replayId).toBe(REPLAY_ID); + expect(result.target).toEqual(EXPLICIT); + expect(result.warnings).toEqual([]); }); test("parses replay URL", () => { - const result = parsePositionalArgs([ + const result = parseReplayArguments([ `https://sentry.io/organizations/test-org/explore/replays/${REPLAY_ID}/`, ]); expect(result.replayId).toBe(REPLAY_ID); - expect(result.targetArg).toBe("test-org/"); + expect(result.target).toEqual(ORG_ALL); + expect(result.baseUrl).toBe("https://sentry.io"); }); test("parses legacy replay URL", () => { - const result = parsePositionalArgs([ + const result = parseReplayArguments([ `https://test-org.sentry.io/replays/${REPLAY_ID}/`, ]); expect(result.replayId).toBe(REPLAY_ID); - expect(result.targetArg).toBe("test-org/"); + expect(result.target).toEqual(ORG_ALL); + }); + + test("throws ContextError for a Sentry URL without a replay ID", () => { + expect(() => + parseReplayArguments(["https://sentry.io/organizations/test-org/"]), + ).toThrow(ContextError); }); test("detects swapped args", () => { - const result = parsePositionalArgs([REPLAY_ID, "test-org/cli"]); + const result = parseReplayArguments([REPLAY_ID, "test-org/cli"]); + expect(result.replayId).toBe(REPLAY_ID); + expect(result.target).toEqual(EXPLICIT); + expect(result.warnings).toEqual([ + `Arguments appear reversed. Interpreting as: test-org/cli ${REPLAY_ID}`, + ]); + }); + + test("throws ContextError without arguments", () => { + expect(() => parseReplayArguments([])).toThrow(ContextError); + expect(() => parseReplayArguments([" "])).toThrow(ContextError); + expect(() => parseReplayArguments(["test-org/cli", ""])).toThrow( + ContextError, + ); + }); + + test("an empty target before the replay ID auto-detects", () => { + // e.g. `sentry replay view "$ORG" ` with `$ORG` unset + const result = parseReplayArguments(["", REPLAY_ID]); expect(result.replayId).toBe(REPLAY_ID); - expect(result.targetArg).toBe("test-org/cli"); - expect(result.warning).toContain("reversed"); + expect(result.target).toEqual({ type: "auto-detect" }); }); test("throws ContextError for org/project with no replay ID", () => { - expect(() => parsePositionalArgs(["test-org/cli"])).toThrow(ContextError); + expect(() => parseReplayArguments(["test-org/cli"])).toThrow(ContextError); + expect(() => parseReplayArguments(["test-org/cli/"])).toThrow(ContextError); }); - test("throws ValidationError for extra positional args", () => { - expect(() => - parsePositionalArgs(["test-org/cli", REPLAY_ID, "extra-arg"]), - ).toThrow(ValidationError); + test("throws ValidationError for an invalid replay ID", () => { + expect(() => parseReplayArguments(["not-hex"])).toThrow(ValidationError); + expect(() => parseReplayArguments(["test-org/cli", "not-hex"])).toThrow( + ValidationError, + ); + }); + + test("ignores extra positional args with a warning", () => { + expect( + parseReplayArguments(["test-org/cli", REPLAY_ID, "extra-arg"]), + ).toMatchObject({ + replayId: REPLAY_ID, + target: { type: "explicit", org: "test-org", project: "cli" }, + warnings: [expect.stringContaining("Extra arguments ignored: extra-arg")], + }); }); }); @@ -197,7 +254,7 @@ describe("viewCommand.func", () => { }, ], }); - resolveTargetSpy = vi.spyOn(resolveTarget, "resolveOrgOptionalFromArg"); + resolveTargetSpy = vi.spyOn(resolveTarget, "resolveOrgOptionalTarget"); openInBrowserSpy = vi.spyOn(browser, "openInBrowser").mockResolvedValue(); }); @@ -408,3 +465,41 @@ describe("viewCommand.func", () => { expect(output).toContain("5s"); }); }); + +describe("replayArguments", () => { + const positional = replayArguments("view", "sentry replay view "); + const context = { cwd: "/tmp" }; + const parse = (raw: string[]) => + applyBoundPositionals(raw, positional, context)[0] as ReplayArguments; + + test("parses the replay ID and target without resolving", () => { + const target = parse(["test-org/cli", REPLAY_ID]); + expect(target.parsed).toEqual({ + replayId: REPLAY_ID, + target: { type: "explicit", org: "test-org", project: "cli" }, + baseUrl: undefined, + warnings: [], + }); + }); + + test("rejects an invalid replay ID when parsing", () => { + let error: unknown; + try { + parse(["not-hex"]); + } catch (caught) { + error = caught; + } + expect((error as ArgumentParseError).exception).toBeInstanceOf( + ValidationError, + ); + }); + + test("applies a pasted URL's server when resolving, not when parsing", async () => { + const target = parse([ + `https://sentry.example.com/organizations/acme/explore/replays/${REPLAY_ID}/`, + ]); + expect(target.parsed.baseUrl).toBe("https://sentry.example.com"); + // No account for this server: applying it throws HostScopeError + await expect(target.resolved).rejects.toThrow(HostScopeError); + }); +}); diff --git a/packages/cli/test/commands/span/view.test.ts b/packages/cli/test/commands/span/view.test.ts index f36d7d7bb..87bbf8dd8 100644 --- a/packages/cli/test/commands/span/view.test.ts +++ b/packages/cli/test/commands/span/view.test.ts @@ -5,9 +5,10 @@ * and output formatting in src/commands/span/view.ts. */ +import { ArgumentParseError } from "@stricli/core"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { - parsePositionalArgs, + parseSpanViewInput, viewCommand, } from "../../../src/commands/span/view.js"; @@ -46,6 +47,7 @@ vi.mock("../../../src/lib/resolve-target.js", async (importOriginal) => { // oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking import * as resolveTarget from "../../../src/lib/resolve-target.js"; +import { parseTraceArgumentsInput } from "../../../src/lib/trace-target.js"; const VALID_TRACE_ID = "aaaa1111bbbb2222cccc3333dddd4444"; const VALID_SPAN_ID = "a1b2c3d4e5f67890"; @@ -92,278 +94,133 @@ describe("validateSpanId", () => { }); }); -describe("parsePositionalArgs", () => { - describe("trace-id + single span-id", () => { - test("returns deferred form for two-arg input (trace-ID recovery runs in command)", () => { - const result = parsePositionalArgs([VALID_TRACE_ID, VALID_SPAN_ID]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe(VALID_TRACE_ID); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID]); - }); - }); +describe("parseSpanViewInput", () => { + /** The trace of `traceArg`, as `span view` parses it */ + const trace = (traceArg: string) => + parseTraceArgumentsInput([traceArg], "", { needsProject: true }); + const SLASH_FORM = `my-org/my-project/${VALID_TRACE_ID}`; - describe("trace-id + multiple span-ids", () => { - test("passes multiple span IDs through as raw", () => { - const result = parsePositionalArgs([ - VALID_TRACE_ID, - VALID_SPAN_ID, - VALID_SPAN_ID_2, - ]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe(VALID_TRACE_ID); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID, VALID_SPAN_ID_2]); + test.each([ + [[VALID_TRACE_ID, VALID_SPAN_ID], VALID_TRACE_ID, [VALID_SPAN_ID]], + [ + [VALID_TRACE_ID, VALID_SPAN_ID, VALID_SPAN_ID_2], + VALID_TRACE_ID, + [VALID_SPAN_ID, VALID_SPAN_ID_2], + ], + [[SLASH_FORM, VALID_SPAN_ID], SLASH_FORM, [VALID_SPAN_ID]], + [ + [SLASH_FORM, VALID_SPAN_ID, VALID_SPAN_ID_2], + SLASH_FORM, + [VALID_SPAN_ID, VALID_SPAN_ID_2], + ], + [ + [`my-org/${VALID_TRACE_ID}`, VALID_SPAN_ID], + `my-org/${VALID_TRACE_ID}`, + [VALID_SPAN_ID], + ], + // `/`, as span list prints it + [[`${VALID_TRACE_ID}/${VALID_SPAN_ID}`], VALID_TRACE_ID, [VALID_SPAN_ID]], + [ + [`${SLASH_FORM}/${VALID_SPAN_ID}`, VALID_SPAN_ID_2], + SLASH_FORM, + [VALID_SPAN_ID, VALID_SPAN_ID_2], + ], + // Several IDs in one argument (CLI-1BD) + [[`${VALID_TRACE_ID} ${VALID_SPAN_ID}`], VALID_TRACE_ID, [VALID_SPAN_ID]], + [[`${SLASH_FORM} ${VALID_SPAN_ID}`], SLASH_FORM, [VALID_SPAN_ID]], + [ + [`${VALID_TRACE_ID}\n${VALID_SPAN_ID}\n${VALID_SPAN_ID_2}\n`], + VALID_TRACE_ID, + [VALID_SPAN_ID, VALID_SPAN_ID_2], + ], + [ + [`${VALID_TRACE_ID}/${VALID_SPAN_ID}\n${VALID_SPAN_ID_2}`], + VALID_TRACE_ID, + [VALID_SPAN_ID, VALID_SPAN_ID_2], + ], + ])("%j → trace %j, spans %j", (args, traceArg, rawSpanIds) => { + expect(parseSpanViewInput(args)).toEqual({ + trace: trace(traceArg), + rawSpanIds, }); }); - describe("org/project/trace-id + span-id", () => { - test("slash-separated target is deferred intact", () => { - const slashForm = `my-org/my-project/${VALID_TRACE_ID}`; - const result = parsePositionalArgs([slashForm, VALID_SPAN_ID]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe(slashForm); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID]); - }); - - test("slash-separated target with multiple span IDs", () => { - const slashForm = `my-org/my-project/${VALID_TRACE_ID}`; - const result = parsePositionalArgs([ - slashForm, - VALID_SPAN_ID, - VALID_SPAN_ID_2, - ]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe(slashForm); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID, VALID_SPAN_ID_2]); + test("keeps the case of IDs for the command to normalize", () => { + expect( + parseSpanViewInput(["AAAA1111BBBB2222CCCC3333DDDD4444/A1B2C3D4E5F67890"]), + ).toEqual({ + trace: trace("AAAA1111BBBB2222CCCC3333DDDD4444"), + rawSpanIds: ["A1B2C3D4E5F67890"], }); }); - describe("auto-split traceId/spanId single-arg format", () => { - test("auto-splits traceId/spanId single-arg format (resolved path)", () => { - const result = parsePositionalArgs([ - "aaaa1111bbbb2222cccc3333dddd4444/a1b2c3d4e5f67890", - ]); - expect(result.kind).toBe("resolved"); - if (result.kind !== "resolved") throw new Error("unreachable"); - expect(result.traceTarget.traceId).toBe( - "aaaa1111bbbb2222cccc3333dddd4444", - ); - expect(result.traceTarget.type).toBe("auto-detect"); - expect(result.rawSpanIds).toEqual(["a1b2c3d4e5f67890"]); - }); - - test("auto-splits with uppercase hex IDs", () => { - const result = parsePositionalArgs([ - "AAAA1111BBBB2222CCCC3333DDDD4444/A1B2C3D4E5F67890", - ]); - expect(result.kind).toBe("resolved"); - if (result.kind !== "resolved") throw new Error("unreachable"); - expect(result.traceTarget.traceId).toBe( - "aaaa1111bbbb2222cccc3333dddd4444", - ); - expect(result.rawSpanIds).toEqual(["a1b2c3d4e5f67890"]); + test("leaves malformed IDs raw, for recovery at the command layer", () => { + expect(parseSpanViewInput(["not-valid", VALID_SPAN_ID])).toEqual({ + trace: trace("not-valid"), + rawSpanIds: [VALID_SPAN_ID], }); - - test("does not auto-split org/traceId format (two args) — defers to command", () => { - // org/traceId has a non-hex org slug, so it shouldn't trigger the auto-split - const result = parsePositionalArgs([ - "my-org/aaaa1111bbbb2222cccc3333dddd4444", - "a1b2c3d4e5f67890", - ]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe( - "my-org/aaaa1111bbbb2222cccc3333dddd4444", - ); - expect(result.rawSpanIds).toEqual(["a1b2c3d4e5f67890"]); + expect(parseSpanViewInput([`not-a-hex-id/${VALID_SPAN_ID}`])).toEqual({ + trace: trace("not-a-hex-id"), + rawSpanIds: [VALID_SPAN_ID], }); - - test("does not auto-split when left is not a valid trace ID", () => { - // Auto-split requires hex-on-both-sides. When left is non-hex, - // the single-arg input falls through to the "missing span IDs" - // branch and throws ContextError. (A two-arg variant would defer - // to the command layer — covered by other tests.) - expect(() => - parsePositionalArgs(["not-a-hex-id/a1b2c3d4e5f67890"]), - ).toThrow(ContextError); + expect( + parseSpanViewInput([`my-org/my-project/not-a-trace/${VALID_SPAN_ID}`]), + ).toEqual({ + trace: trace("my-org/my-project/not-a-trace"), + rawSpanIds: [VALID_SPAN_ID], }); - - test("does not auto-split when right is not a valid span ID", () => { - // Right side 32-char hex is a trace ID (not a span ID) so the - // auto-split heuristic rejects it. As a single-arg form there are - // no span IDs in a second positional → ContextError. - expect(() => - parsePositionalArgs([ - "aaaa1111bbbb2222cccc3333dddd4444/bbbb2222cccc3333dddd4444eeee5555", - ]), - ).toThrow(ContextError); - }); - - test("does not auto-split with multiple slashes (defers to command)", () => { - // org/project/traceId format — deferred to the command layer - // which calls parseTraceTargetWithRecovery. - const slashForm = `my-org/my-project/${VALID_TRACE_ID}`; - const result = parsePositionalArgs([slashForm, VALID_SPAN_ID]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe(slashForm); - }); - - test("org/project/spanId (missing trace ID) throws a trace-focused ContextError (CLI-1GP)", () => { - try { - parsePositionalArgs([`my-org/my-project/${VALID_SPAN_ID}`]); - expect.unreachable("Should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ContextError); - const msg = (error as ContextError).message; - // Points at the missing trace ID, not a missing span ID. - expect(msg).toContain("trace ID"); - expect(msg).toContain(VALID_SPAN_ID); - // Does not misdirect the user to `span list` with the span ID. - expect(msg).not.toContain( - `span list my-org/my-project/${VALID_SPAN_ID}`, - ); - } - }); - - test("org/project/badTrace/spanId surfaces the trace validation detail", () => { - try { - parsePositionalArgs([`my-org/my-project/not-a-trace/${VALID_SPAN_ID}`]); - expect.unreachable("Should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ContextError); - const msg = (error as ContextError).message; - expect(msg).toContain("trace ID"); - } + expect(parseSpanViewInput([VALID_TRACE_ID, "not-a-span-id"])).toEqual({ + trace: trace(VALID_TRACE_ID), + rawSpanIds: ["not-a-span-id"], }); + expect(parseSpanViewInput([VALID_TRACE_ID, "abcd1234"]).rawSpanIds).toEqual( + ["abcd1234"], + ); }); - describe("whitespace-separated single-arg format (CLI-1BD)", () => { - test("splits ' ' passed as one argument", () => { - const result = parsePositionalArgs([ - `${VALID_TRACE_ID} ${VALID_SPAN_ID}`, - ]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe(VALID_TRACE_ID); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID]); - }); - - test("splits 'org/project/ ' passed as one argument", () => { - const slashForm = `my-org/my-project/${VALID_TRACE_ID}`; - const result = parsePositionalArgs([`${slashForm} ${VALID_SPAN_ID}`]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe(slashForm); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID]); - }); - - test("splits newline-separated span IDs after the trace ID", () => { - const result = parsePositionalArgs([ - `${VALID_TRACE_ID}\n${VALID_SPAN_ID}\n${VALID_SPAN_ID_2}\n`, - ]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe(VALID_TRACE_ID); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID, VALID_SPAN_ID_2]); - }); - - test("handles '/' followed by newline-separated span IDs", () => { - const result = parsePositionalArgs([ - `${VALID_TRACE_ID}/${VALID_SPAN_ID}\n${VALID_SPAN_ID_2}`, - ]); - expect(result.kind).toBe("resolved"); - if (result.kind !== "resolved") throw new Error("unreachable"); - expect(result.traceTarget.traceId).toBe(VALID_TRACE_ID); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID, VALID_SPAN_ID_2]); - }); - - test("handles 'org/project//' plus extra span ID args", () => { - const result = parsePositionalArgs([ - `my-org/my-project/${VALID_TRACE_ID}/${VALID_SPAN_ID}`, - VALID_SPAN_ID_2, - ]); - expect(result.kind).toBe("resolved"); - if (result.kind !== "resolved") throw new Error("unreachable"); - expect(result.traceTarget.type).toBe("explicit"); - expect(result.traceTarget.traceId).toBe(VALID_TRACE_ID); - expect(result.rawSpanIds).toEqual([VALID_SPAN_ID, VALID_SPAN_ID_2]); - }); - - test("still throws 'Span ID' ContextError when only a trace ID is given", () => { - expect(() => parsePositionalArgs([` ${VALID_TRACE_ID} `])).toThrow( - ContextError, - ); - }); - - test("whitespace-only argument is treated as no arguments", () => { - expect(() => parsePositionalArgs([" \n "])).toThrow(ContextError); - }); + test("org/project/spanId (missing trace ID) throws a trace-focused ContextError (CLI-1GP)", () => { + try { + parseSpanViewInput([`my-org/my-project/${VALID_SPAN_ID}`]); + expect.unreachable("Should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(ContextError); + const msg = (error as ContextError).message; + // Points at the missing trace ID, not a missing span ID. + expect(msg).toContain("trace ID"); + expect(msg).toContain(VALID_SPAN_ID); + // Does not misdirect the user to `span list` with the span ID. + expect(msg).not.toContain(`span list my-org/my-project/${VALID_SPAN_ID}`); + } }); - describe("error cases", () => { - test("throws ContextError for empty args", () => { - expect(() => parsePositionalArgs([])).toThrow(ContextError); + describe("missing IDs", () => { + test.each([ + [[]], + [[" \n "]], + [[VALID_TRACE_ID]], + [[` ${VALID_TRACE_ID} `]], + [[`${VALID_TRACE_ID}/bbbb2222cccc3333dddd4444eeee5555`]], + ])("%j → ContextError", (args) => { + expect(() => parseSpanViewInput(args)).toThrow(ContextError); }); test("error message mentions trace ID and span ID", () => { - try { - parsePositionalArgs([]); - expect.unreachable("Should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ContextError); - expect((error as ContextError).message).toContain("Trace ID"); - } - }); - - test("throws ContextError when only trace ID provided (no span IDs)", () => { - expect(() => parsePositionalArgs([VALID_TRACE_ID])).toThrow(ContextError); + expect(() => parseSpanViewInput([])).toThrow("Trace ID"); }); test("missing span IDs error suggests span list", () => { - try { - parsePositionalArgs([VALID_TRACE_ID]); - expect.unreachable("Should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ContextError); - expect((error as ContextError).message).toContain("span list"); - } - }); - - test("accepts invalid trace ID as raw (recovery runs at command layer)", () => { - // Previously threw synchronously; now the command-level recovery - // path (parseTraceTargetWithRecovery) handles malformed trace IDs - // consistent with `trace view`. - const result = parsePositionalArgs(["not-valid", VALID_SPAN_ID]); - expect(result.kind).toBe("deferred"); - if (result.kind !== "deferred") throw new Error("unreachable"); - expect(result.rawTraceArg).toBe("not-valid"); - }); - - test("accepts invalid span ID as raw (validation deferred to command)", () => { - // parsePositionalArgs no longer validates span IDs so the command - // layer can run recovery with full trace context. - const result = parsePositionalArgs([VALID_TRACE_ID, "not-a-span-id"]); - expect(result.rawSpanIds).toEqual(["not-a-span-id"]); - }); - - test("accepts too-short span ID as raw", () => { - const result = parsePositionalArgs([VALID_TRACE_ID, "abcd1234"]); - expect(result.rawSpanIds).toEqual(["abcd1234"]); + expect(() => parseSpanViewInput([VALID_TRACE_ID])).toThrow("span list"); }); test("throws ContextError for bare span ID without trace ID (CLI-SC)", () => { - expect(() => parsePositionalArgs(["a1b2c3d4e5f67890"])).toThrow( + expect(() => parseSpanViewInput(["a1b2c3d4e5f67890"])).toThrow( ContextError, ); }); test("bare span ID error identifies the input and suggests correct usage", () => { try { - parsePositionalArgs(["A1B2C3D4E5F67890"]); + parseSpanViewInput(["A1B2C3D4E5F67890"]); expect.unreachable("Should have thrown"); } catch (error) { expect(error).toBeInstanceOf(ContextError); @@ -376,8 +233,8 @@ describe("parsePositionalArgs", () => { test("bare span ID with dashes is still detected (CLI-SC)", () => { // Some tools format span IDs with dashes - expect(() => parsePositionalArgs(["a1b2-c3d4-e5f6-7890"])).toThrow( - ContextError, + expect(() => parseSpanViewInput(["a1b2-c3d4-e5f6-7890"])).toThrow( + "looks like a span ID", ); }); }); @@ -598,18 +455,21 @@ describe("viewCommand.func", () => { expect(parsed[0].ancestors).toEqual([]); }); - test("throws ContextError for org-all target (org/ without project)", async () => { + test("fails to parse without span IDs, before resolving", async () => { const { context } = createContext(); - // "my-org/" is parsed as org-all mode which is not supported for span view - await expect( - func.call( + const error = await func + .call( context, { spans: 0, fresh: false }, `my-org/my-project/${VALID_TRACE_ID}`, - // No span IDs — but we need at least one - ), - ).rejects.toThrow(ContextError); + ) + .catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ArgumentParseError); + expect((error as ArgumentParseError).exception).toBeInstanceOf( + ContextError, + ); + expect(getDetailedTraceSpy).not.toHaveBeenCalled(); }); test("throws ResolutionError for multiple missing span IDs", async () => { diff --git a/packages/cli/test/commands/trace/view.func.test.ts b/packages/cli/test/commands/trace/view.func.test.ts index b5652c3ff..20c35834b 100644 --- a/packages/cli/test/commands/trace/view.func.test.ts +++ b/packages/cli/test/commands/trace/view.func.test.ts @@ -8,6 +8,7 @@ * the func() body without real HTTP calls or database access. */ +import { ArgumentParseError } from "@stricli/core"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { flattenSpanTree, @@ -649,6 +650,39 @@ describe("viewCommand.func", () => { ).rejects.toThrow(ContextError); }); + test("a missing trace ID fails to parse, before any lookup", async () => { + const { context } = createMockContext(); + const func = await viewCommand.loader(); + + const error = await func + .call(context, { json: false, web: false, spans: 100 }, "my-org/web") + .catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ArgumentParseError); + expect((error as ArgumentParseError).exception).toBeInstanceOf( + ContextError, + ); + expect(resolveOrgAndProjectSpy).not.toHaveBeenCalled(); + expect(getDetailedTraceSpy).not.toHaveBeenCalled(); + }); + + test("recovers a malformed trace ID when it resolves", async () => { + getDetailedTraceSpy.mockResolvedValue(sampleSpans); + + const { context } = createMockContext(); + const func = await viewCommand.loader(); + await func.call( + context, + { json: true, web: false, spans: 100 }, + "my-org/aaaa1111bbbb2222cccc3333dddd4444ios", + ); + + expect(getDetailedTraceSpy).toHaveBeenCalledWith( + "my-org", + "aaaa1111bbbb2222cccc3333dddd4444", + expect.anything(), + ); + }); + test("resolves project search target", async () => { findProjectsBySlugSpy.mockResolvedValue({ projects: [ diff --git a/packages/cli/test/commands/trace/view.test.ts b/packages/cli/test/commands/trace/view.test.ts index e338f260c..2953d5bbc 100644 --- a/packages/cli/test/commands/trace/view.test.ts +++ b/packages/cli/test/commands/trace/view.test.ts @@ -1,14 +1,14 @@ /** * Trace View Command Tests * - * Tests for pre-processing (swap detection, issue ID detection) - * and project resolution in src/commands/trace/view.ts. + * Tests for argument parsing (issue ID detection) and project resolution + * in src/commands/trace/view.ts. * * Note: Core trace target parsing tests are in test/lib/trace-target.test.ts. */ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { preProcessArgs } from "../../../src/commands/trace/view.js"; +import { parseTraceViewInput } from "../../../src/commands/trace/view.js"; import type { ProjectWithOrg } from "../../../src/lib/api-client.js"; vi.mock("../../../src/lib/api-client.js", async (importOriginal) => { @@ -24,63 +24,60 @@ vi.mock("../../../src/lib/api-client.js", async (importOriginal) => { // oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking import * as apiClient from "../../../src/lib/api-client.js"; -import { ResolutionError, ValidationError } from "../../../src/lib/errors.js"; +import { + ContextError, + ResolutionError, + ValidationError, +} from "../../../src/lib/errors.js"; import { resolveProjectBoundSlug } from "../../../src/lib/resolve-target.js"; const VALID_TRACE_ID = "aaaa1111bbbb2222cccc3333dddd4444"; -describe("preProcessArgs", () => { - test("returns args unchanged for single arg", () => { - const result = preProcessArgs([VALID_TRACE_ID]); - expect(result.correctedArgs).toEqual([VALID_TRACE_ID]); - expect(result.warning).toBeUndefined(); - expect(result.suggestion).toBeUndefined(); - }); - - test("returns args unchanged for normal two-arg order", () => { - const result = preProcessArgs(["my-org/frontend", VALID_TRACE_ID]); - expect(result.correctedArgs).toEqual(["my-org/frontend", VALID_TRACE_ID]); - expect(result.warning).toBeUndefined(); +describe("parseTraceViewInput", () => { + test("parses trace arguments, keeping the trace ID as typed", () => { + expect(parseTraceViewInput([VALID_TRACE_ID])).toEqual({ + trace: { + parsed: { type: "auto-detect", traceId: VALID_TRACE_ID }, + warnings: [], + }, + }); + expect(parseTraceViewInput(["my-org/frontend", "not-a-trace"])).toEqual({ + trace: { + parsed: { + type: "explicit", + org: "my-org", + project: "frontend", + traceId: "not-a-trace", + }, + targetArg: "my-org/frontend", + warnings: [], + }, + }); }); - test("detects swapped args and corrects order", () => { - // User put trace-id first, org/project second - const result = preProcessArgs([VALID_TRACE_ID, "my-org/frontend"]); - expect(result.correctedArgs).toEqual(["my-org/frontend", VALID_TRACE_ID]); - expect(result.warning).toContain("reversed"); + test("a missing trace ID fails to parse", () => { + expect(() => parseTraceViewInput([])).toThrow(ContextError); + expect(() => parseTraceViewInput(["my-org/frontend"])).toThrow( + ContextError, + ); }); test("detects issue short ID and suggests issue view (two args)", () => { - const result = preProcessArgs(["CAM-82X", VALID_TRACE_ID]); - expect(result.correctedArgs).toEqual(["CAM-82X", VALID_TRACE_ID]); - expect(result.suggestion).toContain("sentry issue view CAM-82X"); - expect(result.issueShortId).toBeUndefined(); - }); - - test("detects single-arg issue short ID for auto-recovery", () => { - const result = preProcessArgs(["CLI-G5"]); - expect(result.correctedArgs).toEqual(["CLI-G5"]); - expect(result.issueShortId).toBe("CLI-G5"); - expect(result.suggestion).toBeUndefined(); - expect(result.warning).toBeUndefined(); - }); - - test("detects multi-segment issue short ID for auto-recovery", () => { - const result = preProcessArgs(["SPOTLIGHT-ELECTRON-4D"]); - expect(result.correctedArgs).toEqual(["SPOTLIGHT-ELECTRON-4D"]); - expect(result.issueShortId).toBe("SPOTLIGHT-ELECTRON-4D"); - }); - - test("does not set issueShortId for single-arg trace ID", () => { - const result = preProcessArgs([VALID_TRACE_ID]); - expect(result.issueShortId).toBeUndefined(); + const result = parseTraceViewInput(["CAM-82X", VALID_TRACE_ID]); + expect(result).toMatchObject({ + suggestion: "Did you mean: sentry issue view CAM-82X", + }); + expect(result).not.toHaveProperty("issueShortId"); }); - test("returns empty args unchanged", () => { - const result = preProcessArgs([]); - expect(result.correctedArgs).toEqual([]); - expect(result.issueShortId).toBeUndefined(); - }); + test.each(["CLI-G5", "SPOTLIGHT-ELECTRON-4D"])( + "a lone issue short ID %j is looked up as an issue", + (shortId) => { + expect(parseTraceViewInput([shortId])).toEqual({ + issueShortId: shortId, + }); + }, + ); }); describe("resolveProjectBoundSlug", () => { diff --git a/packages/cli/test/lib/help-positional.test.ts b/packages/cli/test/lib/help-positional.test.ts index 43ff47584..40429805f 100644 --- a/packages/cli/test/lib/help-positional.test.ts +++ b/packages/cli/test/lib/help-positional.test.ts @@ -187,6 +187,34 @@ describe("help recovery on ValidationError", () => { }); }); +describe("help recovery on argument parse errors", () => { + test("sentry replay view help → shows help instead of the parse error", async () => { + // `replay view`'s arguments parse together in the command wrapper; "help" + // fails as a replay ID there, where recovery can still catch it + const { stdout, stderr } = await runCommand(["replay", "view", "help"]); + + expect(stdout).toContain("sentry replay view"); + expect(stderr).toContain("sentry replay view --help"); + expect(stderr).not.toContain("Failed to parse"); + }); + + test("sentry replay view not-hex → argument parse error", async () => { + // Stricli writes argument errors to the real process's stderr + const stderrWrite = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + try { + const { stdout } = await runCommand(["replay", "view", "not-hex"]); + expect(stdout).not.toContain("sentry replay view"); + expect(stderrWrite.mock.calls.map(([s]) => String(s)).join("")).toContain( + 'Failed to parse "not-hex" for replay-id-or-url', + ); + } finally { + stderrWrite.mockRestore(); + } + }); +}); + describe("help command unchanged", () => { test("sentry help still shows branded help", async () => { const { stdout, stderr } = await runCommand(["help"]); diff --git a/packages/cli/test/lib/hex-id-recovery.test.ts b/packages/cli/test/lib/hex-id-recovery.test.ts index 9c0146477..9c5391512 100644 --- a/packages/cli/test/lib/hex-id-recovery.test.ts +++ b/packages/cli/test/lib/hex-id-recovery.test.ts @@ -331,6 +331,31 @@ describe("recoverHexId decision tree", () => { expect(r.kind === "fuzzy" && r.prefix).toBe("05d6975ab9bb"); }); + test.each([ + ["trace-05d6975ab9bb", "05d6975ab9bb", undefined], + ["span-05d6975a...00000000", "05d6975a", "00000000"], + ])( + "fuzzy-looks up a prefixed partial ID %j", + async (input, prefix, suffix) => { + const fullId = `05d6975ab9bb${"0".repeat(20)}`; + stubAdapter("event", async () => [fullId]); + const r = await recoverHexId(input, "event", CLEAN_CTX); + expect(r).toMatchObject({ kind: "fuzzy", id: fullId, prefix }); + expect(r.kind === "fuzzy" && r.suffix).toBe(suffix); + }, + ); + + test("a prefixed slug is still a slug", async () => { + let called = false; + stubAdapter("event", async () => { + called = true; + return []; + }); + const r = await recoverHexId("span-my-app", "event", CLEAN_CTX); + expect(r).toMatchObject({ kind: "failed", reason: "looks-like-slug" }); + expect(called).toBe(false); + }); + test("fuzzy with multiple matches returns multiple-matches", async () => { const id1 = `05d6975ab9bb${"0".repeat(20)}`; const id2 = `05d6975ab9bb${"1".repeat(20)}`; diff --git a/packages/cli/test/lib/id-arguments.test.ts b/packages/cli/test/lib/id-arguments.test.ts new file mode 100644 index 000000000..a333d582b --- /dev/null +++ b/packages/cli/test/lib/id-arguments.test.ts @@ -0,0 +1,289 @@ +/** + * Tests for the ID-arguments grammar (grammar/id-arguments.pegjs), through + * src/lib/id-arguments.ts. + * + * The grammar does not know any kind of ID, so the same table runs for every + * kind in `ID_KINDS`: the syntax cannot drift between commands. + */ + +import { describe, expect, test } from "vitest"; +import { ContextError, ValidationError } from "../../src/lib/errors.js"; +import { + ID_KINDS, + type IdArgumentsOptions, + type IdKind, + parseIdArgument, + parseIdArguments, +} from "../../src/lib/id-arguments.js"; + +const HEX = "aaaa1111bbbb2222cccc3333dddd4444"; +const HEX2 = "bbbb2222cccc3333dddd4444eeee5555"; +const SPAN = "aaaa1111bbbb2222"; +const SPAN2 = "cccc3333dddd4444"; +const HINT = "sentry thing view [//]"; + +/** An ID of each kind, and a second one */ +const EXAMPLES: Record = { + trace: [HEX, HEX2], + replay: [HEX, HEX2], + log: [HEX, HEX2], + span: [SPAN, SPAN2], +}; + +const KINDS = Object.entries(ID_KINDS).map( + ([name, kind]) => + [name, kind, ...EXAMPLES[name as keyof typeof ID_KINDS]] as const, +); + +function parse( + kind: IdKind, + args: readonly string[], + options: Partial = {}, +) { + return parseIdArguments(args, kind, { usageHint: HINT, ...options }); +} + +describe.each(KINDS)("%s IDs", (_name, kind, id, other) => { + test.each([ + [[id], undefined], + [[`/${id}`], undefined], + [[`${id}//`], undefined], + [[`acme/${id}`], "acme/"], + [[`acme/web/${id}`], "acme/web"], + [[`acme/web/${id}/`], "acme/web"], + [["acme/web", id], "acme/web"], + [["web", id], "web"], + [["My Project", id], "My Project"], + [["", id], undefined], + ])("%j → target %j", (args, target) => { + expect(parse(kind, args)).toEqual({ target, ids: [id], warnings: [] }); + }); + + test("normalizes the ID and trims arguments", () => { + expect(parse(kind, [` acme/${id.toUpperCase()}\n`])).toEqual({ + target: "acme/", + ids: [id], + warnings: [], + }); + }); + + test("reads reversed arguments, with a warning", () => { + expect(parse(kind, [id, "acme/web"])).toEqual({ + target: "acme/web", + ids: [id], + warnings: [`Arguments appear reversed. Interpreting as: acme/web ${id}`], + }); + }); + + test("splits IDs off a target at spaces, and at line breaks", () => { + const expected = { target: "acme/web", ids: [id, other], warnings: [] }; + const multiple = { multiple: true }; + expect(parse(kind, [`acme/web ${id} ${other}`], multiple)).toEqual( + expected, + ); + expect(parse(kind, ["acme/web", `${id}\n${other}\n`], multiple)).toEqual( + expected, + ); + // A display name keeps its spaces + expect(parse(kind, [`My Project ${id}`])).toEqual({ + target: "My Project", + ids: [id], + warnings: [], + }); + }); + + test("more IDs may repeat the target", () => { + expect( + parse(kind, [`acme/web/${id}`, `acme/web/${other}`], { multiple: true }), + ).toEqual({ target: "acme/web", ids: [id, other], warnings: [] }); + expect( + parse(kind, ["acme/web", `acme/web/${other}`], { multiple: true }), + ).toEqual({ target: "acme/web", ids: [other], warnings: [] }); + }); + + test("IDs from different targets are an error", () => { + expect(() => + parse(kind, [`acme/web/${id}`, `acme/api/${other}`], { multiple: true }), + ).toThrow(ValidationError); + }); + + test("keeps one ID, ignoring the rest with a warning", () => { + expect(parse(kind, ["web", "extra", id])).toEqual({ + target: "web", + ids: [id], + warnings: [`Extra arguments ignored: extra. Expected: ${HINT}`], + }); + expect(parse(kind, [`acme/web/${id}`, other])).toEqual({ + target: "acme/web", + ids: [id], + warnings: [`Extra arguments ignored: ${other}. Expected: ${HINT}`], + }); + }); + + test.each([[[]], [[""]], [["acme/web"]], [["acme/web/"]], [["/"]]])( + "%j → missing ID", + (args) => { + expect(() => parse(kind, args)).toThrow(ContextError); + }, + ); + + test.each([[[`acme//${id}`]], [[`/web/${id}`]]])( + "%j → empty org or project", + (args) => { + expect(() => parse(kind, args)).toThrow(ValidationError); + }, + ); + + test.each([ + [["not-hex"], undefined], + [["acme/web/not-hex"], "acme/web"], + [["acme/web", "not-hex"], "acme/web"], + [["not-hex", "acme/web"], "acme/web"], + ])("%j → invalid ID, or raw ID and target %j", (args, target) => { + expect(() => parse(kind, args)).toThrow(ValidationError); + expect(parse(kind, args, { validate: false })).toMatchObject({ + target, + ids: ["not-hex"], + }); + }); + + test.each([ + [`acme/${id.slice(0, 12)}`, id.slice(0, 12)], + [ + `acme/${id.slice(0, 8)}...${id.slice(-8)}`, + `${id.slice(0, 8)}...${id.slice(-8)}`, + ], + [`acme/${id}xyz`, `${id}xyz`], + // A URL fragment prefix, as in `#span-` + [`acme/trace-${id}`, `trace-${id}`], + [`acme/span-${id.slice(0, 12)}`, `span-${id.slice(0, 12)}`], + ])("%j → invalid ID, or raw ID %j after the org", (arg, raw) => { + expect(() => parse(kind, [arg])).toThrow(ValidationError); + expect(parse(kind, [arg], { validate: false })).toEqual({ + target: "acme/", + ids: [raw], + warnings: [], + }); + }); + + test.each([["acme/abc123"], ["acme/deadbeef-app"], ["acme/null"]])( + "%j does not look like an ID → missing ID", + (arg) => { + expect(() => parse(kind, [arg])).toThrow(ContextError); + }, + ); + + test("a command that needs a project reads `/` as the project", () => { + expect(parse(kind, [`web/${id}`], { needsProject: true })).toEqual({ + target: "web", + ids: [id], + warnings: [], + }); + }); + + describe("optional IDs", () => { + const optional = { optional: true }; + + test.each([ + [[], undefined], + [["web"], "web"], + [["acme/"], "acme/"], + [["acme/web"], "acme/web"], + [["acme/web/"], "acme/web/"], + [[`acme/${id.slice(0, 12)}`], `acme/${id.slice(0, 12)}`], + ])("%j → target %j, no ID", (args, target) => { + expect(parse(kind, args, optional)).toEqual({ + target, + ids: [], + warnings: [], + }); + }); + + test.each([ + [[id], undefined], + [[`acme/${id}`], "acme/"], + [["acme/web", id], "acme/web"], + ])("%j → target %j and the ID", (args, target) => { + expect(parse(kind, args, optional)).toEqual({ + target, + ids: [id], + warnings: [], + }); + }); + + test("an argument after the target is an ID", () => { + expect(() => parse(kind, ["web", "not-hex"], optional)).toThrow( + ValidationError, + ); + }); + }); +}); + +describe("parseIdArgument", () => { + test("returns the one ID", () => { + expect( + parseIdArgument(["acme/web", HEX], ID_KINDS.trace, { usageHint: HINT }), + ).toEqual({ target: "acme/web", id: HEX, warnings: [] }); + }); +}); + +describe("URLs", () => { + const traceUrl = `https://acme.sentry.io/explore/traces/trace/${HEX}/`; + const replayUrl = `https://sentry.io/organizations/acme/explore/replays/${HEX}/`; + + test("a URL with the command's kind of ID names the ID and its org", () => { + expect(parse(ID_KINDS.trace, [traceUrl])).toEqual({ + target: "acme/", + ids: [HEX], + baseUrl: "https://acme.sentry.io", + warnings: [], + }); + expect(parse(ID_KINDS.replay, [replayUrl])).toEqual({ + target: "acme/", + ids: [HEX], + baseUrl: "https://sentry.io", + warnings: [], + }); + }); + + test("a URL without one is missing the ID, with an example URL", () => { + try { + parse(ID_KINDS.replay, [traceUrl]); + expect.unreachable(); + } catch (error) { + expect(error).toBeInstanceOf(ContextError); + expect((error as ContextError).message).toContain( + "Pass a replay URL: https://sentry.io/organizations/{org}/explore/replays/{replayId}/", + ); + } + }); + + test("a URL with no ID of any kind is a target when IDs are optional", () => { + const projectUrl = "https://sentry.io/organizations/acme/projects/web/"; + expect(parse(ID_KINDS.trace, [projectUrl], { optional: true })).toEqual({ + target: projectUrl, + ids: [], + warnings: [], + }); + }); +}); + +describe("tokens", () => { + test("rejects NUL", () => { + expect(() => parse(ID_KINDS.trace, [`a\0b`])).toThrow(ValidationError); + }); + + test("a span ID's trace is its target", () => { + expect( + parse(ID_KINDS.span, [`acme/web/${HEX}/${SPAN}`, SPAN2], { + multiple: true, + }), + ).toEqual({ target: `acme/web/${HEX}`, ids: [SPAN, SPAN2], warnings: [] }); + expect( + parse(ID_KINDS.span, [`${HEX} ${SPAN}`], { + multiple: true, + optional: true, + }), + ).toEqual({ target: HEX, ids: [SPAN], warnings: [] }); + }); +}); diff --git a/packages/cli/test/lib/positional-parse.test.ts b/packages/cli/test/lib/positional-parse.test.ts index fe957bde3..099fe70af 100644 --- a/packages/cli/test/lib/positional-parse.test.ts +++ b/packages/cli/test/lib/positional-parse.test.ts @@ -7,6 +7,7 @@ */ import { + ArgumentParseError, buildApplication, type CommandContext, ExitCode, @@ -27,8 +28,14 @@ import { ValidationError, } from "../../src/lib/errors.js"; import { + applyBoundPositionals, + type BoundArguments, + BoundPositional, + boundArguments, checkDestructive, rawPositional, + skipLeadingArgument, + skippedArgument, } from "../../src/lib/positional-parse.js"; import { buildRouteMap } from "../../src/lib/route-map.js"; import { @@ -247,3 +254,178 @@ describe("rawPositional", () => { expect(rawPositional(undefined)).toBeUndefined(); }); }); + +/** `[] `: what the first argument means depends on the count. */ +function parseIdArguments(raw: readonly string[]): { + target?: string; + id: string; +} { + if (raw.length === 0) { + throw new ContextError("ID", "test cmd [] ", []); + } + if (raw.length > 2) { + throw new ValidationError("Too many arguments"); + } + const [first = "", second] = raw; + return second === undefined ? { id: first } : { target: first, id: second }; +} + +const idResolver = vi.fn( + async ( + parsed: ReturnType, + { cwd }: { cwd: string }, + raw: readonly string[], + ) => ({ ...parsed, cwd, raw: [...raw] }), +); + +const idArguments = boundArguments({ + placeholder: "target-or-id", + brief: "[] ", + kind: ORG_PROJECT_TARGET, + parse: parseIdArguments, + resolve: idResolver, +}); + +type IdArguments = BoundArguments< + ReturnType, + Awaited> +>; + +/** Run `test cmd ...args` with {@link idArguments}, recording the result. */ +async function runWithArguments( + args: string[], + positional: typeof idArguments = idArguments, +): Promise<{ + resolved: unknown[]; + stderr: string; + exitCode: number | undefined; +}> { + const resolved: unknown[] = []; + const command = buildCommand< + Record, + [IdArguments], + TestContext + >({ + docs: { brief: "Takes an ID" }, + auth: false, + parameters: { positional }, + // oxlint-disable-next-line require-yield -- test command only records the resolved arguments + async *func(_flags, target) { + resolved.push(await target.resolved); + }, + }); + const app = buildApplication( + buildRouteMap({ routes: { cmd: command }, docs: { brief: "Test" } }), + { name: "test", determineExitCode: getExitCode }, + ); + let stderr = ""; + const stdout = { write: () => true }; + const context = { + cwd: "/work", + process: { + stdout, + stderr: { + write: (s: string) => { + stderr += s; + return true; + }, + }, + exitCode: undefined, + }, + stdout, + } as unknown as TestContext; + await run(app, ["cmd", ...args], context); + return { resolved, stderr, exitCode: context.process.exitCode }; +} + +describe("boundArguments", () => { + test("parses every argument at once, so the count decides what each means", async () => { + expect((await runWithArguments(["abc"])).resolved).toEqual([ + { id: "abc", cwd: "/work", raw: ["abc"] }, + ]); + expect((await runWithArguments(["acme/web", "abc"])).resolved).toEqual([ + { target: "acme/web", id: "abc", cwd: "/work", raw: ["acme/web", "abc"] }, + ]); + }); + + test("a parse error fails as an argument parse error", async () => { + const { resolved, stderr, exitCode } = await runWithArguments([ + "a", + "b", + "c", + ]); + expect(resolved).toEqual([]); + expect(exitCode).toBe(ExitCode.InvalidArgument); + expect(stderr).toContain('Failed to parse "a b c" for target-or-id'); + expect(stderr).toContain("Too many arguments"); + }); + + test("parses no arguments too", async () => { + const { resolved, exitCode, stderr } = await runWithArguments([]); + expect(resolved).toEqual([]); + expect(exitCode).toBe(ExitCode.InvalidArgument); + expect(stderr).toContain("ID is required"); + }); + + test("raw strings (SDK invoke, tests) become one bound value", async () => { + idResolver.mockClear(); + const context = { cwd: "/work" }; + const [value, ...rest] = applyBoundPositionals( + ["acme/web", "abc"], + idArguments, + context, + ); + expect(rest).toEqual([]); + expect(value).toBeInstanceOf(BoundPositional); + const bound = value as IdArguments; + expect(bound.raw).toEqual(["acme/web", "abc"]); + expect(bound.parsed).toEqual({ target: "acme/web", id: "abc" }); + expect(idResolver).not.toHaveBeenCalled(); + await expect(bound.resolved).resolves.toMatchObject({ cwd: "/work" }); + + // A value already parsed is kept + expect(applyBoundPositionals([bound], idArguments, context)).toEqual([ + bound, + ]); + }); + + test("raw strings that fail to parse throw an argument parse error", () => { + let error: unknown; + try { + applyBoundPositionals([], idArguments, {}); + } catch (caught) { + error = caught; + } + expect(error).toBeInstanceOf(ArgumentParseError); + expect((error as ArgumentParseError).exception).toBeInstanceOf( + ContextError, + ); + }); + + test("rawPositional returns every argument", () => { + const [bound] = applyBoundPositionals(["abc"], idArguments, {}); + expect(rawPositional(bound)).toEqual(["abc"]); + }); + + test("skipLeadingArgument leaves a matching first argument out before parsing", async () => { + const skipping = skipLeadingArgument(idArguments, (arg) => arg === "view"); + // Parsed without "view", so the count still decides + expect( + (await runWithArguments(["view", "acme/web", "abc"], skipping)).resolved, + ).toEqual([ + { target: "acme/web", id: "abc", cwd: "/work", raw: ["acme/web", "abc"] }, + ]); + + const [bound] = applyBoundPositionals(["view", "abc"], skipping, {}); + expect(skippedArgument(bound)).toBe("view"); + expect((bound as IdArguments).parsed).toEqual({ id: "abc" }); + const [kept] = applyBoundPositionals(["abc"], skipping, {}); + expect(skippedArgument(kept)).toBeUndefined(); + }); + + test("each argument completes as the given kind", () => { + expect(getCompletionTarget(idArguments.parameter.parse)).toBe( + ORG_PROJECT_TARGET, + ); + }); +}); diff --git a/packages/cli/test/lib/trace-target.test.ts b/packages/cli/test/lib/trace-target.test.ts index 8e105a803..e2af2a222 100644 --- a/packages/cli/test/lib/trace-target.test.ts +++ b/packages/cli/test/lib/trace-target.test.ts @@ -1,8 +1,9 @@ /** * Tests for shared trace-target parsing and resolution. * - * Tests parseTraceTarget, parseSlashSeparatedTraceTarget, - * and targetArgToTraceTarget from src/lib/trace-target.ts. + * Tests parseTraceTarget, the dual-mode parsers, and the resolvers from + * src/lib/trace-target.ts. The syntax itself is tested in + * id-arguments.test.ts. */ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; @@ -18,6 +19,15 @@ vi.mock("../../src/lib/api-client.js", async (importOriginal) => { ); }); +vi.mock("../../src/lib/region.js", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + resolveEffectiveOrg: vi.fn(actual.resolveEffectiveOrg), + }; +}); + // oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking import * as apiClient from "../../src/lib/api-client.js"; import { @@ -25,51 +35,46 @@ import { ResolutionError, ValidationError, } from "../../src/lib/errors.js"; +// oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking +import * as region from "../../src/lib/region.js"; import { - extractRawTraceId, parseDualModeArgs, - parseSlashSeparatedTraceTarget, + parseDualModeInput, parseTraceTarget, + parseTraceArgumentsInput, + parseTraceTargetInput, + resolveTraceArguments, resolveTraceOrg, resolveTraceOrgOptionalProject, resolveTraceOrgProject, - stripTrailingSlashesAfterTraceId, - targetArgToTraceTarget, - traceTargetFromUrl, } from "../../src/lib/trace-target.js"; const VALID_TRACE_ID = "aaaa1111bbbb2222cccc3333dddd4444"; -describe("parseSlashSeparatedTraceTarget", () => { +describe("parseTraceTarget with one argument", () => { const HINT = "sentry span list [//]"; test("bare trace ID → auto-detect", () => { - const result = parseSlashSeparatedTraceTarget(VALID_TRACE_ID, HINT); + const result = parseTraceTarget([VALID_TRACE_ID], HINT); expect(result.type).toBe("auto-detect"); expect(result.traceId).toBe(VALID_TRACE_ID); }); test("normalizes uppercase trace ID", () => { - const result = parseSlashSeparatedTraceTarget( - "AAAA1111BBBB2222CCCC3333DDDD4444", - HINT, - ); + const result = parseTraceTarget(["AAAA1111BBBB2222CCCC3333DDDD4444"], HINT); expect(result.traceId).toBe(VALID_TRACE_ID); }); test("strips UUID dashes from trace ID", () => { - const result = parseSlashSeparatedTraceTarget( - "aaaa1111-bbbb-2222-cccc-3333dddd4444", + const result = parseTraceTarget( + ["aaaa1111-bbbb-2222-cccc-3333dddd4444"], HINT, ); expect(result.traceId).toBe(VALID_TRACE_ID); }); test("org/trace-id → org-scoped", () => { - const result = parseSlashSeparatedTraceTarget( - `my-org/${VALID_TRACE_ID}`, - HINT, - ); + const result = parseTraceTarget([`my-org/${VALID_TRACE_ID}`], HINT); expect(result.type).toBe("org-scoped"); expect(result.traceId).toBe(VALID_TRACE_ID); if (result.type === "org-scoped") { @@ -78,8 +83,8 @@ describe("parseSlashSeparatedTraceTarget", () => { }); test("org/project/trace-id → explicit", () => { - const result = parseSlashSeparatedTraceTarget( - `my-org/my-project/${VALID_TRACE_ID}`, + const result = parseTraceTarget( + [`my-org/my-project/${VALID_TRACE_ID}`], HINT, ); expect(result.type).toBe("explicit"); @@ -93,8 +98,8 @@ describe("parseSlashSeparatedTraceTarget", () => { test("preserves underscores in slugs (see #770)", () => { // Sentry accepts underscores in project slugs, so they must flow // through the trace-target parser unchanged. - const result = parseSlashSeparatedTraceTarget( - `my_org/my_project/${VALID_TRACE_ID}`, + const result = parseTraceTarget( + [`my_org/my_project/${VALID_TRACE_ID}`], HINT, ); expect(result.type).toBe("explicit"); @@ -106,27 +111,31 @@ describe("parseSlashSeparatedTraceTarget", () => { }); test("trailing slash without trace ID throws", () => { - expect(() => parseSlashSeparatedTraceTarget("my-org/", HINT)).toThrow( - ContextError, - ); + expect(() => parseTraceTarget(["my-org/"], HINT)).toThrow(ContextError); }); test("invalid trace ID throws ValidationError", () => { - expect(() => - parseSlashSeparatedTraceTarget("not-a-trace-id", HINT), - ).toThrow(ValidationError); + expect(() => parseTraceTarget(["not-a-trace-id"], HINT)).toThrow( + ValidationError, + ); }); }); -describe("targetArgToTraceTarget", () => { +describe("parseTraceTarget with a target argument", () => { test("explicit target (org/project)", () => { - const result = targetArgToTraceTarget("my-org/my-project", VALID_TRACE_ID); + const result = parseTraceTarget( + ["my-org/my-project", VALID_TRACE_ID], + "sentry trace view ", + ); expect(result.type).toBe("explicit"); expect(result.traceId).toBe(VALID_TRACE_ID); }); test("org-all target (org/) → org-scoped", () => { - const result = targetArgToTraceTarget("my-org/", VALID_TRACE_ID); + const result = parseTraceTarget( + ["my-org/", VALID_TRACE_ID], + "sentry trace view ", + ); expect(result.type).toBe("org-scoped"); if (result.type === "org-scoped") { expect(result.org).toBe("my-org"); @@ -134,7 +143,10 @@ describe("targetArgToTraceTarget", () => { }); test("bare slug → project-search", () => { - const result = targetArgToTraceTarget("frontend", VALID_TRACE_ID); + const result = parseTraceTarget( + ["frontend", VALID_TRACE_ID], + "sentry trace view ", + ); expect(result.type).toBe("project-search"); if (result.type === "project-search") { expect(result.projectSlug).toBe("frontend"); @@ -142,7 +154,10 @@ describe("targetArgToTraceTarget", () => { }); test("preserves scoped display-name metadata", () => { - const result = targetArgToTraceTarget("my-org/My Project", VALID_TRACE_ID); + const result = parseTraceTarget( + ["my-org/My Project", VALID_TRACE_ID], + "sentry trace view ", + ); expect(result).toMatchObject({ type: "project-search", org: "my-org", @@ -152,7 +167,10 @@ describe("targetArgToTraceTarget", () => { }); test("empty string → auto-detect", () => { - const result = targetArgToTraceTarget("", VALID_TRACE_ID); + const result = parseTraceTarget( + ["", VALID_TRACE_ID], + "sentry trace view ", + ); expect(result.type).toBe("auto-detect"); }); }); @@ -180,7 +198,10 @@ describe("trace target resolution", () => { ], orgs: [{ slug: "project-owner", name: "Project Owner" }], }); - const parsed = targetArgToTraceTarget("frontend", VALID_TRACE_ID); + const parsed = parseTraceTarget( + ["frontend", VALID_TRACE_ID], + "sentry trace view ", + ); const resolved = await resolveTraceOrg( parsed, @@ -193,7 +214,10 @@ describe("trace target resolution", () => { test("project-bound errors preserve the full trace usage hint", async () => { findProjectsBySlugSpy.mockResolvedValue({ projects: [], orgs: [] }); - const parsed = targetArgToTraceTarget("missing", VALID_TRACE_ID); + const parsed = parseTraceTarget( + ["missing", VALID_TRACE_ID], + "sentry trace view ", + ); const usageHint = "sentry span view [//]"; try { @@ -207,7 +231,10 @@ describe("trace target resolution", () => { test("org-capable errors preserve the full trace usage hint", async () => { findProjectsBySlugSpy.mockResolvedValue({ projects: [], orgs: [] }); - const parsed = targetArgToTraceTarget("missing", VALID_TRACE_ID); + const parsed = parseTraceTarget( + ["missing", VALID_TRACE_ID], + "sentry trace view ", + ); const usageHint = "sentry trace view [//]"; try { @@ -327,14 +354,15 @@ describe("trailing slashes and Sentry trace URLs (CLI-13F)", () => { expect(result.traceId).toBe(VALID_TRACE_ID); }); - test("traceTargetFromUrl returns null for non-URL input", () => { - expect(traceTargetFromUrl(VALID_TRACE_ID)).toBeNull(); - expect(traceTargetFromUrl(`my-org/${VALID_TRACE_ID}`)).toBeNull(); + test("a Sentry URL without a trace ID is not a trace URL", () => { + expect(() => + parseTraceTarget(["https://sentry.io/organizations/my-org/"], HINT), + ).toThrow(ContextError); }); test("org/project// → explicit", () => { - const result = parseSlashSeparatedTraceTarget( - `my-org/my-project/${VALID_TRACE_ID}/`, + const result = parseTraceTarget( + [`my-org/my-project/${VALID_TRACE_ID}/`], HINT, ); expect(result).toEqual({ @@ -346,28 +374,13 @@ describe("trailing slashes and Sentry trace URLs (CLI-13F)", () => { }); test("/ → auto-detect", () => { - const result = parseSlashSeparatedTraceTarget(`${VALID_TRACE_ID}/`, HINT); + const result = parseTraceTarget([`${VALID_TRACE_ID}/`], HINT); expect(result).toEqual({ type: "auto-detect", traceId: VALID_TRACE_ID }); }); test("org/project/ still throws ContextError (trace ID omitted)", () => { - expect(() => - parseSlashSeparatedTraceTarget("my-org/my-project/", HINT), - ).toThrow(ContextError); - }); - - test("stripTrailingSlashesAfterTraceId leaves non-trace tails alone", () => { - expect(stripTrailingSlashesAfterTraceId("my-org/my-project/")).toBe( - "my-org/my-project/", - ); - expect(stripTrailingSlashesAfterTraceId(`org/${VALID_TRACE_ID}//`)).toBe( - `org/${VALID_TRACE_ID}`, - ); - }); - - test("extractRawTraceId ignores a trailing slash after the trace ID", () => { - expect(extractRawTraceId([`my-org/my-project/${VALID_TRACE_ID}/`])).toEqual( - { rawTraceId: VALID_TRACE_ID, targetArg: "my-org/my-project" }, + expect(() => parseTraceTarget(["my-org/my-project/"], HINT)).toThrow( + ContextError, ); }); @@ -387,3 +400,118 @@ describe("trailing slashes and Sentry trace URLs (CLI-13F)", () => { expect(result.mode).toBe("trace"); }); }); + +describe("parsing without side effects", () => { + const HINT = "sentry log list [/[/]]"; + const SELF_HOSTED_URL = `https://sentry.example.com/organizations/acme/traces/${VALID_TRACE_ID}/`; + let savedUrl: string | undefined; + + beforeEach(() => { + savedUrl = process.env.SENTRY_URL; + }); + + afterEach(() => { + if (savedUrl === undefined) { + delete process.env.SENTRY_URL; + } else { + process.env.SENTRY_URL = savedUrl; + } + }); + + test("a pasted URL's server is returned, not applied", () => { + // Applying a server with no account throws HostScopeError + expect(parseTraceTargetInput([SELF_HOSTED_URL], HINT)).toEqual({ + parsed: { type: "org-scoped", traceId: VALID_TRACE_ID, org: "acme" }, + baseUrl: "https://sentry.example.com", + warnings: [], + }); + expect(parseDualModeInput([SELF_HOSTED_URL], HINT)).toMatchObject({ + mode: "trace", + trace: { baseUrl: "https://sentry.example.com" }, + }); + expect(process.env.SENTRY_URL).toBe(savedUrl); + }); + + test("ignored extra arguments are returned as a warning", () => { + expect( + parseTraceTargetInput(["acme/web", VALID_TRACE_ID, "extra"], HINT), + ).toEqual({ + parsed: { + type: "explicit", + traceId: VALID_TRACE_ID, + org: "acme", + project: "web", + }, + warnings: [`Extra arguments ignored: extra. Expected: ${HINT}`], + }); + }); +}); + +describe("trace arguments, recovering a malformed trace ID", () => { + const HINT = "sentry trace view [/[/]]"; + const resolve = (args: readonly string[]) => + resolveTraceArguments(parseTraceArgumentsInput(args, HINT)); + + beforeEach(() => { + vi.spyOn(region, "resolveEffectiveOrg").mockImplementation( + async (org) => org, + ); + }); + + afterEach(() => { + vi.mocked(region.resolveEffectiveOrg).mockRestore(); + }); + + test("parsing keeps the trace ID as typed", () => { + expect(parseTraceArgumentsInput(["acme/web", "nothex"], HINT)).toEqual({ + parsed: { + type: "explicit", + org: "acme", + project: "web", + traceId: "nothex", + }, + targetArg: "acme/web", + warnings: [], + }); + }); + + test("resolving normalizes a valid trace ID", async () => { + await expect(resolve([VALID_TRACE_ID.toUpperCase()])).resolves.toEqual({ + type: "auto-detect", + traceId: VALID_TRACE_ID, + }); + }); + + test("recovers a malformed trace ID after the org", async () => { + await expect(resolve([`acme/${VALID_TRACE_ID}xyz`])).resolves.toEqual({ + type: "org-scoped", + traceId: VALID_TRACE_ID, + org: "acme", + }); + }); + + test("recovers a trace ID with a URL fragment prefix after the org", async () => { + await expect(resolve([`acme/trace-${VALID_TRACE_ID}`])).resolves.toEqual({ + type: "org-scoped", + traceId: VALID_TRACE_ID, + org: "acme", + }); + }); + + test("a partial trace ID after the org reaches recovery", async () => { + // Without a project there is nothing to look the prefix up in, so + // recovery finds no match, rather than the trace ID being missing. + const prefix = VALID_TRACE_ID.slice(0, 12); + await expect(resolve([`acme/${prefix}`])).rejects.toMatchObject({ + constructor: ResolutionError, + resource: `Trace '${prefix}'`, + headline: "not found", + }); + }); + + test("/ is still missing the trace ID", () => { + expect(() => parseTraceArgumentsInput(["acme/web"], HINT)).toThrow( + ContextError, + ); + }); +}); diff --git a/scripts/cli-lint-rules.test.mjs b/scripts/cli-lint-rules.test.mjs index 46bc0c351..a0e397338 100644 --- a/scripts/cli-lint-rules.test.mjs +++ b/scripts/cli-lint-rules.test.mjs @@ -124,6 +124,42 @@ tester.run("no-silent-catch", plugin.rules["no-silent-catch"], { ], }); +tester.run("no-ad-hoc-id-arguments", plugin.rules["no-ad-hoc-id-arguments"], { + valid: [ + { + filename: commands, + code: "import { HEX_ID_RE } from './hex-id.js'; parseIdArguments(args, ID_KINDS.trace, options);", + }, + { + // Issue and event IDs keep their own syntax + filename: "/repo/packages/cli/src/commands/event/view.ts", + code: "parseSlashSeparatedArg(arg, label, hint);", + }, + { + filename: "/repo/packages/cli/src/lib/id-arguments.ts", + code: "parseIdArguments(args, kind, options);", + }, + ], + invalid: [ + { + filename: commands, + code: "parseSlashSeparatedArg(arg, label, hint);", + errors: 1, + }, + { + filename: commands, + code: "const swapped = detectSwappedViewArgs(first, second);", + errors: 1, + }, + { filename: commands, code: "HEX_ID_RE.test(value);", errors: 1 }, + { + filename: source, + code: "parseIdArguments(args, { label: 'Thing ID', isId }, options);", + errors: 1, + }, + ], +}); + tester.run("no-raw-metadata-queries", plugin.rules["no-raw-metadata-queries"], { valid: [ {