From 62070bdf5315115e64ab9fd26a85c62b75dc76db Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 25 Jun 2026 19:41:08 +0000 Subject: [PATCH 1/4] feat(debug-files): scan inside .zip archives on upload Add ZIP archive scanning to `debug-files upload`, matching the legacy sentry-cli behavior. `.zip` files encountered during a scan are expanded in memory and their entries run through the same filter pipeline as on-disk files. Disable with `--no-zips`. - New `src/lib/dif/zip.ts`: `readZipDifEntries()` detects archives by `.zip` extension + `PK` magic, extracts entries via `fflate.unzipSync`, and bounds decompression with a pre-decompression size filter (zip-bomb guard). Directory and empty entries are dropped; nested archives are not recursed. - `prepareDifs()` gains a `scanZips` option (default true); the per-file path is extracted into `prepareFileDif()` and the parse step into the shared `difFromBuffer()` so on-disk and in-memory entries share logic. - `upload` command: add `--no-zips` flag; thread `scanZips` through. Tests: 12 zip unit tests + 2 command wiring tests. Docs/skill updated. --- docs/src/fragments/commands/debug-files.md | 12 +- package.json | 1 + .../sentry-cli/references/debug-files.md | 5 + pnpm-lock.yaml | 8 + src/commands/debug-files/upload.ts | 28 ++- src/lib/dif/scan.ts | 200 ++++++++++++++---- src/lib/dif/zip.ts | 156 ++++++++++++++ test/commands/debug-files/upload.test.ts | 38 ++++ test/lib/dif/zip.test.ts | 179 ++++++++++++++++ 9 files changed, 578 insertions(+), 49 deletions(-) create mode 100644 src/lib/dif/zip.ts create mode 100644 test/lib/dif/zip.test.ts diff --git a/docs/src/fragments/commands/debug-files.md b/docs/src/fragments/commands/debug-files.md index 82574a228..28f15855f 100644 --- a/docs/src/fragments/commands/debug-files.md +++ b/docs/src/fragments/commands/debug-files.md @@ -30,6 +30,10 @@ sentry debug-files bundle-jvm --output ./out --debug-id --json ./src sentry debug-files upload ./build sentry debug-files upload ./libexample.so --include-sources +# .zip archives are scanned in place; use --no-zips to skip them +sentry debug-files upload ./symbols.zip +sentry debug-files upload ./build --no-zips + # Restrict by type or debug id, and wait for server-side processing sentry debug-files upload ./dsyms --type dsym --wait sentry debug-files upload ./build --id --require-all @@ -60,14 +64,16 @@ sentry debug-files upload ./build --no-upload files via the chunk-upload protocol. Use `--type`/`--id` to restrict which files are sent, `--no-debug`/`--no-unwind`/`--no-sources` to drop files whose only useful feature is the named one, and `--include-sources` to attach a - source bundle per file. `--derived-data` additionally scans Xcode's + source bundle per file. `.zip` archives are scanned in place by default (their + entries run through the same filters; nested archives are not recursed) — pass + `--no-zips` to skip them. `--derived-data` additionally scans Xcode's `~/Library/Developer/Xcode/DerivedData` folder (macOS only). `--no-upload` previews the selection without credentials; `--wait`/`--wait-for` block on server-side processing and exit non-zero if any file fails. `--require-all` fails if a requested `--id` was not found. The server-advertised maximum file size and maximum processing wait are honored automatically (oversized files - are skipped with a warning). Scanning inside ZIP archives, `--symbol-maps`, - and `--il2cpp-mapping` line mappings are not yet supported. + are skipped with a warning). `--symbol-maps` (BCSymbolMap resolution) and + `--il2cpp-mapping` line mappings are not yet supported. - Upload a JVM bundle separately via `sentry debug-files upload --type jvm`. - Supported JVM source file extensions: `.java`, `.kt`, `.scala`, `.sc`, `.groovy`, `.gvy`, `.gy`, `.gsh`, `.clj`, `.cljc` diff --git a/package.json b/package.json index 58962e2f9..b941e8855 100644 --- a/package.json +++ b/package.json @@ -32,6 +32,7 @@ "consola": "^3.4.2", "esbuild": "^0.28.1", "fast-check": "^4.8.0", + "fflate": "^0.8.3", "fossilize": "^0.10.1", "hono": "^4.12.27", "http-cache-semantics": "^4.2.0", diff --git a/plugins/sentry-cli/skills/sentry-cli/references/debug-files.md b/plugins/sentry-cli/skills/sentry-cli/references/debug-files.md index 412330f72..8c67fa320 100644 --- a/plugins/sentry-cli/skills/sentry-cli/references/debug-files.md +++ b/plugins/sentry-cli/skills/sentry-cli/references/debug-files.md @@ -28,6 +28,7 @@ Upload debug information files to Sentry - `--no-sources - Do not upload files whose only feature is source info` - `--include-sources - Build and upload a source bundle for each file with debug info` - `--derived-data - Also scan Xcode's DerivedData folder (macOS only)` +- `--no-zips - Do not scan inside .zip archives` - `--no-upload - Scan and print what would be uploaded without uploading` - `--wait - Wait for server-side processing and report any errors` - `--wait-for - Wait up to this many seconds for server-side processing` @@ -81,6 +82,10 @@ sentry debug-files bundle-jvm --output ./out --debug-id --json ./src sentry debug-files upload ./build sentry debug-files upload ./libexample.so --include-sources +# .zip archives are scanned in place; use --no-zips to skip them +sentry debug-files upload ./symbols.zip +sentry debug-files upload ./build --no-zips + # Restrict by type or debug id, and wait for server-side processing sentry debug-files upload ./dsyms --type dsym --wait sentry debug-files upload ./build --id --require-all diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2e4c33bbd..f8bb795f4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -101,6 +101,9 @@ importers: fast-check: specifier: ^4.8.0 version: 4.8.0 + fflate: + specifier: ^0.8.3 + version: 0.8.3 fossilize: specifier: ^0.10.1 version: 0.10.1 @@ -1421,6 +1424,9 @@ packages: picomatch: optional: true + fflate@0.8.3: + resolution: {integrity: sha512-tbZNuJrLwGUp3zshBtdy4W+ORxZuIh8a5ilyIEQDC5rY1f3U20JMry0Ll3WBzU58EZKsEuJFXhb5gwv8CsPvgA==} + figures@6.1.0: resolution: {integrity: sha512-d+l3qxjSesT4V7v2fh+QnmFnUWv9lSpjarhShNTgBOfA0ttejbQUAlHLitbjkoRiDulW0OPoQPYIGhIC8ohejg==} engines: {node: '>=18'} @@ -3894,6 +3900,8 @@ snapshots: optionalDependencies: picomatch: 4.0.4 + fflate@0.8.3: {} + figures@6.1.0: dependencies: is-unicode-supported: 2.1.0 diff --git a/src/commands/debug-files/upload.ts b/src/commands/debug-files/upload.ts index 0ee065815..079a8e868 100644 --- a/src/commands/debug-files/upload.ts +++ b/src/commands/debug-files/upload.ts @@ -9,9 +9,10 @@ * vars, config defaults), so `--no-upload` (dry-run) needs no credentials. * * Honors the server-advertised `max_file_size` (oversized files are skipped) - * and `max_wait` (clamps the processing wait). `--derived-data` additionally - * scans Xcode's DerivedData folder on macOS. ZIP scanning, `--symbol-maps`, - * and `--il2cpp-mapping` line mappings are deferred to follow-up PRs (see the + * and `max_wait` (clamps the processing wait). `.zip` archives are scanned in + * place (disable with `--no-zips`); `--derived-data` additionally scans Xcode's + * DerivedData folder on macOS. `--symbol-maps` (BCSymbolMap resolution) and + * `--il2cpp-mapping` line mappings are deferred to follow-up PRs (see the * command's full description). */ @@ -126,6 +127,7 @@ type UploadFlags = { "no-sources"?: boolean; "include-sources"?: boolean; "derived-data"?: boolean; + "no-zips"?: boolean; "no-upload"?: boolean; wait?: boolean; "wait-for"?: number; @@ -448,16 +450,19 @@ export const uploadCommand = buildCommand({ " --id Only upload the object with the given debug id (repeatable)\n" + " --no-debug / --no-unwind / --no-sources Drop files whose only\n" + " useful feature is the named one\n" + - " --derived-data Also scan Xcode's DerivedData folder (macOS only)\n\n" + + " --derived-data Also scan Xcode's DerivedData folder (macOS only)\n" + + " --no-zips Do not scan inside .zip archives\n\n" + + ".zip archives are scanned in place by default; nested archives are not " + + "recursed.\n\n" + "Usage:\n" + " sentry debug-files upload ./build\n" + + " sentry debug-files upload ./symbols.zip\n" + " sentry debug-files upload ./libexample.so --include-sources\n" + " sentry debug-files upload ./dsyms --type dsym --wait\n" + " sentry debug-files upload --derived-data --no-upload\n" + - " sentry debug-files upload ./build --no-upload\n\n" + - "Not yet supported (planned): scanning inside ZIP archives, " + - "--symbol-maps (BCSymbolMap resolution), and --il2cpp-mapping line " + - "mappings.", + " sentry debug-files upload ./build --no-zips --no-upload\n\n" + + "Not yet supported (planned): --symbol-maps (BCSymbolMap resolution) " + + "and --il2cpp-mapping line mappings.", }, output: { human: formatUploadResult, @@ -524,6 +529,12 @@ export const uploadCommand = buildCommand({ optional: true, default: false, }, + "no-zips": { + kind: "boolean", + brief: "Do not scan inside .zip archives", + optional: true, + default: false, + }, "no-upload": { kind: "boolean", brief: "Scan and print what would be uploaded without uploading", @@ -590,6 +601,7 @@ export const uploadCommand = buildCommand({ const files = await scanPaths(scanTargets); const { prepared, oversizedCount } = await prepareDifs(files, filters, { maxFileSize, + scanZips: !flags["no-zips"], }); const difs = dedupeDifs( buildDifList(prepared, Boolean(flags["include-sources"])) diff --git a/src/lib/dif/scan.ts b/src/lib/dif/scan.ts index a36f8c835..61db2bc86 100644 --- a/src/lib/dif/scan.ts +++ b/src/lib/dif/scan.ts @@ -8,6 +8,10 @@ * separated from the I/O ({@link scanPaths}, {@link prepareDifs}) so the matching * logic can be property-tested without touching the filesystem. * + * `.zip` archives encountered during a scan are expanded in memory (see + * {@link ./zip.js}) and their entries run through the same filter pipeline, + * unless ZIP scanning is disabled via {@link PrepareDifsOptions.scanZips}. + * * Type matching maps the user-facing `--type` value to the canonical object * file format reported by the parser (e.g. `dsym`/`macho` → `macho`, * `jvm` → `sourcebundle`). This is format-level matching: `--type dsym` and @@ -25,6 +29,7 @@ import { peekFormat, selectBundledObject, } from "./index.js"; +import { readZipDifEntries } from "./zip.js"; const log = logger.withTag("dif.scan"); @@ -384,6 +389,12 @@ export type PrepareDifsOptions = { * legacy `dif_upload` `valid_size` check. */ maxFileSize?: number; + /** + * Whether to look inside `.zip` archives for debug files. Defaults to `true` + * (matching the legacy tool); set to `false` for `--no-zips`. Nested archives + * are never recursed regardless of this setting. + */ + scanZips?: boolean; }; /** Result of {@link prepareDifs}: the uploadable files plus skip telemetry. */ @@ -423,33 +434,26 @@ export async function prepareDifs( ): Promise { const prepared: PreparedDif[] = []; const maxFileSize = options.maxFileSize ?? 0; + const scanZips = options.scanZips ?? true; let oversizedCount = 0; for (const path of paths) { - const peeked = await peekHeader(path); - if (!peeked) { - continue; - } - // Apply the cheap, header-derivable `--type` (format) filter before the - // size gate. This keeps `oversizedCount` aligned with the requested type - // so an oversized file of an unrequested format never triggers an - // "all matched files too large" outcome. Per-object `--id`/feature filters - // still require a full parse and run in {@link readMatchedDif}. - if (!formatMatches(peeked.format, filters.formats)) { - continue; + // A `.zip` archive is expanded in place; its entries replace the container, + // which is itself never parsed as a DIF. `null` means "not a zip" — fall + // through to normal file handling. + if (scanZips) { + const zipResult = await prepareZipDifs(path, filters, maxFileSize); + if (zipResult) { + prepared.push(...zipResult.prepared); + oversizedCount += zipResult.oversizedCount; + continue; + } } - // Gate on size before the full read so an oversized file is never buffered. - // Only recognised debug files of a requested format reach here, so an - // oversized skip means a real, requested DIF was too large. - if (maxFileSize > 0 && peeked.size > maxFileSize) { + + const { dif, oversized } = await prepareFileDif(path, filters, maxFileSize); + if (oversized) { oversizedCount += 1; - log.warn( - `Skipping ${path}: size ${peeked.size} exceeds maximum file size ${maxFileSize}` - ); - continue; } - - const dif = await readMatchedDif(path, filters); if (dif) { prepared.push(dif); } @@ -459,26 +463,59 @@ export async function prepareDifs( } /** - * Fully read and parse a single candidate file, returning it as a - * {@link PreparedDif} when at least one contained object passes the per-object - * filters, or `null` when the file is unreadable, unparseable, empty, or has no - * matching object. Read/parse failures are logged at debug level — a scanned - * tree contains many non-object files. + * Peek, format-gate, size-gate, and (when matching) read a single on-disk + * candidate file into a {@link PreparedDif}. * - * @param path - The candidate file path (already format- and size-gated). + * The cheap, header-derivable `--type` (format) filter runs before the size + * gate so `oversized` is reported only for files of a requested format — an + * oversized file of an unrequested type never triggers an "all matched files + * too large" outcome. Per-object `--id`/feature filters require a full parse + * and run in {@link readMatchedDif}. + * + * @param path - The candidate file path. * @param filters - The resolved filter set. + * @param maxFileSize - Size gate in bytes (`0` disables it). + * @returns The prepared DIF (or `null`) and whether it was skipped for size. */ -async function readMatchedDif( +async function prepareFileDif( path: string, - filters: DifFilters -): Promise { - let content: Buffer; - try { - content = await readFile(path); - } catch (err) { - log.debug(`Skipping unreadable file: ${path}`, err); - return null; + filters: DifFilters, + maxFileSize: number +): Promise<{ dif: PreparedDif | null; oversized: boolean }> { + const peeked = await peekHeader(path); + if (!(peeked && formatMatches(peeked.format, filters.formats))) { + return { dif: null, oversized: false }; } + // Gate on size before the full read so an oversized file is never buffered. + // Only recognised debug files of a requested format reach here, so an + // oversized skip means a real, requested DIF was too large. + if (maxFileSize > 0 && peeked.size > maxFileSize) { + log.warn( + `Skipping ${path}: size ${peeked.size} exceeds maximum file size ${maxFileSize}` + ); + return { dif: null, oversized: true }; + } + return { dif: await readMatchedDif(path, filters), oversized: false }; +} + +/** + * Parse an in-memory object buffer and return it as a {@link PreparedDif} when + * at least one contained object passes the per-object filters, or `null` when + * the buffer is empty, unparseable, or has no matching object. Pure (no I/O); + * parse failures are logged at debug level. Shared by the on-disk path + * ({@link readMatchedDif}) and in-memory ZIP entries + * ({@link difFromCandidateBuffer}). + * + * @param displayPath - Path used for naming and logs (an on-disk path or a + * synthetic `"/"` path). + * @param content - The object bytes. + * @param filters - The resolved filter set. + */ +function difFromBuffer( + displayPath: string, + content: Buffer, + filters: DifFilters +): PreparedDif | null { if (content.length === 0) { return null; } @@ -487,7 +524,7 @@ async function readMatchedDif( try { archive = parseDebugFile(new Uint8Array(content)); } catch (err) { - log.debug(`Skipping unparseable file: ${path}`, err); + log.debug(`Skipping unparseable file: ${displayPath}`, err); return null; } @@ -499,9 +536,96 @@ async function readMatchedDif( } return { - path, + path: displayPath, content, debugId: selectBundledObject(matched)?.debugId, objects: matched, }; } + +/** + * Fully read and parse a single candidate file, returning it as a + * {@link PreparedDif} when at least one contained object passes the per-object + * filters, or `null` when the file is unreadable, unparseable, empty, or has no + * matching object. Read/parse failures are logged at debug level — a scanned + * tree contains many non-object files. + * + * @param path - The candidate file path (already format- and size-gated). + * @param filters - The resolved filter set. + */ +async function readMatchedDif( + path: string, + filters: DifFilters +): Promise { + let content: Buffer; + try { + content = await readFile(path); + } catch (err) { + log.debug(`Skipping unreadable file: ${path}`, err); + return null; + } + return difFromBuffer(path, content, filters); +} + +/** + * Format-gate and parse an in-memory ZIP entry into a {@link PreparedDif}. + * + * Applies the cheap, header-derivable `--type` (format) filter before invoking + * the full parser, mirroring the on-disk peek optimization so non-matching + * entries are not handed to the WASM parser. Returns `null` for unrecognised, + * non-matching, or non-object entries. + * + * @param displayPath - Synthetic `"/"` path for naming and logs. + * @param content - The decompressed entry bytes (already size-gated). + * @param filters - The resolved filter set. + */ +function difFromCandidateBuffer( + displayPath: string, + content: Buffer, + filters: DifFilters +): PreparedDif | null { + if (content.length === 0) { + return null; + } + const header = + content.length > PEEK_HEADER_BYTES + ? new Uint8Array(content.subarray(0, PEEK_HEADER_BYTES)) + : new Uint8Array(content); + const format = peekFormat(header); + if (format === "unknown" || !formatMatches(format, filters.formats)) { + return null; + } + return difFromBuffer(displayPath, content, filters); +} + +/** + * Expand a `.zip` archive at `path` into prepared debug files. + * + * Returns `null` when `path` is not a ZIP (the caller then handles it as a + * normal file). Otherwise extracts each entry (bounded by `maxFileSize`, see + * {@link readZipDifEntries}) and runs it through {@link difFromCandidateBuffer}. + * Nested archives are not recursed. + * + * @param path - The candidate path (possibly a `.zip`). + * @param filters - The resolved filter set. + * @param maxFileSize - Size gate passed through to entry extraction. + * @returns Matched entries plus oversized telemetry, or `null` when not a ZIP. + */ +async function prepareZipDifs( + path: string, + filters: DifFilters, + maxFileSize: number +): Promise<{ prepared: PreparedDif[]; oversizedCount: number } | null> { + const zip = await readZipDifEntries(path, { maxFileSize }); + if (!zip) { + return null; + } + const prepared: PreparedDif[] = []; + for (const entry of zip.entries) { + const dif = difFromCandidateBuffer(entry.path, entry.content, filters); + if (dif) { + prepared.push(dif); + } + } + return { prepared, oversizedCount: zip.oversizedCount }; +} diff --git a/src/lib/dif/zip.ts b/src/lib/dif/zip.ts new file mode 100644 index 000000000..780efd0d3 --- /dev/null +++ b/src/lib/dif/zip.ts @@ -0,0 +1,156 @@ +/** + * ZIP archive scanning for debug information files. + * + * The `debug-files upload` scanner can look inside `.zip` archives for debug + * files, matching the legacy `sentry-cli` behavior (`try_open_zip` / + * `walk_difs_zip`). A file is treated as a ZIP only when its extension is + * `.zip` and its first two bytes are the `PK` local-file-header magic; anything + * else falls back to normal file handling. + * + * Decompression is bounded to guard against zip bombs: entries whose declared + * uncompressed size exceeds the configured maximum are skipped via fflate's + * pre-decompression `filter` and never inflated. Nested archives are not + * recursed (a `.zip` inside a `.zip` is ignored), matching the legacy tool. + */ + +import { open, readFile } from "node:fs/promises"; +import { type UnzipFileInfo, unzipSync } from "fflate"; +import { logger } from "../logger.js"; + +const log = logger.withTag("dif.zip"); + +/** Local-file-header signature ("PK\x03\x04") — first two bytes are enough. */ +const ZIP_MAGIC_0 = 0x50; // 'P' +const ZIP_MAGIC_1 = 0x4b; // 'K' + +/** A debug-file candidate extracted from a ZIP archive. */ +export type ZipDifEntry = { + /** + * Synthetic display path `"/"`. `basename()` of it yields + * the entry's own base name (the DIF name), and the full string preserves the + * archive origin in logs and error messages. + */ + path: string; + /** Decompressed entry bytes. */ + content: Buffer; +}; + +/** Result of {@link readZipDifEntries}. */ +export type ReadZipResult = { + /** Decompressed, non-directory entries that passed the size gate. */ + entries: ZipDifEntry[]; + /** + * Count of entries skipped because their uncompressed size exceeded + * `maxFileSize`. This is format-agnostic: unlike the on-disk path, a + * compressed entry's container format is unknown until it is decompressed, so + * every oversized entry is counted regardless of `--type`. The count only + * feeds advisory output, never which files are uploaded. + */ + oversizedCount: number; +}; + +/** Options for {@link readZipDifEntries}. */ +export type ReadZipOptions = { + /** + * Maximum uncompressed size, in bytes, of an entry to extract. Entries above + * this are skipped without decompression. `0` or omitted means no gate. + */ + maxFileSize?: number; +}; + +/** Whether a ZIP entry name denotes a directory (trailing slash). */ +function isDirectoryEntry(name: string): boolean { + return name.endsWith("/"); +} + +/** + * Cheaply check whether a file begins with the ZIP local-file-header magic, + * without reading the whole file. + */ +async function hasZipMagic(path: string): Promise { + try { + const fd = await open(path, "r"); + try { + const buf = Buffer.alloc(2); + const { bytesRead } = await fd.read(buf, 0, 2, 0); + return ( + bytesRead === 2 && buf[0] === ZIP_MAGIC_0 && buf[1] === ZIP_MAGIC_1 + ); + } finally { + await fd.close(); + } + } catch (err) { + log.debug(`Could not read header of ${path}`, err); + return false; + } +} + +/** + * Open `path` as a ZIP archive and return its candidate debug-file entries. + * + * Returns `null` when the file is not a ZIP — its extension is not `.zip` or it + * lacks the `PK` magic — signalling the caller to handle it as a normal file. + * A malformed or unreadable archive also yields `null` (logged at debug), so + * the container is skipped rather than parsed as a debug file. + * + * Directory and empty entries are dropped. Entries whose uncompressed size + * exceeds `maxFileSize` are skipped before decompression and reflected in + * `oversizedCount`. Nested archives are not recursed. + * + * @param path - Filesystem path to inspect. + * @param options - Optional size gate (see {@link ReadZipOptions}). + * @returns Extracted entries plus oversized telemetry, or `null` when not a ZIP. + */ +export async function readZipDifEntries( + path: string, + options: ReadZipOptions = {} +): Promise { + if (!path.toLowerCase().endsWith(".zip")) { + return null; + } + if (!(await hasZipMagic(path))) { + return null; + } + + const maxFileSize = options.maxFileSize ?? 0; + let oversizedCount = 0; + + let data: Buffer; + try { + data = await readFile(path); + } catch (err) { + log.debug(`Skipping unreadable zip: ${path}`, err); + return null; + } + + // The filter runs before decompression, so oversized and directory entries + // are never inflated into memory (zip-bomb guard). + const filter = (file: UnzipFileInfo): boolean => { + if (isDirectoryEntry(file.name) || file.originalSize === 0) { + return false; + } + if (maxFileSize > 0 && file.originalSize > maxFileSize) { + oversizedCount += 1; + log.warn( + `Skipping ${path}/${file.name}: uncompressed size ${file.originalSize} exceeds maximum file size ${maxFileSize}` + ); + return false; + } + return true; + }; + + let unzipped: Record; + try { + unzipped = unzipSync(new Uint8Array(data), { filter }); + } catch (err) { + log.debug(`Skipping malformed zip: ${path}`, err); + return null; + } + + const entries: ZipDifEntry[] = []; + for (const [name, bytes] of Object.entries(unzipped)) { + entries.push({ path: `${path}/${name}`, content: Buffer.from(bytes) }); + } + + return { entries, oversizedCount }; +} diff --git a/test/commands/debug-files/upload.test.ts b/test/commands/debug-files/upload.test.ts index 7a88685f1..02ac4ba40 100644 --- a/test/commands/debug-files/upload.test.ts +++ b/test/commands/debug-files/upload.test.ts @@ -11,6 +11,7 @@ import { mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { run } from "@stricli/core"; +import { zipSync } from "fflate"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { app } from "../../../src/app.js"; import type { SentryContext } from "../../../src/context.js"; @@ -73,6 +74,16 @@ async function writeBreakpad(name = "example.sym"): Promise { return path; } +/** Write a `.zip` containing one Breakpad symbol file; return its path. */ +async function writeBreakpadZip(name = "symbols.zip"): Promise { + const path = join(tempDir, name); + await writeFile( + path, + zipSync({ "example.sym": new TextEncoder().encode(BREAKPAD_FIXTURE) }) + ); + return path; +} + /** Run `debug-files upload` and capture stdout + exit code. */ async function runUpload( args: string[] @@ -454,4 +465,31 @@ describe("sentry debug-files upload", () => { expect(spy).not.toHaveBeenCalled(); expect(exitCode).toBe(0); }); + + // ── ZIP scanning ───────────────────────────────────────────────── + + test("finds a debug file inside a .zip by default", async () => { + await writeBreakpadZip(); + const { output, exitCode } = await runUpload([ + tempDir, + "--no-upload", + "--json", + ]); + expect(exitCode).toBe(0); + const parsed = JSON.parse(output); + expect(parsed.files).toHaveLength(1); + expect(parsed.files[0].debugId).toBe(KNOWN_DEBUG_ID); + }); + + test("--no-zips ignores debug files inside archives", async () => { + await writeBreakpadZip(); + const { output, exitCode } = await runUpload([ + tempDir, + "--no-zips", + "--no-upload", + "--json", + ]); + expect(exitCode).toBe(0); + expect(JSON.parse(output).files).toHaveLength(0); + }); }); diff --git a/test/lib/dif/zip.test.ts b/test/lib/dif/zip.test.ts new file mode 100644 index 000000000..7ab6208d8 --- /dev/null +++ b/test/lib/dif/zip.test.ts @@ -0,0 +1,179 @@ +/** + * Unit tests for ZIP archive scanning in the DIF scanner. + * + * Covers {@link readZipDifEntries} directly (magic/extension detection, the + * size gate, malformed input) and the end-to-end `prepareDifs` integration + * (entries found, no nested-archive recursion, `scanZips` toggle). Archives are + * built in memory with `fflate.zipSync` so no binary fixtures are needed. + */ + +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { zipSync } from "fflate"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { + buildDifFilters, + prepareDifs, + scanPaths, +} from "../../../src/lib/dif/scan.js"; +import { readZipDifEntries } from "../../../src/lib/dif/zip.js"; + +/** A minimal, valid Breakpad symbol file with a known debug id. */ +const BREAKPAD_FIXTURE = [ + "MODULE Linux x86_64 0F13A5DA412AFBF7C8662048F3294F3D0 example", + "INFO CODE_ID DAA5130F2A41F7FBC8662048F3294F3D439CA7FF", + "FUNC 1000 10 0 main", + "1000 10 42 1", + "PUBLIC 2000 0 some_symbol", +].join("\n"); + +const BREAKPAD_BYTES = new TextEncoder().encode(BREAKPAD_FIXTURE); + +let tempDir: string; + +beforeEach(async () => { + tempDir = await mkdtemp(join(tmpdir(), "df-zip-test-")); +}); + +afterEach(async () => { + await rm(tempDir, { recursive: true, force: true }); +}); + +/** Write a ZIP built from `entries` to `name` under the temp dir, return path. */ +async function writeZip( + name: string, + entries: Record +): Promise { + const path = join(tempDir, name); + await writeFile(path, zipSync(entries)); + return path; +} + +describe("readZipDifEntries", () => { + test("returns null for a non-.zip extension", async () => { + const path = join(tempDir, "data.bin"); + await writeFile(path, zipSync({ "example.sym": BREAKPAD_BYTES })); + expect(await readZipDifEntries(path)).toBeNull(); + }); + + test("returns null for a .zip extension without PK magic", async () => { + const path = join(tempDir, "notzip.zip"); + await writeFile(path, BREAKPAD_BYTES); + expect(await readZipDifEntries(path)).toBeNull(); + }); + + test("returns null for a malformed (truncated) zip", async () => { + const path = join(tempDir, "broken.zip"); + // Valid local-header magic, then garbage — fflate throws, we skip. + await writeFile(path, Buffer.from([0x50, 0x4b, 0x03, 0x04, 0xff, 0xff])); + expect(await readZipDifEntries(path)).toBeNull(); + }); + + test("extracts entries, dropping directory placeholders", async () => { + const path = await writeZip("syms.zip", { + "dir/": new Uint8Array(0), + "example.sym": BREAKPAD_BYTES, + }); + const result = await readZipDifEntries(path); + expect(result).not.toBeNull(); + expect(result?.entries).toHaveLength(1); + expect(result?.entries[0]?.path).toBe(`${path}/example.sym`); + expect(result?.oversizedCount).toBe(0); + }); + + test("size-gates oversized entries before decompression and counts them", async () => { + const path = await writeZip("syms.zip", { + "example.sym": BREAKPAD_BYTES, + }); + const result = await readZipDifEntries(path, { maxFileSize: 1 }); + expect(result?.entries).toHaveLength(0); + expect(result?.oversizedCount).toBe(1); + }); +}); + +describe("prepareDifs ZIP scanning", () => { + test("finds a debug file inside a .zip", async () => { + const path = await writeZip("syms.zip", { + "example.sym": BREAKPAD_BYTES, + }); + const files = await scanPaths([path]); + const { prepared, oversizedCount } = await prepareDifs( + files, + buildDifFilters({}) + ); + expect(prepared).toHaveLength(1); + expect(prepared[0]?.path).toBe(`${path}/example.sym`); + expect(oversizedCount).toBe(0); + }); + + test("does not double-count the .zip container itself", async () => { + const path = await writeZip("syms.zip", { + "example.sym": BREAKPAD_BYTES, + }); + const files = await scanPaths([path]); + const { prepared } = await prepareDifs(files, buildDifFilters({})); + // Exactly the one entry — the container is never also parsed as a DIF. + expect(prepared).toHaveLength(1); + }); + + test("does not recurse into nested .zip archives", async () => { + const inner = zipSync({ "example.sym": BREAKPAD_BYTES }); + const path = await writeZip("outer.zip", { "inner.zip": inner }); + const files = await scanPaths([path]); + const { prepared } = await prepareDifs(files, buildDifFilters({})); + // The inner archive is an opaque, non-object entry — its DIF is not found. + expect(prepared).toHaveLength(0); + }); + + test("scanZips: false ignores entries inside archives", async () => { + const path = await writeZip("syms.zip", { + "example.sym": BREAKPAD_BYTES, + }); + const files = await scanPaths([path]); + const { prepared } = await prepareDifs(files, buildDifFilters({}), { + scanZips: false, + }); + expect(prepared).toHaveLength(0); + }); + + test("propagates the oversized count from skipped zip entries", async () => { + const path = await writeZip("syms.zip", { + "example.sym": BREAKPAD_BYTES, + }); + const files = await scanPaths([path]); + const { prepared, oversizedCount } = await prepareDifs( + files, + buildDifFilters({}), + { maxFileSize: 1 } + ); + expect(prepared).toHaveLength(0); + expect(oversizedCount).toBe(1); + }); + + test("falls back to normal parsing for a .zip-named non-archive", async () => { + // A Breakpad file misnamed `.zip` lacks PK magic, so it is parsed directly. + const path = join(tempDir, "misnamed.zip"); + await writeFile(path, BREAKPAD_FIXTURE); + const files = await scanPaths([path]); + const { prepared } = await prepareDifs(files, buildDifFilters({})); + expect(prepared).toHaveLength(1); + expect(prepared[0]?.path).toBe(path); + }); + + test("applies --type filter to zip entries", async () => { + const path = await writeZip("syms.zip", { + "example.sym": BREAKPAD_BYTES, + }); + const files = await scanPaths([path]); + // Breakpad entry filtered out by --type elf. + const elf = await prepareDifs(files, buildDifFilters({ types: ["elf"] })); + expect(elf.prepared).toHaveLength(0); + // ...and kept by --type breakpad. + const bp = await prepareDifs( + files, + buildDifFilters({ types: ["breakpad"] }) + ); + expect(bp.prepared).toHaveLength(1); + }); +}); From 4a41f2e32ed571b78cf134e89c744f5d54b057f2 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 25 Jun 2026 20:09:33 +0000 Subject: [PATCH 2/4] test(debug-files): assert source bundles are not expanded as zips Source bundles (and JVM bundles / .src.zip) are a ZIP preceded by an 8-byte SYSB+version header, so they start with SYSB, not PK. Verify readZipDifEntries returns null for them so they upload as sourcebundle DIFs rather than being expanded into their inner source files. --- test/lib/dif/zip.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/lib/dif/zip.test.ts b/test/lib/dif/zip.test.ts index 7ab6208d8..c7aea5a41 100644 --- a/test/lib/dif/zip.test.ts +++ b/test/lib/dif/zip.test.ts @@ -63,6 +63,21 @@ describe("readZipDifEntries", () => { expect(await readZipDifEntries(path)).toBeNull(); }); + test("does not treat a source bundle (SYSB header) as a container", async () => { + // symbolic source bundles (incl. JVM bundles, .src.zip) are a ZIP archive + // preceded by an 8-byte `SYSB`+version header, so they start with `SYSB`, + // not `PK`. They must fall through to the DIF parser and upload as-is, never + // get expanded into their inner source files. + const path = join(tempDir, "bundle.src.zip"); + const header = new Uint8Array([0x53, 0x59, 0x53, 0x42, 0x02, 0, 0, 0]); // "SYSB" + v2 + const inner = zipSync({ "example.sym": BREAKPAD_BYTES }); + await writeFile( + path, + Buffer.concat([Buffer.from(header), Buffer.from(inner)]) + ); + expect(await readZipDifEntries(path)).toBeNull(); + }); + test("returns null for a malformed (truncated) zip", async () => { const path = join(tempDir, "broken.zip"); // Valid local-header magic, then garbage — fflate throws, we skip. From d7175e2ea8e9fee647d8bb3c2e948168b9aea4eb Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 25 Jun 2026 20:49:38 +0000 Subject: [PATCH 3/4] ci(e2e): pin Node to 24.18.0 to dodge ERR_STREAM_PREMATURE_CLOSE regression Node 24.17.0 / 22.23.0 (CVE-2026-48931 http.Agent fix) added a `data` listener on idle sockets that makes keep-alive fetch reuse throw false ERR_STREAM_PREMATURE_CLOSE errors. The skill-eval E2E planner hits this as 'Premature close' talking to api.anthropic.com. Fixed in 24.18.0 (nodejs/node#64004). A floating `node-version: "24"` silently reuses the runner's pre-cached buggy 24.17.0, so pin the exact patched version. --- .github/workflows/ci.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b7b476011..fd845a6d7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -710,7 +710,13 @@ jobs: - uses: pnpm/action-setup@v4 - uses: actions/setup-node@v6 with: - node-version: "24" + # Pin exact patch: Node 24.17.0 (and 22.23.0) shipped CVE-2026-48931's + # http.Agent fix, which added a `data` listener on idle sockets that + # makes keep-alive fetch reuse throw false ERR_STREAM_PREMATURE_CLOSE. + # This surfaces as "Premature close" in the skill-eval planner's HTTP + # calls. Fixed in 24.18.0 (nodejs/node#64004). A floating "24" silently + # reuses the runner's pre-cached buggy 24.17.0, so pin the exact patch. + node-version: "24.18.0" - uses: actions/cache@v5 id: cache with: From 9153ae397fbb9133d532afa837014144c4fad1db Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 26 Jun 2026 10:00:39 +0000 Subject: [PATCH 4/4] fix(debug-files): harden zip scanning (bot + adversarial review) Addresses Seer/Bugbot/warden comments and an adversarial review of the in-place .zip scanner: - Bound cumulative decompression per archive (maxTotalSize, default 2 GiB). unzipSync materializes every accepted entry at once, so the per-entry gate alone could not stop a flat zip-bomb or a huge legit archive from exhausting memory. - Gate the container's on-disk size before reading it into memory, mirroring the on-disk peek-then-read discipline (warden M2). - Skip entries using a compression method fflate cannot inflate instead of letting unzipSync throw and discard the whole archive (and its valid sibling DIFs). - Make oversized zip entries advisory-only: they warn per entry but no longer feed the exit-driving oversizedCount. A compressed entry's format is unknown pre-decompression, so counting it produced false "all matched files too large" failures when an unrelated large asset sat inside a .zip (Bugbot/H2). - Guard non-finite originalSize as defense-in-depth (Seer). Adds regression tests for unsupported-compression survival, the cumulative budget, the container gate, and the advisory oversized accounting. --- .lore.md | 180 ++++++++++++--------------------------- src/lib/dif/scan.ts | 67 ++++++++++----- src/lib/dif/zip.ts | 154 ++++++++++++++++++++++++++------- test/lib/dif/zip.test.ts | 104 ++++++++++++++++++++-- 4 files changed, 325 insertions(+), 180 deletions(-) diff --git a/.lore.md b/.lore.md index c18a34c1d..f87035bfe 100644 --- a/.lore.md +++ b/.lore.md @@ -8,7 +8,7 @@ * **@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**: \`ObjectFile.debugSession()\` → \`DebugSession\` (PR #997, merged Jun 24 07:51Z). \`DebugSession\` wraps \`SelfCell\, ObjectDebugSession<'static>>\` using the \`derived\_from\_cell!\` macro. Required adding \`impl<'slf, 'data: 'slf> AsSelf<'slf> for ObjectDebugSession<'data>\` in \`symbolic-debuginfo/src/object.rs\` — \`ObjectDebugSession\` was the only major type missing this impl. \`files()\` returns eager \`FileEntry\[]\` (not iterators — deferred per dav1d). \`sourceByPath(path)\` returns \`SourceFileDescriptor | undefined\`. dav1d flagged potential perf issue: Rust String ↔ JS String encoding mismatch may cause excessive wasm↔JS data copying when reading \`descriptor.contents\`. +* **@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. @@ -61,6 +61,9 @@ * **generate:docs pipeline: 4-script sequence, prerequisites, and output ownership**: Master orchestrator: \`generate:docs\` runs 4 scripts in sequence: (1) \`generate:parser\` → \`script/generate-parser.ts\`, (2) \`generate:command-docs\` → \`script/generate-command-docs.ts\`, (3) \`generate:skill\` → \`script/generate-skill.ts\`, (4) \`generate:docs-sections\` → \`script/generate-docs-sections.ts\`. Prerequisite for: \`dev\`, \`build\`, \`build:all\`, \`bundle\`, \`typecheck\`, \`test:unit\`, \`test:changed\`, \`test:e2e\`. Output ownership: \`docs/src/content/docs/commands/\` and \`docs/src/content/docs/configuration.md\` are gitignored (fully generated). \`docs/src/fragments/\` files are committed source of truth (hand-written custom content). \`DEVELOPMENT.md\`, \`README.md\`, \`contributing.md\`, \`self-hosted.md\`, \`getting-started.mdx\` are committed but have in-place injected sections between named markers. + +* **getsentry/cli skill system: generate-skill.ts outputs 4 artifacts, SKILL.md is auto-generated**: \`script/generate-skill.ts\` (927 lines) generates skill files from Stricli CLI route tree introspection. Outputs: \`plugins/sentry-cli/skills/sentry-cli/SKILL.md\`, \`plugins/sentry-cli/skills/sentry-cli/references/\*.md\` (26 files), \`docs/public/.well-known/skills/index.json\`, \`src/generated/skill-content.ts\`. The \`skill-content.ts\` embeds all skill files into the binary at build time so \`agent-skills.ts\` installs without network fetching. SKILL.md is auto-generated — never edit manually; regenerate with \`pnpm run generate:docs\`. \`.cursor/skills/sentry-cli/\` contains symlinks to \`plugins/\` location. Claude Code uses \`.claude-plugin/marketplace.json\` at repo root. + * **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. @@ -88,6 +91,9 @@ * **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. + * **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. @@ -112,14 +118,11 @@ * **Decided to use job ID instead**: decided to use job ID instead. -* **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 })\`. +* **Migrated to node**: migrated from Bun to Node. * **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. - -* **pnpm overrides for cached lockfile deps need --force to re-resolve**: When adding a pnpm override for a package already resolved in lockfile (e.g. vite transitively via vitest), the override entry appears in lockfile but pnpm doesn't trigger re-resolution — the cached version remains. Fix: run \`pnpm install --force\` after adding new overrides. \`pnpm dedupe\` may time out on large monorepos. Exact version pinning (\`"vite": "8.0.16"\`) is more reliable than range syntax. Discovered while fixing vite@8.0.13→8.0.16 override. - * **Raw markdown output for non-interactive terminals, rendered for TTY**: Markdown-first output pipeline: custom renderer in \`src/lib/formatters/markdown.ts\` walks \`marked\` tokens to produce ANSI-styled output. Commands build CommonMark using helpers (\`mdKvTable()\`, \`mdRow()\`, \`colorTag()\`, \`escapeMarkdownCell()\`, \`safeCodeSpan()\`) and pass through \`renderMarkdown()\`. \`isPlainOutput()\` precedence: \`SENTRY\_PLAIN\_OUTPUT\` > \`NO\_COLOR\` > \`FORCE\_COLOR\` > \`!isTTY\`. \`--json\` always outputs JSON. Colors defined in \`COLORS\` object in \`colors.ts\`. Tests run non-TTY so assertions match raw CommonMark; use \`stripAnsi()\` helper for rendered-mode assertions. @@ -146,6 +149,9 @@ * **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. + +* **Biome cognitive complexity cap of 15 — ZIP branch in prepareDifs pushed it to 17**: Trap: adding a ZIP expansion branch to \`prepareDifs\` in \`scan.ts\` looks like a small addition. But Biome enforces a cognitive complexity cap of 15 — the ZIP branch pushed \`prepareDifs\` to 17. Fix: extract per-file processing into a private helper \`prepareFileDif(path, filters, maxFileSize)\` returning \`{ dif: PreparedDif | null; oversized: boolean }\`. Pattern: when adding conditional branches to existing complex functions, check Biome complexity score first and pre-extract helpers. + * **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. @@ -212,8 +218,14 @@ * **OOM on large non-DIF files — header peek before full read**: Trap: \`prepareDifs\` reads entire file into memory before calling \`parseDebugFile\`. Large files that aren't DIFs (e.g. videos, logs) cause OOM. Fix: call \`peekFormat\` on a small header chunk first to reject non-object files early, then only read the full file if it passes. Pattern documented in \`src/lib/dif/scan.ts\` with \`peek(16)\` + \`peekFormat(header)\`. - -* **Parallel agent sessions can commit stray .lore.md changes to feature branches**: Trap: a parallel agent session running concurrently can check out a different branch, commit a \`.lore.md\` update to the wrong branch (e.g., \`byk/feat/issue-list-recommended-sort\`), then switch away — leaving a stray commit on the feature branch. Confirmed: commit \`84b9f7cfb chore: update .lore.md\` appeared on \`byk/feat/issue-list-recommended-sort\` from a parallel agent that had checked out \`byk/feat/debug-files-new-symbolic-api\`. Before pushing or opening a PR, always verify \`git log\` and \`git diff origin/\\` to confirm no unexpected commits from parallel sessions. + +* **OpenCode memoizes skill list at session start — changes to SKILL.md not picked up until restart**: Trap: modifying \`~/.claude/skills/sentry-cli/SKILL.md\` and seeing count=6 (sentry-cli absent) in the current session looks like a parse/load failure. Fix: OpenCode's \`InstanceState.make\` caches skill discovery once per instance at session start — \`opencode debug skill\` from a fresh invocation shows the true live count (7 including sentry-cli). Stale session snapshots always show the count from when the session started. To verify skill loading, always run \`opencode debug skill\` from a new shell rather than checking \`available\_skills\` in an already-running session. + + +* **OpenCode not detected for skill installation — only .claude and .agents roots are supported**: Trap: OpenCode is detected in \`src/lib/detect-agent.ts\` via \`OPENCODE\_CLIENT\` env var and \`PROCESS\_NAME\_AGENTS\` map — looks like it should drive skill installation. But detection is for telemetry only. \`installAgentSkills()\` and \`src/commands/cli/uninstall.ts\` hardcode \`agentRoots = \['.claude', '.agents']\` — OpenCode is never a skill install target. Fix: to add OpenCode skill support, add its root dir to \`agentRoots\` in both \`agent-skills.ts\` and \`uninstall.ts\`, and add a \`detectOpenCode()\` function parallel to \`detectClaudeCode()\`. + + +* **OpenCode skill scan is session-scoped — new skill files installed mid-session are invisible until restart**: Trap: \`installAgentSkills()\` writes skill files to \`~/.claude/skills/sentry-cli/\` and the files appear on disk immediately — looks like OpenCode will pick them up. But OpenCode scans skills once per session at startup via \`InstanceState.make\` and memoizes the result for the session lifetime — no hot-reload. Fix: any session that started before the skill directory was created will show the old count forever. Users must start a new OpenCode session after \`sentry setup\` installs skills. Confirmed: \`init count=6\` appeared in every log across 2 days because \`~/.claude/skills/sentry-cli/\` didn't exist until Jun 25 19:38 UTC; \`debug skill\` immediately returned 7 in a fresh process. * **parseWithHash short-circuits before the main validateResourceId guard — must self-validate (CLI-1G1)**: GitHub-style \`org/project#SHORTID\` issue identifiers handled by \`parseWithHash()\` in \`src/lib/arg-parsing.ts\`, inserted in \`parseIssueArg\` AFTER the \`@\`-selector block and BEFORE the \`validateResourceId(input.replace(/\\//g,''))\` guard (line ~1115, which rejects \`#\`). Because it runs before that guard, \`parseWithHash\` MUST validate BOTH the project prefix AND the fragment itself. \`validateResourceId\` permits \`:\`, so \`:\` mixed with \`#\` is rejected explicitly. Semantics: \`org/project#ID\` → delegates to \`parseWithSlash('org/project/ID')\`; \`project#ID\` → \`project-search\` via \`parseProjectIdentifier\`; \`#ID\` → bare identifier via \`parseBareIssueIdentifier\`. \`parseProjectIdentifier\` is shared with \`parseWithColon\`. BEHAVIORAL CHANGE: \`CLI-G#anchor\` went from \`ValidationError\` → \`project-search{projectSlug:'cli-g', suffix:'ANCHOR'}\`. Test at \`arg-parsing.test.ts\` injection-hardening block updated accordingly. @@ -224,6 +236,9 @@ * **pnpm test runs generate:docs + generate:sdk before vitest — too slow for direct invocation**: Trap: \`pnpm test\` (or \`pnpm run test:unit\`) runs \`generate:docs && generate:sdk\` before vitest, making the full suite take minutes even for small changes — looks like a normal test command. Fix: invoke vitest directly to skip doc/SDK generation: \`vitest run test/lib test/commands test/types\`. This drops runtime from minutes to ~2-10s. Use \`pnpm run test:unit\` only when you need the generated artifacts to be fresh (e.g., testing doc generation itself). + +* **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. @@ -233,9 +248,18 @@ * **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✓. + +* **sentry-cli SKILL.md has non-standard frontmatter fields (version, requires) — safe for OpenCode but worth knowing**: SKILL.md frontmatter includes \`version\` and \`requires: {bins: \["sentry"], auth: true}\` — fields not in the OpenCode skill spec. OpenCode's \`isSkillFrontmatter()\` only validates \`name: string\` and optional \`description: string\`; extra fields are silently ignored. gray-matter (js-yaml) parses the nested \`requires\` object without error. Trap: nested objects in frontmatter look like they'd cause a YAML parse failure or schema rejection. They don't — confirmed via gray-matter test and zero "failed to load skill" log entries. The skill loads correctly; absence from a session is always a stale-snapshot issue, not a parse issue. + * **Silent nonexistent path in scan — throw ValidationError instead of skip**: Trap: \`scanPaths\` silently skipped nonexistent paths (ENOENT from \`stat\` → \`log.debug\` + continue). Users got empty results with no indication the path was missing. Fix: for explicitly provided paths (not directory children), throw \`ValidationError\` with the path name. Directory children that don't exist are still silently skipped. + +* **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;\` @@ -284,14 +308,14 @@ * **@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**: When working with \`@sentry/symbolic\` WASM in the CLI: (1) always have the latest build before testing — recompile the WASM file if changed; (2) WASM module is loaded via \`initSync({ module })\` passing bytes directly; (3) the module statically imports the generated JS glue (\`symbolic.js\`). The 3-path fallback loader: (1) SEA: \`node:sea.getRawAsset(DIF\_WASM\_ASSET\_KEY)\`, (2) npm bundle: sibling \`dist/vendor/symbolic\_bg.wasm\` via \`import.meta.url\` + \`existsSync\`, (3) dev: \`require.resolve\` from installed package. \`@sentry/symbolic@13.4.0\` introduced \`Archive\` class with \`static peek(data: Uint8Array): string | undefined\` and \`ObjectFile\` class replacing old \`parse\_debug\_file\`/\`peek\_format\` free functions. - - -* **@sentry/symbolic wasm-bindgen instances: auto-freed via FinalizationRegistry — no explicit .free() needed**: wasm-bindgen instances (\`Archive\`, \`ObjectFile\`) created in \`src/lib/dif/index.ts\` are never explicitly \`.free()\`'d — this is acceptable. \`symbolic.js\` registers \`Archive\` in its constructor (\`:45\`) and each \`ObjectFile\` in \`\_\_wrap\` (\`:136\`) with a \`FinalizationRegistry\` (\`:633-647\`) that calls \`.free()\` on GC. For a one-shot CLI that exits after parsing one file, no manual disposal is needed. Only the \`.wasm\` subpath (\`@sentry/symbolic/symbolic\_bg.wasm\`) is marked \`external\` in both esbuild configs (\`build.ts:137\`, \`bundle.ts:226\`); the JS glue exporting \`Archive\` is bundled. +* **@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()\`. + +* **atomicWriteFile in agent-skills.ts: same-dir temp + rename guarantees no partial reads**: \`atomicWriteFile(destPath, content)\` at \`src/lib/agent-skills.ts:72\`: writes to \`.\.\.\.tmp\` in the same directory as \`destPath\`, then calls \`rename()\` into place. Same-directory placement guarantees same filesystem → POSIX atomic rename. Concurrent readers never observe a truncated or partially-written file. Temp file is cleaned up on error. Used by \`writeSkillFiles()\` (replacing in-place \`writeFile\`). Skills are written on every version — write-if-changed optimization was explicitly rejected as unnecessary. + * **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. @@ -304,6 +328,9 @@ * **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). @@ -378,60 +405,33 @@ * **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 address bot review comments (Cursor Bugbot, Sentry bot) by applying fixes before merging**: When automated bots (Cursor Bugbot, Sentry/Warden) post review comments on a PR, the user consistently reads and acts on each finding — either applying a fix or explicitly acknowledging it as stale/already-addressed. The user does not dismiss bot findings as noise; even medium/low severity issues get investigated and fixed if valid. After fixing, the user typically replies to the bot comment referencing the fix commit. This applies to all bot sources: Cursor Bugbot, Sentry bot, and Warden. When multiple bots flag the same location, the user treats it as higher-priority confirmation of a real bug. - - -* **Always assert specific correctness claims about code behavior as red-line facts**: When reviewing code or discussing findings, the user makes explicit, declarative assertions (marked 🔴) about precise runtime behavior — e.g., 'never returns,' 'never reaches X,' 'reports success with exit 0,' 'that is a failure.' These are not questions or suggestions; they are authoritative statements the user expects to be treated as ground truth. When the user makes such assertions, accept them as correct without pushback, incorporate them directly into analysis or fix rationale, and ensure any proposed solution fully addresses the stated behavior. Do not hedge or re-examine these claims unless explicitly invited to. - - -* **Always check Beeper/Slack DMs to verify collaborator status before proceeding with dependent work**: When work depends on a collaborator's action (PR merge, API publish, review), the user checks Beeper chat with that collaborator to confirm the current status before starting or continuing dependent tasks. This applies especially to: verifying PR merges, confirming npm publish status, and checking whether a teammate has claimed or completed a task. The user treats Beeper DM history as the authoritative source of truth for collaboration state, not just GitHub PR status alone. Always check the relevant Beeper chat thread when the user asks about a collaborator's progress or when a blocker depends on another person's action. - -* **Always check git status after tests pass before committing**: After a full test suite run completes successfully (all tests passing), the user consistently checks git status to review all changed files before proceeding to commit. This applies at the end of a work session when tests, typecheck, and lint have all passed. The assistant should proactively 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 staging and committing. +* **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 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. - -* **Always commit and push when local tests pass, relying on CI for full test matrix**: When the full local test suite is too slow or times out, the user accepts targeted test runs (covering only the changed files/units) plus passing typecheck and lint as sufficient local verification. They then commit and push, trusting CI to run the complete test matrix. The assistant should not block on full local suite completion when targeted tests, typecheck, and lint all pass — proceed to commit and push, noting that CI will validate the rest. - * **Always compare PR branch against main before reviewing changes**: When reviewing a PR, the user consistently wants to understand exactly what changed in the PR branch versus main before diving into the content. This means fetching the remote branch if not available locally, running \`git log main..origin/\\` to see commits, and \`git diff\` (with stat) to understand the scope of changes. The user explicitly frames this as needing to know 'what changes were made vs what actually exists on main.' Always establish this baseline diff context first before analyzing or discussing PR content. - -* **Always conduct thorough feasibility analysis before implementing features with native/WASM dependencies**: When approaching features that require native tooling, binary parsing, or WASM compilation (e.g., debug-files commands), the user consistently performs deep upfront feasibility research before writing any implementation code. This includes: enumerating all technical blockers (e.g., memmap2, zstd incompatibility with wasm32), classifying subcommands by complexity tier, evaluating multiple architectural approaches side-by-side, and identifying specific constraints (WASM size, native tool availability, Node SEA environment limits). The user expects the assistant to proactively surface blockers and unknowns rather than jumping to implementation. Provide structured analysis with clear go/no-go signals per feature before proposing code. - * **Always conduct thorough PR reviews with severity-classified findings**: PR review standards: (1) Compare branch vs main first (\`git log main..origin/\\`, \`git diff --stat\`). (2) Verify every PR description claim against actual source files at specific line numbers — never trust PR metadata. (3) Classify findings as BLOCKING vs NON-BLOCKING with file paths and line numbers. (4) Flag LLM-generated planning artifacts (e.g., DOCS-AUDIT.md) as blocking violations of repo conventions. (5) Investigate root causes — check bundle output, trace esbuild variable renaming, identify silent regressions. (6) Run relevant check scripts and grep codebase directly rather than reasoning from PR metadata. - -* **Always coordinate task ownership with collaborators before starting implementation**: When working on a shared codebase with other contributors (e.g., dav1d), the user explicitly checks who is implementing what before starting work, to avoid parallel duplicate effort. They confirm ownership verbally (via chat/Beeper), claim the task, and only then begin implementation. This applies especially when a collaborator has signaled they might draft something themselves. The user also communicates task boundaries proactively — e.g., 'I'll take the debug-session wasm API so you don't need to draft it.' When coordinating, the user opens the relevant chat tool and sends a message to claim the work before branching or writing code. - - -* **Always coordinate with David (dav1d) via Beeper before proceeding on blocked tasks**: When a task is blocked on an external dependency owned by David (dav1d), the user consistently reaches out to him via Beeper (chat \`/open/557079\`) to unblock progress — sharing relevant code references, asking about API availability, or confirming implementation details. The user expects the assistant to draft or send these messages proactively, using BYK's voice, and to treat David's responses as authoritative signals for what to build next. Always use the Beeper deep-link \`/open/557079\` to open David's chat when sending messages. - * **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 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 enforce strict read-only constraints during adversarial PR code reviews**: When the user issues an adversarial code review task, they consistently impose hard read-only constraints: never modify, stage, or commit any tracked files in the working repo; never touch \`.lore.md\`; and place any throwaway test files only inside existing vitest include directories (never \`/tmp\`). They also require mutation testing directives to validate test coverage (e.g., introduce specific bugs and confirm tests fail). The review must never return an empty report. The assistant should treat these constraints as non-negotiable and apply them automatically whenever a PR review is framed as 'adversarial' or 'read-only'. - * **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. * **Always explore e2e test infrastructure thoroughly before debugging or modifying tests**: When approaching e2e test work, always explore the full infrastructure before making changes: \`test/e2e/\` (14 files: api, auth, bundle, completion, delta-upgrade, event, issue, library, log, multiregion, project, skill-eval, telemetry-exit, trace), \`test/fixture.ts\` (getCliCommand, runCli, createE2EContext), \`test/helpers.ts\` (useTestConfigDir, useEnvSandbox, resetHostScopingState, mintSntrysToken, extractFetchUrl), \`test/mocks/\` (server.ts, routes.js, multiregion.ts), \`src/bin.ts\`, \`src/cli.ts\`. Key: \`getCliCommand()\` returns \`\[SENTRY\_CLI\_BINARY]\` if set, else \`\[process.execPath, 'run', 'src/bin.ts']\`. \`createE2EContext.run()\` sets \`SENTRY\_AUTH\_TOKEN: ''\`, \`SENTRY\_TOKEN: ''\`, \`SENTRY\_CLI\_NO\_TELEMETRY: '1'\`. \`test:e2e\` runs without \`--isolate --parallel\`. Map full infrastructure before proposing fixes. - -* **Always fetch security advisories and Dependabot alerts before creating a fix plan**: The user consistently directs you to run two \`gh api\` commands: one for security advisories and one for Dependabot alerts, using headers \`Accept: application/vnd.github+json\` and \`X-GitHub-Api-Version: 2022-11-28\`. After retrieving the data, you must analyze the vulnerabilities and create a detailed plan for fixing them. This sequence recurs across multiple sessions, demonstrating a preferred workflow for data-driven security remediation planning. - - -* **Always fix Biome lint errors after implementation before considering work complete**: After implementing or modifying code, the user consistently runs Biome lint checks and expects all errors to be resolved before the task is done. This includes: running \`biome check --write\` for safe auto-fixes first, then \`--unsafe\` if needed, then manually fixing remaining issues (hoisting regexes to module scope, converting parameter properties to explicit fields, reducing cognitive complexity by extracting helpers, removing non-null assertions, fixing import ordering). The user expects a final clean Biome run (0 errors) followed by a clean TypeScript check (tsc exit 0) before marking work complete. Pre-existing errors in unrelated files are noted but not required to fix. - * **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. @@ -441,20 +441,14 @@ * **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 maintain a structured plan file before starting implementation**: Before beginning any implementation work, the user creates or references a dedicated plan file (in \`.opencode/plans/\`) that documents scope, design decisions, task breakdown, and deferred items. The plan is read and confirmed before coding starts, and may be updated (e.g., appended with roadmap tiers) during the planning phase. Only after the plan is finalized and explicitly declared complete does the user transition to implementation. Always read the plan file early in the session, confirm its contents, and treat it as the authoritative source of truth for task ordering and design decisions. - - -* **Always merge PRs despite transient CI failures when root cause is confirmed external**: When all CI failures are confirmed to be caused by external/transient issues (e.g., Anthropic outage, provider unavailability) rather than code problems, the user proceeds to merge the PR without waiting for those checks to go green. The user investigates the failure root cause first, confirms it's unrelated to the code changes, then merges — even if the merge state shows UNSTABLE due to failing checks. This applies specifically when: (1) the failing checks have no code findings, (2) the failures are clearly transient/infrastructure issues, and (3) all code-related checks pass. - -* **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 until all checks pass or fail before proceeding**: The user consistently polls and tracks CI check statuses after pushing commits or opening PRs, waiting for all checks to reach a terminal state (pass/fail/skip) before moving on. They expect the assistant to actively monitor pending checks (including external gates like \`warden\` and E2E tests), report intermediate states, and surface any bot review comments (Cursor Bugbot, Seer, Sentry bot) that appear during the CI run. Only after all checks are green (or explicitly acknowledged as skipped) does the user consider the PR ready for the next step. +* **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 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. @@ -462,62 +456,26 @@ * **Always perform thorough quality reviews of PRs distinguishing blocking vs non-blocking issues**: When reviewing PRs, the user expects a structured quality review that: (1) categorizes issues as BLOCKING vs non-blocking/low-priority, (2) verifies each claimed change against the actual codebase, (3) flags LLM-generated planning artifacts (e.g., DOCS-AUDIT.md) that violate repo conventions as blocking issues, (4) checks for missed/inconsistent changes across all affected files, and (5) confirms correct changes are working as intended. The user wants specific file paths and line numbers cited for each issue. Non-blocking issues should still be reported but clearly distinguished from blockers. - -* **Always pin @sentry/symbolic to exact published version before pushing/opening PRs**: When working with @sentry/symbolic in the CLI repo, the user consistently ensures the package.json pins the exact published npm version (e.g., '13.5.0') — not a local file: tarball — before committing, force-pushing, or opening a PR. Local tarball pins (file: specifiers) are used only during development/validation and must be replaced with the exact npm version before the final commit. If a commit was made with a local/stale pin, the user amends it to fold the version bump into the same commit rather than adding a separate bump commit. - * **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 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 provide exact code context and error details when issues arise**: When the assistant encounters a tool failure, formatting problem, or code error, the user consistently supplies exact file content, error messages, and specific directives to enable precise fixes. This includes pasting relevant code snippets, exact error output, and detailed instructions for replacement or refactoring. The user expects the assistant to use this information to accurately identify and correct issues without guessing, preferring concrete context over assumptions. - * **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 similar command implementations before writing new ones**: Before implementing a new CLI command, the user systematically reads multiple existing command files in the same command group (e.g., \`check.ts\`, \`bundle-jvm.ts\`, \`bundle-sources.ts\`) plus related utilities (\`read-file.ts\`, \`resolve-target.ts\`) to extract reusable patterns: auth settings, positional/flag definitions, error handling conventions, shared utilities, exit code patterns, and output types. Follow this pattern by surveying existing sibling commands and shared libs before generating new command code, and mirror their conventions exactly (e.g., \`auth: false\`, \`ValidationError\` usage, \`CommandOutput\` yield pattern, hint strings). - - -* **Always read existing test files before writing new tests to match established patterns**: Before writing tests for a new feature, the user reads the existing test file for the same command or a closely related command to understand the exact testing conventions in use: temp dir setup (mkdtemp/rm vs useTestConfigDir), fixture naming, how the app runner is invoked, how stdout is captured, and what assertion style is used. Apply the same patterns (same helpers, same fixture format, same run() call shape) when writing new tests rather than inventing a new structure. - * **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 record detailed post-merge state in session observations including squash commit hash, adjacent commits, and completed todo items**: After a PR is merged, the user consistently logs: (1) the squash commit hash and full commit message, (2) the names of adjacent commits on main at merge time, (3) confirmation that all todo/task items are completed with their descriptions, and (4) a technical summary of what the fix actually does. This pattern appears in session observations marked with 🟡 timestamps. When assisting with PR workflows, ensure post-merge summaries capture the squash SHA, neighboring commits on main, task completion status, and a concise fix description — not just a simple 'merged' confirmation. - - -* **Always record precise pre-edit state before making code changes**: Before any code modification, the user expects a thorough read and documentation of the exact pre-edit state: function signatures, line numbers, types, logic flow, imports, and any bugs or design decisions. This applies especially when refactoring — the user wants the assistant to capture what exists (e.g., 'Pre-edit \`loadOldBinary\` bug: ...', 'Pre-edit \`OldFileHandle\` type: ...') before proposing or applying changes. The pattern also includes noting which exports/tests reference the code being changed. This ensures the assistant has full context and can reason about regressions, API compatibility, and test impacts before touching anything. - * **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 reply to automated review bot comments after fixing the reported issue**: After fixing a Bugbot-reported issue and pushing the fix commit, the user replies directly to the Bugbot inline comment thread with a message that references the fix commit hash and briefly explains what was done. This applies to each individual Bugbot finding that was addressed. The reply should include the commit SHA, confirm the fix, and optionally summarize the resolution. Do not just push the fix silently — always close the loop by posting a reply to the specific Bugbot comment thread. - - -* **Always request READ-ONLY critical code reviews with explicit focus areas**: When asking for a code review, the user consistently specifies: (1) it is READ-ONLY — no file modifications; (2) it is 'critical' or 'objective'; (3) the exact repo path, branch, and base branch; (4) the specific files under review; (5) explicit focus areas that CI cannot catch (e.g., correctness, memory management, format compatibility, error handling, convention compliance). Always treat these reviews as advisory-only, never modify files, and structure feedback around the user-specified focus areas rather than general style or formatting concerns. - - -* **Always request research/review phases before implementation or modification**: The user consistently separates research/review from implementation. Before writing or modifying code, they explicitly request a study or review phase (e.g., 'research/review ONLY — no file modifications') to understand existing patterns, analogs, and infrastructure. They provide relevant context files and comparable implementations as reference material. Only after this research phase do they proceed to implementation. When reviewing, the assistant should read and analyze without making changes, then summarize findings, design decisions, and potential issues before any code is written. - - -* **Always require a critical self-review before merging any PR**: Before merging any PR, the user consistently requires a thorough critical self-review of both the code and the PR description, explicitly preferring a subagent for objectivity/adversarial perspective. The subagent must be read-only (no file modifications, no branch switching). The review should cover: correctness of logic, accuracy of PR description claims, edge cases, regression test adequacy, and any stale/aspirational statements. The review must always produce a report (never empty). Issues found should be fixed before merging. This is a non-negotiable pre-merge gate, not optional. - * **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 respond to adversarial/bot review findings by applying concrete fixes and re-running mutation/regression tests to verify**: When a code review (adversarial subagent, Cursor Bugbot, Seer, or similar) identifies a vacuous test, inaccurate comment, or swallowed error, the user expects: (1) the specific finding to be fixed immediately (strengthen the test assertion, correct the comment, add a tradeoff note), (2) mutation testing or targeted re-runs to confirm the fix now catches the previously-uncaught mutation, and (3) a full test+lint+typecheck pass before committing. The user also expects replies posted to bot review comments acknowledging the fix with the commit SHA. Do not defer or mark findings as 'low priority' — apply all SHOULD-FIX items before committing. - -* **Always run Biome auto-fix first before manually addressing lint errors**: When lint errors are found, the user expects the assistant to first attempt Biome's auto-fix (\`biome check --write\` or equivalent) to resolve formatting and safe-fixable errors automatically, then assess what manual fixes remain. The user does not want manual edits applied to formatting issues that Biome can handle automatically. After auto-fix, remaining errors (non-formatting issues like \`noParameterProperties\`, \`noEvolvingTypes\`, complexity violations) are addressed manually. The workflow is always: run auto-fix → verify results → fix remaining errors manually → confirm clean lint exit code 0. - - -* **Always run lint and typecheck after code changes, then fix all errors before proceeding**: After each code modification, the user consistently executes the project's typechecker (tsc), linter (Biome), and test suite (vitest) and shares the complete output with the assistant. This iterative edit-verify-fix cycle allows the assistant to catch and correct any issues—such as type errors, lint violations, or failing tests—before proceeding. The user expects the assistant to diagnose the root cause of all reported failures and apply targeted fixes, then re-run verification until all checks pass cleanly. This pattern maintains high code quality and prevents regressions throughout development. +* **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. @@ -540,42 +498,18 @@ * **Always track pre-existing failures separately from introduced regressions**: When running tests, the user consistently distinguishes between failures that existed before their changes and failures caused by their changes. They verify pre-existing failures by checking out main/stashing changes and confirming the same failures reproduce. Only new failures introduced by the current branch are treated as actionable. When reporting test results, always clarify which failures are pre-existing (with evidence) versus newly introduced, and never treat pre-existing failures as blockers for the current fix. - -* **Always use Beeper deep-link /open/557079 to contact David**: When the user needs to send a message to David (dav1d/Dav1dde), they consistently use the Beeper deep-link command \`/open/557079\` to open the DM chat. This applies whenever coordinating on API availability, confirming implementation details, sharing PR status, or asking blocking questions. The assistant should use this exact deep-link path without prompting the user for contact details or confirmation of how to reach David. - - -* **Always use specific file paths when staging git commits, never \`git add -A\`**: When staging files for a commit, the user explicitly lists individual file paths rather than using \`git add -A\` or \`git add .\`. This avoids accidentally including untracked files (e.g., stray markdown files, \`.lore.md\` conflicts) or unintended changes. When a rebase or merge goes wrong due to bulk staging, the user aborts and re-applies changes manually with targeted \`git add \\` calls. Always recommend staging by explicit file path, and warn against broad staging commands that could sweep in unrelated files. - - -* **Always validate published package artifacts via pack-install smoke tests, not just unit/wasm-pack tests**: When working on WASM/npm packages, the user consistently insists on validating the actual shipped artifact (via \`npm pack\` → install tarball → import → \`node:test\`) rather than relying on \`wasm-pack test\` or similar build-time tests. The user understands that \`wasm-pack test\` builds its own glue and never exercises the \`--target web\` \`symbolic.js\` + \`initSync\` + exports map that consumers actually load. When bugs are found (e.g., class name shadowing JS globals), the user attributes the miss to this gap and adds a packaged-artifact smoke test as a permanent fixture. Always recommend or implement pack-install smoke tests for npm/WASM packages, not just wasm-pack or unit tests. - * **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 CI passes and track PR status after each push**: After every commit push or conflict resolution, the user consistently checks CI job results (recording each job name, status, and duration), notes the overall PR merge state (mergeable, blocked, etc.), and identifies what is blocking merge (e.g., required review, pending jobs). The user tracks this information in structured session logs. When CI reveals gaps (e.g., wasm smoke test only running on release builds, not PRs), the user acts to close those gaps by adding new CI jobs. Always report CI status with job names, pass/fail, and timing after any push. - * **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 code comments that assert invariants**: The user consistently reads and evaluates code comments that make absolute claims such as 'always', 'never', or 'must'. They check whether the implementation actually upholds these claims and may flag discrepancies. During code review or implementation, the assistant should proactively identify such comments, verify their correctness, and ensure they accurately reflect the code behavior to prevent misleading future readers. - - -* **Always verify compilation after applying code changes**: After making code changes (edits, refactors, new impls, or fixes), the user consistently runs a compile/check command (e.g., \`cargo check\`, \`cargo check --target wasm32-unknown-unknown\`, \`cargo check -p \ --all-features\`) to confirm the changes compile cleanly before proceeding. If compilation fails, the user investigates the root cause, applies a fix, and re-runs the check until it passes. This applies to both library crates and wasm targets. The assistant should proactively suggest or run the appropriate compile check after any code modification, and report the result (pass/fail, duration, any warnings or errors). - - -* **Always verify fix targets by reading full file content before applying changes**: Before making any code edit, the user retrieves and reads the full file content to confirm the exact line numbers, surrounding context, and precise condition to change. This applies especially when acting on bugbot/linter findings — the user does not trust the reported line number alone but validates the actual code structure first. When proposing or applying fixes, always confirm the specific line and expression to change by referencing the retrieved file content, not just the issue description. - - -* **Always verify lint and typecheck pass after code changes before committing**: Always verify lint and typecheck pass after code changes before committing: Run Biome lint and typecheck after implementing or modifying code. When lint errors appear, verify whether pre-existing (use git stash) vs. newly introduced — fix only newly introduced issues. Expect clean lint output (0 errors) and passing tests before committing or pushing. Use \`npmx.dev\` (not npmjs.com or npmx.com) when searching for existing npm packages. - - -* **Always verify npm publish status before assuming merged code is available**: When working with packages (especially \`@sentry/symbolic\`), the user consistently checks whether a merged PR has actually been published to npm before proceeding. They inspect dist-tags, recent version lists, installed package file contents, and exported class/method surfaces to confirm the installed version matches expectations. If the needed API (e.g., \`debugSession()\`) is only on \`master\` but not yet published, they treat it as unavailable and work around it. Always check \`npm dist-tags\`, installed \`.d.ts\` exports, and actual \`node\_modules\` file contents rather than assuming merged == published. - * **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. @@ -585,14 +519,8 @@ * **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 work through a numbered task list in a single session when tackling multi-step changes**: When the user has a multi-step change (e.g., build artifact, re-pin dependency, refactor code, update types/tests, verify, lint), they establish an explicit numbered task list with priorities and statuses (in\_progress/pending) at the start of the session and work through all items sequentially in that same session. Follow this pattern by tracking task state explicitly, completing items in order, and not splitting related work across sessions unless blocked. When reporting progress, reference the task number and update its status. - -* **Always write tests after implementing new modules or features**: After implementing a new CLI command file, the user consistently requests a corresponding test file. Tests cover validation paths, flag combinations, error states, happy paths, and env var management. The user expects tests to follow established patterns from similar existing test files (e.g., proguard upload tests, check tests), use \`vi.spyOn\` for API mocking, and use the project's standard test runner patterns (\`run(app, ...)\` or direct loader invocation). The user treats test writing as a required step in the implementation workflow, not optional. - - -* **Bot review triage: distinguish real bugs from SDK-mirroring false positives**: When Sentry Seer or Cursor Bugbot flags 'unusual' code that intentionally mirrors upstream SDK behavior (e.g., \`http\_proxy\` as last-resort fallback for HTTPS URLs — deliberate in \`@sentry/node-core\` \`applyNoProxyOption\`), decline with a written rationale referencing the SDK source rather than silently changing behavior. Verify against \`node\_modules/@sentry/node-core/build/esm/transports/http.js\`, post a reply explaining the precedent, and resolve the thread. Real bugs (uppercase env var support, whitespace trimming, wildcard handling) get fixed; SDK-mirroring 'bugs' get explained and dismissed. Removing the mirror creates a divergence where users get different proxy semantics from our transport vs. the SDK default. +* **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). * **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\`. @@ -600,9 +528,6 @@ * **Follow the established git workflow (branch, PR, review)**: Behavioral pattern detected across 6 sessions (action: enforced-workflow). The user consistently demonstrates this behavior. - -* **Never push or create PRs without explicit user confirmation**: The user expects the assistant to pause before pushing branches or opening PRs, even when those are logical next steps in a workflow. The assistant should complete local work (commits, fixes, tests) and then explicitly wait for the user to say 'push' or 'open a PR' before doing so. This applies even when AGENTS.md or task lists include PR creation as a step — treat it as blocked until the user confirms. Always surface the pause point clearly so the user knows action is pending their approval. - * **Never treat incomplete operations as successful — always surface silent failures**: When reviewing code, the user explicitly asserts that incomplete operations must never report success. Examples from \`debug-files upload\`: \`not\_found\` after deadline is a failure (exit 1), not success (exit 0). Symlink cycles must not hang silently. Missing requested IDs must be surfaced. Any failure path that silently exits 0 or produces no diagnostic is a blocker. @@ -611,3 +536,6 @@ * **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. diff --git a/src/lib/dif/scan.ts b/src/lib/dif/scan.ts index 61db2bc86..06c35d961 100644 --- a/src/lib/dif/scan.ts +++ b/src/lib/dif/scan.ts @@ -29,7 +29,7 @@ import { peekFormat, selectBundledObject, } from "./index.js"; -import { readZipDifEntries } from "./zip.js"; +import { DEFAULT_MAX_ZIP_TOTAL_SIZE, readZipDifEntries } from "./zip.js"; const log = logger.withTag("dif.scan"); @@ -395,6 +395,12 @@ export type PrepareDifsOptions = { * are never recursed regardless of this setting. */ scanZips?: boolean; + /** + * Cumulative uncompressed-extraction budget per `.zip` archive (and the + * maximum container size), in bytes. Bounds peak decompression memory. + * Defaults to {@link DEFAULT_MAX_ZIP_TOTAL_SIZE}; `0` disables the budget. + */ + maxZipTotalSize?: number; }; /** Result of {@link prepareDifs}: the uploadable files plus skip telemetry. */ @@ -402,9 +408,12 @@ export type PrepareDifsResult = { /** The files to upload, each with its matched objects and primary id. */ prepared: PreparedDif[]; /** - * Number of recognised debug files skipped solely because they exceeded - * `maxFileSize`. Lets callers distinguish "nothing found" from "everything - * found was too large" so they can fail with an accurate message. + * Number of recognised, format-matched **on-disk** debug files skipped solely + * because they exceeded `maxFileSize`. Lets callers distinguish "nothing + * found" from "everything found was too large" so they can fail with an + * accurate message. Oversized entries inside `.zip` archives are deliberately + * excluded — their format is unknown until decompression, so they warn + * individually instead of driving this count. */ oversizedCount: number; }; @@ -435,17 +444,30 @@ export async function prepareDifs( const prepared: PreparedDif[] = []; const maxFileSize = options.maxFileSize ?? 0; const scanZips = options.scanZips ?? true; + const maxZipTotalSize = options.maxZipTotalSize ?? DEFAULT_MAX_ZIP_TOTAL_SIZE; let oversizedCount = 0; for (const path of paths) { // A `.zip` archive is expanded in place; its entries replace the container, - // which is itself never parsed as a DIF. `null` means "not a zip" — fall - // through to normal file handling. + // which is itself never parsed as a DIF. A non-null result (even empty) + // means the path was a `.zip` and is fully handled. `null` means "not a + // zip" — fall through to normal file handling. if (scanZips) { - const zipResult = await prepareZipDifs(path, filters, maxFileSize); - if (zipResult) { - prepared.push(...zipResult.prepared); - oversizedCount += zipResult.oversizedCount; + const zipPrepared = await prepareZipDifs( + path, + filters, + maxFileSize, + maxZipTotalSize + ); + // Oversized zip entries are advisory only — they warn per entry and + // intentionally do NOT feed `oversizedCount`. That counter gates the + // command's exit code, and a compressed entry's format is unknown until + // it is inflated, so it cannot be attributed to the requested `--type` + // the way the format-accurate on-disk gate below can. Counting them would + // turn an unrelated oversized asset inside a `.zip` into a false + // "all matched files too large" failure. + if (zipPrepared) { + prepared.push(...zipPrepared); continue; } } @@ -602,21 +624,28 @@ function difFromCandidateBuffer( * Expand a `.zip` archive at `path` into prepared debug files. * * Returns `null` when `path` is not a ZIP (the caller then handles it as a - * normal file). Otherwise extracts each entry (bounded by `maxFileSize`, see - * {@link readZipDifEntries}) and runs it through {@link difFromCandidateBuffer}. - * Nested archives are not recursed. + * normal file) or when the container is skipped wholesale (too large / malformed + * — see {@link readZipDifEntries}). Otherwise extracts each entry (bounded by + * `maxFileSize` and `maxTotalSize`) and runs it through + * {@link difFromCandidateBuffer}. Nested archives are not recursed. + * + * Oversized entries are not surfaced here: they warn per entry inside + * {@link readZipDifEntries} and are deliberately excluded from the exit-driving + * oversized count (their format is unknown pre-decompression). * * @param path - The candidate path (possibly a `.zip`). * @param filters - The resolved filter set. - * @param maxFileSize - Size gate passed through to entry extraction. - * @returns Matched entries plus oversized telemetry, or `null` when not a ZIP. + * @param maxFileSize - Per-entry size gate passed through to entry extraction. + * @param maxTotalSize - Cumulative extraction budget / container size cap. + * @returns The matched debug files (possibly empty), or `null` when not a ZIP. */ async function prepareZipDifs( path: string, filters: DifFilters, - maxFileSize: number -): Promise<{ prepared: PreparedDif[]; oversizedCount: number } | null> { - const zip = await readZipDifEntries(path, { maxFileSize }); + maxFileSize: number, + maxTotalSize: number +): Promise { + const zip = await readZipDifEntries(path, { maxFileSize, maxTotalSize }); if (!zip) { return null; } @@ -627,5 +656,5 @@ async function prepareZipDifs( prepared.push(dif); } } - return { prepared, oversizedCount: zip.oversizedCount }; + return prepared; } diff --git a/src/lib/dif/zip.ts b/src/lib/dif/zip.ts index 780efd0d3..5120b19c4 100644 --- a/src/lib/dif/zip.ts +++ b/src/lib/dif/zip.ts @@ -7,10 +7,25 @@ * `.zip` and its first two bytes are the `PK` local-file-header magic; anything * else falls back to normal file handling. * - * Decompression is bounded to guard against zip bombs: entries whose declared - * uncompressed size exceeds the configured maximum are skipped via fflate's - * pre-decompression `filter` and never inflated. Nested archives are not - * recursed (a `.zip` inside a `.zip` is ignored), matching the legacy tool. + * Decompression is bounded on three axes to guard against zip bombs and to + * cap peak memory, since `unzipSync` materializes every accepted entry at once: + * + * 1. **Container gate** — the archive is `stat`-ed before being read; a + * container whose on-disk (compressed) size already exceeds the total + * extraction budget is skipped without ever being buffered. + * 2. **Per-entry gate** — an entry whose declared uncompressed size exceeds + * `maxFileSize` is skipped via fflate's pre-decompression `filter` and + * never inflated. + * 3. **Cumulative gate** — once the running total of accepted uncompressed + * bytes would exceed `maxTotalSize`, no further entries are inflated. This + * bounds the classic flat zip-bomb (many in-range entries) that a per-entry + * gate alone cannot stop. + * + * Entries using a compression method fflate cannot inflate (anything other than + * store/deflate) are skipped rather than extracted: returning `true` for such + * an entry makes `unzipSync` throw and discards the *entire* archive, taking + * valid sibling DIFs with it. Nested archives are not recursed (a `.zip` inside + * a `.zip` is ignored), matching the legacy tool. */ import { open, readFile } from "node:fs/promises"; @@ -23,6 +38,25 @@ const log = logger.withTag("dif.zip"); const ZIP_MAGIC_0 = 0x50; // 'P' const ZIP_MAGIC_1 = 0x4b; // 'K' +/** + * ZIP compression methods fflate's `unzipSync` can inflate: store (0) and + * deflate (8). Any other method (LZMA, bzip2, zstd, AES, …) makes `unzipSync` + * throw, so such entries are filtered out before extraction. + */ +const SUPPORTED_COMPRESSION = new Set([0, 8]); + +/** + * Default cumulative uncompressed-extraction budget per archive, in bytes + * (2 GiB, mirroring `DEFAULT_MAX_DIF_SIZE`). Because `unzipSync` holds every + * accepted entry in memory simultaneously, this caps an archive's peak + * decompression footprint. It also gates the container's own on-disk size: a + * compressed archive larger than this cannot fit the budget once inflated, so + * it is skipped rather than buffered. Entries beyond the budget are skipped + * with a warning; for archives whose legitimate contents exceed it, extract + * them and scan the directory instead. + */ +export const DEFAULT_MAX_ZIP_TOTAL_SIZE = 2 * 1024 * 1024 * 1024; + /** A debug-file candidate extracted from a ZIP archive. */ export type ZipDifEntry = { /** @@ -37,14 +71,17 @@ export type ZipDifEntry = { /** Result of {@link readZipDifEntries}. */ export type ReadZipResult = { - /** Decompressed, non-directory entries that passed the size gate. */ + /** Decompressed, non-directory entries that passed the size gates. */ entries: ZipDifEntry[]; /** - * Count of entries skipped because their uncompressed size exceeded - * `maxFileSize`. This is format-agnostic: unlike the on-disk path, a - * compressed entry's container format is unknown until it is decompressed, so - * every oversized entry is counted regardless of `--type`. The count only - * feeds advisory output, never which files are uploaded. + * Advisory count of entries skipped because their uncompressed size exceeded + * `maxFileSize`. This is **format-agnostic** — a compressed entry's container + * format is unknown until it is decompressed, so every oversized entry is + * counted regardless of `--type`. It is surfaced only via per-entry warnings + * and must NOT drive the command's exit code or "all files too large" + * message; that decision uses the on-disk path's format-accurate count (see + * {@link import("./scan.js").PrepareDifsResult}). The cumulative-budget and + * container gates are not reflected here (they already warn independently). */ oversizedCount: number; }; @@ -52,10 +89,18 @@ export type ReadZipResult = { /** Options for {@link readZipDifEntries}. */ export type ReadZipOptions = { /** - * Maximum uncompressed size, in bytes, of an entry to extract. Entries above - * this are skipped without decompression. `0` or omitted means no gate. + * Maximum uncompressed size, in bytes, of a single entry to extract. Entries + * above this are skipped without decompression. `0` or omitted means no + * per-entry gate. */ maxFileSize?: number; + /** + * Cumulative uncompressed-extraction budget for the whole archive, in bytes, + * and the maximum on-disk size of the container itself. Defaults to + * {@link DEFAULT_MAX_ZIP_TOTAL_SIZE}; `0` disables both the cumulative and + * container gates. + */ + maxTotalSize?: number; }; /** Whether a ZIP entry name denotes a directory (trailing slash). */ @@ -64,24 +109,31 @@ function isDirectoryEntry(name: string): boolean { } /** - * Cheaply check whether a file begins with the ZIP local-file-header magic, - * without reading the whole file. + * Cheaply inspect a candidate without reading its body: report whether it + * begins with the ZIP local-file-header magic and its on-disk size (used by the + * container gate). A single open serves both the magic peek and the `stat`. + * + * @returns `{ isZip, size }`; `isZip` is `false` (and `size` `0`) when the file + * cannot be opened or is shorter than the 2-byte magic. */ -async function hasZipMagic(path: string): Promise { +async function peekZipContainer( + path: string +): Promise<{ isZip: boolean; size: number }> { try { const fd = await open(path, "r"); try { + const { size } = await fd.stat(); const buf = Buffer.alloc(2); const { bytesRead } = await fd.read(buf, 0, 2, 0); - return ( - bytesRead === 2 && buf[0] === ZIP_MAGIC_0 && buf[1] === ZIP_MAGIC_1 - ); + const isZip = + bytesRead === 2 && buf[0] === ZIP_MAGIC_0 && buf[1] === ZIP_MAGIC_1; + return { isZip, size }; } finally { await fd.close(); } } catch (err) { log.debug(`Could not read header of ${path}`, err); - return false; + return { isZip: false, size: 0 }; } } @@ -90,16 +142,19 @@ async function hasZipMagic(path: string): Promise { * * Returns `null` when the file is not a ZIP — its extension is not `.zip` or it * lacks the `PK` magic — signalling the caller to handle it as a normal file. - * A malformed or unreadable archive also yields `null` (logged at debug), so - * the container is skipped rather than parsed as a debug file. + * A malformed or unreadable archive, or one whose compressed size exceeds the + * total extraction budget, also yields `null` (logged), so the container is + * skipped rather than parsed as a debug file. * - * Directory and empty entries are dropped. Entries whose uncompressed size - * exceeds `maxFileSize` are skipped before decompression and reflected in - * `oversizedCount`. Nested archives are not recursed. + * Directory and empty entries are dropped, as are entries using a compression + * method fflate cannot inflate. Entries whose uncompressed size exceeds + * `maxFileSize`, or which would push cumulative extraction past `maxTotalSize`, + * are skipped before decompression. Nested archives are not recursed. * * @param path - Filesystem path to inspect. - * @param options - Optional size gate (see {@link ReadZipOptions}). - * @returns Extracted entries plus oversized telemetry, or `null` when not a ZIP. + * @param options - Optional size gates (see {@link ReadZipOptions}). + * @returns Extracted entries plus oversized telemetry, or `null` when not a ZIP + * (or when the container is skipped wholesale). */ export async function readZipDifEntries( path: string, @@ -108,12 +163,25 @@ export async function readZipDifEntries( if (!path.toLowerCase().endsWith(".zip")) { return null; } - if (!(await hasZipMagic(path))) { + + const { isZip, size } = await peekZipContainer(path); + if (!isZip) { return null; } const maxFileSize = options.maxFileSize ?? 0; - let oversizedCount = 0; + const maxTotalSize = options.maxTotalSize ?? DEFAULT_MAX_ZIP_TOTAL_SIZE; + + // Container gate: never buffer an archive whose compressed size already + // exceeds the total budget — its inflated contents cannot fit it anyway, and + // reading it would risk exhausting memory. The caller then peeks the `PK` + // header, finds no object format, and skips it without a full read. + if (maxTotalSize > 0 && size > maxTotalSize) { + log.warn( + `Skipping ${path}: archive size ${size} exceeds maximum total extraction size ${maxTotalSize}` + ); + return null; + } let data: Buffer; try { @@ -123,12 +191,29 @@ export async function readZipDifEntries( return null; } - // The filter runs before decompression, so oversized and directory entries - // are never inflated into memory (zip-bomb guard). + let oversizedCount = 0; + let acceptedTotal = 0; + + // The filter runs before decompression, so rejected entries are never + // inflated into memory (the zip-bomb / memory guard). const filter = (file: UnzipFileInfo): boolean => { if (isDirectoryEntry(file.name) || file.originalSize === 0) { return false; } + // Returning true for an entry fflate cannot inflate makes unzipSync throw, + // which would discard the whole archive and its valid siblings. Skip it. + if (!SUPPORTED_COMPRESSION.has(file.compression)) { + log.debug( + `Skipping ${path}/${file.name}: unsupported compression method ${file.compression}` + ); + return false; + } + // Defense-in-depth: fflate derives originalSize from the central directory, + // but never inflate an entry whose declared size we cannot trust. + if (!Number.isFinite(file.originalSize)) { + log.debug(`Skipping ${path}/${file.name}: unknown uncompressed size`); + return false; + } if (maxFileSize > 0 && file.originalSize > maxFileSize) { oversizedCount += 1; log.warn( @@ -136,6 +221,15 @@ export async function readZipDifEntries( ); return false; } + // Cap cumulative extraction so a flat zip-bomb of many in-range entries + // cannot inflate unbounded (unzipSync holds all accepted entries at once). + if (maxTotalSize > 0 && acceptedTotal + file.originalSize > maxTotalSize) { + log.warn( + `Skipping ${path}/${file.name}: would exceed maximum total extraction size ${maxTotalSize}` + ); + return false; + } + acceptedTotal += file.originalSize; return true; }; diff --git a/test/lib/dif/zip.test.ts b/test/lib/dif/zip.test.ts index c7aea5a41..d21860607 100644 --- a/test/lib/dif/zip.test.ts +++ b/test/lib/dif/zip.test.ts @@ -2,9 +2,12 @@ * Unit tests for ZIP archive scanning in the DIF scanner. * * Covers {@link readZipDifEntries} directly (magic/extension detection, the - * size gate, malformed input) and the end-to-end `prepareDifs` integration - * (entries found, no nested-archive recursion, `scanZips` toggle). Archives are - * built in memory with `fflate.zipSync` so no binary fixtures are needed. + * per-entry / cumulative / container size gates, unsupported-compression + * handling, malformed input) and the end-to-end `prepareDifs` integration + * (entries found, no nested-archive recursion, `scanZips` toggle, advisory + * oversized accounting). Archives are built in memory with `fflate.zipSync` + * (with a header-patching helper for unsupported compression) so no binary + * fixtures are needed. */ import { mkdtemp, rm, writeFile } from "node:fs/promises"; @@ -50,6 +53,58 @@ async function writeZip( return path; } +/** + * Overwrite the 2-byte compression-method field of `targetName` (in both its + * local file header and central directory record) with `method`, producing an + * archive fflate cannot inflate for that one entry. Offsets follow the ZIP + * spec: LFH method @+8 / fnLen @+26 / name @+30; CDH method @+10 / fnLen @+28 / + * name @+46. + */ +function corruptCompressionMethod( + zip: Uint8Array, + targetName: string, + method: number +): Uint8Array { + const out = new Uint8Array(zip); + const target = new TextEncoder().encode(targetName); + const nameMatchesAt = (start: number): boolean => { + if (start + target.length > out.length) { + return false; + } + for (let k = 0; k < target.length; k++) { + if (out[start + k] !== target[k]) { + return false; + } + } + return true; + }; + // Little-endian 16-bit read/write via arithmetic (Biome bans bitwise ops). + const readU16 = (offset: number): number => + (out[offset] ?? 0) + (out[offset + 1] ?? 0) * 256; + const setMethod = (offset: number) => { + out[offset] = method % 256; + out[offset + 1] = Math.floor(method / 256) % 256; + }; + for (let i = 0; i + 4 <= out.length; i++) { + if (out[i] !== 0x50 || out[i + 1] !== 0x4b) { + continue; + } + if (out[i + 2] === 0x03 && out[i + 3] === 0x04) { + if (readU16(i + 26) === target.length && nameMatchesAt(i + 30)) { + setMethod(i + 8); + } + } else if ( + out[i + 2] === 0x01 && + out[i + 3] === 0x02 && + readU16(i + 28) === target.length && + nameMatchesAt(i + 46) + ) { + setMethod(i + 10); + } + } + return out; +} + describe("readZipDifEntries", () => { test("returns null for a non-.zip extension", async () => { const path = join(tempDir, "data.bin"); @@ -105,6 +160,39 @@ describe("readZipDifEntries", () => { expect(result?.entries).toHaveLength(0); expect(result?.oversizedCount).toBe(1); }); + + test("skips an unsupported-compression entry without discarding the archive", async () => { + // fflate throws on any method other than store/deflate; returning it from + // the filter would discard the whole archive and its valid siblings. The + // unsupported entry must be dropped while its sibling DIF still extracts. + const raw = zipSync( + { "good.sym": BREAKPAD_BYTES, "bad.bin": new Uint8Array([1, 2, 3, 4]) }, + { level: 0 } + ); + const path = join(tempDir, "mixed.zip"); + await writeFile(path, corruptCompressionMethod(raw, "bad.bin", 99)); + const result = await readZipDifEntries(path); + expect(result).not.toBeNull(); + expect(result?.entries.map((e) => e.path)).toEqual([`${path}/good.sym`]); + }); + + test("skips a container whose compressed size exceeds the total budget", async () => { + // The whole archive is larger than 1 byte, so it is skipped wholesale + // rather than buffered into memory. + const path = await writeZip("syms.zip", { "example.sym": BREAKPAD_BYTES }); + expect(await readZipDifEntries(path, { maxTotalSize: 1 })).toBeNull(); + }); + + test("caps cumulative extraction via maxTotalSize", async () => { + // Two highly-compressible entries: the container (compressed) stays under + // the budget so it is read, but the second entry would push cumulative + // *uncompressed* extraction past it and is skipped. + const big = new Uint8Array(10_000); + const path = await writeZip("syms.zip", { "a.bin": big, "b.bin": big }); + const result = await readZipDifEntries(path, { maxTotalSize: 15_000 }); + expect(result?.entries).toHaveLength(1); + expect(result?.entries[0]?.path).toBe(`${path}/a.bin`); + }); }); describe("prepareDifs ZIP scanning", () => { @@ -152,7 +240,13 @@ describe("prepareDifs ZIP scanning", () => { expect(prepared).toHaveLength(0); }); - test("propagates the oversized count from skipped zip entries", async () => { + test("oversized zip entries are advisory and do not drive the exit count", async () => { + // A compressed entry's format is unknown until it is inflated, so an + // oversized zip entry cannot be attributed to the requested --type the way + // an on-disk file can. It is skipped (and warned per entry) but must not + // inflate `oversizedCount`, which gates the command's exit code — otherwise + // an unrelated large asset inside a .zip would cause a false + // "all matched files too large" failure. const path = await writeZip("syms.zip", { "example.sym": BREAKPAD_BYTES, }); @@ -163,7 +257,7 @@ describe("prepareDifs ZIP scanning", () => { { maxFileSize: 1 } ); expect(prepared).toHaveLength(0); - expect(oversizedCount).toBe(1); + expect(oversizedCount).toBe(0); }); test("falls back to normal parsing for a .zip-named non-archive", async () => {