From 2d5b7924f041badea1f1856d0470cb2d538fb39f Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Sat, 27 Jun 2026 13:35:02 +0000 Subject: [PATCH 1/2] fix(errors): surface user search-query 400s as ValidationError, keep CLI-built 400s reported (CLI-FA) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Silencing every "Error parsing search query" 400 in the error classifiers papered over a real bug class: a 400 means the server rejected our request as malformed, and that has two distinct causes only distinguishable at the command boundary (where flags.query is in scope): - The user typed an invalid --query — a user input mistake. - The CLI built an invalid query itself — a genuine CLI defect. The blanket silence hid both. Now: - toSearchQueryError() converts a parse-400 into a clean, actionable ValidationError ONLY when the user supplied --query (issue/explore/trace list). With no user --query, the original ApiError(400) propagates and stays reported to Sentry as the bug it is. - Removed the isSearchQueryParseError special-case from all three classifiers (classifySilenced, isUserError, isUserApiError); a raw 400 is now always a reported CLI bug. --- .lore.md | 208 +++++++++++++++++++++---------- src/commands/explore.ts | 10 +- src/commands/issue/list.ts | 16 +++ src/commands/trace/list.ts | 4 + src/lib/error-reporting.ts | 15 +-- src/lib/errors.ts | 67 +++++++++- src/lib/telemetry.ts | 14 +-- test/commands/issue/list.test.ts | 74 +++++++++++ test/lib/error-reporting.test.ts | 28 ++--- test/lib/errors.test.ts | 54 +++++++- test/lib/telemetry.test.ts | 7 +- 11 files changed, 380 insertions(+), 117 deletions(-) diff --git a/.lore.md b/.lore.md index f87035bfe..72ec7def6 100644 --- a/.lore.md +++ b/.lore.md @@ -7,12 +7,6 @@ * **@sentry/symbolic 13.4.0 API surface: SourceBundleWriter for bundle-sources command**: \`@sentry/symbolic@13.4.0\` exports 4 classes: \`Archive\`, \`FileEntry\`, \`ObjectFile\`, \`SourceBundleWriter\`, plus \`SourceFileDescriptor\`. Key for CLI source-tier commands: \`SourceBundleWriter.writeObject(object: ObjectFile, object\_name: string, filter: Function, provider: Function): Uint8Array | undefined\` — callback-based; provider reads source content by path, filter selects files. \`bundle-sources\` is directly implementable (provider reads from disk). \`print-sources\` is BLOCKED — \`ObjectFile\` has no \`sourceFiles()\` enumeration method in 13.4.0 (only props: arch, codeId, debugId, fileFormat, hasDebugInfo, hasSources, hasSymbols, hasUnwindInfo, kind). \`SourceFileDescriptor\` has get/set props: contents, debugId, path, sourceMappingUrl, url, type. Confirmed by Dav1dde (Sebastian Zivota's colleague) on Jun 23 2026. - -* **@sentry/symbolic wasm-debug-session API: DebugSession via SelfCell + AsSelf**: \`@sentry/symbolic\` wasm-debug-session API: \`ObjectFile.debugSession()\` → \`DebugSession\` wrapping \`SelfCell\, ObjectDebugSession<'static>>\` via \`derived\_from\_cell!\` macro. Required \`impl AsSelf\` for \`ObjectDebugSession\` in \`symbolic-debuginfo/src/object.rs\`. \`files()\` returns eager \`FileEntry\[]\`; \`sourceByPath(path)\` returns \`SourceFileDescriptor | undefined\`. Note: Rust↔JS String encoding mismatch may cause excessive wasm↔JS data copying when reading \`descriptor.contents\`. wasm-bindgen instances (\`Archive\`, \`ObjectFile\`) are auto-freed via \`FinalizationRegistry\` — no explicit \`.free()\` needed for one-shot CLI usage. - - -* **403/401 enrichment pipeline in infrastructure.ts — centralized, no interactive auto-fix**: \`src/lib/api/infrastructure.ts\` centralizes HTTP error enrichment. \`enrichDetail()\` dispatches: 403→\`enrich403Detail\`, 401→\`enrich401Detail\`, others pass raw. \`enrich403Detail()\` three branches: (1) \`rawDetail.includes('disabled this feature')\` → org-policy message; (2) \`isEnvTokenActive()\` → \`extractRequiredScopes(rawDetail)\`; scopes found → definite missing-scope message; no scopes → hedged message; (3) OAuth → re-auth suggestion. \`throwApiError()\` and \`throwRawApiError()\` both set \`ApiError.enriched403=true\`. No interactive auto-fix. \`buildPermissionError()\` in \`project/delete.ts\` NEVER suggests \`sentry auth login\` — re-auth via OAuth won't change permissions. OAuth \`auth login\` always grants required scopes, so scope hints only apply to env-var tokens. 401 errors: fix is always re-authenticate — scope hints do NOT apply. - * **Auth token env var override pattern: SENTRY\_AUTH\_TOKEN > SENTRY\_TOKEN > SQLite**: Auth token precedence in \`src/lib/db/auth.ts\`: \`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. \`runInteractiveLogin\` catches OAuth flow errors internally and returns falsy on failure; login command sets \`process.exitCode = 1\` and returns normally (does NOT reject). Tests expecting \`rejects.toThrow()\` will fail — assert via fetch-call inspection instead. \`requestDeviceCode\` requires \`SENTRY\_CLIENT\_ID\` env var. @@ -49,9 +43,6 @@ * **delta-upgrade.ts: patch chain resolution and application architecture**: Two channels: stable (GitHub Releases) and nightly (GHCR \`patch-\\` tags). Patch format: TRDIFF10 (zig-bsdiff + zstd). Constants: \`MAX\_STABLE\_CHAIN\_DEPTH=10\`, \`MAX\_NIGHTLY\_CHAIN\_DEPTH=30\`, \`SIZE\_THRESHOLD\_RATIO=0.6\`. Stable: single API call fetches releases with asset metadata, parallel \`Promise.all\` download. Nightly: list tags → filter semver range → fetch manifests → parallel blob download. \`applyPatchesSequentially()\` alternates between two intermediate files (\`${destPath}.patching.a\`/\`.b\`) — never read/write same path (mmap corruption). SHA-256 verified ONCE after all patches applied, not per-intermediate. Cache-first: \`tryLoadCachedChain()\` with key \`patch-chain:{from}-{to}\`. \`canAttemptDelta()\` blocks on dev version, cross-channel, or downgrade. - -* **generate-command-docs.ts: fragment system, markers, and gitignored output**: \`script/generate-command-docs.ts\` (463 lines): bootstraps stub \`src/generated/skill-content.ts\` if missing, then dynamically imports \`src/app.js\`, \`src/lib/introspect.js\`, \`src/lib/env-registry.js\`. Generates per-route pages to \`docs/src/content/docs/commands/{name}.md\` (gitignored). Each page = auto-generated content + \`\\` marker + optional fragment from \`docs/src/fragments/commands/{name}.md\` (committed). \`GLOBAL\_FLAG\_NAMES = \["json","fields","help","helpAll","log-level"]\` excluded from per-command docs. \`SKIP\_ROUTES = \["help"]\`. Also generates \`index.md\` (commands table) and \`configuration.md\` (env vars + \`docs/src/fragments/configuration.md\`). Cleans up legacy \`docs/src/content/docs/commands/cli/\` subdirectory on each run. - * **generate-docs-sections.ts: in-place marker injection into committed files**: \`script/generate-docs-sections.ts\` (555+ lines): injects auto-generated content into committed files between named marker pairs. Marker styles: HTML \`\\` (\`.md\`); MDX \`{/\* GENERATED:START name \*/}\` (\`.mdx\`). \`--check\` flag: dry-run, exits 1 if stale. 13 sections across 5 files: \`contributing.md\` (project-structure, dev-prereq, build-commands), \`DEVELOPMENT.md\` (oauth-scopes, dev-env-vars, dev-prereq, build-toolchain), \`self-hosted.md\` (oauth-scopes, self-hosted-env-vars), \`README.md\` (dev-prereq, library-prereq, dev-scripts), \`getting-started.mdx\` (platform-support). Version extractors (\`extractPnpmVersion\`, \`extractNodeVersion\`) \*\*throw on mismatch\*\* — no silent fallbacks. No Bun references remain. CI \`check-generated\` job runs with \`--check\` flag. @@ -67,9 +58,6 @@ * **getsentry/symbolic WASM architecture: zstd via C zstd-sys wasm-shim, self\_cell ownership**: WASM build uses C zstd via \`zstd-sys\` for ALL targets including \`wasm32-unknown-unknown\`. \`zstd-sys\` ships \`wasm-shim/\` with C headers; \`build.rs\` auto-enables shim for wasm32. CI \`wasm-build\` job installs \`clang lld llvm\`. ruzstd dropped (significantly slower per crate author + Sebastian Zivota). Ownership model: \`self\_cell\`-based (\`SelfCell\, di::Archive<'static>>\`). \`ObjectFile\` rename fix: \`#\[wasm\_bindgen(js\_name = "ObjectFile")]\`. Canonical field names use \`.name()\` method (lowercase: \`elf\`, \`x86\_64\`) not \`{:?}\` Debug formatting. Smoke tests in \`symbolic-wasm/npm/\` so \`npm test\` works from npm dir. PR sequencing: symbolic PRs (#988, #992) must merge + republish before CLI \`bundle-sources\` PR. - -* **Host-scoped token model: auth.host column + three-layer enforcement**: Host-scoped token model (schema v16): every token bound to issuing host via \`auth.host\` column, lazy-migrated from boot-env. Trust established ONLY via \`sentry auth login --url\` or shell-exported \`SENTRY\_HOST\`/\`SENTRY\_URL\` at boot — \`.sentryclirc\` URL never a trust source. Three enforcement layers: (1) \`applySentryUrlContext\` throws on URL-arg mismatch; (2) \`applySentryCliRcEnvShim\` throws on rc-url mismatch (auth login/logout bypass via \`skipUrlTrustCheck\`); (3) fetch-layer \`isRequestOriginTrusted\`. Region trust: in-process Set in \`db/regions.ts\`, auto-synced by \`setOrgRegion(s)\`. \`clearTrustedHostState\` must NOT clear login anchor (breaks IAP re-auth). \`HostScopeError\` has overloads \`(message)\` and \`(source, destinationUrl, tokenHost)\`. Test helpers: \`resetHostScopingState()\` bundles \`resetEnvTokenHostForTesting\` + \`resetLoginTrustAnchorForTesting\` + \`resetTrustedRegionUrlsForTesting\`. E2E: pass \`--url ${ctx.serverUrl}\` to \`auth login --token\`; \`SENTRY\_URL\` alone doesn't anchor. Multi-region tests need \`registerTrustedRegionUrls\`. - * **InkUI teardown order — 6 steps, all try/catch, torndown guard prevents double-unmount**: \`InkUI.tearDown()\` must follow this order: (1) stop tip-rotation interval; (2) detach SIGINT listener + \`store.setRequestCancel(undefined)\`; (3) \`instance.clear()\`; (4) \`instance.unmount()\`; (5) restore alternate screen \`\x1b\[?1049l\`; (6) \`freshStdin.setRawMode(false)\` + \`.pause()\` + \`.destroy()\`. \`torndown: boolean\` guard prevents double-unmount (throws on some platforms). \`cancelRequested\` guard: second Ctrl+C → \`process.exit(130)\`. Every step wrapped in try/catch. @@ -82,6 +70,9 @@ * **Issue list sort values: SortValue type, VALID\_SORT\_VALUES, getComparator, and SDK IssueSort**: In \`src/commands/issue/list.ts\`: \`SortValue\` type (line 141, @internal) and \`VALID\_SORT\_VALUES\` array (line 143) are the CLI's local sort constraints. \`IssueSort\` in \`src/lib/api/issues.ts\` (lines 40–42) is derived from \`@sentry/api\` SDK via \`NonNullable\\['sort']>\` — no API layer change needed when adding new sort values. SDK already includes \`'date' | 'freq' | 'inbox' | 'new' | 'recommended' | 'trends' | 'user'\`. To add a new sort value: (1) add to \`SortValue\` union and \`VALID\_SORT\_VALUES\`, (2) add case to \`getComparator()\` switch (default falls back to date), (3) update flag \`brief\` string, (4) change \`default\` if needed. \`appendIssueFlags()\` guards \`--sort\` omission only when \`flags.sort !== 'date'\` — update guard if default changes. + +* **maxZipTotalSize: 2GiB memory-safety budget separate from server maxFileSize policy**: \`PrepareDifsOptions.maxZipTotalSize\` (default \`DEFAULT\_MAX\_ZIP\_TOTAL\_SIZE\` = 2GiB) is a cumulative uncompressed-extraction budget per \`.zip\` archive and a container size cap — it bounds peak decompression memory. It is distinct from \`maxFileSize\` (per-entry server upload policy). Commands do not need to pass \`maxZipTotalSize\` explicitly; \`prepareDifs\` applies the 2GiB default automatically. \`0\` disables the budget. Passed through \`prepareZipDifs\` → \`readZipDifEntries\` as \`maxTotalSize\`. + * **Node SEA ink sidecar: node:sea.getAsset() replaces Bun /$bunfs/ virtual FS**: (architecture) Node SEA ink sidecar: \`node:sea.getAsset()\` replaces Bun \`/$bunfs/\` virtual FS. Ink UI sidecar embedded via \`fossilize --assets dist-build/ink-app.js\`; asset key = raw CLI arg. At runtime: \`sea.getRawAsset('dist-build/ink-app.js')\`. Main bundle never calls \`import('ink')\` — sidecar pre-bundled by text-import-plugin. Dual-mode: detect SEA via \`createRequire(import.meta.url)('node:sea')\` with try/catch fallback. \`useSnapshot: true\` BROKEN. \`useCodeCache: true\` ~15% startup improvement but platform-specific V8 blob. Suppress \`ExperimentalWarning: SQLite\`: \`process.on('warning', ...)\` at very top of \`src/bin.ts\` BEFORE any imports. fossilize asset manifest key = \`basename(manifestPath)\`; entry keys = \`entry.file\`. \`new Worker(new URL(...))\` HANGS in SEA — use Blob+URL.createObjectURL. @@ -92,7 +83,7 @@ * **Sentry CLI resolve-target cascade has 5 priority levels with env var support**: Resolve-target cascade: (1) CLI flags, (2) SENTRY\_ORG/SENTRY\_PROJECT env vars, (3) SQLite defaults, (4) DSN auto-detection, (5) directory name inference. SENTRY\_PROJECT supports \`org/project\` combo — SENTRY\_ORG ignored if set. Schema v13 merged \`defaults\` table into \`metadata\` KV with keys \`defaults.{org,project,telemetry,url}\`; getters/setters in \`src/lib/db/defaults.ts\`. Prefer dedicated SQLite tables + migrations over \`metadata\` KV for non-trivial caches. Hidden global \`--org\`/\`--project\` flags: \`mergeGlobalFlags()\` in command.ts injects hidden flag shapes, \`applyOrgProjectFlags()\` writes to \`SENTRY\_ORG\`/\`SENTRY\_PROJECT\` before auth guard. No short aliases (\`-p\` conflicts). \`@sentry/api\` SDK: wrap types at \`src/lib/api/\*.ts\` with \`as unknown as SentryX\` casts; never leak to commands. \`unwrapResult\`/\`unwrapPaginatedResult\` must stay CLI-owned. \`apiRequestToRegion\` auto-sets JSON Content-Type; \`rawApiRequest\` preserves strings. -* **sentry-cli skill install paths: ~/.claude/ and ~/.agents/ only — OpenCode is never a target**: Skill source-of-truth: \`plugins/sentry-cli/skills/sentry-cli/SKILL.md\` (602 lines) + \`references/\` (28 per-command .md files). \`installAgentSkills()\` in \`src/lib/agent-skills.ts\` installs to \`~/.agents/skills/sentry-cli/\` and \`~/.claude/skills/sentry-cli/\` only — no OpenCode path. OpenCode IS detected via \`OPENCODE\_CLIENT\` env in \`src/lib/detect-agent.ts\` but only for telemetry, not skill installation. \`.opencode/\` and \`opencode.json\*\` are gitignored (lines 72–74). Cursor symlinks live at \`.cursor/skills/sentry-cli/\` pointing into \`plugins/\`. OpenCode scans \`~/.claude/skills/\*\*/SKILL.md\` and \`~/.agents/\*\*/SKILL.md\` — \`.cursor/\` is NOT scanned. \`installAgentSkills()\` never creates top-level agent roots — their presence is the detection signal that the user already has a compatible agent installed. Skills are updated on every version bump (write-if-changed optimization is unnecessary). Writes use atomic rename: temp file \`.\.\.\.tmp\` in same dir → \`rename()\` into place, guaranteeing readers never observe a partial file. +* **sentry-cli skill install paths: ~/.claude/ and ~/.agents/ only — OpenCode is never a target**: Skill source-of-truth: \`plugins/sentry-cli/skills/sentry-cli/SKILL.md\` (602 lines) + \`references/\` (28 per-command .md files). \`installAgentSkills()\` in \`src/lib/agent-skills.ts\` installs to \`~/.agents/skills/sentry-cli/\` and \`~/.claude/skills/sentry-cli/\` only — no OpenCode path. OpenCode IS detected via \`OPENCODE\_CLIENT\` env in \`src/lib/detect-agent.ts\` but only for telemetry, not skill installation. \`.opencode/\` and \`opencode.json\*\` are gitignored. Cursor symlinks live at \`.cursor/skills/sentry-cli/\` pointing into \`plugins/\`. \`installAgentSkills()\` never creates top-level agent roots — their presence is the detection signal. Skills are updated on every version bump. Writes use atomic rename: temp file \`.\.\.\.tmp\` in same dir → \`rename()\` into place. Temp file dot-prefix ensures it never matches the \`SKILL.md\` glob agents scan. \`SKILL\_FILES\` is a \`ReadonlyMap\\` with 28 entries; \`SKILL.md\` is written last (entry 28) — favorable ordering for discovery race. Windows \`rename()\` raises EPERM/EBUSY if dest is open — atomicity guarantee is POSIX-only. * **src/cli.ts: middleware chain, completion optimization, sensitive argv redaction**: \`src/cli.ts\` exports \`startCli()\`, \`runCli()\`, \`runCompletion()\`. Middleware chain (innermost-first): \`\[seerTrialMiddleware, autoAuthMiddleware]\` — auth is outermost. \`autoAuthMiddleware\` uses \`isatty(0)\` not \`process.stdin.isTTY\` (Bun returns undefined). \`runCompletion()\` sets \`SENTRY\_CLI\_NO\_TELEMETRY=1\` to skip \`@sentry/node-core\` lazy-require (~280ms). \`redactArgv()\` handles \`--flag=value\` and \`--flag \\` forms; \`SENSITIVE\_ARGV\_FLAGS\` includes \`token\` and \`auth-token\`. \`reportUnknownCommand()\` wrapped in try/catch — telemetry must never crash CLI. \`preloadProjectContext()\` calls \`captureEnvTokenHost()\` BEFORE any env mutation. @@ -100,6 +91,9 @@ * **stdin-reopen.ts: forwardFreshTtyToStdin() idempotency and isTTY backfill pattern**: \`src/lib/init/stdin-reopen.ts\` exports \`forwardFreshTtyToStdin(deps?)\` returning a \`Disposable\` (\`TtyForwardingHandle\`) — always non-null so callers use \`using tty = forwardFreshTtyToStdin()\` without null-checking. Idempotency: repeated calls return \`NOOP\_HANDLE\` (secondary callers don't tear down primary's install). isTTY backfill: captures \`previousIsTty\` before touching; if \`undefined\`, uses \`Object.defineProperty\` to set \`isTTY: true, writable: true, configurable: true\` — required because Ink/clack gates \`setRawMode(true)\` on \`input.isTTY\`, so without backfill the fresh fd stays in canonical mode. \`pause\`/\`resume\` replaced with noops to prevent Bun kqueue EINVAL on fd-0 transitions. \`TtyDeps\` allows injection of \`openTty\` and \`isTty\` for test isolation. + +* **symbolic-il2cpp WASM binding: from\_object\_with\_provider pattern and default-features risk**: \`ObjectLineMapping::from\_object\_with\_provider\\` (bounds: \`O: ObjectLike\`, \`B: AsRef<\[u8]>\`, \`P: FnMut(\&str) -> Option\\`) added in PR #1005; \`from\_object\` delegates via \`|path| ByteView::open(path).ok()\`. WASM binding \`il2cppLineMapping(provider)\` calls JS via \`provider.call1(\&JsValue::UNDEFINED, \&JsValue::from\_str(path))\`; returns \`None\` if null/undefined; serializes via \`mapping.to\_writer\`. Risk: \`symbolic-il2cpp\` dep in \`symbolic-wasm/Cargo.toml\` uses default features — if \`symbolic-debuginfo\` defaults gain a non-wasm-friendly feature it leaks silently. Hardening: add \`default-features = false\` to the \`symbolic-il2cpp\` dep. + * **symbolic-wasm class-based API: Rc\> ownership + on-demand re-parse pattern**: symbolic-wasm ownership model: Dav1dde's PR #992 uses \`SelfCell\, di::Archive<'static>>\` — \`self\_cell\`-based ownership, no re-parse. \`derived\_from\_cell!\` macro uses \`std::mem::transmute\` + \`SelfCell::from\_raw\` to clone owner, letting \`objects()\` return owned \`Object\` cells sharing the same \`ByteView\`. PR #991 (Rc\> + re-parse) closed in favor of this. \`Object\` getters: \`debugId\`, \`codeId\`, \`arch\`, \`fileFormat\`, \`kind\`, \`hasSymbols\`, \`hasDebugInfo\`, \`hasUnwindInfo\`, \`hasSources\`. \`Archive\` methods: \`new(data)\`, \`peek(data)->Option\\`, \`fileFormat\`, \`objectCount\`, \`objects()->Result\>\`. CRITICAL: Rust struct named \`Object\` but exported as \`ObjectFile\` via \`#\[wasm\_bindgen(js\_name = "ObjectFile")]\` — see gotcha entry for why. @@ -118,7 +112,7 @@ * **Decided to use job ID instead**: decided to use job ID instead. -* **Migrated to node**: migrated from Bun to Node. +* **Migrated to node**: Migrated to Node.js (from Bun). Migration complete as of fossilize 0.7.0 update. Stack: pnpm, vitest, tsx, Node SEA via fossilize. PRs #1017, #1018, #1019 on getsentry/cli. Benchmarks vs v0.34.0 (Bun): download 32MB→30MB, startup ~1s both, shell completions 180ms→150ms. All macOS binaries signed+notarized. fossilize handles SEA builds with V8 code cache on linux+macOS, strips debug symbols automatically. \`bun.lock\` deleted, \`vitest.config.ts\` added, all test files migrated to vitest. \`script/build.ts\` uses fossilize (\`--no-bundle\`) with esbuild for bundling — does NOT use \`Bun.build({ compile: true })\`. * **Node.js slim build flags for SEA binary size reduction**: Node.js configure.py size-reduction flags for SEA builds: \`--with-intl=small-icu\` (English-only ICU, saves ~26-28 MiB — biggest win; CLI uses hardcoded en-US/sv-SE locales, safe); \`--with-intl=none\` (saves ~28-30 MiB but breaks \`Intl.NumberFormat\`/\`String.normalize()\` — NOT safe for this CLI); \`--without-inspector\` saves ~2-4 MiB; \`--without-amaro\` saves ~0.5 MiB; \`--v8-disable-maglev\` saves ~1-2 MiB; \`--enable-lto\` saves ~3-5 MiB. AVOID: \`--without-ssl\` (breaks HTTPS), \`--without-lief\` (BREAKS SEA), \`--without-sqlite\` (BREAKS CLI — uses node:sqlite), \`--disable-single-executable-application\` (BREAKS EVERYTHING), \`--v8-lite-mode\` (10x slower). Custom build deferred indefinitely — requires 5 native CI runners, ~3.5h cold build vs 5min fossilize. Cross-compilation from Linux to darwin NOT officially supported. @@ -146,6 +140,9 @@ * **acquireLock ENOENT: parent directory may not exist — must mkdir before writeFileSync**: Trap: \`acquireLock(lockPath)\` calls \`writeFileSync(lockPath, pid, { flag: 'wx' })\` — looks safe because the lock path is derived from the install path. But the parent directory (e.g. \`~/.sentry/bin\` or a stale \`/tmp/sentry-test-install/\`) may not exist, causing ENOENT (Sentry issue CLI-1E1). Fix: call \`mkdirSync(dirname(lockPath), { recursive: true, mode: 0o755 })\` BEFORE the try block in \`acquireLock\` (\`src/lib/binary.ts\`). CRITICAL: keep mkdir OUTSIDE the try block — if inside, mkdir's EEXIST error routes into \`handleExistingLock\`, obscuring root cause (comment claiming infinite recursion is inaccurate: actual path is EEXIST→handleExistingLock→readFileSync→ENOTDIR, single throw). The directory creation is idempotent — \`recursive: true\` is a no-op if it already exists. Merged in PR #1125 (squash \`ee768454c\`). + +* **Atomic-write test tautology: 'no temp files' test passes even without atomic behavior**: Trap: a test that checks 'no \`.tmp\` files left behind after install' looks like it guards atomic write behavior. But if the non-atomic path (\`writeFile\` directly) never creates \`.tmp\` files in the first place, the \`leftovers\` filter is trivially empty and the test passes with the atomic code removed entirely — confirmed via mutation test on \`byk/fix/agent-skills-atomic-write\`. Fix: the test must exercise the overwrite/update path (where a concurrent reader could observe truncation) AND inject a fault (e.g., mock \`rename\` to throw) to verify cleanup. A test that only runs the happy-path fresh-install never touches the race condition the feature guards against. Per red-green directive: if the test passes with the guard deleted, it isn't testing the guard. + * **batch-queue.ts: 404 from upstream treated as transient — provider never disabled**: Trap: \`BatchProvider.submit()\` returns \`null\` for any non-401/403 HTTP error, including 404. \`submitBatch()\` treats \`null\` as transient and falls back — no disable happens. For providers that don't implement \`/v1/messages/batches\` (e.g. MiniMax), this causes a wasted HTTP round-trip every 30s forever. Fix: add \`"not-found"\` return value for 404 in both Anthropic and OpenAI submit methods. In \`submitBatch()\`, handle \`"not-found"\` with provider-level disable: add \`disabledBatchProviders: Set\\` (keyed by provider name), persist to \`kv\_meta\` via \`setKV()\`, restore on startup. Add fast-path bypass in both \`flush()\` and \`prompt()\`. Provider-level (not per-session) because the URL is baked in at construction — one provider per process. \`groupKey()\` = \`authFingerprint(cred)|providerID\`; per-credential disable was removed in favor of per-session historically. @@ -155,6 +152,12 @@ * **Biome noParameterProperties: never use TypeScript constructor parameter properties in class definitions**: Trap: TypeScript parameter properties (\`constructor(private readonly handle: FileHandle)\`) look like idiomatic shorthand and compile fine. But Biome enforces \`noParameterProperties\` and will error. Fix: always declare explicit class fields and assign in constructor body. Applies to all new classes in \`src/lib/\*\*/\*.ts\`. Caught during \`FileOldReader\`/\`MemoryOldReader\` addition in \`bspatch.ts\` — 4 Biome errors at lines 281, 310, 311, 312. + +* **Biome stdin check reports 'contents aren't fixed' false positive — use file path instead**: Trap: running \`biome check --stdin-file-path=\\` and piping content exits with code 1 and 'contents aren't fixed' even when \`--write\` produces no diff — looks like a real formatting violation. Fix: run Biome directly on the file path (\`biome check \\`) rather than via stdin mode. The stdin mode false positive was confirmed during the \`agent-skills.ts\` review: exit code 1 from stdin, but \`--write\` produced zero changes. Always use file-path invocation for authoritative Biome results. + + +* **build-binary job in ci.yml uses setup-node — not Bun-only as commonly assumed**: Trap: \`build-binary\` job in \`ci.yml\` is assumed to use only Bun (not \`setup-node\`) because the binary build pipeline uses fossilize/esbuild. But \`ci.yml\` lines 261–263 contain an \`actions/setup-node@v6\` step inside \`build-binary\` — PR #1145 changed it from \`node-version: "22"\` to \`${{ env.NODE\_VERSION\_22 }}\`. The PR description incorrectly stated \`build-binary\` is unaffected. Fix: always grep \`ci.yml\` for \`setup-node\` rather than assuming job intent from its name. The Node install in \`build-binary\` is likely for pnpm/tooling, not the compiled artifact. + * **bundle-sources.ts exit code 1 for no-sources relies on cli.ts never resetting exitCode — fragile invariant**: Trap: using \`this.process.exitCode = 1\` directly (at \`bundle-sources.ts:145\`) instead of throwing \`OutputError\`(=60) looks like a valid shortcut. It works today because \`cli.ts:622-649\` never resets \`exitCode\` on a clean return. But this is a fragile invariant — if \`cli.ts\` ever resets \`exitCode\`, the no-sources case silently exits 0. The \`OutputError\`(=60) idiom is the correct pattern. The \`exitCode=1\` approach was kept for consistency with \`check.ts\` pattern; a comment was added explaining the choice. Consider migrating to \`OutputError\` in a future cleanup. @@ -170,18 +173,27 @@ * **dashboard revisions/restore and issue events subcommands are undocumented in fragment files**: RESOLVED in PR #1024. \`docs/src/fragments/commands/dashboard.md\` now documents \`revisions\` and \`restore\`. \`docs/src/fragments/commands/issue.md\` now documents \`events\` and \`@latest\`/\`@most\_frequent\` selectors. \`docs/src/fragments/commands/cli.md\` now documents \`defaults\` and \`import\`. \`check:fragments\` (Check 5) now validates subcommand coverage within fragment files — not just file existence. When adding new subcommands, always update the corresponding fragment in \`docs/src/fragments/commands/\` AND run \`pnpm run check:fragments\` to verify coverage. + +* **debug-files upload: partial size-drop silently exits 0 — doUpload must receive oversizedCount**: Trap: \`doNothingToUpload\` (all-dropped path) correctly calls \`setExitCode(1)\` when \`oversizedCount > 0\`, establishing the contract that oversized files → non-zero exit. But \`doUpload\` (partial-drop path) did not receive \`oversizedCount\`/\`maxFileSize\` — so uploading 3 files where 1 was oversized exited 0 silently. Fix (PR #1146): pass \`oversizedCount\` and \`maxFileSize\` to \`doUpload\`; append scan-oversize message to failures hint; call \`setExitCode(1)\` when \`oversizedCount > 0\` even with no upload failures. Warden finding 9LL-87A on PR #1140. + * **delta-upgrade intermediate files: always alternate .patching.a/.b — never write to source path**: Trap: writing patch output to the same path as input looks like a simple in-place update. Fix: \`applyPatchesSequentially()\` alternates between \`${destPath}.patching.a\` and \`${destPath}.patching.b\` because the old binary is mmap'd for reading — writing to the source path truncates it and corrupts output. Single-patch chains apply old→dest directly (safe, different paths). \`cleanupIntermediates()\` called in \`finally\` block removes both intermediates via \`unlinkSync\` (ignoring errors) — always runs even on failure. This is a standing directive: 'Always clean up intermediate files, even on failure' and 'never target the same path.' * **DEVELOPMENT.md hand-written prose is not covered by any staleness check**: RESOLVED in PR #1024. \`DEVELOPMENT.md\` hand-written prose is now wrapped in \`GENERATED:START/END\` markers: \`dev-prereq\` (lines 4-7) and \`build-toolchain\` (lines 91-97). These sections are now auto-generated from \`package.json\` by \`generate-docs-sections.ts\` and validated by \`check:docs-sections --check\`. The only remaining non-generated prose in \`DEVELOPMENT.md\` is the OAuth app setup instructions and architecture description — these don't reference toolchain versions. \`generate-docs-sections.ts\` no longer contains any Bun references; \`extractPnpmVersion()\` and \`extractNodeVersion()\` throw on mismatch. + +* **error-reporting.ts: three independent 4xx classifiers must stay in sync — isUserError, classifySilenced, isUserApiError**: Trap: \`classifySilenced\` (Sentry capture gate), \`isUserError\` in \`errors.ts\` (upgrade nudge via \`getErrorUpdateNotification\`), and \`isUserApiError\` in \`telemetry.ts\` (span attribution) all classify 4xx errors independently. Adding a new silence rule to only one leaves the others out of sync — e.g., query-parse 400s were silenced in \`classifySilenced\` but still triggered the upgrade nudge until \`isUserError\` was updated. Fix: whenever a new error classification is added (e.g., \`isSearchQueryParseError\`, \`isNetworkError\`), update all three classifiers. Export shared predicates from \`errors.ts\` so all three import the same function. + + +* **eval-skill-fork.yml has no setup-node step — floating Node version bypasses pins**: Trap: \`.github/workflows/eval-skill-fork.yml\` job \`eval\` has no \`actions/setup-node\` step at all — it runs on the runner image's system Node (e.g. 24.17.0/22.23.0), the exact versions the Node-pin PR (#1145) aimed to avoid. The PR's claim 'every setup-node step is pinned' is technically true but the intent is unmet because this job never calls setup-node. Fix: add a pinned \`actions/setup-node@v6\` step with \`node-version: ${{ env.NODE\_VERSION\_24 }}\` before \`pnpm install\` in the \`eval\` job. Discovered via subagent adversarial review of PR #1145. + + +* **eval-skill-fork.yml missing setup-node — fork PRs exposed to Node regression**: Trap: PR #1145 pinned Node versions in all 4 main workflow files and the PR description claimed completeness. But \`.github/workflows/eval-skill-fork.yml\` job \`eval\` (lines 27–57) has no \`actions/setup-node\` step — it runs on the runner's system Node. This is the exact workflow exhibiting \`ERR\_STREAM\_PREMATURE\_CLOSE\` (CVE-2026-48931 regression in Node 24.17.0/22.23.0). Fix: add \`actions/setup-node@v6\` with \`node-version: ${{ env.NODE\_VERSION\_24 }}\` before \`pnpm install\` in that job. When pinning Node versions across CI, always grep ALL workflow files for \`setup-node\` AND check for jobs that invoke Node without an explicit setup step. + * **existsSync+realpathSync TOCTOU: catch ENOENT instead**: Trap: \`if (!existsSync(p)) return resolve(p); return realpathSync(p)\` looks safe but has a TOCTOU race. Also: \`realpathSync\` inside async is inconsistent. Fix: call \`await realpath(p)\` (node:fs/promises) directly; catch \`ENOENT\` to fall back to \`resolve(p)\`; log non-ENOENT errors via \`logger.debug(msg, error)\` before falling back. When mocking in vitest, mock \`node:fs/promises\` not \`node:fs\`. RELATED: In cleanup/unlink catch blocks, only log non-ENOENT errors — \`ENOENT\` during cleanup is expected. Pattern: \`if ((error as NodeJS.ErrnoException).code !== 'ENOENT') logger.debug(msg, error)\`. Pre-existing silent \`catch { // Ignore }\` blocks must be fixed to log non-ENOENT errors. Confirmed fixed in PR #1046 (\`fix/install-binary-symlink-self-copy\`). - -* **gateway-smoke.test.ts: dynamic import('@loreai/gateway') requires dist/ to be built first**: Trap: \`packages/opencode/test/gateway-smoke.test.ts\` uses \`await import('@loreai/gateway')\` dynamically. The gateway package only exports from \`./dist/\` (no source entry point). Without a prior build step, the test fails with 'Cannot find module @loreai/gateway'. This is a pre-existing issue — the test requires \`bun run build\` in the gateway package before running. The 2 failures in \`gateway-smoke.test.ts\` are pre-existing and unrelated to application changes. - * **generate-docs-sections.ts still references Bun — extractBunVersion() silently falls back to hardcoded '1.3'**: RESOLVED in PR #1024. \`generate-docs-sections.ts\` previously had \`BUN\_VERSION\_RE\` and \`extractBunVersion()\` that silently returned hardcoded \`'1.3'\` when \`packageManager\` was \`pnpm@10.11.0\`. Fixed: replaced with \`extractPnpmVersion()\` and \`extractNodeVersion()\` that \*\*throw on mismatch\*\* instead of silently falling back. \`generateDevPrereq()\`, \`generateDevPrereqContributing()\`, \`generateLibraryPrereq()\` now reference Node.js + pnpm. \`DEVELOPMENT.md\` lines 5 and 91 are now wrapped in \`GENERATED:START/END\` markers so they can't drift again. No Bun references remain in the script. @@ -194,6 +206,9 @@ * **GitHub CI skips pull\_request workflows entirely when PR has merge conflicts**: Trap: missing CI jobs on a PR look like a workflow trigger bug or branch filter issue — easy to spend time investigating ci.yml triggers. Fix: check \`mergeable\`/\`mergeStateStatus\` first. When a PR is \`CONFLICTING\` (GitHub cannot compute the merge ref), ALL \`pull\_request\`-triggered workflows are silently skipped — only \`pull\_request\_target\` and CodeQL/external checks still run. Confirmed on sentry-cli (TypeScript) PR #1123: full Build/lint/test suite absent because \`mergeStateStatus: DIRTY\`. Resolution: rebase or resolve conflicts, then CI triggers normally. + +* **isNetworkError must NOT match ApiError(status=0) — TLS cert errors share status 0**: Trap: \`ApiError\` with \`status === 0\` looks like a network failure (no HTTP response received) and is tempting to include in \`isNetworkError()\`. But TLS certificate errors are also wrapped as \`ApiError(status=0)\` — once wrapped, \`isTlsCertError()\` cannot reliably re-detect them. Silencing all status-0 errors would suppress actionable TLS config issues. Fix: \`isNetworkError()\` matches ONLY \`error instanceof TypeError && error.message === 'fetch failed'\` (the unambiguous undici network signature). \`ApiError(status=0)\` is handled separately via an explicit \`error.status === 0\` branch in \`isUserError()\` with a comment explaining the TLS overlap. Callers that want status-0 treated as user-env check it explicitly. + * **issue list --sort recommended: client-side re-sort must be skipped for single-project results**: Trap: applying \`getComparator('recommended')\` to single-project results looks like consistent behavior. Fix: the \`isMultiProject\` guard at list.ts:1144-1148 is the ONLY client-side issue sort — it's intentionally skipped for single-project results because re-sorting would silently replace the server's recommended ranking with a \`lastSeen\` fallback. \`getComparator('recommended')\` returns \`compareDates(a.lastSeen, b.lastSeen)\` — same as \`date\` sort — so applying it to single-project results would corrupt the server's relevance ranking without any visible error. @@ -209,6 +224,9 @@ * **Local tarball paths in package.json break CI with pnpm install --frozen-lockfile**: Trap: \`file:/tmp/opencode/sentry-symbolic-new.tgz\` in dependencies looks harmless locally — \`pnpm install\` works fine. But CI runs \`pnpm install --frozen-lockfile\`, which exits 254 when the tarball path doesn't exist. The lockfile also changes the specifier from published version to path, causing diffs on every install. Fix: always keep published version specifiers (e.g. \`13.4.0\`) in package.json. Use \`npm pack\` + separate install for local testing, or pnpm overrides. + +* **login.ts blind catch: bare catch around getUserRegions() mislabels network/server failures as invalid token**: Trap: a bare \`catch {}\` around \`getUserRegions()\` in \`src/commands/auth/login.ts\` that always calls \`clearAuth()\` + throws \`AuthError('invalid')\` looks like correct token-validation error handling. But it conflates genuine 401/403 (bad token) with network errors, 5xx server errors, and parse failures — clearing a possibly-valid token and showing a misleading 'Invalid API token' message. Fix (PR #1153): extract \`handleTokenValidationError()\` helper — only clears auth and throws \`AuthError('invalid')\` for \`ApiError\` with status 401 or 403; re-throws original error for all other failures. \`AuthError('invalid')\` is now safe to silence in \`classifySilenced\` because it only fires on genuine auth rejections. + * **MastraClient has no dispose API — use AbortController for cleanup**: MastraClient has no \`close()\`/\`dispose()\` API — cleanup via \`ClientOptions.abortSignal\` (constructor) or per-prompt \`signal\`. Without explicit abort, Bun's fetch dispatcher keep-alive sockets hold the event loop alive past natural exit. Pattern in \`src/lib/init/wizard-runner.ts\`: create \`AbortController\` per \`runWizard\`, pass \`abortSignal: controller.signal\` to \`new MastraClient(...)\`, abort via \`using \_ = { \[Symbol.dispose]: () => controller.abort() }\`. Custom \`fetch\` wrapper must preserve \`init.signal\` via spread. Tests capture \`ClientOptions\` via \`spyOn(MastraClient.prototype, 'getWorkflow').mockImplementation(function() { capturedOpts.push(this.options); ... })\`. @@ -239,11 +257,14 @@ * **pnpm test triggers generate:docs + generate:sdk pre-steps — always times out in CI-like environments**: Trap: \`pnpm test\` looks like the standard way to run tests. But \`test:unit\` = \`pnpm run generate:docs && pnpm run generate:sdk && vitest run test/lib test/commands test/types --coverage\` — the doc/SDK generation pre-steps cause 120s+ timeouts. Fix: run vitest directly on specific test files: \`npx vitest run test/lib/dif test/commands/debug-files\` (or similar scoped paths). Use \`pnpm test\` only for final pre-commit validation. \`test:init-eval\` is the only script without the preamble (uses \`--testTimeout 600000\` instead). - -* **print-sources must never claim to preview bundle-sources while listing sources bundle-sources would skip**: Trap: \`print-sources\` iterates ALL objects in an archive (full inspection value), while \`bundle-sources\` only bundles the first object with debug info (\`selectBundledObject\`). Listing all objects then showing a \`bundle-sources\` hint looks like a preview of that command — but it isn't. Fix: add multi-object warning naming the exact slice that would be bundled (via \`selectBundledObject\`), add \`hasDebugInfo\` to JSON output so consumers can apply the same selection rule, and ensure footer hint distinguishes no-objects / enumeration-failure / no-sources. 🔴 Directive: never claim to preview \`bundle-sources\` while collecting sources \`bundle-sources\` would never collect. + +* **prepareZipDifs null return means 'not a zip' OR 'container skipped' — non-null (even empty) means fully handled**: Trap: \`null\` from \`prepareZipDifs\` previously meant only 'not a ZIP'. After the maxTotalSize refactor, \`null\` also means 'container skipped wholesale (too large / malformed)' — both cases should fall through to normal file handling. Fix: the contract is now 'non-null result (even empty array) means the path was a \`.zip\` and is fully handled; \`null\` means fall through.' The \`continue\` in \`prepareDifs\` fires on any non-null result, so an empty array correctly skips the file without treating it as a non-ZIP. Document this dual-null semantics in JSDoc on \`prepareZipDifs\`. - -* **process.stdin.isTTY unreliable in Bun — use isatty(0) and backfill for clack**: \`process.stdin.isTTY\` unreliable — use \`isatty(0)\` from \`node:tty\`. Bun's single-file binary can leave \`process.stdin.isTTY === undefined\` on TTY fds. \`@clack/core\` gates \`setRawMode(true)\` on \`input.isTTY\`, silently disabling raw mode. Fix: backfill \`process.stdin.isTTY = true\` when \`isatty(0)\` confirms. Debugging: \`src/lib/init/tty-diagnostics.ts\` \`dumpTtyDiagnostics(label)\` — no-op unless \`SENTRY\_INIT\_DIAGNOSTICS=1\`. + +* **prepareZipDifs oversizedCount must NOT feed exit-driving counter — format unknown pre-decompression**: Trap: zip entry oversized warnings look like they should increment \`oversizedCount\` in \`prepareDifs\`, just like on-disk file oversize does — both are 'too large' signals. But a compressed entry's format is unknown until inflated, so it can't be attributed to the requested \`--type\`. Counting it would turn an unrelated oversized asset inside a \`.zip\` into a false 'all matched files too large' failure and wrong exit code. Fix: \`prepareZipDifs\` returns \`PreparedDif\[]\` (not \`{ prepared, oversizedCount }\`); oversized zip entries warn per-entry inside \`readZipDifEntries\` only. The \`oversizedCount\` counter in \`prepareDifs\` is exclusively for format-accurate on-disk files. + + +* **print-sources must never claim to preview bundle-sources while listing sources bundle-sources would skip**: Trap: \`print-sources\` iterates ALL objects in an archive (full inspection value), while \`bundle-sources\` only bundles the first object with debug info (\`selectBundledObject\`). Listing all objects then showing a \`bundle-sources\` hint looks like a preview of that command — but it isn't. Fix: add multi-object warning naming the exact slice that would be bundled (via \`selectBundledObject\`), add \`hasDebugInfo\` to JSON output so consumers can apply the same selection rule, and ensure footer hint distinguishes no-objects / enumeration-failure / no-sources. 🔴 Directive: never claim to preview \`bundle-sources\` while collecting sources \`bundle-sources\` would never collect. Warden finding ANP-RKY (size gate after full file read) was fixed in PR #1141 via \`prepareDifs\` refactor — \`peeked.size\` from \`fd.stat()\` checked before \`readFile\`. * **ruzstd partial decompression: must validate output size explicitly**: Trap: \`ruzstd::StreamingDecoder\` (unlike \`zstd::bulk::decompress\`) silently returns a partial result when passed a too-small \`size\` — it does NOT error. Fix: read \`size + 1\` bytes into the output buffer, then assert \`decompressed.len() == size\`; return \`None\` on mismatch. This matches \`zstd::bulk::decompress\` error-on-mismatch semantics. Confirmed via test: exact(560)→Some(560)✓, toosmall(550)→None✓, toolarge(570)→None✓. @@ -257,9 +278,6 @@ * **skill-eval E2E tests fail on Anthropic API network errors — not a code regression**: Trap: \`test/e2e/skill-eval.test.ts\` failures (\`claude-sonnet-4-6 meets threshold\`, \`claude-opus-4-6 meets threshold\`) look like regressions introduced by the current PR. Root cause: these tests call \`api.anthropic.com\` directly — \`\[planner] API error: Invalid response body ... Premature close\` is an external Anthropic API outage, not a code bug. All 126 non-LLM E2E tests pass. Fix: confirm by checking logs for \`Premature close\` pattern; if present, stop re-running (wastes CI resources) and post a PR comment documenting the outage. Do not merge while CI is red — wait for API recovery. - -* **Source bundles use SYSB magic, not PK — never confused with ZIP archives**: Trap: source bundles (\`.src.zip\`, JVM bundles) have a \`.zip\` extension and contain a ZIP archive internally — looks like they'd be expanded by ZIP scanning. Fix: symbolic source bundles prepend an 8-byte \`SourceBundleHeader\` with magic \`SYSB\` (not \`PK\x03\x04\`). \`hasZipMagic\` checks bytes 0–1 for \`PK\` — returns false for source bundles, so they fall through to the DIF parser and upload as \`sourcebundle\`. Only true \`PK\`-magic ZIPs get expanded. Confirmed via \`symbolic-debuginfo/src/sourcebundle/mod.rs\`: \`BUNDLE\_MAGIC: \[u8; 4] = \*b"SYSB"\` at line 73. - * **SQLite transaction() ROLLBACK can throw, discarding original error**: (gotcha) SQLite transaction ROLLBACK error-swallowing trap: In \`src/lib/db/sqlite.ts\`, \`transaction()\` catches errors and runs \`this.db.exec('ROLLBACK')\`. If ROLLBACK itself throws, the original error is lost. Fix: \`const origErr = e; try { this.db.exec('ROLLBACK'); } catch (rbErr) { log.debug(...); } throw origErr;\` @@ -269,6 +287,9 @@ * **strip fails on Node SEA binaries — must strip BEFORE fossilize injection**: Strip debug symbols must happen BEFORE fossilize SEA injection. Trap: \`strip --strip-unneeded\` on a plain Node binary saves ~17 MiB and still runs — looks like it should work on the final SEA binary too. But after postject injects the SEA blob, \`strip\` fails: 'section .text can't be allocated in segment 2'. Fix: as of fossilize 0.7.0, stripping is built into fossilize itself — it strips the copied binary (already unsigned for macOS/Windows) BEFORE calling postject. Cross-strip from Linux to macOS silently fails (caught); native macOS runners strip correctly with \`strip -x\`. Windows skipped (no debug symbols). \`stripCachedNodeBinaries()\` was removed from \`script/build.ts\` in fossilize 0.7.0 update — fossilize handles it natively. + +* **symbolic-wasm utils.rs: stale edit anchoring silently drops enum variants — always verify full enum after edits**: Trap: when editing \`symbolic-wasm/src/utils.rs\` Error enum, a string-anchored edit that starts mid-enum (e.g., at \`SourceBundle\` variant) silently drops variants above the anchor (e.g., \`ObjectError\`). This looks correct because the file compiles — but thiserror stops generating \`From\\` impl, causing 8 \`E0277\` errors in \`mod.rs\`. Fix: after any edit to the \`Error\` enum, immediately verify all variants are present (\`ObjectError\`, \`SourceBundleError\`, \`Io\`) before running cargo check. The coherence-conflict hypothesis (thiserror + io::Error blanket) was a red herring — root cause was the dropped variant. + * **symbolic-wasm: JS callback errors silently swallowed via .ok()? — must propagate as JsError**: Trap: using \`.ok()?\` on \`call1\` (JS function invocation) and \`dyn\_into::\()\` looks like idiomatic Rust error-to-Option conversion. Fix: both failures must propagate as \`JsError\` to the caller — thrown JS exceptions yield partial bundles with no feedback; non-\`Uint8Array\` returns (ArrayBuffer, plain arrays) silently skip files. Pattern: capture callback error in \`Option\\` outside closure, set it on failure, check after closure returns. Make \`with\_object\` generic over \`E: From\\` so both error paths unify. Flagged by Cursor Bugbot (Medium) and Sentry Seer (Medium) on PR #991. @@ -293,6 +314,9 @@ * **Whole-buffer matchAll slower than split+test when aggregated over many files**: Grep/scan traps in \`src/lib/scan/\`: (1) Whole-buffer \`regex.exec\` 12× faster per-file but ~1.6× SLOWER over 10k files — early-exit at \`maxResults\` via \`mapFilesConcurrent.onResult\` wins. (2) Literal prefilter is FILE-LEVEL gate (\`indexOf\`→skip); per-line verify breaks cross-newline patterns and Unicode length-changing \`toLowerCase\`. (3) Extractor \`hasTopLevelAlternation\`+\`skipGroup\` must call \`skipCharacterClass\`. (4) Wake-latch race: use latched \`pendingWake\` flag, not \`let notify=null; await new Promise(r=>notify=r)\`. (5) \`mapFilesConcurrent\` filters \`null\` but NOT \`\[]\` — return \`null\` for no-op files. (6) \`collectGlob\`/\`collectGrep\` must NOT forward \`maxResults\` to iterator; drain uncapped, set \`truncated=true\`. Worker pool: lazy singleton, size \`min(8, max(2, availableParallelism()))\`. Matches encoded as \`Uint32Array\` quads transferred via \`postMessage\` (~40% faster). \`new Worker(new URL(...))\` HANGS in SEA binaries — use Blob+URL.createObjectURL. FIFO \`pending\` queue per worker. \`ref()\`/\`unref()\` idempotent — only unref when \`inflight\` drops to 0. Disable via \`SENTRY\_SCAN\_DISABLE\_WORKERS=1\`. + +* **Windows rename() raises EPERM/EBUSY on open destination — atomic write is POSIX-only**: Trap: \`rename()\` for atomic file swap looks cross-platform because Node.js exposes it on all OSes. But on Windows, if the destination file is open by a concurrent reader, \`rename()\` raises EPERM or EBUSY — the swap fails rather than being atomic. The JSDoc claim 'eliminates the truncation race' is unqualified but only holds on POSIX. \`win32-x64\` is a shipped sentry-cli target. Impact is graceful (write returns null, captureException fires) but the atomicity guarantee must be documented as POSIX-only. Fix: qualify JSDoc with 'on POSIX systems'; on Windows the fallback is non-atomic \`writeFile\` (same as before the patch). + * **worktree node\_modules symlink causes esbuild host/binary version mismatch**: Trap: when running multiple agent worktrees in parallel, \`node\_modules\` may be a symlink to a sibling worktree's install — looks fine until esbuild runs. Error: 'Host version X does not match binary version Y' crashes all generate scripts and typecheck. Fix: remove the symlink and run \`pnpm install\` in the worktree root to get an independent \`node\_modules\`. The symlink is created by mutation-test workflows that intentionally share \`node\_modules\` for speed — but only safe when both worktrees are on the same branch/lockfile. @@ -302,14 +326,11 @@ ### Pattern -* **@sentry/symbolic 13.4.0 API: Archive/ObjectFile class contract and WASM memory management**: Exported classes: \`Archive\` (\`objects()\`, \`peek()\`, \`fileFormat\`, \`objectCount\`), \`ObjectFile\` (\`debugId\`, \`codeId\`, \`arch\`, \`fileFormat\`, \`hasDebugInfo\`, \`hasSources\`, \`hasSymbols\`, \`hasUnwindInfo\`, \`kind\`, \`debugSession()\`), \`DebugSession\` (\`files()\`, \`sourceByPath()\`), \`SourceBundleWriter\` (\`writeObject()\`, \`collectIl2cppSources\` setter, \`isEmpty\`), \`SourceFileDescriptor\` (\`type\`, \`contents\`, \`url\`, \`path\`, \`sourceMappingUrl\`, \`debugId\`). NOT exposed: \`ObjectLineMapping::from\_object\` (il2cpp line-mapping DIF), BCSymbolMap parsing, UuidMapping plist parsing, embedded Portable PDB extraction. These gaps make BCSymbolMap/il2cpp DIF creation impossible without native tooling. +* **@sentry/symbolic 13.4.0 API: Archive/ObjectFile class contract and WASM memory management**: Exported classes: \`Archive\` (\`objects()\`, \`peek()\`, \`fileFormat\`, \`objectCount\`), \`ObjectFile\` (\`debugId\`, \`codeId\`, \`arch\`, \`fileFormat\`, \`hasDebugInfo\`, \`hasSources\`, \`hasSymbols\`, \`hasUnwindInfo\`, \`kind\`, \`debugSession()\`, \`embeddedPpdb()\`, \`il2cppLineMapping(provider)\`), \`DebugSession\` (\`files()\`, \`sourceByPath()\`), \`SourceBundleWriter\` (\`writeObject()\`, \`collectIl2cppSources\` setter, \`isEmpty\`), \`SourceFileDescriptor\` (\`type\`, \`contents\`, \`url\`, \`path\`, \`sourceMappingUrl\`, \`debugId\`). \`il2cppLineMapping(provider)\` added in PR #1005 — calls JS provider with C++ file path, returns serialized JSON \`ObjectLineMapping\` or \`undefined\`. \`embeddedPpdb()\` added in PR #1004. NOT exposed: BCSymbolMap parsing, UuidMapping plist parsing. * **@sentry/symbolic release flow: build from master → local tarball → npm publish via craft**: Steps: 1. Checkout \`origin/master\` in \`~/Code/getsentry/symbolic\` — ensures all merged PRs included. 2. Run \`cd symbolic-wasm && bash build-npm.sh\` — compiles wasm32, runs wasm-opt, packs tarball, runs smoke test (3 assertions). 3. Pin CLI \`package.json\` to \`file:\` tarball for local validation — run tests, typecheck, lint. 4. Revert \`file:\` pin before committing — wait for npm publish (craft flow via getsentry/publish). 5. Once new version (e.g. 13.5.0) appears on npm, update \`package.json\` to semver pin. Gotchas: - Merge ≠ publish: PR #997 merged Jun 24 but npm still showed 13.4.0 — always verify \`npm dist-tags\` before committing semver pin. - \`file:\` tarball pin must never be committed to the CLI repo. Verify: - \[ ] \`npm view @sentry/symbolic dist-tags.latest\` shows expected version. - \[ ] Smoke test 3/3 pass including 'debug session enumerates referenced source files'. - -* **@sentry/symbolic WASM build: always have latest build + initSync pattern**: \`@sentry/symbolic\` WASM loading pattern: (1) always have latest WASM build before testing; (2) load via \`initSync({ module })\` passing bytes directly; (3) 3-path fallback loader: SEA (\`node:sea.getRawAsset\`), npm bundle (sibling \`dist/vendor/symbolic\_bg.wasm\` via \`import.meta.url\` + \`existsSync\`), dev (\`require.resolve\`). Only the \`.wasm\` subpath is marked \`external\` in esbuild configs; JS glue is bundled. \`@sentry/symbolic@13.4.0\` introduced \`Archive\` class with \`static peek(data: Uint8Array): string | undefined\` and \`ObjectFile\` class replacing old free functions. - * **@sentry/symbolic: object.has\_sources() only reports embedded sources — use debug\_session.files() to detect any sources**: In \`@sentry/symbolic\` (mirroring \`print\_sources.rs\` in legacy Rust sentry-cli): \`object.has\_sources()\` only reports \*embedded\* sources, NOT referenced files. To detect whether an object has any sources at all, use \`debug\_session.files().next().is\_none()\`. Core enumeration pattern: \`Archive::parse(\&data)\` → \`archive.objects()\` → \`object.debug\_session()?.files()\` → \`FileEntry.abs\_path\_str()\` → \`debug\_session.source\_by\_path(abs\_path)\` → \`SourceFileDescriptor\` (fields: \`contents()\`, \`url()\`, \`debug\_id()\`, \`source\_mapping\_url()\`). PE with embedded PDB: also handle via \`pe.embedded\_ppdb()\`. @@ -319,18 +340,15 @@ * **bundle-sources exit-code convention: manual exitCode=1 vs OutputError for no-sources path**: In \`src/commands/debug-files/bundle-sources.ts\`, the no-sources-found path uses \`this.process.exitCode = 1\` (not \`OutputError\`). This is a known Medium deviation from the framework-blessed pattern: \`OutputError\` (exit 60) is robust against framework finalization resetting exitCode; manual assignment is fragile. The \`-o\` flag also does NOT create parent directories (unlike \`bundle-jvm\`), so \`writeFile\` throws raw ENOENT if the parent doesn't exist. Both are accepted as-is in PR #1126 — do not 'fix' them without a follow-up PR discussion. Exit code map: ValidationError→21, success→0, no-sources→1. + +* **CI Node version pinning: centralized env block per workflow file, ternary for matrix jobs**: Node CVE-2026-48931 fix: \`NODE\_VERSION\_22="22.23.1"\`, \`NODE\_VERSION\_24="24.18.0"\` (22.23.0 had the vulnerability; fix landed in 22.23.1 via nodejs/node#64004). Pattern: add top-level \`env:\` block to each workflow file (ci.yml, release.yml, sentry-release.yml, docs-preview.yml) with both constants + rationale comment. Reference via \`${{ env.NODE\_VERSION\_22 }}\`. Matrix jobs (build-npm) use ternary: \`${{ matrix.node == '24' && env.NODE\_VERSION\_24 || env.NODE\_VERSION\_22 }}\` — matrix labels stay as bare majors (\`\["22","24"]\`) for job naming. Gotcha: \`eval-skill-fork.yml\` has no \`setup-node\` step at all — must add one explicitly \[\[019f03bb-f9cf-7208-a183-d4f0074480f9]]. + * **clack-utils.ts filename preserved intentionally — rename deferred to next cleanup PR**: \`src/lib/init/clack-utils.ts\` filename kept (not renamed to \`wizard-utils.ts\`) to keep PR 4 diff focused on clack removal. No clack references remain in the file. \`WizardCancelledError\` lives here. \`abortIfCancelled\()\` return type uses \`Exclude\\` to narrow union types. \`FEATURE\_DISPLAY\_ORDER\` and \`CANONICAL\_STEP\_ORDER\` (12 steps) also defined here. Rename is intentionally deferred. * **createSourceBundle: object selection, sync provider contract, writer lifecycle**: \`createSourceBundle(data, objectName, readSource)\` in \`src/lib/dif/index.ts\`: selects \`objects.find(o => o.hasDebugInfo) ?? objects\[0]\`; returns \`{bundle:null, debugId:null, fileCount:0}\` if no objects. \`SourceBundleWriter.writeObject\` is synchronous — provider/filter callbacks must be sync (\`readFileSync\` in bundle-sources.ts). Writer is single-use: \`writeObject\` calls \`\_\_destroy\_into\_raw()\` (zeroes ptr, unregisters FinalizationRegistry). Provider returning \`null\` signals skip (WASM glue checks \`arg0 == null\`). \`bundle === null || fileCount === 0\` correctly catches manifest-only ZIPs with zero source files. - -* **debug-files shared readDebugFile extracted to read-file.ts — not inlined per command**: \`src/commands/debug-files/read-file.ts\` is the shared async helper for reading DIF files. Throws \`ValidationError\` on ENOENT (\`File '${path}' does not exist.\`), EISDIR (\`Path '${path}' is a directory...\`), and generic read failures. Both \`check.ts\` and \`bundle-sources.ts\` import from \`./read-file.js\`. Pattern: extract shared file-reading logic here rather than duplicating in each debug-files subcommand. - - -* **debug-files upload ZIP scanning: architecture and invariants**: \`src/lib/dif/zip.ts\` exports \`readZipDifEntries()\`: detects archives by \`.zip\` extension + \`PK\` magic bytes (NOT \`SYSB\`), extracts via \`fflate.unzipSync\` with pre-decompression size filter (zip-bomb guard), drops directory/empty entries, skips malformed archives gracefully. Key invariants (code-comment level): (1) nested ZIPs never recursed regardless of \`scanZips\` setting; (2) a \`.zip\` container is never parsed as a DIF itself — \`null\` from \`prepareZipDifs\` falls through to normal file handling; (3) oversized files of unrequested formats never trigger 'all matched files too large' — format filter runs before size gate. Source bundles use \`SYSB\` magic (not \`PK\`) so \`hasZipMagic\` correctly skips them — no regression on \`.src.zip\` or JVM bundle uploads. - * **debug-files upload: DIF assemble wire format and chunk-upload pipeline**: Native DIF assemble body: \`{ \[overallSha1]: { name, debug\_id?, chunks: string\[] } }\` — identical shape to \`proguard.ts\`/\`dart-symbols.ts\`. \`debug\_id\` is advisory (server re-parses). Per-file upload: each file chunked as raw bytes via \`hashBuffer\`; primary object selected via \`selectBundledObject\` (first with debug info, fallback to first). Assemble endpoint: \`projects/${org}/${project}/files/difs/assemble/\`. Constants: \`DEFAULT\_MAX\_DIF\_SIZE=2GB\`, \`DEFAULT\_MAX\_WAIT=300s\`. \`--wait\` flag controls whether to poll until assembly completes. Deferred: ZIP scanning, BCSymbolMap/dsymutil, Xcode derived-data, il2cpp mapping (require native tools not available in WASM). @@ -341,7 +359,13 @@ * **Dedupe resolved entity IDs in batch operations before API call**: Batch issue merge (\`src/commands/issue/merge.ts\`): (1) Dedupe by resolved numeric ID after \`Promise.all(args.map(resolveIssue))\` — users may pass same entity as \`CLI-K9\`, \`my-org/CLI-K9\`, or \`123\`. Throw \`ValidationError\` if \`new Set(ids).size < 2\`. (2) Reject \`undefined\` orgs in cross-org check — bare numeric IDs without DSN/config resolve with \`org: undefined\`. (3) Pass \`--into\` through \`resolveIssue()\`; compare by numeric \`id\`, not \`shortId\`. (4) Sentry bulk merge API picks canonical parent by event count — \`--into\` is preference only; warn when API's \`parent\` differs. -* **DifObjectSources: enumerationError + all-or-nothing files contract**: \`DifObjectSources\` in \`src/lib/dif/index.ts\` has \`enumerationError: string | null\`. When \`debugSession()\` throws, the catch block sets \`enumerationError\` to the error message AND resets \`files\` to empty — never a partial list paired with an error. This all-or-nothing contract is a standing invariant: callers can trust that \`files.length > 0\` implies \`enumerationError === null\`. Footer hint in \`print-sources\` uses \`failed.length > 0\` (any failure) not \`failed.length === objects.length\` (all failed) to trigger the read-error path. +* **DifObjectSources: enumerationError + all-or-nothing files contract**: Exported classes: \`Archive\` (\`objects()\`, \`peek()\`, \`fileFormat\`, \`objectCount\`), \`ObjectFile\` (\`debugId\`, \`codeId\`, \`arch\`, \`fileFormat\`, \`hasDebugInfo\`, \`hasSources\`, \`hasSymbols\`, \`hasUnwindInfo\`, \`kind\`, \`debugSession()\`, \`embeddedPpdb()\`), \`DebugSession\` (\`files()\`, \`sourceByPath()\`), \`SourceBundleWriter\` (\`writeObject()\`, \`collectIl2cppSources\` setter, \`isEmpty\`), \`SourceFileDescriptor\` (\`type\`, \`contents\`, \`url\`, \`path\`, \`sourceMappingUrl\`, \`debugId\`). \`embeddedPpdb()\` added in PR #1004. NOT exposed: \`ObjectLineMapping::from\_object\` (IL2CPP line-mapping DIF), BCSymbolMap parsing (\`BcSymbolMap::parse\` + \`Object::load\_symbolmap()\`), UuidMapping plist parsing (\`UuidMapping::parse\_plist\`). IL2CPP ObjectLineMapping lives in \`symbolic-il2cpp/src/line\_mapping/from\_object.rs\`; API: \`ObjectLineMapping::from\_object(object)\` + \`to\_writer()\`. BCSymbolMap \`load\_symbolmap\` has deprecation TODO pointing at symcache Transformer. UuidMapping only meaningful paired with BCSymbolMap. These gaps make BCSymbolMap/IL2CPP DIF creation impossible without native tooling. + + +* **error-reporting.ts silencing rules: which error types are silenced and why**: Current \`classifySilenced()\` branches (post-PRs #1148–#1153): - \`OutputError\` → \`'output\_error'\` (piped output closed early) - \`ContextError\` → \`'context\_missing'\` (user omitted required value; never a CLI bug) - \`AuthError\` (any reason: \`not\_authenticated\`, \`expired\`, \`invalid\`) → \`'auth\_expected'\` (all auth reasons are user/env state after CLI-19 fix) - \`ApiError\` status >400 && <500 → \`'api\_user\_error'\` - \`ApiError\` + \`isSearchQueryParseError()\` → \`'api\_query\_error'\` (status 400 with 'Error parsing search query' detail) - \`TypeError\` + \`isNetworkError()\` → \`'network\_error'\` (raw 'fetch failed' only) - else → \`null\` (captured) Metric emission must never block error handling — all metric/logger calls in \`recordSilencedError()\` are wrapped in try/catch. + + +* **getsentry/symbolic CHANGELOG.md: use bold \*\*Features\*\* style, not ### headings**: Danger bot on getsentry/symbolic PRs requires a CHANGELOG.md entry before merge. The bot's example uses \`### Features\` heading style, but the actual repo convention (established in PR #997 and #1004) uses \`\*\*Features\*\*\` bold style under the \`## Unreleased\` section. Always match the bold style. Entry format: \`- WASM: expose \\\`Method()\\\` for \. (\[#NNNN]\(url))\`. Danger re-runs after the changelog commit and reports 'All green' when satisfied. * **getsentry/symbolic: follow-up PR pattern for merged PR review comments**: getsentry/symbolic PR #988 (feat/source-bundle-provider) follow-up to Dav1dde review: \*\*Implemented:\*\* (1) \`write\_object\_with\_filter\_and\_provider\` private inner method takes both filter \`F\` and provider \`P: Fn(\&str) -> Option\\`; public methods delegate to it. (2) \`SharedCursor\` (\`Rc\>>>\`) removed — replaced with plain \`Cursor::new(Vec::new())\` + \`into\_inner()\`. (3) Provider changed from destructive \`contents.remove(path)\` to non-destructive \`contents.get(path).map(|v| v.as\_slice())\`. (4) Tests use \`unwrap()\` not \`-> Result\`. (5) \`smoke-test.mjs\` + \`build-npm.sh\` wiring + \`ci.yml\` \`wasm-smoke\` job added — closes gap where wasm bindings were only compile-checked on PRs. Byte-identity verified: sha256 \`4d29224558f27174fef90e81d5c7e80fd388249f25cb48cfc4c5eb316e3b067b\` identical BASE vs HEAD. @@ -358,11 +382,11 @@ * **idle.ts eviction: upstream uses per-function cleanup in idle.ts, not centralized evictSession in pipeline.ts**: Upstream (main branch) puts session eviction logic directly in \`idle.ts\` rather than a centralized \`evictSession()\` in \`pipeline.ts\`. \`idle.ts\` imports cleanup functions individually: \`evictSession as evictGradientSession\` from \`@loreai/core\`; also \`deleteSessionAuth\`, \`clearAuthStale\` from \`./auth\`; \`deleteSessionCosts\` from \`./cost-tracker\`; \`deleteBillingPrefix\` from \`./cch\`; \`clearWarmupAuthDisabled\` from \`./cache-warmer\`. The \`startIdleScheduler\` signature uses \`onEvict?: (sessionID: string) => void\` (upstream) vs \`onEvictSession?: (sessionID: string) => boolean\` (branch). Upstream inline \`onEvict\` in \`pipeline.ts\` cleans 5 Maps: \`headerSessionIndex\`, \`ltmSessionCache\`, \`ltmPinnedText\`, \`stableLtmCache\`, \`cwdWarned\`. When merging, adopt upstream's per-function approach and add any missing cleanup calls. - -* **InkUI ink-app.js sidecar loading — three runtime contexts**: \`createInkUI()\` resolves \`inkAppPath\` differently per runtime: (1) Node SEA binary — \`sea.getAsset('dist-build/ink-app.js', 'utf-8')\`, write to \`mkdtempSync\`, import via \`pathToFileURL\`, then \`rmSync\` temp dir (best-effort); (2) Node/npm bundle — \`inkAppPath\` starts with \`'./'\`, resolve via \`new URL(inkAppPath, import.meta.url).href\`; (3) Dev mode — absolute filesystem path. Imported via \`with { type: 'file' }\` from \`./ink-app.tsx\`. See also \[\[019e4fe7-dbf1-7ed6-8b39-473e2e4ea29e]] for SEA temp file cleanup pattern. + +* **Node version pinning convention: workflow-level env vars NODE\_VERSION\_22 / NODE\_VERSION\_24**: As of PR #1145, all GitHub Actions workflows in sentry-cli (TypeScript) centralize Node version pins as workflow-level \`env\` vars: \`NODE\_VERSION\_22: "22.23.1"\` and \`NODE\_VERSION\_24: "24.18.0"\`. All \`actions/setup-node\` steps reference \`${{ env.NODE\_VERSION\_22 }}\` or \`${{ env.NODE\_VERSION\_24 }}\` — no bare \`"22"\`/\`"24"\` strings. Matrix jobs use ternary: \`${{ matrix.node == '24' && env.NODE\_VERSION\_24 || env.NODE\_VERSION\_22 }}\`. Motivation: Node 24.17.0/22.23.0 shipped \`ERR\_STREAM\_PREMATURE\_CLOSE\` regression (CVE-2026-48931 http.Agent fix); fixed in 24.18.0/22.23.1 (nodejs/node#64004). When bumping Node, update the \`env\` block in each workflow file. - -* **IssueViewOutputSchema: extends SentryIssueSchema with enrichment fields for --fields discoverability**: \`IssueViewOutputSchema\` in \`src/types/sentry.ts\` extends \`SentryIssueSchema\` with 4 enrichment fields added by \`jsonTransformIssueView\`: \`event\` (z.unknown().nullable().optional() — too complex to enumerate; documenting all sub-fields would bloat the table without adding useful \`--fields\` discoverability), \`org\` (string | null), \`replayIds\` (array), \`trace\` (object with \`traceId\`+\`spans\`, nullable). Wired via \`schema: IssueViewOutputSchema\` on the \`output\` config in \`view.ts\`. Pattern mirrors \`ReplayViewOutputSchema\` in \`src/types/replay.ts:356-367\`. Exported from \`src/types/index.ts\`. Generates a JSON Fields table in \`references/issue.md\` and \`--help\` output. \`event\` uses \`z.unknown()\` because documenting all sub-fields (stacktraces, breadcrumbs, contexts) would bloat the table without adding useful \`--fields\` discoverability. + +* **parseIssueArg multi-line handling: first non-blank line wins (CLI-1G1)**: Issue identifiers are always single-line atomic tokens. \`parseIssueArg\` in \`src/lib/arg-parsing.ts\` uses \`LINE\_SPLIT\_PATTERN = /\r?\n/\` to split input, trims each line, and takes the first non-empty line (PR #1148, CLI-1G1). Rationale: \`.trim()\` only strips leading/trailing whitespace — embedded newlines from command substitution or agent note-appending survive and reach \`validateResourceId\`. Joining lines (like the \`api.ts\` LINE\_BREAK\_PATTERN approach) is wrong for atomic tokens because \`CLI-ABC\n\\` would produce garbage. Splitting on \`\n\` is safe because newlines are control characters already rejected by \`validateResourceId\` — splitting never breaks project display names with spaces. * **Preserve ApiError type so classifySilenced can silence 4xx errors**: Preserve ApiError type for classifySilenced: \`classifySilenced\` (src/lib/error-reporting.ts) only silences \`ApiError\` with status 401-499 — wrapping in generic \`CliError\` loses \`status\` and causes 403s to be captured. Re-throw via \`new ApiError(msg, error.status, error.detail, error.endpoint)\` with terse message (\`ApiError.format()\` appends detail/endpoint). \`ValidationError\` without \`field\` collapses unfielded errors into one fingerprint; always pass \`field\`. Fingerprint rule changes don't retroactively re-fingerprint — manually merge new groups into canonical old parents. \`ApiError\` rule keys by \`api\_status + command\`. @@ -379,17 +403,17 @@ * **Sentry SDK tree-shaking patches must be regenerated via bun patch workflow**: Sentry SDK tree-shaking via bun patch: \`patchedDependencies\` in \`package.json\` strips unused exports from \`@sentry/core\` and \`@sentry/node-core\`. Non-light root of \`@sentry/node-core\` pulls uninstalled \`@opentelemetry/instrumentation\` — \*\*always import from \`@sentry/node-core/light\`\*\* (subpaths: \`.\`, \`./light\`, \`./light/otlp\`, \`./init\`, \`./loader\`, \`./import\`). No supported import for \`HttpsProxyAgent\`. Bumping SDK: remove old patches, \`rm -rf ~/.bun/install/cache/@sentry\`, \`bun install\`, \`bun patch @sentry/core\`, edit, \`bun patch --commit\`; repeat for node-core. Preserved: \`\_INTERNAL\_safeUnref\`, \`\_INTERNAL\_safeDateNow\`, \`nodeRuntimeMetricsIntegration\`. Before stripping any core export, grep \`node-core/build/{cjs,esm}/light/sdk.js\` for runtime usage (e.g. \`spanStreamingIntegration\` when \`traceLifecycle === 'stream'\`). Remove \`.bun-tag-\*\` hunks from generated patches. Manual \`git diff\` patches fail. - -* **sentry-cli debug-files: @sentry/symbolic usage locations for API migration**: \`@sentry/symbolic\` consumed in sentry-cli (TypeScript) at \`src/lib/dif/index.ts\`: imports \`initSync\`, \`parse\_debug\_file as wasmParseDebugFile\`, \`peek\_format as wasmPeekFormat\`. \`DIF\_WASM\_ASSET\_KEY = 'dist-build/symbolic\_bg.wasm'\` (SEA asset key). \`SYMBOLIC\_WASM\_SUBPATH = '@sentry/symbolic/symbolic\_bg.wasm'\` (dev resolution, marked external in esbuild). 3-path WASM loader: SEA \`node:sea.getRawAsset\`, sibling \`./vendor/symbolic\_bg.wasm\`, dev \`\_require.resolve(SYMBOLIC\_WASM\_SUBPATH)\`. Public API: \`parseDebugFile(data: Uint8Array): DifArchiveInfo\`, \`peekFormat(data: Uint8Array): string\`. Internal \`RawArchiveInfo\` (snake\_case Rust) mapped to camelCase \`DifArchiveInfo\`. Migration to \`Archive\`/\`ObjectFile\` class-based API on branch \`byk/feat/debug-files-new-symbolic-api\` (commit \`3052a6e2d\`). Consumer: \`src/commands/debug-files/check.ts\` imports \`DifArchiveInfo\`, \`parseDebugFile\`. - * **setup.ts bestEffort() wrapper: post-install steps must never crash setup**: \`src/commands/cli/setup.ts\` \`bestEffort(stepName, fn)\` wraps non-essential post-install steps (recording install info, shell completions, agent skills) in try/catch. On failure: calls \`warn(stepName, error)\` + \`captureException(error, { level: 'warning', tags: { 'setup.step': stepName } })\`. These steps must NEVER crash setup — enforced by \`bestEffort()\`. \`runConfigurationSteps()\` applies \`bestEffort()\` independently to all 4 steps. Install dir priority: (1) \`$SENTRY\_INSTALL\_DIR\`, (2) \`~/.local/bin\` if exists+in PATH, (3) \`~/bin\` if exists+in PATH, (4) \`~/.sentry/bin\` fallback. Welcome message only on fresh install (not upgrades). * **Shared pagination infrastructure: buildPaginationContextKey and parseCursorFlag**: Pagination infrastructure + org flag injection: Bidirectional pagination via cursor stack in \`src/lib/db/pagination.ts\`. \`resolveCursor(flag, key, contextKey)\` maps keywords (next/prev/first/last) to \`{cursor, direction}\`. \`advancePaginationState\` manages stack — back-then-forward truncates stale entries. Critical: \`resolveCursor()\` must be called INSIDE \`org-all\` override closures, not before \`dispatchOrgScopedList\`. \`issue list --limit\` is global total: \`fetchWithBudget\` Phase 1 divides evenly, Phase 2 redistributes surplus. \`trimWithProjectGuarantee\` ensures ≥1 issue per project. Compound cursor (pipe-separated) enables \`-c last\` for multi-target pagination. JSON output wraps in \`{ data, hasMore }\` with optional \`errors\` array. \`sort\` flag is resolved once in \`func()\` before dispatch — never re-derived by infra. \`handleOrgAllIssues\` returns server order (no client-side sort). \`isMultiProject\` guard gates client-side sort at list.ts:1144-1148. - -* **Stricli optional flags with runtime-resolved defaults: omit static default, resolve in func()**: When a flag's default depends on runtime state (e.g., host type), Stricli static defaults cannot be used. Pattern: mark flag \`optional: true\` with no \`default\` in the flag definition; in \`func()\`, immediately resolve: \`const flags = { ...rawFlags, sort: rawFlags.sort ?? defaultIssueSort() }\`. All downstream code receives the concrete resolved type. \`ListFlagsInput\` uses \`Omit\ & { readonly sort?: SortValue }\` as the \`func()\` parameter type; \`ListFlags\` (with required sort) is used everywhere else. This avoids Stricli's static-default limitation while keeping type safety. + +* **symbolic-il2cpp integration tests: use symbolic-testutils dev-dependency with Object::parse pattern**: Integration tests for \`symbolic-il2cpp\` live in \`symbolic-il2cpp/tests/\` (separate from unit tests in \`src/\`). Add \`symbolic-testutils = { path = "../symbolic-testutils" }\` as dev-dependency (path-only, safe for publishing — matches \`symbolic-debuginfo\` pattern). Use \`ByteView::open(fixture("..."))\` → \`Object::parse(\&view)?\` to get a real \`ObjectLike\`. Fixture files live in \`symbolic-testutils/fixtures/\`. Native unit test with mock \`ObjectLike\` rejected as too heavyweight (many methods to implement). PR #1005 added \`from\_object\_with\_provider\_empty\_without\_sources\` and \`from\_object\_with\_provider\_parses\_source\_info\` tests. + + +* **symbolic-wasm two-layer testing: wasm\_bindgen\_test (Layer 1) + npm pack smoke test (Layer 2)**: Layer 1: \`wasm\_bindgen\_test\` in \`symbolic-wasm/tests/debuginfo.rs\`, run via \`wasm-pack test --node\` in CI (\`ci.yml\`). Layer 2: \`smoke-test.mjs\` packs tarball via \`npm pack\`, installs into temp dir, runs \`node --test package-smoke.test.mjs\` with \`SMOKE\_FIXTURE\` env var. Layer 2 catches exports-map/resolution breakage and runtime errors that \`wasm-pack test\` misses (it builds its own glue, never loads the shipped \`symbolic.js\`). Run \`bash symbolic-wasm/build-npm.sh\` locally for Layer 2 (wasm-pack not required locally). CI runs both layers: \`ci.yml\` (Layer 1) and \`build.yml\` (Layer 2). * **Telemetry instrumentation pattern: withTracingSpan + captureException for handled errors**: For graceful-fallback operations, use \`withTracingSpan\` from \`src/lib/telemetry.ts\` for child spans and \`captureException\` from \`@sentry/bun\` (named import — Biome forbids namespace imports) with \`level: 'warning'\` for non-fatal errors. \`withTracingSpan\` uses \`onlyIfParent: true\` — no-op without active transaction. User-visible fallbacks use \`log.warn()\` not \`log.debug()\`. Several commands bypass telemetry by importing \`buildCommand\` from \`@stricli/core\` directly instead of \`../../lib/command.js\` (trace/list, trace/view, log/view, api.ts, help.ts). @@ -405,8 +429,20 @@ * **Always add new check scripts to both package.json and CI workflow**: When introducing a new check or validation script, the user expects it to be registered in two places simultaneously: (1) as a named script in \`package.json\` alongside other \`check:\*\` scripts, and (2) as a \`- run: pnpm run \\` step in \`.github/workflows/ci.yml\`. Never add to only one location. This applies to any new linting, validation, or verification script added to the project. - -* **Always check git status after tests pass before committing**: Always check git status after tests pass before committing: After a full test suite run completes successfully, check git status to review all changed files before staging. Run \`git status\` (and often \`git diff\`) after confirming a clean test run to enumerate modified/new files — including generated files, docs, and skill files — before committing. Also verify \`git log\` and \`git diff origin/\\` to confirm no unexpected commits from parallel agent sessions (parallel agents can commit stray \`.lore.md\` changes to wrong branches). + +* **Always annotate design invariants and constraints as code comments at the exact location they apply**: The user consistently embeds precise behavioral invariants, sequencing constraints, and design rationale as code comments directly at the relevant lines (e.g., 'never tripped by an absent DerivedData directory' at line 68, 'server's advertised maxFileSize gates the scan' at lines 582–583, 'Never unknown (those return null)' in scan.ts JSDoc, 'At least one feature always remains wanted' in buildDifFilters JSDoc). When reviewing or writing code, the user expects these invariants to be documented inline, not in external docs or commit messages. Assistant should preserve and add such comments when implementing or modifying logic that has non-obvious behavioral guarantees. + + +* **Always ask the assistant to identify and recommend the next feature to implement from a prioritized backlog**: When working through a multi-feature implementation roadmap (e.g., Tier B/C WASM features), the user consistently asks the assistant to survey remaining unimplemented features, analyze their complexity/value/feasibility, and recommend which one to tackle next. The user expects the assistant to: (1) enumerate what's left, (2) locate the relevant APIs and source files, and (3) provide a reasoned recommendation with rationale (e.g., real-world value, deprecation status, complexity). The assistant should proactively do the research rather than asking the user to specify. + + +* **Always avoid over-silencing errors by requiring targeted, opt-in criteria**: When silencing or suppressing errors in error-reporting logic, the user consistently prefers narrow, explicit criteria over broad blanket rules. They favor opt-in flags (e.g., \`expected: true\`) or specific field/status/message matchers rather than silencing entire error classes. They deliberately exclude ambiguous cases (e.g., 400s that might be CLI bugs vs. user input errors) and document the reasoning. When proposing to silence a new error type, always justify why it is purely environmental or user-caused, and avoid silencing anything that could mask a real code defect. Backstop rules (e.g., silence all status-0) are acceptable only when the error class is provably non-actionable for the team. + + +* **Always check open PRs and recent commits before starting new work to avoid duplication**: At the start of each session or new task, the user expects the agent to inspect open PRs and recently merged commits before beginning any implementation. This prevents duplicate work, identifies already-merged changes, and surfaces orphaned branches. The agent should run checks equivalent to listing open PRs, reviewing recent git log on main, and checking the current branch state (ahead/behind, clean working tree) before planning or executing any new work. + + +* **Always clarify ambiguous requests before proceeding with implementation**: When the user gives a short, context-dependent instruction (e.g., 'model CLI patches after these'), the assistant should pause and ask for clarification rather than guessing. The user works across multiple repos (symbolic WASM, TypeScript CLI, Rust CLI) with overlapping feature threads, so ambiguous references to 'these' or similar pronouns can mean different things. The user expects the assistant to identify the candidate interpretations explicitly and ask which one to pursue, rather than picking one and proceeding. This pattern appears consistently when the user's intent could map to 2+ distinct workstreams. * **Always clarify that the repo uses plain git (not jj) when jj commands fail**: When a jj command fails with 'no jj repo in .', the user consistently clarifies that the repo is a plain git repo and that jj's 'never fails on conflict' behavior is being referenced conceptually — meaning conflicts should be recorded/resolved rather than aborting operations. The agent should: (1) fall back to git commands immediately without retrying jj, (2) handle merge conflicts by stashing, pulling, and resolving (e.g., \`git checkout --theirs\` for files like \`.lore.md\`), and (3) not attempt \`jj git init\` or any jj initialization. This pattern appears at the start of every build session. @@ -420,11 +456,11 @@ * **Always create a dedicated branch when updating fossilize versions**: When a new version of fossilize is released, always create a branch named \`chore/fossilize-{version}\` tracking origin/main, update the dependency, remove any functionality now handled natively by fossilize (e.g., \`stripCachedNodeBinaries()\` removed in 0.7.0), verify the build succeeds, then commit with \`chore: update fossilize to X.Y.Z\`. Follow this exact pattern: branch → update dep → remove superseded code → build verify → commit → PR. - -* **Always design tests with explicit edge case rationale before implementation**: When planning tests, the user specifies not just what to test but \*why\* each test case exists — documenting the exact scenario, the specific input values chosen (e.g., 'two entries of 10,000 zero bytes — highly compressible'), and the expected behavior with reasoning (e.g., 'old behavior was buggy'). Tests are categorized by regression type (M1, M2, H1) and designed to target specific failure modes like zip-bomb bypasses, budget edge cases, and unsupported compression methods. Follow this pattern by proposing test cases with named categories, explicit input rationale, and documented expected outputs before writing any test code. + +* **Always distinguish E2E skill-eval failures as Anthropic API flakes, not code regressions**: When E2E tests fail on \`skill-eval.test.ts\`, the user consistently expects the assistant to immediately check whether the failure is due to Anthropic API outages ('Premature close' errors on api.anthropic.com) rather than code regressions. The assistant should: (1) confirm all other checks (lint, unit, warden, build) are green, (2) identify the specific error pattern in logs, (3) classify the failure as infrastructure flake vs. regression, (4) avoid unnecessary re-runs during confirmed sustained outages, and (5) present the user with clear options (retry vs. admin override) rather than making the merge decision unilaterally. The user treats this as a known flaky test category requiring triage, not automatic re-runs. -* **Always document invariants and non-obvious design decisions as inline code comments or JSDoc**: When implementing functions, types, or test utilities with non-obvious invariants, the user consistently adds explicit inline comments or JSDoc explaining \*why\* a design choice was made — not just what it does. Examples: why a function always returns non-null, why a branch is intentionally omitted, why a specific API is used over an alternative, and what would break if the pattern changed. These comments are written as assertions ('Always returned (never null)...', 'Always restore — never delete.') and reference the downstream consumer or failure mode. When generating or modifying code, always include this kind of explanatory commentary for invariants, idempotency guarantees, and intentional omissions. +* **Always document invariants and non-obvious design decisions as inline code comments or JSDoc**: The user consistently embeds precise behavioral invariants, edge cases, and design constraints directly in source code as JSDoc doc comments or inline comments rather than only in external docs. These comments assert things like 'never X', 'always Y', 'only when Z'. When reviewing or writing code, expect the user to treat these comments as authoritative specifications. When generating code, include JSDoc/inline comments that explicitly state invariants, known limitations, and non-obvious behavioral guarantees (e.g., 'never fully materialised', 'always returns false', 'mtime is always 0 when recordMtimes: false'). Do not omit these even if the behavior seems obvious from the code. * **Always explore codebase systematically before implementing changes**: When investigating a feature or bug, the user consistently requests reads of multiple related files in sequence before any implementation: the command file, the API layer, the type definitions, the formatters, and any generated/skill files. They want to understand the complete data flow end-to-end (API response → types → command → output/formatter) before making changes. Always read all relevant files across these layers when asked to investigate or analyze a feature, rather than jumping straight to implementation or reading only one file. @@ -435,20 +471,20 @@ * **Always flag import framework mismatches as blocking CI issues in PR reviews**: When reviewing PRs, the user consistently identifies test files using the wrong import framework (e.g., \`bun:test\` instead of \`vitest\`) as a BLOCKING issue, not a non-blocking suggestion. This applies when the project has migrated frameworks and all other test files use the new one. The user expects the reviewer/assistant to explicitly label it as blocking (B1, B2, etc.) and distinguish it from non-blocking issues (N1, N2, etc.), using a clear severity classification system in PR review feedback. - -* **Always honor Retry-After header when present in LLM adapter**: (architecture) LLM adapter backoff in \`packages/gateway/src/llm-adapter.ts\`: Always honor Retry-After — \`backoffMs()\` returns \`Math.min(retryAfterMs, cap)\` where cap is \`RETRY\_AFTER\_CAP\_URGENT\_MS=8\_000\` or \`RETRY\_AFTER\_CAP\_BACKGROUND\_MS=120\_000\`. TRANSIENT\_CODES={429,500,502,503,529}; MAX\_RETRIES: rate-limit=3, server=3, urgent=2. Backoff (no Retry-After): 429 background=60s/120s/180s; urgent=min(1000×2^n,4000); 5xx background=min(1000×2^n,8000). Bearer tokens inject \`billingBlock\` as first system block; \`signBody()\` replaces \`cch=00000\` with xxHash64. System prompt caching uses \`cache\_control:{type:'ephemeral',ttl:'1h'}\`. \`opts.thinking\` NOT forwarded to bare API calls. Circuit breaker tripped on non-urgent 429s via \`tripCircuitBreaker()\`. Gateway auth (\`packages/gateway/src/auth.ts\`): \`AuthCredential\` (api-key|bearer). Two-level lookup: \`sessionAuth\` Map → \`lastSeenAuth\` global fallback via \`resolveAuth(sessionID?)\`. \`authFingerprint()\` = SHA-256 truncated to 16 hex chars. - * **Always investigate root cause by tracing through multiple specific code layers before accepting a fix**: When facing a runtime bug (especially undefined values from framework internals), the user consistently demands thorough investigation across multiple layers — framework source code (node\_modules), wrapper utilities, bundler config, and call sites — before accepting any fix. The user explicitly rejects surface-level explanations and pushes for tracing the exact code path that produces the unexpected value. Only after exhausting the investigation does the user accept a defensive fix strategy. When directing investigation, the user specifies concrete areas to search (e.g., 4 specific code locations). Always read and analyze the relevant framework internals, not just application code. - -* **Always investigate unexpected file modifications before committing**: When reviewing git status or diffs before committing, the user expects the agent to flag and investigate any modified files that the agent did not explicitly change during the session. The agent should identify the provenance of unexpected changes (e.g., leftover from prior sessions, side effects of test runs, or generate steps) before staging and committing. The agent should never silently include unrecognized modifications in a commit without first explaining why those files are present in the diff. - * **Always keep work-in-progress repos at permanent paths under ~/Code/getsentry/**: When working on a repository across sessions, the user expects it to live at a permanent location under \`~/Code/getsentry/\\` (e.g., \`~/Code/getsentry/symbolic\`, \`~/Code/getsentry/cli\`), not in ephemeral directories like \`/tmp\`. If a clone exists in \`/tmp\`, move it to the permanent location and remove the temporary one. This applies to any \`getsentry\` org repo being actively developed on. Always use the permanent path when referencing or operating on the repo in subsequent steps. -* **Always migrate Bun-specific APIs and tooling to Node.js equivalents**: 🔴 Directive (repeated 25+ sessions): ALWAYS prefer editing existing files in the codebase. NEVER write new files unless explicitly required. NEVER proactively create documentation files (\*.md) or README files — only create documentation files if explicitly requested by the user. +* **Always migrate Bun-specific APIs and tooling to Node.js equivalents**: Migrating from Bun to Node.js/pnpm. Replace Bun-specific APIs: \`Bun.spawn\`→\`node:child\_process\`, \`Bun.sleep\`→\`node:timers/promises\`, \`bun:sqlite\`→\`node:sqlite\`, \`bun run\`→\`pnpm run\`/\`tsx\`, \`bun.lock\`→pnpm lockfile. All packages in \`devDependencies\` (never \`dependencies\`). Exception: \`script/build.ts\` uses fossilize (not \`Bun.build\`) and stays on Bun for the build-binary CI job. \`script/bundle.ts\` (npm bundle) uses esbuild via tsx and is Node-native. \`packageManager\` field in package.json: \`pnpm@10.11.0\`. After each migration phase, ensure lint and tests pass before committing. Migration is complete as of main branch (bun.lock deleted, vitest.config.ts added, all test files migrated to vitest). + + +* **Always monitor CI status after pushing or posting PR replies**: After completing an action on a PR (pushing commits, posting review replies, responding to bot findings), the user consistently expects the assistant to immediately check and track CI status. The assistant should proactively monitor pending checks, report which have passed/failed/skipped, identify what remains outstanding, and continue following up until all critical checks resolve. Don't wait to be asked — after any PR action, shift focus to CI status tracking and flag any remaining pending checks explicitly. + + +* **Always mutation-test new tests to verify they fail without the guarded behavior**: When reviewing or writing tests, the user performs mutation testing to confirm a test is non-tautological: they revert or remove the specific implementation behavior the test is supposed to guard (e.g., replacing \`atomicWriteFile\` with plain \`writeFile\`, or moving \`mkdirSync\` inside a try block), then re-run the suite. If the test still passes after the mutation, the user flags it as a blocking defect ('vacuous' or 'tautological') and requires a fix before the PR can ship. This applies especially to tests that assert absence of side effects (e.g., no leftover temp files, no recursion) where the assertion may be trivially true even without the feature. * **Always perform thorough codebase exploration before designing or implementing fixes**: When investigating a bug or feature, the user consistently requests comprehensive upfront exploration across multiple files before any code changes. This includes: reading relevant command and API files completely, searching for all references to key terms/parameters, checking type definitions in SDK/node\_modules, and understanding the full data flow from flags to API calls. The user expects the assistant to map out the entire call chain, identify misleading comments, and surface all related code paths before proposing a solution. Do not jump to fixes — first read all relevant files thoroughly and report findings. @@ -459,30 +495,45 @@ * **Always plan systemic fixes with structured multi-problem breakdowns before implementation**: When the user identifies documentation or tooling issues, they consistently organize them as numbered problems with precise file locations, line numbers, and root causes before any code is written. They expect the assistant to engage at the planning level first — proposing detection strategies, fix approaches, and tradeoffs — and to consolidate related problems (e.g., merging overlapping tasks) rather than treating each in isolation. Plans are written to files and iterated on. Implementation only follows after the plan is agreed upon. The user prefers systemic/automated fixes (e.g., derive patterns from package.json) over one-off patches. + +* **Always prefer eliminating waste over accepting known inefficiencies**: When the assistant identifies a performance inefficiency or redundant work (e.g., a binary sniff whose result is discarded, a vacuous test, an inaccurate comment), the user consistently chooses to fix it rather than accept it as a known tradeoff. Even when the assistant presents the fix as optional or a recommendation, the user locks it in as a definite task. Apply this pattern by: treating identified inefficiencies as action items rather than informational notes, and including the fix in the plan/PR scope by default rather than flagging it as a future improvement. + * **Always prefer systemic/automated solutions over one-off fixes**: When the user identifies errors, gaps, or problems, they explicitly direct the assistant to create or fix systems that prevent the entire class of errors in the future, rather than applying isolated one-off fixes. This applies especially when evaluating code quality, reviewing PRs, or addressing bugs. The user wants automated checks (e.g., CI steps, lint rules, scripts) and general solutions that scale, not patches that only address the immediate symptom. When planning or executing fixes, always ask: 'Can this be automated or systematized?' and prefer that approach. * **Always read and document full file details before proceeding with analysis or implementation**: When exploring a codebase, the user consistently reads files in full and records comprehensive structured details: exact line counts, all imports, every exported type/interface with their fields, all constants, all function signatures with their logic, and any notable comments or assertions. This applies to both source files and build/tooling scripts. The user expects the assistant to capture and reference these details precisely rather than summarizing loosely. When examining related files (e.g., a module and its consumers), the user reads each completely before drawing conclusions. This pattern applies during architecture exploration, feature planning, and documentation generation tasks. + +* **Always read existing code files in full before implementing new features**: Before writing any new code, the user consistently reads the full content of relevant existing files (source files, type definitions, related commands, utilities) to understand established patterns, types, and conventions. This applies to command files, library modules, WASM bindings, and type definitions. The user expects implementations to follow patterns already present in the codebase (e.g., \`auth: false\`, \`readDebugFile\`, \`CommandOutput\`, \`derived\_from\_cell!\`, \`AsSelf\` impls). When implementing something new, always reference and mirror the patterns found in analogous existing files rather than inventing new approaches. + * **Always read source files thoroughly before asking questions or making changes**: The user consistently reads full source files (often 400-900+ lines) and traces complete data flow pipelines across multiple modules before taking action. They examine types, constants, function signatures, and cross-module dependencies in depth. They do not ask clarifying questions upfront — instead they investigate the codebase themselves to build a complete mental model. When helping this user, proactively read all relevant files in full, trace imports and data flows end-to-end, and present comprehensive findings rather than asking what they want to know. Assume they want the full picture, not a summary. * **Always reference external tools and prior art when exploring build/size optimization approaches**: When investigating build pipeline improvements or binary size reduction, the user consistently references specific external tools, repos, and contacts (e.g., Vercel's build-binary.mjs, binpunch, fossilize, Melkey's work) as starting points for evaluation. They expect the assistant to analyze whether each referenced approach actually applies to their specific setup before recommending it. The user wants a clear breakdown of what's relevant vs. irrelevant given their actual architecture (e.g., 'we already use esbuild full bundling, so node\_modules stripping doesn't apply'), followed by concrete alternative opportunities ranked by impact. + +* **Always request critical objective code reviews before merging PRs**: Before merging a PR, the user requests a thorough, critical, and objective code review with a specific checklist of verification items. Reviews cover: completeness of changes (grep-based verification), correctness of logic (matrix conditions, guards, version accuracy), YAML/scoping correctness, absence of unintended changes, accuracy of PR description, and CI/linting findings. The user explicitly states 'Do NOT modify any files. Review only.' Reviews are structured around numbered verification items the user provides upfront. The assistant should read all changed files, run grep/shell commands to verify completeness, and report findings per the user's checklist without making any edits. + + +* **Always request READ-ONLY code reviews with explicit no-modification constraints**: When asking for code reviews, the user consistently specifies: (1) the review must be READ-ONLY — no files written, no code modified, no branches switched; (2) the diff command to use (e.g., \`git --no-pager diff origin/master\`); (3) the repo path and branch context; (4) the PR's purpose and key design decisions upfront. The user frames reviews as 'critical' and 'objective'. Reviews cover multiple files and specific concerns (error handling, API design, type correctness, test coverage, etc.). The assistant must never produce code edits during a review session — only analysis and findings. + + +* **Always request review from Dav1dde (David) after creating or updating PRs in the symbolic repo**: When working on PRs in the getsentry/symbolic repository, the user consistently designates Dav1dde (GitHub handle) as the reviewer. This applies both when creating new PRs and after addressing bot comments or CI failures. The user also communicates with David via Beeper DM to share PR links and discuss design decisions. After all CI checks pass and bot issues (e.g., missing CHANGELOG entry) are resolved, the user expects the reviewer request to be sent to Dav1dde. Always add Dav1dde as reviewer on symbolic repo PRs and ensure CI is green and bot comments are addressed before or alongside requesting review. + * **Always research technical approaches thoroughly before implementation**: When facing a significant technical decision or migration, the user consistently requests deep research into multiple approaches before writing any code. This includes: fetching specific upstream documentation/source files (e.g., BUILDING.md, configure.py), identifying concrete flags/options, estimating build times, and evaluating cross-compilation feasibility. The user wants tradeoffs between paths laid out explicitly. Only after research is complete does implementation begin. When presenting research, include specific flags, URLs, estimated costs (time/size), and platform constraints. - -* **Always run Biome auto-fix first before manually addressing lint errors**: Lint/typecheck workflow: run Biome auto-fix first, then manual fixes, then tsc. Always: (1) \`biome check --write\` for auto-fixable issues, (2) \`--unsafe\` if needed, (3) manually fix remaining (hoisted regexes, parameter properties, cognitive complexity >15 via helper extraction, import ordering), (4) \`tsc\` exit 0. When complexity exceeds 15, extract named helper functions — do not restructure conditionals or suppress the rule. Pre-existing errors in unrelated files are noted but not required to fix. - - -* **Always sequence npm publication before CLI integration (no vendored blobs)**: Always sequence npm publication before CLI integration (no vendored blobs): Publish upstream packages to npm first through the proper release pipeline before integrating into the CLI. Correct sequence: (1) create and merge full upstream PR including CI, build, and craft/npm targets, (2) wait for npm publish, (3) open/merge CLI PR consuming it as a proper npm dependency. CLI PR #1109 (MERGED): \`@sentry/symbolic@13.3.0\` consumed as devDependency (pinned exactly — no \`^\` or \`~\` for Sentry-scoped packages). 3-path wasm loader: (1) SEA: \`node:sea.getRawAsset(DIF\_WASM\_ASSET\_KEY)\`, (2) npm bundle: sibling \`dist/vendor/symbolic\_bg.wasm\` via \`existsSync(new URL('./vendor/symbolic\_bg.wasm', import.meta.url))\`, (3) dev: \`readFileSync(\_require.resolve(SYMBOLIC\_WASM\_SUBPATH))\`. \`@sentry/symbolic/symbolic\_bg.wasm\` marked \`external\` in esbuild (both \`build.ts\` and \`bundle.ts\`). Vendored crate \`crates/dif-wasm/\` and \`src/lib/dif/vendor/\` fully deleted. + +* **Always run the full build-npm.sh pipeline to validate WASM changes before committing**: Before committing or pushing WASM-related changes, the user consistently runs the full \`build-npm.sh\` pipeline (or \`make npm\`) end-to-end. This includes: cargo build for wasm32, wasm-bindgen, wasm-opt, and the npm smoke test suite. The user treats a passing full pipeline run as the gate for readiness — not just \`cargo test\` or \`cargo clippy\` alone. When the pipeline fails, the user investigates and fixes the root cause before proceeding. Always confirm the full pipeline passes (including smoke tests reporting N pass, 0 fail and a valid .tgz output) before treating WASM work as complete. * **Always stage all modified files before committing, not just already-staged ones**: When preparing to commit, the user reviews git status and expects ALL modified files to be staged together — not just files already in the index. If unstaged modified files exist alongside staged ones, the user treats this as an incomplete commit state that needs to be resolved before proceeding. The user reviews the full list of changed files (staged + unstaged) as a checklist against completed tasks, and expects the commit to encompass all related changes from the session as a single coherent unit. + +* **Always start new feature work on a dedicated branch off a known-good upstream commit**: When beginning a new feature or task, the user consistently creates a new branch from a specific, verified upstream commit (often a recently landed PR on main), confirms the branch point, and then begins planning or implementation. The user names branches with a personal prefix (e.g., \`byk/feat/...\`) and the branch creation is always the first recorded action of a session. The assistant should acknowledge the branch and its base commit, confirm any relevant upstream state, and then proceed to planning or implementation without prompting the user to verify the branch setup. + * **Always start with thorough codebase exploration before planning or implementing changes**: When the user wants to add a feature or make a change, they consistently begin by identifying all relevant code locations first — type definitions, default values, API layer, command files, schemas, and tests — before writing any code. They explicitly enumerate investigation targets upfront. This applies to both small changes (adding a sort value) and larger features (new commands). The assistant should treat the initial request as a research/mapping task, perform parallel searches across all relevant files, and present a comprehensive picture of the existing implementation before proposing any changes. Confirmed across many sessions including issue sort addition (June 22 2026). @@ -492,6 +543,12 @@ * **Always switch from plan mode to build mode before executing changes**: The user consistently uses a two-phase workflow: first planning (read-only exploration, writing a plan file), then explicitly approving a switch to build/agent mode before any changes are executed. When the user approves the mode switch, the assistant should immediately begin executing the existing plan file — typically by re-reading the key files to be modified. Never execute changes while still in plan mode, even if the plan is complete and approved. Wait for the explicit mode-switch approval before acting. + +* **Always track and resolve PR review findings with explicit severity labels before merging**: When a PR review surfaces issues, the user categorizes them by severity (H1/High, M1/Medium, L1/L2/L3/Low) and works through them systematically before considering the PR ready. Each finding gets a root-cause confirmation step, a targeted fix, and a commit. The user maintains an explicit task list (e.g., H1 \[completed], M1 \[completed], CI verify \[in\_progress]) and updates it as items are resolved. False claims in PR descriptions are treated as medium-severity issues requiring correction, not cosmetic cleanup. The user expects the assistant to track this task list state and report progress against it. + + +* **Always track and verify CI check results to full green before considering a PR complete**: The user consistently monitors CI pipelines until all checks reach a final state (pass/skip/neutral) with zero failures before treating a PR as done. This includes: checking rollup summaries, investigating any bot findings (Warden, Bugbot, Seer), confirming findings are either fixed or explicitly declined with rationale, and verifying follow-up PRs also reach green. The user expects the assistant to proactively report CI status, distinguish pending vs. final states, and flag any outstanding checks or unresolved findings. A PR is only considered complete when all checks are resolved and any bot review comments have been addressed. + * **Always track migration progress with explicit completion criteria and remaining blockers**: The Bun→Node migration is complete only when \`Bun.build({ compile: true })\` is replaced by fossilize in \`script/build.ts\`. As of the current session, \`script/build.ts\` already uses fossilize (\`--no-bundle\`, \`--out-dir dist-bin\`, \`--node-version lts\`) with esbuild for bundling — the migration is complete. NODE\_VERSION='lts' in build.ts. The user expects the assistant to track this state across sessions and confirm the migration is done. When resuming sessions, verify \`script/build.ts\` does not contain \`Bun.build({ compile: true })\` before declaring migration complete. @@ -501,26 +558,38 @@ * **Always verify all tasks are complete before committing, then commit with descriptive conventional commit messages**: Before committing, the user reviews a task checklist to confirm all items are completed or in-progress. They stage all relevant files, then commit with a conventional commit message (e.g., 'docs: fix stale Bun references and add systemic doc checks') that summarizes the scope of changes. The commit message reflects the primary theme of the work session. The user expects the assistant to help verify task completion status, check git status, and confirm the commit succeeds with a summary of files changed and insertions/deletions. - -* **Always verify and address all bot review findings before merging a PR**: Before merging any PR, the user requires a complete review of all automated bot findings (Bugbot, Seer, sentry-warden, cursor\[bot], etc.) and expects each finding to be either fixed or explicitly dismissed with justification. The user treats bot findings as blocking merge prerequisites, reads the full finding details carefully, investigates root causes in the actual code, applies fixes, and runs lint/typecheck/tests to confirm correctness. Only after all real issues are resolved does the user proceed with the squash-merge. Separate or unrelated concerns are deferred as follow-up items rather than blocking the current PR. - * **Always verify CI passes after pushing commits to PRs**: After pushing commits to a PR, the user consistently checks CI status and waits for all checks to go green before considering the work done. This includes checking each individual CI job (linting, tests across platforms, doc comments, etc.), investigating any failures locally to reproduce them, and pushing fixes if needed. The user treats a clean CI run as a required completion criterion for each PR push, not an optional step. When CI reveals issues (e.g., broken intra-doc links, version mismatches, merge conflicts blocking workflow triggers), the user immediately diagnoses and fixes them. CRITICAL: if CI jobs are entirely absent (not failing — just missing), check \`mergeable\`/\`mergeStateStatus\` first — merge conflicts silently suppress \`pull\_request\` workflow triggers. * **Always verify code claims against actual file contents before accepting them as true**: When evaluating PRs, documentation, or assertions about code behavior, the user systematically cross-checks every claim against the actual source files at specific line numbers. They expect the assistant to read the real files, confirm exact line locations, quote the relevant code/comments, and flag discrepancies between what is claimed and what the code actually does. The user marks confirmed findings with 🟡 (verified) and actionable directives with 🔴 (user assertion/directive). Never accept a PR description or assertion at face value — always ground-truth it against the codebase with precise line references. + +* **Always verify facts with tool results before accepting claims as correct**: The user consistently cross-checks assertions — including their own prior statements and PR descriptions — against actual tool results (changelogs, diffs, grep output, CI logs). When a claim is found to be inaccurate (e.g., 'build-binary uses Bun not setup-node', or which Node version first contained a CVE fix), the user expects it to be flagged explicitly with the correct finding. The user treats unverified claims as hypotheses, not facts, and expects the assistant to surface discrepancies rather than accept stated motivations or descriptions at face value. Apply this skepticism to PR descriptions, commit messages, and version numbers alike. + * **Always verify PR claims against actual codebase before accepting changes**: When reviewing a PR, the user consistently directs the assistant to check each stated claim against the real source files on the main branch rather than trusting the PR description or commit messages. This applies especially to documentation PRs: the user wants specific file paths, line numbers, and code excerpts cited as evidence. The user also cross-checks automated tooling (scripts, CI configs) against what they actually produce. When a PR introduces fixes, the user wants confirmation that the underlying problem genuinely existed and that the fix is correct — not just that the PR author says so. Always run the relevant check scripts and grep the codebase directly rather than reasoning from PR metadata alone. + +* **Always verify pre-existing errors are not introduced by the current change**: When typecheck or lint reveals errors, the user expects explicit confirmation that those errors existed before the current change — not just that they're unrelated. The standard method is to stash/restore changes and run the same check on the clean baseline, then confirm the errors appear identically on origin/main. This applies to tsc errors, missing modules, and environmental issues. The agent should proactively perform this verification and report it clearly (e.g., 'confirmed pre-existing on origin/main by stashing changes and running typecheck') rather than simply asserting the errors are unrelated without proof. + + +* **Always verify tests, lint, and typecheck pass before committing**: Before committing and pushing changes, the user consistently runs the full verification sequence: unit tests, lint check, and typecheck. All three must pass (green) before proceeding to commit. The user treats this as a mandatory gate — even when individual targeted tests pass, the full suite is preferred. If the full suite is too slow or times out locally, the user accepts relying on CI as a fallback, but still runs what is feasible locally first. After verification passes, the assistant should proceed directly to commit and push without waiting for additional confirmation. + + +* **Always wait for Unit Tests to complete before proceeding with PR actions**: When monitoring CI status on PRs, the user consistently treats Unit Tests as the most critical pending check. Even when most other checks (CodeQL, Lint & Typecheck, semgrep, Secret Scan, etc.) have passed, the user waits for Unit Tests to finish before taking any further action (merging, reviewing, etc.). Seer Code Review and warden are also monitored but Unit Tests is the explicit blocker. The assistant should identify Unit Tests as the primary pending check to watch, report its status prominently, and not suggest proceeding until it completes. + * **Always work around the worktree conflict error when merging to main**: When merging PRs locally, the user consistently encounters \`fatal: 'main' is already used by worktree at ...\` and expects a workaround to be applied automatically rather than treating it as a blocking error. The merge is always completed successfully despite this error (e.g., using \`gh pr merge\` via CLI or other workaround). Never stop or report failure when this specific worktree conflict appears — proceed with the merge using an alternative method and confirm the PR was merged successfully. * **Always work from a structured plan file before executing multi-step tasks**: When tackling multi-step or multi-file changes, the user consistently creates a formal plan file (e.g., \`.opencode/plans/\-\.md\`) during a planning phase before any edits are made. The plan enumerates discrete numbered tasks with priorities and target files. Execution only begins after the user explicitly approves the plan. During execution, tasks are marked in\_progress and completed sequentially. The user expects this plan-then-execute workflow to be followed strictly — no file edits during planning, and tasks tracked against the approved plan. + +* **Always wrap metric/telemetry calls in try/catch to prevent blocking error handling**: When implementing error reporting, silencing, or metric emission logic, the user requires that all metric and logger calls (e.g., \`Sentry.metrics.distribution\`, \`Sentry.logger.info\`) are wrapped in try/catch blocks so they never block or interfere with the primary error handling flow. This is enforced via code comments (e.g., 'Metric emission must never block error handling.') and applies universally to any function like \`recordSilencedError()\` that emits telemetry as a side effect of error processing. + -* **Always write tests after implementing new modules or features**: Always write tests after implementing new modules or features. Tests cover validation paths, flag combinations, error states, happy paths, and env var management. Follow established patterns from similar existing test files, use \`vi.spyOn\` for API mocking, and use the project's standard runner patterns (\`run(app, ...)\` or direct loader invocation). Test writing is a required step, not optional. Also: always read existing sibling command implementations before writing new ones to extract reusable patterns (auth settings, positional/flag definitions, error handling, shared utilities, exit code patterns). +* **Always write tests after implementing new modules or features**: After implementing a new module or integrating a feature, the user consistently adds corresponding tests — both a dedicated test file for the new module (e.g., \`semantic-display.test.ts\`) and additional tests in existing test files for integration points (e.g., new describe blocks or test cases in \`local.test.ts\`). The user also reads existing test files first to understand patterns before writing new tests. Tests are added as a required step in the todo list, not as an afterthought, and are followed by typecheck/test runs to verify correctness. * **Create dedicated branch for dependabot/security fixes tracking origin/main**: When fixing Dependabot alerts or security advisories, create a dedicated branch named \`chore/fix-\-\\` tracking origin/main (not a feature branch). Include all fix changes in a single focused commit. Push, create PR, and address CI failures immediately. This ensures security fixes are isolated from feature work and can be merged independently. Branch name example: \`chore/fix-dependabot-alerts-2026-06-23\`. @@ -534,8 +603,11 @@ * **Never uses form-data at runtime — only dev transitive dependency via @types/node-fetch**: User stated never uses form-data at runtime — only dev transitive dependency via @types/node-fetch. - -* **Prefers Bun-native APIs; use buildCommand from lib/command.js (never @stricli/core directly); use buildRouteMap from lib/route-map.js; silent catch blocks prohibited; every new src/lib/\*\*/\*.ts must start with module-level JSDoc; test isolation via useTestConfigDir(); prefer property-based and model-based tests over unit tests; DEFAULT\_NUM\_RUNS = 50; architecture tree documented; error exit code ranges: 1x=auth**: Project conventions (AGENTS.md): use \`pnpm run\`/\`pnpm install\`/\`pnpm add -D\` (NOT bun for package management); use buildCommand from lib/command.js (never @stricli/core directly); use buildRouteMap from lib/route-map.js; silent catch blocks prohibited — every catch must re-throw, log with log.debug()/log.warn(), or return fallback WITH a log.debug() call; every new src/lib/\*\*/\*.ts must start with module-level JSDoc; test isolation via useTestConfigDir(); prefer property-based and model-based tests over unit tests; DEFAULT\_NUM\_RUNS=50; error exit code ranges: 1x=auth, 2x=input/config, 3x=API/network, 4x=feature/billing, 5x=operations, 6x=command-specific. Testing: vitest + fast-check (NEVER bun:test). All packages in devDependencies (CI enforces via \`pnpm run check:deps\`). NEVER merge a PR if CI is failing unless explicitly told to ignore. Always use \`pnpm add -D \\` — never add to \`dependencies\`. - * **Prefers freshly-created location over existing**: \`installAgentSkills()\` in \`src/lib/agent-skills.ts\` returns \`results.find(location => location.created) ?? results\[0] ?? null\` — prefers freshly-created location over existing. Installs to \`~/.agents/skills/sentry-cli/\` and \`~/.claude/skills/sentry-cli/\` only. Detection: \`detectClaudeCode()\` checks \`existsSync(join(homeDir, '.claude'))\`; installer never creates top-level agent roots — presence of root dir is the detection signal. OpenCode is detected via \`OPENCODE\_CLIENT\` env var in \`detect-agent.ts\` for telemetry only — NOT for skill installation. No OpenCode skill install path exists anywhere in the repo. + + +* **Respect explicitly rejected approaches**: Behavioral pattern detected across 4 sessions (action: rejected-approach). The user consistently demonstrates this behavior. + + +* **Review code before committing**: Behavioral pattern detected across 4 sessions (action: requested-review). The user consistently demonstrates this behavior. diff --git a/src/commands/explore.ts b/src/commands/explore.ts index 322d08d9e..6e401e8f3 100644 --- a/src/commands/explore.ts +++ b/src/commands/explore.ts @@ -20,7 +20,7 @@ import { hasPreviousPage, resolveCursor, } from "../lib/db/pagination.js"; -import { ValidationError } from "../lib/errors.js"; +import { toSearchQueryError, ValidationError } from "../lib/errors.js"; import { filterFields } from "../lib/formatters/json.js"; import { buildMetaColumns } from "../lib/formatters/meta-table.js"; import { CommandOutput } from "../lib/formatters/output.js"; @@ -792,7 +792,13 @@ export const exploreCommand = buildListCommand("explore", { message: `Querying ${dataset} in ${project ? `${org}/${project}` : org}...`, json: flags.json, }, - () => config.fetch({ cursor, limit: flags.limit, timeRange }) + () => + config + .fetch({ cursor, limit: flags.limit, timeRange }) + .catch((error: unknown): never => { + // An unparseable user --query is a user input mistake, not a CLI bug. + throw toSearchQueryError(error, flags.query); + }) ); advancePaginationState(PAGINATION_KEY, contextKey, direction, nextCursor); diff --git a/src/commands/issue/list.ts b/src/commands/issue/list.ts index 7b2f679ca..94a4bcf4f 100644 --- a/src/commands/issue/list.ts +++ b/src/commands/issue/list.ts @@ -40,6 +40,7 @@ import { createDsnFingerprint } from "../../lib/dsn/index.js"; import { ApiError, ContextError, + toSearchQueryError, ValidationError, withAuthGuard, } from "../../lib/errors.js"; @@ -901,6 +902,13 @@ function enrichIssueListError( error: unknown, flags: Pick ): never { + // A user-supplied --query the server cannot parse is a user input mistake, + // not a CLI bug: surface it as a ValidationError. A 400 with no user --query + // (the CLI built a bad query) falls through and stays a reported ApiError. + const queryError = toSearchQueryError(error, flags.query); + if (queryError !== error) { + throw queryError; + } if (error instanceof ApiError) { if (error.status === 400) { throw new ApiError( @@ -1119,6 +1127,14 @@ async function handleResolvedTargets( const { error: first } = failures[0]!; const prefix = `Failed to fetch issues from ${targets.length} project(s)`; + // A user-supplied --query the server cannot parse is a user input mistake, + // not a CLI bug — surface it as a ValidationError before wrapping as a + // multi-project ApiError. CLI-authored 400s fall through and stay reported. + const queryError = toSearchQueryError(first, flags.query); + if (queryError !== first) { + throw queryError; + } + // Propagate ApiError so telemetry sees the original status code. // For 400 errors, append actionable suggestions since the user's query // or parameters are likely malformed. Common causes: invalid Sentry diff --git a/src/commands/trace/list.ts b/src/commands/trace/list.ts index 5cc4be841..061f56643 100644 --- a/src/commands/trace/list.ts +++ b/src/commands/trace/list.ts @@ -13,6 +13,7 @@ import { hasPreviousPage, resolveCursor, } from "../../lib/db/pagination.js"; +import { toSearchQueryError } from "../../lib/errors.js"; import { formatTraceTable } from "../../lib/formatters/index.js"; import { filterFields } from "../../lib/formatters/json.js"; import { CommandOutput } from "../../lib/formatters/output.js"; @@ -289,6 +290,9 @@ export const listCommand = buildListCommand("trace", { sort: flags.sort, cursor, ...timeRangeToApiParams(timeRange), + }).catch((error: unknown): never => { + // An unparseable user --query is a user input mistake, not a CLI bug. + throw toSearchQueryError(error, flags.query); }) ); diff --git a/src/lib/error-reporting.ts b/src/lib/error-reporting.ts index ac145c6e5..03f215b70 100644 --- a/src/lib/error-reporting.ts +++ b/src/lib/error-reporting.ts @@ -28,7 +28,6 @@ import { DeviceFlowError, HostScopeError, isNetworkError, - isSearchQueryParseError, OutputError, ResolutionError, SeerError, @@ -51,7 +50,6 @@ type SilenceReason = | "context_missing" | "auth_expected" | "api_user_error" - | "api_query_error" | "network_error"; /** @@ -94,14 +92,11 @@ export function classifySilenced(error: unknown): SilenceReason | null { if (error instanceof ApiError && error.status > 400 && error.status < 500) { return "api_user_error"; } - // A 400 normally signals a malformed request the CLI built (a code defect), - // so it is captured by default. The exception: when the server reports it - // could not parse the user's search query, the `--query` syntax is wrong - // (CLI-FA: ~450 users across issue/explore/trace list). That is a user input - // error, and the API already returns an actionable message, so silence it. - if (error instanceof ApiError && isSearchQueryParseError(error)) { - return "api_query_error"; - } + // A 400 (Bad Request) signals a malformed request the CLI built — a code + // defect — so it is always captured. A user's unparseable `--query` is NOT a + // 400 here: it is converted to a ValidationError at the command boundary + // (toSearchQueryError) so the user gets an actionable message and the bug + // signal for CLI-authored bad queries is preserved (CLI-FA). return null; } diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 001ab80ea..bd2ea12fc 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -780,6 +780,63 @@ export function isSearchQueryParseError(error: ApiError): boolean { ); } +/** + * Standard actionable hint shown when a user-supplied search query is rejected. + * Exported so command boundaries can reuse it alongside command-specific hints. + */ +export const SEARCH_QUERY_HELP = + "Check your --query syntax (Sentry search reference: https://docs.sentry.io/concepts/search/)"; + +/** + * Convert a Sentry search-query parse failure into a {@link ValidationError} + * when the user actually supplied the query; otherwise return the error + * unchanged. + * + * HTTP 400 means the server rejected our request as malformed, which has two + * very different causes that are only distinguishable at the command boundary + * (where `flags.query` is in scope): + * + * - **The user typed an invalid `--query`** — a user input mistake. We surface + * a clean, actionable {@link ValidationError} (correct exit code, no "CLI + * bug" upgrade nudge) instead of a raw "400 Bad Request". + * - **The CLI built the query itself** (no user `--query`) — a genuine CLI + * defect. The original {@link ApiError} (status 400) is returned untouched so + * it stays reported to Sentry as the bug it is. + * + * This is why search-query 400s are no longer special-cased in the error + * classifiers (`classifySilenced`, {@link isUserError}, `isUserApiError`): + * silencing every "Error parsing search query" 400 also hid the CLI-authored + * ones, papering over real bugs. + * + * @param error - The error thrown by a list/search API call + * @param userQuery - The user-supplied search query (`flags.query`), if any + * @param extraSuggestions - Optional command-specific hints appended after the + * standard search-syntax help + * @returns A {@link ValidationError} for user-query mistakes, else `error` + */ +export function toSearchQueryError( + error: unknown, + userQuery: string | undefined, + extraSuggestions: readonly string[] = [] +): unknown { + if ( + userQuery && + error instanceof ApiError && + isSearchQueryParseError(error) + ) { + const lines: string[] = []; + if (error.detail) { + lines.push(error.detail, ""); + } + lines.push("Suggestions:"); + for (const suggestion of [SEARCH_QUERY_HELP, ...extraSuggestions]) { + lines.push(` • ${suggestion}`); + } + return new ValidationError(lines.join("\n"), "query"); + } + return error; +} + /** * Whether an error is a raw network-level fetch failure — the CLI could not * reach the Sentry API at all (offline, DNS failure, connection @@ -822,12 +879,10 @@ export function isUserError(error: unknown): boolean { if (error.status === 0) { return true; } - // 400 usually means the CLI constructed a bad request, so it is treated as - // a CLI bug — except when the server reports the user's search query was - // unparseable, which is a user input mistake. - if (isSearchQueryParseError(error)) { - return true; - } + // 400 means the CLI constructed a bad request, which is a CLI bug — not a + // user error. (A user's unparseable `--query` is converted to a + // ValidationError at the command boundary via toSearchQueryError, so it + // never reaches here as an ApiError.) return error.status > 400 && error.status < 500; } diff --git a/src/lib/telemetry.ts b/src/lib/telemetry.ts index e29fb3f0f..c6f65c9b2 100644 --- a/src/lib/telemetry.ts +++ b/src/lib/telemetry.ts @@ -37,7 +37,7 @@ import { enrichEventWithGroupingTags, reportCliError, } from "./error-reporting.js"; -import { ApiError, isSearchQueryParseError } from "./errors.js"; +import { ApiError } from "./errors.js"; import { attachSentryReporter, logger } from "./logger.js"; import { getSentryBaseUrl, isSentrySaasUrl } from "./sentry-urls.js"; import { makeCompressedTransport } from "./telemetry/zstd-transport.js"; @@ -347,10 +347,10 @@ export function isEbadfError(event: Sentry.ErrorEvent): boolean { * Check if an error is a user-caused (401–499) API error. * * 401–499 errors are user errors — wrong issue IDs, no access, rate limited — - * not CLI bugs. 400 Bad Request is **excluded** because it usually indicates - * the CLI constructed a malformed API request, which is a code defect — except - * when the server reports the user's search query was unparseable, which is a - * user input mistake (kept in sync with `classifySilenced` / `isUserError`). + * not CLI bugs. 400 Bad Request is **excluded** because it indicates the CLI + * constructed a malformed API request, which is a code defect. (A user's + * unparseable `--query` is converted to a ValidationError at the command + * boundary via toSearchQueryError, so it never reaches here as an ApiError.) * * These should be recorded as span attributes for volume-spike detection in * Discover, but should NOT be captured as Sentry exceptions. @@ -361,9 +361,7 @@ export function isUserApiError(error: unknown): boolean { if (!(error instanceof ApiError)) { return false; } - return ( - isSearchQueryParseError(error) || (error.status > 400 && error.status < 500) - ); + return error.status > 400 && error.status < 500; } /** diff --git a/test/commands/issue/list.test.ts b/test/commands/issue/list.test.ts index 7ceb62cb9..5ec9290fc 100644 --- a/test/commands/issue/list.test.ts +++ b/test/commands/issue/list.test.ts @@ -286,6 +286,80 @@ describe("issue list: error propagation", () => { expect(apiErr.detail).toBeDefined(); } }); + + test("converts a search-query parse 400 to a ValidationError when --query is set", async () => { + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input, init); + const projectResp = mockDefaultProject(req.url); + if (projectResp) return projectResp; + if (req.url.includes("/issues/")) { + return new Response( + JSON.stringify({ + detail: "Error parsing search query: invalid status value of '403'", + }), + { status: 400 } + ); + } + return new Response(JSON.stringify([]), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }); + + const { context } = createContext(); + + try { + await func.call(context, { + limit: 10, + sort: "date", + period: parsePeriod("90d"), + json: false, + query: "is:403", + }); + expect.unreachable("Should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(ValidationError); + expect((error as ValidationError).field).toBe("query"); + expect((error as Error).message).toContain("Error parsing search query"); + } + }); + + test("keeps a search-query parse 400 as a reported ApiError when no --query is set", async () => { + // No user --query means the CLI built the bad query — a real bug that must + // stay a reported ApiError(400), not get reclassified as user input. + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input, init); + const projectResp = mockDefaultProject(req.url); + if (projectResp) return projectResp; + if (req.url.includes("/issues/")) { + return new Response( + JSON.stringify({ + detail: "Error parsing search query: invalid status value of '403'", + }), + { status: 400 } + ); + } + return new Response(JSON.stringify([]), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }); + + const { context } = createContext(); + + try { + await func.call(context, { + limit: 10, + sort: "date", + period: parsePeriod("90d"), + json: false, + }); + expect.unreachable("Should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(ApiError); + expect((error as ApiError).status).toBe(400); + } + }); }); describe("issue list: org-as-project detection", () => { diff --git a/test/lib/error-reporting.test.ts b/test/lib/error-reporting.test.ts index 75582ded8..252631daa 100644 --- a/test/lib/error-reporting.test.ts +++ b/test/lib/error-reporting.test.ts @@ -26,7 +26,6 @@ import { ConfigError, ContextError, HostScopeError, - isSearchQueryParseError, OutputError, ResolutionError, SeerError, @@ -259,7 +258,11 @@ describe("classifySilenced", () => { expect(classifySilenced(new TypeError("x is not a function"))).toBeNull(); }); - test("silences ApiError 400 with a search-query parse detail", () => { + test("does NOT silence a raw ApiError 400 with a search-query parse detail", () => { + // A user's unparseable --query is converted to a ValidationError at the + // command boundary (toSearchQueryError). A search-query 400 reaching the + // classifier therefore means the CLI built a bad query itself — a real bug + // that must stay reported (CLI-FA: stop papering over root causes). expect( classifySilenced( new ApiError( @@ -268,31 +271,16 @@ describe("classifySilenced", () => { "Error parsing search query: invalid status value of '403'" ) ) - ).toBe("api_query_error"); - }); - - test("silences a wrapped issue-list query 400 (detail prepended)", () => { - // enrichIssueListError prepends the server detail, then appends CLI hints. - const detail = - "Error parsing search query: Empty string after 'status:'\n\n" + - "Suggestions:\n • Check your --query syntax"; - expect( - classifySilenced(new ApiError("Failed to fetch issues", 400, detail)) - ).toBe("api_query_error"); + ).toBeNull(); }); - test("does NOT silence a 400 whose detail is not a query parse error", () => { + test("does NOT silence any 400 regardless of detail", () => { expect( classifySilenced( new ApiError("bad", 400, "Invalid dashboard widget configuration") ) ).toBeNull(); - }); - - test("does NOT treat a 4xx-with-query-marker as a query 400", () => { - // status must be exactly 400; other 4xx already silence via api_user_error. - const err = new ApiError("x", 422, "Error parsing search query: ..."); - expect(isSearchQueryParseError(err)).toBe(false); + expect(classifySilenced(new ApiError("bad", 400))).toBeNull(); }); test.each([ diff --git a/test/lib/errors.test.ts b/test/lib/errors.test.ts index 485235419..ba8c3bbe7 100644 --- a/test/lib/errors.test.ts +++ b/test/lib/errors.test.ts @@ -19,6 +19,7 @@ import { SeerError, stringifyUnknown, TimeoutError, + toSearchQueryError, UpgradeError, ValidationError, WizardError, @@ -507,9 +508,12 @@ describe("isUserError", () => { ], ["ApiError 400", new ApiError("bad request", 400), false], [ + // A user's unparseable --query becomes a ValidationError at the command + // boundary (toSearchQueryError); a raw search-query 400 reaching here is + // a CLI-built bad request — a CLI bug, not a user error. "ApiError 400 (search query parse)", new ApiError("bad", 400, "Error parsing search query: invalid status"), - true, + false, ], ["ApiError 401", new ApiError("unauthorized", 401), true], ["ApiError 418", new ApiError("teapot", 418), true], @@ -588,6 +592,54 @@ describe("isSearchQueryParseError", () => { }); }); +describe("toSearchQueryError", () => { + const parseError = () => + new ApiError( + "Failed to fetch issues", + 400, + "Error parsing search query: bad" + ); + + test("converts a parse 400 to a ValidationError when the user supplied --query", () => { + const result = toSearchQueryError(parseError(), "is:403"); + expect(result).toBeInstanceOf(ValidationError); + expect((result as ValidationError).field).toBe("query"); + }); + + test("includes the server detail and a search-syntax suggestion in the message", () => { + const result = toSearchQueryError( + parseError(), + "is:403" + ) as ValidationError; + expect(result.message).toContain("Error parsing search query: bad"); + expect(result.message).toContain("Sentry search reference"); + }); + + test("appends command-specific extra suggestions", () => { + const result = toSearchQueryError(parseError(), "is:403", [ + "Try a shorter time range", + ]) as ValidationError; + expect(result.message).toContain("Try a shorter time range"); + }); + + test("returns the original error untouched when the user supplied no --query", () => { + // No user query → the CLI built the bad query → keep it a reported 400. + const error = parseError(); + expect(toSearchQueryError(error, undefined)).toBe(error); + expect(toSearchQueryError(error, "")).toBe(error); + }); + + test("returns the original error for non-search-query 400s", () => { + const error = new ApiError("bad", 400, "Invalid widget configuration"); + expect(toSearchQueryError(error, "is:403")).toBe(error); + }); + + test("returns the original error for non-ApiError values", () => { + const error = new Error("boom"); + expect(toSearchQueryError(error, "is:403")).toBe(error); + }); +}); + describe("exit codes", () => { test("CliError base defaults to EXIT.GENERAL (1)", () => { expect(new CliError("err").exitCode).toBe(EXIT.GENERAL); diff --git a/test/lib/telemetry.test.ts b/test/lib/telemetry.test.ts index f493243f2..022ae513c 100644 --- a/test/lib/telemetry.test.ts +++ b/test/lib/telemetry.test.ts @@ -320,12 +320,15 @@ describe("isUserApiError", () => { expect(isUserApiError(new ApiError("Bad request", 400))).toBe(false); }); - test("returns true for 400 search-query parse error (user input)", () => { + test("returns false for a 400 search-query parse error (CLI-built bad query)", () => { + // A user's unparseable --query is converted to a ValidationError at the + // command boundary, so a search-query 400 reaching here is a CLI-built bad + // request — a CLI bug, not a user API error. expect( isUserApiError( new ApiError("bad", 400, "Error parsing search query: invalid status") ) - ).toBe(true); + ).toBe(false); }); test("returns true for 401 Unauthorized", () => { From a44094d65db6d2c2766b20a63e40b874979d30dd Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Sat, 27 Jun 2026 14:59:59 +0000 Subject: [PATCH 2/2] fix(errors): convert user --query 400s in remaining list commands Warden flagged that removing the search-query special-case from the classifiers requires every command forwarding a user --query to call toSearchQueryError, or its parse-400s get reported as CLI bugs. Wire it into the remaining commands that hit the events/logs/spans search backend (which emits "Error parsing search query"): log list, span list, event list, issue events, and trace logs. --- src/commands/event/list.ts | 5 ++++- src/commands/issue/events.ts | 5 ++++- src/commands/log/list.ts | 4 ++++ src/commands/span/list.ts | 4 ++++ src/commands/trace/logs.ts | 4 ++++ 5 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/commands/event/list.ts b/src/commands/event/list.ts index 42519dcaf..d05a50607 100644 --- a/src/commands/event/list.ts +++ b/src/commands/event/list.ts @@ -15,7 +15,7 @@ import { hasPreviousPage, resolveCursor, } from "../../lib/db/pagination.js"; -import { ContextError } from "../../lib/errors.js"; +import { ContextError, toSearchQueryError } from "../../lib/errors.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { buildListCommand, paginationHint } from "../../lib/list-command.js"; import { withProgress } from "../../lib/polling.js"; @@ -155,6 +155,9 @@ export const listCommand = buildListCommand("event", { full: flags.full, cursor, ...timeRangeToApiParams(timeRange), + }).catch((error: unknown): never => { + // An unparseable user --query is a user input mistake, not a CLI bug. + throw toSearchQueryError(error, flags.query); }) ); diff --git a/src/commands/issue/events.ts b/src/commands/issue/events.ts index eea8420ab..3b9159d0f 100644 --- a/src/commands/issue/events.ts +++ b/src/commands/issue/events.ts @@ -12,7 +12,7 @@ import { hasPreviousPage, resolveCursor, } from "../../lib/db/pagination.js"; -import { ContextError } from "../../lib/errors.js"; +import { ContextError, toSearchQueryError } from "../../lib/errors.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { buildListCommand, paginationHint } from "../../lib/list-command.js"; import { withProgress } from "../../lib/polling.js"; @@ -137,6 +137,9 @@ export const eventsCommand = buildListCommand("issue", { full: flags.full, cursor, ...timeRangeToApiParams(timeRange), + }).catch((error: unknown): never => { + // An unparseable user --query is a user input mistake, not a CLI bug. + throw toSearchQueryError(error, flags.query); }) ); diff --git a/src/commands/log/list.ts b/src/commands/log/list.ts index b9b3eb8c8..3844da5c5 100644 --- a/src/commands/log/list.ts +++ b/src/commands/log/list.ts @@ -22,6 +22,7 @@ import { import { AuthError, stringifyUnknown, + toSearchQueryError, ValidationError, } from "../../lib/errors.js"; import { @@ -180,6 +181,9 @@ async function executeSingleFetch( ...timeRangeToApiParams(timeRange), sort: flags.sort, extraFields: flags.fields, + }).catch((error: unknown): never => { + // An unparseable user --query is a user input mistake, not a CLI bug. + throw toSearchQueryError(error, flags.query); }); const periodLabel = diff --git a/src/commands/span/list.ts b/src/commands/span/list.ts index 6b6666c99..00ab8b2cf 100644 --- a/src/commands/span/list.ts +++ b/src/commands/span/list.ts @@ -20,6 +20,7 @@ import { hasPreviousPage, resolveCursor, } from "../../lib/db/pagination.js"; +import { toSearchQueryError } from "../../lib/errors.js"; import { type FlatSpan, formatSpanTable, @@ -390,6 +391,9 @@ async function handleTraceMode( ...timeRangeToApiParams(timeRange), extraFields: extraApiFields, allProjects: true, + }).catch((error: unknown): never => { + // An unparseable user --query is a user input mistake, not a CLI bug. + throw toSearchQueryError(error, flags.query); }) ); diff --git a/src/commands/trace/logs.ts b/src/commands/trace/logs.ts index 357c2e3c2..a12f33976 100644 --- a/src/commands/trace/logs.ts +++ b/src/commands/trace/logs.ts @@ -13,6 +13,7 @@ import { } from "../../lib/arg-parsing.js"; import { openInBrowser } from "../../lib/browser.js"; import { buildCommand } from "../../lib/command.js"; +import { toSearchQueryError } from "../../lib/errors.js"; import { filterFields } from "../../lib/formatters/json.js"; import { formatLogTable } from "../../lib/formatters/log.js"; import { CommandOutput, formatFooter } from "../../lib/formatters/output.js"; @@ -207,6 +208,9 @@ export const logsCommand = buildCommand({ limit: flags.limit, query, sort: flags.sort, + }).catch((error: unknown): never => { + // An unparseable user --query is a user input mistake, not a CLI bug. + throw toSearchQueryError(error, flags.query); }) );