Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .oxlintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
"sentry-cli/no-direct-target-resolution": "error",
"sentry-cli/require-target-resolution": "error",
"sentry-cli/no-ad-hoc-id-arguments": "error",
"sentry-cli/require-completion-parser": "error",
"sentry-cli/no-generic-is-record": "error",
"sentry-cli/prefer-paginate-helper": "error",
"sentry-cli/no-manual-pagination": "error",
Expand Down
2 changes: 1 addition & 1 deletion apps/local/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"build": "tsc -b && vite build",
"lint": "oxlint",
"preview": "vite preview",
"test": "pnpm --filter sentry exec node script/ensure-generated.mjs schema && vitest run",
"test": "pnpm --filter sentry exec node script/ensure-generated.mjs schema grammar && vitest run",
"test:integration": "pnpm run test src/App.integration.test.tsx",
"typecheck": "tsc -b",
"sample": "tsx scripts/replay.ts"
Expand Down
57 changes: 41 additions & 16 deletions packages/cli/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -391,35 +391,60 @@ grammar knows no kind of ID: the command's `ID_KINDS` entry tells it what an
ID looks like, so the syntax cannot drift between commands. It accepts any
input; errors come from `parseIdArguments`, never Peggy's own syntax errors.

**Shell completion:** Every visible positional declares a completion kind,
a key of `COMPLETERS` in `src/lib/complete.ts`; `test/lib/completion-coverage.test.ts`
walks the route tree and fails, naming the command and placeholder, for any
positional without one. Pick the parser by what the argument takes:
**Shell completion:** Every visible positional and every visible
value-taking flag declares a completion kind, a key of `COMPLETERS` in
`src/lib/complete.ts`; `test/lib/completion-coverage.test.ts` walks the
route tree and fails, naming the command and the placeholder or flag, for
any without one. The `sentry-cli/require-completion-parser` lint rule
rejects, in `src/commands/` and `src/lib/`, the parsers that cannot carry a
kind: built-ins (`parse: String`, `Number`, `Boolean`, `Date`, `parseInt`,
`JSON.parse`, …), inline functions (`parse: (value) => …`), and
`boundArguments` without a `kind`. A flag with nothing to suggest says so
with `parseText` or `textParser(parse)`. The rare parameter that must stay
without a kind goes in `lint-rules/completion-exemptions.json` with its
file, command, parameter (`--flag` or `<placeholder>`), and reason; both the
lint rule and the coverage test read it, and the test fails on an entry
that no longer needs exempting. An `oxlint-disable-next-line
sentry-cli/require-completion-parser -- reason` comment silences only the
lint rule; the coverage test cannot see comments, so the parameter still
needs a kind or an allow-list entry. Pick the parser by what the argument
takes:

| Argument | Parser | Completes |
|----------|--------|-----------|
| Org, project, issue, dashboard target | the bound parsers above (`boundPositional(kind, …)`, `boundArguments({ kind, … })`, `listTargetPositional`, `issuePositional`, `releaseParameter`) | `ORG_TARGET` org slugs; `ORG_PROJECT_TARGET` `org/`, `org/project`, aliases; `ISSUE_TARGET` plus cached issue short IDs; `DASHBOARD_TARGET` cached dashboards |
| File or directory path | `parseFilePath` | entries of the partial's directory, directories with a trailing `/` |
| File or directory path (`--output`, `--ignore-file`) | `parseFilePath` | entries of the partial's directory, directories with a trailing `/` |
| Directory | `parseDirectoryPath` | directories only |
| Target or directory (`init`) | `boundArguments({ kind: ORG_PROJECT_OR_DIRECTORY_TARGET, … })` | `org/project` targets, or directories once the word looks like a path (`./`, `/`, `~/`), so a suggestion is never read as a target |
| Fixed values | `choiceParser(values)` (take the values from the module that defines them) | those values; a variadic positional completes only its first word (`cli defaults <key> <value>`). The parser returns its input, so the command still validates it |
| Stored account | `parseAccountSelector` | stored accounts (`auth switch`, `auth logout`) |
| Fixed values | `choiceParser(values)`, or `choiceParser(values, parse)` to keep a command's own validating parser (`choiceParser(VALID_SORT_VALUES, parseSort)`). Take the values from the module that defines them | those values; a variadic positional completes only its first word (`cli defaults <key> <value>`). Without `parse` the parser returns its input, so the command still validates it |
| Comma-separated fixed values (`--scope`, `--features`) | `choiceListParser(values)` (or `(values, parse)`) | the next item of the list, leaving out the items already in it (`org:read,org:w` → `org:read,org:write`) |
| Platform | `parsePlatform` | platform identifiers, common ones first |
| Stored account | `parseAccountSelector` | stored accounts (`auth switch`, `auth logout`, `org list --account`) |
| CLI command path | `parseCommandPath` | route names, one segment per word (`help`) |
| URL | `parseUrl` (the command validates it) | nothing |
| Free text, IDs with no cache, names of new things | `parseText` | nothing |
| Numbers and other values a command parses itself (`parsePort`, `parseLimit`) | `textParser(parse)` | nothing |

Import the pre-tagged parsers; never tag `String` or write
`withCompletionTarget` in a command (the lint rules above reject it there).
Org, project, and issue targets stay bound parsers. `enum` flags complete
their `values` like `choiceParser`.
`withCompletionTarget` in a command (the lint rules above reject it there),
and never leave `parse: String` on a flag. Org, project, and issue targets
stay bound parsers. `enum` flags complete their `values` like
`choiceParser`. `buildCommand` tags the injected `--fields` once: with an
output schema, it completes the schema's field paths, nested ones in dot
notation (`--fields id,ti` → `id,title`, `project.s` → `project.slug`);
without one, nothing.

Shared value parsers are not tagged where they live; `PARSER_TARGETS` in
`completion-target.ts` lists them instead, so every flag that parses with
`sanitizeIssueQuery` completes issue search terms (`is:unr` →
`is:unresolved`), every flag that parses with `parsePeriod` completes
relative periods, and `parseLoginEmail` completes stored accounts. Fixed
values (search keys, period units) come from the modules that define them
through `COMPLETION_VALUES`.
`completion-target.ts` lists them instead, with a kind or the values they
accept, so every flag that parses with `sanitizeIssueQuery` completes issue
search terms (`is:unr` → `is:unresolved`), every flag that parses with
`parsePeriod` completes relative periods, `parseCursorFlag` completes
`next`/`prev`/`first`, `parseLogSort` its directions, `numberParser`
nothing, and `parseLoginEmail` completes stored accounts. Fixed values
(search keys, period units) come from the modules that define them
through `COMPLETION_VALUES`. A new shared parser goes in `PARSER_TARGETS`;
its module must not import `completion-target.ts`, or the two would load
in a cycle.
Issue and dashboard suggestions come from the `completion_cache` table. The
authenticated fetch in `src/lib/sentry-client.ts` offers every API response to
`src/lib/api/completion-seed.ts`, which stores entities from the `@sentry/api`
Expand Down
126 changes: 124 additions & 2 deletions packages/cli/lint-rules/cli-oxlint-plugin.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { readFileSync } from "node:fs";
import { marked } from "marked";

const name = "sentry-cli";
Expand All @@ -14,9 +15,9 @@ function call(node, property) {
return node.type === "CallExpression" && member(node.callee, property);
}

function rule(message, visitors) {
function rule(message, visitors, schema = []) {
return {
meta: { type: "problem", docs: { description: message }, schema: [] },
meta: { type: "problem", docs: { description: message }, schema },
create(context) {
const filename = context.filename.replaceAll("\\", "/");
if (!filename.includes("/packages/cli/")) return {};
Expand Down Expand Up @@ -226,6 +227,81 @@ function isIdKind(node) {
);
}

/**
* Parameters exempt from shell completion coverage, shared with
* test/lib/completion-coverage.test.ts.
*/
const COMPLETION_EXEMPTIONS = JSON.parse(
readFileSync(
new URL("./completion-exemptions.json", import.meta.url),
"utf8",
),
).exemptions;

/** Built-in conversions, which carry no completion kind. */
const BUILTIN_PARSERS = new Set([
"String",
"Number",
"Boolean",
"BigInt",
"Date",
"parseInt",
"parseFloat",
]);

/** Objects whose static parsers (`JSON.parse`, `Number.parseInt`) are built-ins. */
const BUILTIN_PARSER_OBJECTS = new Set(["JSON", "Number", "Date", "BigInt"]);

/** The non-computed property `key` of an object literal. */
function objectProperty(node, key) {
return node.properties.find(
(p) =>
p.type === "Property" &&
!p.computed &&
(p.key.name ?? p.key.value) === key,
);
}

/**
* The parameter an object literal defines, as the coverage test names it:
* `--name` for a flag, keyed by its name (flags can have a `placeholder`
* too), and `<placeholder>` for a positional, which sits in a tuple's
* `parameters` array or an array positional's `parameter`.
*/
function parameterName(node) {
const parent = node.parent;
if (
parent?.type === "Property" &&
parent.value === node &&
!parent.computed
) {
const key = parent.key.name ?? parent.key.value;
if (key !== "parameter") return `--${key}`;
}
const placeholder = objectProperty(node, "placeholder")?.value;
if (placeholder?.type === "Literal" && typeof placeholder.value === "string")
return `<${placeholder.value}>`;
return undefined;
}
Comment thread
cursor[bot] marked this conversation as resolved.
Resolved

/** Why a `parse` value declares no completion kind, or `undefined`. */
function untaggedParser(value) {
if (value.type === "Identifier" && BUILTIN_PARSERS.has(value.name))
return `\`${value.name}\` has no completion kind. Use parseText, textParser(${value.name}), or another tagged parser from src/lib/completion-target.ts.`;
if (
value.type === "MemberExpression" &&
value.object.type === "Identifier" &&
BUILTIN_PARSER_OBJECTS.has(value.object.name)
)
return `\`${value.object.name}.${value.property.name}\` has no completion kind. Wrap it in textParser() or choiceParser().`;
if (
value.type === "ArrowFunctionExpression" ||
value.type === "FunctionExpression"
)
return "An inline parser has no completion kind. Wrap it in textParser(), choiceParser(), or choiceListParser(), or use a bound parser.";
return undefined;
}

const rules = {
"no-silent-catch": rule(
"Handle or report caught errors instead of discarding them.",
Expand Down Expand Up @@ -417,6 +493,52 @@ const rules = {
};
},
),
"require-completion-parser": rule(
"Give every positional and value-taking flag a parser that declares its completion kind.",
(context, file, report) => {
if (!file.includes("/src/commands/") && !file.includes("/src/lib/"))
return {};
const exemptions =
context.options[0]?.exemptions ?? COMPLETION_EXEMPTIONS;
const exempt = (node) => {
const parameter = parameterName(node);
return exemptions.some(
(entry) =>
file.endsWith(`/packages/cli/${entry.file}`) &&
entry.parameter === parameter,
);
};
return {
ObjectExpression(node) {
const parse = objectProperty(node, "parse");
if (!parse) return;
// boundArguments({ kind, parse, resolve }): `kind` is the
// completion kind, and `parse` reads every argument at once.
if (objectProperty(node, "resolve")) {
if (!objectProperty(node, "kind") && !exempt(node))
report(
node,
"Give boundArguments a completion `kind` (ORG_PROJECT_TARGET, …).",
);
return;
}
const kind = objectProperty(node, "kind")?.value;
const parsedFlag =
kind?.type === "Literal" && kind.value === "parsed";
if (!parsedFlag && !objectProperty(node, "brief")) return;
const reason = untaggedParser(parse.value);
if (reason && !exempt(node)) report(parse.value, reason);
},
};
},
[
{
type: "object",
properties: { exemptions: { type: "array" } },
additionalProperties: false,
},
],
),
"no-generic-is-record": rule(
"Use a shape-specific guard instead of isRecord().",
(_context, file, report) =>
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/lint-rules/completion-exemptions.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"$comment": "Parameters exempt from shell completion coverage: the sentry-cli/require-completion-parser lint rule and test/lib/completion-coverage.test.ts both read this list. Each entry names the file that defines the parameter, the command path, the parameter (--flag or <placeholder>), and why it has no completion kind. Prefer parseText or textParser(parse) over an entry here; see \"Shell completion\" in packages/cli/AGENTS.md.",
"exemptions": []
}
15 changes: 11 additions & 4 deletions packages/cli/script/ensure-generated.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
* step to document or forget.
*
* Usage:
* node script/ensure-generated.mjs # everything the CLI imports
* node script/ensure-generated.mjs schema # only the API schema
* node script/ensure-generated.mjs # everything the CLI imports
* node script/ensure-generated.mjs schema # only the API schema
* node script/ensure-generated.mjs schema grammar # the schema and parsers
*
* Plain Node (no tsx) so the check adds no noticeable startup time.
*/
Expand Down Expand Up @@ -42,12 +43,18 @@ function run(script) {
execSync(`pnpm run ${script}`, { stdio: "inherit" });
}

/** The groups named on the command line, or every group when none are. */
const groups = process.argv.slice(2);
function wants(group) {
return groups.length === 0 || groups.includes(group);
}

if (!SCHEMA_OUTPUTS.every(existsSync)) {
run("generate:schema");
}
if (process.argv[2] !== "schema" && !GRAMMAR_OUTPUTS.every(existsSync)) {
if (wants("grammar") && !GRAMMAR_OUTPUTS.every(existsSync)) {
run("generate:grammar");
}
if (process.argv[2] !== "schema" && !DOCS_OUTPUTS.every(existsSync)) {
if (wants("docs") && !DOCS_OUTPUTS.every(existsSync)) {
run("generate:docs");
}
5 changes: 3 additions & 2 deletions packages/cli/src/commands/agent-conversation/list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import type { SentryContext } from "../../context.js";
import { getProject, listConversations } from "../../lib/api-client.js";
import { validateLimit } from "../../lib/arg-parsing.js";
import { parseText, textParser } from "../../lib/completion-target.js";
import {
advancePaginationState,
buildPaginationContextKey,
Expand Down Expand Up @@ -208,13 +209,13 @@ export const listCommand = buildListCommand("agent-conversation", {
flags: {
limit: {
kind: "parsed",
parse: parseLimit,
parse: textParser(parseLimit),
brief: `Number of conversations (${LIST_MIN_LIMIT}-${LIST_MAX_LIMIT})`,
default: String(LIST_DEFAULT_LIMIT),
},
query: {
kind: "parsed",
parse: String,
parse: parseText,
brief:
"Any matching span selects its conversation; totals include all spans in selected projects and time range",
optional: true,
Expand Down
18 changes: 11 additions & 7 deletions packages/cli/src/commands/alert/issues/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from "../../../lib/api-client.js";
import type { ParsedOrgProject } from "../../../lib/arg-parsing.js";
import { buildCommand, numberParser } from "../../../lib/command.js";
import { choiceParser, parseText } from "../../../lib/completion-target.js";
import {
ContextError,
stringifyUnknown,
Expand All @@ -27,6 +28,7 @@ import {
orgProjectTarget,
} from "../../../lib/target-positional.js";
import {
ISSUE_MATCH_MODES,
matchToLogicType,
parseJsonObjectList,
parseMatchMode,
Expand Down Expand Up @@ -150,19 +152,19 @@ export const createCommand = buildCommand({
flags: {
name: {
kind: "parsed",
parse: String,
parse: parseText,
brief: "Rule name",
},
condition: {
kind: "parsed",
parse: String,
parse: parseText,
variadic: true,
optional: true,
brief: "Condition object JSON (repeatable, or pass one JSON array)",
},
action: {
kind: "parsed",
parse: String,
parse: parseText,
variadic: true,
optional: true,
brief: "Action object JSON (repeatable, or pass one JSON array)",
Expand All @@ -175,26 +177,28 @@ export const createCommand = buildCommand({
},
environment: {
kind: "parsed",
parse: String,
parse: parseText,
optional: true,
brief: "Environment filter",
},
filter: {
kind: "parsed",
parse: String,
parse: parseText,
variadic: true,
optional: true,
brief: "Filter object JSON (repeatable, or pass one JSON array)",
},
"filter-match": {
kind: "parsed",
parse: (value: string) => parseMatchMode(value, "filter-match"),
parse: choiceParser(ISSUE_MATCH_MODES, (value) =>
parseMatchMode(value, "filter-match"),
),
optional: true,
brief: "Filter match mode: all or any",
},
owner: {
kind: "parsed",
parse: String,
parse: parseText,
optional: true,
brief: "Owner (team:user style value accepted by Sentry API)",
},
Expand Down
Loading
Loading