Repository navigation
feat(bundler-plugin): capture the build's commit SHA for repo source fetching - #2
Merged
Merged
Conversation
…fetching
A remapped JS frame can only show its original source if the uploaded map embeds
`sourcesContent`. When it does not, the worker gives up — `_regenerate_context`
reports `no_sources_content` and the frame ships with no source at all.
The fix is to fetch the file from the repository the customer connected to the
app. Everything needed for that already exists on the backend: `build.vcs.commit_sha`
(validated + indexed), the `_lookup_build_vcs` join from a crash's app version to a
build record, `candidate_customer_refs` preferring a commit over tags and branches,
and `fetch_snippet` for GitHub/GitLab/Bitbucket. The single missing piece was that
nothing on a JavaScript build ever captured the commit it was made from.
This captures it. It does NOT reimplement CI/git detection: `bugsee-cli vcs-metadata`
is the canonical resolver, contractually wire-compatible with the Android Gradle
plugin and the appserver's `sanitizeVcs`, and the iOS agent and fastlane plugin were
both deleted in favour of shelling out to it. A fourth implementation, in JavaScript,
is the one that would drift. So the plugin spawns it and passes the result through
opaquely — a provider added on the backend needs no release here.
What it adds on top is the one case the canonical resolver has no notion of: a DIRTY
WORKING TREE. A SHA that does not describe the built source would make the backend
fetch and display the WRONG lines of code for a crash frame, so it is dropped by
default (`allowDirtyCommit` opts out). Untracked files are deliberately not dirt —
they change no committed file, and counting them would disable the feature on most
working checkouts. Crucially "cannot tell" is NOT "dirty": only a definite
`git diff --quiet HEAD --` exit 1 drops the SHA, because the most common deployment
shape there is — a CI container with no git binary and no .git, whose SHA came from
the provider's env var — reports unknown, and treating that as dirty would disable
the feature exactly where it works best.
Nothing here can fail a build. No git, no repository, no commits yet, a shallow
clone, a detached HEAD, a bugsee-cli that is missing or too old, a resolver that
threw, a probe that hung — every one degrades to "no commit recorded". Verified
against the real bugsee-cli binary and real repositories: a clean checkout yields
{commit_sha, branch}, a dirty one yields {branch}.
The delivery half is designed but NOT built — no bugsee-cli command can register a
build record without an artifact today. Options and the one open decision are in
docs/design/source-maps.md §9.4.
Also exports run-cli's spawn adapter as `spawnProcess`, so the git probe runs through
the same hardened options as the CLI spawn (never a shell, stdin ignored).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
…eam, and silence Two independent read-only reviews of the capture slice. Every real finding fixed test-first; the ten surviving mutants they named are now caught, plus twelve more against these fixes. SEV1 — the plugin could fail a build. `projectRoot` defaulted to `process.cwd()` inside `resolvePluginOptions`, which runs SYNCHRONOUSLY from the unplugin factory at config-evaluation time — outside every containment layer, not gated on failOnError. `process.cwd()` throws ENOENT/uv_cwd when the process's working directory has been unlinked, which a build script that recreates its own directory really does. So a plugin whose entire contract is that it cannot break a build could break one, and would do it even when fully disabled, since the field was computed before `enabled` was consulted. The cwd is now resolved lazily, inside the contained collection path, and a disabled feature resolves nothing at all. The injected `env` seam leaked. `resolvePluginOptions` read BUGSEE_BUILD_COMMIT from the injected env while `resolveVcsMetadata` fell back to `process.env` behind it, so an ambient value defeated an injected one — the exact duplication the comment claimed to be avoiding. The plugin layer now passes an empty env, having already consulted it. The module said nothing, while a comment claimed it turned a silent server-side discard "into something the build log can say out loud". It does now, for the two cases a user can act on: a malformed commit override (naming the value they typed) and a SHA dropped as dirty (naming allowDirtyCommit). Routed through the plugin's own onError sink so a host can capture it. Also: collection is memoized per project root — writeBundle fires once per OUTPUT and an SSR build emits several, so each was forking vcs-metadata AND git diff concurrently for one answer; released on settle, so a commit made mid-watch is still picked up. `vcs: false` with an explicit `commit` now honours the commit instead of discarding it. One timer instead of two in the dirtiness probe (the loser was never cleared, leaking a closure per rebuild under --watch). And `spawnProcess`'s signal message names the command that actually died — it serves `git` now, and hard-coded "bugsee-cli" into a line blaming the wrong binary. Test strengthening, all mutation-verified: the regex anchor is load-bearing (a suffixed hex run was accepted whole); the dirty probe's ARGUMENT is asserted (a process.cwd() mutant silently probed the wrong tree in a monorepo); the abort signal is asserted to reach the spawn AND to fire on timeout; the real dirty-repo path runs with NO probe injected, so binding the default to "always clean" — which disables the feature in production while every stubbed test stays green — now fails; shallow clone, staged-only and unborn-HEAD cases added; real-git tests insulated from the developer's global config (commit.gpgsign=true would have failed the suite) and split so one failure hides nothing. Three JSON fixtures that asserted `undefined` where the buggy path also yields `undefined` are relabelled as regression fixtures rather than dressed up as coverage. The user-facing doc no longer claims the feature works end to end: delivery is not built, and it now says so. Limitations the review surfaced — the probe running after the build wrote its output, covering the whole repo, and not checking the reported SHA against HEAD — are documented for users and recorded in design §9.5. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…aning per option A fresh review of both prior commits. Most of what it found was introduced by the FIXES in 225422a, which is the argument for the convergent gate rather than against it. The memoization is REVERTED. It was added so a multi-output SSR build would fork `vcs-metadata` once instead of once per output — except bundlers await each `bundle.write` in turn (vite's `for (const output of ...) await bundle.write(output)`; rollup's `hookParallel('writeBundle')` sits INSIDE each write), so the in-flight entry was always gone by the next output and the memo never fired for the case it existed for. What it did do was put a module-level map, shared by every plugin instance in the process, in front of a value that depends on `commit`, `allowDirtyCommit`, the notice sink and the injected resolver — none of which were in the key. Two plugin instances defaulting `projectRoot` to the same cwd, one with `allowDirtyCommit: true`, and the other joins its result and records a SHA for a dirty tree. That is precisely the invariant the feature exists to protect. Shared mutable state for a benefit that was never obtained; the honest fix is to delete it. `vcs: false` now means OFF at every layer. Round 1 made the resolver honour an explicit `commit` when disabled while the plugin layer short-circuited before ever calling it — so the two surfaces answered the same configuration differently, and the suite carried a green test pinning each contradictory answer. One option, one meaning: off records nothing, and a caller who wants a commit leaves `vcs` alone and sets `commit`. Notices move off `onError` to a new `onNotice` option. `onError` is documented as taking a contained failure and everywhere else receives an `Error`; hosts do `e.message`, `e instanceof Error`, or fail the pipeline on a non-empty list, and a plain string about a dirty tree breaks all three. A throwing sink is now swallowed too — `resolveVcsMetadata` is public API documented as never throwing, and an unguarded sink both made that conditional and cost the caller the whole metadata object (branch, repo, provider) rather than one message. Smaller, all real: the malformed-commit notice said "no commit will be recorded" when the code goes on to record the DETECTED one two lines later — the package's own test asserts that fallback. An empty `BUGSEE_BUILD_COMMIT` warned on every build forever, though CI templating of an unset variable produces `''` constantly and the token resolution beside it already treats `''` as absent. The signal-termination line now names no binary at all: "bugsee-cli" was wrong once this adapter also ran `git`, and `command` is equally wrong because on the DEFAULT install it is `process.execPath`, so an OOM-killed CLI reported that node had died. And the ORDERING NOTE I added in round 1 stated the ordering backwards — measured, it is `slot-freed -> caller-resumed`, which is *why* a sequential watch-mode caller finds the slot free; as written a maintainer acting on it would have changed the wrong thing. The usage doc claimed `dryRun` logs what was captured. It didn't — the plugin discards `uploadSourcemaps`' return value entirely, so a capture-only feature had no way to show a user it had done anything. It does now. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…round 2's guard Round 2's fixes held up. Round 3 found one narrow instance of the same pattern that made round 2 necessary, plus doc drift. Round 2 made a throwing `onNotice` harmless INSIDE the resolver and wrote the rule down absolutely: losing the whole metadata object because a host's logger blew up is a far worse trade than losing one message. But the dry-run diagnostic that the same commit added calls the sink DIRECTLY from `collectVcs`, inside the try whose catch returns undefined — so a throwing sink discarded the entire VcsMetadata object, branch and repo and provider included. Exactly the invariant round 2 wrote down, violated by the feature round 2 added, at the layer that owns the sink. Guarded there now, and pinned by a test at that layer rather than only at the resolver — which is why it slipped through. The malformed-commit notice also fired on the `enabled: false` path, where round 2 had reworded its tail to promise a fallback to "the detected commit, if any" — on the one path that detects nothing and records nothing. Two changes in one commit that were never composed. The off-return now precedes the notice. Two tests carried names describing things that are no longer true or that they cannot see: one asserted console fallback under a name about the removed `onError` routing (and was a straight duplicate of the test that replaced it — deleted), and one claimed to cover the resolver's `enabled: false` short-circuit when the plugin layer returns before the resolver is ever called. Renamed to state the half it actually pins; `vcs.test.ts` is what covers the other half. The sample verification docs quoted `bugsee-cli was terminated by signal SIGKILL` as reproducible evidence — it no longer is — and claimed every `BugseePluginOptions` field had been driven from a real build. The five VCS options have not been. Both say so now. A HAZARD CHECKED AND RULED OUT, worth recording because the failure would have been silent and total: `git diff --quiet` is stat-sensitive when the index is stale, and a CI checkout or a restored build cache rewrites every mtime — if that read as dirty, the SHA would be dropped on every CI build, killing the feature precisely where it matters most. Measured on git 2.50.1: porcelain `git diff` refreshes the index first, so an mtime rewrite AND a same-bytes rewrite with a new inode both read clean; only the plumbing `git diff-index` needs an explicit `update-index --refresh`. Now pinned by a real-git test so a future switch to plumbing cannot quietly reintroduce it. Also corrected the cost claim, which was wrong on both count and scope: capture spawns TWO subprocesses (vcs-metadata plus the git probe) per OUTPUT DIRECTORY, not one per build — the code comment and the usage doc disagreed with each other and design §9.5.3. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…te depended on Round 3's code was correct — the first round to introduce no new functional defect. What it introduced was three false claims, plus round 4 found a real hole none of the first three rounds had spotted. THE REAL ONE. Round 3 recorded the stale-index hazard as "measured and ruled out": a CI checkout rewrites every mtime, and if `git diff --quiet` read that as dirty the SHA would be dropped on every CI build — the feature silently dead exactly where it matters most. The measurement was right, but it made the guarantee a property of the HOST'S CONFIG rather than of the probe. The child inherits the ambient environment and so reads ~/.gitconfig, where `diff.autoRefreshIndex=false` restores the entire failure. The probe now pins it for the single invocation. `diff.relative=false` is pinned alongside it, closing the matching hole in the other direction: `diff.relative=true` scopes the diff to the cwd's subtree, which would have quietly contradicted the whole-repository behaviour both docs promise, and in a monorepo built from one package directory a dirty sibling would have stopped counting. Both are now asserted directly, and by real-git tests that write the hostile setting into the repository's own config — removing either pin fails them, and so does restoring the pre-fix argv. THE FALSE CLAIMS. Round 3's message and design §9.7 both recorded a test rename that is not in the tree: the edit was an unasserted string replace whose anchor had already been changed by round 2, so it silently did nothing while the documentation said it was done. The test kept a name claiming to cover the resolver's `enabled: false` branch at a layer that returns before the resolver is ever called — precisely the defect round 3 said it was fixing, left in place and recorded as fixed. Renamed for real this time, with the anchor asserted. The lesson is written into §9.8, because a no-op edit that reports success is worse than one that fails. Also: "Turning it on today costs one two short-lived subprocesses" — a botched edit in the very user-facing line round 3 was correcting. And a comment pointing "below" at code that sits above it. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…claimed docstring Round 5 was the first round to find no correctness defect, no build-failure path and no security regression. Two accuracy items, both real. THE ONE THAT MATTERED. The test guarding the CI-checkout hazard rewrote a file's mtime to NOW. Whether git sees a stat mismatch at all depends on whether it was built with USE_NSEC: without it mtime is compared at SECOND granularity, and the whole test body runs inside one second — so "now" matches the recorded stat and the test passes even with the pin removed. On several Linux distro builds of git, the assertion protecting "a CI checkout must not read as dirty" was therefore inert, while round 4's commit message claimed removing either pin fails these tests. It now uses the epoch, which cannot match at any granularity and cannot be racily-clean either, since the index's own recorded mtime is untouched. Re-verified by mutation on this machine: removing either pin, or flipping autoRefreshIndex to false, now fails. The probe's docstring claimed to pin THE settings its answer depends on. It pins the two most likely ones. `diff.ignoreSubmodules=all` also changes the answer — a submodule whose checkout differs from the recorded gitlink reads clean, so a SHA gets recorded that does not describe the built source — and so do the ambient GIT_DIR / GIT_WORK_TREE / GIT_INDEX_FILE that git sets for every hook, which point a probe run from a pre-commit hook at the commit-in-progress index. `ignoreSubmodules` is deliberately NOT pinned: a third `-c` would not fix it, because per-submodule `submodule.<name>.ignore` outranks the diff-level setting. Named in the docstring rather than papered over. Both remaining cases fail toward recording a SHA, never toward breaking a build. Also recorded, verified rather than assumed: the `-c` pins are safe on any git, because `-c` keys are not validated against a registry — on a git predating `diff.relative` (2.31) the pin is a silent no-op, not an error. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
… that "proved" it Round 6 was meant to be the convergence check on round 5's two small edits. Verifying them instead showed that two earlier rounds' conclusions were wrong — both because a plausible claim about git was reasoned about carefully and never actually run. Round 4 objected that the dirty probe's safety was a property of the HOST'S config, because `diff.autoRefreshIndex=false` in ~/.gitconfig would make a CI checkout's rewritten mtimes read as dirty and drop the SHA on every build. I accepted it and pinned the setting. It is a no-op. Measured, content identical, mtime rewritten, git 2.50.1: git diff --quiet HEAD -- (porcelain, what we run) 0 clean at BOTH settings git diff-index --quiet HEAD -- (plumbing) 1 dirty Porcelain `git diff` compares CONTENT for a stat-unmatched entry regardless of the setting, so it cannot affect this command. Round 3's original conclusion was right all along, and for a better reason than it gave: the safety comes from using the PORCELAIN, and a switch to `diff-index` would be the silent regression that costs every CI build its SHA. That is now what the mtime test actually guards — flipping the argv to `diff-index` fails it. Worse than the useless pin was the evidence for it. Rounds 4 and 5 both recorded "removing either pin fails these tests", and it was true — but only the literal argv assertion ever failed, never a behavioural one, because the setting cannot change the answer. A test that asserts an argv will always catch an argv change; that is not evidence the argv matters. Every pin is now backed by a test that drives real git and would fail on the BEHAVIOUR, with the argv assertion demoted to documentation. And the hostile-config assertion was inert twice over: it re-applied the same mtime after an earlier probe in the same test, and porcelain `git diff` rewrites the refreshed stat into .git/index — so the stat simply matched. An assertion riding on the side effect of the assertion above it. Deleted rather than repaired, since the setting is irrelevant. What survives is real and now properly tested: `-c diff.relative=false`. Measured, probing <repo>/pkg with a modified <repo>/a.txt exits 0 (clean) with the host's `diff.relative=true` inherited and 1 (dirty) with the pin. In a monorepo built from one package directory, a dirty sibling silently stopped counting — contradicting the whole-repository behaviour both docs promise. The lesson is written into design §9.10 because it generalises past this branch: a review finding about tool behaviour is a HYPOTHESIS until it is run. Three rounds of careful reasoning about `git diff` produced a no-op fix, a false doc claim and two assertions that could not fail; one five-line shell script settled it. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
`sourcemaps.e2e.ts` asserted the spawn sequence POSITIONALLY — `lines[0]` inject, `lines[1]` upload — and the commit-SHA probe inserts a call ahead of both, so every index shifted and the suite went red on CI. The gate that would have caught it locally was never run: `pnpm ci:local` does NOT include e2e, and this is a change whose entire observable effect is which subprocesses get spawned. `--e2e` is the gate for it, and it is what this commit was verified with. Fixed by asserting the sequence that now exists rather than loosening the check: 1. vcs-metadata --working-dir <cwd> 2. sourcemaps inject <outDir> 3. debug-files upload … --version 1.2.3 --build 99 The probe being FIRST is the part worth pinning: the metadata it resolves is the build's identity and has to exist before the upload that will eventually carry it. Two assertions are deliberately about what does NOT happen: - the upload argv is UNCHANGED by the probe. The metadata is collected and echoed on the result but is not on the wire yet — there is no field for it in the upload protocol (docs/design/source-maps.md §9). That assertion is what will fail, on purpose, the day one is added. - the probe is NOT given the app token. It is a local git query needing no credentials, so handing them over would expose them to one more subprocess for nothing. I first asserted the opposite, assuming it inherited the environment; it does not, and the real behaviour is the better one. Mutation-checked: defaulting `vcs` to false — the plausible way to lose the probe — fails this test. `pnpm ci:local --e2e --force` green, on a branch now merged up to main. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
Why
A remapped JS frame can only show its original source if the uploaded map embeds it in
sourcesContent. When it does not, the worker gives up —symbolfiles/sourcemap.py::_regenerate_contextreportscontext_unavailable = 'no_sources_content'and the frame ships with no source at all.esbuild, Rollup, Vite (incl.
hiddenmodes) and a real Next.js production build all embedsourcesContentby default. The gap istsc --sourceMapwithoutinlineSources, bareterser, and Rollup's opt-insourcemapExcludeSources. Real, but uncommon.Option B is to fetch the file from the repository the customer connected to the app. Datadog requires
sourcesContent; Sentry expects it and treats public-URL fetching as explicitly discouraged; BugSnag and Rollbar fetch over plain HTTP from the path in the map. Nobody fetches from a connected VCS repo.What the research found
Almost the entire pipeline already exists. The only genuinely missing piece was the commit SHA of a JavaScript build.
models/build.js:232-240sanitizeVcs), partial-indexedPOST /v2/apps/{token}/builds→builds.service.js:793bugsee-cli vcs-metadatajobs/issues.py::_lookup_build_vcscandidate_customer_refs— a commit beats tags/branchesfetch_snippet+vcs_auth.pyNo new backend schema is needed. (
get_build_by_commitis appserver-MCP-only, has no REST route, and is the reverse direction — not this join.)What this PR builds
resolveVcsMetadata/isWorkingTreeDirty/resolveCommitOverridein@bugsee/bundler-plugin-core, plumbed throughresolvePluginOptions(vcs,commit,allowDirtyCommit,projectRoot,onNotice) →runPluginUpload→uploadSourcemaps.It spawns
bugsee-cli vcs-metadatarather than reimplementing detection. That resolver is wire-compatible with the Android Gradle plugin'sVcsMetadataResolverand the appserver'ssanitizeVcs; the iOS agent and the fastlane plugin were both deleted in favour of shelling out to it. A fourth implementation, in JavaScript, is the one that drifts.The dirty-tree gate is the one case the canonical resolver has no notion of. A SHA that doesn't describe the built source would make the backend display the wrong lines of code for a frame. Untracked files aren't dirt. "Cannot tell" is not "dirty" — the commonest deployment (CI container, no
git, SHA from the provider's env) reports unknown.Degradation
Nothing here can fail a build. No git · not a repository · no commits yet · shallow clone · detached HEAD ·
bugsee-climissing or too old · resolver threw · probe hung · a host's notice sink threw ·process.cwd()threw → all mean "no commit recorded".Verified against the real
bugsee-clibinary and real repositories: clean →{commit_sha, branch}; dirty →{branch}.Not in this PR — the open decision
No
bugsee-clicommand registers a build without an artifact:upload buildrequires--artifactand hard-codesrequest_artifact_upload: true;upload build-inforequires ≥1 sidecar and an org feature flag;debug-files uploadcarries a closed 5-key metadata struct.Recommended: a register-only mode (
upload build --no-artifact) reusing the payloadxcode.rs:629-730already builds withvcsnested — no appserver change at all.environment.app.{version, package_id, build}→ build record. The SDK reports those fromlaunch(); the plugin defaults--version/--buildto0.0.0/0. They must agree.Review
Six rounds. Rounds 2–4 each found problems the previous round's fixes had introduced; round 5 was the first with no functional defect. Notable:
process.cwd()in the synchronous config path, outside every containment layer, firing even for a disabled plugin;bundle.write) while putting a process-wide map in front of a value whose key omittedallowDirtyCommit, so one instance could record a SHA for a dirty tree;vcs: false;diff.autoRefreshIndexoverride added on an unverified claim. Measured, porcelaingit diff --quiet HEAD --content-compares at both settings; only the plumbingdiff-indexis stat-sensitive. The "proof" that the pin mattered was an argv assertion catching an argv change. Removed; the surviving-c diff.relative=falsepin is backed by a real-git behavioural test (probing a subdirectory must still see a dirty file at the repo root), and the porcelain-vs-plumbing choice is now guarded too.Gates
pnpm ci:localgreen. 197 tests; 100% line/fn/stmt, ~99% branch. Per-entity mutator loop across eight rounds: 67 mutations, all caught — survivors were fixed by strengthening the assertion, never the implementation.🤖 Generated with Claude Code