From e5211d82c2b39cd51ec1c0d1f53de42b050dd512 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:50:07 +0000 Subject: [PATCH] ref(cli): Parse trace logs and span list arguments before they run Move `trace logs` and `span list` onto `boundArguments`, like `trace view`, `span view` and `log list`. The parse is pure and runs before the auth guard; the resolver applies a pasted URL's server and the warnings, then resolves the org (and project). With no callers left, remove `parseTraceTarget` and `parseDualModeArgs`; tests use the side-effect-free `parseTraceTargetInput` and `parseDualModeInput`. Part of #1564. Co-Authored-By: BYK --- packages/cli/lint-rules/cli-oxlint-plugin.js | 2 + packages/cli/src/commands/span/list.ts | 152 ++++++++++++------ packages/cli/src/commands/trace/logs.ts | 79 ++++++--- packages/cli/src/lib/trace-target.ts | 83 +++------- packages/cli/test/commands/span/list.test.ts | 128 +++++++++------ packages/cli/test/commands/trace/logs.test.ts | 31 +++- .../test/commands/trace/view.property.test.ts | 28 ++-- packages/cli/test/lib/help-positional.test.ts | 8 + packages/cli/test/lib/trace-target.test.ts | 96 ++++++----- 9 files changed, 366 insertions(+), 241 deletions(-) diff --git a/packages/cli/lint-rules/cli-oxlint-plugin.js b/packages/cli/lint-rules/cli-oxlint-plugin.js index 22b7c2cf0..97a95a26b 100644 --- a/packages/cli/lint-rules/cli-oxlint-plugin.js +++ b/packages/cli/lint-rules/cli-oxlint-plugin.js @@ -69,7 +69,9 @@ const BOUND_TARGET_TYPES = new Set([ "LogListArguments", "LogViewArguments", "ReplayArguments", + "SpanListArguments", "SpanViewArguments", + "TraceLogsArguments", "TraceViewArguments", ]); diff --git a/packages/cli/src/commands/span/list.ts b/packages/cli/src/commands/span/list.ts index 2ac60e728..28c83633e 100644 --- a/packages/cli/src/commands/span/list.ts +++ b/packages/cli/src/commands/span/list.ts @@ -7,14 +7,19 @@ * - **Project mode** (no trace ID): lists spans across the entire project * - **Trace mode** (trace ID provided): lists spans within a specific trace * - * Disambiguation uses {@link isTraceId} to detect 32-char hex trace IDs. + * Disambiguation uses {@link parseDualModeInput}: trace mode when the + * arguments name a trace ID. */ import type { SentryContext } from "../../context.js"; import type { SpanSortValue } from "../../lib/api/traces.js"; import { listSpans } from "../../lib/api-client.js"; -import { validateLimit } from "../../lib/arg-parsing.js"; -import { parseOrgProjectTarget } from "../../lib/completion-target.js"; +import { + type ParsedOrgProject, + parseOrgProjectInput, + validateLimit, +} from "../../lib/arg-parsing.js"; +import { ORG_PROJECT_TARGET } from "../../lib/completion-target.js"; import { advancePaginationState, buildPaginationContextKey, @@ -42,11 +47,16 @@ import { TARGET_PATTERN_NOTE, } from "../../lib/list-command.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 { applySentryUrlContext } from "../../lib/sentry-url-parser.js"; import { appendPeriodHint, serializeTimeRange, @@ -54,9 +64,11 @@ import { timeRangeToApiParams, } from "../../lib/time-range.js"; import { - type ParsedTraceTarget, - parseDualModeArgs, + applyTraceTargetInput, + parseDualModeInput, + type ResolvedTraceOrgProject, resolveTraceOrgProject, + type TraceTargetInput, warnIfNormalized, } from "../../lib/trace-target.js"; import { SpanListItemSchema } from "../../types/index.js"; @@ -188,16 +200,66 @@ export function parseSort(value: string): SpanSortValue { return value as SpanSortValue; } +/** What `span list` lists, as parsed: a project's spans or a trace's. */ +type SpanListInput = + | { + readonly mode: "project"; + readonly target: ParsedOrgProject; + /** Server of a pasted Sentry URL, for `applySentryUrlContext` */ + readonly baseUrl?: string; + } + | { readonly mode: "trace"; readonly trace: TraceTargetInput }; + +/** What `span list` lists, resolved. */ +type SpanListTarget = + | { readonly mode: "project"; readonly org: string; readonly project: string } + | ({ readonly mode: "trace" } & ResolvedTraceOrgProject); + +/** The arguments of `span list`. */ +type SpanListArguments = BoundArguments; + /** - * Disambiguate span list positional arguments. + * Disambiguate span list positional arguments with + * {@link parseDualModeInput}, without side effects. Both modes need a + * project, so `/` names project ``. * - * Thin wrapper around {@link parseDualModeArgs} that binds the - * trace-mode usage hint for span list. + * @internal Exported for testing */ -export function parseSpanListArgs( - args: string[], -): ReturnType { - return parseDualModeArgs(args, TRACE_USAGE_HINT, { needsProject: true }); +export function parseSpanListInput(args: readonly string[]): SpanListInput { + const input = parseDualModeInput(args, TRACE_USAGE_HINT, { + needsProject: true, + }); + if (input.mode === "trace") { + return input; + } + const { parsed, baseUrl } = parseOrgProjectInput(input.target); + return { mode: "project", target: parsed, baseUrl }; +} + +/** Resolve `span list` arguments to the project, or the trace and project. */ +async function resolveSpanListTarget( + input: SpanListInput, + context: SentryContext, +): Promise { + if (input.mode === "trace") { + const parsed = applyTraceTargetInput(input.trace); + warnIfNormalized(parsed, "span.list"); + const resolved = await resolveTraceOrgProject( + parsed, + context.cwd, + TRACE_USAGE_HINT, + ); + return { mode: "trace", ...resolved }; + } + if (input.baseUrl) { + applySentryUrlContext(input.baseUrl); + } + const { org, project } = await resolveProjectBoundTarget( + input.target, + context.cwd, + COMMAND_NAME, + ); + return { mode: "project", org, project }; } // --------------------------------------------------------------------------- @@ -348,7 +410,6 @@ function jsonTransformSpanList(data: SpanListData, fields?: string[]): unknown { /** Shared context passed to mode handlers from the Stricli command function. */ type ModeContext = { - cwd: string; flags: ListFlags; /** Parsed time range from the --period flag */ timeRange: TimeRange; @@ -359,20 +420,14 @@ type ModeContext = { /** * Handle trace mode: list spans within a specific trace. * - * Resolves the trace target, builds a query prefixed with `trace:{id}`, - * and returns the result with a trace-specific header and hints. + * Builds a query prefixed with `trace:{id}` and returns the result with a + * trace-specific header and hints. */ async function handleTraceMode( - parsed: ParsedTraceTarget, + { traceId, org, project }: ResolvedTraceOrgProject, ctx: ModeContext, ): Promise<{ output: SpanListData; hint?: string }> { - const { flags, cwd, extraApiFields, timeRange } = ctx; - warnIfNormalized(parsed, "span.list"); - const { traceId, org, project } = await resolveTraceOrgProject( - parsed, - cwd, - TRACE_USAGE_HINT, - ); + const { flags, extraApiFields, timeRange } = ctx; const queryParts = [`trace:${traceId}`]; if (flags.query) { queryParts.push(translateSpanQuery(flags.query)); @@ -449,19 +504,13 @@ async function handleTraceMode( /** * Handle project mode: list spans across the entire project. * - * Resolves the org/project target and queries the spans dataset - * without a trace ID filter. + * Queries the spans dataset without a trace ID filter. */ async function handleProjectMode( - target: string | undefined, + { org, project }: { readonly org: string; readonly project: string }, ctx: ModeContext, ): Promise<{ output: SpanListData; hint?: string }> { - const { flags, cwd, extraApiFields, timeRange } = ctx; - const { org, project } = await resolveProjectBoundFromArg( - target, - cwd, - COMMAND_NAME, - ); + const { flags, extraApiFields, timeRange } = ctx; // Resolve slug → numeric ID so the Events query scopes via the `project` // param. `project:` only matches actively-selected projects (#1317). const projectId = await resolveLogProjectId(org, project); @@ -607,14 +656,13 @@ export const listCommand = buildListCommand("span", { schema: SpanListItemSchema, }, parameters: { - positional: { - kind: "array", - parameter: { - placeholder: "org/project/trace-id", - brief: "[/] or [//]", - parse: parseOrgProjectTarget, - }, - }, + positional: boundArguments({ + placeholder: "org/project/trace-id", + brief: "[/] or [//]", + kind: ORG_PROJECT_TARGET, + parse: parseSpanListInput, + resolve: resolveSpanListTarget, + }), flags: { limit: { kind: "parsed", @@ -644,22 +692,20 @@ export const listCommand = buildListCommand("span", { s: "sort", }, }, - async *func(this: SentryContext, flags: ListFlags, ...args: string[]) { - const { cwd } = this; + async *func( + this: SentryContext, + flags: ListFlags, + target: SpanListArguments, + ) { const timeRange = flags.period; - const parsed = parseSpanListArgs(args); const extraApiFields = extractExtraApiFields(flags.fields); - const modeCtx: ModeContext = { - cwd, - flags, - timeRange, - extraApiFields, - }; + const modeCtx: ModeContext = { flags, timeRange, extraApiFields }; + const resolved = await target.resolved; const { output, hint } = - parsed.mode === "trace" - ? await handleTraceMode(parsed.parsed, modeCtx) - : await handleProjectMode(parsed.target, modeCtx); + resolved.mode === "trace" + ? await handleTraceMode(resolved, modeCtx) + : await handleProjectMode(resolved, modeCtx); yield new CommandOutput(output); return { hint }; diff --git a/packages/cli/src/commands/trace/logs.ts b/packages/cli/src/commands/trace/logs.ts index 0124ac3da..afe1e97b4 100644 --- a/packages/cli/src/commands/trace/logs.ts +++ b/packages/cli/src/commands/trace/logs.ts @@ -13,7 +13,7 @@ import { } 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 { toSearchQueryError } from "../../lib/errors.js"; import { filterFields } from "../../lib/formatters/json.js"; import { formatLogTable } from "../../lib/formatters/log.js"; @@ -25,6 +25,10 @@ import { LIST_MAX_LIMIT, } from "../../lib/list-command.js"; import { withProgress } from "../../lib/polling.js"; +import { + type BoundArguments, + boundArguments, +} from "../../lib/positional-parse.js"; import { sanitizeQuery } from "../../lib/search-query.js"; import { buildTraceUrl } from "../../lib/sentry-web-urls.js"; import { @@ -35,8 +39,11 @@ import { timeRangeToApiParams, } from "../../lib/time-range.js"; import { - parseTraceTarget, + applyTraceTargetInput, + parseTraceTargetInput, + type ResolvedTraceOrg, resolveTraceOrg, + type TraceTargetInput, warnIfNormalized, } from "../../lib/trace-target.js"; @@ -97,6 +104,42 @@ function parseLimit(value: string): number { return validateLimit(value, 1, LIST_MAX_LIMIT); } +/** `trace logs` arguments, resolved. */ +type TraceLogsTarget = ResolvedTraceOrg & { + /** Project the user named, to filter the trace's logs by */ + readonly projectFilter?: string; +}; + +/** The arguments of `trace logs`. */ +type TraceLogsArguments = BoundArguments; + +/** Parse `trace logs` arguments, without side effects. */ +function parseTraceLogsInput(args: readonly string[]): TraceTargetInput { + return parseTraceTargetInput(args, USAGE_HINT); +} + +/** + * Resolve `trace logs` arguments to the trace's org; a project the user + * named becomes a filter. + */ +async function resolveTraceLogsTarget( + input: TraceTargetInput, + context: SentryContext, +): Promise { + const parsed = applyTraceTargetInput(input); + warnIfNormalized(parsed, "trace.logs"); + const { traceId, org } = await resolveTraceOrg( + parsed, + context.cwd, + USAGE_HINT, + ); + return { + traceId, + org, + projectFilter: parsed.type === "explicit" ? parsed.project : undefined, + }; +} + export const logsCommand = buildCommand({ docs: { brief: "View logs associated with a trace", @@ -151,15 +194,14 @@ export const logsCommand = buildCommand({ }, }, parameters: { - positional: { - kind: "array", - parameter: { - placeholder: "org/project/trace-id", - brief: - "[/[/]] - Optional org/project and required trace ID", - parse: parseOrgProjectTarget, - }, - }, + positional: boundArguments({ + placeholder: "org/project/trace-id", + brief: + "[/[/]] - Optional org/project and required trace ID", + kind: ORG_PROJECT_TARGET, + parse: parseTraceLogsInput, + resolve: resolveTraceLogsTarget, + }), flags: { web: { kind: "boolean", @@ -202,17 +244,14 @@ export const logsCommand = buildCommand({ s: "sort", }, }, - async *func(this: SentryContext, flags: LogsFlags, ...args: string[]) { + async *func( + this: SentryContext, + flags: LogsFlags, + target: TraceLogsArguments, + ) { applyFreshFlag(flags); - const { cwd } = this; const timeRange = flags.period; - - // Parse and resolve org/trace-id (project captured for filtering) - const parsed = parseTraceTarget(args, USAGE_HINT); - warnIfNormalized(parsed, "trace.logs"); - const { traceId, org } = await resolveTraceOrg(parsed, cwd, USAGE_HINT); - const projectFilter = - parsed.type === "explicit" ? parsed.project : undefined; + const { traceId, org, projectFilter } = await target.resolved; if (flags.web) { await openInBrowser(buildTraceUrl(org, traceId), "trace"); diff --git a/packages/cli/src/lib/trace-target.ts b/packages/cli/src/lib/trace-target.ts index ab0efd3b7..abaffff06 100644 --- a/packages/cli/src/lib/trace-target.ts +++ b/packages/cli/src/lib/trace-target.ts @@ -130,28 +130,6 @@ export type ResolvedTraceOrg = { // Parsing // --------------------------------------------------------------------------- -/** - * Parse trace-related positional arguments into a {@link ParsedTraceTarget}. - * - * 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 - * @returns Parsed trace target with type discrimination - * @throws {ContextError} If no arguments are provided - * @throws {ValidationError} If the trace ID format is invalid - */ -export function parseTraceTarget( - args: readonly string[], - usageHint: string, - options: TraceArgumentsOptions = {}, -): ParsedTraceTarget { - return applyTraceTargetInput(parseTraceTargetInput(args, usageHint, options)); -} - /** How a trace command takes its trace ID. */ export type TraceArgumentsOptions = { /** @@ -162,9 +140,17 @@ export type TraceArgumentsOptions = { }; /** - * Parse trace-related positional arguments like {@link parseTraceTarget}, + * Parse trace-related positional arguments into a {@link ParsedTraceTarget}, * without side effects: the server of a pasted Sentry URL and the warnings - * are returned for the caller to apply. + * are returned for the caller to apply ({@link applyTraceTargetInput}). + * + * Takes the forms every ID-taking command does (see `id-arguments.ts`): + * ``, `/`, `//`, + * ` `, ` `, or a trace URL. Extra + * arguments are ignored with a warning. + * + * @throws {ContextError} When the trace ID is missing + * @throws {ValidationError} When the trace ID or the target is invalid */ export function parseTraceTargetInput( args: readonly string[], @@ -535,51 +521,22 @@ export async function resolveTraceOrg( // --------------------------------------------------------------------------- /** - * Result from dual-mode argument disambiguation. - * - * Used by commands that support both project-scoped listing (no trace ID) - * and trace-scoped listing (trace ID provided), like `span list` and - * `log list`. - * - * Discriminated on `mode`: - * - `"project"` — no trace ID detected; `target` is the optional org/project arg - * - `"trace"` — a 32-char hex trace ID was found; `parsed` contains the full target - */ -export type ParsedDualModeArgs = - | { mode: "project"; target?: string } - | { mode: "trace"; parsed: ParsedTraceTarget }; - -/** - * Disambiguate positional arguments for dual-mode list commands. - * - * 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 + * Positional arguments of a dual-mode list command, parsed without side + * effects. Discriminated on `mode`: + * - `"project"` — no trace ID; `target` is the optional org/project argument + * - `"trace"` — a trace ID; `trace` is the target to apply and resolve */ -export function parseDualModeArgs( - args: readonly string[], - traceUsageHint: string, - options: TraceArgumentsOptions = {}, -): ParsedDualModeArgs { - const input = parseDualModeInput(args, traceUsageHint, options); - return input.mode === "trace" - ? { mode: "trace", parsed: applyTraceTargetInput(input.trace) } - : input; -} - -/** {@link ParsedDualModeArgs} parsed without side effects. */ export type DualModeInput = | { mode: "project"; target?: string } | { mode: "trace"; trace: TraceTargetInput }; /** - * Disambiguate dual-mode arguments like {@link parseDualModeArgs}, without - * side effects: trace mode returns a {@link TraceTargetInput} for the caller - * to apply. + * Disambiguate positional arguments for dual-mode list commands (`span list`, + * `log list`), without side effects. + * + * Trace mode when the arguments name a trace ID in any of the forms + * {@link parseTraceTargetInput} takes; otherwise project mode, with the + * first argument as the target. */ export function parseDualModeInput( args: readonly string[], diff --git a/packages/cli/test/commands/span/list.test.ts b/packages/cli/test/commands/span/list.test.ts index 7a8986362..5f84c175e 100644 --- a/packages/cli/test/commands/span/list.test.ts +++ b/packages/cli/test/commands/span/list.test.ts @@ -3,7 +3,7 @@ * * Tests for the dual-mode span list command: * - parseSort: sort flag validation - * - parseSpanListArgs: positional argument disambiguation (project vs trace mode) + * - parseSpanListInput: positional argument disambiguation (project vs trace mode) * - listCommand.func (trace mode): existing trace-scoped behavior * - listCommand.func (project mode): new project-scoped behavior */ @@ -12,7 +12,7 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { listCommand, parseSort, - parseSpanListArgs, + parseSpanListInput, } from "../../../src/commands/span/list.js"; vi.mock("../../../src/lib/api-client.js", async (importOriginal) => { @@ -64,7 +64,7 @@ const VALID_TRACE_ID = "aaaa1111bbbb2222cccc3333dddd4444"; // Reference paginationDb early to prevent import stripping by auto-organize const _paginationDbRef = paginationDb; -// Note: parseTraceTarget parsing tests are in test/lib/trace-target.test.ts +// Note: trace-target parsing tests are in test/lib/trace-target.test.ts // ============================================================================ // parseSort @@ -93,63 +93,98 @@ describe("parseSort", () => { }); // ============================================================================ -// parseSpanListArgs — positional argument disambiguation +// parseSpanListInput — positional argument disambiguation // ============================================================================ -describe("parseSpanListArgs", () => { - test("no args → project mode", () => { - const result = parseSpanListArgs([]); - expect(result).toEqual({ mode: "project" }); +describe("parseSpanListInput", () => { + test("no args → project mode, auto-detected", () => { + expect(parseSpanListInput([])).toEqual({ + mode: "project", + target: { type: "auto-detect" }, + }); }); test("org/project → project mode with target", () => { - const result = parseSpanListArgs(["my-org/my-project"]); - expect(result.mode).toBe("project"); - if (result.mode === "project") { - expect(result.target).toBe("my-org/my-project"); - } + expect(parseSpanListInput(["my-org/my-project"])).toEqual({ + mode: "project", + target: { type: "explicit", org: "my-org", project: "my-project" }, + }); }); - test("bare project name → project mode with target", () => { - const result = parseSpanListArgs(["my-project"]); - expect(result.mode).toBe("project"); - if (result.mode === "project") { - expect(result.target).toBe("my-project"); - } + test("bare project name → project mode, searched", () => { + expect(parseSpanListInput(["my-project"])).toMatchObject({ + mode: "project", + target: { type: "project-search", projectSlug: "my-project" }, + }); }); test("trace ID → trace mode", () => { - const result = parseSpanListArgs([VALID_TRACE_ID]); - expect(result.mode).toBe("trace"); + expect(parseSpanListInput([VALID_TRACE_ID])).toEqual({ + mode: "trace", + trace: { + parsed: { type: "auto-detect", traceId: VALID_TRACE_ID }, + warnings: [], + }, + }); }); test("org/project/trace-id → trace mode", () => { - const result = parseSpanListArgs([`my-org/my-project/${VALID_TRACE_ID}`]); - expect(result.mode).toBe("trace"); + expect( + parseSpanListInput([`my-org/my-project/${VALID_TRACE_ID}`]), + ).toMatchObject({ + mode: "trace", + trace: { + parsed: { + type: "explicit", + traceId: VALID_TRACE_ID, + org: "my-org", + project: "my-project", + }, + }, + }); + }); + + test("/trace-id names project , since span list needs one", () => { + expect(parseSpanListInput([`frontend/${VALID_TRACE_ID}`])).toMatchObject({ + mode: "trace", + trace: { parsed: { type: "project-search", projectSlug: "frontend" } }, + }); }); test("project + trace-id (space-separated) → trace mode", () => { - const result = parseSpanListArgs(["my-project", VALID_TRACE_ID]); - expect(result.mode).toBe("trace"); + expect(parseSpanListInput(["my-project", VALID_TRACE_ID]).mode).toBe( + "trace", + ); }); test("org/project + trace-id (space-separated) → trace mode", () => { - const result = parseSpanListArgs(["my-org/my-project", VALID_TRACE_ID]); - expect(result.mode).toBe("trace"); + expect(parseSpanListInput(["my-org/my-project", VALID_TRACE_ID]).mode).toBe( + "trace", + ); }); test("short non-hex string → project mode", () => { - const result = parseSpanListArgs(["frontend"]); - expect(result.mode).toBe("project"); - if (result.mode === "project") { - expect(result.target).toBe("frontend"); - } + expect(parseSpanListInput(["frontend"]).mode).toBe("project"); }); test("32-char non-hex string → project mode", () => { // 32 chars but not valid hex - const result = parseSpanListArgs(["abcdefghijklmnopqrstuvwxyz123456"]); - expect(result.mode).toBe("project"); + expect(parseSpanListInput(["abcdefghijklmnopqrstuvwxyz123456"]).mode).toBe( + "project", + ); + }); + + test("a pasted URL's server is returned, not applied", () => { + const savedUrl = process.env.SENTRY_URL; + expect( + parseSpanListInput([ + `https://sentry.example.com/organizations/acme/traces/${VALID_TRACE_ID}/`, + ]), + ).toMatchObject({ + mode: "trace", + trace: { baseUrl: "https://sentry.example.com" }, + }); + expect(process.env.SENTRY_URL).toBe(savedUrl); }); }); @@ -585,9 +620,10 @@ describe("listCommand.func (trace mode)", () => { describe("listCommand.func (project mode)", () => { let func: ListFunc; - // span/list.ts calls resolveProjectBoundFromArg (not resolveOrgAndProject) + // Project mode resolves with resolveProjectBoundTarget (not + // resolveOrgAndProject) const resolveOrgAndProjectSpy = vi.mocked( - resolveTarget.resolveProjectBoundFromArg, + resolveTarget.resolveProjectBoundTarget, ); const listSpansSpy = vi.mocked(apiClient.listSpans); const resolveCursorSpy = vi.mocked(paginationDb.resolveCursor); @@ -618,16 +654,12 @@ describe("listCommand.func (project mode)", () => { beforeEach(async () => { func = (await listCommand.loader()) as unknown as ListFunc; - // Mock resolveProjectBoundFromArg to parse explicit "org/project" targets - // and fall back to a default for auto-detect (no target). - resolveOrgAndProjectSpy.mockImplementation( - async (target: string | undefined) => { - if (target?.includes("/")) { - const [org, project] = target.split("/"); - return { org: org!, project: project! }; - } - return { org: "test-org", project: "test-project" }; - }, + // Resolve explicit "org/project" targets as typed and fall back to a + // default for auto-detect (no target). + resolveOrgAndProjectSpy.mockImplementation(async (target) => + target.type === "explicit" + ? { org: target.org, project: target.project } + : { org: "test-org", project: "test-project" }, ); vi.spyOn(resolveTarget, "resolveLogProjectId").mockResolvedValue(4242); resolveCursorSpy.mockReturnValue({ @@ -702,9 +734,9 @@ describe("listCommand.func (project mode)", () => { "my-project", expect.anything(), ); - // resolveProjectBoundFromArg is called with the explicit target + // The explicit target is parsed before it is resolved expect(resolveOrgAndProjectSpy).toHaveBeenCalledWith( - "my-org/my-project", + { type: "explicit", org: "my-org", project: "my-project" }, expect.any(String), expect.any(String), ); diff --git a/packages/cli/test/commands/trace/logs.test.ts b/packages/cli/test/commands/trace/logs.test.ts index 7c31cafa6..c6fa16b8f 100644 --- a/packages/cli/test/commands/trace/logs.test.ts +++ b/packages/cli/test/commands/trace/logs.test.ts @@ -20,6 +20,7 @@ import { test, vi, } from "vitest"; +import { ArgumentParseError } from "@stricli/core"; import { logsCommand } from "../../../src/commands/trace/logs.js"; vi.mock("../../../src/lib/api-client.js", async (importOriginal) => { @@ -49,7 +50,7 @@ vi.mock("../../../src/lib/browser.js", async (importOriginal) => { // oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking import * as browser from "../../../src/lib/browser.js"; -import { ContextError } from "../../../src/lib/errors.js"; +import { ContextError, ValidationError } from "../../../src/lib/errors.js"; vi.mock("../../../src/lib/polling.js", async (importOriginal) => { const actual = @@ -81,7 +82,7 @@ import * as resolveTarget from "../../../src/lib/resolve-target.js"; import { parsePeriod } from "../../../src/lib/time-range.js"; import type { TraceLog } from "../../../src/types/sentry.js"; -// Note: parseTraceTarget parsing tests are in test/lib/trace-target.test.ts +// Note: trace-target parsing tests are in test/lib/trace-target.test.ts const TRACE_ID = "aaaa1111bbbb2222cccc3333dddd4444"; const ORG = "test-org"; @@ -467,6 +468,32 @@ describe("logsCommand.func", () => { ), ).rejects.toThrow(ContextError); }); + + test.each([ + ["a missing trace ID", "my-org/web", ContextError], + ["an invalid trace ID", "not-a-trace-id", ValidationError], + ])("%s fails to parse, before any lookup", async (_, arg, cause) => { + const { context } = createMockContext(); + const func = await logsCommand.loader(); + + const error = await func + .call( + context, + { + json: false, + web: false, + period: parsePeriod("14d"), + limit: 100, + sort: "newest", + }, + arg, + ) + .catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ArgumentParseError); + expect((error as ArgumentParseError).exception).toBeInstanceOf(cause); + expect(resolveOrgSpy).not.toHaveBeenCalled(); + expect(listTraceLogsSpy).not.toHaveBeenCalled(); + }); }); describe("flag forwarding to API", () => { diff --git a/packages/cli/test/commands/trace/view.property.test.ts b/packages/cli/test/commands/trace/view.property.test.ts index 53abd4de3..9f80f0818 100644 --- a/packages/cli/test/commands/trace/view.property.test.ts +++ b/packages/cli/test/commands/trace/view.property.test.ts @@ -1,7 +1,7 @@ /** * Property-Based Tests for Trace Target Parsing * - * Uses fast-check to verify invariants of parseTraceTarget() + * Uses fast-check to verify invariants of parseTraceTargetInput() * that should hold for any valid input. These tests cover the * shared abstraction used by trace view, span list, span view, * and trace logs. @@ -18,7 +18,7 @@ import { import { describe, expect, test } from "vitest"; import { flattenSpanTree } from "../../../src/commands/trace/view.js"; import { ContextError, ValidationError } from "../../../src/lib/errors.js"; -import { parseTraceTarget } from "../../../src/lib/trace-target.js"; +import { parseTraceTargetInput } from "../../../src/lib/trace-target.js"; import type { TraceSpan } from "../../../src/types/sentry.js"; import { DEFAULT_NUM_RUNS } from "../../model-based/helpers.js"; @@ -39,11 +39,11 @@ function toUuidFormat(hex: string): string { return `${hex.slice(0, 8)}-${hex.slice(8, 12)}-${hex.slice(12, 16)}-${hex.slice(16, 20)}-${hex.slice(20)}`; } -describe("parseTraceTarget properties", () => { +describe("parseTraceTargetInput properties", () => { test("single valid trace ID: returns auto-detect with correct traceId", async () => { fcAssert( property(traceIdArb, (input) => { - const result = parseTraceTarget([input], HINT); + const result = parseTraceTargetInput([input], HINT).parsed; expect(result.type).toBe("auto-detect"); expect(result.traceId).toBe(input); }), @@ -57,7 +57,7 @@ describe("parseTraceTarget properties", () => { tuple(slugArb, slugArb, traceIdArb), ([org, project, traceId]) => { const combined = `${org}/${project}/${traceId}`; - const result = parseTraceTarget([combined], HINT); + const result = parseTraceTargetInput([combined], HINT).parsed; expect(result.type).toBe("explicit"); expect(result.traceId).toBe(traceId); if (result.type === "explicit") { @@ -74,7 +74,7 @@ describe("parseTraceTarget properties", () => { fcAssert( property(tuple(slugArb, traceIdArb), ([org, traceId]) => { const combined = `${org}/${traceId}`; - const result = parseTraceTarget([combined], HINT); + const result = parseTraceTargetInput([combined], HINT).parsed; expect(result.type).toBe("org-scoped"); expect(result.traceId).toBe(traceId); if (result.type === "org-scoped") { @@ -91,7 +91,7 @@ describe("parseTraceTarget properties", () => { tuple(slugArb, slugArb, traceIdArb), ([org, project, traceId]) => { const target = `${org}/${project}`; - const result = parseTraceTarget([target, traceId], HINT); + const result = parseTraceTargetInput([target, traceId], HINT).parsed; expect(result.type).toBe("explicit"); expect(result.traceId).toBe(traceId); }, @@ -104,8 +104,8 @@ describe("parseTraceTarget properties", () => { fcAssert( property(tuple(slugArb, traceIdArb), ([target, traceId]) => { const args = [target, traceId]; - const result1 = parseTraceTarget(args, HINT); - const result2 = parseTraceTarget(args, HINT); + const result1 = parseTraceTargetInput(args, HINT).parsed; + const result2 = parseTraceTargetInput(args, HINT).parsed; expect(result1).toEqual(result2); }), { numRuns: DEFAULT_NUM_RUNS }, @@ -113,13 +113,13 @@ describe("parseTraceTarget properties", () => { }); test("empty args always throws ContextError", () => { - expect(() => parseTraceTarget([], HINT)).toThrow(ContextError); + expect(() => parseTraceTargetInput([], HINT).parsed).toThrow(ContextError); }); test("result always has traceId property defined", async () => { fcAssert( property(traceIdArb, (traceId) => { - const result = parseTraceTarget([traceId], HINT); + const result = parseTraceTargetInput([traceId], HINT).parsed; expect(result.traceId).toBeDefined(); expect(typeof result.traceId).toBe("string"); }), @@ -131,7 +131,7 @@ describe("parseTraceTarget properties", () => { fcAssert( property(traceIdArb, (hex) => { const uuid = toUuidFormat(hex); - const result = parseTraceTarget([uuid], HINT); + const result = parseTraceTargetInput([uuid], HINT).parsed; expect(result.traceId).toBe(hex); }), { numRuns: DEFAULT_NUM_RUNS }, @@ -142,7 +142,9 @@ describe("parseTraceTarget properties", () => { const invalidIdArb = stringMatching(/^[g-z]{10,20}$/); fcAssert( property(invalidIdArb, (badId) => { - expect(() => parseTraceTarget([badId], HINT)).toThrow(ValidationError); + expect(() => parseTraceTargetInput([badId], HINT).parsed).toThrow( + ValidationError, + ); }), { numRuns: DEFAULT_NUM_RUNS }, ); diff --git a/packages/cli/test/lib/help-positional.test.ts b/packages/cli/test/lib/help-positional.test.ts index 40429805f..a85f2e0bd 100644 --- a/packages/cli/test/lib/help-positional.test.ts +++ b/packages/cli/test/lib/help-positional.test.ts @@ -198,6 +198,14 @@ describe("help recovery on argument parse errors", () => { expect(stderr).not.toContain("Failed to parse"); }); + test("sentry trace logs help → shows help instead of the parse error", async () => { + const { stdout, stderr } = await runCommand(["trace", "logs", "help"]); + + expect(stdout).toContain("sentry trace logs"); + expect(stderr).toContain("sentry trace logs --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 diff --git a/packages/cli/test/lib/trace-target.test.ts b/packages/cli/test/lib/trace-target.test.ts index e2af2a222..2ac3d92da 100644 --- a/packages/cli/test/lib/trace-target.test.ts +++ b/packages/cli/test/lib/trace-target.test.ts @@ -1,7 +1,7 @@ /** * Tests for shared trace-target parsing and resolution. * - * Tests parseTraceTarget, the dual-mode parsers, and the resolvers from + * Tests the trace-target and dual-mode parsers, and the resolvers from * src/lib/trace-target.ts. The syntax itself is tested in * id-arguments.test.ts. */ @@ -38,9 +38,8 @@ import { // oxlint-disable-next-line sentry-cli/no-namespace-import -- needed for spyOn mocking import * as region from "../../src/lib/region.js"; import { - parseDualModeArgs, + type ParsedTraceTarget, parseDualModeInput, - parseTraceTarget, parseTraceArgumentsInput, parseTraceTargetInput, resolveTraceArguments, @@ -51,22 +50,33 @@ import { const VALID_TRACE_ID = "aaaa1111bbbb2222cccc3333dddd4444"; -describe("parseTraceTarget with one argument", () => { +/** The target {@link parseTraceTargetInput} parses `args` into. */ +function parsedTraceTarget( + args: readonly string[], + usageHint: string, +): ParsedTraceTarget { + return parseTraceTargetInput(args, usageHint).parsed; +} + +describe("parseTraceTargetInput with one argument", () => { const HINT = "sentry span list [//]"; test("bare trace ID → auto-detect", () => { - const result = parseTraceTarget([VALID_TRACE_ID], HINT); + const result = parsedTraceTarget([VALID_TRACE_ID], HINT); expect(result.type).toBe("auto-detect"); expect(result.traceId).toBe(VALID_TRACE_ID); }); test("normalizes uppercase trace ID", () => { - const result = parseTraceTarget(["AAAA1111BBBB2222CCCC3333DDDD4444"], HINT); + const result = parsedTraceTarget( + ["AAAA1111BBBB2222CCCC3333DDDD4444"], + HINT, + ); expect(result.traceId).toBe(VALID_TRACE_ID); }); test("strips UUID dashes from trace ID", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( ["aaaa1111-bbbb-2222-cccc-3333dddd4444"], HINT, ); @@ -74,7 +84,7 @@ describe("parseTraceTarget with one argument", () => { }); test("org/trace-id → org-scoped", () => { - const result = parseTraceTarget([`my-org/${VALID_TRACE_ID}`], HINT); + const result = parsedTraceTarget([`my-org/${VALID_TRACE_ID}`], HINT); expect(result.type).toBe("org-scoped"); expect(result.traceId).toBe(VALID_TRACE_ID); if (result.type === "org-scoped") { @@ -83,7 +93,7 @@ describe("parseTraceTarget with one argument", () => { }); test("org/project/trace-id → explicit", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( [`my-org/my-project/${VALID_TRACE_ID}`], HINT, ); @@ -98,7 +108,7 @@ describe("parseTraceTarget with one argument", () => { 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 = parseTraceTarget( + const result = parsedTraceTarget( [`my_org/my_project/${VALID_TRACE_ID}`], HINT, ); @@ -111,19 +121,19 @@ describe("parseTraceTarget with one argument", () => { }); test("trailing slash without trace ID throws", () => { - expect(() => parseTraceTarget(["my-org/"], HINT)).toThrow(ContextError); + expect(() => parsedTraceTarget(["my-org/"], HINT)).toThrow(ContextError); }); test("invalid trace ID throws ValidationError", () => { - expect(() => parseTraceTarget(["not-a-trace-id"], HINT)).toThrow( + expect(() => parsedTraceTarget(["not-a-trace-id"], HINT)).toThrow( ValidationError, ); }); }); -describe("parseTraceTarget with a target argument", () => { +describe("parseTraceTargetInput with a target argument", () => { test("explicit target (org/project)", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( ["my-org/my-project", VALID_TRACE_ID], "sentry trace view ", ); @@ -132,7 +142,7 @@ describe("parseTraceTarget with a target argument", () => { }); test("org-all target (org/) → org-scoped", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( ["my-org/", VALID_TRACE_ID], "sentry trace view ", ); @@ -143,7 +153,7 @@ describe("parseTraceTarget with a target argument", () => { }); test("bare slug → project-search", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( ["frontend", VALID_TRACE_ID], "sentry trace view ", ); @@ -154,7 +164,7 @@ describe("parseTraceTarget with a target argument", () => { }); test("preserves scoped display-name metadata", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( ["my-org/My Project", VALID_TRACE_ID], "sentry trace view ", ); @@ -167,7 +177,7 @@ describe("parseTraceTarget with a target argument", () => { }); test("empty string → auto-detect", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( ["", VALID_TRACE_ID], "sentry trace view ", ); @@ -198,7 +208,7 @@ describe("trace target resolution", () => { ], orgs: [{ slug: "project-owner", name: "Project Owner" }], }); - const parsed = parseTraceTarget( + const parsed = parsedTraceTarget( ["frontend", VALID_TRACE_ID], "sentry trace view ", ); @@ -214,7 +224,7 @@ describe("trace target resolution", () => { test("project-bound errors preserve the full trace usage hint", async () => { findProjectsBySlugSpy.mockResolvedValue({ projects: [], orgs: [] }); - const parsed = parseTraceTarget( + const parsed = parsedTraceTarget( ["missing", VALID_TRACE_ID], "sentry trace view ", ); @@ -231,7 +241,7 @@ describe("trace target resolution", () => { test("org-capable errors preserve the full trace usage hint", async () => { findProjectsBySlugSpy.mockResolvedValue({ projects: [], orgs: [] }); - const parsed = parseTraceTarget( + const parsed = parsedTraceTarget( ["missing", VALID_TRACE_ID], "sentry trace view ", ); @@ -247,21 +257,21 @@ describe("trace target resolution", () => { }); }); -describe("parseTraceTarget", () => { +describe("parseTraceTargetInput", () => { const HINT = "sentry span list [//]"; test("empty args throws ContextError", () => { - expect(() => parseTraceTarget([], HINT)).toThrow(ContextError); + expect(() => parsedTraceTarget([], HINT)).toThrow(ContextError); }); test("single arg: bare trace ID → auto-detect", () => { - const result = parseTraceTarget([VALID_TRACE_ID], HINT); + const result = parsedTraceTarget([VALID_TRACE_ID], HINT); expect(result.type).toBe("auto-detect"); expect(result.traceId).toBe(VALID_TRACE_ID); }); test("single arg: org/project/trace-id → explicit", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( [`my-org/my-project/${VALID_TRACE_ID}`], HINT, ); @@ -270,12 +280,12 @@ describe("parseTraceTarget", () => { }); test("single arg: org/trace-id → org-scoped", () => { - const result = parseTraceTarget([`my-org/${VALID_TRACE_ID}`], HINT); + const result = parsedTraceTarget([`my-org/${VALID_TRACE_ID}`], HINT); expect(result.type).toBe("org-scoped"); }); test("two args: target + trace-id", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( ["my-org/my-project", VALID_TRACE_ID], HINT, ); @@ -284,13 +294,13 @@ describe("parseTraceTarget", () => { }); test("two args: bare slug + trace-id → project-search", () => { - const result = parseTraceTarget(["frontend", VALID_TRACE_ID], HINT); + const result = parsedTraceTarget(["frontend", VALID_TRACE_ID], HINT); expect(result.type).toBe("project-search"); expect(result.traceId).toBe(VALID_TRACE_ID); }); test("invalid trace ID throws ValidationError", () => { - expect(() => parseTraceTarget(["not-valid"], HINT)).toThrow( + expect(() => parsedTraceTarget(["not-valid"], HINT)).toThrow( ValidationError, ); }); @@ -320,7 +330,7 @@ describe("trailing slashes and Sentry trace URLs (CLI-13F)", () => { }); test("canonical trace URL with trailing slash → org-scoped", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( [`https://my-org.sentry.io/explore/traces/trace/${VALID_TRACE_ID}/`], HINT, ); @@ -332,7 +342,7 @@ describe("trailing slashes and Sentry trace URLs (CLI-13F)", () => { }); test("trace URL without trailing slash and with query → org-scoped", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( [ `https://my-org.sentry.io/explore/traces/trace/${VALID_TRACE_ID}?statsPeriod=24h`, ], @@ -346,7 +356,7 @@ describe("trailing slashes and Sentry trace URLs (CLI-13F)", () => { }); test("legacy /organizations/ trace URL → org-scoped", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( [`https://sentry.io/organizations/my-org/traces/${VALID_TRACE_ID}/`], HINT, ); @@ -356,12 +366,12 @@ describe("trailing slashes and Sentry trace URLs (CLI-13F)", () => { test("a Sentry URL without a trace ID is not a trace URL", () => { expect(() => - parseTraceTarget(["https://sentry.io/organizations/my-org/"], HINT), + parsedTraceTarget(["https://sentry.io/organizations/my-org/"], HINT), ).toThrow(ContextError); }); test("org/project// → explicit", () => { - const result = parseTraceTarget( + const result = parsedTraceTarget( [`my-org/my-project/${VALID_TRACE_ID}/`], HINT, ); @@ -374,29 +384,31 @@ describe("trailing slashes and Sentry trace URLs (CLI-13F)", () => { }); test("/ → auto-detect", () => { - const result = parseTraceTarget([`${VALID_TRACE_ID}/`], HINT); + const result = parsedTraceTarget([`${VALID_TRACE_ID}/`], HINT); expect(result).toEqual({ type: "auto-detect", traceId: VALID_TRACE_ID }); }); test("org/project/ still throws ContextError (trace ID omitted)", () => { - expect(() => parseTraceTarget(["my-org/my-project/"], HINT)).toThrow( + expect(() => parsedTraceTarget(["my-org/my-project/"], HINT)).toThrow( ContextError, ); }); - test("parseDualModeArgs treats a trace URL as trace mode", () => { - const result = parseDualModeArgs( + test("parseDualModeInput treats a trace URL as trace mode", () => { + const result = parseDualModeInput( [`https://my-org.sentry.io/explore/traces/trace/${VALID_TRACE_ID}/`], HINT, ); - expect(result).toEqual({ + expect(result).toMatchObject({ mode: "trace", - parsed: { type: "org-scoped", traceId: VALID_TRACE_ID, org: "my-org" }, + trace: { + parsed: { type: "org-scoped", traceId: VALID_TRACE_ID, org: "my-org" }, + }, }); }); - test("parseDualModeArgs treats org// as trace mode", () => { - const result = parseDualModeArgs([`my-org/${VALID_TRACE_ID}/`], HINT); + test("parseDualModeInput treats org// as trace mode", () => { + const result = parseDualModeInput([`my-org/${VALID_TRACE_ID}/`], HINT); expect(result.mode).toBe("trace"); }); });