From 0331386082275797071e61598b1c586379635bfb Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 10 Apr 2026 23:50:38 +0000 Subject: [PATCH 1/2] fix(dashboard): guard sort param by dataset in widget table queries (#129) queryWidgetTable was passing sort/orderby to the events API for all datasets, but the sort param is only supported on the spans dataset. Errors/discover endpoints reject it with 400 Bad Request. Also enrich ApiError captures in exceptionWhileRunningCommand with structured Sentry context (status, endpoint, detail) for better diagnostics on future API errors. --- AGENTS.md | 107 +++++++++++++++----------------------- src/app.ts | 9 ++++ src/lib/api/dashboards.ts | 4 +- 3 files changed, 55 insertions(+), 65 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f53de10b3..20d5fc03a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -984,92 +984,86 @@ mock.module("./some-module", () => ({ ### Architecture - -* **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. + +* **commandPrefix on SentryContext enables command identity in buildCommand wrapper**: commandPrefix and help-as-positional recovery: \`SentryContext.commandPrefix\` (optional \`readonly string\[]\`) is populated in \`forCommand()\` — Stricli calls this with the full prefix (e.g., \`\["sentry", "issue", "list"]\`) for help recovery and telemetry. Help recovery: (1) Leaf commands: \`maybeRecoverWithHelp\` catches \`CliError\` if any positional was \`"help"\`, shows help via \`introspectCommand()\`. (2) Route groups: post-run check in \`bin.ts\` detects \`ExitCode.UnknownCommand\` + last arg \`"help"\`, rewrites argv. Both dynamic-import \`help.js\` to avoid circular deps. - -* **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. + +* **Dashboard widget interval computed from terminal width and layout before API calls**: Dashboard widget interval computed from terminal width: \`computeOptimalInterval()\` in \`src/lib/api/dashboards.ts\` calculates optimal chart interval before API calls. Formula: \`colWidth = floor(layout.w / 6 \* termWidth)\`, \`chartWidth = colWidth - 4 - gutterW\`, \`idealSeconds = periodSeconds / chartWidth\`. Snaps to nearest Sentry interval bucket (\`1m\`–\`1d\`). \`periodToSeconds()\` parses \`"24h"\`, \`"7d"\` etc. \`queryAllWidgets\` computes per-widget intervals using \`getTermWidth()\` (min 80, fallback 100). - -* **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. + +* **defaultCommand:help blocks Stricli fuzzy matching for top-level typos**: Fuzzy matching for CLI typos: Stricli has built-in Damerau-Levenshtein for subcommand/flag typos within known routes. \`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 \\`. Additional fuzzy: tab-completion in \`complete.ts\`, platform suggestions in \`platforms.ts\`, plural alias detection in \`app.ts\`. JSON includes \`suggestions\` array. - -* **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 }\`. + +* **DSN org prefix normalization in arg-parsing.ts**: DSN org prefix normalization has four code paths: (1) \`extractOrgIdFromHost\` in \`dsn/parser.ts\` strips \`o\` prefix during parsing. (2) \`stripDsnOrgPrefix()\` handles user-typed \`o1081365/\` in \`parseOrgProjectArg()\` and \`resolveEffectiveOrg()\`. (3) \`normalizeNumericOrg()\` in \`resolve-target.ts\` handles bare numeric IDs — checks DB cache via \`getOrgByNumericId()\`, falls back to \`listOrganizationsUncached()\`. (4) Dashboard's \`resolveOrgFromTarget()\` pipes through \`resolveEffectiveOrg()\`. Critical: many API endpoints reject numeric org IDs with 404/403 — always normalize to slugs. - -* **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. + +* **GHCR versioned nightly tags for delta upgrade support**: GHCR nightly distribution with delta upgrades: 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. - -* **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\`. + +* **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. -### Decision + +* **resolveProjectBySlug carries full projectData to avoid redundant getProject calls**: resolveProjectBySlug carries full projectData: Returns \`{ org, project, projectData: SentryProject }\` from \`findProjectsBySlug()\`. \`ResolvedOrgProject\` and \`ResolvedTarget\` have optional \`projectData?\` (populated only in project-search path). Downstream commands use \`resolved.projectData ?? await getProject(org, project)\` to skip redundant API calls (~500-800ms savings). When testing resolution results with optional fields, use \`toMatchObject()\` not \`toEqual()\`. - -* **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. + +* **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. * **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 dashboard API rejects discover/transaction-like widget types — use spans**: Sentry dashboard API rejects deprecated widget types — use spans: \`widgetType: 'discover'\` and \`'transaction-like'\` are rejected. Use \`'spans'\` for new widgets. Codebase splits \`WIDGET\_TYPES\` (active) vs \`ALL\_WIDGET\_TYPES\` (includes deprecated for parsing). \`DashboardWidgetInputSchema\` uses \`ALL\_WIDGET\_TYPES\` so editing existing deprecated widgets passes Zod. \`validateWidgetEnums()\` rejects deprecated for creation but accepts \`skipDeprecatedCheck: true\` for the edit path where \`effectiveDataset\` may be inherited. Tests must use \`error-events\` instead of \`discover\`. -* **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 issue stats field: time-series controlled by groupStatsPeriod**: Issue stats and list 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. \`--compact\` tri-state: explicit overrides; \`undefined\` triggers \`shouldAutoCompact(rowCount)\` — compact if \`3N + 3 > termHeight\`. * **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 source files, 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). CI runs this after every push via a \`github-actions\[bot]\` commit, overwriting any manual edits. 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/content/docs/commands/\*.md\` — examples extracted per command via marked AST parsing. After editing sources, run \`bun run generate:skill\` locally and commit both source and generated files. CI's \`bun run check:skill\` fails if generated files are stale. +* **SKILL.md is fully generated — edit source files, not output**: SKILL.md is fully generated — edit source files, not output: Skill files under \`plugins/sentry-cli/skills/sentry-cli/\` are fully generated by \`bun run generate:skill\` (runs as part of \`dev\`, \`build\`, \`typecheck\`, \`test\`). To change content, edit sources: (1) \`docs/src/content/docs/agent-guidance.md\` → SKILL.md Agent Guidance section. (2) \`src/commands/\*/\` flag \`brief\` strings → reference file descriptions. (3) \`docs/src/fragments/commands/\*.md\` → hand-written examples. Command docs (\`docs/src/content/docs/commands/\*.md\`) are gitignored, rebuilt from fragments + CLI metadata. \`bun run check:fragments\` validates consistency. CI auto-commits stale skill files. -* **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. +* **Stricli route errors are uninterceptable — only post-run detection works**: Stricli route errors are uninterceptable — only post-run detection works: (1) Route failures write to stderr and return \`ExitCode.UnknownCommand\` internally; only post-\`run()\` \`process.exitCode\` check works. (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. Top-level typos via \`defaultCommand:help\` → \`OutputError\` → \`process.exit(1)\` skip error reporting. -* **Three Sentry APIs for span custom attributes with different capabilities**: Three Sentry endpoints serve span data with different custom attribute support: (1) \`/trace/{traceId}/\` — hierarchical tree; supports \`additional\_attributes\` query param to request named attributes (must enumerate). Returns \`measurements\` (web vitals, always populated with zeros for non-browser spans). (2) \`/projects/{org}/{project}/trace-items/{itemId}/?trace\_id={id}\&item\_type=spans\` — single span full detail; returns ALL attributes automatically as \`{name, type, value}\[]\`. No enumeration needed. Frontend span detail sidebar uses this. (3) \`/events/?dataset=spans\&field=X\` — list/search; requires explicit \`field\` params. The CLI's \`span view\` calls endpoint 2 via \`getSpanDetails()\` with p-limit(5) concurrency. \`trace view\` passes \`--fields\` to endpoint 1 as \`additional\_attributes\`. - - -* **Trace view measurements filtering strips zero-valued web vitals from JSON**: The \`/trace/{traceId}/\` endpoint returns hardcoded web vitals measurements (LCP, FCP, CLS, INP, TTFB + score ratios) on ALL spans, even non-browser ones where they're all zero. \`filterSpanMeasurements()\` in \`trace/view.ts\` recursively strips zero-valued entries from the JSON output. Non-zero measurements on root pageload spans are preserved. Uses destructuring + spread (not \`delete\`) to satisfy Biome's \`noDelete\` rule. +* **Three Sentry APIs for span custom attributes with different capabilities**: Three Sentry APIs for span data with different custom attribute support: (1) \`/trace/{traceId}/\` — hierarchical tree; \`additional\_attributes\` query param enumerates requested attributes. Returns \`measurements\` (web vitals, zero-filled on non-browser spans — \`filterSpanMeasurements()\` strips zeros in JSON). (2) \`/projects/{org}/{project}/trace-items/{itemId}/?trace\_id={id}\&item\_type=spans\` — single span full detail; returns ALL attributes as \`{name, type, value}\[]\` automatically. CLI's \`span view\` uses this via \`getSpanDetails()\`. (3) \`/events/?dataset=spans\&field=X\` — list/search; requires explicit \`field\` params. * **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. +### Decision + + +* **400 Bad Request from Sentry API indicates a CLI bug, not a user error**: The user considers 400 Bad Request responses from the Sentry API to be CLI bugs — the CLI should never construct a malformed request. Unlike 404 (wrong ID) or 403 (no access), a 400 means the request payload or parameters are invalid, which is a code defect. Therefore \`Sentry.captureException()\` should continue capturing 400s. The \`isClientApiError()\` function in \`telemetry.ts\` treats all 4xx uniformly, but 400 specifically warrants different handling from other client errors like 404/403/429. When fixing noisy telemetry, distinguish between 'user pointed at wrong thing' (404/403) and 'CLI sent bad data' (400). ### Gotcha - -* **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. + +* **Biome lint: Response.redirect() required, nested ternaries forbidden**: Biome lint rules and complexity: (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 — extract catch blocks to helper functions (pattern: \`function handleXxxError(error: unknown, ...ctx): never\`). Used in \`plan.ts\` with \`handleSeerCommandError()\`. Also extract validation helpers like \`rejectInvalidDataset()\`. - -* **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 . + +* **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. + + +* **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\`. (2) \`Bun.mmap()\` always opens with PROT\_WRITE — macOS SIGKILL on signed Mach-O, Linux ETXTBSY. Fix: \`new Uint8Array(await Bun.file(path).arrayBuffer())\`. (3) Wrap \`Bun.which()\` with optional \`pathEnv\` param for deterministic testing. (4) Command func tests: \`const func = await cmd.loader()\`, then \`func.call(mockContext, flags, ...args)\`. File naming: \`\*.func.test.ts\`. - -* **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 queryWidgetTable must guard sort param by dataset like queryWidgetTimeseries**: The Sentry events API \`sort\` parameter is only supported on the \`spans\` dataset. Passing \`sort\` to \`errors\` or \`discover\` datasets returns 400 Bad Request. In \`src/lib/api/dashboards.ts\`, \`queryWidgetTimeseries\` correctly guards this (line 387: \`if (dataset === 'spans')\`), but \`queryWidgetTable\` must also apply the same guard. Without it, any table/big\_number widget with \`orderby\` set on a non-spans dataset triggers a 400 that gets caught and silently displayed as a widget error. The fix: \`sort: dataset === 'spans' ? query?.orderby || undefined : undefined\`. * **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 . - -* **Skill eval judge needs explicit CLI context about auto-detect and flags**: The skill eval judge (Haiku) reads the compact Command Reference literally. Without explicit context, it rejects valid plans because: (1) it treats \`\\` positional args as mandatory when they're optional via auto-detection, and (2) it rejects standard flags (\`--json\`, \`--query\`, \`--limit\`, \`--fields\`, \`--period\`) not shown in the compact reference. Fix: the judge prompt in \`test/skill-eval/helpers/judge.ts\` must include notes that positional args like \`\\` are optional (CLI auto-detects from DSN/config), and that commands support additional flags documented in separate reference files. Also guard against empty \`commandReference\` extraction in \`script/eval-skill.ts\` — log a warning instead of silently producing an adversarial prompt where no commands can satisfy the judge. - * **spansIndexed is not a valid Sentry dataset — use spans**: The Sentry Events/Explore API accepts 5 dataset values: \`spans\`, \`transactions\`, \`logs\`, \`errors\`, \`discover\`. The name \`spansIndexed\` is invalid and returns a generic HTTP 500 "Internal error" with no helpful validation message. This trips up AI agents and users. Valid datasets are documented in \`src/lib/api/datasets.ts\` (\`EVENTS\_API\_DATASETS\` constant) and in \`docs/commands/api.md\`. - -* **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. - - -* **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 -* **--fields dual role: output filtering + API field selection for span list**: In \`span list\`, \`--fields\` serves dual purpose: filters JSON output AND requests extra API fields. \`extractExtraApiFields()\` in \`span/list.ts\` checks each field name against \`OUTPUT\_TO\_API\_FIELD\` mapping (e.g., \`span\_id\`→\`id\`, \`op\`→\`span.op\`). Unknown names are treated as custom attributes and added to the \`field\` param in the events API request. \`FIELD\_GROUP\_ALIASES\` supports shorthand expansion (e.g., \`gen\_ai\` → 4 gen\_ai.\* fields). Extra fields survive Zod via \`SpanListItemSchema.passthrough()\` and are forwarded by \`spanListItemToFlatSpan(item, extraFieldNames)\` onto \`FlatSpan\`'s index signature. \`formatSpanTable()\` dynamically adds columns for extra attributes. +* **--fields dual role: output filtering + API field selection for span list**: --fields dual role in span list: filters JSON output AND requests extra API fields. \`extractExtraApiFields()\` checks names against \`OUTPUT\_TO\_API\_FIELD\` mapping. Unknown names are treated as custom attributes added to the \`field\` API param. \`FIELD\_GROUP\_ALIASES\` supports shorthand expansion (e.g., \`gen\_ai\` → 4 fields). Extra fields survive Zod via \`SpanListItemSchema.passthrough()\` and are forwarded by \`spanListItemToFlatSpan()\`. \`formatSpanTable()\` dynamically adds columns. * **--since is an alias for --period via shared PERIOD\_ALIASES**: \`PERIOD\_ALIASES\` in \`src/lib/list-command.ts\` maps both \`t\` and \`since\` to \`period\`. All commands using \`LIST\_PERIOD\_FLAG\` get \`--since\` as an alias for \`--period\` automatically via spread \`...PERIOD\_ALIASES\`. This was added because AI agents and humans naturally try \`--since 1h\` instead of \`--period 1h\`. @@ -1077,17 +1071,8 @@ mock.module("./some-module", () => ({ * **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. - -* **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. - - -* **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\`. - - -* **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. - - -* **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. + +* **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. * **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"\`. @@ -1096,17 +1081,11 @@ mock.module("./some-module", () => ({ * **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. +* **Redact sensitive flags in raw argv before sending to telemetry**: Redact sensitive flags in raw argv before sending to telemetry: \`withTelemetry\` calls \`initTelemetryContext()\` BEFORE the callback — sets user ID, email, instance ID, runtime, is\_self\_hosted tags. For org context, reads \`getDefaultOrganization()\` from SQLite (no API call). Redact \`SENSITIVE\_FLAGS\` (currently \`token\`) — scan for \`--token\`/\`-token\`, replace value with \`\[REDACTED]\`. Handle both \`--flag value\` and \`--flag=value\` forms. + + +* **Set Sentry context for ApiError before captureException for structured diagnostics**: When \`Sentry.captureException(exc)\` is called for an \`ApiError\`, the SDK only captures \`name\`, \`message\`, and \`stacktrace\` — custom properties like \`status\`, \`endpoint\`, and \`detail\` are lost. Always call \`Sentry.setContext('api\_error', { status, endpoint, detail })\` before \`captureException\` so these fields appear as structured context in the Sentry event. This was added in \`exceptionWhileRunningCommand\` in \`app.ts\`. Without it, events show only the generic message 'API request failed: 400 Bad Request' with no way to identify which endpoint failed. * **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. - - -* **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. - - -* **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. - - -* **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/app.ts b/src/app.ts index c17326d03..ad039c27f 100644 --- a/src/app.ts +++ b/src/app.ts @@ -45,6 +45,7 @@ import { } from "./lib/command-suggestions.js"; import { CLI_VERSION } from "./lib/constants.js"; import { + ApiError, AuthError, CliError, getExitCode, @@ -322,6 +323,14 @@ const customText: ApplicationText = { // Report command errors to Sentry. Stricli catches exceptions and doesn't // re-throw, so we must capture here to get visibility into command failures. + // 400 Bad Request = CLI bug (we constructed a malformed request). + if (exc instanceof ApiError) { + Sentry.setContext("api_error", { + status: exc.status, + endpoint: exc.endpoint, + detail: exc.detail, + }); + } Sentry.captureException(exc); if (exc instanceof CliError) { diff --git a/src/lib/api/dashboards.ts b/src/lib/api/dashboards.ts index f75d19263..4302d97a5 100644 --- a/src/lib/api/dashboards.ts +++ b/src/lib/api/dashboards.ts @@ -444,7 +444,9 @@ async function queryWidgetTable( statsPeriod, start, end, - sort: query?.orderby || undefined, + // sort is only supported on the spans dataset — + // errors/discover endpoints reject it with 400. + sort: dataset === "spans" ? query?.orderby || undefined : undefined, per_page: widget.limit ?? 10, environment: options.environment, project: options.project?.map(String), From b9e498e3a76eea10f1d9169cbf262def6a2178a2 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Sat, 11 Apr 2026 00:12:35 +0000 Subject: [PATCH 2/2] fix(telemetry): record HTTP response status on authenticated fetch spans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The authenticatedFetch wrapper returned Response objects without throwing on 4xx, so withTracing always marked HTTP spans as "ok" regardless of the actual HTTP status. This made traces misleading — a 400 Bad Request showed span.status: "ok" because the span status was set before apiRequestToRegion checked response.ok. Switch from withHttpSpan to withTracingSpan in createAuthenticatedFetch to set http.response.status_code as a span attribute and mark non-ok responses with error status. OAuth callers of withHttpSpan are unaffected since they already throw inside the callback on non-ok responses. --- src/lib/sentry-client.ts | 56 +++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/src/lib/sentry-client.ts b/src/lib/sentry-client.ts index fcd826a16..7a748b717 100644 --- a/src/lib/sentry-client.ts +++ b/src/lib/sentry-client.ts @@ -17,7 +17,7 @@ import { import { getAuthToken, refreshToken } from "./db/auth.js"; import { logger } from "./logger.js"; import { getCachedResponse, storeCachedResponse } from "./response-cache.js"; -import { withHttpSpan } from "./telemetry.js"; +import { withTracingSpan } from "./telemetry.js"; const log = logger.withTag("http"); @@ -361,30 +361,40 @@ function createAuthenticatedFetch(): ( init?.method ?? (input instanceof Request ? input.method : "GET"); const urlPath = extractUrlPath(input); - return withHttpSpan(method, urlPath, async () => { - const fullUrl = extractFullUrl(input); - const startTime = performance.now(); - - // Check cache before auth/retry for GET requests. - // Uses current token (no refresh) so lookups are fast but Vary-correct. - const cached = await tryCacheHit( - method, - fullUrl, - authHeaders(getAuthToken()) - ); - if (cached) { + return withTracingSpan( + `${method} ${urlPath}`, + "http.client", + async (span) => { + const fullUrl = extractFullUrl(input); + const startTime = performance.now(); + + // Check cache before auth/retry for GET requests. + // Uses current token (no refresh) so lookups are fast but Vary-correct. + const cached = await tryCacheHit( + method, + fullUrl, + authHeaders(getAuthToken()) + ); + if (cached) { + span.setAttribute("http.response.status_code", cached.status); + log.debug( + `${method} ${urlPath} → ${cached.status} (cache hit, ${(performance.now() - startTime).toFixed(0)}ms)` + ); + return cached; + } + + const response = await fetchWithRetry(input, init, method, fullUrl); + span.setAttribute("http.response.status_code", response.status); + if (!response.ok) { + span.setStatus({ code: 2, message: `${response.status}` }); + } log.debug( - `${method} ${urlPath} → ${cached.status} (cache hit, ${(performance.now() - startTime).toFixed(0)}ms)` + `${method} ${urlPath} → ${response.status} (${(performance.now() - startTime).toFixed(0)}ms)` ); - return cached; - } - - const response = await fetchWithRetry(input, init, method, fullUrl); - log.debug( - `${method} ${urlPath} → ${response.status} (${(performance.now() - startTime).toFixed(0)}ms)` - ); - return response; - }); + return response; + }, + { "http.request.method": method, "url.path": urlPath } + ); }; }