diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index b212ea208..db32055fd 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -225,6 +225,15 @@ export const myRoute = buildRouteMap({ }); ``` +**Reading the command tree:** code in `src/lib/` reads the top-level route map +with `getRoutes()` from `src/lib/route-registry.ts`, never by importing +`app.ts`. Every command reaches `src/lib/` through `buildCommand`, so that +import would pull every command into any bundle built from one of them. +`app.ts` registers its map with `setRoutes()`; a test that needs the real tree +imports `src/app.js`. `test/lib/route-registry.test.ts` fails when a module in +`src/lib/` reaches `app.ts`; the only exception is `sdk-invoke.ts`, the +library-mode runner that executes the whole application. + ### Positional Arguments Required identifiers (trace IDs, span IDs) should be **positional args**, not diff --git a/packages/cli/script/build.ts b/packages/cli/script/build.ts index 44d5df56b..bb4afa647 100644 --- a/packages/cli/script/build.ts +++ b/packages/cli/script/build.ts @@ -51,7 +51,7 @@ const NODE_VERSION = "lts"; /** Files whose lazy relative imports must be included in the native bundle. */ const REQUIRE_ALIAS_FILTER = - /(?:db[\\/](?:index|schema)|formatters[\\/](?:highlight|json)|list-command|telemetry)\.ts$/; + /(?:db[\\/](?:index|schema)|formatters[\\/](?:highlight|json)|telemetry)\.ts$/; const REQUIRE_ALIAS_RE = /\b_require\(/g; /** Build-time constants injected into the binary */ diff --git a/packages/cli/script/bundle.ts b/packages/cli/script/bundle.ts index cb7e9517a..308bbbeda 100644 --- a/packages/cli/script/bundle.ts +++ b/packages/cli/script/bundle.ts @@ -171,7 +171,7 @@ const sentrySourcemapPlugin: Plugin = { * `formatters/highlight` loads cli-highlight only on the first code block. */ const REQUIRE_ALIAS_FILTER = - /(?:db[\\/](?:index|schema|sqlite)|formatters[\\/](?:highlight|json)|list-command|telemetry)\.ts$/; + /(?:db[\\/](?:index|schema|sqlite)|formatters[\\/](?:highlight|json)|telemetry)\.ts$/; const REQUIRE_ALIAS_RE = /\b_require\(/g; const IDENTIFIER_FILTER_RE = /^[A-Za-z_$][\w$]*$/; // Match both the prefixed and bare specifiers — bundled dependencies (e.g. diff --git a/packages/cli/src/app.ts b/packages/cli/src/app.ts index 5359ae2de..805819c4f 100644 --- a/packages/cli/src/app.ts +++ b/packages/cli/src/app.ts @@ -93,6 +93,7 @@ import { buildTopLevelFlags } from "./lib/global-flags.js"; import { renderJsonHelp } from "./lib/help.js"; import { isRouteMap, type RouteMap } from "./lib/introspect.js"; import { buildRouteMap } from "./lib/route-map.js"; +import { setRoutes } from "./lib/route-registry.js"; /** * Plural alias → singular route name mapping. @@ -227,6 +228,10 @@ export const routes = buildRouteMap({ }, }); +// Library modules (help, completions, ...) read the command tree from the +// registry instead of importing this module, which would pull in every command. +setRoutes(routes); + /** * Route group names that have `defaultCommand` set. * diff --git a/packages/cli/src/commands/schema.ts b/packages/cli/src/commands/schema.ts index d2593cfde..2826b9d1a 100644 --- a/packages/cli/src/commands/schema.ts +++ b/packages/cli/src/commands/schema.ts @@ -33,6 +33,7 @@ import { CommandOutput } from "../lib/formatters/output.js"; import { fuzzyMatch } from "../lib/fuzzy.js"; import { getCommandApiReferences, type RouteMap } from "../lib/introspect.js"; import { filterByName, isGlobFilter } from "../lib/list-filter.js"; +import { getRoutes } from "../lib/route-registry.js"; // --------------------------------------------------------------------------- // Output data types @@ -543,9 +544,8 @@ export const schemaCommand = buildCommand({ }); } - const { routes } = await import("../app.js"); const references = getCommandApiReferences( - routes as unknown as RouteMap, + getRoutes() as unknown as RouteMap, parseEndpointQuery(query).text, ); diff --git a/packages/cli/src/lib/completions.ts b/packages/cli/src/lib/completions.ts index 7e9e85fee..8e6f49ff9 100644 --- a/packages/cli/src/lib/completions.ts +++ b/packages/cli/src/lib/completions.ts @@ -16,8 +16,8 @@ import { existsSync, mkdirSync } from "node:fs"; import { writeFile } from "node:fs/promises"; import { dirname, join } from "node:path"; -import { routes } from "../app.js"; import { isRouteMap } from "./introspect.js"; +import { getRoutes } from "./route-registry.js"; import type { ShellType } from "./shell.js"; /** Where completions are installed */ @@ -55,7 +55,7 @@ export function extractCommandTree(): CommandTree { const groups: CommandGroup[] = []; const standalone: CommandEntry[] = []; - for (const entry of routes.getAllEntries()) { + for (const entry of getRoutes().getAllEntries()) { if (entry.hidden) { continue; } diff --git a/packages/cli/src/lib/help.ts b/packages/cli/src/lib/help.ts index e8ca36189..c5f8867f4 100644 --- a/packages/cli/src/lib/help.ts +++ b/packages/cli/src/lib/help.ts @@ -6,7 +6,6 @@ * Commands are auto-generated from Stricli's route structure. */ -import { routes } from "../app.js"; import { formatBanner } from "./banner.js"; import { isAuthenticated } from "./db/auth.js"; import { detectAgent } from "./detect-agent.js"; @@ -29,6 +28,7 @@ import { type RouteMapEntry, resolveCommandPath, } from "./introspect.js"; +import { getRoutes } from "./route-registry.js"; import { graphicsBanner, graphicsBannerExtraRows } from "./sixel.js"; const TAGLINE = "The command-line interface for Sentry"; @@ -112,7 +112,7 @@ const MIN_OVERVIEW_COMMANDS = 3; /** Visible top-level route entries, in registration order. */ function visibleRouteEntries(): RouteMapEntry[] { // Cast to our introspection types — Stricli's generic types are compatible - const routeMap = routes as unknown as RouteMap; + const routeMap = getRoutes() as unknown as RouteMap; return routeMap .getAllEntries() .filter((entry: RouteMapEntry) => !entry.hidden); @@ -460,7 +460,7 @@ export function introspectAllCommands(): { envVars: HelpEnvVarInfo[]; flags: HelpFlagInfo[]; } { - const routeMap = routes as unknown as RouteMap; + const routeMap = getRoutes() as unknown as RouteMap; return { routes: extractAllRoutes(routeMap), envVars: buildTopLevelEnvVars(), @@ -476,7 +476,7 @@ export function introspectAllCommands(): { export function introspectCommand( commandPath: string[], ): CommandInfo | RouteInfo | { error: string; suggestions?: string[] } { - const routeMap = routes as unknown as RouteMap; + const routeMap = getRoutes() as unknown as RouteMap; const resolved = resolveCommandPath(routeMap, commandPath); if (!resolved) { return { error: `Command not found: ${commandPath.join(" ")}` }; diff --git a/packages/cli/src/lib/list-command.ts b/packages/cli/src/lib/list-command.ts index 388b6bc01..0e3550e64 100644 --- a/packages/cli/src/lib/list-command.ts +++ b/packages/cli/src/lib/list-command.ts @@ -13,12 +13,8 @@ * buildOrgListCommand */ -import { createRequire } from "node:module"; import type { Aliases, Command, CommandContext } from "@stricli/core"; import type { SentryContext } from "../context.js"; - -const _require = createRequire(import.meta.url); - import type { ListTargetOptions } from "./arg-parsing.js"; import { buildCommand, @@ -27,10 +23,6 @@ import { } from "./command.js"; import { ORG_PROJECT_TARGET, ORG_TARGET } from "./completion-target.js"; import { disableOrgCache } from "./db/regions.js"; -import { logger } from "./logger.js"; - -const log = logger.withTag("list-command"); - import { disableDsnCache } from "./dsn/index.js"; import { warning } from "./formatters/colors.js"; import { @@ -51,6 +43,7 @@ import { skippedArgument, } from "./positional-parse.js"; import { disableResponseCache } from "./response-cache.js"; +import { type CliRoutes, findRoutes } from "./route-registry.js"; import { type ListCommandTarget, listTarget } from "./target-positional.js"; import { PERIOD_BRIEF, parsePeriod } from "./time-range.js"; import { shellQuote } from "./utils.js"; @@ -445,7 +438,8 @@ export const LIST_BASE_ALIASES: Aliases = { n: "limit", c: "cursor" }; // Level B: subcommand interception for plural aliases // --------------------------------------------------------------------------- -let _subcommandsByRoute: Map> | undefined; +/** Subcommand names by route group, per registered route map. */ +const subcommandsByRoutes = new WeakMap>>(); /** * Entry shape returned by Stricli's getAllEntries(). @@ -475,39 +469,32 @@ function collectChildNames(parent: { /** * Get the subcommand names for a given singular route (e.g. "project" → {"list", "view"}). * - * Lazily walks the Stricli route map on first call. Uses `require()` to break - * the circular dependency: list-command → app → commands → list-command. + * Walks the registered route map once and caches the result for it. With no + * route map registered (a command's `func` called directly, as in unit + * tests), there are no subcommands to intercept. */ function getSubcommandsForRoute(routeName: string): Set { - if (!_subcommandsByRoute) { - _subcommandsByRoute = new Map(); - - try { - const { routes } = _require("../app.js") as { - routes: { getAllEntries: () => readonly RouteEntry[] }; - }; - - for (const entry of routes.getAllEntries()) { - const target = entry.target as unknown as Record; - if (typeof target?.getAllEntries === "function") { - _subcommandsByRoute.set( - entry.name.original, - collectChildNames( - target as { getAllEntries: () => readonly RouteEntry[] }, - ), - ); - } + const routes = findRoutes(); + if (!routes) { + return new Set(); + } + let byRoute = subcommandsByRoutes.get(routes); + if (!byRoute) { + byRoute = new Map(); + for (const entry of routes.getAllEntries()) { + const target = entry.target as unknown as Record; + if (typeof target?.getAllEntries === "function") { + byRoute.set( + entry.name.original, + collectChildNames( + target as { getAllEntries: () => readonly RouteEntry[] }, + ), + ); } - } catch (error) { - // In test environments (vitest), require("../app.js") may fail because - // Node's ESM resolver can't resolve .js→.ts for transitive imports. - // Gracefully degrade: interceptSubcommand will treat all targets as - // plain values (no subcommand interception). - log.debug("Failed to load app routes for subcommand detection", error); } + subcommandsByRoutes.set(routes, byRoute); } - - return _subcommandsByRoute.get(routeName) ?? new Set(); + return byRoute.get(routeName) ?? new Set(); } /** diff --git a/packages/cli/src/lib/route-registry.ts b/packages/cli/src/lib/route-registry.ts new file mode 100644 index 000000000..8033c5d82 --- /dev/null +++ b/packages/cli/src/lib/route-registry.ts @@ -0,0 +1,42 @@ +/** + * The route map the running CLI was built from. + * + * Library modules (help, completions, list-command) need the command tree, + * but importing `app.ts` for it would pull every command into any bundle + * that reaches those modules — and every command reaches them through + * `buildCommand`. Instead, whatever builds the application registers its + * route map here: `app.ts` for the full CLI, or another entry point with a + * smaller map. Tests can register a fake one. + */ + +import type { RouteMap } from "@stricli/core"; +import type { SentryContext } from "../context.js"; + +/** Top-level route map of the CLI application. */ +export type CliRoutes = RouteMap; + +let registeredRoutes: CliRoutes | undefined; + +/** Register the route map the CLI application runs. Replaces any earlier one. */ +export function setRoutes(routes: CliRoutes): void { + registeredRoutes = routes; +} + +/** The registered route map, or `undefined` before registration. */ +export function findRoutes(): CliRoutes | undefined { + return registeredRoutes; +} + +/** + * The registered route map. + * + * @throws {Error} When no entry point has called {@link setRoutes} yet + */ +export function getRoutes(): CliRoutes { + if (!registeredRoutes) { + throw new Error( + "CLI routes are not registered. The entry point that builds the application must call setRoutes() first (src/app.ts does).", + ); + } + return registeredRoutes; +} diff --git a/packages/cli/test/commands/schema-path.test.ts b/packages/cli/test/commands/schema-path.test.ts index 965d235e1..b73e56b54 100644 --- a/packages/cli/test/commands/schema-path.test.ts +++ b/packages/cli/test/commands/schema-path.test.ts @@ -3,6 +3,8 @@ */ import { describe, expect, test, vi } from "vitest"; +// Registers the CLI route map that command API references are read from. +import "../../src/app.js"; import { schemaCommand } from "../../src/commands/schema.js"; import type { SentryContext } from "../../src/context.js"; import { getAllEndpoints } from "../../src/lib/api-schema.js"; diff --git a/packages/cli/test/lib/completions.test.ts b/packages/cli/test/lib/completions.test.ts index b41c3dd1a..d02b1377c 100644 --- a/packages/cli/test/lib/completions.test.ts +++ b/packages/cli/test/lib/completions.test.ts @@ -10,6 +10,8 @@ import { chmodSync, existsSync, mkdirSync, rmSync } from "node:fs"; import { readFile } from "node:fs/promises"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, test } from "vitest"; +// Registers the CLI route map the completion scripts are generated from. +import "../../src/app.js"; import { extractCommandTree, getCompletionPath, diff --git a/packages/cli/test/lib/help-json.test.ts b/packages/cli/test/lib/help-json.test.ts index 4c6d06ee7..09e70dc36 100644 --- a/packages/cli/test/lib/help-json.test.ts +++ b/packages/cli/test/lib/help-json.test.ts @@ -9,6 +9,8 @@ */ import { describe, expect, test } from "vitest"; +// Registers the CLI route map the help output is built from. +import "../../src/app.js"; import { isRecoverableUnknownCommand, isVersionRequest, diff --git a/packages/cli/test/lib/help.test.ts b/packages/cli/test/lib/help.test.ts index 2069b402e..d2b8e6907 100644 --- a/packages/cli/test/lib/help.test.ts +++ b/packages/cli/test/lib/help.test.ts @@ -6,6 +6,8 @@ */ import { describe, expect, test } from "vitest"; +// Registers the CLI route map the help output is built from. +import "../../src/app.js"; import { bannerLinesForWidth, formatBanner } from "../../src/lib/banner.js"; import { withEnv } from "../../src/lib/env.js"; import { introspectAllCommands, printCustomHelp } from "../../src/lib/help.js"; diff --git a/packages/cli/test/lib/route-registry.test.ts b/packages/cli/test/lib/route-registry.test.ts new file mode 100644 index 000000000..41d4a1ad7 --- /dev/null +++ b/packages/cli/test/lib/route-registry.test.ts @@ -0,0 +1,150 @@ +/** + * The route registry: library modules read the command tree from whichever + * entry point built the application, never from `app.ts` itself. + */ + +import { readdir, readFile } from "node:fs/promises"; +import { join, relative, resolve } from "node:path"; +import { build } from "esbuild"; +import { describe, expect, test, vi } from "vitest"; +import { buildCommand } from "../../src/lib/command.js"; +import { extractCommandTree } from "../../src/lib/completions.js"; +import { introspectAllCommands } from "../../src/lib/help.js"; +import { interceptSubcommand } from "../../src/lib/list-command.js"; +import { buildRouteMap } from "../../src/lib/route-map.js"; +import { setRoutes } from "../../src/lib/route-registry.js"; + +const SRC = join(import.meta.dirname, "../../src"); +const APP = join(SRC, "app.ts"); +/** Library-mode runner: executes the full application, reached only from `src/index.ts`. */ +const SDK_RUNNER = join(SRC, "lib/sdk-invoke.ts"); + +const ANY_IMPORT = /.*/; +const RELATIVE_IMPORT = /^\.\.?\//; +const JS_EXTENSION = /\.js$/; +const TS_SOURCE = /\.tsx?$/; +const LAZY_REQUIRE = /\b_require\(/g; + +function command(brief: string) { + return buildCommand({ + docs: { brief }, + auth: false, + async *func() { + yield; + }, + }); +} + +/** A smaller CLI, like the one the MCP server build registers. */ +function mcpServerRoutes() { + return buildRouteMap({ + routes: { + mcp: command("Run the MCP server"), + auth: buildRouteMap({ + routes: { login: command("Log in"), whoami: command("Who am I") }, + docs: { brief: "Authenticate" }, + }), + help: command("Help"), + }, + defaultCommand: "mcp", + docs: { brief: "Sentry MCP server" }, + }); +} + +describe("route registry", () => { + test("reading routes before an entry point registers them fails clearly", async () => { + vi.resetModules(); + const { getRoutes } = await import("../../src/lib/route-registry.js"); + expect(() => getRoutes()).toThrow("CLI routes are not registered"); + }); + + test("help, completions, and subcommand hints read the registered map", () => { + setRoutes(mcpServerRoutes()); + + expect(extractCommandTree()).toEqual({ + groups: [ + { + name: "auth", + brief: "Authenticate", + subcommands: [ + { name: "login", brief: "Log in" }, + { name: "whoami", brief: "Who am I" }, + ], + }, + ], + standalone: [ + { name: "mcp", brief: "Run the MCP server" }, + { name: "help", brief: "Help" }, + ], + }); + expect(introspectAllCommands().routes.map((route) => route.name)).toEqual([ + "mcp", + "auth", + "help", + ]); + + let hint = ""; + const stderr = { write: (s: string) => (hint += s) }; + expect(interceptSubcommand("login", stderr, "auth")).toBeUndefined(); + expect(hint).toContain("sentry auth login"); + }); + + test("library modules never import app.ts", async () => { + const files = await readdir(join(SRC, "lib"), { recursive: true }); + const entryPoints = files + .filter((file) => TS_SOURCE.test(file)) + .map((file) => join(SRC, "lib", file)) + .filter((file) => file !== SDK_RUNNER); + + await expect( + build({ + entryPoints, + bundle: true, + write: false, + outdir: "out", + splitting: true, + platform: "node", + format: "esm", + logLevel: "silent", + plugins: [ + { + name: "check-app-imports", + setup(builder) { + builder.onResolve({ filter: ANY_IMPORT }, (args) => { + if (args.kind === "entry-point") { + return; + } + // Only our own modules matter; `with { type }` imports are assets. + if (!RELATIVE_IMPORT.test(args.path) || args.with.type) { + return { path: args.path, external: true }; + } + const target = resolve(args.resolveDir, args.path).replace( + JS_EXTENSION, + ".ts", + ); + if (target === APP) { + return { + errors: [ + { + text: `${relative(SRC, args.importer)} imports app.ts; read routes from lib/route-registry.ts instead`, + }, + ], + }; + } + return; + }); + // Follow lazy `_require()` loads too, as the bundle scripts do. + builder.onLoad({ filter: TS_SOURCE }, async (args) => ({ + contents: (await readFile(args.path, "utf-8")).replace( + LAZY_REQUIRE, + "require(", + ), + loader: args.path.endsWith(".tsx") ? "tsx" : "ts", + })); + }, + }, + ], + }), + ).resolves.toMatchObject({ errors: [] }); + }); +}); diff --git a/packages/cli/test/lib/route-subcommands.test.ts b/packages/cli/test/lib/route-subcommands.test.ts index 65f5c2005..e7e42c0d6 100644 --- a/packages/cli/test/lib/route-subcommands.test.ts +++ b/packages/cli/test/lib/route-subcommands.test.ts @@ -3,6 +3,8 @@ */ import { describe, expect, test } from "vitest"; +// Registers the CLI route map that subcommand names are read from. +import "../../src/app.js"; import { interceptSubcommand } from "../../src/lib/list-command.js"; function makeStderr(): { write(s: string): void; output: string } { @@ -18,12 +20,7 @@ function makeStderr(): { write(s: string): void; output: string } { } describe("interceptSubcommand", () => { - // Skip: interceptSubcommand relies on require("../app.js") to load the - // Stricli route map, which fails under vitest because Node's CJS require - // can't resolve .js→.ts for transitive ESM imports. The try-catch in - // getSubcommandsForRoute gracefully degrades to empty sets in test. - // oxlint-disable-next-line sentry-cli/no-skipped-tests -- require("../app.js") fails in vitest — CJS can't resolve .js→.ts - test.skip("returns undefined and writes hint for known subcommand", () => { + test("returns undefined and writes hint for known subcommand", () => { const stderr = makeStderr(); const result = interceptSubcommand("list", stderr, "project"); expect(result).toBeUndefined(); @@ -54,17 +51,13 @@ describe("interceptSubcommand", () => { expect(stderr.output).toBe(""); }); - // Skip: same reason as above — require("../app.js") fails in vitest - // oxlint-disable-next-line sentry-cli/no-skipped-tests -- require("../app.js") fails in vitest — CJS can't resolve .js→.ts - test.skip("hint includes the route name and subcommand", () => { + test("hint includes the route name and subcommand", () => { const stderr = makeStderr(); interceptSubcommand("view", stderr, "issue"); expect(stderr.output).toContain("sentry issue view"); }); - // Skip: same reason as above — require("../app.js") fails in vitest - // oxlint-disable-next-line sentry-cli/no-skipped-tests -- require("../app.js") fails in vitest — CJS can't resolve .js→.ts - test.skip("handles 'explain' and 'plan' subcommands for issue route", () => { + test("handles 'explain' and 'plan' subcommands for issue route", () => { const stderr1 = makeStderr(); expect(interceptSubcommand("explain", stderr1, "issue")).toBeUndefined(); expect(stderr1.output).toContain("sentry issue explain");