🤖 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: 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 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".
|
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".
|
@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. |
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".
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".
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.
|
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. |
|
@codex review — rebased onto latest main and squashed to a single review-hardened commit (all 17 rounds' fixes included; history lives in the resolved threads). Reconciliation notes: the cross-process lock work now lives on the extracted FileLeaseManager as a withConfigWriteLock lease, with the review-driven protocol hardening (inspection/cleanup propagation, EPERM-present rule, rename-aside release, self-reclaim registry) applied to the shared protocol — the coder OAuth lease keeps its degrade-to-null contract; getConfig composes getClientConfig with the marker-aware telemetry state. Head is 5daedcb. |
6403c29 to
5daedcb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5daedcbd52
ℹ️ 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: Didn't find any major issues. Breezy! 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. |
Rebased onto the Effect config-semaphore refactor: editConfig's section routing and per-write file locking now layer on enqueueConfigEditEffect (queue turns re-check section state under their permit), and the telemetry routes use handlerGen. Squashed, review-hardened branch (17 Codex rounds), rebased onto the config-module split (FileLeaseManager extraction). The toggle: Settings → General switch plus a Toggle Usage Telemetry palette command (serialized read-modify-write), applied live via TelemetryService.setConfigEnabled — a runtime opt-out tears the client down un-latched so a peer process re-enable can lazily re-init, while app-quit shutdown latches permanently and drains in-flight init through the apply chain. isEnabled() reflects the live config gate; capture() re-checks per event. Fail-closed parsing: a present-but-invalid telemetryEnabled reads as disabled. Persistence is a two-record transaction (Config.setTelemetryEnabledPersisted): explicit field write in both directions, strict write verification with rollback, and a downgrade-surviving telemetry_opt_out sidecar marker — older builds' whitelist saves drop the unknown field; the marker restores the choice on upgrade, fails closed on unreadable stats, and reconciles from the explicit field at startup (2s lock budget) after a crash-split. The generic mux_config_write document writer refuses telemetryEnabled and runs its whole read->mutate->write under the same lock. Cross-process integrity rides FileLeaseManager: a new withConfigWriteLock lease (45s/60s) serializes every editConfig load->save, the telemetry transaction (sections never pass through; in-section edits bypass the queue on a serialized chain, joined before release), and tool writes. The shared dir-lock protocol is hardened from review: inspection/cleanup failures propagate instead of spinning the event loop, EPERM counts as contention only with a present lock, release is an owner-checked atomic rename-aside, and markers a process fails to unlink self-reclaim via a process-wide registry. Docs cover the toggle, the marker, and the pre-toggle-build limitation (XUM_DISABLE_TELEMETRY=1 for downgrades). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review — rebased onto latest main (145003e: Fable 5.1 promotion + the Effect refactors for config semaphore pipeline, streamManager resource seams, memoryConsolidation, and oRPC streams); conflicts resolved by re-porting this branch's changes onto the refactored structures. No behavioral changes beyond the rebase. |
db22308 to
dc78eb4
Compare
|
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: dc78eb4005
ℹ️ 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".
| // process's own breaker may reclaim it — marker names are | ||
| // generation-unique, so the registry can never match a lock a | ||
| // concurrent holder in this process currently owns. | ||
| unreleasedDirLockMarkers.add(ownerFile); |
There was a problem hiding this comment.
When the primary directory rename and fallback marker unlink both fail transiently with errors such as EIO or ESTALE, this records the completed generation only in the releasing process's module-local set. Fresh evidence beyond the earlier claimed atomic-release fix is this explicit double-failure path: a peer process cannot see that set, continues treating the marker's still-live PID as an active owner, and times out every config write until the original process acquires again or exits. Retry release or persist an owner-checked released state that all processes can recognize.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| "utf-8" | ||
| ); | ||
| } else { | ||
| fs.rmSync(this.telemetryOptOutMarkerFile, { force: true }); |
There was a problem hiding this comment.
When corrupted persisted state leaves telemetry_opt_out as a directory, fs.rmSync(path, { force: true }) throws ERR_FS_EISDIR rather than removing it. Startup reconciliation swallows that error, and an explicit re-enable writes telemetryEnabled: true, fails here, then rolls the field back while the directory continues to force the effective state off; subsequent startups cannot reconcile the now-absent field, so the Settings control remains unable to re-enable telemetry without manual filesystem repair. Quarantine or otherwise self-heal an unexpected directory-shaped marker.
AGENTS.md reference: AGENTS.md:L108-L113
Useful? React with 👍 / 👎.
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