ref(cli): replace argv-hoist preprocessor with a Stricli top-level-flags patch - #1340
ref(cli): replace argv-hoist preprocessor with a Stricli top-level-flags patch#1340jared-outpost[bot] wants to merge 5 commits into
Conversation
…ags patch
Global flags placed before the subcommand (`sentry --verbose issue list`)
used to be relocated to the tail of argv by the `argv-hoist.ts` preprocessor,
because Stricli only parses flags at the leaf command and treats a global flag
in a route position as an unknown subcommand.
This teaches Stricli's route scanner about a fixed allow-list of Sentry global
flags via the existing `@stricli/core` patch (same mechanism as the `-H`
removal), so `buildRouteScanner` recognizes `--verbose`, `--json`, `--org`,
`--project`, `--log-level`, `--fields` (and the `-v` alias, plus `=`-inline and
value forms) at any route depth and forwards them to the leaf command instead
of failing route resolution. The patch also drops Stricli's built-in
`-v`=version alias so `-v` stays the CLI's `--verbose` alias at every position;
`--version` remains the version flag.
`argv-hoist.ts` is gone. The two transforms Stricli can't do at arbitrary route
depth remain as thin app-boundary glue in `argv-glue.ts`: `--version`
normalization (Stricli only prints it at argv[0]) and the `--help --json`
rewrite to the `help` command (Stricli intercepts `--help` and ignores `--json`).
- Regenerate the `@stricli/core` patch (both dist/index.{js,cjs}).
- Guard the new patch effects in `check:patches` (requiredMarker support).
- Replace argv-hoist tests with argv-glue unit tests + a run(app,...)
integration suite covering global flags at depth, value-flag consumption,
the `-v` regression, and `--` escape passthrough.
Closes #1339
|
|
marked ready — full CI is green (Build: lint/typecheck, unit + e2e tests, binary + npm smoke builds; plus Warden, Semgrep, CodeQL, Socket, dependency review). @BYK — flagging the two deliberate calls for your read: the top-level-flag token set is hardcoded in the |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Without argv hoisting, global flags can sit between `cli` and its subcommand (`sentry cli --verbose setup`), so the positional `args[1]` check in shouldSuppressNotification no longer sees `setup`/`fix` and update notifications leak into those management commands. Resolve the subcommand by skipping global flags (and value-flag values) after `cli`. Flagged by Cursor Bugbot on #1340.
shouldSuppressNotification only matched `cli` at args[0], so global flags placed before the command (`sentry --verbose cli setup`) — now possible since flags are no longer hoisted — leaked update notifications into management commands. Locate the `cli` group past leading global flags, and stop rewriteHelpJsonRequest at `--` without discarding `--help --json` already seen before it. Flagged by Cursor Bugbot and Seer on #1340.
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 388f884. Configure here.
The top-level-flags scanner patch latched the next token as a value for value-taking global flags (`--org`, `--fields`, `--log-level`, `--project`) *before* the --help/-h interception. So `sentry --org --help` (value flag given without a value) consumed --help as the org value and help never fired. Skip the latch for help tokens so they fall through to the help handler, matching Stricli's leaf parser, which never consumes a following flag as a value. Flagged by Cursor Bugbot on #1340.
|
Jared, you need to make this a generic patch: something we can send as a PR upstream. I see |
… option Rework the @stricli/core route-scanner patch so the top-level (global) flags allow-list is passed in via a new `scanner.topLevelFlags` config option instead of hardcoding SENTRY_TOP_LEVEL_*_FLAGS in the patch. The scanner now reads the allow-list (booleanFlags/valueFlags sets) from config and is inert when unset, keeping stock Stricli behavior unchanged — this makes the top-level-flags feature upstreamable. The Sentry CLI supplies the allow-list from GLOBAL_FLAGS via buildTopLevelFlags() wired into app.ts's scanner config, so adding a global flag stays a one-line change. Adds the topLevelFlags field to ScannerConfiguration in the patched .d.ts, updates check:patches to assert matchTopLevelFlag, and adds a test pinning the derivation contract. Addresses review feedback from @BYK on #1340.
|
reworked the patch to be generic/upstreamable in 3b43098. the scanner no longer hardcodes the Sentry-specific allow-list is now supplied at init: the |
1 participant
Global flags before the subcommand (
sentry --verbose issue list) used to be relocated to the tail of argv byargv-hoist.ts, because Stricli only parses flags at the leaf command and treats a global flag in a route position as an unknown subcommand.This teaches Stricli's route scanner about a fixed allow-list of Sentry global flags via the existing
@stricli/corepatch (same mechanism as the-Hremoval), sobuildRouteScannerrecognizes--verbose,--json,--org,--project,--log-level,--fields(and the-valias, plus=-inline/value forms) at any route depth and forwards them to the leaf command instead of failing route resolution. The patch also drops Stricli's built-in-v=version alias so-vstays the CLI's--verbosealias at every position;--versionremains the version flag.argv-hoist.tsis deleted. The two transforms Stricli can't do at arbitrary route depth stay as thin app-boundary glue inargv-glue.ts:--versionnormalization (Stricli only prints it at argv[0]) and the--help --jsonrewrite to thehelpcommand (Stricli intercepts--helpand ignores--json).Scope note: the last checklist item from the issue — upstreaming the top-level-flags behavior to Stricli — is intentionally left out of this PR; the local patch stays until/unless accepted.
Testing
pnpm exec vitest run test/lib/argv-glue.test.ts test/lib/argv-glue.integration.test.ts test/commands/help.test.ts test/commands/bash-hook.test.ts(77 passed)pnpm run check:patches,tsc --noEmit,biome checkon changed files — all cleanrun(app, preprocessArgv(argv)):issue list --help --jsonand--help --jsonemit structured JSON;cli --versionand--versionprint the version;--verbose/--org/-vbefore or between route segments reach the leaf;-- passthruis not consumed as a global flagCloses #1339