Skip to content

feat(telemetry): add the OpenTelemetry pipeline and core Berd events - #16

Merged
kalvinnchau merged 7 commits into
mainfrom
simple-telemetry
Aug 18, 2026
Merged

feat(telemetry): add the OpenTelemetry pipeline and core Berd events#16
kalvinnchau merged 7 commits into
mainfrom
simple-telemetry

Conversation

@matt2e

@matt2e matt2e commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Berd had no product analytics. This adds the full renderer-to-gateway telemetry path plus the first catalog of Berd events, instrumented at the points where each user action actually commits rather than where it is merely requested.

Pipeline

  • An OTel logs pipeline in src/shared/telemetry (client.ts, exporter.ts, identity.ts) batches records and hands the serialized OTLP/HTTP JSON body to the Tauri layer, so no renderer code holds a credential or an endpoint.
  • src-tauri/src/commands/telemetry.rs owns auth and transport: anonymous POST /v1/bootstrap with a validated installation id, token TTL read from the response, a single 401 re-auth retry, and POST /v1/logs carrying x-berd-schema-version: berd-otlp-logs-v1 against the gateway's berd service and berd.telemetry scope.
  • The client stays inert unless enabled: development never reaches the wire, startup gates are bounded so a stalled bootstrap cannot wedge the app, dropped records are counted, and batch size and attribute lengths are capped under the gateway's body limit. Empty optional attributes are omitted rather than sent as empty values.
  • The staging/prod gateway hosts are not decided yet, so the endpoint defaults to a dummy .invalid host in the real endpoint's shape. Swapping in a real host is pure configuration — VITE_OTLP_LOGS_ENDPOINT, ALLOWED_OTEL_LOGS_HOSTS, the CSP connect-src, and the pinned test values — with no code-path changes (see the TODO(telemetry-otel) in vite.config.ts).

Events

src/shared/telemetry/events has one module per surface, covering app launch, chat sends, agent edits, home pins, and project mutations. Each is anchored to its commit point:

  • App launch fires once per app start, not per reload; detached session windows initialize telemetry but do not report a launch.
  • Chat send events fire at the user-message commit, including steer sends at the steer commit and released deferred-workspace sends; is_first_message is gated on the session history having replayed.
  • Agent Edit Completed is anchored to the persisted persona write, and agent creation is counted from the duplicate, import, and onboarding surfaces.
  • Pin/unpin reports only changes that survive persistence, uses the id the pin will keep, emits per item on bulk unpin, and stays silent for starter-agent seeding and the legacy-Berdy pin migration.
  • berdctl project mutations are deliberately excluded; the exclusion is documented at the command modules.

Ported from squareup/berd@5e28e6e1, adapted to this repository's drift (worktree startup modes, pre-send workspace setup) with the telemetry behavior and wire contract intact.

Related issue

None found — searched open and closed issues/PRs for telemetry, no duplicates.

Testing

Tests accompany each surface, pinning the wire body contract (exporter.test.ts, client.test.ts), the event attribute shapes (events.test.ts, homePinTelemetry.test.ts), and the commit-point behavior (useChatSessionController.test.ts, AgentBuilderRail.test.tsx, AgentsView.entry.test.tsx, usePinToHomeWidget.test.tsx, HomeView.test.tsx, OnboardingFlow.test.tsx, main.test.tsx).

Relevant gates for a reviewer to run: just check, just test, just tauri-check, just clippy. No user-visible UI change, so no screenshots.

@matt2e
matt2e force-pushed the simple-telemetry branch 5 times, most recently from cf80094 to b336bfd Compare August 17, 2026 22:45
kalvinnchau
kalvinnchau previously approved these changes Aug 18, 2026
@matt2e
matt2e marked this pull request as ready for review August 18, 2026 01:45
@matt2e
matt2e requested a review from a team August 18, 2026 01:45

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

REQUEST_CHANGES. Fresh static review completed for the exact three-dot comparison bec8c6e...eac38e8. Two blocking consent/boundary defects and two non-blocking native-boundary hardening issues are publishable. The supplied GitHub evidence was inspected: DCO, frontend checks/tests, frontend build smoke, clean-room install, transcript virtualization, and Tauri Rust checks on macOS/Windows/Linux all report success; required checks still independently govern merge readiness. Final self-check covered every changed telemetry user flow and instrumentation surface, accessibility, English/Spanish localization, navigation and consent guards, async/error/never-completes/lifecycle/race behavior, test honesty, project laws and design-system rules, duplicate overlap, changed-line anchors, and concrete user effect. No supplied review threads exist, and overlapping boundary concerns were consolidated.

Deterministic publication result: 2 blocking and 2 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.

Comment thread src-tauri/src/commands/telemetry.rs
Comment thread src-tauri/src/commands/telemetry.rs
Comment thread src-tauri/src/commands/telemetry.rs
Comment thread src-tauri/src/commands/telemetry.rs

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

COMMENT. Fresh static review completed for the exact three-dot comparison bec8c6e...8d88edf. Two new non-blocking findings are publishable. One still-present native-boundary concern is suppressed forever because its resolved authoritative thread contains a substantive human deferral. The prior Windows replacement, mid-flight revocation, and alternate-port issues are fixed at this SHA and are not findings. Supplied GitHub evidence was inspected: DCO, frontend checks/tests, frontend build smoke, clean-room install, transcript virtualization, and Tauri Rust checks on Windows and macOS passed; Tauri Rust checks on Linux failed. CI remains separate from the code verdict and required checks independently govern merge readiness. Final self-check covered every changed app-launch, consent, native export, chat send/steer/deferred-send, agent, project, Home pin, onboarding, search, feedback, and berdctl flow; accessibility; English/Spanish localization; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system and architectural-law compliance; duplicate overlap; current changed-line anchors; and concrete evidence plus user effect for severity. Unsupported and overlapping claims were removed.

Deterministic publication result: 0 blocking and 2 non-blocking finding(s) publishable; 1 duplicate(s) suppressed.

const hasPersona = overridePersona
? overridePersona.id !== null
: Boolean(selectedPersonaId);
const provider = selectedProvider;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 P2 · Chat telemetry ignores the dispatched target (non-blocking)

Foreground send telemetry derives provider and model from live controller selection state at commit time. The queued attempt itself dispatches with the authoritative target captured in sendOptions.sessionSelection, and a later provider/model selection can update pendingExecutionTarget and selectedProvider while the admitted attempt is still preparing. The event can therefore report a different target than the message actually used.

User effect: Analytics can attribute a successfully sent message to the wrong provider or model, making provider/model adoption and reliability data misleading even though the chat itself uses the correct captured target.

Recommended fix: Pass the active attempt's captured sessionSelection into the commit telemetry callback and derive both provider and model from that single authoritative target, falling back to the committed session target only when no attempt snapshot exists.

Test: Add a controller test that captures a queued attempt on provider/model A, changes the pending selection to provider/model B before invoking onUserMessageCommitted, and asserts both chat events report A only.

Comment thread src/features/settings/ui/SystemSettings.tsx
morgmart
morgmart previously approved these changes Aug 18, 2026
@matt2e
matt2e enabled auto-merge (squash) August 18, 2026 06:38
@matt2e
matt2e disabled auto-merge August 18, 2026 06:38
@matt2e
matt2e enabled auto-merge (squash) August 18, 2026 06:56

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

REQUEST_CHANGES. Fresh static review completed for the exact three-dot comparison 64979a0...0f7f0ef. One new blocking cross-platform build-integration finding is publishable. Two still-present concerns are suppressed by authoritative thread memory: one unresolved automated thread and one resolved thread with a substantive human deferral. The prior unavailable-consent UI issue is fixed at this SHA; the Windows settings replacement, mid-flight revocation, and alternate-port issues also remain fixed. Supplied GitHub evidence was inspected and all listed checks report success: DCO, frontend checks/tests, frontend build smoke, clean-room install, transcript virtualization, and Tauri Rust checks on Linux, Windows, and macOS. CI remains separate from the code verdict and required checks independently govern merge readiness. Final self-check covered every changed app-launch, consent, native export, chat send/steer/deferred-send, agent, project, Home pin, onboarding, search, feedback, and berdctl flow; accessibility; English/Spanish localization; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; release/build integration; design-system and architectural-law compliance; duplicate overlap; current changed-line anchors; and concrete evidence plus user effect for blocking severity. The documented startup limitation of the runtime telemetry toggle was treated as an explicit tradeoff rather than invented as a finding.

Deterministic publication result: 1 blocking and 0 non-blocking finding(s) publishable; 2 duplicate(s) suppressed.

Comment thread src-tauri/Cargo.toml
@matt2e
matt2e disabled auto-merge August 18, 2026 07:16
@matt2e
matt2e enabled auto-merge (squash) August 18, 2026 07:16
@matt2e
matt2e requested review from kalvinnchau and morgmart August 18, 2026 07:17
@matt2e
matt2e disabled auto-merge August 18, 2026 07:19

Copy link
Copy Markdown
Contributor

🤖 Follow-up review at 0f7f0ef: I don’t think this is ready to merge yet. All current checks are green, but these concerns remain:

  1. P1 — enforced telemetry can silently send nothing. VITE_TELEMETRY_ENFORCED=1 is not mapped to the native block-telemetry-enforced feature in the Windows feature mapper. The same drift exists in the hand-built Unix just bundle and bundle-debug feature lists. Those builds can hide the consent control while native consent remains default-off, so every export is rejected unless an enabled settings file already exists. Please map the flag on Windows and include the Cargo feature in both Unix recipes.

  2. P2 — the 256 KiB body guarantee is not UTF-8 safe. The sizing test uses ASCII only, while the cap is in JavaScript characters rather than bytes. With this PR’s pinned OpenTelemetry versions, an exact serializer probe for 128 berd_chat_session_started records with 256-character CJK provider/model values produced 264,667 bytes, above the configured 262,144-byte gateway limit. That batch gets a 413 and is permanently dropped. Lowering the batch limit to 64, or enforcing UTF-8 byte limits, plus a multibyte worst-case test would close the gap.

  3. P2 — foreground chat attribution still reads live selection state. The telemetry callback uses the controller’s current provider/model instead of the queued attempt’s captured sessionSelection. A selection change during preparation can therefore misattribute the committed send. The existing thread has the concrete fix context: feat(telemetry): add the OpenTelemetry pipeline and core Berd events #16 (comment)

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

NO NEW PUBLICATION. Fresh static review completed for the exact three-dot comparison 64979a0...4324a48 with zero new publishable findings. One still-present issue is suppressed because its authoritative automated thread remains unresolved, so approval is not recommended; one native-boundary concern is suppressed forever because its resolved thread contains a substantive human deferral. The prior Windows feature-gate mismatch is fixed at this SHA, as are the unavailable-consent UI, Windows settings replacement, mid-flight revocation, and alternate-port issues. Supplied GitHub evidence was inspected: clean-room install and DCO passed; frontend checks/tests and Tauri Rust checks on Linux, macOS, and Windows were in progress. Pending checks do not make validation incomplete, and required checks independently govern merge readiness. Final self-check covered every changed app-launch, consent, native export, chat send/steer/deferred-send, agent, project, Home pin, onboarding, search, feedback, and berdctl flow; accessibility; English/Spanish localization; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; release/build integration; design-system and architectural-law compliance; duplicate overlap; current changed-line anchors; and concrete evidence plus user effect for blocking severity. No unsupported finding was added merely to produce output.

Deterministic publication result: 0 blocking and 0 non-blocking finding(s) publishable; 2 duplicate(s) suppressed.

matt2e and others added 2 commits August 18, 2026 20:47
Adds an opt-in telemetry stack that reports a small, hand-authored catalog
of product events over OTLP logs, gated behind a consent setting that is
off until the user turns it on.

- Renderer client batches and exports events, enforces consent at emit
  time, aborts in-flight exports when consent is revoked, counts events
  suppressed while consent is unsettled, and flushes when a window hides
  or closes.
- Native Tauri commands own consent persistence and enforcement, gateway
  token exchange (clamping the reported TTL, keeping a sibling's fresh
  token on 401 invalidation), and OTLP endpoint validation that rejects
  userinfo and explicit ports.
- Wire schema `berd-otlp-logs-v1` tags events with distribution.channel
  and carries no user_id, agent_id, project_id, or item_id. An inert
  distribution-sink seam sits on the emit path.
- Events cover the app, chat, agent, project, and home surfaces, with
  telemetry anchors contained so a throw never breaks the caller.
- Staging builds point at the live staging gateway and production builds
  at otel.berd.xyz; the settings consent row hides without the telemetry
  capability.
- `just dev` prints every fired event to the terminal and the dev event
  viewer renders those lines grey.

Enforced-telemetry builds need two switches to move together:
`VITE_TELEMETRY_ENFORCED=1` for the renderer and
`--features block-telemetry-enforced` for the native side. All five build
paths now set both halves, so none can produce a build that hides the
consent toggle and then rejects every export as "Telemetry is disabled for
this installation". `_bundle-unix` and `_bundle-debug-unix` call
`scripts/block-feature-gates.sh` instead of hand-rolling their Cargo
feature list, `Get-BerdAppFeatures` carries the gate for every Windows
lane, and `build_linux_docker.sh` forwards the flag across the container
boundary. Windows cannot call the bash mapper (no guaranteed bash in the
release image), so the release-script tests derive the canonical gate list
from that mapper and pin the PowerShell table, `build-macos.sh`, and the
Docker forwarding list against it.

Also pins the OTel packages in lockstep at 0.221.0/2.10.0 and sherpa-onnx
at 1.12.40, and runs the app crate's telemetry tests in the tauri-test
gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…emetry consent

Apply the first-run landing page from d6f97db28 onto the telemetry
branch, replacing that commit's localStorage consent stub with this
branch's Rust-owned consent setting so the onboarding flow and the
Settings toggle drive the same opt-in pipeline.

- The landing page asks for anonymous usage-data consent up front with
  the checkbox defaulted ON — the one consent surface that defaults to
  sharing — while every other path leaves the persisted setting at its
  opt-in default (OFF). Advancing persists the choice through
  updateTelemetryEnabled(); the write is fire-and-forget and fails
  closed, so a failed write can neither block onboarding nor enable
  sharing.
- The source commit's consentPreference.ts and startup.ts are dropped:
  telemetry still initializes unconditionally at boot (events buffer
  through the consent gate and drop unless the setting loads as
  enabled), and the consent-store subscription brings the pipeline up
  the moment the welcome page or the Settings toggle grants consent.
- The welcome checkbox follows the Settings row's visibility rules:
  hidden in enforced-telemetry builds and without the telemetry
  capability, and hiding writes nothing.
- The "learn more" details become a shared UsageDataDialog (what we
  collect / what we don't), now also opened from a link on the Settings
  privacy row so both consent surfaces present the same story; its
  strings move to privacy.telemetry.usageDialog in the settings
  namespace.
- The source commit's consent gating around trackFeedbackSubmitted is
  moot here — this branch already removed that event.
- main.tsx keeps the per-window telemetry wiring pins but adopts the
  deferred boot: a loading view renders while get_installation_cohort
  classifies the install, established installs graduate straight past
  onboarding, and a cohort failure falls back to "unknown" without
  blocking boot.

Carried over unchanged from the source commit: the
get_installation_cohort command and versioned marker persistence,
onboarding graduation and persisted-state validation, graduating the
first-run-onboarding experiment to default behavior, localized dialog
close labels, and the artifact camera distance scale.

Validated with just check, just test, just tauri-check, just clippy,
and the app crate's installation_cohort tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

REQUEST_CHANGES. Fresh static review completed for the exact three-dot comparison 64979a0...78274b6. Two new blocking onboarding findings and one new non-blocking consent finding are publishable. Two still-present concerns are suppressed by authoritative thread memory: one unresolved automated thread and one resolved thread with a substantive human deferral. The prior Windows feature-gate mismatch, unavailable-consent UI, Windows settings replacement, mid-flight revocation, and alternate-port issues remain fixed. Supplied GitHub evidence was inspected and all listed checks report success: DCO, frontend checks/tests, frontend build smoke, clean-room install, transcript virtualization, and Tauri Rust checks on Linux, Windows, and macOS. CI remains separate from the code verdict and required checks independently govern merge readiness. Final self-check covered every changed app-launch, installation-cohort classification and migration, onboarding and consent flow, native export, chat send/steer/deferred-send, agent, project, Home pin, search, feedback, and berdctl flow; accessibility; English/Spanish localization; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; release/build integration; design-system and architectural-law compliance; duplicate overlap; current changed-line anchors; and concrete evidence plus user effect for blocking severity. The lower-confidence concurrent consent-writer concern was removed rather than overstated, and no finding was invented merely to produce output.

Deterministic publication result: 2 blocking and 1 non-blocking finding(s) publishable; 2 duplicate(s) suppressed.

Comment thread src/app/AppShell.tsx Outdated
Comment thread src/main.tsx
Comment thread src/features/onboarding/ui/WelcomeStep.tsx Outdated
matt2e and others added 2 commits August 18, 2026 22:21
A fresh install's first screen was whatever app startup was doing. AppShell
checked the startup loader and the connectivity diagnostic before the
onboarding gate, and `useAppStartup` hard-awaits the full chat runtime —
spawning and connecting to the `goosed` sidecar, loading runtime config,
refreshing provider readiness. So a slow sidecar showed a spinner and a
broken one (Gatekeeper/AV interference, port exhaustion, corrupted install)
showed a technical diagnostic, and in neither case did the user reach the
welcome page, which is also where telemetry consent is asked for. The
ordering predates this branch, but graduating first-run onboarding to
default behavior made incomplete onboarding the route for every fresh
install, so the fix belongs to finishing that graduation.

Onboarding now renders first, and the steps that actually need the runtime
take its state instead of assuming it is up:

- AppShell derives an `OnboardingRuntimeState` (`ready` means startup
  settled without an issue, since `startup.ready` is set even when startup
  threw) and hands it to `OnboardingFlow`. The dev-only `?startupLoading`
  override still preempts everything for loader parity, and once onboarding
  completes the loader and diagnostic gates apply as before — a still-broken
  runtime lands on the normal diagnostic with the consent and setup choices
  already captured.
- Agent adoption is the only onboarding step that calls the runtime
  (`listPersonas`/`createPersona` over ACP), so it is the only one gated:
  "Keep 'em" holds a pending state while startup is in flight and gives way
  to an inline error plus a retry when startup failed. "Skip for now" needs
  nothing and stays available throughout. Welcome, the work-type picker, and
  the harness picker are static, and harness setup drives native doctor and
  agent-setup commands that do not need the sidecar.

Normal startup latency now hides behind reading the consent page instead of
behind a spinner.

Validated with just check and just test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
`initialize_installation_cohort` threw away a classification it had
already positively determined, in two places. Because the previous commit
graduated first-run onboarding to default behavior, an `Unknown` cohort
routes an established user into the landing page — where the usage-data
checkbox is pre-checked ON and advancing persists that consent. So a
transient filesystem failure did not merely misroute someone; it
pre-answered a consent question they never triggered.

- An affirmative probe now outweighs a failing one. `Ok(true)` from the
  current-layout probe plus `Err` from `legacy_layout_database_exists`
  (or the mirror) returned `Err`, even though the first probe had already
  proven the install is established. Classification is now a match over
  both results: any affirmative sighting wins, `Ok(false)` twice is
  fresh, and only a genuinely indeterminate install — no sighting plus a
  probe that could not answer — is an error.
- The marker is demoted to the cache it is. A failure in
  `NamedTempFile::new_in`, `write_all`, `sync_all`, or a non-
  `AlreadyExists` persist error discarded the detected cohort; a full
  disk or an `fsync` failing on a network-mounted home directory turned a
  known-established user into `Unknown`. The write failure is now logged
  and the detected cohort returned. The `Ok(published)` arm is unchanged,
  so `persist_noclobber`'s winner semantics still hold.

An unpersisted *fresh* classification means the next launch reclassifies
as established (`berd.sqlite` exists by then) and skips the landing page.
That was already true of the old error path, so it is not a regression,
and it errs toward consent staying at its opt-in default.

Validated with the app crate's installation_cohort tests, just tauri-check,
and just clippy.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

REQUEST_CHANGES. Fresh static review completed for the exact three-dot comparison 64979a0...0dad9bd. One consolidated blocking onboarding finding is publishable: the previously resolved unknown-cohort issue still exists without a substantive human reply and can therefore be reopened, and the same fail-unsafe routing also occurs for a preserved future-version onboarding record. Three still-present concerns are suppressed by authoritative thread memory: one unresolved automated thread and two resolved threads with substantive human replies. The prior welcome-before-runtime issue is fixed at this SHA, as are the Windows feature-gate mismatch, unavailable-consent UI, mid-flight revocation, and alternate-port issues. Supplied GitHub evidence was inspected and all listed checks report success: DCO, frontend checks/tests, frontend build smoke, clean-room install, transcript virtualization, and Tauri Rust checks on Linux, Windows, and macOS. CI remains separate from the code verdict and required checks independently govern merge readiness. Final self-check covered every changed app-launch, installation-cohort classification and migration, onboarding/runtime/consent flow, native export, chat send/steer/deferred-send, agent, project, Home pin, search, feedback, and berdctl flow; accessibility; English/Spanish localization; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; release/build integration; design-system and architectural-law compliance; duplicate overlap; current changed-line anchors; and concrete evidence plus user effect for blocking severity. Overlapping established-user onboarding failures were consolidated into one finding, and no unsupported finding was added merely to produce output.

Deterministic publication result: 1 blocking and 0 non-blocking finding(s) publishable; 3 duplicate(s) suppressed.

Comment thread src/main.tsx
.catch((error) => {
console.error("Failed to resolve installation cohort:", error);
reportRendererError("installation_cohort_failed", error);
initializeOnboardingGraduation("unknown");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 P1 · Indeterminate installs enter onboarding (blocking)

The new detection logic improves affirmative layout handling, but a cohort lookup timeout, IPC failure, unreadable metadata, corrupt marker, or unsupported marker still becomes the unknown cohort, and initializeOnboardingGraduation graduates only the explicit established cohort. With no valid local record, unknown retains the initial welcome state. A second path has the same contradiction: for an established cohort, a future-version onboarding record is preserved without setting the in-memory snapshot to completed, then hydration interprets that record as INITIAL_ONBOARDING_STATE. In both cases the system has not positively identified a fresh install but routes it into first-run onboarding.

User effect: An existing user can be forced through onboarding after a transient cohort failure, damaged/future marker, or downgrade from a newer app version, interrupting access to their established workspace and making Berd appear reset.

Recommended fix: Make first-run onboarding require affirmative fresh-install evidence. Treat unknown as a bounded recovery/retry or conservative established outcome, and when an established cohort has a future-version onboarding record, preserve its bytes while setting the current in-memory lifecycle to completed rather than parsing it as a fresh state.

Test: Replace the unknown-is-incomplete expectation with timeout, IPC error, malformed/future marker, and metadata-failure cases proving they cannot show first-run onboarding without affirmative freshness. Add a downgrade test that preserves a future-version record byte-for-byte while getOnboardingSnapshot reports completed for an established cohort; retain a positive fresh-cohort test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is fine for launch

matt2e and others added 3 commits August 18, 2026 23:03
`ensureTelemetryConsentLoaded()` issues the one boot-time read of the
Rust-owned telemetry setting and its resolution callback overwrote the
consent store unconditionally, even though `updateTelemetryEnabled()`
settles the same store and nothing ordered the two.

The welcome page is exactly the caller that can lose that race: it calls
`updateTelemetryEnabled(true)` fire-and-forget with no `loaded` gate,
unlike the Settings toggle. If the boot read snapshotted the pre-write
value natively but resolved after the write settled, it clobbered the
store back to `{loaded: true, enabled: false}` — the renderer then
silently emitted nothing for the whole session and the Settings toggle
showed OFF while `telemetry-settings.json` said ON, self-healing only on
the next renderer. The error branch had the same bug with worse polarity:
a read that *failed* after a settled write force-disabled consent the
user had just granted.

Both callbacks now go through a `settleFromRead` helper that skips the
`setState` when the store is already `loaded`. That is safe in every
interleaving: the `loadStarted` guard admits exactly one read, so
`loaded` at resolution time can only mean the write settled first, and
the write reflects what the native side holds *after* the read's
snapshot was taken — causally fresher. If the read executed natively
after the write its value matches and skipping is a no-op; if before,
skipping is the correction. `updateTelemetryEnabled` keeps overwriting
unconditionally, since a completed write is always the freshest fact.

The failure log stays unconditional — a failed read is worth recording
even when its settle is superseded.

Validated with just check and just test; the two new consent tests fail
against the old callbacks and pass with the guard.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
WelcomeStep held the usage-data checkbox in useState(true), so navigating
welcome → work-types → Back remounted the step and silently reverted an
explicit uncheck to checked; advancing again then fired
updateTelemetryEnabled(true), overwriting an explicit opt-out without a
new deliberate choice.

The answer now lives in the onboarding store like every other step
choice, and the page still never reads the telemetry-enabled setting —
it only writes it:

- OnboardingState gains shareUsageData: boolean | null (null = the
  welcome ceremony has never been completed). It is recorded when the
  user advances, not per toggle, so declining is still just leaving the
  page. replay preserves it like completedHarnessSetupIds; reset clears
  it. Persisted-state validation accepts boolean/null/missing (missing
  hydrates as null, so no version bump) and rejects anything else.
- WelcomeStep displays a per-visit override layered on the recorded
  answer (visitAnswer ?? recorded ?? true): the first visit still
  defaults ON, and a revisit shows what was actually chosen.
- The consent write on advance is asymmetric. Sharing ON is written only
  when it is news — the first completion of the ceremony or a deliberate
  change on a revisit — so an untouched pass back through welcome can
  never overwrite a later Settings opt-out. Sharing OFF is always
  written when the user advances past an unchecked box: writing OFF can
  only ever stop data from being sent, it closes the check → advance →
  Back → uncheck → advance hole outright, and it retries an earlier
  disable whose fire-and-forget write failed.
- The consentHidden gate is unchanged: hidden means no write and no
  recorded answer.

Flow-level tests pin the round trip both ways: an opt-out stays
unchecked across Back and only ever writes false, and an untouched
opt-in revisit writes exactly once.

Validated with just check and just test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
recordHomeItemPinIntent/recordHomeItemUnpinIntent were the only telemetry
anchors on this branch with no try/catch, breaking the branch's "a throw
never breaks the caller" rule. Both land in recordIntent, which resolves a
chat's pin identity and maps the whole confirmed layout synchronously in
the caller's frame — and every one of the six call sites runs it after an
optimistic store mutation has already applied, so a throw always misreports
a change that succeeded:

- usePinToHomeWidget's pinToHome/unpinFromHome catch it on their success
  path and show a false "pin failed" / "unpin failed" toast.
- pinBatchToHome/unpinBatchFromHome throw mid-forEach, skipping the
  remaining addWidget/removeWidget calls — a partial batch plus an error
  toast.
- HomeView's canvas paths propagate it uncaught into the WidgetPicker add
  handler and the widget-frame/UnpinPill remove handler. Worse, the
  telemetry-only resolution there ran *before* removeWidget(id), so a throw
  meant the remove button silently did nothing.

Containment goes at the chokepoint, matching fireChatSendTelemetry: the
try/catch is inside the anchor, the catch calls perfLog, and a comment
states the structural constraint. Six call-site try/catches would be six
repetitions a seventh surface could forget, and once the anchor cannot
throw, the false-toast and half-batch paths close with no diff in
usePinToHomeWidget.

flushPendingIntents is contained too. It runs from the save-lifecycle
listeners so it can't break a product caller, but it maps the same
confirmed layout, and a corrupt one should stay a dropped event instead of
window.onerror noise. Its intents are now drained before resolution rather
than after reading the layout, so resolution stays terminal even when it
throws — a held-back intent would let the next save's transition, possibly
another window's, read as this user's action arriving.

HomeView keeps two guards for the resolution code the chokepoint can't
cover. The add wrapper wraps its entityPinTargetFromWidget + record block.
The remove wrapper must resolve before removeWidget (the instance has to
still exist), so it contains that separately: a resolution throw degrades
to "no intent recorded" rather than a dead remove button.

Tests drive the containment through a real seam — a confirmed layout
layoutItemsToHomeWidgets cannot map — rather than mocking the anchors away,
so they exercise the containment instead of replacing it. Both fail against
the uncontained module.

Validated with just check and just test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

NO NEW PUBLICATION. Fresh static review completed for the exact three-dot comparison 64979a0...17c34a7 with zero new publishable findings. Four still-present concerns are suppressed by authoritative thread memory: two unresolved automated threads and two resolved threads with substantive human replies. Because unresolved prior automated findings remain, approval is not recommended and no new publication is required. The latest changes preserve the onboarding consent answer across back navigation/replay, prevent a delayed startup read from overwriting a completed consent write, and harden Home pin telemetry without exposing a new supported finding. The welcome-before-runtime fix remains present; the Windows feature-gate mismatch, unavailable-consent UI, mid-flight revocation, and alternate-port issues remain fixed. Supplied GitHub evidence was inspected: DCO, clean-room install, and Linux Rust checks passed; frontend checks/tests and Rust checks on macOS and Windows were still in progress. Pending checks do not make validation incomplete, and required checks independently govern merge readiness. Final self-check covered every changed app-launch, installation-cohort classification and migration, onboarding/runtime/consent flow, native export, chat send/steer/deferred-send, agent, project, Home pin, search, feedback, and berdctl flow; accessibility; English/Spanish localization; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; release/build integration; design-system and architectural-law compliance; duplicate overlap; current changed-line anchors; and concrete evidence plus user effect for blocking severity. No unsupported finding was added merely to produce output.

Deterministic publication result: 0 blocking and 0 non-blocking finding(s) publishable; 4 duplicate(s) suppressed.

@kalvinnchau
kalvinnchau merged commit 9fefd79 into main Aug 18, 2026
8 checks passed
@kalvinnchau
kalvinnchau deleted the simple-telemetry branch August 18, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants