Skip to content

fix(cli): Keep CliError exit codes for argument parser failures - #1651

Closed
sentry-junior[bot] wants to merge 1 commit into
mainfrom
byk/cli-parser-cli-error-exit-codes
Closed

sentry-junior[bot] wants to merge 1 commit into
mainfrom
byk/cli-parser-cli-error-exit-codes

Conversation

@sentry-junior

@sentry-junior sentry-junior Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Refs #1564.

When a flag or positional parse function throws, Stricli wraps the error in ArgumentParseError and always exits with ExitCode.InvalidArgument (-4, which the shell sees as 252). So a ValidationError from one of our flag parsers loses its exit code. For example, sentry issue list --period bogus and sentry log list --sort sideways exit 252, but the exit code docs say a bad flag value exits 21. #1594 avoided this for target positionals by parsing them lazily inside func. Flag parsers still run in Stricli's scanner.

Changes:

  • The @stricli/core patch adds an optional determineArgumentErrorExitCode application hook. Stricli calls it for each scanner error, and the first number it returns becomes the exit code. If the hook is unset or returns undefined, Stricli exits with InvalidArgument as before. The hook has a PATCH(getsentry/cli) marker, and check:patches now checks that it is present in both bundles.
  • app.ts returns the wrapped CliError's exitCode from that hook. It also formats these errors with formatError, which handles format() and credential redaction. The Failed to parse "<input>" for <param>: prefix stays the same.

Errors that are not CliErrors keep exit code 252. This includes Stricli's own numberParser (--limit abc), unknown flags, and missing positionals. Parse-phase errors still don't go through reportCliError or the --json error output. Both are unchanged here.

via BYK.

--

View Junior Session in Sentry

Stricli wraps any exception thrown from a flag or positional `parse`
function in ArgumentParseError and always exits with InvalidArgument
(-4, seen as 252). A ValidationError from a parser such as
`--period bogus` therefore exited 252 instead of the documented 21,
and lost its CliError formatting.

The @stricli/core patch now accepts an optional
`determineArgumentErrorExitCode` application hook. The app uses it to
return the wrapped CliError's own exit code; other scanner errors keep
InvalidArgument. Parser CliErrors are also rendered through
formatError, keeping Stricli's input and parameter prefix.

This removes one blocker for moving target parsing into positional
parsers.

Refs #1564

Co-Authored-By: BYK <burak.kaya@sentry.io>
@vercel

vercel Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
sentry-local Ready Ready Preview Oct 23, 2026 11:53pm UTC

Request Review

@sentry-junior sentry-junior Bot added the CLI Sentry CLI and CLI documentation label Oct 23, 2026
@github-actions github-actions Bot added the risk: medium PR risk score: medium label Oct 23, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 85b8870. Configure here.

Comment thread packages/cli/patches/@stricli%2Fcore@1.2.8.patch

@BYK BYK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point I'd prefer reverting to argument parser error for everything and accepting that or trying to wrap the stricli error class with ours if possible without patching it

@sentry-junior

sentry-junior Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

@BYK, I looked into fixing this without patching Stricli. I haven't pushed anything yet. Stricli maps every scanner error to ExitCode.InvalidArgument and passes our app nothing but the formatter (exceptionWhileParsingArguments), which gets no context. So any patch-free fix has to remap the exit code after run(). I see two ways to do that, and both have a cost:

  1. Record the error in the formatter, then remap after run() in cli.ts and in both sdk-invoke.ts runners. This is small, but the recorded error lives in module-level state. Concurrent SDK run() calls could read each other's error and get the wrong exit code.
  2. Wrap every flag and positional parse in buildCommand. The wrapper would record the CliError keyed by this.process (one per run, so no race) and rethrow it. The cost is that parser identity carries metadata: withCompletionTarget stores data on the function, and lazyParsers is a WeakSet of them. So the wrapper has to copy those over, and it touches every command's parameters.

Neither one is clearly better than keeping the patch, so I lean toward your first option: keep exit 252 for scanner-phase errors, document it, and close this PR. One question first: by "reverting to argument parser error for everything", do you also mean undoing #1594's lazy positionals, so that positional parse errors go back to 252 too? Or only leaving flags as they are on main?

@sentry-junior

sentry-junior Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of #1655, which follows the review here: no Stricli patch. Argument parse errors exit 252 everywhere, including target positionals, and the docs now say so.

@sentry-junior sentry-junior Bot closed this Oct 25, 2026
BYK added a commit that referenced this pull request Oct 25, 2026
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 inside `func`. 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 first `target.resolved` read.
`target.parsed`, `target.resolved` and `target.raw` behave as before for
commands, and no command changes beyond a comment.

Changes:
- `positional-parse.ts`: `lazyPositional`/`LazyPositional` become
`boundPositional`/`BoundPositional`. The parse runs when Stricli calls
the parser. `resolved` stays memoized and runs inside `func`, after the
auth guard.
- **No side effects in the parse.** It now runs before the auth guard,
and it also runs when a later argument fails to parse.
`parseOrgProjectInput` is the pure half of `parseOrgProjectArg`: 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 with `HostScopeError` (exit 13) after the auth guard.
`parseOrgProjectArg` itself is unchanged for its other callers.
- **Destructive check.** `buildCommand` checks the targets of
destructive commands after the auth guard and rc URL check, before
`func` runs. Before, the check only ran inside `target.resolved`, so a
destructive command that read only `target.parsed` would skip it.
- **Docs.** `exit-codes.md` gains 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 CLI `AGENTS.md`
and the lint rule message describe the new flow.

Tests:
- `test/lib/positional-parse.test.ts` covers: an invalid target exits
with `InvalidArgument`; a destructive command refuses auto-detect even
when it only reads `parsed`; 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.ts` runs the real app: `issue
list --period bogus` and `trace list @latest` both exit with
`InvalidArgument`.
- `lint` and `typecheck` pass. Full `test:unit`: 11336 passed, 6 failed.
The 6 are chmod-based permission tests that also fail on clean `main` in
this sandbox, which runs as a user that bypasses chmod.

<!-- 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 successfully deployed

1 active deployment
Preview — 85b8870b Deployed Oct 23, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLI Sentry CLI and CLI documentation risk: medium PR risk score: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant