fix(sourcemap): error on zero pairs + restore docs sourcemap emission#846
Merged
fix(sourcemap): error on zero pairs + restore docs sourcemap emission#846
Conversation
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.
Contributor
|
Contributor
Codecov Results 📊✅ 5950 passed | Total: 5950 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
All tests are passing successfully. ❌ Patch coverage is 76.36%. Project has 12877 uncovered lines. Files with missing lines (3)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 75.17% 75.51% +0.34%
==========================================
Files 285 285 —
Lines 52476 52577 +101
Branches 0 0 —
==========================================
+ Hits 39443 39700 +257
- Misses 13033 12877 -156
- Partials 0 0 —Generated by Codecov Action |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 `<name>.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.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Addresses two follow-up bot findings on the previous commit (1094d9b): - **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.
- 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.
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).
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5dcf415. Configure here.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #845.
Summary
Two related bugs caused Sentry events for
cli.sentry.devto ship unsymbolicated in production with no CI signal:.mapfiles since the Astro 6 upgrade (security(deps): upgrade docs to Astro 6 / Starlight 0.38 #816).astro.config.mjssetsvite.build.sourcemap: "hidden", but Astro 6 reads fromvite.environments.{client,ssr}.build.sourcemap(Vite 7 Environments API), falling back tofalse. The legacy top-level path is silently ignored for the client bundle.sentry sourcemap inject/uploadsilently succeeded on zero pairs. The docs step logged "Files uploaded: 0" and exited 0 for 20+ consecutive main-push runs, hiding the misconfiguration.Behavior change
sentry sourcemap injectandsentry sourcemap uploadnow exit non-zero by default when zero JS+sourcemap pairs are discovered. Callers that legitimately invoke these commands on potentially-empty directories should pass--allow-emptyto preserve the old behavior.This is a deliberate default-strict change — the silent-zero-file failure mode has no legitimate use case in CI and turns every bundler regression into a silent production outage. Worth calling out in release notes.
Changes
docs/astro.config.mjs: setsourcemap: "hidden"onvite.environments.client(andssr, defensive). Verified locally:docs/dist/_astro/now contains 11.mapfiles (previously 0), andsentry sourcemap inject docs/dist/injects debug IDs into 5 module chunks.src/lib/sourcemap/inject.ts:discoverFilePairsfor read-only discovery (no writes).assertDirectoryReadable(dir)— distinguishes ENOENT / not-a-directory / EACCES with specific error messages.diagnoseEmptyDiscovery(dir)+buildEmptyDiscoveryError(dir, diag)— tailors the zero-pair error to the specific shape (empty dir / JS-only / maps-only / basename-mismatch).src/commands/sourcemap/{inject,upload}.ts:assertDirectoryReadable → discoverFilePairs → empty-check → resolveOrgAndProject → injectDirectory → upload. All pre-API error paths are side-effect-free — no debug IDs get written until we know the upload will proceed.--allow-emptyflag (boolean, defaultfalse).test/commands/sourcemap/upload.test.tscovering every empty-discovery branch, the non-existent-dir and not-a-directory branches, happy path (uploadSourcemaps spy), and a regression test that asserts the command does not mutate files on the error path.docs/src/fragments/commands/sourcemap.mdwith--allow-emptyexamples and bundler-config cheat-sheet.CI wiring
No workflow changes needed. The default-strict behavior catches any future regression automatically —
.github/workflows/ci.ymlanddocs-preview.ymldon't pass--allow-empty, so they'll fail loud if.mapfiles stop being emitted again.Verification
bun test test/lib/sourcemap test/commands/sourcemap test/lib/api/sourcemaps.test.ts).bun run typecheckclean.bun run linthas only a single pre-existing warning (unrelated, insrc/lib/formatters/markdown.ts).--allow-emptyand the happy path.Review cycle
1094d9bf. See review thread on that commit for per-item responses.--allow-emptyhint — fixed ine8452ad7with a regression test that locks in the discover-first invariant.Follow-ups (not in this PR)
Build Binarystep logs, where each target uploads itsbin.js.mapwith auth). Once this PR merges and a real docs push happens, zstd will finally be exercised on docs uploads too — I'll confirm from the resulting CI logs and update Docs sourcemap upload has been a no-op since Astro 6 upgrade (#816); CLI silently succeeds on 0 files #845.