Repository navigation
ref(cli): Parse target positionals in Stricli's argument scanner - #1655
Merged
Merged
Conversation
Target positionals no longer parse lazily inside `func`. Stricli runs the parse in its argument scanner, so an invalid target fails like any other argument: "Failed to parse ..." and exit code 252, the same as flags. Only resolution stays deferred, on the first `target.resolved` read. The parse is now free of side effects: the server of a pasted Sentry URL is applied when the target resolves, after the auth guard and rc URL check, so a host-scope failure still exits 13. `parseOrgProjectInput` is the pure half of `parseOrgProjectArg`. Destructive commands check their targets in `buildCommand` before `func` runs, so an auto-detected target is refused whether the command reads `parsed` or `resolved`. The exit code docs now describe argument errors (252) and no longer list bad flag values under 21. Refs #1564 Co-Authored-By: BYK <burak.kaya@sentry.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This was referenced Oct 25, 2026
BYK
approved these changes
Oct 25, 2026
Arguments Stricli cannot parse exit 252 without reaching reportCliError, so they went unseen. `exceptionWhileParsingArguments` now reports each one as an `invalid_argument` info message, grouped into one issue like `unknown_command`, with the scanner error class, the parser's error class, and the redacted message the user saw. Refs #1564 Co-Authored-By: BYK <burak.kaya@sentry.io>
BYK
approved these changes
Oct 25, 2026
BYK
added a commit
that referenced
this pull request
Oct 29, 2026
…ositional (#1709) Refs #1564. Now that #1575 has merged, `agent-conversation list` can move to the target positional. Before this change, `agent-conversation list` declared its target with `LIST_TARGET_POSITIONAL` and called `resolveOrgOptionalFromArg` inside `func`. Now it uses `orgOptionalTarget` and awaits `target.resolved`, the same way `feedback list`, `replay list` and `explore` do. Resolution hasn't changed. It still goes through `resolveOrgOptionalTarget` with the command name `agent-conversation list`. Completion is also the same, since both parsers carry the org/project tag. One behavior change: a malformed target now fails in Stricli's argument scanner and exits 252, like every other target positional since #1655. Before, it failed inside `func`. What's left on `LIST_TARGET_POSITIONAL`/`NAMED_LIST_TARGET_POSITIONAL` (`issue list`, `dsn list`, `alert issues list`, `project list`) goes through `dispatchOrgScopedList`, which resolves differently for each target mode. None of them has a single resolver to bind, so I've left them out of this PR. The follow-up is tracked on #1564. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:local%3Aweb%3Aa3773d7739de1b873375a6f1 --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/local%3Aweb%3Aa3773d7739de1b873375a6f1/?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
Follow-up to #1655 for #1564. Some positionals mean different things depending on how many arguments were given, like `[<org>/<project>] <replay-id>` or `log list`'s optional trailing trace ID. Those kept parsing in `func`, because a Stricli `parse` only 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 pure `parse` over all the arguments, bound to a `resolve` that runs after the auth guard, the same contract as `boundPositional`. `func` takes one `BoundArguments<Parsed, Resolved>`, reads `.parsed` and awaits `.resolved`. Each argument still completes as its completion kind. **No Stricli patch.** `buildCommand` already runs `applyBoundPositionals` before `func`, so `boundArguments` positionals are parsed 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. It still prints `Failed to parse "…" for <placeholder>` and exits 252, before the auth guard, just like a scanner error. - If an argument is `help`, the error is thrown inside the wrapper's `try` instead, so `sentry replay view help` still prints the command's help. - An earlier revision added a `combine` hook to our `@stricli/core` patch. 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 from `ID_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 from `parseIdArguments`, 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. - With `multiple`, more IDs can follow any of those, and each can repeat the target (`<target>/<id>`). Before, only log view did this. - One argument can hold several lines, or IDs after a space. This replaces span view's whitespace split and log view's newline split. - Extra arguments are ignored with a warning, and the first real ID is kept. Replay used to reject them. - Slashes pasted after an ID are ignored. An empty org or project in a path is an error. 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 by `log list` and `span list` (a project's items or a trace's), and by `span 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 view` and `span list`. `log view` already read it that way. - `validate: false`: IDs come back raw, so the command can recover a malformed ID once it knows the target. Used by `log view` and `span 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-arguments` lint rule rejects hand-written ID parsing in commands (`parseSlashSeparatedArg`, `detectSwappedViewArgs`, `HEX_ID_RE`, `SPAN_ID_RE`). It also rejects a `parseIdArguments` call whose kind isn't from `ID_KINDS`. `event view` is 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:** - With `boundArguments` (parsed before the command runs): `log list`, `replay view`, `replay download`, `trace view`, `span view` and `log view`. - With the shared parser, still parsing in `func`: `trace logs` and `span list`. All of them lose their own splitting, reversed-argument detection and full-path handling. `trace view` keeps only its issue-short-ID shortcut. - `trace view`, `span view` and `log view` recover malformed IDs, which needs the API. Their parse keeps IDs as typed, and `resolve` validates or recovers them. `parseTraceTargetWithRecovery` is split into a pure `parseTraceArgumentsInput` and an async `resolveTraceArguments`. - Parsing a pasted Sentry URL no longer applies its server. The parse returns `baseUrl` and warnings, and `resolve` applies them. - Malformed input now exits 252 in the migrated `boundArguments` commands: a bad replay ID, or a replay URL without an ID. In `trace view`, `span view` and `log view`, a missing ID or a bad target exits 252 before auth or any lookup; a malformed ID still goes through recovery. - SDK callers get an `ArgumentParseError` whose `.exception` is the parser's error, where they used to get that error directly. - `replay download` gets org/project completion, which `replay view` already had. **Behavior changes**, all for input that used to fail, warn, or be ignored: - `replay view <target> <id> extra` warns and ignores `extra` instead of failing. - `log list <project> not-a-trace-id` and `span list <project> not-a-trace-id` fail with "Invalid trace ID" instead of silently listing the project. - `span list <project>/<trace-id>` and `span 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, like `trace 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 …`): `skipLeadingArgument` drops the subcommand name before the arguments are parsed, so it can't change what the other arguments mean. `buildListCommand` then only prints the tip. This fixes both Bugbot findings. **Next:** `trace logs` and `span list` can move onto `boundArguments` too. Issue IDs and `event view` stay 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. - CLI unit suite: 11,749 passed. The 7 failures are sandbox-only (permission tests and `delta-upgrade`) and also fail on `main`. - Smoke-tested on a bundled build: - Exit 252: `replay view not-hex`, `replay view test-org/cli` ("Replay ID is required"), a trace URL passed to `replay view` (missing replay ID, with an example replay URL), and `log list web not-hex`. - `span view <span-id>`, `span view org/project/<span-id>` and `span 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. - The `trace view` / `span view` / `log view` migration (9a899d4) was checked with `tsc`, 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. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:local%3Aweb%3Aa3773d7739de1b873375a6f1 --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/local%3Aweb%3Aa3773d7739de1b873375a6f1/?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.
Refs #1564. Replaces #1651.
Per the review on #1651, this drops the special handling that kept a
CliError's exit code when an argument fails to parse. Target positionals no longer parse lazily insidefunc. Stricli parses them in its argument scanner, so an invalid target now fails like a bad flag:Failed to parse "<input>" for <placeholder>: …and exit code 252. Only resolution is still deferred, to the firsttarget.resolvedread.target.parsed,target.resolvedandtarget.rawbehave as before for commands, and no command changes beyond a comment.Changes:
positional-parse.ts:lazyPositional/LazyPositionalbecomeboundPositional/BoundPositional. The parse runs when Stricli calls the parser.resolvedstays memoized and runs insidefunc, after the auth guard.parseOrgProjectInputis the pure half ofparseOrgProjectArg: it returns a pasted Sentry URL's server instead of applying it. The target resolver applies that server, so a URL for a server you have no account on still fails withHostScopeError(exit 13) after the auth guard.parseOrgProjectArgitself is unchanged for its other callers.buildCommandchecks the targets of destructive commands after the auth guard and rc URL check, beforefuncruns. Before, the check only ran insidetarget.resolved, so a destructive command that read onlytarget.parsedwould skip it.exit-codes.mdgains a 252 row and an "Argument errors" section, and code 21 no longer claims bad flag values. The agent guidance and the generated skill get a 252 row, and the CLIAGENTS.mdand the lint rule message describe the new flow.Tests:
test/lib/positional-parse.test.tscovers: an invalid target exits withInvalidArgument; a destructive command refuses auto-detect even when it only readsparsed; awaiting a target does not resolve it; and a pasted URL's server is applied on resolve, not on parse.test/lib/scanner-flags.integration.test.tsruns the real app:issue list --period bogusandtrace list @latestboth exit withInvalidArgument.lintandtypecheckpass. Fulltest:unit: 11336 passed, 6 failed. The 6 are chmod-based permission tests that also fail on cleanmainin this sandbox, which runs as a user that bypasses chmod.via BYK.
--
View Junior Session in Sentry