Repository navigation
ref(cli): Parse dashboard and issue arguments with the shared ID grammar - #1730
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
e4ed966 to
30a9d53
Compare
BYK
left a comment
There was a problem hiding this comment.
Question: can we not extend the existing <id> parsers to <id-or-title> and maybe use it for search? I can see this then being used for issues too. Not sure if replays have titles but we may use this for traces, spans, logs etc. to matche on something?
Too stretched?
BYK
left a comment
There was a problem hiding this comment.
dashboard view my-org My Dashboard (unquoted title) still uses My, but now warns that Dashboard was ignored instead of dropping it silently.
Can we not make this work somehow?
Half-stretched, I think. It works for kinds that have a name people actually type, and stops working where there isn't one.
I'd keep that out of this PR, which is a behavior-preserving move, and do it as a follow-up: add
Yes, mostly. Two parts:
|
Dashboard commands parsed their positionals by hand inside `func` (`parseDashboardPositionalArgs`, `parseDashboardListArgs`). They now use `grammar/dashboard-arguments.pegjs` and parse before `func` runs with `boundArguments`, like the ID commands. Dashboards are named by numeric ID or title, and titles can hold spaces and slashes, so they can't share the hex-ID grammar. They follow its conventions instead: parsing has no side effects, a pasted URL's server and any warnings are applied on resolve, and extra arguments are ignored with a warning. Part of #1564 Co-Authored-By: BYK <burak.kaya@sentry.io>
CI lints before the grammar is generated, so a type imported from src/generated/ is an error type there. Infer the URL shape instead, like id-arguments.ts does.
`dashboard view my-org My Dashboard` took "My" and ignored "Dashboard" with a warning. After a target or an org URL, the remaining arguments now form one title, like the `dashboard list` filter. Arguments after a dashboard URL are still extra.
30a9d53 to
7386945
Compare
…get-parser Conflicts: packages/cli/AGENTS.md (kept main's event view note and this branch's dashboard note), packages/cli/lint-rules/cli-oxlint-plugin.js (kept both EventViewArguments and the Dashboard*Arguments types). Co-Authored-By: BYK <burak.kaya@sentry.io>
…ns (#1739) Part of #1564. Every command that still parses a positional inside `func` is now named on a lint allow-list with its follow-up, so the rest can be migrated one by one and no new one can appear. Release commands, the small straggler, move to a bound positional. A new table-driven test checks that commands taking IDs read the same inputs the same way. ## Lint - `no-ad-hoc-id-arguments` now also rejects `parseIssueTarget` and `parseDashboardTarget` in commands. These parsers only tag a positional for completion and leave the parsing to `func`. - `require-target-resolution` now also rejects `parseOrgTarget`, `parseOrgProjectTarget`, `parseOrgProjectArg`, and the `resolve{OrgOnly,OrgOptional,ProjectBound}FromArg` helpers in commands. - Both rules use allow-lists (`AD_HOC_ID_PARSING_ALLOWED`, `UNBOUND_TARGET_PARSING_ALLOWED`), and each entry names its follow-up. The lists only shrink: an allow-listed file that stops using the names gets "Remove this file from the allow-list". - `event view` is no longer exempt, since #1733 merged. - `ReleaseTarget` was added to `BOUND_TARGET_TYPES`. - New rule tests are in `scripts/cli-lint-rules.test.mjs`. ## Release positional `releaseParameter(USAGE_HINT, brief)` in `release/parse.ts` returns a bound `ReleaseTarget`. `func` reads `{ version, org, detectedFrom }` from `await target.resolved`. A blank version now fails the parse (exit 252) instead of failing inside `func`. Migrated commands: archive, create, delete, deploy, deploys, finalize, restore, set-commits, view. ## Cross-command conventions test `test/lib/argument-conventions.test.ts` runs the same inputs through each command's own positional with `applyBoundPositionals`, the way Stricli does. The inputs are: - a bare ID - `<x>/<id>` - `<org>/<project>/<id>` - `<org>/<project> <id>` - reversed arguments (expects a warning) - extra or multiple IDs - a missing ID (expects `ArgumentParseError`) - a pasted URL (its server must be applied only when `.resolved` runs) Commands covered: trace view/logs, span view/list, log view/list, event view, replay view/download, the single-issue commands (issue and feedback), and all release commands. Where a command legitimately differs, its row says how: - `slashTarget: "project"` for span view, log view and event view - `multiple` for span view, log view and event view - `optional` for span list and log list - `separateTarget: false` for span view - no URL for log view ## Audit: positional parsing in `func` Excludes `event view` (#1733, merged) and `dashboard/*` (#1730). **Already on bound parsers (verified)** - [x] `trace view`, `trace logs` (#1711, #1728) - [x] `span view`, `span list` (#1711, #1728) - [x] `log view`, `log list`: fully migrated in #1714. The remaining `parseOrgProjectInput` call is inside the bound `parseLogListInput`, not in `func`. - [x] `replay view`, `replay download`: fully migrated in #1714 (`parseIdArgument` is called inside the bound parser). - [x] Single-issue commands (`issuePositional`, #1729) and `event view` (#1733) - [x] `release *` (9 commands): **migrated in this PR** **Allow-listed, still parsing in `func`** - [ ] `issue view`, `issue explain` (`issue/utils.ts`), `issue merge`: these take several issues and need an array issue positional (#1564) - [ ] `alert issues create/delete/edit/view`: `[<org>/[<project>/]]<rule>`, resolving to every matching project (#1564: alert rule target) - [ ] `alert metrics create/delete/edit/list/view`: `[<org>/]<rule>`, org-scoped (#1564: alert rule target) - [ ] `agent-conversation view`, `integration delete`: `[<org>/]<id>` with a non-hex ID, the same shape as `releaseParameter` (#1564: org-scoped ID) - [ ] `org view`, `trial list`, `trial start`: a bare org positional (#1564: org positional) - [ ] `init`, `project create`: a target followed by other positionals (#1564) - [ ] `dashboard create`, `dashboard list/restore/revisions/view`, `dashboard widget add/delete/edit`: covered by #1730. These entries will fail the ratchet once #1730 lands, and whichever PR merges second removes them. **Not target parsing, and not covered by these rules** - `monitor run`: `args[0]` is a monitor slug followed by a passthrough command. This is arity handling, not target parsing. - Paths, endpoints, versions, selectors and doc parts: `api`, `auth login/switch`, `build/debug-files/snapshots upload`, `debug-files find` (debug IDs), `docs *`, `help`, `local *`, `schema`, `cli upgrade`, `org list`, `react-native xcode`, `wasm-split`. ## Checks `pnpm run tsc` and `pnpm run lint` pass, and the lint-rule tests pass (70/70). The new conventions test and the release and issue tests pass. The only full-suite failures locally are sandbox noise: tests that need to run without root, e2e and init-eval tests that need a build or keys, and timing-sensitive tests. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3Af950a2e3aa4d0471d8b1b07a64bfa60d --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/agent%3Af950a2e3aa4d0471d8b1b07a64bfa60d/?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>
…get-parser Conflicts: packages/cli/AGENTS.md (kept both this branch's dashboard note and main's argument-conventions note; dropped parseDashboardTarget, which this branch removes, from main's list of completion-only parsers). Also removed the dashboard entries from the #1739 allow-lists in packages/cli/lint-rules/cli-oxlint-plugin.js, since these commands now parse before func. Co-Authored-By: BYK <burak.kaya@sentry.io>
`id-arguments.pegjs` now has kinds that name items without an ID (`IdKind.names`): what is not, and may not be, an ID is a name. Dashboards (`ID_KINDS.dashboard`) are a numeric ID or a title, which may hold spaces and slashes and, after a target, takes the rest of the arguments. Issues (`ID_KINDS.issue`) are an identifier, one whole token that `issue-identifier.ts` takes apart with the #1729 grammar, or search text. The dashboard grammar is gone. The parser makes no decision about a bare word before a dashboard title: it returns both readings. `resolveDashboardInput` checks the word as an org (cache first, then the API) and, on a 404/403, falls back to the whole input as the title in the detected org, with a warning; when that misses too, one error names both readings. A server error propagates. Search text for an issue resolves through the org's issue search: the one issue it matches, or an error listing none or several. Trace, span, log, and replay IDs fail on text with a hint to search for it, from `validate()` so recovery comes first. `event view` tells issues from events with `ID_KINDS.issue.distinctForm`, keeping #1738's order. Co-Authored-By: BYK <burak.kaya@sentry.io>
…#1742) Part of #1564. Follow-up to #1730 and #1739. #1739 added `argument-conventions.test.ts`, which checks that commands sharing a parser read the same input the same way. #1730 then moved dashboards and issues onto the shared ID grammar, and dashboards and issues weren't in that test yet. This adds them. It only changes `packages/cli/test/lib/argument-conventions.test.ts`. **Dashboard commands:** `view`, `restore`, `revisions`, and `widget add/edit/delete` must all read every `DASHBOARD_INPUTS` entry the same way `dashboard view` does: - Inputs: a bare ID or title, `<org>/<id>`, `<org>/<project>/<id>`, a separate target, `<target> <title words…>`, the bare-org fallback (`My Dashboard`), reversed arguments, extra arguments, and dashboard and org URLs. - Usage-hint text in warnings is normalized before comparing, and `widget add`'s own widget title is removed first. `widget add` only gets inputs of up to 2 arguments, because its last argument is the widget title. - Per command: a missing dashboard fails at parse time, a pasted URL's server is applied on resolve and not on parse, `widget add` splits off its title, and `widget add` rejects more than 3 arguments. **`dashboard list`:** its own block, because it reads a target plus a title filter, not a dashboard. A dashboard URL fails with a suggestion to use `dashboard view`. **Issue commands:** `ISSUE_INPUTS` gains `login timeout`, so every issue command must read search text the same way `issue resolve` does. A separate test checks that it parses as `{ type: "search" }`. No lint allow-list changes: #1730 already removed the dashboard entries. The `issue/utils.ts` and `issue/merge.ts` entries are still in use and get migrated separately. Verification: - `argument-conventions.test.ts`: 336 passed, 19 skipped. - Dashboard, issue and event command suites, plus the id-arguments and dashboard-arguments lib tests: 31 files pass. - `pnpm run lint` in packages/cli is clean, `tsc --noEmit` is clean, and `scripts/cli-lint-rules.test.mjs` passes 70/70. <!-- 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>
Part of #1564. Dashboard commands parsed their positionals by hand inside
func, and issue commands used their own grammar (#1729). This moves both onto the shared ID grammar (grammar/id-arguments.pegjs, #1714), parsed beforefuncruns. Event view's issue checks (#1733, #1738) use the new issue kind too.Shared grammar: kinds with names.
IdKind.namesadds one idea toid-arguments.pegjs: a kind may name an item without its ID, and anything that is not, and may not be, an ID is a name. Kinds without names keep their grammar paths exactly as before. Kinds with names keep each line whole (no splitting at spaces), soTop 10stays one title and never becomes titleTop+ ID10.ID_KINDS.dashboard: a numeric ID or a title, which may hold spaces and slashes (Latency/p95).<org>/[<project>/]<digits>reads as a path only when slugs come before the digits, soErrors.v2/123and/123are titles. After a target, the title takes the rest of the arguments (my-org/ My Dashboard). A bare first word names an org (orgScoped).ID_KINDS.issue: an identifier (numeric ID, short ID, selector, issue URL) is one whole token (wholeIds).src/lib/issue-identifier.tstakes it apart with the ref(cli): Parse issue arguments with their own grammar, before the command runs #1729 grammar, which is unchanged. Text with spaces is a name, meaning a search.dashboard-arguments.pegjsis gone.dashboard listkeeps its thinparseDashboardListArgs, because a bare word there is a title filter (see ref(cli): Parse list command targets in their positional #1711).Bare first word: org or title? For
dashboard view My Dashboard, the parser makes no decision. It returns the structural reading (orgMy, titleDashboard) and the whole input as one title (nameWithTarget, surfaced asorgOrTitle).resolveDashboardInputdecides after auth:Myis an org: the cached org list and region cache, thenGET /organizations/My/only on a cache miss.No org 'My'; treating "My Dashboard" as the dashboard title. Use my-org/ to name an org.and looks up "My Dashboard" in the auto-detected org.Myto findDashboardin, and noMy Dashboardin<detected-org>) and hints at<org>/ Dashboardand at quoting.my-org/ …(explicit org), a numeric ref, or a URL.Issue search text.
issue view "login timeout"resolves throughGET /organizations/{org}/issues/?query=…in the detected org. One hit is the issue. Zero or several hits raise aResolutionErrorthat lists up to 5 candidates as runnable commands and points atsentry issue list <org>/ --query '…'. I checked the endpoint against getsentry/sentry (organization_group_index.py,group_index/index.py):querytakes free text, an exact event ID or short ID gets a fast lookup, and an explicit query replaces the defaultis:unresolved.Search hints. Text where a trace, span, log, or replay ID belongs (
trace view login) now fails withNot a trace ID: "login". To search, try `sentry trace list --query 'login'`.. The hint comes fromvalidate(), so commands that recover malformed IDs (trace view,span view,log view) recover first and use the hint only as their final error. Event IDs get no hint, becauseevent listlists one issue's events.event view.
ID_KINDS.issue.distinctForm(uppercase short ID or numeric) replaceslooksLikeIssueShortId+isAllDigitsand keeps #1738's order:<org>/<short-id>, then<short-id>/<event-id>(even an all-digit event ID), then<org>/<numeric-id>. I added a regression test for the all-digit event ID and one for that order.Behavior changes:
dashboard view my-org My Dashboard(unquoted title) looks up "My Dashboard". Before, it usedMyand droppedDashboard. The same goes formy-org/ My Dashboardandhttps://acme.sentry.io/ My Dashboard.dashboard view My Dashboard: when there is no orgMy(404/403), it falls back to the title "My Dashboard" in the detected org, with the warning above. If neither reading finds a dashboard, one error names both. Dashboard commands with one dashboard ref get this (view,restore,revisions,widget edit/delete, andwidget addafter it splits off its widget title).dashboard view 42 X,my-org/42 X. Before, the first of two arguments was always the target. After a target, a numeric ID keeps the rest as extra (my-org 123 extra→ ID 123, ignoredextra) instead of joining them into a title. Arguments after the ID go through the shared rules: extra, reversed (42 my-org/), or the shared "reference different projects" error when they name another target.issue view "login timeout"(text with spaces) searches instead of failing as a malformed identifier. Control characters and URLs are still rejected.list, an invalid target slug) fail before the auth guard, as in the earlier commits on this branch. They printFailed to parse "…" for <placeholder>: …and exit 252.widget add/edit/delete: an argument error comes before flag errors like "Specify --index or --title".widget add's positional completes cached dashboards.parseDashboardTargethad no callers left and is removed.Lint ratchet (#1739): the dashboard entries are off both allow-lists, since these commands now parse before
func. The issue entries (issue/utils.ts,issue/merge.ts, which need an array issue positional) stay.Syncing: this branch merges
main(no rebase). 54ebfa2 and 7f52f19 merge #1729, #1732–#1734, #1736, #1738 and #1739.Verification:
tsc,pnpm run lint(oxlint, format, ast-grep) andscripts/cli-lint-rules.test.mjs(70/70) pass.test/lib,test/grammarand the dashboard, issue, event, trace, span, log, replay and feedback command tests: 10,158 passed. The 6 failures are the sandbox-only permission tests (completions EACCES/EPERM, code-scanner, file-changes, telemetry readonly, upgrade lock), which also fail onmain. The full CLI suite shows the same 6, plus e2e/eval files that need a build or credentials.my-org/ My Dashboard, and a numeric ref.validate: falsedeferral.via BYK.
--
View Junior Session in Sentry