🤖 refactor: remove remote PostHog feature flag evaluation - #3784
Conversation
Experiments were never rolled out remotely: none of the 18 EXPERIMENT_IDS exist as PostHog flags, so every lookup returned undefined and the service failed closed. Enabling an experiment has only ever been possible via the local Settings toggle, which makes the whole remote path inert while still emitting $feature_flag_called on every launch (20% of billable events). Removes remote evaluation, the variant disk cache and TTL, background refresh, and renderer polling. Local Settings toggles and their backend sync are unchanged, so no gated feature changes state for any user. - ExperimentsService keeps only local overrides and their persistence - experiments.getAll / experiments.reload had no remaining consumers - isExperimentLocallyEnabled collapses into isExperimentEnabled, which now structurally means "the user opted in locally" - localOverrideOnly is redundant now that it describes every experiment - feature_flags.json still writes an empty experiments map so a downgrade can read overrides back
Removing experiments.getAll dropped the renderer's fallback to backend override state, so a cleared localStorage left the Settings toggle showing off while the persisted backend override kept the gate on. That matters most for skill-dynamic-context, whose backend gate executes repo-controlled shell commands. Replaces per-experiment setOverride with a single sync call carrying the renderer's complete local state. The backend replaces its whole override map, so an override the renderer no longer has is cleared instead of orphaned, and the two layers cannot drift. Also drops stale references to PostHog assignment in docs that the removal made inaccurate.
userOverridable meant "the user can override the remote PostHog assignment". With remote evaluation gone, a local toggle is the only way to enable anything, so the field was true for all 18 experiments and every check on it was always-true branching. Removing it also deletes an unreachable default-value path in the non-hook reader. Also removes useAllExperiments, which had no consumers.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ff842e7bd
ℹ️ 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 flagged that replacing the whole override map on every renderer mount lets a client with empty origin-scoped localStorage (remote browser client, another profile, a changed port) disable every persisted experiment just by connecting. Restores the read the removal had dropped instead: the renderer fetches backend overrides and displays them when it has no local override, and writes stay per-experiment so a client can never clear overrides it never knew about. An explicit local toggle still wins, which also settles the race against an in-flight read. This resolves the original divergence too: a cleared localStorage now shows the backend's state rather than hiding an enabled gate behind an off toggle.
|
@codex review |
|
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". |
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
#3784 removed the `userOverridable` field from ExperimentDefinition along with the `exp.userOverridable === true` clause in the Settings filter, but left the comments that described that clause. The ExperimentsSection comment now misdescribed the code: the filter keys on `showInSettings`, and every experiment is a local opt-in toggle, so the "non-overridable ones are hidden" rationale no longer applies. Comment-only; no behavior change.
Summary
Removes the remote PostHog feature-flag evaluation mechanism from mux. Experiments were never actually rolled out remotely: none of the experiment IDs exist as PostHog flags, so every lookup returned
undefinedand the service failed closed. Enabling an experiment has only ever been possible through the local Settings toggle, which made the whole remote path inert while still emitting$feature_flag_calledon every launch. Local toggles and their backend sync are kept, so no gated feature changes state for any user.Background
PostHog data for the mux project showed
$feature_flag_calledhad grown to 20.2% of all billable events (391,465 of 1,938,370 over 90 days). Investigating the growth found the cause was not volume from real experiments:stats_tab_v1,post-compaction-context), and neither is inEXPERIMENT_IDS.EXPERIMENT_IDS.programmatic-tool-callingalone burned 10,475 calls returning nothing.Because
ExperimentsServicefails closed (override ?? cachedRemote ?? null, and the cache could never populate for an undefined flag), the remote layer could not influence any gate. All real uptake came from users toggling in Settings: 236 of 11,008 users have ever toggled anything, with keep rates between 64% and 86%.Every experiment added to the registry permanently added roughly one wasted event per user per launch, so the cost grew with the registry rather than with usage.
Notable consequence worth stating explicitly: no experiment has ever been remotely rolled out. Every "experiment" is an opt-in local toggle. If anyone assumed these were staged rollouts, that assumption was wrong.
Implementation
Three commits, ordered so the mechanical removal is reviewable on its own.
1. Remove remote evaluation. Deletes
client.getFeatureFlag, the variant disk cache and its 10-minute TTL, background refresh, renderer polling with exponential backoff, theexperiments.getAll/experiments.reloadoRPC routes, andTelemetryService.getFeatureFlag/getPostHogClient/getDistinctId(which had no other production callers).isExperimentLocallyEnabledcollapses intoisExperimentEnabled, which now structurally means "the user opted in locally"; that is the property theskill-dynamic-contextshell-execution gate depends on.localOverrideOnlyis dropped because it now describes every experiment.feature_flags.jsonstill writes an emptyexperimentsmap, because a build from before this change aborts reading the file when that key is absent and would silently drop the user's overrides on downgrade.2. Keep backend overrides authoritative across clients. Removing
getAlldropped the renderer's fallback to backend override state, so a cleared localStorage would show a toggle as off while the persisted backend override kept the gate on. That is most consequential forskill-dynamic-context, whose backend gate executes repo-controlled shell commands. The fix restores the read: the renderer fetches backend overrides and displays them when it has no local override, and writes stay per-experiment so no client can clear an override it never set. Precedence is local override > backend override > default. An explicit toggle wins, which also settles the race against an in-flight read.An earlier revision of this PR instead had the renderer replace the whole backend map on mount. Codex correctly flagged that as unsafe: localStorage is origin-scoped, so a remote browser client, another profile, or a changed port starts empty and would disable every persisted experiment just by connecting. That approach was replaced with the read-based one above.
3. Drop
userOverridable. The field meant "the user can override the remote PostHog assignment". With remote evaluation gone it wastruefor all 18 experiments and every check on it was always-true branching, including an unreachable default-value path in the non-hook reader. Also removesuseAllExperiments, which had no consumers.Deliberately unchanged: the 18 gated features. Deleting or force-enabling them is a set of product decisions, it would flip state for the 236 users who opted in and the 55 who opted out, and one gate guards shell execution of repo-controlled commands.
Validation
Red-green verified each new guard by toggling the implementation and confirming a genuine failure, then restoring and confirming green:
experimentsmap fails the test.overrides.clear()insidesetOverride) fails the test asserting a client with empty local state cannot clear overrides it never knew about.Also ran
tests/ui/layout/rightSidebar.test.ts(14/14), which exercises theagent-browserexperiment tab gating end to end through the localStorage path this change preserves.Risks
Low regression risk to gate behavior, because the resolution path strictly narrows: previously
override ?? cachedRemote ?? null, nowoverrideonly. Since no flag was ever defined in PostHog, no user's cache could hold a value, so resolved state is unchanged.One intentional behavior change at the edge: a manually seeded or hand-edited
feature_flags.jsoncontaining a cached variant no longer enables anything. That path is now ignored by design, since a remote assignment should not survive as an implicit opt-in.Affected product areas are the surfaces reading experiment state: Settings, right-sidebar tabs (browser, memory, timeline, workflows, desktop), slash commands and the command palette, multi-project workspaces, and AI tool registration. All read through the unchanged local toggle path.
Telemetry detail
setFeatureFlagVariantis intentionally kept. It attaches$feature/<key>super-properties to existing events rather than emitting new ones, so it costs nothing billable and it is what powers the enabled/disabled breakdown per experiment. Onlyclient.getFeatureFlagproduced the$feature_flag_calledvolume.The
experiment_overriddenevent drops itsassignedVariantproperty, which was alwaysnullonce remote assignment was gone. The zod schema, TS payload type, and call site were updated together.Pains
posthog-jsis now an unused dependency (zero source imports), but removing it changes the Nix offline dependency cache and requires a CI round-trip to recover the newoutputHash, since Nix is unavailable in the dev sandbox. Left out of this PR deliberately to keep the diff coherent.One pre-existing test failure is unrelated to this change:
WorkspaceService bash monitor wakes > accepted history suppresses redelivery while wake-store reconciliation keeps failingfails identically on the branch point (f14eade0c), verified on a detached worktree. It is a test mock missinggetForegroundToolCallIdsin bash-monitor code this PR does not touch.Generated with
mux• Model:anthropic:claude-opus-5• Thinking:max