From 9bf38cefe0f4728d67964a2b9b22e20f2fd4e4e7 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 24 Apr 2026 18:41:07 +0000 Subject: [PATCH 1/6] fix(sourcemap): error on zero pairs + restore docs sourcemap emission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related bugs caused Sentry events for the docs site (cli.sentry.dev) to ship unsymbolicated in production, with no CI signal: 1. **Docs build emitted no .map files.** The `astro.config.mjs` sets `vite.build.sourcemap: "hidden"`, but Astro 6 reads that from `vite.environments.{client,ssr}.build.sourcemap` (Vite 7's Environments API — see `astro/dist/core/build/static-build.js:281`), falling back to `false` if unset. The legacy top-level path is ignored for the client bundle. Fix: set the flag on both environments. 2. **`sentry sourcemap inject/upload` silently succeeded on zero pairs.** When the bundler didn't emit `.map` files, `sourcemap upload` would print "Files uploaded: 0" and exit 0, hiding the misconfiguration. Add a default-strict behavior: throw `ValidationError` when no JS + sourcemap pairs are discovered, with `--allow-empty` as the escape hatch for callers that legitimately invoke upload on potentially-empty directories. Applies symmetrically to `inject` — a zero-discovery inject almost always means a missing-.map misconfiguration, distinct from the idempotent "0 injected, N skipped" re-run case which still succeeds. Docs/CI workflows don't need changes: the default-strict behavior catches the regression automatically. Verified locally: after fix (1), `docs/dist/_astro/` contains 11 `.map` files and `sentry sourcemap inject docs/dist/` injects debug IDs into 5 JS chunks (matching the number of module chunks that have companion maps — the remaining 6 `.map` files are split-chunk variants without a primary JS counterpart, expected). The CLI binary build already uploads sourcemaps correctly per target (see `script/build.ts#uploadSourcemapToSentry`) — this was only a docs-site regression. Ref: CLI binary zstd compression (#823) couldn't be evaluated via docs uploads because 0 bytes were ever being sent; the CLI binary path has been exercising zstd since PR #823 merged. --- docs/astro.config.mjs | 12 +- .../skills/sentry-cli/references/sourcemap.md | 2 + src/commands/sourcemap/inject.ts | 24 +- src/commands/sourcemap/upload.ts | 29 ++- test/commands/sourcemap/upload.test.ts | 212 ++++++++++++++++++ 5 files changed, 274 insertions(+), 5 deletions(-) create mode 100644 test/commands/sourcemap/upload.test.ts diff --git a/docs/astro.config.mjs b/docs/astro.config.mjs index 8fe3ce518..8acaefb06 100644 --- a/docs/astro.config.mjs +++ b/docs/astro.config.mjs @@ -14,9 +14,17 @@ export default defineConfig({ // Generate sourcemaps for Sentry. "hidden" produces .map files without // adding //# sourceMappingURL comments to the output (the debug IDs // injected post-build by `sentry sourcemap inject` are used instead). + // + // Astro 6 uses Vite 7's Environments API and reads `sourcemap` from + // `vite.environments.{client,ssr}.build.sourcemap` (see + // astro/dist/core/build/static-build.js), falling back to `false` — NOT + // to legacy top-level `vite.build.sourcemap`. We set it on both + // environments to cover both the client bundle (what `sentry sourcemap + // inject/upload` operates on) and SSR output. vite: { - build: { - sourcemap: "hidden", + environments: { + client: { build: { sourcemap: "hidden" } }, + ssr: { build: { sourcemap: "hidden" } }, }, }, integrations: [ diff --git a/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md b/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md index 3907d68cb..c4dcf9049 100644 --- a/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md +++ b/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md @@ -18,6 +18,7 @@ Inject debug IDs into JavaScript files and sourcemaps **Flags:** - `--ext - Comma-separated file extensions to process (default: .js,.cjs,.mjs)` - `--dry-run - Show what would be modified without writing` +- `--allow-empty - Exit successfully when no JS + sourcemap pairs are discovered (default: error out to catch silent build misconfigurations)` **Examples:** @@ -39,6 +40,7 @@ Upload sourcemaps to Sentry **Flags:** - `--release - Release version to associate with the upload` - `--url-prefix - URL prefix for uploaded files (default: ~/) - (default: "~/")` +- `--allow-empty - Exit successfully when no sourcemap pairs are found (default: error out to catch silent build misconfigurations)` **Examples:** diff --git a/src/commands/sourcemap/inject.ts b/src/commands/sourcemap/inject.ts index 282de2c1b..8cd15405b 100644 --- a/src/commands/sourcemap/inject.ts +++ b/src/commands/sourcemap/inject.ts @@ -7,6 +7,7 @@ import type { SentryContext } from "../../context.js"; import { buildCommand } from "../../lib/command.js"; +import { ValidationError } from "../../lib/errors.js"; import { colorTag, mdKvTable, @@ -88,11 +89,19 @@ export const injectCommand = buildCommand({ optional: true, default: false, }, + "allow-empty": { + kind: "boolean", + brief: + "Exit successfully when no JS + sourcemap pairs are discovered " + + "(default: error out to catch silent build misconfigurations)", + optional: true, + default: false, + }, }, }, async *func( this: SentryContext, - flags: { ext?: string; "dry-run"?: boolean }, + flags: { ext?: string; "dry-run"?: boolean; "allow-empty"?: boolean }, dir: string ) { const extensions = flags.ext?.split(",").map((e) => e.trim()); @@ -101,6 +110,19 @@ export const injectCommand = buildCommand({ dryRun: flags["dry-run"], }); + // Guard against silent misconfigurations: zero *discovered* pairs almost + // always means the bundler didn't emit .map files. This is distinct from + // zero *injected* (which is legitimate when every pair already has a + // debug ID — the idempotent re-run case). Callers that legitimately + // invoke inject on potentially-empty directories can pass --allow-empty. + if (results.length === 0 && !flags["allow-empty"]) { + throw new ValidationError( + `No JS + sourcemap pairs found in '${dir}'. Ensure your bundler ` + + "emits sourcemaps (e.g., Vite: `build.sourcemap: 'hidden'`), or " + + "pass --allow-empty to suppress this error." + ); + } + const modified = results.filter((r) => r.injected).length; const skipped = results.length - modified; diff --git a/src/commands/sourcemap/upload.ts b/src/commands/sourcemap/upload.ts index 84af8fbbc..cbc27fd9c 100644 --- a/src/commands/sourcemap/upload.ts +++ b/src/commands/sourcemap/upload.ts @@ -13,7 +13,7 @@ import { uploadSourcemaps, } from "../../lib/api/sourcemaps.js"; import { buildCommand } from "../../lib/command.js"; -import { ContextError } from "../../lib/errors.js"; +import { ContextError, ValidationError } from "../../lib/errors.js"; import { mdKvTable, renderMarkdown } from "../../lib/formatters/markdown.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { resolveOrgAndProject } from "../../lib/resolve-target.js"; @@ -87,11 +87,23 @@ export const uploadCommand = buildCommand({ optional: true, default: "~/", }, + "allow-empty": { + kind: "boolean", + brief: + "Exit successfully when no sourcemap pairs are found (default: " + + "error out to catch silent build misconfigurations)", + optional: true, + default: false, + }, }, }, async *func( this: SentryContext, - flags: { release?: string; "url-prefix"?: string }, + flags: { + release?: string; + "url-prefix"?: string; + "allow-empty"?: boolean; + }, dir: string ) { // Resolve org/project via the standard cascade @@ -108,6 +120,19 @@ export const uploadCommand = buildCommand({ const results = await injectDirectory(dir); if (results.length === 0) { + // Silent misconfigurations (e.g., the bundler didn't emit .map files) + // used to succeed with "0 uploaded". That makes post-deploy Sentry + // events unsymbolicated with no build-time signal. Default to erroring + // out so CI fails loudly; `--allow-empty` preserves the old behavior + // for callers that legitimately invoke upload on potentially-empty + // directories. + if (!flags["allow-empty"]) { + throw new ValidationError( + `No JS + sourcemap pairs found in '${dir}'. Ensure your bundler ` + + "emits sourcemaps (e.g., Vite: `build.sourcemap: 'hidden'`), or " + + "pass --allow-empty to suppress this error." + ); + } yield new CommandOutput({ org, project, diff --git a/test/commands/sourcemap/upload.test.ts b/test/commands/sourcemap/upload.test.ts new file mode 100644 index 000000000..7947a978e --- /dev/null +++ b/test/commands/sourcemap/upload.test.ts @@ -0,0 +1,212 @@ +/** + * sourcemap inject / upload command tests + * + * Focused on the "strict by default, --allow-empty opt-out" behavior added + * to guard against silent bundler misconfigurations where no .map files are + * present in the upload directory. A zero-file upload used to succeed + * silently, producing no Sentry symbolication and no CI signal — the exact + * failure mode the getsentry/cli docs site hit (see PR #xxx). + */ + +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { injectCommand } from "../../../src/commands/sourcemap/inject.js"; +import { uploadCommand } from "../../../src/commands/sourcemap/upload.js"; +import { ValidationError } from "../../../src/lib/errors.js"; + +// The loader() return is the wrapper-produced async function (NOT a +// generator) that internally iterates the original generator body and +// writes rendered output to ctx.stdout. Errors thrown inside the +// generator body propagate out as a rejected promise. +type InjectFuncArgs = { + ext?: string; + "dry-run"?: boolean; + "allow-empty"?: boolean; +}; +type UploadFuncArgs = { + release?: string; + "url-prefix"?: string; + "allow-empty"?: boolean; +}; +type CmdFunc = (this: unknown, flags: A, dir: string) => Promise; + +function makeContext() { + const stdoutChunks: string[] = []; + const stderrChunks: string[] = []; + return { + ctx: { + stdout: { + write: mock((s: string) => { + stdoutChunks.push(s); + }), + }, + stderr: { + write: mock((s: string) => { + stderrChunks.push(s); + }), + }, + cwd: "/tmp", + process: { + stdout: { + write: mock((s: string) => { + stdoutChunks.push(s); + }), + }, + stderr: { + write: mock((s: string) => { + stderrChunks.push(s); + }), + }, + }, + }, + stdout: () => stdoutChunks.join(""), + stderr: () => stderrChunks.join(""), + }; +} + +async function runFunc( + func: CmdFunc, + ctx: unknown, + flags: A, + dir: string +): Promise { + await func.call(ctx, flags, dir); +} + +describe("sourcemap inject command — --allow-empty behavior", () => { + let dir: string; + let func: CmdFunc; + + beforeEach(async () => { + dir = mkdtempSync(join(tmpdir(), "sentry-inject-cmd-")); + func = (await injectCommand.loader()) as unknown as CmdFunc; + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + test("empty directory: throws ValidationError by default", async () => { + const { ctx } = makeContext(); + await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( + ValidationError + ); + }); + + test("empty directory: error message mentions the directory and escape hatch", async () => { + const { ctx } = makeContext(); + try { + await runFunc(func, ctx, {}, dir); + expect.unreachable("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(ValidationError); + const msg = (err as Error).message; + expect(msg).toContain(dir); + expect(msg).toContain("--allow-empty"); + } + }); + + test("empty directory with --allow-empty: succeeds silently", async () => { + const { ctx } = makeContext(); + await expect( + runFunc(func, ctx, { "allow-empty": true }, dir) + ).resolves.toBeUndefined(); + }); + + test("directory with a .js + .map pair: succeeds (0 pairs guard not triggered)", async () => { + writeFileSync(join(dir, "app.js"), "console.log(1)\n"); + writeFileSync(join(dir, "app.js.map"), '{"version":3}\n'); + const { ctx } = makeContext(); + await expect(runFunc(func, ctx, {}, dir)).resolves.toBeUndefined(); + }); + + test(".js files without matching .map files: throws (0 pairs discovered)", async () => { + // A common misconfiguration: bundler emits JS but no sourcemaps. + writeFileSync(join(dir, "app.js"), "console.log(1)\n"); + writeFileSync(join(dir, "other.js"), "console.log(2)\n"); + const { ctx } = makeContext(); + await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( + ValidationError + ); + }); + + test(".map files without matching .js files: throws (0 pairs discovered)", async () => { + // Stray sourcemaps without their JS — also not usable. + writeFileSync(join(dir, "app.js.map"), '{"version":3}\n'); + const { ctx } = makeContext(); + await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( + ValidationError + ); + }); +}); + +describe("sourcemap upload command — --allow-empty behavior", () => { + let dir: string; + let func: CmdFunc; + let savedEnv: Record; + + beforeEach(async () => { + dir = mkdtempSync(join(tmpdir(), "sentry-upload-cmd-")); + // resolveOrgAndProject reads env vars; short-circuit via SENTRY_ORG / + // SENTRY_PROJECT so the test doesn't need network or config files. + savedEnv = { + SENTRY_ORG: process.env.SENTRY_ORG, + SENTRY_PROJECT: process.env.SENTRY_PROJECT, + }; + process.env.SENTRY_ORG = "test-org"; + process.env.SENTRY_PROJECT = "test-project"; + func = (await uploadCommand.loader()) as unknown as CmdFunc; + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + for (const [k, v] of Object.entries(savedEnv)) { + if (v === undefined) { + delete process.env[k]; + } else { + process.env[k] = v; + } + } + }); + + test("empty directory: throws ValidationError by default", async () => { + const { ctx } = makeContext(); + await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( + ValidationError + ); + }); + + test("empty directory: error message mentions the directory and escape hatch", async () => { + const { ctx } = makeContext(); + try { + await runFunc(func, ctx, {}, dir); + expect.unreachable("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(ValidationError); + const msg = (err as Error).message; + expect(msg).toContain(dir); + expect(msg).toContain("--allow-empty"); + } + }); + + test("empty directory with --allow-empty: succeeds silently", async () => { + const { ctx } = makeContext(); + await expect( + runFunc(func, ctx, { "allow-empty": true }, dir) + ).resolves.toBeUndefined(); + }); + + test("directory with .js files but no .map files: throws", async () => { + // Exact reproduction of the silent-failure mode: docs site had .js + // files emitted but no .map files, and upload reported success with + // 0 files uploaded. + mkdirSync(join(dir, "_astro")); + writeFileSync(join(dir, "_astro", "app.js"), "console.log(1)\n"); + const { ctx } = makeContext(); + await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( + ValidationError + ); + }); +}); From 1094d9bf8feb58c90e3214e424d30cac95e54bda Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 24 Apr 2026 19:01:33 +0000 Subject: [PATCH 2/6] review: tailor error messages, reorder checks, add happy-path test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses self-review findings on PR #846 (see review thread): - **Non-existent / non-directory argument now produces a distinct error.** Added `assertDirectoryReadable()` in `src/lib/sourcemap/inject.ts` that runs BEFORE the discovery walk and throws `ValidationError` with specific wording for ENOENT ("Directory '…' does not exist.") and non-directory paths ("Path '…' is not a directory."). The previous zero-pair error erroneously told users their bundler was misconfigured when the actual problem was a typoed path. - **Reorder `sentry sourcemap upload` checks: dir-read → discover → resolve org/project.** A user hitting the bundler misconfig on a laptop without Sentry credentials configured used to see "Organization and project are required" (misleading — the upstream cause was something else entirely). Moving the directory check first ensures local/unauthenticated invocations get the actionable error. - **Richer zero-pair diagnostics.** New `diagnoseEmptyDiscovery()` re-walks the dir once counting JS and .map files separately, and `buildEmptyDiscoveryError()` produces a tailored message for each shape: - 0 JS + 0 maps → "contains no JS or sourcemap files" - N JS + 0 maps → "Found N JS file(s) but no companion .map …" - 0 JS + N maps → "Found N .map file(s) but no companion JS …" - N JS + M maps, no basename match → "no JS file has a matching `.map` companion" All four branches are regression-tested in `test/commands/sourcemap/upload.test.ts`. - **Add happy-path upload test.** Verifies that a dir with a valid JS+map pair reaches `uploadSourcemaps` with the right org/project and artifact count. Previously only error paths were tested — the feature we're protecting was untested at the command level. - **Drop dead `process.stdout`/`process.stderr` keys from test context.** The wrapper uses `this.stdout`/`this.stderr` directly (see `src/lib/command.ts:566`); the nested `process.*` keys were misleading and contradicted the Stricli convention documented in AGENTS.md. - **Tighten doc fragments + command descriptions.** Both commands now explicitly mention the strict-by-default behavior and `--allow-empty` in `fullDescription` and in `docs/src/fragments/commands/sourcemap.md` (which renders into the public docs site and SKILL.md for AI agents). - **Soften the Astro config comment.** The SSR `sourcemap: "hidden"` line is a no-op today (static-only site), but guards against regressions if a server adapter is added later — comment now says so, and adds a crossref to issue #845. Test count: 15 new tests (5 added over the initial 10), all passing in CI. Full sourcemap suite: 46 pass, 0 fail, 301 expect() calls. --- docs/astro.config.mjs | 11 +- docs/src/fragments/commands/sourcemap.md | 24 ++ .../skills/sentry-cli/references/sourcemap.md | 2 + src/commands/sourcemap/inject.ts | 33 ++- src/commands/sourcemap/upload.ts | 61 +++-- src/lib/sourcemap/inject.ts | 138 ++++++++++++ test/commands/sourcemap/upload.test.ts | 212 ++++++++++++++---- 7 files changed, 403 insertions(+), 78 deletions(-) diff --git a/docs/astro.config.mjs b/docs/astro.config.mjs index 8acaefb06..8c94593d5 100644 --- a/docs/astro.config.mjs +++ b/docs/astro.config.mjs @@ -18,9 +18,14 @@ export default defineConfig({ // Astro 6 uses Vite 7's Environments API and reads `sourcemap` from // `vite.environments.{client,ssr}.build.sourcemap` (see // astro/dist/core/build/static-build.js), falling back to `false` — NOT - // to legacy top-level `vite.build.sourcemap`. We set it on both - // environments to cover both the client bundle (what `sentry sourcemap - // inject/upload` operates on) and SSR output. + // the legacy top-level `vite.build.sourcemap`. The silent Astro-5→6 + // migration was the root cause of the cli.sentry.dev sourcemap upload + // going dark from 2026-04-22 to 2026-04-24 (see #845, fixed in #846). + // + // We set it on the `client` environment (which produces `dist/_astro/` + // — what `sentry sourcemap inject/upload docs/dist/` operates on). The + // `ssr` line is a no-op today because this site is static-only, but + // guards against regressions if a server adapter is added later. vite: { environments: { client: { build: { sourcemap: "hidden" } }, diff --git a/docs/src/fragments/commands/sourcemap.md b/docs/src/fragments/commands/sourcemap.md index ba5af15f0..1852767ac 100644 --- a/docs/src/fragments/commands/sourcemap.md +++ b/docs/src/fragments/commands/sourcemap.md @@ -27,3 +27,27 @@ sentry sourcemap upload ./dist --release 1.0.0 # Set a custom URL prefix sentry sourcemap upload ./dist --url-prefix '~/static/js/' ``` + +## Error handling + +Both `sentry sourcemap inject` and `sentry sourcemap upload` exit with an +error if zero JS + sourcemap pairs are discovered in the target +directory. This catches silent bundler misconfigurations — the most +common cause is a bundler that isn't emitting `.map` files: + +``` +# Vite / Astro: set `vite.build.sourcemap: "hidden"` (Astro 5) or +# `vite.environments.client.build.sourcemap: "hidden"` (Astro 6+). + +# webpack: set `devtool: "hidden-source-map"`. + +# esbuild: set `sourcemap: true` or `sourcemap: "linked"`. +``` + +For CI steps that may run against legitimately-empty directories (e.g., +library-only repos, conditional release skips), pass `--allow-empty` to +suppress the error: + +```bash +sentry sourcemap upload ./dist --allow-empty +``` diff --git a/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md b/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md index c4dcf9049..dcb8408ba 100644 --- a/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md +++ b/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md @@ -53,6 +53,8 @@ sentry sourcemap upload ./dist --release 1.0.0 # Set a custom URL prefix sentry sourcemap upload ./dist --url-prefix '~/static/js/' + +sentry sourcemap upload ./dist --allow-empty ``` All commands also support `--json`, `--fields`, `--help`, `--log-level`, and `--verbose` flags. diff --git a/src/commands/sourcemap/inject.ts b/src/commands/sourcemap/inject.ts index 8cd15405b..b07190223 100644 --- a/src/commands/sourcemap/inject.ts +++ b/src/commands/sourcemap/inject.ts @@ -7,7 +7,6 @@ import type { SentryContext } from "../../context.js"; import { buildCommand } from "../../lib/command.js"; -import { ValidationError } from "../../lib/errors.js"; import { colorTag, mdKvTable, @@ -15,6 +14,9 @@ import { } from "../../lib/formatters/markdown.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { + assertDirectoryReadable, + buildEmptyDiscoveryError, + diagnoseEmptyDiscovery, type InjectResult, injectDirectory, } from "../../lib/sourcemap/inject.js"; @@ -56,10 +58,15 @@ export const injectCommand = buildCommand({ "Scans a directory for .js/.mjs/.cjs files and their companion .map files, " + "then injects Sentry debug IDs for reliable sourcemap resolution.\n\n" + "The injection is idempotent — files that already have debug IDs are skipped.\n\n" + + "Exits with an error if zero JS + sourcemap pairs are discovered " + + "(typical cause: bundler not emitting .map files). Pass " + + "--allow-empty to suppress this check for directories that may " + + "legitimately be empty.\n\n" + "Usage:\n" + " sentry sourcemap inject ./dist\n" + " sentry sourcemap inject ./build --ext .js,.mjs\n" + - " sentry sourcemap inject ./out --dry-run", + " sentry sourcemap inject ./out --dry-run\n" + + " sentry sourcemap inject ./maybe-empty --allow-empty", }, output: { human: formatInjectResult, @@ -104,23 +111,25 @@ export const injectCommand = buildCommand({ flags: { ext?: string; "dry-run"?: boolean; "allow-empty"?: boolean }, dir: string ) { + // Distinct errors for "directory missing" vs "directory empty/ + // misconfigured" — see src/lib/sourcemap/inject.ts. + await assertDirectoryReadable(dir); + const extensions = flags.ext?.split(",").map((e) => e.trim()); const results = await injectDirectory(dir, { extensions, dryRun: flags["dry-run"], }); - // Guard against silent misconfigurations: zero *discovered* pairs almost - // always means the bundler didn't emit .map files. This is distinct from - // zero *injected* (which is legitimate when every pair already has a - // debug ID — the idempotent re-run case). Callers that legitimately - // invoke inject on potentially-empty directories can pass --allow-empty. + // Guard against silent misconfigurations: zero *discovered* pairs + // almost always means the bundler didn't emit .map files. This is + // distinct from zero *injected* (which is legitimate when every + // pair already has a debug ID — the idempotent re-run case). + // Callers that legitimately invoke inject on potentially-empty + // directories can pass --allow-empty. if (results.length === 0 && !flags["allow-empty"]) { - throw new ValidationError( - `No JS + sourcemap pairs found in '${dir}'. Ensure your bundler ` + - "emits sourcemaps (e.g., Vite: `build.sourcemap: 'hidden'`), or " + - "pass --allow-empty to suppress this error." - ); + const diag = await diagnoseEmptyDiscovery(dir, { extensions }); + throw buildEmptyDiscoveryError(dir, diag); } const modified = results.filter((r) => r.injected).length; diff --git a/src/commands/sourcemap/upload.ts b/src/commands/sourcemap/upload.ts index cbc27fd9c..0a2b89ce1 100644 --- a/src/commands/sourcemap/upload.ts +++ b/src/commands/sourcemap/upload.ts @@ -13,11 +13,16 @@ import { uploadSourcemaps, } from "../../lib/api/sourcemaps.js"; import { buildCommand } from "../../lib/command.js"; -import { ContextError, ValidationError } from "../../lib/errors.js"; +import { ContextError } from "../../lib/errors.js"; import { mdKvTable, renderMarkdown } from "../../lib/formatters/markdown.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { resolveOrgAndProject } from "../../lib/resolve-target.js"; -import { injectDirectory } from "../../lib/sourcemap/inject.js"; +import { + assertDirectoryReadable, + buildEmptyDiscoveryError, + diagnoseEmptyDiscovery, + injectDirectory, +} from "../../lib/sourcemap/inject.js"; /** Result type for the upload command. */ type UploadCommandResult = { @@ -54,10 +59,15 @@ export const uploadCommand = buildCommand({ "debug-ID-based matching.\n\n" + "Automatically injects debug IDs into any files that don't already have them.\n" + "Org/project are auto-detected from DSN, env vars, or config defaults.\n\n" + + "Exits with an error if zero JS + sourcemap pairs are discovered " + + "(typical cause: bundler not emitting .map files). Pass " + + "--allow-empty to suppress this check for directories that may " + + "legitimately be empty.\n\n" + "Usage:\n" + " sentry sourcemap upload ./dist\n" + " sentry sourcemap upload ./dist --release 1.0.0\n" + - " sentry sourcemap upload ./dist --url-prefix '~/static/js/'", + " sentry sourcemap upload ./dist --url-prefix '~/static/js/'\n" + + " sentry sourcemap upload ./maybe-empty --allow-empty", }, output: { human: formatUploadResult, @@ -106,7 +116,32 @@ export const uploadCommand = buildCommand({ }, dir: string ) { - // Resolve org/project via the standard cascade + // Validate the directory BEFORE resolving org/project so a + // missing/typoed path or a bundler misconfiguration produces the + // right error even when the user has no Sentry credentials + // configured locally. Non-existent directories get a distinct + // message; empty/misconfigured directories get the bundler-oriented + // diagnostic. See src/lib/sourcemap/inject.ts for the helpers. + await assertDirectoryReadable(dir); + + // Discover and inject debug IDs into JS + sourcemap pairs + const results = await injectDirectory(dir); + + if (results.length === 0 && !flags["allow-empty"]) { + // Silent misconfigurations (e.g., the bundler didn't emit .map + // files) used to succeed with "0 uploaded". That makes post-deploy + // Sentry events unsymbolicated with no build-time signal. Default + // to erroring out so CI fails loudly; `--allow-empty` preserves + // the old behavior for callers that legitimately invoke upload on + // potentially-empty directories. + const diag = await diagnoseEmptyDiscovery(dir); + throw buildEmptyDiscoveryError(dir, diag); + } + + // Resolve org/project via the standard cascade. Runs AFTER the + // directory check so local/unauthenticated invocations still get the + // actionable bundler-oriented error instead of "Organization and + // project are required". const resolved = await resolveOrgAndProject({ cwd: this.cwd, usageHint: USAGE_HINT, @@ -116,23 +151,9 @@ export const uploadCommand = buildCommand({ } const { org, project } = resolved; - // Discover and inject debug IDs into JS + sourcemap pairs - const results = await injectDirectory(dir); - if (results.length === 0) { - // Silent misconfigurations (e.g., the bundler didn't emit .map files) - // used to succeed with "0 uploaded". That makes post-deploy Sentry - // events unsymbolicated with no build-time signal. Default to erroring - // out so CI fails loudly; `--allow-empty` preserves the old behavior - // for callers that legitimately invoke upload on potentially-empty - // directories. - if (!flags["allow-empty"]) { - throw new ValidationError( - `No JS + sourcemap pairs found in '${dir}'. Ensure your bundler ` + - "emits sourcemaps (e.g., Vite: `build.sourcemap: 'hidden'`), or " + - "pass --allow-empty to suppress this error." - ); - } + // --allow-empty path: succeed silently for callers that + // legitimately invoke upload on potentially-empty directories. yield new CommandOutput({ org, project, diff --git a/src/lib/sourcemap/inject.ts b/src/lib/sourcemap/inject.ts index 3cf399771..5e0292e0b 100644 --- a/src/lib/sourcemap/inject.ts +++ b/src/lib/sourcemap/inject.ts @@ -8,6 +8,7 @@ import { readFile, stat } from "node:fs/promises"; import { resolve as resolvePath } from "node:path"; import { NODE_MODULES_DIRNAME } from "../constants.js"; +import { ValidationError } from "../errors.js"; import { walkFiles } from "../scan/index.js"; import { EXISTING_DEBUGID_RE, injectDebugId } from "./debug-id.js"; @@ -138,3 +139,140 @@ async function discoverFilePairs( } return pairs; } + +/** + * Validate that `dir` is an existing directory readable for sourcemap + * discovery, and classify what's inside for diagnostic purposes when the + * scan returns zero pairs. Used by `sentry sourcemap inject` / + * `sourcemap upload` to produce actionable error messages instead of the + * generic "no pairs found" that hid the getsentry/cli docs-site silent + * failure mode (Astro 6 stopped emitting `.map` files; CI stayed green). + * + * Separate from `injectDirectory` so: + * - The upload command can stat the directory BEFORE resolving + * org/project, producing the bundler-oriented error even when the + * user has no Sentry credentials configured. + * - The diagnostic walk is skipped on the happy path (pairs > 0). + * + * @throws ValidationError if `dir` doesn't exist or isn't a directory. + */ +export async function assertDirectoryReadable(dir: string): Promise { + try { + const s = await stat(dir); + if (!s.isDirectory()) { + throw new ValidationError( + `Path '${dir}' is not a directory.`, + "directory" + ); + } + } catch (err) { + if (err instanceof ValidationError) { + throw err; + } + const code = (err as NodeJS.ErrnoException).code; + if (code === "ENOENT") { + throw new ValidationError( + `Directory '${dir}' does not exist.`, + "directory" + ); + } + // EACCES, EPERM, etc. — surface the underlying system error. + const msg = err instanceof Error ? err.message : String(err); + throw new ValidationError( + `Cannot read directory '${dir}': ${msg}`, + "directory" + ); + } +} + +/** + * Diagnostic counts for a directory that produced zero JS + sourcemap + * pairs. Used to tailor the error message to the specific failure mode + * the user is hitting (see callers in `src/commands/sourcemap/`). + */ +export type DiscoveryDiagnostic = { + /** Total `.js`/`.cjs`/`.mjs` files encountered. */ + jsFiles: number; + /** Total `.map` files encountered. */ + mapFiles: number; +}; + +/** + * Scan a directory for JS and map files separately to diagnose why the + * primary pair-discovery returned zero results. Cheap (same walker, + * single pass); only called on the error path. + */ +export async function diagnoseEmptyDiscovery( + dir: string, + options: InjectDirectoryOptions = {} +): Promise { + const jsExtensions = options.extensions + ? new Set(options.extensions.map((e) => (e.startsWith(".") ? e : `.${e}`))) + : DEFAULT_EXTENSIONS; + const absDir = resolvePath(dir); + let jsFiles = 0; + let mapFiles = 0; + for await (const entry of walkFiles({ + cwd: absDir, + // Pass both sets of extensions in one walk to avoid a second traversal. + extensions: new Set([...jsExtensions, ".map"]), + alwaysSkipDirs: SOURCEMAP_SKIP_DIRS, + hidden: false, + respectGitignore: false, + maxFileSize: Number.POSITIVE_INFINITY, + })) { + if (entry.absolutePath.endsWith(".map")) { + mapFiles += 1; + } else { + jsFiles += 1; + } + } + return { jsFiles, mapFiles }; +} + +/** + * Build an actionable error message for the zero-pairs case, tailored to + * which side of the JS/map pairing is missing. Shared between the inject + * and upload commands so the wording stays consistent. + */ +export function buildEmptyDiscoveryError( + dir: string, + diag: DiscoveryDiagnostic +): ValidationError { + const { jsFiles, mapFiles } = diag; + if (jsFiles === 0 && mapFiles === 0) { + return new ValidationError( + `Directory '${dir}' contains no JS or sourcemap files. ` + + "Check the path points at your build output, or pass " + + "--allow-empty to suppress this error.", + "directory" + ); + } + if (jsFiles > 0 && mapFiles === 0) { + return new ValidationError( + `Found ${jsFiles} JS file(s) in '${dir}' but no companion .map ` + + "files. Your bundler is not emitting sourcemaps. For Vite/Astro: " + + "`vite.environments.client.build.sourcemap: 'hidden'`. For webpack: " + + "`devtool: 'hidden-source-map'`. Pass --allow-empty to suppress.", + "directory" + ); + } + if (mapFiles > 0 && jsFiles === 0) { + return new ValidationError( + `Found ${mapFiles} .map file(s) in '${dir}' but no companion JS ` + + "files. Ensure your build emits both JS and maps to the same " + + "directory. Pass --allow-empty to suppress.", + "directory" + ); + } + // Both sides have files but no pairs matched by basename — e.g. + // hash-renamed JS paired with a stable-name map, or maps in a sibling + // `maps/` dir. + return new ValidationError( + `Found ${jsFiles} JS and ${mapFiles} .map file(s) in '${dir}' but ` + + "no JS file has a matching `.map` companion. Check that your " + + "bundler emits JS and sourcemaps with matching basenames. Pass " + + "--allow-empty to suppress.", + "directory" + ); +} diff --git a/test/commands/sourcemap/upload.test.ts b/test/commands/sourcemap/upload.test.ts index 7947a978e..a70f8199b 100644 --- a/test/commands/sourcemap/upload.test.ts +++ b/test/commands/sourcemap/upload.test.ts @@ -2,24 +2,42 @@ * sourcemap inject / upload command tests * * Focused on the "strict by default, --allow-empty opt-out" behavior added - * to guard against silent bundler misconfigurations where no .map files are - * present in the upload directory. A zero-file upload used to succeed - * silently, producing no Sentry symbolication and no CI signal — the exact - * failure mode the getsentry/cli docs site hit (see PR #xxx). + * in #846 to guard against silent bundler misconfigurations where no .map + * files are present in the upload directory. A zero-file upload used to + * succeed silently, producing no Sentry symbolication and no CI signal — + * the exact failure mode the getsentry/cli docs site hit (see #845). + * + * Also covers the diagnostic branches in `buildEmptyDiscoveryError` + * (src/lib/sourcemap/inject.ts) that tailor the error message to the + * specific input shape: empty dir vs. JS-only vs. maps-only. */ -import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { + afterEach, + beforeEach, + describe, + expect, + mock, + spyOn, + test, +} from "bun:test"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { injectCommand } from "../../../src/commands/sourcemap/inject.js"; import { uploadCommand } from "../../../src/commands/sourcemap/upload.js"; +// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking +import * as sourcemapsApi from "../../../src/lib/api/sourcemaps.js"; import { ValidationError } from "../../../src/lib/errors.js"; // The loader() return is the wrapper-produced async function (NOT a // generator) that internally iterates the original generator body and // writes rendered output to ctx.stdout. Errors thrown inside the // generator body propagate out as a rejected promise. +// +// The wrapper uses `this.stdout`/`this.stderr` directly (see +// `src/lib/command.ts:566`), not `this.process.*`. See AGENTS.md "Stricli +// buildCommand" lore entry. type InjectFuncArgs = { ext?: string; "dry-run"?: boolean; @@ -48,18 +66,6 @@ function makeContext() { }), }, cwd: "/tmp", - process: { - stdout: { - write: mock((s: string) => { - stdoutChunks.push(s); - }), - }, - stderr: { - write: mock((s: string) => { - stderrChunks.push(s); - }), - }, - }, }, stdout: () => stdoutChunks.join(""), stderr: () => stderrChunks.join(""), @@ -88,14 +94,7 @@ describe("sourcemap inject command — --allow-empty behavior", () => { rmSync(dir, { recursive: true, force: true }); }); - test("empty directory: throws ValidationError by default", async () => { - const { ctx } = makeContext(); - await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( - ValidationError - ); - }); - - test("empty directory: error message mentions the directory and escape hatch", async () => { + test("empty directory: throws ValidationError mentioning empty-dir", async () => { const { ctx } = makeContext(); try { await runFunc(func, ctx, {}, dir); @@ -105,6 +104,7 @@ describe("sourcemap inject command — --allow-empty behavior", () => { const msg = (err as Error).message; expect(msg).toContain(dir); expect(msg).toContain("--allow-empty"); + expect(msg).toMatch(/no JS or sourcemap files/i); } }); @@ -122,23 +122,98 @@ describe("sourcemap inject command — --allow-empty behavior", () => { await expect(runFunc(func, ctx, {}, dir)).resolves.toBeUndefined(); }); - test(".js files without matching .map files: throws (0 pairs discovered)", async () => { - // A common misconfiguration: bundler emits JS but no sourcemaps. + test(".js files without matching .map files: throws with bundler hint", async () => { + // The getsentry/cli docs-site failure mode: JS emitted but no .map + // files (#845). Error should explicitly name Vite/webpack config. writeFileSync(join(dir, "app.js"), "console.log(1)\n"); writeFileSync(join(dir, "other.js"), "console.log(2)\n"); const { ctx } = makeContext(); - await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( - ValidationError - ); + try { + await runFunc(func, ctx, {}, dir); + expect.unreachable("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(ValidationError); + const msg = (err as Error).message; + expect(msg).toContain("2 JS file(s)"); + expect(msg).toMatch(/vite|webpack/i); + expect(msg).toContain("sourcemap"); + } }); - test(".map files without matching .js files: throws (0 pairs discovered)", async () => { - // Stray sourcemaps without their JS — also not usable. + test(".map files without matching .js files: throws with mismatch hint", async () => { writeFileSync(join(dir, "app.js.map"), '{"version":3}\n'); const { ctx } = makeContext(); - await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( - ValidationError - ); + try { + await runFunc(func, ctx, {}, dir); + expect.unreachable("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(ValidationError); + const msg = (err as Error).message; + expect(msg).toContain(".map file(s)"); + expect(msg).toContain("no companion JS"); + } + }); + + test("js and map present but no basename match: reports both counts", async () => { + // e.g. hash-renamed JS paired with a stable-name map. + writeFileSync(join(dir, "app.abc123.js"), "console.log(1)\n"); + writeFileSync(join(dir, "app.js.map"), '{"version":3}\n'); + const { ctx } = makeContext(); + try { + await runFunc(func, ctx, {}, dir); + expect.unreachable("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(ValidationError); + const msg = (err as Error).message; + expect(msg).toContain("1 JS"); + expect(msg).toContain("1 .map"); + expect(msg).toContain("matching basename"); + } + }); + + test("non-existent directory: throws with distinct 'does not exist' message", async () => { + const missing = join(dir, "does-not-exist"); + const { ctx } = makeContext(); + try { + await runFunc(func, ctx, {}, missing); + expect.unreachable("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(ValidationError); + const msg = (err as Error).message; + expect(msg).toContain(missing); + expect(msg).toMatch(/does not exist/i); + // Must NOT conflate with the bundler hint. + expect(msg).not.toContain("--allow-empty"); + } + }); + + test("path is a file, not a directory: throws with distinct message", async () => { + const filePath = join(dir, "not-a-dir.txt"); + writeFileSync(filePath, "hello\n"); + const { ctx } = makeContext(); + try { + await runFunc(func, ctx, {}, filePath); + expect.unreachable("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(ValidationError); + const msg = (err as Error).message; + expect(msg).toContain(filePath); + expect(msg).toMatch(/not a directory/i); + } + }); + + test("--dry-run + empty directory: still errors (dry-run is not an escape hatch)", async () => { + const { ctx } = makeContext(); + await expect( + runFunc(func, ctx, { "dry-run": true }, dir) + ).rejects.toBeInstanceOf(ValidationError); + }); + + test("--dry-run + --allow-empty: succeeds silently", async () => { + const { ctx } = makeContext(); + await expect( + runFunc(func, ctx, { "dry-run": true, "allow-empty": true }, dir) + ).resolves.toBeUndefined(); }); }); @@ -171,14 +246,7 @@ describe("sourcemap upload command — --allow-empty behavior", () => { } }); - test("empty directory: throws ValidationError by default", async () => { - const { ctx } = makeContext(); - await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( - ValidationError - ); - }); - - test("empty directory: error message mentions the directory and escape hatch", async () => { + test("empty directory: throws ValidationError mentioning empty-dir", async () => { const { ctx } = makeContext(); try { await runFunc(func, ctx, {}, dir); @@ -209,4 +277,62 @@ describe("sourcemap upload command — --allow-empty behavior", () => { ValidationError ); }); + + test("non-existent directory: throws before touching Sentry creds", async () => { + // Even with SENTRY_ORG/SENTRY_PROJECT cleared, the dir-check should + // fire first — that's the whole point of reordering the checks so + // local/unauthenticated invocations get actionable errors. + delete process.env.SENTRY_ORG; + delete process.env.SENTRY_PROJECT; + const missing = join(dir, "does-not-exist"); + const { ctx } = makeContext(); + try { + await runFunc(func, ctx, {}, missing); + expect.unreachable("should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(ValidationError); + const msg = (err as Error).message; + expect(msg).toMatch(/does not exist/i); + } + }); + + test("happy path: directory with JS+map pair invokes uploadSourcemaps", async () => { + // Prove the guard doesn't false-positive on a real upload path, and + // that we reach the API call with sensible artifact files. + mkdirSync(join(dir, "_astro")); + const jsPath = join(dir, "_astro", "app.js"); + const mapPath = join(dir, "_astro", "app.js.map"); + writeFileSync(jsPath, "console.log(1)\n"); + // Valid minimal sourcemap so injectDebugId's inject step doesn't + // choke parsing. + writeFileSync( + mapPath, + JSON.stringify({ + version: 3, + sources: ["app.ts"], + names: [], + mappings: "", + }) + ); + + const uploadSpy = spyOn( + sourcemapsApi, + "uploadSourcemaps" + ).mockResolvedValue(undefined); + try { + const { ctx } = makeContext(); + await runFunc(func, ctx, {}, dir); + expect(uploadSpy).toHaveBeenCalledTimes(1); + const callArgs = uploadSpy.mock.calls[0]?.[0]; + expect(callArgs?.org).toBe("test-org"); + expect(callArgs?.project).toBe("test-project"); + // One JS + one sourcemap artifact per pair. + expect(callArgs?.files).toHaveLength(2); + const types = callArgs?.files.map((f) => f.type); + expect(types).toContain("minified_source"); + expect(types).toContain("source_map"); + } finally { + uploadSpy.mockRestore(); + } + }); }); From e8452ad7c1739fb7efaa4b0cf3d9e7c10f65edb4 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 24 Apr 2026 19:12:09 +0000 Subject: [PATCH 3/6] review: split discovery + inject so empty-dir errors don't mutate files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two follow-up bot findings on the previous commit (1094d9bf): - **Cursor Bugbot (Medium)**: "File modifications happen before credential validation." Previously, `injectDirectory` ran BEFORE `resolveOrgAndProject`, so an empty/misconfigured directory would still have no files to mutate — but a non-empty directory with missing SENTRY_ORG/SENTRY_PROJECT would get debug IDs written into every JS pair before the command errored on credential resolution. An unannounced behavior change. Fix: split `injectDirectory` into a read-only discovery pass (`discoverFilePairs`, now exported) and the mutating inject pass. New control flow in both commands: 1. assertDirectoryReadable(dir) — stat, no writes 2. discoverFilePairs(dir) — walk, no writes 3. if empty and not --allow-empty → error (no writes) 4. resolveOrgAndProject — may fail, no writes 5. if empty and --allow-empty → early exit (no writes) 6. injectDirectory(dir) — writes debug IDs 7. uploadSourcemaps — API call Upload-to-API failures still leave files mutated (user explicitly requested upload, injection is part of that operation), but all error paths BEFORE the API call are now side-effect-free. - **Seer (Low)**: The `--allow-empty` success-path hint told users to run `sentry sourcemap inject`, which the upload command had already attempted internally (via `injectDirectory`). Misleading guidance. Fix: replace the hint with "If this is unexpected, check your bundler emits .map files." — which matches the actual root cause for anyone who lands in this state. Added a regression test (`test/commands/sourcemap/upload.test.ts` "error path does not mutate files (js-only dir)") that writes a JS file, invokes upload, asserts the error fires, and verifies the file bytes are unchanged afterwards. Locks in the discover-first invariant so future refactors can't silently reintroduce the pre-error mutation. 16 tests in the new file, all passing. --- src/commands/sourcemap/inject.ts | 25 ++++++++++++----- src/commands/sourcemap/upload.ts | 39 ++++++++++++++++---------- src/lib/sourcemap/inject.ts | 15 ++++++++-- test/commands/sourcemap/upload.test.ts | 22 +++++++++++++++ 4 files changed, 76 insertions(+), 25 deletions(-) diff --git a/src/commands/sourcemap/inject.ts b/src/commands/sourcemap/inject.ts index b07190223..b5a00a516 100644 --- a/src/commands/sourcemap/inject.ts +++ b/src/commands/sourcemap/inject.ts @@ -17,6 +17,7 @@ import { assertDirectoryReadable, buildEmptyDiscoveryError, diagnoseEmptyDiscovery, + discoverFilePairs, type InjectResult, injectDirectory, } from "../../lib/sourcemap/inject.js"; @@ -111,15 +112,16 @@ export const injectCommand = buildCommand({ flags: { ext?: string; "dry-run"?: boolean; "allow-empty"?: boolean }, dir: string ) { - // Distinct errors for "directory missing" vs "directory empty/ - // misconfigured" — see src/lib/sourcemap/inject.ts. + // Phase 1 — read-only validation. Distinct errors for "directory + // missing" vs "directory empty/misconfigured". Discovery runs + // without side effects so we never write debug IDs into files when + // the upstream state is doomed (empty dir, typo'd path). await assertDirectoryReadable(dir); const extensions = flags.ext?.split(",").map((e) => e.trim()); - const results = await injectDirectory(dir, { - extensions, - dryRun: flags["dry-run"], - }); + const extSet = extensions + ? new Set(extensions.map((e) => (e.startsWith(".") ? e : `.${e}`))) + : undefined; // Guard against silent misconfigurations: zero *discovered* pairs // almost always means the bundler didn't emit .map files. This is @@ -127,11 +129,20 @@ export const injectCommand = buildCommand({ // pair already has a debug ID — the idempotent re-run case). // Callers that legitimately invoke inject on potentially-empty // directories can pass --allow-empty. - if (results.length === 0 && !flags["allow-empty"]) { + const pairs = await discoverFilePairs(dir, extSet); + if (pairs.length === 0 && !flags["allow-empty"]) { const diag = await diagnoseEmptyDiscovery(dir, { extensions }); throw buildEmptyDiscoveryError(dir, diag); } + // Phase 2 — mutating work (skipped in dry-run). The second pass + // through `injectDirectory` re-walks the directory; this is cheap + // relative to the sourcemap parsing/rewriting it does per pair. + const results = await injectDirectory(dir, { + extensions, + dryRun: flags["dry-run"], + }); + const modified = results.filter((r) => r.injected).length; const skipped = results.length - modified; diff --git a/src/commands/sourcemap/upload.ts b/src/commands/sourcemap/upload.ts index 0a2b89ce1..b0ee2bf64 100644 --- a/src/commands/sourcemap/upload.ts +++ b/src/commands/sourcemap/upload.ts @@ -21,6 +21,7 @@ import { assertDirectoryReadable, buildEmptyDiscoveryError, diagnoseEmptyDiscovery, + discoverFilePairs, injectDirectory, } from "../../lib/sourcemap/inject.js"; @@ -116,18 +117,17 @@ export const uploadCommand = buildCommand({ }, dir: string ) { - // Validate the directory BEFORE resolving org/project so a - // missing/typoed path or a bundler misconfiguration produces the - // right error even when the user has no Sentry credentials - // configured locally. Non-existent directories get a distinct - // message; empty/misconfigured directories get the bundler-oriented - // diagnostic. See src/lib/sourcemap/inject.ts for the helpers. + // Phase 1 — read-only validation. Runs BEFORE `injectDirectory` + // (which writes to disk) and BEFORE `resolveOrgAndProject` (which + // may fail on missing credentials). This keeps the command + // side-effect-free on every error path, so a user whose upload + // fails for any reason (typoed path, missing creds, bundler + // misconfig) doesn't end up with partially-injected debug IDs and + // a rewritten `.map` file. await assertDirectoryReadable(dir); + const pairs = await discoverFilePairs(dir); - // Discover and inject debug IDs into JS + sourcemap pairs - const results = await injectDirectory(dir); - - if (results.length === 0 && !flags["allow-empty"]) { + if (pairs.length === 0 && !flags["allow-empty"]) { // Silent misconfigurations (e.g., the bundler didn't emit .map // files) used to succeed with "0 uploaded". That makes post-deploy // Sentry events unsymbolicated with no build-time signal. Default @@ -141,7 +141,8 @@ export const uploadCommand = buildCommand({ // Resolve org/project via the standard cascade. Runs AFTER the // directory check so local/unauthenticated invocations still get the // actionable bundler-oriented error instead of "Organization and - // project are required". + // project are required", but BEFORE the actual debug-ID injection + // so we never mutate user files on a doomed run. const resolved = await resolveOrgAndProject({ cwd: this.cwd, usageHint: USAGE_HINT, @@ -151,9 +152,9 @@ export const uploadCommand = buildCommand({ } const { org, project } = resolved; - if (results.length === 0) { - // --allow-empty path: succeed silently for callers that - // legitimately invoke upload on potentially-empty directories. + if (pairs.length === 0) { + // --allow-empty path: nothing to do. Don't recommend running + // `sentry sourcemap inject` — we'd hit the same empty-dir state. yield new CommandOutput({ org, project, @@ -161,10 +162,18 @@ export const uploadCommand = buildCommand({ filesUploaded: 0, }); return { - hint: "No JS + sourcemap pairs found. Run `sentry sourcemap inject` first.", + hint: + "No JS + sourcemap pairs found in the target directory. " + + "If this is unexpected, check your bundler emits .map files.", }; } + // Phase 2 — mutating work. Inject debug IDs into each pair. Only + // runs once we know (a) the directory exists, (b) it has pairs, and + // (c) Sentry credentials are resolved. `injectDirectory` is + // idempotent on re-runs (skips files that already carry a debug ID). + const results = await injectDirectory(dir); + const urlPrefix = flags["url-prefix"] ?? "~/"; // Build artifact file list with paths relative to the upload directory diff --git a/src/lib/sourcemap/inject.ts b/src/lib/sourcemap/inject.ts index 5e0292e0b..825f0e6ef 100644 --- a/src/lib/sourcemap/inject.ts +++ b/src/lib/sourcemap/inject.ts @@ -73,7 +73,7 @@ export async function injectDirectory( } /** A discovered JS + sourcemap pair. */ -type FilePair = { jsPath: string; mapPath: string }; +export type FilePair = { jsPath: string; mapPath: string }; /** * Check if a path has a companion .map file. @@ -114,9 +114,18 @@ async function findCompanionMap(jsPath: string): Promise { */ const SOURCEMAP_SKIP_DIRS: readonly string[] = [NODE_MODULES_DIRNAME]; -async function discoverFilePairs( +/** + * Discovery pass with no side effects — returns the list of JS + + * sourcemap pairs without injecting debug IDs. Exposed so the + * `sentry sourcemap upload` command can run a read-only emptiness + * check (and emit the bundler-oriented diagnostic) BEFORE calling + * {@link injectDirectory} — which writes to disk and would otherwise + * leave partial state when upload subsequently fails on credentials + * resolution. + */ +export async function discoverFilePairs( dir: string, - extensions: Set + extensions: Set = DEFAULT_EXTENSIONS ): Promise { // `walkFiles` requires an absolute cwd. CLI callers pass // user-supplied positional args like `./dist` directly through to diff --git a/test/commands/sourcemap/upload.test.ts b/test/commands/sourcemap/upload.test.ts index a70f8199b..af6102631 100644 --- a/test/commands/sourcemap/upload.test.ts +++ b/test/commands/sourcemap/upload.test.ts @@ -296,6 +296,28 @@ describe("sourcemap upload command — --allow-empty behavior", () => { } }); + test("error path does not mutate files (js-only dir)", async () => { + // Regression guard for the Bugbot finding that led to the + // discover-first refactor: previously `injectDirectory` ran BEFORE + // the emptiness check, so a js-only dir got debug IDs written into + // every JS before the bundler-misconfig error fired. Now discovery + // is a read-only pass; injection only runs when we've decided + // upload will proceed. + mkdirSync(join(dir, "_astro")); + const jsPath = join(dir, "_astro", "app.js"); + const original = "console.log(1)\n"; + writeFileSync(jsPath, original); + const { ctx } = makeContext(); + await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( + ValidationError + ); + // File must be unchanged — no debug ID IIFE, no sourcemap comment. + const after = await Bun.file(jsPath).text(); + expect(after).toBe(original); + expect(after).not.toContain("_sentryDebugIds"); + expect(after).not.toContain("sentry-dbid"); + }); + test("happy path: directory with JS+map pair invokes uploadSourcemaps", async () => { // Prove the guard doesn't false-positive on a real upload path, and // that we reach the API call with sensible artifact files. From 6649777b311a1917e36689a270ed2aee7385f1dd Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Sat, 25 Apr 2026 18:27:10 +0000 Subject: [PATCH 4/6] chore: remove AI-generated slop in comments and helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs/astro.config.mjs: trim 12-line comment with date ranges and PR refs to 3 lines explaining the technical reason for the Environments-API path. - src/lib/sourcemap/inject.ts: - Trim JSDoc on assertDirectoryReadable, diagnoseEmptyDiscovery, discoverFilePairs, buildEmptyDiscoveryError to single-purpose descriptions; drop caller-specific narration ("see callers in src/commands/sourcemap/", "the getsentry/cli docs-site silent failure mode", historical context that belongs in commit messages). - Replace `extensions: new Set([...jsExtensions, ".map"])` array spread with mutate-and-add on a fresh Set; avoids the intermediate array and a mutation hazard against the shared DEFAULT_EXTENSIONS when the caller doesn't override extensions. - src/commands/sourcemap/upload.ts: drop "Phase 1 / Phase 2" framing comments; the call sequence is self-explanatory from the function names. Trim the multi-line block restating the PR description. - test/commands/sourcemap/upload.test.ts: - Drop the runFunc() pass-through wrapper (just call func.call() directly, matching test/commands/init.test.ts). - Simplify makeContext() to return the ctx directly instead of a {ctx, stdout, stderr} struct — tests don't read stdout/stderr. - Trim header comment from 13 lines to 4; drop the loader() explainer comment that referenced `src/lib/command.ts:566` and AGENTS.md. - Drop in-test comments referencing #845/#846/Bugbot — process noise that doesn't help future readers. --- docs/astro.config.mjs | 14 +-- src/commands/sourcemap/upload.ts | 27 +---- src/lib/sourcemap/inject.ts | 66 +++++------ test/commands/sourcemap/upload.test.ts | 149 ++++++++----------------- 4 files changed, 76 insertions(+), 180 deletions(-) diff --git a/docs/astro.config.mjs b/docs/astro.config.mjs index 8c94593d5..540d5b667 100644 --- a/docs/astro.config.mjs +++ b/docs/astro.config.mjs @@ -15,17 +15,9 @@ export default defineConfig({ // adding //# sourceMappingURL comments to the output (the debug IDs // injected post-build by `sentry sourcemap inject` are used instead). // - // Astro 6 uses Vite 7's Environments API and reads `sourcemap` from - // `vite.environments.{client,ssr}.build.sourcemap` (see - // astro/dist/core/build/static-build.js), falling back to `false` — NOT - // the legacy top-level `vite.build.sourcemap`. The silent Astro-5→6 - // migration was the root cause of the cli.sentry.dev sourcemap upload - // going dark from 2026-04-22 to 2026-04-24 (see #845, fixed in #846). - // - // We set it on the `client` environment (which produces `dist/_astro/` - // — what `sentry sourcemap inject/upload docs/dist/` operates on). The - // `ssr` line is a no-op today because this site is static-only, but - // guards against regressions if a server adapter is added later. + // Astro 6 / Vite 7 reads `sourcemap` from + // `vite.environments.{client,ssr}.build.sourcemap` (Environments API), + // not the legacy top-level `vite.build.sourcemap`. vite: { environments: { client: { build: { sourcemap: "hidden" } }, diff --git a/src/commands/sourcemap/upload.ts b/src/commands/sourcemap/upload.ts index b0ee2bf64..5fe6a14da 100644 --- a/src/commands/sourcemap/upload.ts +++ b/src/commands/sourcemap/upload.ts @@ -117,32 +117,17 @@ export const uploadCommand = buildCommand({ }, dir: string ) { - // Phase 1 — read-only validation. Runs BEFORE `injectDirectory` - // (which writes to disk) and BEFORE `resolveOrgAndProject` (which - // may fail on missing credentials). This keeps the command - // side-effect-free on every error path, so a user whose upload - // fails for any reason (typoed path, missing creds, bundler - // misconfig) doesn't end up with partially-injected debug IDs and - // a rewritten `.map` file. + // Validate the directory and discover pairs read-only first so we + // don't write debug IDs when the upload won't proceed (empty dir, + // typoed path, missing credentials). await assertDirectoryReadable(dir); const pairs = await discoverFilePairs(dir); if (pairs.length === 0 && !flags["allow-empty"]) { - // Silent misconfigurations (e.g., the bundler didn't emit .map - // files) used to succeed with "0 uploaded". That makes post-deploy - // Sentry events unsymbolicated with no build-time signal. Default - // to erroring out so CI fails loudly; `--allow-empty` preserves - // the old behavior for callers that legitimately invoke upload on - // potentially-empty directories. const diag = await diagnoseEmptyDiscovery(dir); throw buildEmptyDiscoveryError(dir, diag); } - // Resolve org/project via the standard cascade. Runs AFTER the - // directory check so local/unauthenticated invocations still get the - // actionable bundler-oriented error instead of "Organization and - // project are required", but BEFORE the actual debug-ID injection - // so we never mutate user files on a doomed run. const resolved = await resolveOrgAndProject({ cwd: this.cwd, usageHint: USAGE_HINT, @@ -153,8 +138,6 @@ export const uploadCommand = buildCommand({ const { org, project } = resolved; if (pairs.length === 0) { - // --allow-empty path: nothing to do. Don't recommend running - // `sentry sourcemap inject` — we'd hit the same empty-dir state. yield new CommandOutput({ org, project, @@ -168,10 +151,6 @@ export const uploadCommand = buildCommand({ }; } - // Phase 2 — mutating work. Inject debug IDs into each pair. Only - // runs once we know (a) the directory exists, (b) it has pairs, and - // (c) Sentry credentials are resolved. `injectDirectory` is - // idempotent on re-runs (skips files that already carry a debug ID). const results = await injectDirectory(dir); const urlPrefix = flags["url-prefix"] ?? "~/"; diff --git a/src/lib/sourcemap/inject.ts b/src/lib/sourcemap/inject.ts index 825f0e6ef..c2de1bef5 100644 --- a/src/lib/sourcemap/inject.ts +++ b/src/lib/sourcemap/inject.ts @@ -115,13 +115,10 @@ async function findCompanionMap(jsPath: string): Promise { const SOURCEMAP_SKIP_DIRS: readonly string[] = [NODE_MODULES_DIRNAME]; /** - * Discovery pass with no side effects — returns the list of JS + - * sourcemap pairs without injecting debug IDs. Exposed so the - * `sentry sourcemap upload` command can run a read-only emptiness - * check (and emit the bundler-oriented diagnostic) BEFORE calling - * {@link injectDirectory} — which writes to disk and would otherwise - * leave partial state when upload subsequently fails on credentials - * resolution. + * Read-only discovery pass — returns the list of JS + sourcemap pairs + * without injecting debug IDs. Used as a pre-check by the upload + * command so the directory isn't mutated when the upload won't + * proceed (empty dir, missing credentials, etc.). */ export async function discoverFilePairs( dir: string, @@ -150,20 +147,9 @@ export async function discoverFilePairs( } /** - * Validate that `dir` is an existing directory readable for sourcemap - * discovery, and classify what's inside for diagnostic purposes when the - * scan returns zero pairs. Used by `sentry sourcemap inject` / - * `sourcemap upload` to produce actionable error messages instead of the - * generic "no pairs found" that hid the getsentry/cli docs-site silent - * failure mode (Astro 6 stopped emitting `.map` files; CI stayed green). - * - * Separate from `injectDirectory` so: - * - The upload command can stat the directory BEFORE resolving - * org/project, producing the bundler-oriented error even when the - * user has no Sentry credentials configured. - * - The diagnostic walk is skipped on the happy path (pairs > 0). - * - * @throws ValidationError if `dir` doesn't exist or isn't a directory. + * Throw {@link ValidationError} if `dir` doesn't exist or isn't a + * readable directory. Distinct messages per failure mode so the user + * gets a useful pointer instead of "no sourcemaps found". */ export async function assertDirectoryReadable(dir: string): Promise { try { @@ -185,7 +171,6 @@ export async function assertDirectoryReadable(dir: string): Promise { "directory" ); } - // EACCES, EPERM, etc. — surface the underlying system error. const msg = err instanceof Error ? err.message : String(err); throw new ValidationError( `Cannot read directory '${dir}': ${msg}`, @@ -195,36 +180,39 @@ export async function assertDirectoryReadable(dir: string): Promise { } /** - * Diagnostic counts for a directory that produced zero JS + sourcemap - * pairs. Used to tailor the error message to the specific failure mode - * the user is hitting (see callers in `src/commands/sourcemap/`). + * Counts of JS and `.map` files in a directory, used by + * {@link buildEmptyDiscoveryError} to tailor the zero-pairs error. */ export type DiscoveryDiagnostic = { - /** Total `.js`/`.cjs`/`.mjs` files encountered. */ jsFiles: number; - /** Total `.map` files encountered. */ mapFiles: number; }; /** - * Scan a directory for JS and map files separately to diagnose why the - * primary pair-discovery returned zero results. Cheap (same walker, - * single pass); only called on the error path. + * Count JS and `.map` files in a single walker pass. Only called on + * the zero-pairs error path. */ export async function diagnoseEmptyDiscovery( dir: string, options: InjectDirectoryOptions = {} ): Promise { - const jsExtensions = options.extensions - ? new Set(options.extensions.map((e) => (e.startsWith(".") ? e : `.${e}`))) - : DEFAULT_EXTENSIONS; + // Build one set covering JS extensions + `.map` so the walker visits + // both in a single pass. + const extensions = new Set(DEFAULT_EXTENSIONS); + if (options.extensions) { + extensions.clear(); + for (const e of options.extensions) { + extensions.add(e.startsWith(".") ? e : `.${e}`); + } + } + extensions.add(".map"); + const absDir = resolvePath(dir); let jsFiles = 0; let mapFiles = 0; for await (const entry of walkFiles({ cwd: absDir, - // Pass both sets of extensions in one walk to avoid a second traversal. - extensions: new Set([...jsExtensions, ".map"]), + extensions, alwaysSkipDirs: SOURCEMAP_SKIP_DIRS, hidden: false, respectGitignore: false, @@ -240,9 +228,8 @@ export async function diagnoseEmptyDiscovery( } /** - * Build an actionable error message for the zero-pairs case, tailored to - * which side of the JS/map pairing is missing. Shared between the inject - * and upload commands so the wording stays consistent. + * Build an actionable error for the zero-pairs case, tailored to + * which side of the JS/map pairing is missing. */ export function buildEmptyDiscoveryError( dir: string, @@ -274,9 +261,6 @@ export function buildEmptyDiscoveryError( "directory" ); } - // Both sides have files but no pairs matched by basename — e.g. - // hash-renamed JS paired with a stable-name map, or maps in a sibling - // `maps/` dir. return new ValidationError( `Found ${jsFiles} JS and ${mapFiles} .map file(s) in '${dir}' but ` + "no JS file has a matching `.map` companion. Check that your " + diff --git a/test/commands/sourcemap/upload.test.ts b/test/commands/sourcemap/upload.test.ts index af6102631..719205c62 100644 --- a/test/commands/sourcemap/upload.test.ts +++ b/test/commands/sourcemap/upload.test.ts @@ -1,15 +1,7 @@ /** - * sourcemap inject / upload command tests - * - * Focused on the "strict by default, --allow-empty opt-out" behavior added - * in #846 to guard against silent bundler misconfigurations where no .map - * files are present in the upload directory. A zero-file upload used to - * succeed silently, producing no Sentry symbolication and no CI signal — - * the exact failure mode the getsentry/cli docs site hit (see #845). - * - * Also covers the diagnostic branches in `buildEmptyDiscoveryError` - * (src/lib/sourcemap/inject.ts) that tailor the error message to the - * specific input shape: empty dir vs. JS-only vs. maps-only. + * Tests for the strict-by-default zero-pairs behavior on `sentry + * sourcemap inject` / `upload`, including the per-shape diagnostic + * branches in `buildEmptyDiscoveryError`. */ import { @@ -30,14 +22,6 @@ import { uploadCommand } from "../../../src/commands/sourcemap/upload.js"; import * as sourcemapsApi from "../../../src/lib/api/sourcemaps.js"; import { ValidationError } from "../../../src/lib/errors.js"; -// The loader() return is the wrapper-produced async function (NOT a -// generator) that internally iterates the original generator body and -// writes rendered output to ctx.stdout. Errors thrown inside the -// generator body propagate out as a rejected promise. -// -// The wrapper uses `this.stdout`/`this.stderr` directly (see -// `src/lib/command.ts:566`), not `this.process.*`. See AGENTS.md "Stricli -// buildCommand" lore entry. type InjectFuncArgs = { ext?: string; "dry-run"?: boolean; @@ -51,36 +35,13 @@ type UploadFuncArgs = { type CmdFunc = (this: unknown, flags: A, dir: string) => Promise; function makeContext() { - const stdoutChunks: string[] = []; - const stderrChunks: string[] = []; return { - ctx: { - stdout: { - write: mock((s: string) => { - stdoutChunks.push(s); - }), - }, - stderr: { - write: mock((s: string) => { - stderrChunks.push(s); - }), - }, - cwd: "/tmp", - }, - stdout: () => stdoutChunks.join(""), - stderr: () => stderrChunks.join(""), + stdout: { write: mock(() => true) }, + stderr: { write: mock(() => true) }, + cwd: "/tmp", }; } -async function runFunc( - func: CmdFunc, - ctx: unknown, - flags: A, - dir: string -): Promise { - await func.call(ctx, flags, dir); -} - describe("sourcemap inject command — --allow-empty behavior", () => { let dir: string; let func: CmdFunc; @@ -94,10 +55,10 @@ describe("sourcemap inject command — --allow-empty behavior", () => { rmSync(dir, { recursive: true, force: true }); }); - test("empty directory: throws ValidationError mentioning empty-dir", async () => { - const { ctx } = makeContext(); + test("empty directory: throws actionable ValidationError", async () => { + const ctx = makeContext(); try { - await runFunc(func, ctx, {}, dir); + await func.call(ctx, {}, dir); expect.unreachable("should have thrown"); } catch (err) { expect(err).toBeInstanceOf(ValidationError); @@ -109,27 +70,25 @@ describe("sourcemap inject command — --allow-empty behavior", () => { }); test("empty directory with --allow-empty: succeeds silently", async () => { - const { ctx } = makeContext(); + const ctx = makeContext(); await expect( - runFunc(func, ctx, { "allow-empty": true }, dir) + func.call(ctx, { "allow-empty": true }, dir) ).resolves.toBeUndefined(); }); test("directory with a .js + .map pair: succeeds (0 pairs guard not triggered)", async () => { writeFileSync(join(dir, "app.js"), "console.log(1)\n"); writeFileSync(join(dir, "app.js.map"), '{"version":3}\n'); - const { ctx } = makeContext(); - await expect(runFunc(func, ctx, {}, dir)).resolves.toBeUndefined(); + const ctx = makeContext(); + await expect(func.call(ctx, {}, dir)).resolves.toBeUndefined(); }); test(".js files without matching .map files: throws with bundler hint", async () => { - // The getsentry/cli docs-site failure mode: JS emitted but no .map - // files (#845). Error should explicitly name Vite/webpack config. writeFileSync(join(dir, "app.js"), "console.log(1)\n"); writeFileSync(join(dir, "other.js"), "console.log(2)\n"); - const { ctx } = makeContext(); + const ctx = makeContext(); try { - await runFunc(func, ctx, {}, dir); + await func.call(ctx, {}, dir); expect.unreachable("should have thrown"); } catch (err) { expect(err).toBeInstanceOf(ValidationError); @@ -142,9 +101,9 @@ describe("sourcemap inject command — --allow-empty behavior", () => { test(".map files without matching .js files: throws with mismatch hint", async () => { writeFileSync(join(dir, "app.js.map"), '{"version":3}\n'); - const { ctx } = makeContext(); + const ctx = makeContext(); try { - await runFunc(func, ctx, {}, dir); + await func.call(ctx, {}, dir); expect.unreachable("should have thrown"); } catch (err) { expect(err).toBeInstanceOf(ValidationError); @@ -155,12 +114,11 @@ describe("sourcemap inject command — --allow-empty behavior", () => { }); test("js and map present but no basename match: reports both counts", async () => { - // e.g. hash-renamed JS paired with a stable-name map. writeFileSync(join(dir, "app.abc123.js"), "console.log(1)\n"); writeFileSync(join(dir, "app.js.map"), '{"version":3}\n'); - const { ctx } = makeContext(); + const ctx = makeContext(); try { - await runFunc(func, ctx, {}, dir); + await func.call(ctx, {}, dir); expect.unreachable("should have thrown"); } catch (err) { expect(err).toBeInstanceOf(ValidationError); @@ -173,16 +131,15 @@ describe("sourcemap inject command — --allow-empty behavior", () => { test("non-existent directory: throws with distinct 'does not exist' message", async () => { const missing = join(dir, "does-not-exist"); - const { ctx } = makeContext(); + const ctx = makeContext(); try { - await runFunc(func, ctx, {}, missing); + await func.call(ctx, {}, missing); expect.unreachable("should have thrown"); } catch (err) { expect(err).toBeInstanceOf(ValidationError); const msg = (err as Error).message; expect(msg).toContain(missing); expect(msg).toMatch(/does not exist/i); - // Must NOT conflate with the bundler hint. expect(msg).not.toContain("--allow-empty"); } }); @@ -190,9 +147,9 @@ describe("sourcemap inject command — --allow-empty behavior", () => { test("path is a file, not a directory: throws with distinct message", async () => { const filePath = join(dir, "not-a-dir.txt"); writeFileSync(filePath, "hello\n"); - const { ctx } = makeContext(); + const ctx = makeContext(); try { - await runFunc(func, ctx, {}, filePath); + await func.call(ctx, {}, filePath); expect.unreachable("should have thrown"); } catch (err) { expect(err).toBeInstanceOf(ValidationError); @@ -203,16 +160,16 @@ describe("sourcemap inject command — --allow-empty behavior", () => { }); test("--dry-run + empty directory: still errors (dry-run is not an escape hatch)", async () => { - const { ctx } = makeContext(); + const ctx = makeContext(); await expect( - runFunc(func, ctx, { "dry-run": true }, dir) + func.call(ctx, { "dry-run": true }, dir) ).rejects.toBeInstanceOf(ValidationError); }); test("--dry-run + --allow-empty: succeeds silently", async () => { - const { ctx } = makeContext(); + const ctx = makeContext(); await expect( - runFunc(func, ctx, { "dry-run": true, "allow-empty": true }, dir) + func.call(ctx, { "dry-run": true, "allow-empty": true }, dir) ).resolves.toBeUndefined(); }); }); @@ -224,8 +181,7 @@ describe("sourcemap upload command — --allow-empty behavior", () => { beforeEach(async () => { dir = mkdtempSync(join(tmpdir(), "sentry-upload-cmd-")); - // resolveOrgAndProject reads env vars; short-circuit via SENTRY_ORG / - // SENTRY_PROJECT so the test doesn't need network or config files. + // Short-circuit resolveOrgAndProject so tests don't need DSN/config. savedEnv = { SENTRY_ORG: process.env.SENTRY_ORG, SENTRY_PROJECT: process.env.SENTRY_PROJECT, @@ -246,10 +202,10 @@ describe("sourcemap upload command — --allow-empty behavior", () => { } }); - test("empty directory: throws ValidationError mentioning empty-dir", async () => { - const { ctx } = makeContext(); + test("empty directory: throws actionable ValidationError", async () => { + const ctx = makeContext(); try { - await runFunc(func, ctx, {}, dir); + await func.call(ctx, {}, dir); expect.unreachable("should have thrown"); } catch (err) { expect(err).toBeInstanceOf(ValidationError); @@ -260,34 +216,29 @@ describe("sourcemap upload command — --allow-empty behavior", () => { }); test("empty directory with --allow-empty: succeeds silently", async () => { - const { ctx } = makeContext(); + const ctx = makeContext(); await expect( - runFunc(func, ctx, { "allow-empty": true }, dir) + func.call(ctx, { "allow-empty": true }, dir) ).resolves.toBeUndefined(); }); test("directory with .js files but no .map files: throws", async () => { - // Exact reproduction of the silent-failure mode: docs site had .js - // files emitted but no .map files, and upload reported success with - // 0 files uploaded. mkdirSync(join(dir, "_astro")); writeFileSync(join(dir, "_astro", "app.js"), "console.log(1)\n"); - const { ctx } = makeContext(); - await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( + const ctx = makeContext(); + await expect(func.call(ctx, {}, dir)).rejects.toBeInstanceOf( ValidationError ); }); - test("non-existent directory: throws before touching Sentry creds", async () => { - // Even with SENTRY_ORG/SENTRY_PROJECT cleared, the dir-check should - // fire first — that's the whole point of reordering the checks so - // local/unauthenticated invocations get actionable errors. + test("non-existent directory: throws before resolving org/project", async () => { + // Cleared so the dir-check has to fire first to produce a useful error. delete process.env.SENTRY_ORG; delete process.env.SENTRY_PROJECT; const missing = join(dir, "does-not-exist"); - const { ctx } = makeContext(); + const ctx = makeContext(); try { - await runFunc(func, ctx, {}, missing); + await func.call(ctx, {}, missing); expect.unreachable("should have thrown"); } catch (err) { expect(err).toBeInstanceOf(ValidationError); @@ -297,21 +248,16 @@ describe("sourcemap upload command — --allow-empty behavior", () => { }); test("error path does not mutate files (js-only dir)", async () => { - // Regression guard for the Bugbot finding that led to the - // discover-first refactor: previously `injectDirectory` ran BEFORE - // the emptiness check, so a js-only dir got debug IDs written into - // every JS before the bundler-misconfig error fired. Now discovery - // is a read-only pass; injection only runs when we've decided - // upload will proceed. + // Discovery must be read-only — injection only runs once we've + // decided the upload will proceed. mkdirSync(join(dir, "_astro")); const jsPath = join(dir, "_astro", "app.js"); const original = "console.log(1)\n"; writeFileSync(jsPath, original); - const { ctx } = makeContext(); - await expect(runFunc(func, ctx, {}, dir)).rejects.toBeInstanceOf( + const ctx = makeContext(); + await expect(func.call(ctx, {}, dir)).rejects.toBeInstanceOf( ValidationError ); - // File must be unchanged — no debug ID IIFE, no sourcemap comment. const after = await Bun.file(jsPath).text(); expect(after).toBe(original); expect(after).not.toContain("_sentryDebugIds"); @@ -319,14 +265,10 @@ describe("sourcemap upload command — --allow-empty behavior", () => { }); test("happy path: directory with JS+map pair invokes uploadSourcemaps", async () => { - // Prove the guard doesn't false-positive on a real upload path, and - // that we reach the API call with sensible artifact files. mkdirSync(join(dir, "_astro")); const jsPath = join(dir, "_astro", "app.js"); const mapPath = join(dir, "_astro", "app.js.map"); writeFileSync(jsPath, "console.log(1)\n"); - // Valid minimal sourcemap so injectDebugId's inject step doesn't - // choke parsing. writeFileSync( mapPath, JSON.stringify({ @@ -342,13 +284,12 @@ describe("sourcemap upload command — --allow-empty behavior", () => { "uploadSourcemaps" ).mockResolvedValue(undefined); try { - const { ctx } = makeContext(); - await runFunc(func, ctx, {}, dir); + const ctx = makeContext(); + await func.call(ctx, {}, dir); expect(uploadSpy).toHaveBeenCalledTimes(1); const callArgs = uploadSpy.mock.calls[0]?.[0]; expect(callArgs?.org).toBe("test-org"); expect(callArgs?.project).toBe("test-project"); - // One JS + one sourcemap artifact per pair. expect(callArgs?.files).toHaveLength(2); const types = callArgs?.files.map((f) => f.type); expect(types).toContain("minified_source"); From 5dcf415aeaa7f4dad4bc8050b0ed9d03b4b63571 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Sat, 25 Apr 2026 18:34:44 +0000 Subject: [PATCH 5/6] review: align --allow-empty wording, simplify Set construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final-review polish: - src/lib/sourcemap/inject.ts: `diagnoseEmptyDiscovery` now matches the Set-construction convention from `injectDirectory` (single ternary + one mutation) instead of clear-and-refill. - src/commands/sourcemap/{inject,upload}.ts: align `--allow-empty` brief wording (both: "Exit successfully when no JS + sourcemap pairs are found …"). - src/commands/sourcemap/inject.ts: drop "Phase 1 / Phase 2" comments (missed in the prior slop-removal pass). --- .../skills/sentry-cli/references/sourcemap.md | 4 ++-- src/commands/sourcemap/inject.ts | 19 +++++-------------- src/commands/sourcemap/upload.ts | 4 ++-- src/lib/sourcemap/inject.ts | 10 +++------- 4 files changed, 12 insertions(+), 25 deletions(-) diff --git a/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md b/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md index dcb8408ba..f8e991268 100644 --- a/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md +++ b/plugins/sentry-cli/skills/sentry-cli/references/sourcemap.md @@ -18,7 +18,7 @@ Inject debug IDs into JavaScript files and sourcemaps **Flags:** - `--ext - Comma-separated file extensions to process (default: .js,.cjs,.mjs)` - `--dry-run - Show what would be modified without writing` -- `--allow-empty - Exit successfully when no JS + sourcemap pairs are discovered (default: error out to catch silent build misconfigurations)` +- `--allow-empty - Exit successfully when no JS + sourcemap pairs are found (default: error out to catch silent build misconfigurations)` **Examples:** @@ -40,7 +40,7 @@ Upload sourcemaps to Sentry **Flags:** - `--release - Release version to associate with the upload` - `--url-prefix - URL prefix for uploaded files (default: ~/) - (default: "~/")` -- `--allow-empty - Exit successfully when no sourcemap pairs are found (default: error out to catch silent build misconfigurations)` +- `--allow-empty - Exit successfully when no JS + sourcemap pairs are found (default: error out to catch silent build misconfigurations)` **Examples:** diff --git a/src/commands/sourcemap/inject.ts b/src/commands/sourcemap/inject.ts index b5a00a516..a7f2ff623 100644 --- a/src/commands/sourcemap/inject.ts +++ b/src/commands/sourcemap/inject.ts @@ -100,7 +100,7 @@ export const injectCommand = buildCommand({ "allow-empty": { kind: "boolean", brief: - "Exit successfully when no JS + sourcemap pairs are discovered " + + "Exit successfully when no JS + sourcemap pairs are found " + "(default: error out to catch silent build misconfigurations)", optional: true, default: false, @@ -112,10 +112,10 @@ export const injectCommand = buildCommand({ flags: { ext?: string; "dry-run"?: boolean; "allow-empty"?: boolean }, dir: string ) { - // Phase 1 — read-only validation. Distinct errors for "directory - // missing" vs "directory empty/misconfigured". Discovery runs - // without side effects so we never write debug IDs into files when - // the upstream state is doomed (empty dir, typo'd path). + // Discover pairs read-only first so we don't error after partially + // mutating files. Zero *discovered* pairs (distinct from zero + // *injected* — the idempotent re-run case) almost always means a + // missing-.map bundler misconfiguration; --allow-empty opts out. await assertDirectoryReadable(dir); const extensions = flags.ext?.split(",").map((e) => e.trim()); @@ -123,21 +123,12 @@ export const injectCommand = buildCommand({ ? new Set(extensions.map((e) => (e.startsWith(".") ? e : `.${e}`))) : undefined; - // Guard against silent misconfigurations: zero *discovered* pairs - // almost always means the bundler didn't emit .map files. This is - // distinct from zero *injected* (which is legitimate when every - // pair already has a debug ID — the idempotent re-run case). - // Callers that legitimately invoke inject on potentially-empty - // directories can pass --allow-empty. const pairs = await discoverFilePairs(dir, extSet); if (pairs.length === 0 && !flags["allow-empty"]) { const diag = await diagnoseEmptyDiscovery(dir, { extensions }); throw buildEmptyDiscoveryError(dir, diag); } - // Phase 2 — mutating work (skipped in dry-run). The second pass - // through `injectDirectory` re-walks the directory; this is cheap - // relative to the sourcemap parsing/rewriting it does per pair. const results = await injectDirectory(dir, { extensions, dryRun: flags["dry-run"], diff --git a/src/commands/sourcemap/upload.ts b/src/commands/sourcemap/upload.ts index 5fe6a14da..b71b8e8d4 100644 --- a/src/commands/sourcemap/upload.ts +++ b/src/commands/sourcemap/upload.ts @@ -101,8 +101,8 @@ export const uploadCommand = buildCommand({ "allow-empty": { kind: "boolean", brief: - "Exit successfully when no sourcemap pairs are found (default: " + - "error out to catch silent build misconfigurations)", + "Exit successfully when no JS + sourcemap pairs are found " + + "(default: error out to catch silent build misconfigurations)", optional: true, default: false, }, diff --git a/src/lib/sourcemap/inject.ts b/src/lib/sourcemap/inject.ts index c2de1bef5..592daa7d0 100644 --- a/src/lib/sourcemap/inject.ts +++ b/src/lib/sourcemap/inject.ts @@ -198,13 +198,9 @@ export async function diagnoseEmptyDiscovery( ): Promise { // Build one set covering JS extensions + `.map` so the walker visits // both in a single pass. - const extensions = new Set(DEFAULT_EXTENSIONS); - if (options.extensions) { - extensions.clear(); - for (const e of options.extensions) { - extensions.add(e.startsWith(".") ? e : `.${e}`); - } - } + const extensions = options.extensions + ? new Set(options.extensions.map((e) => (e.startsWith(".") ? e : `.${e}`))) + : new Set(DEFAULT_EXTENSIONS); extensions.add(".map"); const absDir = resolvePath(dir); From a2f3d69a7f8a88883841d01cce82b77b6cc211e5 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Sat, 25 Apr 2026 18:43:27 +0000 Subject: [PATCH 6/6] fix(sourcemap): --allow-empty skips org/project resolution Cursor Bugbot caught: with `--allow-empty`, the empty-pairs path still called `resolveOrgAndProject` and threw `ContextError` when no DSN/env/config was available. The use cases the docs name for `--allow-empty` (library-only repos, conditional release skips) often run without that context configured, so the flag wasn't actually usable in its advertised scenarios. Short-circuit the empty + allow-empty branch before resolution. Make `org`/`project` optional on the result type so the no-op output renders without them. Adds a regression test that runs the path with SENTRY_ORG/SENTRY_PROJECT cleared. --- src/commands/sourcemap/upload.ts | 54 ++++++++++++++------------ test/commands/sourcemap/upload.test.ts | 12 ++++++ 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/src/commands/sourcemap/upload.ts b/src/commands/sourcemap/upload.ts index b71b8e8d4..853e31620 100644 --- a/src/commands/sourcemap/upload.ts +++ b/src/commands/sourcemap/upload.ts @@ -27,10 +27,11 @@ import { /** Result type for the upload command. */ type UploadCommandResult = { - /** Organization slug. */ - org: string; - /** Project slug. */ - project: string; + /** Organization slug. Omitted when --allow-empty short-circuits before + * org/project resolution. */ + org?: string; + /** Project slug. Omitted in the same short-circuit case. */ + project?: string; /** Release version, if provided. */ release?: string; /** Number of file pairs uploaded. */ @@ -39,11 +40,14 @@ type UploadCommandResult = { /** Format human-readable output for upload results. */ function formatUploadResult(data: UploadCommandResult): string { - const rows: [string, string][] = [ - ["Organization", data.org], - ["Project", data.project], - ["Files uploaded", String(data.filesUploaded)], - ]; + const rows: [string, string][] = []; + if (data.org) { + rows.push(["Organization", data.org]); + } + if (data.project) { + rows.push(["Project", data.project]); + } + rows.push(["Files uploaded", String(data.filesUploaded)]); if (data.release) { rows.push(["Release", data.release]); } @@ -123,24 +127,15 @@ export const uploadCommand = buildCommand({ await assertDirectoryReadable(dir); const pairs = await discoverFilePairs(dir); - if (pairs.length === 0 && !flags["allow-empty"]) { - const diag = await diagnoseEmptyDiscovery(dir); - throw buildEmptyDiscoveryError(dir, diag); - } - - const resolved = await resolveOrgAndProject({ - cwd: this.cwd, - usageHint: USAGE_HINT, - }); - if (!resolved) { - throw new ContextError("Organization and project", USAGE_HINT); - } - const { org, project } = resolved; - if (pairs.length === 0) { + if (!flags["allow-empty"]) { + const diag = await diagnoseEmptyDiscovery(dir); + throw buildEmptyDiscoveryError(dir, diag); + } + // --allow-empty: nothing to upload, so don't require Sentry + // credentials. This makes the flag actually usable in the + // library-only / conditional-release-skip cases the docs name. yield new CommandOutput({ - org, - project, release: flags.release, filesUploaded: 0, }); @@ -151,6 +146,15 @@ export const uploadCommand = buildCommand({ }; } + const resolved = await resolveOrgAndProject({ + cwd: this.cwd, + usageHint: USAGE_HINT, + }); + if (!resolved) { + throw new ContextError("Organization and project", USAGE_HINT); + } + const { org, project } = resolved; + const results = await injectDirectory(dir); const urlPrefix = flags["url-prefix"] ?? "~/"; diff --git a/test/commands/sourcemap/upload.test.ts b/test/commands/sourcemap/upload.test.ts index 719205c62..b453ad646 100644 --- a/test/commands/sourcemap/upload.test.ts +++ b/test/commands/sourcemap/upload.test.ts @@ -222,6 +222,18 @@ describe("sourcemap upload command — --allow-empty behavior", () => { ).resolves.toBeUndefined(); }); + test("empty directory with --allow-empty: does not require credentials", async () => { + // The library-only / conditional-release-skip cases named in the + // docs may run without DSN/org/project context. With nothing to + // upload, the command must not insist on resolving them. + delete process.env.SENTRY_ORG; + delete process.env.SENTRY_PROJECT; + const ctx = makeContext(); + await expect( + func.call(ctx, { "allow-empty": true }, dir) + ).resolves.toBeUndefined(); + }); + test("directory with .js files but no .map files: throws", async () => { mkdirSync(join(dir, "_astro")); writeFileSync(join(dir, "_astro", "app.js"), "console.log(1)\n");