🤖 feat: add Usage Telemetry toggle to Settings → General - #3850
Conversation
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b20a688ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Both findings addressed in 9610baa:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9610baa072
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-2 finding addressed in 47d8ad0: when the reconciling |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47d8ad024f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-3 finding addressed in c5e3eae: the switch renders disabled while |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5e3eae115
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Both round-4 findings addressed in e54b346:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e54b34668d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-5 finding addressed in a9819ad: notifications arriving while local writes are in flight are no longer dropped — they set a missed-notification flag that replays the backend refresh when the pending-writes counter drains, so an external change landing during the write window (including our own write's early notification) always reconciles. Covered by a test where another client's enable arrives mid-flight during a local opt-out and the switch ends ON. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9819ad28a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-6 finding addressed in 34006f2: the persistence verification now re-reads with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34006f2930
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-7 finding addressed in 893cd6b: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 893cd6b2fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-8 finding addressed in e6aea88: the subscription now triggers a re-sync as soon as the iterator is connected, so a change landing between the initial snapshot and establishment can no longer strand a stale switch. Covered by a test that holds the subscription unestablished while another client opts out and asserts the switch syncs on connect (no event ever pushed for the missed change). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6aea88e97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Both round-9 findings addressed in 49a35c2:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49a35c27c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
49a35c2 to
bd10767
Compare
|
@codex review Round-10 finding addressed (branch also rebased onto latest main, clean replay): deferred-notification replays now go through a ref the api-change effect keeps pointed at the CURRENT client generation, so a write settling after an API replacement can no longer consume the notification against the disconnected client. Covered by a test choreographing write-pending → API swap → late old-client failure, asserting the switch syncs through the replacement client. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd107679e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-12 finding — cross-process
|
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ecafd938
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…in the generic writer Codex round 10: - tryBreakStaleDirLock's cleanup catches (orphan rmdir, marker unlink, final rmdir) treated every failure as benign, feeding withDirLock's no-await retry — an undeletable stale lock froze the event loop for the 45s deadline. Benign races (ENOENT, ENOTEMPTY) still continue; real cleanup failures (EACCES/EIO) propagate, and the acquire wrapper's lock-present rule turns them into a clear rejection. - writeConfigDocument refuses telemetryEnabled changes: the tool-facing generic writer holds neither the write lock nor the marker sync, so a direct field write could split the two-record transaction (opt-out with no downgrade marker; re-enable with a stale marker). Same-value passthrough still writes unrelated fields. Covered in xum_config_write.test.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b66aed0731
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex round 11: - writeConfigDocument's telemetryEnabled compare was a TOCTOU preflight: a toggle could complete its field/marker transaction between the writer's read and its whole-document save, which then restored the stale field. The config-document read/compare/write now runs under the same cross-process config_write.lock (via a public Config wrapper), so tool writes serialize against every editConfig and telemetry transaction. - isEnabled() now includes the live config gate: a peer process opt-out leaves this process's client non-null while capture() refuses per event, and /telemetry/status must agree with the effective state. The env gate needs no re-check (env is process-fixed; an env-disabled process never creates a client). Unit test flips the gate under a simulated client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review — addressed both round-11 findings: the generic config-document writer now runs its read/compare/write under config_write.lock (closing the TOCTOU), and isEnabled() reflects the live config gate so /telemetry/status agrees with capture(). Inline replies on the threads. Head is 1ca8e9f. |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ca8e9f456
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e->write Codex round 12: locking only writeConfigDocument left the tool's SOURCE read outside the lock — an editConfig landing between readConfigDocumentUnvalidated and the save would be overwritten by the stale whole-document snapshot even when telemetryEnabled itself matched. mux_config_write now wraps the entire read->mutate->write for config.json in withConfigDocumentWriteLock (exported from configReadWrite), and writeConfigDocument takes configWriteLockHeld to skip the nested acquire (the dir lock is not re-entrant). Providers documents keep their existing path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34ed4679f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d at startup Codex round 13: a crash between the verified field write and the marker sync left the two records permanently split. Startup now runs reconcileTelemetryOptOutMarker (under the write lock, before the enablement gates read either record): an explicit telemetryEnabled: false recreates a missing marker — also durably codifying hand-edited opt-outs — and an explicit true removes a stale one. An absent field with a marker present is deliberately left alone: that state IS the downgrade round-trip the marker exists to survive, a crash-mid-enable is indistinguishable from it, and failing closed is the privacy-safe direction (the next toggle repairs it). Best-effort by the startup-must-never-crash rule. Covered for all three states in config.telemetryEnabled.test.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review — round 13: crash-split field/marker records now reconcile at startup from the explicit field (inline reply); the peer-process Settings-UI staleness finding is resolved as out of scope — it is the pre-existing instance-local Config.onConfigChanged architecture affecting every setting, and this PR only improves cross-process behavior (rationale on the thread). Head is dd62993. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd62993908
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…refresh Codex round 14: - The toggle now persists telemetryEnabled explicitly in BOTH directions (no sparse delete on enable): a crash between the verified field write and the marker removal leaves an explicit true the startup reconciliation repairs from, instead of an absent field that reads as the downgrade-survivor state and restarts opted out. Absent still means enabled-by-default for untouched configs. - Startup reconciliation acquires the write lock with a 2s budget: a peer legitimately holding the lock must not stall app initialization for the 45s acquisition budget — telemetry stays fail-closed and the next toggle (or restart) reconciles. - GeneralSection's subscription refresh renders indeterminate state as ON when its single edge-triggered getConfig read fails (nonce-guarded): keeping OFF while collection may have resumed is the one lie the privacy switch can't tell — same doctrine as the toggle failure path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20c878a0e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sed markers Codex round 15: - tryInstallDirLock treats a rename EPERM as contention only when the target lock actually exists (P1): a policy/antivirus denial with no lock in place otherwise alternated with the breaker's ENOENT retry-now answer into a no-await spin for the full acquisition budget. - A release-path unlink failure (EIO/ESTALE) left a marker naming this still-live pid, which the breaker's owner-alive rule then honored forever — every later edit waited out the acquisition budget until process exit. Failed releases are now recorded in a per-process registry and self-reclaimed by the breaker; marker names are generation-unique, so the registry can never match a lock a concurrent section currently holds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89d0cec05b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… succeeds Codex round 16: the self-reclaim registry moves to module scope — Config instances can be short-lived (the tool's document writer constructs one per write), and an instance-held record vanished with its object while the orphaned marker kept naming this live pid. The registry entry is now removed only once the marker's unlink provably succeeded (or the marker is confirmed absent), so a second transient failure cannot consume the one record that makes the orphan reclaimable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 39bde424ee
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…half-released generation Codex round 17: an unlink failure after a completed write left a marker naming this live pid that PEER processes (blind to the in-memory orphan registry) honored as a live owner, stalling their config writes while this process sat idle. Release now renames the whole generation aside in one owner-checked syscall — the lock path is vacated atomically for every process, no partial unlink/rmdir state is observable, and the renamed remains ride the existing .stage- TTL sweeper when the immediate delete fails. The unlink+registry path remains as the fallback when the rename itself fails (same-process self-reclaim, as before). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
1 participant
Summary
Adds a Usage Telemetry toggle to Settings → General (under a new Privacy group) so telemetry can be disabled (and re-enabled) from the UI. The choice persists as
telemetryEnabledin~/.xum/config.jsonand applies immediately — disabling shuts the PostHog client down mid-session, re-enabling re-runs the full enablement check.XUM_DISABLE_TELEMETRY=1remains a hard override that wins over the toggle; when the environment forces telemetry off, the switch renders disabled with a note saying so instead of pretending to control anything.Background
Telemetry originally had a client-side opt-out (referenced in #905 when reporting moved to the backend), but that surface disappeared along the way, leaving the environment variable as the only user-facing switch — which is hard to apply to a Dock-launched macOS app (GUI apps don't inherit shell profiles). A Settings toggle is the conventional surface for this in developer tools; docs continue to point at
payload.tsfor transparency about what is sent.Implementation
telemetryEnabled?: booleanconfig field (absent/true = enabled, false = disabled), stored sparsely: re-enabling deletes the key.shouldEnableTelemetrygains adisabledByConfiginput;TelemetryServicereceives anisDisabledByConfigcallback from the service container and consults it duringinitialize()and percapture()— the per-event re-check keeps API-server processes honest even if a toggle apply hasn't reached them.config.updateTelemetryEnabledroute persists the choice and callstelemetryService.setConfigEnabled()for the live apply. Applies are serialized (a promise chain) andinitialize()is re-entrant-safe, so rapid toggling can't interleave PostHog shutdown/init and strand a live client after an opt-out;shutdown()nulls the client before awaiting the flush so nothing can capture into a draining client.isExplicitlyDisabled()now includes the config opt-out, so features gated on explicit opt-out (e.g. link sharing) treat the Settings toggle the same asXUM_DISABLE_TELEMETRY=1.getConfigexposestelemetryDisabledByEnv; the Settings row renders the switch hard-disabled with an explanatory note when the environment override is active.Config.saveConfigswallows disk errors, so the route re-reads the persisted value aftereditConfigand fails loudly (before touching the live client) when the write didn't land. On the frontend, each toggle records an intent id — a superseded request's failure no-ops, and the latest intent's failure reloads the backend truth; if that truth is unreachable too, the switch renders ON (indeterminate state must never read "off" while collection may continue) until a successful config load reconciles it. Rapid toggling can't be clobbered by an early failure.1.Review-round hardening
Eleven Codex review rounds tightened the privacy edges (all threads resolved):
~/.muxthatexistsSyncwould mask as "missing" — reports disabled; only a genuine ENOENT (fresh install) means enabled. The RPC's persistence verification uses a strict read whose failure fails the request rather than masquerading as a confirmed opt-out.initialize();shutdown()nulls before flushing;capture()re-checks the config per event and lazily re-initializes (rate-limited) when another process re-enables telemetry.Validation
isExplicitlyDisabled()reflects the config opt-out;telemetryEnabledround-trips thesaveConfigwhitelist (including clearing back to default).Risks
Low. The enablement change is additive (one new early-return input); with the field absent, behavior is byte-identical to today. The live-apply path reuses the existing
shutdown()/initialize()lifecycle, now serialized against concurrent applies. Worst case on a config read failure inside the callback is telemetry staying in its startup state.🤖 Generated with Claude Code