Repository navigation
ref(cli): Parse every kind of ID argument with one grammar - #1714
Merged
Merged
Conversation
Follow-up to #1655 for #1564. Positionals whose meaning depends on how many arguments were given (`[<org>/<project>] <replay-id>`, `log list`'s trailing trace ID) kept parsing in `func`, because a per-element Stricli `parse` can't see the count. Our `@stricli/core` patch now lets an array positional declare `combine`: it gets every parsed element and returns the one value the command receives. A throw becomes an `ArgumentParseError`, so these commands fail in the scanner with exit 252 like other arguments. `boundArguments` (`src/lib/positional-parse.ts`) builds such a positional: a pure `parse` over all the arguments, bound to a `resolve` that runs after the auth guard, like `boundPositional`. `log list`, `replay view` and `replay download` move onto it. Parsing a pasted Sentry URL no longer applies its server; the parse returns `baseUrl` (and trace-target warnings), and `resolve` applies them. `parseTraceTargetInput` and `parseDualModeInput` are the side-effect-free trace parsers; the existing ones wrap them, so span and trace commands are unchanged and can migrate onto the same primitive next. A parse error next to a `help` argument is deferred to `func`, so `sentry replay view help` still shows the command's help. Plural alias interception drops the first argument and parses the rest again. Co-Authored-By: BYK <burak.kaya@sentry.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Addresses Bugbot on #1714. `buildListCommand` dropped a subcommand name (`sentry log list view …`) in `func` and parsed the other arguments again, after the scanner had already parsed all of them: - `log list view acme/web <trace-id>` failed in the scanner (exit 252), because "acme/web" was read as the trace ID, so the intercept never ran - a parse error deferred next to `help` in the second parse was never thrown, so `func` read an undefined parse `skipLeadingArgument` now wraps a `boundArguments` positional so the name is left out in the scanner, before it changes what the other arguments mean. `func` only prints the hint.
Drops the array positional `combine` hook from our `@stricli/core` patch. `buildCommand` already runs `applyBoundPositionals` before `func`, so it now parses `boundArguments` positionals there for every caller, CLI and SDK alike. A parse error becomes Stricli's own `ArgumentParseError`. `getExitCode` maps it to `InvalidArgument` and `exceptionWhileRunningCommand` formats it as an argument error, so it still prints `Failed to parse "…" for <placeholder>` and exits 252, before the auth guard. Next to a `help` argument it is thrown inside the wrapper's try instead, so the help recovery catches it. That replaces the deferred-error bookkeeping. The patch, its `check:patches` markers and the lockfile hash are back to main's. Co-Authored-By: BYK <burak.kaya@sentry.io>
The TypeScript that decided which argument is the ID and which is its target (`replay view`'s `parsePositionalArgs`, `parseTraceTargetInput`, `parseSlashSeparatedTraceTarget`, `parseDualModeInput`) counted arguments and split on slashes by hand. `grammar/id-arguments.pegjs` now does that, over every argument at once: each one, trimmed, follows a NUL, so arity is part of the input. Its start rules are `trace`, `tracePath`, `traceOrProject` (log list, span list) and `replay`. They return what the arguments name (`IdArguments`: the ID, its org/project scope, a URL's server, and warnings), and callers map that onto their own target types. ID checks and errors go through hooks, so messages and error classes match what they were. `generate-parsers.mjs` compiles the `startRule` literals of a grammar's declarations as its allowed start rules. Behavior changes, all for input that used to fail or misparse: - `log list`/`span list`: trace mode and the trace ID now come from the same argument, so `<target> <extra> <trace-id>` warns about `<extra>` instead of rejecting it as the trace ID. - `replay`: a slash after the replay ID and a leading `/` are accepted, like for trace IDs. Co-Authored-By: BYK <burak.kaya@sentry.io>
Addresses Warden on #1714. - A first argument that ends in a trace ID is the trace target, even when a second trace ID follows. `trace` and `traceOrProject` used to read it as the target of the second ID, so `trace logs acme/<id> <id2>` used `<id2>`. The later arguments are now ignored with a warning, as after any other form. - `<org>//<id>` and `/<project>/<id>` have an ID; an empty org or project segment is a `ValidationError` ("Invalid format"), not a missing ID.
Addresses Warden on #1714. `replay view "" <replay-id>` (an unset `$ORG`, say) failed with "Replay ID is required": the empty-first-argument rule also matched when a replay ID followed. Only an empty single argument is a missing ID now; an empty target auto-detects, as for trace IDs.
The ID-arguments grammar no longer knows trace or replay IDs. It has one start rule, and the command says what its IDs look like through an ID_KINDS entry (trace, replay, log, span): the predicate, the validator, and which part of a Sentry URL holds one. A bare 32-hex value never tells the parser its kind. Every ID-taking command now takes the same forms: `<id>`, `<org>/<id>`, `<org>/<project>/<id>`, `<target> <id>`, `<id> <org>/<project>` (reversed, with a warning), a URL, and with `multiple`, more IDs that may repeat the target. One argument may hold several lines, or IDs after a space. Extra arguments are ignored with a warning, keeping the first real ID. `optional` covers `log list` and `span list`; `needsProject` makes `<x>/<id>` name project `<x>` for commands that need one; `validate: false` leaves malformed IDs for recovery. Migrated span view and log view as well. They drop their hand-written splitting, swap detection, and full-path handling. Trace view drops its swap detection, and trace recovery reuses the raw parse instead of re-deriving the ID's slot. A new sentry-cli/no-ad-hoc-id-arguments lint rule rejects ID parsing by hand in commands (parseSlashSeparatedArg, detectSwappedViewArgs, HEX_ID_RE, SPAN_ID_RE), with event view allowed since it takes issue IDs, and rejects ID kinds defined outside ID_KINDS. The grammar tests run one table against every kind. Co-Authored-By: BYK <burak.kaya@sentry.io>
`<org>/<id>` with a truncated, ellipsized, or junk-suffixed ID parsed as an org/project target with no ID, so trace commands said the trace ID was required instead of recovering it from the prefix. Read the last segment as an ID when it has enough hex for recovery to look up (the same check recovery uses) and is not a slug or a shell sentinel. `<org>/<project>` is still a target without an ID, and so is any token when IDs are optional.
`trace view`, `span view` and `log view` were the last ID-taking commands parsing their positionals in `func`. They now use `boundArguments`, like `replay view` and `log list`: the parse runs in the argument scanner with no side effects, so a missing ID or a bad target fails like any other argument (exit 252) before auth or any lookup, and `func` awaits `target.resolved`. These commands recover malformed IDs, which needs the API, so the parse keeps IDs as typed and `resolve` validates or recovers them. `parseTraceTargetWithRecovery` splits into the two halves: `parseTraceArgumentsInput` (pure) and `resolveTraceArguments` (applies a pasted URL's server and warnings, then validates or recovers the trace ID). `log view` parses its target with `parseOrgProjectInput` and applies the server when it resolves. Co-Authored-By: BYK <burak.kaya@sentry.io>
`<org>/trace-<id>` (an ID copied with its URL fragment prefix) was read as `<org>/<project>`, because the slug check saw the prefix's dash and letters. Strip the `trace-`, `span-`, `event-` and `txn-` prefixes before deciding the segment is a slug, as recovery does, so it reaches recovery like a bare `trace-<id>`.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 099a650. Configure here.
Recovery's slug check ran on the input with its URL fragment prefix, so `trace-<prefix>` or `span-<a>...<b>` failed as "looks like a project slug" and never reached the fuzzy lookup, though `<org>/trace-<prefix>` now gets to recovery. Share the prefix-stripping slug check between the grammar's ID hint and recovery. `span-my-app` is still a slug.
…operty `<org>/<x>` is now read as a log ID to recover when `x` has enough hex for a fuzzy lookup, so a generated project slug like `aa000a00` no longer makes the one-slash property throw ContextError. Generate only project slugs that cannot be a partial ID there, and add a property that `<org>/<partial-id>` parses as org target plus raw log ID.
BYK
approved these changes
Oct 29, 2026
This was referenced Oct 29, 2026
BYK
added a commit
that referenced
this pull request
Oct 29, 2026
…1728) Part of #1564. Follow-up to #1714, which left `trace logs` and `span list` on the shared ID grammar but still parsing inside `func`. Both now declare their positional with `boundArguments`, like `trace view`, `span view` and `log list`: - **`trace logs`** parses with `parseTraceTargetInput` (pure). Its resolver applies a pasted URL's server and the warnings, then resolves the org with `resolveTraceOrg`; an explicit project still becomes the `project:` filter. - **`span list`** parses with `parseDualModeInput(..., { needsProject: true })`, plus `parseOrgProjectInput` in project mode, the same shape as `log list`. The resolver applies the URL context and resolves the project (`resolveProjectBoundTarget`) or the trace's org and project (`resolveTraceOrgProject`). The mode handlers now take resolved values instead of resolving themselves. - `parseTraceTarget` and `parseDualModeArgs` had no callers left, so they're removed. Their tests now go through `parseTraceTargetInput` / `parseDualModeInput`, keeping the same coverage. - `SpanListArguments` and `TraceLogsArguments` are added to the `require-target-resolution` lint rule's type list. **Behavior changes** (input that already failed): - A missing or invalid trace ID, or a bad target, now fails before the auth guard as an argument error: `Failed to parse "…" for org/project/trace-id: …`, exit 252. Before, it failed inside `func`. For example, `trace logs not-hex`, `trace logs my-org/web`, `span list my-org/web not-hex`. - SDK callers get an `ArgumentParseError` whose `.exception` is the parser's error, as with the other `boundArguments` commands. - `sentry trace logs help` still shows the command's help, now through the parse-error path (a new test covers it). Resolution order, URL-context timing (applied in the resolver, after auth), help text and completion kind are otherwise unchanged. **Tests:** new `parseSpanListInput` tables (including `<x>/<trace-id>` → project `<x>` and a URL's `baseUrl` not being applied), `trace logs` parse failures before any lookup, and `trace logs help` in `help-positional`. Project-mode tests now mock `resolveProjectBoundTarget`. **Verification:** - `tsc`, CLI and repo lint (oxlint, oxfmt, ast-grep, lint-rule tests) pass. - CLI unit suite: 11,976 passed. The 6 failures are permission tests that only fail in the sandbox (running as root). - Smoke-tested with `pnpm run cli`: the three inputs above exit 252 with the parse error, and `trace logs help` prints help. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3A6afff18d4507cd4bc63a317180ceb0f7 --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/agent%3A6afff18d4507cd4bc63a317180ceb0f7/?project=4510944073809921) <!-- junior-session-footer:end --> Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: BYK <burak.kaya@sentry.io>
BYK
added a commit
that referenced
this pull request
Oct 29, 2026
…mmand runs (#1729) Part of #1564, following #1714. Issue arguments (numeric IDs, short IDs, `@` selectors, `#` references, issue URLs) get their own grammar. Commands that take one issue now parse it before `func` runs. **Why a separate grammar.** Issue IDs aren't hex IDs, so they stay out of `grammar/id-arguments.pegjs`, which doesn't care what kind of ID it parses. The forms used to live in about 500 lines of hand-written branches in `arg-parsing.ts`. They are now: - `grammar/issue-argument.pegjs` lists every form and splits an argument into its parts. Like the ID grammar, it accepts any input and never throws: a malformed argument comes back as `invalid` with a reason. - `parseIssueArgument` (`src/lib/issue-arguments.ts`) validates and normalizes those parts into the same `ParsedIssueArg` as before. It has no side effects: it returns a pasted URL's server as `baseUrl` instead of applying it. - `parseIssueArg` keeps its behavior (it applies the URL's server) on top of `parseIssueArgument`. `issue list`, `issue view`, `issue explain` and `issue merge` still use it through `resolveIssue`. The existing `parseIssueArg` suites (unit, property and URL-poisoning) pass unchanged, apart from where they import it from. `test/lib/issue-arguments.test.ts` adds the error reasons, the `#` forms, and a check that a pasted URL's server isn't applied at parse time. **Parse before run.** `src/commands/issue/positional.ts` adds `issuePositional({ command })` and `issueParameter`. Each is a `boundPositional` whose `target.resolved` calls `resolveIssue`. They're used by `issue archive`, `events`, `plan`, `resolve`, `unresolve`, `tags`, `link`, `unlink` and `event list`. `feedback view`, `resolve`, `spam` and `unresolve` get the same treatment through `feedback/positional.ts`. `resolveIssue` (and `resolveFeedback`) now accept the parsed argument as well as a raw string. - `tags` keeps the tag key and `--environment` in its recovery hints. `plan`, `link` and `unlink` need the stricter `resolveOrgAndIssueId`. So these commands pass `requireIssueArgument(target.parsed, …)` themselves instead of awaiting `target.resolved`. - `event list` still bails out when its plural-alias interception drops the positional. **Behavior changes (intentional):** - A malformed issue argument in these commands now fails the parse, like other bound positionals: `Failed to parse "…" for issue: …`, exit 252, before the auth guard. - A pasted issue URL's server is applied when the issue resolves, not while parsing. An attacker host still fails in `applySentryUrlContext` before any request. **Not in this PR:** - `issue view`, `issue explain` and `issue merge` take several issues and warn on the ones that fail, both bad input and failed lookups. Parsing them up front would turn one bad argument into a failure for the whole command, so they need their own `boundArguments` design. - `issue list` belongs to #1711. - `event view` is stacked on this PR. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3Ab47afb146f99778033f77dec53fa2175 --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/agent%3Ab47afb146f99778033f77dec53fa2175/?project=4510944073809921) <!-- junior-session-footer:end --> Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: BYK <burak.kaya@sentry.io>
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Follow-up to #1655 for #1564. Some positionals mean different things depending on how many arguments were given, like
[<org>/<project>] <replay-id>orlog list's optional trailing trace ID. Those kept parsing infunc, because a Stricliparseonly sees one argument at a time. Each ID-taking command also had its own rules for them.The new parser type:
boundArguments(src/lib/positional-parse.ts). It's an array positional with a pureparseover all the arguments, bound to aresolvethat runs after the auth guard, the same contract asboundPositional.functakes oneBoundArguments<Parsed, Resolved>, reads.parsedand awaits.resolved. Each argument still completes as its completion kind.No Stricli patch.
buildCommandalready runsapplyBoundPositionalsbeforefunc, soboundArgumentspositionals are parsed there for every caller, CLI and SDK alike.ArgumentParseError.getExitCodemaps it toInvalidArgument, andexceptionWhileRunningCommandformats it as an argument error. It still printsFailed to parse "…" for <placeholder>and exits 252, before the auth guard, just like a scanner error.help, the error is thrown inside the wrapper'stryinstead, sosentry replay view helpstill prints the command's help.combinehook to our@stricli/corepatch. I've removed it and proposed the hook upstream instead. If it lands, the parse can move into Stricli's scanner.One grammar for every kind of ID (
grammar/id-arguments.pegjs,src/lib/id-arguments.ts). The grammar doesn't know about any kind of ID. It has one start rule and only decides which token is the target and which are IDs. The command passes its kind fromID_KINDS(trace,replay,log,span): what an ID looks like, how to validate it, and which part of a Sentry URL holds one. A bare 32-hex value never tells the parser which kind it is. Errors come fromparseIdArguments, never from Peggy.Every ID-taking command now accepts the same forms. Each one takes the friendliest behavior any command had before:
<id>,<org>/<id>,<org>/<project>/<id>,<target> <id>, or a URL with the command's kind of ID.<id> <org>/<project>is read as reversed, with a warning. Before, only trace view, replay and log view did this.multiple, more IDs can follow any of those, and each can repeat the target (<target>/<id>). Before, only log view did this.Three options cover the differences between commands that are real:
optional: a command can run without an ID, and a lone argument is the target. Used bylog listandspan list(a project's items or a trace's), and byspan view, where the target of the span IDs is the trace.needsProject:<x>/<id>means project<x>instead of org<x>. Used by commands that need a project:log view,span viewandspan list.log viewalready read it that way.validate: false: IDs come back raw, so the command can recover a malformed ID once it knows the target. Used bylog viewandspan view. Trace recovery uses it too, instead of working out again which argument held the ID.Keeping it consistent: the new
sentry-cli/no-ad-hoc-id-argumentslint rule rejects hand-written ID parsing in commands (parseSlashSeparatedArg,detectSwappedViewArgs,HEX_ID_RE,SPAN_ID_RE). It also rejects aparseIdArgumentscall whose kind isn't fromID_KINDS.event viewis exempt because it also takes issue IDs, which keep their own syntax. The grammar tests run one table against every kind, so the commands can't drift apart.Migrated:
boundArguments(parsed before the command runs):log list,replay view,replay download,trace view,span viewandlog view.func:trace logsandspan list. All of them lose their own splitting, reversed-argument detection and full-path handling.trace viewkeeps only its issue-short-ID shortcut.trace view,span viewandlog viewrecover malformed IDs, which needs the API. Their parse keeps IDs as typed, andresolvevalidates or recovers them.parseTraceTargetWithRecoveryis split into a pureparseTraceArgumentsInputand an asyncresolveTraceArguments.baseUrland warnings, andresolveapplies them.boundArgumentscommands: a bad replay ID, or a replay URL without an ID. Intrace view,span viewandlog view, a missing ID or a bad target exits 252 before auth or any lookup; a malformed ID still goes through recovery.ArgumentParseErrorwhose.exceptionis the parser's error, where they used to get that error directly.replay downloadgets org/project completion, whichreplay viewalready had.Behavior changes, all for input that used to fail, warn, or be ignored:
replay view <target> <id> extrawarns and ignoresextrainstead of failing.log list <project> not-a-trace-idandspan list <project> not-a-trace-idfail with "Invalid trace ID" instead of silently listing the project.span list <project>/<trace-id>andspan view <project>/<trace-id> <span-id>look up project<project>instead of failing with "Specific project required".span view org/project/not-a-trace/<span-id>goes through trace-ID recovery, liketrace view, instead of failing right away.<org>/<partial-id>(at least 8 hex chars, an ellipsized ID, or junk after an ID) is read as an ID to recover, as before.<org>/<project>is still a target missing its ID.Subcommand-name interception (
sentry log list view …):skipLeadingArgumentdrops the subcommand name before the arguments are parsed, so it can't change what the other arguments mean.buildListCommandthen only prints the tip. This fixes both Bugbot findings.Next:
trace logsandspan listcan move ontoboundArgumentstoo. Issue IDs andevent viewstay separate. The list-target positional for dispatch-based lists is in #1711.Verification (Node 24):
tsc, lint (including the new rule), format, ast-grep and the lint-rule tests pass.delta-upgrade) and also fail onmain.replay view not-hex,replay view test-org/cli("Replay ID is required"), a trace URL passed toreplay view(missing replay ID, with an example replay URL), andlog list web not-hex.span view <span-id>,span view org/project/<span-id>andspan view <trace-id>show the targeted "trace ID and span ID", "trace ID" and "span ID" required errors.log view a/b/<id> c/d/<id>reports IDs from different projects.trace view/span view/log viewmigration (9a899d4) was checked withtsc, lint, format, the lint-rule tests, and the command and lib unit suites (failures only in sandbox-only permission, SQLite and subprocess tests). The generated SDK is unchanged. It has not been smoke-tested on a bundled build.via BYK.
--
View Junior Session in Sentry