From 57aacfd9199cf829a4f9dfb95af233070268f566 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 9 Apr 2026 18:43:45 +0000 Subject: [PATCH] feat(cli): hoist global flags from any argv position and add -v alias Global flags (--verbose, --json, --log-level, --fields) only worked when placed after the leaf command because Stricli parses flags at the leaf level only. This adds an argv preprocessor that relocates these flags from any position to the end of argv before Stricli processes it, allowing patterns like `sentry --verbose issue list` and `sentry cli -v upgrade`. Also injects -v as a short alias for --verbose on all leaf commands, following the existing buildDeleteCommand alias injection pattern. --- AGENTS.md | 120 ++++------- src/cli.ts | 16 +- src/lib/argv-hoist.ts | 189 +++++++++++++++++ src/lib/command.ts | 21 +- src/lib/global-flags.ts | 43 ++++ test/lib/argv-hoist.property.test.ts | 171 ++++++++++++++++ test/lib/argv-hoist.test.ts | 290 +++++++++++++++++++++++++++ 7 files changed, 766 insertions(+), 84 deletions(-) create mode 100644 src/lib/argv-hoist.ts create mode 100644 src/lib/global-flags.ts create mode 100644 test/lib/argv-hoist.property.test.ts create mode 100644 test/lib/argv-hoist.test.ts diff --git a/AGENTS.md b/AGENTS.md index 4f40f7f87..8d2560227 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -984,108 +984,66 @@ mock.module("./some-module", () => ({ ### Architecture - -* **commandPrefix on SentryContext enables command identity in buildCommand wrapper**: \`SentryContext.commandPrefix\` (optional \`readonly string\[]\`) is populated in \`forCommand()\` in \`context.ts\` — Stricli calls this with the full prefix (e.g., \`\["sentry", "issue", "list"]\`) before running the command. This enables the \`buildCommand\` wrapper to identify which command is executing for help recovery and telemetry. Previously, \`forCommand\` only set telemetry span names. + +* **Auth token env var override pattern: SENTRY\_AUTH\_TOKEN > SENTRY\_TOKEN > SQLite**: Auth in \`src/lib/db/auth.ts\` follows layered precedence: \`SENTRY\_AUTH\_TOKEN\` > \`SENTRY\_TOKEN\` > SQLite OAuth token. \`getEnvToken()\` trims env vars (empty/whitespace = unset). \`AuthSource\` tracks provenance. \`ENV\_SOURCE\_PREFIX = "env:"\` — use \`.length\` not hardcoded 4. Env tokens bypass refresh/expiry. \`isEnvTokenActive()\` guards auth commands. Logout must NOT clear stored auth when env token active. These functions stay in \`db/auth.ts\` despite not touching DB because they're tightly coupled with token retrieval. - -* **Dashboard widget interval computed from terminal width and layout before API calls**: Dashboard chart widgets compute optimal \`interval\` before making API calls using terminal width and widget layout. Formula: \`colWidth = floor(layout.w / 6 \* termWidth)\`, \`chartWidth = colWidth - 4 - gutterW\` (~5-7), \`idealSeconds = periodSeconds / chartWidth\`. Snaps to nearest Sentry interval bucket (\`1m\`, \`5m\`, \`15m\`, \`30m\`, \`1h\`, \`4h\`, \`1d\`). Lives in \`computeOptimalInterval()\` in \`src/lib/api/dashboards.ts\`. \`periodToSeconds()\` parses \`"24h"\`, \`"7d"\` etc. The \`PERIOD\_RE\` regex is hoisted to module scope (Biome requires top-level regex). \`WidgetQueryParams\` gains optional \`interval?: string\` field; \`queryWidgetTimeseries\` uses \`params.interval ?? widget.interval\` for the API call. \`queryAllWidgets\` computes per-widget intervals using \`getTermWidth()\` logic (min 80, fallback 100). + +* **Consola chosen as CLI logger with Sentry createConsolaReporter integration**: Consola is the CLI logger with Sentry \`createConsolaReporter\` integration. Two reporters: FancyReporter (stderr) + Sentry structured logs. Level via \`SENTRY\_LOG\_LEVEL\`. \`buildCommand\` injects hidden \`--log-level\`/\`--verbose\` flags. \`withTag()\` creates independent instances; \`setLogLevel()\` propagates via registry. All user-facing output must use consola, not raw stderr. \`HandlerContext\` intentionally omits stderr. - -* **defaultCommand:help blocks Stricli fuzzy matching for top-level typos**: Fuzzy matching across CLI subsystems: (1) Stricli built-in Damerau-Levenshtein for subcommand/flag typos within known routes. (2) \`defaultCommand: "help"\` bypasses this for top-level typos — fixed by \`resolveCommandPath()\` in \`introspect.ts\` returning \`UnresolvedPath\` with suggestions via \`fuzzyMatch()\` from \`fuzzy.ts\` (up to 3). Covers \`sentry \\` and \`sentry help \\`. (3) \`fuzzyMatch()\` in \`complete.ts\` for tab-completion (Levenshtein+prefix+contains). (4) \`levenshtein()\` in \`platforms.ts\` for platform suggestions. (5) Plural alias detection in \`app.ts\`. JSON includes \`suggestions\` array. + +* **Input validation layer: src/lib/input-validation.ts guards CLI arg parsing**: Four validators in \`src/lib/input-validation.ts\` guard against agent-hallucinated inputs: \`rejectControlChars\` (ASCII < 0x20), \`rejectPreEncoded\` (%XX), \`validateResourceId\` (rejects ?, #, %, whitespace), \`validateEndpoint\` (rejects \`..\` traversal). Applied in \`parseSlashOrgProject\`, bare-slug path in \`parseOrgProjectArg\`, \`parseIssueArg\`, and \`normalizeEndpoint\` (api.ts). NOT applied in \`parseSlashSeparatedArg\` for no-slash plain IDs — those may contain structural separators (newlines for log view batch IDs) that callers split downstream. Validation targets user-facing parse boundaries only; env vars and DB cache values are trusted. - -* **DSN org prefix normalization in arg-parsing.ts**: DSN org ID normalization has four code paths: (1) \`extractOrgIdFromHost\` in \`dsn/parser.ts\` strips \`o\` prefix during DSN parsing → bare \`"1081365"\`. (2) \`stripDsnOrgPrefix()\` strips \`o\` from user-typed inputs like \`o1081365/\`, applied in \`parseOrgProjectArg()\` and \`resolveEffectiveOrg()\`. (3) \`normalizeNumericOrg()\` in \`resolve-target.ts\` handles bare numeric IDs from cold-cache DSN detection — checks \`getOrgByNumericId()\` from DB cache, falls back to \`listOrganizationsUncached()\` to populate the mapping. Called from \`resolveOrg()\` step 4 (DSN auto-detect path). (4) Dashboard's \`resolveOrgFromTarget()\` pipes explicit org through \`resolveEffectiveOrg()\` for \`o\`-prefixed forms. Critical: many API endpoints reject numeric org IDs with 404/403 — always normalize to slugs before API calls. + +* **Magic @ selectors resolve issues dynamically via sort-based list API queries**: Magic \`@\` selectors (\`@latest\`, \`@most\_frequent\`) in \`parseIssueArg\` are detected early (before \`validateResourceId\`) because \`@\` is not in the forbidden charset. \`SELECTOR\_MAP\` provides case-insensitive matching with common variations (\`@mostfrequent\`, \`@most-frequent\`). Resolution in \`resolveSelector\` (issue/utils.ts) maps selectors to \`IssueSort\` values (\`date\`, \`freq\`), calls \`listIssuesPaginated\` with \`perPage: 1\` and \`query: 'is:unresolved'\`. Supports org-prefixed form: \`sentry/@latest\`. Unrecognized \`@\`-prefixed strings fall through to suffix-only parsing (not an error). The \`ParsedIssueArg\` union includes \`{ type: 'selector'; selector: IssueSelector; org?: string }\`. - -* **GHCR versioned nightly tags for delta upgrade support**: GHCR nightly distribution uses three tag types: \`:nightly\` (rolling), \`:nightly-\\` (immutable), \`:patch-\\` (delta manifest). Delta patches use zig-bsdiff TRDIFF10 (zstd-compressed), ~50KB vs ~29MB full. Client bspatch via \`Bun.zstdDecompressSync()\`. N-1 patches only, full download fallback, SHA-256 verify, 60% size threshold. npm/Node excluded. Test mocks: use \`mockGhcrNightlyVersion()\` helper. + +* **Sentry SDK uses @sentry/node-core/light instead of @sentry/bun to avoid OTel overhead**: The CLI uses \`@sentry/node-core/light\` instead of \`@sentry/bun\` to avoid loading the full OpenTelemetry stack (~150ms, 24MB). \`@sentry/core\` barrel is patched via \`bun patch\` to remove ~32 unused exports saving ~13ms. Key gotcha: \`LightNodeClient\` constructor hardcodes \`runtime: { name: 'node' }\` AFTER spreading user options, so passing \`runtime\` in \`Sentry.init()\` is silently overwritten. Fix: patch \`client.getOptions().runtime\` post-init (returns mutable ref). The CLI does this in \`telemetry.ts\` to report \`bun\` runtime when running as binary. Trade-offs: transport falls back to Node's \`http\` module instead of native \`fetch\`. Upstream issues: getsentry/sentry-javascript#19885 and #19886. - -* **Issue list auto-pagination beyond API's 100-item cap**: Sentry API silently caps \`limit\` at 100 per request. \`listIssuesAllPages()\` auto-paginates using Link headers, bounded by MAX\_PAGINATION\_PAGES (50). \`API\_MAX\_PER\_PAGE\` constant is shared across all paginated consumers. \`--limit\` means total results everywhere (max 1000, default 25). Org-all mode uses \`fetchOrgAllIssues()\`; explicit \`--cursor\` does single-page fetch to preserve cursor chain. + +* **Zod schema on OutputConfig enables self-documenting JSON fields in help and SKILL.md**: List commands register a \`schema?: ZodType\` on \`OutputConfig\\` (in \`src/lib/formatters/output.ts\`). \`extractSchemaFields()\` walks Zod object shapes to produce \`SchemaFieldInfo\[]\` (name, type, description, optional). In \`command.ts\`, \`buildFieldsFlag()\` enriches the \`--fields\` flag brief with available names; \`enrichDocsWithSchema()\` appends a fields section to \`fullDescription\`. The schema is exposed as \`\_\_jsonSchema\` on the built command for introspection — \`introspect.ts\` reads it into \`CommandInfo.jsonFields\`. \`help.ts\` renders fields in \`sentry help \\` output. \`generate-skill.ts\` renders a markdown table in reference docs. For \`buildOrgListCommand\`/\`dispatchOrgScopedList\`, pass \`schema\` via \`OrgListConfig\` — \`list-command.ts\` forwards it to \`OutputConfig\`. - -* **resolveProjectBySlug carries full projectData to avoid redundant getProject calls**: \`resolveProjectBySlug()\` returns \`{ org, project, projectData: SentryProject }\` — the full project object from \`findProjectsBySlug()\`. \`ResolvedOrgProject\` and \`ResolvedTarget\` have optional \`projectData?\` (populated only in project-search path, not explicit/auto-detect). Downstream commands (\`project/view\`, \`project/delete\`, \`dashboard/create\`) use \`projectData\` when available to skip redundant \`getProject()\` API calls (~500-800ms savings). Pattern: \`resolved.projectData ?? await getProject(org, project)\` for callers that need both paths. +### Decision - -* **Self-hosted OAuth device flow requires Sentry 26.1.0+ and SENTRY\_CLIENT\_ID**: Self-hosted OAuth device flow requires Sentry 26.1.0+ and both \`SENTRY\_URL\` and \`SENTRY\_CLIENT\_ID\` env vars. Users must create a public OAuth app in Settings → Developer Settings. The client ID is NOT optional for self-hosted. Fallback for older instances: \`sentry auth login --token\`. \`getSentryUrl()\` and \`getClientId()\` in \`src/lib/oauth.ts\` read lazily (not at module load) so URL parsing from arguments can set \`SENTRY\_URL\` after import. + +* **All view subcommands should use \ \ positional pattern**: All \`\* view\` subcommands should follow a consistent \`\ \\` positional argument pattern where target is the optional \`org/project\` specifier. During migration, use opportunistic argument swapping with a stderr warning when args are in wrong order. This is an instance of the broader CLI UX auto-correction pattern: safe when input is already invalid, correction is unambiguous, warning goes to stderr. Normalize at command level, keep parsers pure. Model after \`gh\` CLI conventions. - -* **Sentry CLI markdown-first formatting pipeline replaces ad-hoc ANSI**: Formatters build CommonMark strings; \`renderMarkdown()\` renders to ANSI for TTY or raw markdown for non-TTY. Key helpers: \`colorTag()\`, \`mdKvTable()\`, \`mdRow()\`, \`mdTableHeader()\` (\`:\` suffix = right-aligned), \`renderTextTable()\`. \`isPlainOutput()\` checks \`SENTRY\_PLAIN\_OUTPUT\` > \`NO\_COLOR\` > \`!isTTY\`. Batch path: \`formatXxxTable()\`. Streaming path: \`StreamingTable\` (TTY) or raw markdown rows (plain). Both share \`buildXxxRowCells()\`. - - -* **Sentry dashboard API rejects discover/transaction-like widget types — use spans**: The Sentry Dashboard API rejects \`widgetType: 'discover'\` and \`widgetType: 'transaction-like'\` as deprecated. Use \`widgetType: 'spans'\` for new widgets. The codebase splits types into \`WIDGET\_TYPES\` (active, for creation) and \`ALL\_WIDGET\_TYPES\` (including deprecated, for parsing server responses). \`DashboardWidgetInputSchema\` must use \`ALL\_WIDGET\_TYPES\` so editing existing widgets with deprecated types passes Zod validation. \`validateWidgetEnums()\` in \`resolve.ts\` rejects deprecated types for new widget creation — but accepts \`skipDeprecatedCheck: true\` for the edit path, where \`effectiveDataset\` may inherit a deprecated type from the existing widget. Cross-validation (display vs dataset compatibility) still runs on effective values. Tests must use \`error-events\` instead of \`discover\`; it shares \`DISCOVER\_AGGREGATE\_FUNCTIONS\` including \`failure\_rate\`. - - -* **Sentry issue stats field: time-series controlled by groupStatsPeriod**: Sentry issue stats and list table layout: \`stats\` key depends on \`groupStatsPeriod\` (\`""\`, \`"14d"\`, \`"24h"\`, \`"auto"\`); \`statsPeriod\` controls window. \*\*Critical\*\*: \`count\` is period-scoped — use \`lifetime.count\` for true total. Issue list uses \`groupStatsPeriod: 'auto'\` for sparklines. Columns: SHORT ID, ISSUE, SEEN, AGE, TREND, EVENTS, USERS, TRIAGE. TREND hidden < 100 cols. \`--compact\` tri-state: explicit overrides; \`undefined\` triggers \`shouldAutoCompact(rowCount)\` — compact if \`3N + 3 > termHeight\`. Height formula \`3N + 3\` (last row has no trailing separator). - - -* **Sentry trace-logs API is org-scoped, not project-scoped**: The Sentry trace-logs endpoint (\`/organizations/{org}/trace-logs/\`) is org-scoped, so \`trace logs\` uses \`resolveOrg()\` not \`resolveOrgAndProject()\`. The endpoint is PRIVATE in Sentry source, excluded from the public OpenAPI schema — \`@sentry/api\` has no generated types. The hand-written \`TraceLogSchema\` in \`src/types/sentry.ts\` is required until Sentry makes it public. - - -* **SKILL.md is fully generated — edit fragment files for custom content, not output**: The skill files under \`plugins/sentry-cli/skills/sentry-cli/\` (SKILL.md + references/\*.md) are fully generated by \`bun run generate:skill\` (script/generate-skill.ts). They are rebuilt automatically by \`bun run generate:docs\` which runs as part of \`dev\`, \`build\`, \`typecheck\`, and \`test\` scripts. To change skill content, edit the \*\*sources\*\*: (1) \`docs/src/content/docs/agent-guidance.md\` — embedded into SKILL.md's Agent Guidance section with heading levels bumped. (2) \`src/commands/\*/\` flag \`brief\` strings — generate the reference file flag descriptions. (3) \`docs/src/fragments/commands/\*.md\` — hand-written examples and guides appended to generated command docs. Command docs (\`docs/src/content/docs/commands/\*.md\`) are also gitignored and rebuilt from fragments + CLI metadata by \`generate:command-docs\`. \`bun run check:fragments\` validates fragment ↔ route consistency. - - -* **Stricli route errors are uninterceptable — only post-run detection works**: Stricli route errors, exit codes, and OutputError — error propagation gaps: (1) Route failures are uninterceptable — Stricli writes to stderr and returns \`ExitCode.UnknownCommand\` internally. Only post-\`run()\` \`process.exitCode\` check works. \`exceptionWhileRunningCommand\` only fires for errors in command \`func()\`. (2) \`ExitCode.UnknownCommand\` is \`-5\`. Bun reads \`251\` (unsigned byte), Node reads \`-5\` — compare both. (3) \`OutputError\` in \`handleOutputError\` calls \`process.exit()\` immediately, bypassing telemetry and \`exceptionWhileRunningCommand\`. Top-level typos via \`defaultCommand:help\` → \`OutputError\` → \`process.exit(1)\` skip all error reporting. - - -* **withAuthGuard returns discriminated Result type, not fallback+onError**: \`withAuthGuard\(fn)\` in \`src/lib/errors.ts\` returns a discriminated Result: \`{ ok: true, value: T } | { ok: false, error: unknown }\`. AuthErrors always re-throw (triggers bin.ts auto-login). All other errors are captured. Callers inspect \`result.ok\` to degrade gracefully. Used across 12+ files. + +* **Sentry-derived terminal color palette tuned for dual-background contrast**: The CLI's chart/dashboard palette uses 10 colors derived from Sentry's categorical chart hues (\`static/app/utils/theme/scraps/tokens/color.tsx\` in getsentry/sentry), each adjusted to mid-luminance to achieve ≥3:1 contrast on both dark (#1e1e1e) and light (#f0f0f0) backgrounds. Key adjustments: orange darkened from #FF9838→#C06F20, green #67C800→#3D8F09, yellow #FFD00E→#9E8B18, purple lightened #5D3EB2→#8B6AC8, indigo #50219C→#7B50D0. Blurple (#7553FF), pink (#F0369A), magenta (#B82D90) used as-is. Teal (#228A83) added to fill a hue gap. ANSI 16-color codes were considered but rejected in favor of hex since the mid-luminance hex values provide guaranteed contrast regardless of terminal theme configuration. ### Gotcha - -* **Biome lint: Response.redirect() required, nested ternaries forbidden**: Biome lint rules that frequently trip up this codebase: (1) \`useResponseRedirect\`: use \`Response.redirect(url, status)\` not \`new Response\`. (2) \`noNestedTernary\`: use \`if/else\`. (3) \`noComputedPropertyAccess\`: use \`obj.property\` not \`obj\["property"]\`. (4) Max cognitive complexity 15 per function — extract helpers to stay under. + +* **Biome lint bans process.stdout in commands — use isPlainOutput() and yield tokens instead**: A custom lint rule prevents \`process.stdout\` usage in command files — all output must go through \`yield CommandOutput\` or the Stricli context's \`this.stdout\`. For TTY detection, use \`isPlainOutput()\` from \`src/lib/formatters/plain-detect.ts\` instead of \`process.stdout.isTTY\`. For ANSI control sequences (screen clear, cursor movement), yield a \`ClearScreen\` token and let the \`buildCommand\` wrapper handle it. This keeps commands decoupled from stdout details. - -* **Bugbot flags defensive null-checks as dead code — keep them with JSDoc justification**: Cursor Bugbot and Sentry Seer repeatedly flag two false positives: (1) defensive null-checks as "dead code" — keep them with JSDoc explaining why the guard exists for future safety, especially when removing would require \`!\` assertions banned by \`noNonNullAssertion\`. (2) stderr spinner output during \`--json\` mode — always a false positive since progress goes to stderr, JSON to stdout. Reply explaining the rationale and resolve. + +* **Dashboard tracemetrics dataset uses comma-separated aggregate format**: SDK v10+ custom metrics (, , ) emit envelope items. Dashboard widgets for these MUST use with aggregate format — e.g., . The parameter must match the SDK emission exactly: if no unit specified, for memory metrics, for uptime. only supports , , , , display types — no or . Widgets with always require . Sort expressions must reference aggregates present in . - -* **Bun mock.module for node:tty requires default export and class stubs**: Bun testing gotchas: (1) \`mock.module()\` for CJS built-ins requires a \`default\` re-export plus all named exports. Missing any causes \`SyntaxError: Export named 'X' not found\`. Always check the real module's full export list. (2) \`Bun.mmap()\` always opens with PROT\_WRITE — macOS SIGKILL on signed Mach-O, Linux ETXTBSY. Fix: use \`new Uint8Array(await Bun.file(path).arrayBuffer())\` in bspatch.ts. (3) Wrap \`Bun.which()\` with optional \`pathEnv\` param for deterministic testing without mocks. + +* **Dot-notation field filtering is ambiguous for keys containing dots**: The \`filterFields\` function in \`src/lib/formatters/json.ts\` uses dot-notation to address nested fields (e.g., \`metadata.value\`). This means object keys that literally contain dots are ambiguous and cannot be addressed. Property-based tests for this function must generate field name arbitraries that exclude dots — use a restricted charset like \`\[a-zA-Z0-9\_]\` in fast-check arbitraries. Counterexample found by fast-check: \`{"a":{".":false}}\` with path \`"a."\` splits into \`\["a", ""]\` and fails to resolve. - -* **CLI telemetry command tags use sentry. prefix with dots not bare names**: The \`buildCommand\` wrapper sets the \`command\` telemetry tag using the full Stricli command prefix joined with dots: \`sentry.issue.explain\`, \`sentry.issue.list\`, \`sentry.api\`, etc. — NOT bare names like \`issue.explain\`. When querying Sentry Discover or building dashboard widgets, always use the \`sentry.\` prefix. Verify actual tag values with a Discover query (\`field:command, count()\`, grouped by \`command\`) before assuming the format. + +* **Git worktree blocks branch checkout of branches used in other worktrees**: \`git checkout main\` fails with "already used by worktree at ..." when another worktree has that branch checked out. In this repo's worktree setup, use \`git checkout origin/main --detach\` or create feature branches from \`origin/main\` directly: \`git checkout -b fix/foo origin/main\`. This is a standard git worktree constraint but catches people off guard in the CLI repo which uses worktrees for parallel development. -* **Dashboard tracemetrics dataset uses comma-separated aggregate format**: SDK v10+ custom metrics (`Sentry.metrics.distribution()`, `.gauge()`, `.count()`) emit `trace_metric` envelope items. Dashboard widgets for these MUST use `--dataset tracemetrics` with aggregate format `aggregation(value,metric_name,metric_type,unit)` — e.g., `p50(value,completion.duration_ms,distribution,none)`. The `unit` parameter must match the SDK emission exactly: `none` if no unit specified, `byte` for memory metrics, `second` for uptime. `tracemetrics` only supports `line`, `area`, `bar`, `big_number`, `categorical_bar` display types — no `table` or `stacked_area`. Widgets with `--group-by` always require `--limit`. Sort expressions must reference aggregates present in `--query`. + +* **Spinner stdout/stderr collision: log messages inside withProgress appear on spinner line**: The \`withProgress\` spinner in \`src/lib/polling.ts\` writes to stdout using \`\r\x1b\[K\` (no trailing newline). Consola logger writes to stderr. On a shared terminal, any \`log.info()\` called \*\*inside\*\* the \`withProgress\` callback appears on the same line as the spinner text because stderr doesn't know about stdout's carriage-return positioning. Fix pattern: propagate data out of the callback via return value, then call \`log.info()\` \*\*after\*\* \`withProgress\` completes (when the \`finally\` block has already cleared the spinner line). This affected \`downloadBinaryToTemp\` in \`upgrade.ts\` where \`log.info('Applied delta patch...')\` fired inside the spinner callback. - -* **Use toMatchObject not toEqual when testing resolution results with optional fields**: When \`resolveProjectBySlug()\` or \`resolveOrgProjectTarget()\` adds optional fields (like \`projectData\`) to the return type, tests using \`expect(result).toEqual({ org, project })\` fail because \`toEqual\` requires exact match. Use \`toMatchObject({ org, project })\` instead — it checks the specified subset without failing on extra properties. This affects tests across \`event/view\`, \`log/view\`, \`trace/view\`, and \`trace/list\` test files. + +* **Stricli rejects unknown flags — pre-parsed global flags must be consumed from argv**: Stricli's arg parser is strict: any \`--flag\` not registered on a command throws \`No flag registered for --flag\`. Global flags (parsed before Stricli in bin.ts) MUST be spliced out of argv. \`--log-level\` was correctly consumed but \`--verbose\` was intentionally left in (for the \`api\` command's own \`--verbose\`). This breaks every other command. Also, \`argv.indexOf('--flag')\` doesn't match \`--flag=value\` form — must check both space-separated and equals-sign forms when pre-parsing. A Biome \`noRestrictedImports\` lint rule in \`biome.jsonc\` now blocks \`import { buildCommand } from "@stricli/core"\` at error level — only \`src/lib/command.ts\` is exempted. Other \`@stricli/core\` exports (\`buildRouteMap\`, \`run\`, etc.) are allowed. ### Pattern - -* **Branch naming and commit message conventions for Sentry CLI**: Branch naming: \`feat/\\` or \`fix/\-\\` (e.g., \`feat/ghcr-nightly-distribution\`, \`fix/268-limit-auto-pagination\`). Commit message format: \`type(scope): description (#issue)\` (e.g., \`fix(issue-list): auto-paginate --limit beyond 100 (#268)\`, \`feat(nightly): distribute via GHCR instead of GitHub Releases\`). Types seen: fix, refactor, meta, release, feat. PRs are created as drafts via \`gh pr create --draft\`. Implementation plans are attached to commits via \`git notes add\` rather than in PR body or commit message. - - -* **Codecov patch coverage only counts test:unit and test:isolated, not E2E**: CI coverage merges \`test:unit\` (\`test/lib test/commands test/types --coverage\`) and \`test:isolated\` (\`test/isolated --coverage\`) into \`coverage/merged.lcov\`. E2E tests (\`test/e2e\`) are NOT included in coverage reports. So func tests that spy on exports (e.g., \`spyOn(apiClient, 'getLogs')\`) give zero coverage to the mocked function's body. To cover \`api-client.ts\` function bodies in unit tests, mock \`globalThis.fetch\` + \`setOrgRegion()\` + \`setAuthToken()\` and call the real function. - - -* **Extract catch blocks to helper functions to stay under Biome complexity limit**: When a command's \`func()\` body exceeds Biome's cognitive complexity limit of 15, extract the \`catch\` block into a standalone \`never\`-returning helper function. Pattern: \`function handleXxxError(error: unknown, ...context): never { ... throw error; }\`. The helper handles error classification, telemetry recording, and error mapping before re-throwing. Used in \`plan.ts\` with \`handleSeerCommandError()\`. The extraction must move ALL catch-block logic — leaving partial code behind creates syntax errors. - - -* **Help-as-positional recovery uses error-path, not pre-execution interception**: Three layers of help-as-positional recovery exist: (1) \*\*Leaf commands\*\* (\`sentry issue list help\`): \`maybeRecoverWithHelp\` in \`buildCommand\` wrapper catches \`CliError\` (excluding \`OutputError\`) in the error handler. If any positional arg was \`"help"\`, shows help via \`introspectCommand()\` using \`commandPrefix\` stored on \`SentryContext\`. Only \`-h\` is NOT checked — Stricli intercepts it natively during route scanning. (2) \*\*Route groups\*\* (\`sentry dashboard help\`): Post-run check in \`bin.ts\` detects \`ExitCode.UnknownCommand\` + last arg \`"help"\`, rewrites argv to \`\["help", ...rest]\` and re-runs through the custom help command. (3) Both require \`commandPrefix\` on \`SentryContext\` (set in \`forCommand\`). Dynamic-imports \`help.js\` to avoid circular deps. - - -* **Sentry SDK tree-shaking patches must be regenerated via bun patch workflow**: The CLI uses \`patchedDependencies\` in \`package.json\` to tree-shake unused exports from \`@sentry/core\` and \`@sentry/node-core\` (AI integrations, feature flags, profiler, etc.). When bumping SDK versions: (1) remove old patches and \`patchedDependencies\` entries, (2) \`rm -rf ~/.bun/install/cache/@sentry\` to clear bun's cache (edits persist in cache otherwise), (3) \`bun install\` fresh, (4) \`bun patch @sentry/core\` then edit files and \`bun patch --commit\`, repeat for node-core. Key preserved exports: \`\_INTERNAL\_safeUnref\`, \`\_INTERNAL\_safeDateNow\` (core), \`nodeRuntimeMetricsIntegration\` (node-core). Manually generating patch files with \`git diff\` may fail — bun expects specific hash formats. Always use \`bun patch --commit\` to generate patches. - - -* **Shared pagination infrastructure: buildPaginationContextKey and parseCursorFlag**: Schema v12 replaced \`pagination\_cursors.cursor TEXT\` with \`cursor\_stack TEXT\` (JSON array) + \`page\_index INTEGER\`. Stack-based API in \`src/lib/db/pagination.ts\`: \`resolveCursor(flag, key, contextKey)\` maps keywords (next/prev/previous/first/last) to \`{cursor, direction}\`. \`advancePaginationState(key, contextKey, direction, nextCursor)\` pushes/pops the stack — back-then-forward truncates stale entries. \`hasPreviousPage(key, contextKey)\` checks \`page\_index > 0\`. \`clearPaginationState(key)\` removes state. \`parseCursorFlag\` in \`list-command.ts\` accepts next/prev/previous/first/last keywords. \`paginationHint()\` in \`org-list.ts\` builds bidirectional hints (\`-c prev | -c next\`). JSON envelope includes \`hasPrev\` boolean. All 7 list commands (trace, span, issue, project, team, repo, dashboard) use this stack API. \`resolveCursor()\` must be called inside \`org-all\` override closures. - - -* **Pagination contextKey must include all query-varying parameters with escaping**: Pagination \`contextKey\` must encode every query-varying parameter (sort, query, period) with \`escapeContextKeyValue()\` (replaces \`|\` with \`%7C\`). Always provide a fallback before escaping since \`flags.period\` may be \`undefined\` in tests despite having a default: \`flags.period ? escapeContextKeyValue(flags.period) : "90d"\`. - - -* **PR review workflow: reply, resolve, amend, force-push**: PR review workflow: (1) Read unresolved threads via GraphQL, (2) make code changes, (3) run lint+typecheck+tests, (4) create a SEPARATE commit per review round (not amend) for incremental review, (5) push normally, (6) reply to comments via REST API, (7) resolve threads via GraphQL \`resolveReviewThread\`. Only amend+force-push when user explicitly asks or pre-commit hook modified files. - - -* **Redact sensitive flags in raw argv before sending to telemetry**: Telemetry context and argv redaction patterns: \`withTelemetry\` calls \`initTelemetryContext()\` BEFORE the callback — user ID, email, instance ID, runtime, and is\_self\_hosted tags are automatically set. For org context, read \`getDefaultOrganization()\` from SQLite (no API call). When sending raw argv, redact sensitive flags: \`SENSITIVE\_FLAGS\` in \`telemetry.ts\` (currently \`token\`). Scan for \`--token\`/\`-token\`, replace following value with \`\[REDACTED]\`. Handle both \`--flag value\` and \`--flag=value\` forms. \`setFlagContext\` handles parsed flags separately. + +* **ClearScreen yield token for in-place terminal refresh in buildCommand wrapper**: Commands needing in-place refresh yield a \`ClearScreen\` token from \`src/lib/formatters/output.ts\`. The \`handleYieldedValue\` function in \`buildCommand\` sets a \`pendingClear\` flag; when the next \`CommandOutput\` is rendered, \`renderCommandOutput\` prepends \`\x1b\[H\x1b\[J\` and writes everything in a \*\*single \`stdout.write()\` call\*\* — no flicker. In JSON/plain modes the clear is silently ignored. Pattern: \`yield ClearScreen()\` then \`yield CommandOutput(data)\`. Critical: never split clear and content into separate writes. Also: never add a redundant clear-screen inside a \`HumanRenderer.render()\` method — the \`ClearScreen\` token is the sole mechanism. The dashboard renderer originally had its own \`\x1b\[2J\x1b\[H\` prepend on re-renders, causing double clears; this was removed. - -* **Stricli optional boolean flags produce tri-state (true/false/undefined)**: Stricli boolean flags with \`optional: true\` (no \`default\`) produce \`boolean | undefined\` in the flags type. \`--flag\` → \`true\`, \`--no-flag\` → \`false\`, omitted → \`undefined\`. This enables auto-detect patterns: explicit user choice overrides, \`undefined\` triggers heuristic. Used by \`--compact\` on issue list. The flag type must be \`readonly field?: boolean\` (not \`readonly field: boolean\`). This differs from \`default: false\` which always produces a defined boolean. + +* **Property-based tests for input validators use stringMatching for forbidden char coverage**: In \`test/lib/input-validation.property.test.ts\`, forbidden-character arbitraries are built with \`stringMatching\` targeting specific regex patterns (e.g., \`/^\[^\x00-\x1f]\*\[\x00-\x1f]\[^\x00-\x1f]\*$/\` for control chars). This ensures fast-check generates strings that always contain the forbidden character while varying surrounding content. The \`biome-ignore lint/suspicious/noControlCharactersInRegex\` suppression is needed on the control char regex constant in \`input-validation.ts\`. - -* **Testing Stricli command func() bodies via spyOn mocking**: Stricli/Bun test patterns: (1) Command func tests: \`const func = await cmd.loader()\`, then \`func.call(mockContext, flags, ...args)\`. \`loader()\` return type union causes LSP errors — false positives that pass \`tsc\`. File naming: \`\*.func.test.ts\`. (2) ESM prevents \`vi.spyOn\` on Node built-in exports. Workaround: test subclass that overrides the method calling the built-in. (3) Follow-mode uses \`setTimeout\`-based scheduling; test with \`interceptSigint()\` helper. \`Bun.sleep()\` has no AbortSignal so \`setTimeout\`/\`clearTimeout\` required. + +* **Sentry's official color token source location in getsentry/sentry repo**: Sentry's canonical color palette lives in \`static/app/utils/theme/scraps/tokens/color.tsx\` in getsentry/sentry. It defines \`categorical.light\` and \`categorical.dark\` palettes with named colors (blurple, purple, indigo, plum, magenta, pink, salmon, orange, yellow, lime, green). Chart palettes are built in \`static/app/utils/theme/theme.tsx\` using \`CHART\_PALETTE\_LIGHT\` and \`CHART\_PALETTE\_DARK\` arrays that progressively add colors as series count grows (1→blurple, 6→blurple/indigo/pink/orange/yellow/green, etc.). GitHub API tree endpoint (\`/git/trees/master?recursive=1\`) can locate files without needing authenticated code search. - -* **validateWidgetEnums skipDeprecatedCheck for edit-path inherited datasets**: When editing a widget, \`effectiveDataset = flags.dataset ?? existing.widgetType\` may inherit a deprecated type (e.g., \`discover\`). The \`validateWidgetEnums\` deprecation check must be skipped for inherited values — only fire when the user explicitly passes \`--dataset\`. Solution: \`validateWidgetEnums(effectiveDisplay, effectiveDataset, { skipDeprecatedCheck: true })\` in \`edit.ts\`. The cross-validation between display type and dataset still runs on effective values, catching incompatible combos. The deprecation rejection helper \`rejectInvalidDataset()\` is extracted to keep \`validateWidgetEnums\` under Biome's complexity limit of 15. + +* **Shared flag constants in list-command.ts for cross-command consistency**: \`src/lib/list-command.ts\` exports shared Stricli flag definitions (\`FIELDS\_FLAG\`, \`FRESH\_FLAG\`, \`FRESH\_ALIASES\`) reused across all commands. When adding a new global-ish flag to multiple commands, define it once here as a const satisfying Stricli's flag shape, then spread into each command's \`flags\` object. The \`--fields\` flag is \`{ kind: 'parsed', parse: String, brief: '...', optional: true }\`. \`parseFieldsList()\` in \`formatters/json.ts\` handles comma-separated parsing with trim/dedup. \`writeJson()\` accepts an optional \`fields\` array and calls \`filterFields()\` before serialization. - -* **set-commits default mode makes speculative --auto API call by design**: When \`release set-commits\` is called without \`--auto\` or \`--local\`, it tries auto-discovery first and falls back to local git on 400 error. This matches the reference sentry-cli behavior (parity-correct). A per-org negative cache in the \`metadata\` table (\`repos_configured.\` = \`"false"\`, 1-hour TTL) skips the speculative auto call on subsequent runs when no repo integration is configured. The cache clears on successful auto-discovery. + +* **SKILL.md generator must filter hidden Stricli flags**: \`script/generate-skill.ts\` introspects Stricli's route tree to auto-generate SKILL.md. \`FlagDef\` must include \`hidden?: boolean\`; \`extractFlags\` propagates it so \`generateCommandDoc\` filters out hidden flags alongside \`help\`/\`helpAll\`. Hidden flags from \`buildCommand\` (\`--log-level\`, \`--verbose\`) appear globally in \`docs/src/content/docs/commands/index.md\` Global Options section, pulled into SKILL.md via \`loadCommandsOverview\`. When \`cmd.jsonFields\` is present (from Zod schema registration), \`generateFullCommandDoc\` renders a markdown "JSON Fields" table with field name, type, and description columns in reference docs. diff --git a/src/cli.ts b/src/cli.ts index 5c749d8bc..413e50e1d 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -136,6 +136,7 @@ export async function runCli(cliArgs: string[]): Promise { const { isatty } = await import("node:tty"); const { ExitCode, run } = await import("@stricli/core"); const { app } = await import("./app.js"); + const { hoistGlobalFlags } = await import("./lib/argv-hoist.js"); const { buildContext } = await import("./context.js"); const { AuthError, OutputError, formatError, getExitCode } = await import( "./lib/errors.js" @@ -155,6 +156,13 @@ export async function runCli(cliArgs: string[]): Promise { shouldSuppressNotification, } = await import("./lib/version-check.js"); + // Move global flags (--verbose, -v, --log-level, --json, --fields) from any + // position to the end of argv, where Stricli's leaf-command parser can + // find them. This allows `sentry --verbose issue list` to work. + // The original cliArgs are kept for post-run checks (e.g., help recovery) + // that rely on the original token positions. + const hoistedArgs = hoistGlobalFlags(cliArgs); + // --------------------------------------------------------------------------- // Error-recovery middleware // --------------------------------------------------------------------------- @@ -363,7 +371,9 @@ export async function runCli(cliArgs: string[]): Promise { setLogLevel(envLogLevel); } - const suppressNotification = shouldSuppressNotification(cliArgs); + // Use hoisted args so positional checks (e.g., args[0] === "cli") work + // even when global flags precede the subcommand in the original argv. + const suppressNotification = shouldSuppressNotification(hoistedArgs); // Start background update check (non-blocking) if (!suppressNotification) { @@ -371,7 +381,7 @@ export async function runCli(cliArgs: string[]): Promise { } try { - await executor(cliArgs); + await executor(hoistedArgs); // When Stricli can't match a subcommand in a route group (e.g., // `sentry dashboard help`), it writes "No command registered for `help`" @@ -380,6 +390,8 @@ export async function runCli(cliArgs: string[]): Promise { // the custom help command with proper introspection output. // Check both raw (-5) and unsigned (251) forms because Node.js keeps // the raw value while Bun converts to unsigned byte. + // Uses original cliArgs (not hoisted) so the `at(-1) === "help"` check + // works when global flags were placed before "help". if ( (process.exitCode === ExitCode.UnknownCommand || process.exitCode === (ExitCode.UnknownCommand + 256) % 256) && diff --git a/src/lib/argv-hoist.ts b/src/lib/argv-hoist.ts new file mode 100644 index 000000000..2f71f829e --- /dev/null +++ b/src/lib/argv-hoist.ts @@ -0,0 +1,189 @@ +/** + * Argv preprocessor that moves global flags to the end of the argument list. + * + * Stricli only parses flags at the leaf command level, so flags like + * `--verbose` placed before the subcommand (`sentry --verbose issue list`) + * fail route resolution. This module relocates known global flags from any + * position to the tail of argv where Stricli's leaf-command parser can + * find them. + * + * Flag metadata is derived from the shared {@link GLOBAL_FLAGS} definition + * in `global-flags.ts` so both the hoisting preprocessor and the + * `buildCommand` injection stay in sync automatically. + */ + +import { GLOBAL_FLAGS } from "./global-flags.js"; + +/** Resolved flag metadata used by the hoisting algorithm. */ +type HoistableFlag = { + /** Long flag name without `--` prefix (e.g., `"verbose"`) */ + readonly name: string; + /** Single-char short alias without `-` prefix, or `null` if none */ + readonly short: string | null; + /** Whether the flag consumes the next token as its value */ + readonly takesValue: boolean; + /** Whether `--no-` is recognized as the negation form */ + readonly negatable: boolean; +}; + +/** Derive hoisting metadata from the shared flag definitions. */ +const HOISTABLE_FLAGS: readonly HoistableFlag[] = GLOBAL_FLAGS.map((f) => ({ + name: f.name, + short: f.short, + takesValue: f.kind === "value", + negatable: f.kind === "boolean", +})); + +/** Pre-built lookup: long name → flag definition */ +const FLAG_BY_NAME = new Map(HOISTABLE_FLAGS.map((f) => [f.name, f])); + +/** Pre-built lookup: short alias → flag definition */ +const FLAG_BY_SHORT = new Map( + HOISTABLE_FLAGS.filter( + (f): f is HoistableFlag & { short: string } => f.short !== null + ).map((f) => [f.short, f]) +); + +/** Names that support `--no-` negation */ +const NEGATABLE_NAMES = new Set( + HOISTABLE_FLAGS.filter((f) => f.negatable).map((f) => f.name) +); + +/** + * Match result from {@link matchHoistable}. + * + * - `"plain"`: `--flag` (boolean) or `--flag` (value-taking, value is next token) + * - `"eq"`: `--flag=value` (value embedded in token) + * - `"negated"`: `--no-flag` + * - `"short"`: `-v` (single-char alias) + */ +type MatchForm = "plain" | "eq" | "negated" | "short"; + +/** Try matching a `--no-` negation form. */ +function matchNegated( + name: string +): { flag: HoistableFlag; form: MatchForm } | null { + if (!name.startsWith("no-")) { + return null; + } + const baseName = name.slice(3); + if (!NEGATABLE_NAMES.has(baseName)) { + return null; + } + const flag = FLAG_BY_NAME.get(baseName); + return flag ? { flag, form: "negated" } : null; +} + +/** + * Match a token against the hoistable flag registry. + * + * @returns The matched flag and form, or `null` if not hoistable. + */ +function matchHoistable( + token: string +): { flag: HoistableFlag; form: MatchForm } | null { + // Short alias: -v (exactly two chars, dash + letter) + if (token.length === 2 && token[0] === "-" && token[1] !== "-") { + const flag = FLAG_BY_SHORT.get(token[1] ?? ""); + return flag ? { flag, form: "short" } : null; + } + + if (!token.startsWith("--")) { + return null; + } + + // --flag=value form + const eqIdx = token.indexOf("="); + if (eqIdx !== -1) { + const name = token.slice(2, eqIdx); + const flag = FLAG_BY_NAME.get(name); + return flag?.takesValue ? { flag, form: "eq" } : null; + } + + const name = token.slice(2); + const negated = matchNegated(name); + if (negated) { + return negated; + } + const flag = FLAG_BY_NAME.get(name); + return flag ? { flag, form: "plain" } : null; +} + +/** + * Hoist a single matched flag token (and its value if applicable) into the + * `hoisted` array, advancing the index past the consumed tokens. + * + * Extracted from the main loop to keep {@link hoistGlobalFlags} under + * Biome's cognitive complexity limit. + */ +function consumeFlag( + argv: readonly string[], + index: number, + match: { flag: HoistableFlag; form: MatchForm }, + hoisted: string[] +): number { + const token = argv[index] ?? ""; + + // --flag=value or --no-flag: always a single token + if (match.form === "eq" || match.form === "negated") { + hoisted.push(token); + return index + 1; + } + + // --flag or -v: may consume a following value token + if (match.flag.takesValue) { + hoisted.push(token); + const next = index + 1; + if (next < argv.length) { + hoisted.push(argv[next] ?? ""); + return next + 1; + } + // No value follows — the bare flag is still hoisted; + // Stricli will report the missing value at parse time. + return next; + } + + // Boolean flag (--flag or -v): single token + hoisted.push(token); + return index + 1; +} + +/** + * Move global flags from any position in argv to the end. + * + * Tokens after `--` are never touched. The relative order of both + * hoisted and non-hoisted tokens is preserved. + * + * @param argv - Raw CLI arguments (e.g., `process.argv.slice(2)`) + * @returns New array with global flags relocated to the tail + */ +export function hoistGlobalFlags(argv: readonly string[]): string[] { + const remaining: string[] = []; + const hoisted: string[] = []; + /** Tokens from `--` onward (positional-only region). */ + const positionalTail: string[] = []; + + let i = 0; + while (i < argv.length) { + const token = argv[i] ?? ""; + + // Stop scanning at -- separator; pass everything through verbatim. + // Hoisted flags must appear BEFORE -- so Stricli parses them as flags. + if (token === "--") { + for (let j = i; j < argv.length; j += 1) { + positionalTail.push(argv[j] ?? ""); + } + break; + } + + const match = matchHoistable(token); + if (match) { + i = consumeFlag(argv, i, match, hoisted); + } else { + remaining.push(token); + i += 1; + } + } + + return [...remaining, ...hoisted, ...positionalTail]; +} diff --git a/src/lib/command.ts b/src/lib/command.ts index fc856c84f..ea36e34fd 100644 --- a/src/lib/command.ts +++ b/src/lib/command.ts @@ -54,6 +54,7 @@ import { writeFooter, } from "./formatters/output.js"; import { isPlainOutput } from "./formatters/plain-detect.js"; +import { GLOBAL_FLAGS } from "./global-flags.js"; import { LOG_LEVEL_NAMES, type LogLevelName, @@ -377,7 +378,25 @@ export function buildCommand< // This makes field info visible in Stricli's --help output. const enrichedDocs = enrichDocsWithSchema(builderArgs.docs, outputConfig); - const mergedParams = { ...existingParams, flags: mergedFlags }; + // Inject short aliases for global flags (e.g., -v → --verbose). + // Derived from the shared GLOBAL_FLAGS definition so adding a new + // global flag with a short alias automatically propagates here. + const existingAliases = (existingParams.aliases ?? {}) as Record< + string, + unknown + >; + const mergedAliases: Record = { ...existingAliases }; + for (const gf of GLOBAL_FLAGS) { + if (gf.short && !(gf.name in existingFlags || gf.short in mergedAliases)) { + mergedAliases[gf.short] = gf.name; + } + } + + const mergedParams = { + ...existingParams, + flags: mergedFlags, + aliases: mergedAliases, + }; /** * If the yielded value is a {@link CommandOutput}, render it via diff --git a/src/lib/global-flags.ts b/src/lib/global-flags.ts new file mode 100644 index 000000000..179c5af19 --- /dev/null +++ b/src/lib/global-flags.ts @@ -0,0 +1,43 @@ +/** + * Single source of truth for global CLI flags. + * + * Global flags are injected into every leaf command by {@link buildCommand} + * and hoisted from any argv position by {@link hoistGlobalFlags}. This + * module defines the metadata once so both systems stay in sync + * automatically — adding a flag here is all that's needed. + * + * The Stricli flag *shapes* (kind, brief, default, etc.) remain in + * `command.ts` because they depend on Stricli types and runtime values. + * This module only stores the identity and argv-level behavior of each flag. + */ + +/** + * Behavior category for a global flag. + * + * - `"boolean"` — standalone toggle, supports `--no-` negation + * - `"value"` — consumes the next token (or `=`-joined value) + */ +type GlobalFlagKind = "boolean" | "value"; + +/** Metadata for a single global CLI flag. */ +type GlobalFlagDef = { + /** Long flag name without `--` prefix (e.g., `"verbose"`) */ + readonly name: string; + /** Single-char short alias without `-` prefix, or `null` if none */ + readonly short: string | null; + /** Whether this is a boolean toggle or a value-taking flag */ + readonly kind: GlobalFlagKind; +}; + +/** + * All global flags that are injected into every leaf command. + * + * Order doesn't matter — both the hoisting preprocessor and the + * `buildCommand` wrapper build lookup structures from this list. + */ +export const GLOBAL_FLAGS: readonly GlobalFlagDef[] = [ + { name: "verbose", short: "v", kind: "boolean" }, + { name: "log-level", short: null, kind: "value" }, + { name: "json", short: null, kind: "boolean" }, + { name: "fields", short: null, kind: "value" }, +]; diff --git a/test/lib/argv-hoist.property.test.ts b/test/lib/argv-hoist.property.test.ts new file mode 100644 index 000000000..8f3dac59c --- /dev/null +++ b/test/lib/argv-hoist.property.test.ts @@ -0,0 +1,171 @@ +/** + * Property-based tests for {@link hoistGlobalFlags}. + * + * These verify invariants that must hold for any valid input: + * 1. Token conservation — no tokens added or dropped + * 2. Order preservation — non-hoisted tokens keep their relative order + * 3. Idempotency — hoisting twice gives the same result as hoisting once + */ + +import { describe, expect, test } from "bun:test"; +import { array, constantFrom, assert as fcAssert, property } from "fast-check"; +import { hoistGlobalFlags } from "../../src/lib/argv-hoist.js"; +import { DEFAULT_NUM_RUNS } from "../model-based/helpers.js"; + +/** Tokens that should be hoisted (global flags and their values) */ +const GLOBAL_FLAG_TOKENS = [ + "--verbose", + "--no-verbose", + "--json", + "--no-json", + "-v", + "--log-level", + "--fields", +] as const; + +/** Tokens that should never be hoisted */ +const NON_GLOBAL_TOKENS = [ + "issue", + "list", + "view", + "my-org/", + "my-org/my-project", + "123", + "--limit", + "25", + "--sort", + "date", + "-x", + "-h", + "help", + "cli", + "upgrade", + "api", +] as const; + +/** All tokens mixed together (excluding -- separator, handled separately) */ +const ALL_TOKENS = [...GLOBAL_FLAG_TOKENS, ...NON_GLOBAL_TOKENS] as const; + +const nonGlobalTokenArb = constantFrom(...NON_GLOBAL_TOKENS); +const allTokenArb = constantFrom(...ALL_TOKENS); +const argvArb = array(allTokenArb, { minLength: 0, maxLength: 12 }); + +/** + * Check if a token is a hoistable global flag or its negation/short form. + * Must match the flag registry in argv-hoist.ts. + */ +const HOISTABLE_SET = new Set([ + "--verbose", + "--no-verbose", + "--json", + "--no-json", + "-v", + "--log-level", + "--fields", +]); + +function isHoistableToken(token: string): boolean { + return HOISTABLE_SET.has(token); +} + +describe("property: hoistGlobalFlags", () => { + test("token conservation: output contains exactly the same tokens as input", () => { + fcAssert( + property(argvArb, (argv) => { + const result = hoistGlobalFlags(argv); + expect([...result].sort()).toEqual([...argv].sort()); + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("order preservation: non-hoistable tokens keep relative order", () => { + const mixedArgvArb = array( + constantFrom( + "--verbose", + "-v", + "--json", + "issue", + "list", + "my-org/", + "--limit", + "25", + "cli", + "upgrade" + ), + { minLength: 0, maxLength: 12 } + ); + + fcAssert( + property(mixedArgvArb, (argv) => { + const result = hoistGlobalFlags(argv); + const nonHoisted = result.filter((t) => !isHoistableToken(t)); + const originalNonHoisted = argv.filter((t) => !isHoistableToken(t)); + expect(nonHoisted).toEqual(originalNonHoisted); + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("idempotency: hoisting twice gives the same result as once", () => { + fcAssert( + property(argvArb, (argv) => { + const once = hoistGlobalFlags(argv); + const twice = hoistGlobalFlags(once); + expect(twice).toEqual(once); + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("hoisted tokens appear after all non-hoisted tokens", () => { + /** Value-taking flags whose next token also gets hoisted */ + const VALUE_TAKING = new Set(["--log-level", "--fields"]); + + fcAssert( + property(argvArb, (argv) => { + const result = hoistGlobalFlags(argv); + // Find the index of the first hoisted token + const firstHoistedIdx = result.findIndex((t) => isHoistableToken(t)); + if (firstHoistedIdx === -1) { + return; // No hoistable tokens — nothing to check + } + // All tokens after the first hoisted token should be either: + // (a) a hoistable flag, or (b) a value following a value-taking flag + const tail = result.slice(firstHoistedIdx); + let skipNext = false; + for (const token of tail) { + if (skipNext) { + skipNext = false; + continue; + } + if (isHoistableToken(token)) { + // If it's a value-taking flag, skip the next token (its value) + if (VALUE_TAKING.has(token)) { + skipNext = true; + } + continue; + } + // Token is not hoistable — fail + expect(token).toBe( + `` + ); + } + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("no-op for argv with only non-global tokens", () => { + const nonGlobalArgvArb = array(nonGlobalTokenArb, { + minLength: 0, + maxLength: 10, + }); + fcAssert( + property(nonGlobalArgvArb, (argv) => { + expect(hoistGlobalFlags(argv)).toEqual(argv); + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); +}); diff --git a/test/lib/argv-hoist.test.ts b/test/lib/argv-hoist.test.ts new file mode 100644 index 000000000..46b0194c8 --- /dev/null +++ b/test/lib/argv-hoist.test.ts @@ -0,0 +1,290 @@ +/** + * Unit tests for {@link hoistGlobalFlags}. + * + * Core invariants (token conservation, order preservation, idempotency) are + * tested via property-based tests in argv-hoist.property.test.ts. These tests + * focus on specific scenarios and edge cases. + */ + +import { describe, expect, test } from "bun:test"; +import { hoistGlobalFlags } from "../../src/lib/argv-hoist.js"; + +describe("hoistGlobalFlags", () => { + // ------------------------------------------------------------------------- + // Passthrough (no hoistable flags) + // ------------------------------------------------------------------------- + + test("returns empty array for empty input", () => { + expect(hoistGlobalFlags([])).toEqual([]); + }); + + test("returns argv unchanged when no global flags present", () => { + expect(hoistGlobalFlags(["issue", "list", "--limit", "25"])).toEqual([ + "issue", + "list", + "--limit", + "25", + ]); + }); + + test("does not hoist unknown flags", () => { + expect(hoistGlobalFlags(["--limit", "25", "issue", "list"])).toEqual([ + "--limit", + "25", + "issue", + "list", + ]); + }); + + // ------------------------------------------------------------------------- + // Boolean flag hoisting: --verbose + // ------------------------------------------------------------------------- + + test("hoists --verbose from before subcommand", () => { + expect(hoistGlobalFlags(["--verbose", "issue", "list"])).toEqual([ + "issue", + "list", + "--verbose", + ]); + }); + + test("hoists --verbose from middle position", () => { + expect(hoistGlobalFlags(["cli", "--verbose", "upgrade"])).toEqual([ + "cli", + "upgrade", + "--verbose", + ]); + }); + + test("flag already at end stays at end", () => { + expect(hoistGlobalFlags(["issue", "list", "--verbose"])).toEqual([ + "issue", + "list", + "--verbose", + ]); + }); + + // ------------------------------------------------------------------------- + // Short alias: -v + // ------------------------------------------------------------------------- + + test("hoists -v from before subcommand", () => { + expect(hoistGlobalFlags(["-v", "issue", "list"])).toEqual([ + "issue", + "list", + "-v", + ]); + }); + + test("hoists -v from middle position", () => { + expect(hoistGlobalFlags(["cli", "-v", "upgrade"])).toEqual([ + "cli", + "upgrade", + "-v", + ]); + }); + + test("does not hoist unknown short flags", () => { + expect(hoistGlobalFlags(["-x", "issue", "list"])).toEqual([ + "-x", + "issue", + "list", + ]); + }); + + // ------------------------------------------------------------------------- + // Negation: --no-verbose, --no-json + // ------------------------------------------------------------------------- + + test("hoists --no-verbose", () => { + expect(hoistGlobalFlags(["--no-verbose", "issue", "list"])).toEqual([ + "issue", + "list", + "--no-verbose", + ]); + }); + + test("hoists --no-json", () => { + expect(hoistGlobalFlags(["--no-json", "issue", "list"])).toEqual([ + "issue", + "list", + "--no-json", + ]); + }); + + test("does not hoist --no-log-level (not negatable)", () => { + expect(hoistGlobalFlags(["--no-log-level", "issue", "list"])).toEqual([ + "--no-log-level", + "issue", + "list", + ]); + }); + + // ------------------------------------------------------------------------- + // Boolean flag: --json + // ------------------------------------------------------------------------- + + test("hoists --json from before subcommand", () => { + expect(hoistGlobalFlags(["--json", "issue", "list"])).toEqual([ + "issue", + "list", + "--json", + ]); + }); + + // ------------------------------------------------------------------------- + // Value flag: --log-level (separate value) + // ------------------------------------------------------------------------- + + test("hoists --log-level with separate value", () => { + expect(hoistGlobalFlags(["--log-level", "debug", "issue", "list"])).toEqual( + ["issue", "list", "--log-level", "debug"] + ); + }); + + test("hoists --log-level=debug as single token", () => { + expect(hoistGlobalFlags(["--log-level=debug", "issue", "list"])).toEqual([ + "issue", + "list", + "--log-level=debug", + ]); + }); + + test("hoists --log-level at end with no value", () => { + expect(hoistGlobalFlags(["issue", "list", "--log-level"])).toEqual([ + "issue", + "list", + "--log-level", + ]); + }); + + // ------------------------------------------------------------------------- + // Value flag: --fields + // ------------------------------------------------------------------------- + + test("hoists --fields with separate value", () => { + expect(hoistGlobalFlags(["--fields", "id,title", "issue", "list"])).toEqual( + ["issue", "list", "--fields", "id,title"] + ); + }); + + test("hoists --fields=id,title as single token", () => { + expect(hoistGlobalFlags(["--fields=id,title", "issue", "list"])).toEqual([ + "issue", + "list", + "--fields=id,title", + ]); + }); + + // ------------------------------------------------------------------------- + // Multiple flags + // ------------------------------------------------------------------------- + + test("hoists multiple global flags preserving relative order", () => { + expect(hoistGlobalFlags(["--verbose", "--json", "issue", "list"])).toEqual([ + "issue", + "list", + "--verbose", + "--json", + ]); + }); + + test("hoists flags from mixed positions", () => { + expect( + hoistGlobalFlags([ + "--verbose", + "issue", + "--json", + "list", + "--fields", + "id", + ]) + ).toEqual(["issue", "list", "--verbose", "--json", "--fields", "id"]); + }); + + // ------------------------------------------------------------------------- + // Repeated flags + // ------------------------------------------------------------------------- + + test("hoists duplicate --verbose flags", () => { + expect( + hoistGlobalFlags(["--verbose", "issue", "--verbose", "list"]) + ).toEqual(["issue", "list", "--verbose", "--verbose"]); + }); + + // ------------------------------------------------------------------------- + // -- separator + // ------------------------------------------------------------------------- + + test("does not hoist flags after -- separator", () => { + expect(hoistGlobalFlags(["issue", "--", "--verbose", "list"])).toEqual([ + "issue", + "--", + "--verbose", + "list", + ]); + }); + + test("hoists flags before -- and places them before the separator", () => { + expect(hoistGlobalFlags(["--verbose", "issue", "--", "--json"])).toEqual([ + "issue", + "--verbose", + "--", + "--json", + ]); + }); + + test("hoisted flags appear before -- not after it", () => { + expect( + hoistGlobalFlags(["--verbose", "issue", "--", "positional-arg"]) + ).toEqual(["issue", "--verbose", "--", "positional-arg"]); + }); + + // ------------------------------------------------------------------------- + // Real-world scenarios + // ------------------------------------------------------------------------- + + test("hoists from root level for deeply nested commands", () => { + expect(hoistGlobalFlags(["--verbose", "--json", "cli", "upgrade"])).toEqual( + ["cli", "upgrade", "--verbose", "--json"] + ); + }); + + test("hoists --verbose for api command", () => { + expect(hoistGlobalFlags(["--verbose", "api", "/endpoint"])).toEqual([ + "api", + "/endpoint", + "--verbose", + ]); + }); + + test("hoists -v with log-level from root", () => { + expect( + hoistGlobalFlags(["-v", "--log-level", "debug", "cli", "upgrade"]) + ).toEqual(["cli", "upgrade", "-v", "--log-level", "debug"]); + }); + + test("preserves non-global flags in original position", () => { + expect( + hoistGlobalFlags([ + "--verbose", + "issue", + "list", + "my-org/", + "--limit", + "25", + "--sort", + "date", + ]) + ).toEqual([ + "issue", + "list", + "my-org/", + "--limit", + "25", + "--sort", + "date", + "--verbose", + ]); + }); +});