ref(cli): Ratchet hand-parsed positionals and test argument conventions

packages/cli/AGENTS.md

@@ -250,6 +250,10 @@ the `sentry-cli/no-ad-hoc-id-arguments` lint rule rejects hand-written ID
 parsing (`parseSlashSeparatedArg`, `HEX_ID_RE`, ...) in commands and kinds
 defined outside `ID_KINDS`. Issue IDs keep their own syntax; `event view`,
 which also takes them, checks for an issue first (`parseEventViewInput`).
+`test/lib/argument-conventions.test.ts` feeds the same inputs to every
+command that takes IDs, issues, or releases, through the command's own
+positional; add a new command to its tables, and encode a legitimate
+difference (such as `needsProject`) in its row.
 
 **Issue positionals:** issue IDs, short IDs, selectors, and issue URLs are
 parsed by `parseIssueArgument` (`src/lib/issue-arguments.ts`), whose forms
@@ -284,7 +288,21 @@ project), or `orgProjectTarget(resolve)` for a command-specific resolver (see
 with `await target.resolved`. It never calls `parseOrgProjectArg` or a resolver
 itself, and an omitted target arrives as auto-detect. The
 `sentry-cli/require-target-resolution` lint rule fails a function that takes
-one of these types and never reads `.resolved`.
+one of these types and never reads `.resolved`. It also rejects, in commands,
+what leaves a target for `func` to parse: the completion-only parsers
+`parseOrgTarget` and `parseOrgProjectTarget`, `parseOrgProjectArg`, and the
+`resolve*FromArg` helpers. Commands not migrated yet are on
+`UNBOUND_TARGET_PARSING_ALLOWED` in `lint-rules/cli-oxlint-plugin.js`, each
+with its follow-up; the list is a ratchet, so a file that stops using them
+fails until its entry is removed. `AD_HOC_ID_PARSING_ALLOWED` does the same
+for ID positionals, including the completion-only `parseIssueTarget` and
+`parseDashboardTarget`.
+
+**Release positionals:** `[<org>/]<version>` uses
+`releaseParameter(USAGE_HINT, brief)` from `src/commands/release/parse.ts`
+inside a tuple positional. `func` takes a `ReleaseTarget` and gets
+`{ version, org, detectedFrom }` from `await target.resolved`; a blank version
+fails the parse.
 
 List commands take their target from `listTargetPositional()` in
 `src/lib/list-command.ts` (`LIST_TARGET_POSITIONAL`,
@@ -340,13 +358,16 @@ 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:** Target positionals that take an org, project, issue, or
-dashboard use a completion-aware parser from `src/lib/completion-target.ts`
-instead of `String`: `parseOrgProjectTarget` completes `org/`, `org/project`,
-and project aliases; `parseOrgTarget` completes org slugs only;
-`parseIssueTarget` adds cached issue short IDs; `parseDashboardTarget`
-completes cached dashboards. `issueIdsPositional` already uses them. A
-parser tagged with `withCompletionTarget`, such as the `target-positional.ts`
-parsers behind `listTargetPositional`, completes the same way. Shared value
+dashboard are tagged with a completion kind from `src/lib/completion-target.ts`
+(`ORG_PROJECT_TARGET` completes `org/`, `org/project`, and project aliases;
+`ORG_TARGET` org slugs only; `ISSUE_TARGET` adds cached issue short IDs;
+`DASHBOARD_TARGET` cached dashboards). The bound parsers above
+(`boundPositional(kind, …)`, `boundArguments({ kind, … })`) carry the kind, so
+the `target-positional.ts` parsers behind `listTargetPositional`,
+`issuePositional`, and `releaseParameter` complete the right values. The
+completion-only parsers (`parseOrgProjectTarget`, `parseOrgTarget`,
+`parseIssueTarget`, `parseDashboardTarget`) remain for the allow-listed
+commands that still parse in `func`; new commands use a bound parser. 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

packages/cli/lint-rules/cli-oxlint-plugin.js

@@ -20,8 +20,8 @@ function rule(message, visitors) {
     create(context) {
       const filename = context.filename.replaceAll("\\", "/");
       if (!filename.includes("/packages/cli/")) return {};
-      return visitors(context, filename, (node) =>
-        context.report({ node, message }),
+      return visitors(context, filename, (node, override = message) =>
+        context.report({ node, message: override }),
       );
     },
   };
@@ -65,6 +65,7 @@ const BOUND_TARGET_TYPES = new Set([
   "OrgOptionalTarget",
   "OrgProjectTarget",
   "ProjectBoundTarget",
+  "ReleaseTarget",
   "BoundArguments",
   "EventViewArguments",
   "LogListArguments",
@@ -157,15 +158,127 @@ function regexSource(node) {
 /**
  * Helpers that parse ID positionals by hand. Commands parse IDs with
  * `parseIdArguments()` (src/lib/id-arguments.ts), so every command takes
- * the same forms.
+ * the same forms. `parseIssueTarget` and `parseDashboardTarget` only tag a
+ * positional for completion, leaving the parsing to `func`; issues have
+ * their own grammar (`issuePositional`, src/lib/issue-arguments.ts).
  */
 const AD_HOC_ID_PARSING = new Set([
   "parseSlashSeparatedArg",
   "detectSwappedViewArgs",
   "HEX_ID_RE",
   "SPAN_ID_RE",
+  "parseIssueTarget",
+  "parseDashboardTarget",
 ]);
 
+/**
+ * Commands still allowed to, each with its follow-up. A ratchet: an entry
+ * whose file no longer uses them is reported, so the list only shrinks.
+ */
+const AD_HOC_ID_PARSING_ALLOWED = new Map([
+  // #1730 parses dashboard arguments with their own grammar
+  ["/src/commands/dashboard/restore.ts", "#1730"],
+  ["/src/commands/dashboard/revisions.ts", "#1730"],
+  ["/src/commands/dashboard/view.ts", "#1730"],
+  ["/src/commands/dashboard/widget/delete.ts", "#1730"],
+  ["/src/commands/dashboard/widget/edit.ts", "#1730"],
+  // Follow-up (#1564): several issues per command (`issue view`,
+  // `issue explain`, `issue merge`) need an array issue positional
+  ["/src/commands/issue/utils.ts", "#1564: array issue positional"],
+  ["/src/commands/issue/merge.ts", "#1564: array issue positional"],
+]);
+
+/**
+ * Helpers that leave a target positional for `func` to parse:
+ * `parseOrgTarget` and `parseOrgProjectTarget` only tag it for completion,
+ * and the others parse a raw string (applying a pasted URL's server as they
+ * go). Commands take targets as bound positionals instead
+ * (src/lib/target-positional.ts, `boundArguments`), which parse before the
+ * command runs and resolve on `target.resolved`.
+ */
+const UNBOUND_TARGET_PARSING = new Set([
+  "parseOrgTarget",
+  "parseOrgProjectTarget",
+  "parseOrgProjectArg",
+  "resolveOrgOnlyFromArg",
+  "resolveOrgOptionalFromArg",
+  "resolveProjectBoundFromArg",
+]);
+
+/** Commands still allowed to, each with its follow-up; a ratchet too. */
+const UNBOUND_TARGET_PARSING_ALLOWED = new Map([
+  // #1730 parses dashboard arguments with their own grammar
+  ["/src/commands/dashboard/list.ts", "#1730"],
+  ["/src/commands/dashboard/restore.ts", "#1730"],
+  ["/src/commands/dashboard/revisions.ts", "#1730"],
+  ["/src/commands/dashboard/view.ts", "#1730"],
+  ["/src/commands/dashboard/widget/add.ts", "#1730"],
+  ["/src/commands/dashboard/widget/delete.ts", "#1730"],
+  ["/src/commands/dashboard/widget/edit.ts", "#1730"],
+  // Follow-ups (#1564):
+  // `[<org>/[<project>/]]<rule>`, resolving to every matching project
+  ["/src/commands/alert/issues/create.ts", "#1564: alert rule target"],
+  ["/src/commands/alert/issues/delete.ts", "#1564: alert rule target"],
+  ["/src/commands/alert/issues/edit.ts", "#1564: alert rule target"],
+  ["/src/commands/alert/issues/view.ts", "#1564: alert rule target"],
+  // `[<org>/]<rule>`, org-scoped
+  ["/src/commands/alert/metrics/create.ts", "#1564: alert rule target"],
+  ["/src/commands/alert/metrics/delete.ts", "#1564: alert rule target"],
+  ["/src/commands/alert/metrics/edit.ts", "#1564: alert rule target"],
+  ["/src/commands/alert/metrics/list.ts", "#1564: alert rule target"],
+  ["/src/commands/alert/metrics/view.ts", "#1564: alert rule target"],
+  // `[<org>/]<id>` with a non-hex ID, like `releaseParameter`
+  ["/src/commands/agent-conversation/view.ts", "#1564: org-scoped ID"],
+  ["/src/commands/integration/delete.ts", "#1564: org-scoped ID"],
+  // An org slug alone
+  ["/src/commands/org/view.ts", "#1564: org positional"],
+  ["/src/commands/trial/list.ts", "#1564: org positional"],
+  ["/src/commands/trial/start.ts", "#1564: org positional"],
+  // A target followed by other positionals
+  ["/src/commands/dashboard/create.ts", "#1564: target and title"],
+  ["/src/commands/init.ts", "#1564: target and directory"],
+  ["/src/commands/project/create.ts", "#1564: name and platform"],
+]);
+
+/** The follow-up of `file` in an allow-list, if it is on it. */
+function allowedFollowUp(allowed, file) {
+  for (const [path, followUp] of allowed) {
+    if (file.endsWith(path)) return followUp;
+  }
+  return undefined;
+}
+
+/**
+ * Visitors reporting each reference to one of `names` in a command file,
+ * except in files on `allowed`; those are reported instead when they no
+ * longer reference any, so the allow-list only shrinks. Imports and
+ * `typeof` references are not uses.
+ */
+function ratchetedNames(file, names, allowed, report, message) {
+  if (!file.includes("/src/commands/")) return {};
+  const followUp = allowedFollowUp(allowed, file);
+  let used = false;
+  return {
+    Identifier(node) {
+      if (
+        !names.has(node.name) ||
+        node.parent?.type === "ImportSpecifier" ||
+        node.parent?.type === "TSTypeQuery"
+      )
+        return;
+      used = true;
+      if (followUp === undefined) report(node, message);
+    },
+    "Program:exit"(node) {
+      if (followUp !== undefined && !used)
+        report(
+          node,
+          `Remove this file from the allow-list (${followUp}): it no longer parses its positional by hand.`,
+        );
+    },
+  };
+}
+
 const ID_ARGUMENT_PARSERS = new Set(["parseIdArguments", "parseIdArgument"]);
 
 /** Whether `node` is `ID_KINDS.<kind>`. */
@@ -342,6 +455,13 @@ const rules = {
         FunctionDeclaration: check,
         FunctionExpression: check,
         ArrowFunctionExpression: check,
+        ...ratchetedNames(
+          file,
+          UNBOUND_TARGET_PARSING,
+          UNBOUND_TARGET_PARSING_ALLOWED,
+          report,
+          "Take the target as a bound positional (src/lib/target-positional.ts, boundArguments) instead of parsing it in func.",
+        ),
       };
     },
   ),
@@ -350,16 +470,13 @@ const rules = {
     (_context, file, report) => {
       if (!file.includes("/src/") || file.endsWith("/lib/id-arguments.ts"))
         return {};
-      const commands = file.includes("/src/commands/");
       return {
-        Identifier(node) {
-          if (
-            commands &&
-            AD_HOC_ID_PARSING.has(node.name) &&
-            node.parent?.type !== "ImportSpecifier"
-          )
-            report(node);
-        },
+        ...ratchetedNames(
+          file,
+          AD_HOC_ID_PARSING,
+          AD_HOC_ID_PARSING_ALLOWED,
+          report,
+        ),
         CallExpression(node) {
           if (
             ID_ARGUMENT_PARSERS.has(node.callee.name) &&

packages/cli/src/commands/release/archive.ts

@@ -9,7 +9,6 @@
 import type { SentryContext } from "../../context.js";
 import { updateRelease } from "../../lib/api-client.js";
 import { buildCommand } from "../../lib/command.js";
-import { parseOrgTarget } from "../../lib/completion-target.js";
 import {
   colorTag,
   escapeMarkdownInline,
@@ -20,7 +19,7 @@ import {
 import { CommandOutput } from "../../lib/formatters/output.js";
 import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../lib/mutate-command.js";
 import type { SentryRelease } from "../../types/index.js";
-import { resolveReleaseTarget } from "./parse.js";
+import { type ReleaseTarget, releaseParameter } from "./parse.js";
 
 const USAGE_HINT = "sentry release archive [<org>/]<version>";
 
@@ -76,11 +75,10 @@ export const archiveCommand = buildCommand({
     positional: {
       kind: "tuple",
       parameters: [
-        {
-          placeholder: "org/version",
-          brief: "[<org>/]<version> - Release version to archive",
-          parse: parseOrgTarget,
-        },
+        releaseParameter(
+          USAGE_HINT,
+          "[<org>/]<version> - Release version to archive",
+        ),
       ],
     },
     flags: {
@@ -95,15 +93,9 @@ export const archiveCommand = buildCommand({
       readonly json: boolean;
       readonly fields?: string[];
     },
-    target: string,
+    target: ReleaseTarget,
   ) {
-    const { cwd } = this;
-
-    const { version, org, detectedFrom } = await resolveReleaseTarget(
-      target,
-      USAGE_HINT,
-      cwd,
-    );
+    const { version, org, detectedFrom } = await target.resolved;
 
     if (flags["dry-run"]) {
       yield new CommandOutput({ dryRun: true, version, org });

packages/cli/src/commands/release/create.ts

@@ -7,7 +7,6 @@
 import type { SentryContext } from "../../context.js";
 import { createRelease } from "../../lib/api-client.js";
 import { buildCommand } from "../../lib/command.js";
-import { parseOrgTarget } from "../../lib/completion-target.js";
 import {
   colorTag,
   escapeMarkdownInline,
@@ -18,7 +17,7 @@ import {
 import { CommandOutput } from "../../lib/formatters/output.js";
 import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../lib/mutate-command.js";
 import type { SentryRelease } from "../../types/index.js";
-import { resolveReleaseTarget } from "./parse.js";
+import { type ReleaseTarget, releaseParameter } from "./parse.js";
 
 const USAGE_HINT = "sentry release create [<org>/]<version>";
 
@@ -112,11 +111,10 @@ export const createCommand = buildCommand({
     positional: {
       kind: "tuple",
       parameters: [
-        {
-          placeholder: "org/version",
-          brief: "[<org>/]<version> - Release version to create",
-          parse: parseOrgTarget,
-        },
+        releaseParameter(
+          USAGE_HINT,
+          "[<org>/]<version> - Release version to create",
+        ),
       ],
     },
     flags: {
@@ -158,15 +156,9 @@ export const createCommand = buildCommand({
       readonly json: boolean;
       readonly fields?: string[];
     },
-    target: string,
+    target: ReleaseTarget,
   ) {
-    const { cwd } = this;
-
-    const { version, org } = await resolveReleaseTarget(
-      target,
-      USAGE_HINT,
-      cwd,
-    );
+    const { version, org } = await target.resolved;
 
     const body: Parameters<typeof createRelease>[1] = { version };
     if (flags.project) {

packages/cli/src/commands/release/delete.ts

@@ -9,7 +9,6 @@
 
 import type { SentryContext } from "../../context.js";
 import { deleteRelease, getRelease } from "../../lib/api-client.js";
-import { parseOrgTarget } from "../../lib/completion-target.js";
 import { ApiError } from "../../lib/errors.js";
 import { renderMarkdown, safeCodeSpan } from "../../lib/formatters/markdown.js";
 import { CommandOutput } from "../../lib/formatters/output.js";
@@ -19,7 +18,7 @@ import {
   isConfirmationBypassed,
 } from "../../lib/mutate-command.js";
 import { buildReleaseUrl } from "../../lib/sentry-web-urls.js";
-import { resolveReleaseTarget } from "./parse.js";
+import { type ReleaseTarget, releaseParameter } from "./parse.js";
 
 const USAGE_HINT = "sentry release delete [<org>/]<version>";
 
@@ -105,22 +104,15 @@ export const deleteCommand = buildDeleteCommand({
     positional: {
       kind: "tuple",
       parameters: [
-        {
-          placeholder: "org/version",
-          brief: "[<org>/]<version> - Release version to delete",
-          parse: parseOrgTarget,
-        },
+        releaseParameter(
+          USAGE_HINT,
+          "[<org>/]<version> - Release version to delete",
+        ),
       ],
     },
   },
-  async *func(this: SentryContext, flags: DeleteFlags, target: string) {
-    const { cwd } = this;
-
-    const { version, org } = await resolveReleaseTarget(
-      target,
-      USAGE_HINT,
-      cwd,
-    );
+  async *func(this: SentryContext, flags: DeleteFlags, target: ReleaseTarget) {
+    const { version, org } = await target.resolved;
 
     // Verify the release exists before prompting for confirmation
     const release = await getRelease(org, version);