Repository navigation
feat(cli): Declare and lint a completion kind for every flag - #1759
sentry-junior[bot] wants to merge 5 commits into
Conversation
Every visible value-taking flag now says how shell completion handles its value, and test/lib/completion-coverage.test.ts fails, naming the command and flag, for any flag without a kind. There is no allow-list. - --fields completes the output schema's field paths, nested ones in dot notation, one comma-separated item at a time, leaving out the fields already listed (new choice-list kind, choiceListParser). - Shared parsers get kinds through PARSER_TARGETS: numberParser and sanitizeQuery complete nothing; parseCursorFlag, parseLogSort, parseSpanDepth, parseInstallationMethod, and parseReleaseChannel complete the values they accept. parseCursorFlag moves to arg-parsing.ts so completion-target.ts can import it without a cycle. - A command's own validating parser keeps its checks when wrapped: choiceParser(values, parse) completes values (sort, status, format, method, datasets), textParser(parse) completes nothing (ports, limits, thresholds). - Every parse: String flag uses a named parser: paths complete files or directories, --platform completes platforms, org list --account completes accounts, --scope/--features/--type complete their lists, --timezone completes tz names, and the rest are parseText.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The viewer integration tests import `commands/local/server.ts`, which now reaches `lib/arg-parsing.ts` through the tagged flag parsers in `completion-target.ts`. That module imports the generated grammar, so a fresh CI checkout with only the API schema generated failed to resolve `org-project-target-grammar.js`. `ensure-generated.mjs` now takes the groups to check, and the viewer test asks for the schema and the grammar.
BYK
left a comment
There was a problem hiding this comment.
Stopped at 41 files as it is apparent that you left many String parsers at parseText even though it is easy to see they are not random/free-form text. Let's do this properly.
| until: { | ||
| kind: "parsed", | ||
| parse: String, | ||
| parse: parseText, |
There was a problem hiding this comment.
Based on the brief looks like this can be richer?
| parse: parseText, | ||
| brief: "Native integration ID, when multiple installations match", | ||
| optional: true, | ||
| }, | ||
| app: { | ||
| kind: "parsed", | ||
| parse: String, | ||
| parse: parseText, |
There was a problem hiding this comment.
Can these not be found through API calls etc?
| field: { | ||
| kind: "parsed", | ||
| parse: String, | ||
| parse: parseText, |
There was a problem hiding this comment.
Looks like this actually has a strict format rather than free-form text?
| into: { | ||
| kind: "parsed", | ||
| parse: String, | ||
| parse: parseText, |
There was a problem hiding this comment.
So rather than a random text, this actually needs to be an issue target?
Resolve the list-command.ts import conflict: keep main's removal of the createRequire route lookup and this branch's import of parseCursorFlag from arg-parsing.ts. Add the flagNames field that main's extractor now returns to the branch's new introspect test. Co-Authored-By: BYK <126780+BYK@users.noreply.github.com>
Add sentry-cli/require-completion-parser. In src/commands/ and src/lib/ it rejects, on command positionals and parsed flags, the parsers that can never carry a completion kind: built-ins (String, Number, Boolean, BigInt, Date, parseInt, parseFloat, JSON.parse, Number.parseInt, ...), inline functions, and boundArguments without a kind. Named parsers pass lint; completion-coverage.test.ts still checks at runtime that each one is tagged. Parameters that must stay without a kind go in lint-rules/completion-exemptions.json, with file, command, parameter and reason. The lint rule and the coverage test both read it, and the test fails on an entry whose parameter is covered or gone. The list starts empty: no current parameter needs it, and the rule finds nothing in the current code. An oxlint-disable comment silences only the lint rule. The coverage test cannot see comments, so the parameter still needs a kind or an entry. Co-Authored-By: BYK <126780+BYK@users.noreply.github.com>
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 e37b649. Configure here.
require-completion-parser named any parameter with a `placeholder` as a positional (`<method>`), but flags such as `api --method` declare placeholders too, and the coverage test names them `--method`. An exemption written as `--method` never matched in the lint rule, and one written as `<method>` was flagged as stale by the test. Name a parameter by its property key unless that key is an array positional's `parameter`, and fall back to the placeholder only for positionals. Add RuleTester cases for an exempt flag with a placeholder and an exempt array positional.

Follow-up to #1751, which did this for positionals. Now every visible value-taking flag declares how shell completion handles it, and both lint and the test suite fail when a new positional or flag doesn't.
What changed
test/lib/completion-coverage.test.tsnow checks every visible, non-boolean flag on every visible command, not just positionals. A failure names the command and the flag.sentry-cli/require-completion-parser(insrc/commands/andsrc/lib/). It rejects parsers on positionals and parsed flags that can never carry a completion kind: built-ins (String,Number,Boolean,BigInt,Date,parseInt,parseFloat,JSON.parse, …), inline functions, andboundArgumentswithout akind. Named parsers pass lint; the coverage test still checks at runtime that each one is tagged. It reports nothing on the current code.lint-rules/completion-exemptions.json(file, command, parameter, reason). The lint rule and the coverage test both read it, and the test fails on an entry that no longer needs exempting. It starts empty. Anoxlint-disable-next-line sentry-cli/require-completion-parser -- reasoncomment silences only the lint rule; the coverage test cannot see comments, so the parameter still needs a kind or an entry.choice-listkind for comma-separated values.--fields id,ti<TAB>offersid,title, leaves out fields already chosen, and handles nested dot paths.buildFieldsFlagtags it once, and the generator writes each command's schema field names.PARSER_TARGETSnow covers the shared numeric, cursor, and limit parsers (text). It also covers fixed-value parsers such as sort, status, format, span depth, install method, HTTP method, and release channel. Their values come from the modules that define them; the lists are not duplicated.--platformcompletes from the platform list. Path flags complete as files or directories.--queryuses issue-query where the query is an issue search.parse: Stringis replaced with a named, tagged parser fromcompletion-target.ts. AddedtextParser(parse), andchoiceParsernow accepts an optionalparse.list-command.tstoarg-parsing.tsto avoid an import cycle.packages/cli/AGENTS.mdto cover flags, the lint rule, and the allow-list.src/lib/completions.tsis untouched so it doesn't conflict with the parallel shell-script work.Visible non-boolean flags: 466. Without a kind: 447 before, 0 after.
Verification
pnpm run typecheck,pnpm run lint(root and CLI), andpnpm run test:ci-projects(124/124, includes the lint-rule tests) all pass.test/lib test/commands test/types test/script): 12,733 pass, 7 fail. The 7 are sandbox-only filesystem and permission failures in files this PR doesn't touch:completions,upgrade,delta-upgrade,db/resilient,dsn/code-scanner, andinit/tools/file-changes.Stringfails both the lint rule and the coverage test. With an allow-list entry for it, both pass. Keeping the entry after restoring the parser fails the coverage test as stale. A lint comment alone passes lint but still fails the coverage test.JSON.parse, inline arrow and method parsers, positionals in lib helpers,boundArgumentswithoutkind, and allow-list hits and misses.origin/main(no rebase). It conflicted only on thelist-command.tsimports, and theintrospecttest now expects main's newflagNamesfield.Follow-ups
--environment,--release,--dist,--team, and bare-slug project flags. These are text for now.--fieldsflags belong to commands that have no output schema, so they fall back to text.via @BYK.
--
View Session in Sentry