diff --git a/AGENTS.md b/AGENTS.md index 4f40f7f8..8d256022 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 5c749d8b..413e50e1 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 00000000..2f71f829 --- /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 fc856c84..ea36e34f 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 00000000..179c5af1 --- /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 00000000..8f3dac59 --- /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 00000000..46b0194c --- /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", + ]); + }); +});