Follow-up to #79: the DSN it shipped with cannot work, and the native path it enabled was unscrubbed

[bharathm03]

Follow-up review of #79, which merged before its review finished. Thirteen of fifteen findings fixed; the rest is recorded below.

Action required before any of this reports anything

Create the repo secret SENTRY_DSN_BRIDGE, with a numeric project id.

All three desktop bridge builds baked in secrets.SENTRY_DSN — the app's DSN, whose project id is a slug. The JS SDKs accept only a numeric project id (measured; sentry-dart differs, it takes the last path segment as an opaque String, which is why the app works). So #79 shipped bridge crash reporting permanently inert on macOS, Windows and Linux: every consenting host would initialise, capture, and flush successfully while transmitting nothing, and flush resolves true. Nothing on the machine or in CI would ever indicate a problem — the first time anyone looked for bridge crashes there would be none, and it would read as stability.

The builds now read SENTRY_DSN_BRIDGE. Until that secret exists, reporting stays off loudly, in host.log. errex must issue a numeric-id project for this; that part is not a code change.

The ordering bug this exposed

The DSN gate ran after Sentry.init. init installs both top-level process handlers before it ever looks at the DSN, and nothing takes them off again — Sentry.close() disables the client but leaves the listeners attached.

So on the shipping (refused-DSN) config, a client that could never transmit still owned every fatal path in the process. The warn-mode rejection handler console.errors the raw reason — full stack, unredacted paths — into a stderr teed to ~/.antgrid/host.log, and takes the rejection away from Bun's own reporting. A leak and a diagnostic regression, both on the path that believed reporting was off.

hasNumericProjectId now decides ahead of init, so a refusal installs no client and no listener. The test asserts the listener counts are unchanged.

Note that the SDK's own validateDsn is not the backstop it appears to be: it opens with if (!DEBUG_BUILD) return true, and DEBUG_BUILD is only typeof __SENTRY_DEBUG__ === "undefined" || __SENTRY_DEBUG__. Defining that false at build time — a routine bundle-size flag — would make the SDK accept a slug id and post envelopes to a URL built from it.

Native crashes were never scrubbed

sentry_flutter's C binding sets dsn/sample_rate/debug/environment/release/session/dist/max_breadcrumbs/database_path/handler_path and never sentry_options_set_before_send. sentry-native writes and posts its own envelope, so nothing from a Windows or Linux native crash passes through beforeSend — while beforeSend and its whole test suite sit inert one layer up.

The nativeDatabasePath change in #79 is exactly what turns that capture path on for the first time. It would have gone from broken-and-silent to working-and-leaking.

beforeBreadcrumb closes the part that is reachable: NativeScopeObserver mirrors the Dart scope down, and beforeBreadcrumb runs before the observers are notified, so scrubbing there keeps paths out of the copy the native layer holds. Native-originated frames and contexts remain outside Dart's reach by construction — the comment now states that rather than implying coverage.

Consent could silently revert

_resolveNativeDatabasePath's catch (_) { return null; } restored the exact behaviour the function exists to fix, with no symptom by construction: no handler process, no database, and (auto-session-tracking being a native option) no release-health session. The one signal that would show the pipeline was dead is the same one the fallback removed. Now logged.

main.dart reads consent through telemetryEnabledProvider rather than the settings object directly.

Smaller, all in the same family

• Modules was not excluded and event.modules not scrubbed. It walks up from process.cwd() for a package.json — and the host inherits the spawning app's cwd, so what it finds is not knowable from the bridge. Disk I/O on the fatal path for a field nothing reads.
• tracesSampleRate, spotlight and debug were undefined, and getClientOptions fills each from the ambient environment. SENTRY_SPOTLIGHT in a developer's shell would fan every envelope to a second loopback destination; SENTRY_TRACES_SAMPLE_RATE would emit transactions, which beforeSend does not see at all (that is beforeSendTransaction, never set). Pinned.
• redactNullable guarded on === undefined, so a runtime null reached String.replace and threw. A throw inside beforeSend is swallowed and drops the event — the report explaining the crash would be the one that never arrives.
• redactDeep assigned into an object literal, so a key named __proto__ ran Object.prototype's setter instead of creating a property, silently losing that entry. Now Object.fromEntries.
• debug_meta.images[].code_file is an absolute path and was travelling unscrubbed.
• A failed first-project open did console.error then a bare process.exit(1), reaching neither the flush nor either handler. A mint failure against a revoked credential pair lands there and nowhere else — the exact class of failure this instrumentation exists to answer.
• startControlPlane() moved below the handler registration. The old comment claimed the preceding window was one "where no PTY exists yet"; in fact startControlPlane writes host.json and logs ready, and only then spends seconds on the relay handshake and OAuth mint. With host.json on disk the app can drive project:open over loopback for that whole stretch, and a crash there would find Sentry's handler as the sole listener, take the fatal path, and skip the teardown that sweeps every PTY — survivable on Windows via the job object, orphaning the agent tree on POSIX.
• credentials.test.ts's headline assertion was vacuous: safeParse(base).data?.telemetryEnabled is undefined both when the payload parses without the field and when the schema rejects it. Dropping .optional() would have kept it green while every older app and CLI host died at readBootstrapPayload.
• crash-scrubber.test.ts now imports EXCLUDED_INTEGRATIONS instead of duplicating it, and its afterEach clears the client off the scope — close() leaves it there, so a later case read the previous case's DSN.

The rename fix from #79 was not quite right

Deferring disposal to scheduleMicrotask unwound the focus notification but still landed inside the frame showing the TextField, so the field outlived the controller and focus node it is built against. Any pointer, key or traversal event in that gap touches a disposed ChangeNotifier — a second crash in the same family, reachable only by timing, so it would not reproduce on demand. A post-frame callback runs after the setState rebuild that takes the field down.

The regression test also closed neither the CachedSessionsStore nor the ProjectSession; both own timers and subscriptions that would outlive the tree.

Not fixed

Finding 5 — consent is captured by value at first spawn. bootstrapBuilder closes over telemetryEnabled, and openProject assigns with ??=, so the first assignment wins for the app's whole life. HostController._scheduleRestart re-invokes that stale builder. A user opts out, the host later dies and auto-respawns with telemetryEnabled: true, and reports keep flowing from a machine whose owner revoked consent — nothing short of an app restart clears it. Revocation also never reaches an already-running host.

This contradicts credentials.ts, which documents the toggle as taking effect on the next spawn. It is a real consent bug and should be fixed, but the fix is a signature change across openProject/warmHost/_openInner plus three test doubles, and choosing where the live read comes from has a Riverpod lifetime trap (a per-project ref captured in a container-lifetime closure throws after disposal). That is a design call, not a review edit.

Finding 9 — desktop symbols are not archived. build-desktop.yml uploads only the .dmg, .msix and Linux bundle; PDBs and dSYMs die with the runner, and the toolchains are not bit-reproducible so rebuilding the tag yields non-matching build ids. Since this PR is what makes native capture work, the first real crash would arrive as module+offset and be permanently unreadable. The doc now says so; the CI change is left for a separate PR.

Testing

Gate	Result
bun run --filter antgrid-bridge test	3251 pass / 14 skip / 0 fail
tsc --noEmit (bridge)	clean
flutter analyze	No issues found
flutter test	3143 pass / 0 fail
check:font-tokens	clean
dart format	0 changed