[TOF-425] Implement flags and experiments skill - #42
Open
chenxing-mixpanel wants to merge 14 commits into
Open
Conversation
The existing flag and experiment skills operate entirely in the Mixpanel console. Neither opens a codebase, and manage-feature-flags explicitly defers per-language call signatures to the docs. Nothing owned the step between "flag configured" and "flag evaluating in the customer's code" -- which is exactly where a first-time user stalls. This skill owns that step, and ends at a verified exposure event rather than at generated code. - Five modes: Quick Start, Full Setup, Add a Flag, Migrate, Audit - Per-SDK reference covering all ten flag-capable SDKs, with the minimum version each one needs (below it, flags return nothing and raise no error) - Exposure-correctness rules, which differ by language and are the most likely thing to be hallucinated: client SDKs dedupe per SDK instance, server SDKs not at all; suppression is a positional arg on Node, only reachable via get_variant on Python, and a custom tracker on Go; the all-variants call never fires exposure on any SDK - Verification loop with local-testing options and a diagnostic checklist - Migration reference that leads with "do not migrate a running experiment" Declared engine: optional to match tracking-implementation -- code generation and Live View verification need no engine, and flag creation falls back to the Mixpanel UI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two problems in the shipped skills that the new implementation skill makes visible. Routing: manage-feature-flags and manage-experiment cross-route only to each other, so "add a feature flag to my app" lands on a console skill that cannot write code. Both descriptions now exclude implementation work, and manage-feature-flags' SDK reference routes to the implementation skill instead of deferring to the docs. Launching into the void: launch.md's readiness checklist was eight configuration checks with nothing verifying that any code evaluates the backing flag. An experiment could pass every check, go ACTIVE, irreversibly lock its variants, model and cohort, and then accrue nothing. monitor.md went on to misdiagnose the result -- its pace rule reads "pace means the design is wrong, not the experiment" and routes to sizing.md, offering a statistical remedy for a problem with no statistical cause. - launch.md: new first blocker for zero exposures on the backing flag, downgradable to a warning only on explicit confirmation that the code is merged and pending deploy (some teams ship and launch together). Step 2 routes it to the implementation skill rather than back to design, which cannot fix it. - monitor.md: "dead exposure stream" is now separate from slow pace, with distinct branches for never-had-exposures, had-them-then-stopped, and flag-disabled-or-ramped-to-zero -- the last of which is not an implementation problem and does not warrant terminating. - design.md: step 9 asks whether the implementation is in place before routing to launch. The draft auto-creates the backing flag, so its key exists at that point and implementation can finally begin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
/review-skill scored the skill 68% and /review-document scored its four references 43-65%. The most serious finding was mechanical: the code snippets did not parse. sdk-snippets.md (415 -> 325). Android had no init at all, just a comment placeholder, while SKILL.md promised "per-platform init code is in sdk-snippets.md" -- the exact call an agent would hallucinate, on a major mobile platform. Node and Go fences concatenated alternative local/remote configurations into one block, so both redeclared their client variable and neither ran; the Go fence also used time.Second without importing time and referenced a TrackerBuilder API that appeared in no snippet. Reworked so every fence is independently valid, wrote the real Android init, and moved the cross-SDK divergence table to the top -- its own contents list already said it was the part to read first, and it sat at line 398. exposure-correctness.md. The first-exposure-only recipe was presented as universally available; Java and Ruby document no per-call suppression, so a customer on either was being sent to an API that may not exist. Now gated on an availability table. The recipe also never named the method it told you to call, and described the dedup store as "a session flag, a cache key, or a column" without saying that only one of those satisfies the requirement -- a session store yields per-session exposure while the reader believes they have first-exposure-only. Added a worked example and stated the durability requirement. The consent-gating advice contradicted itself, prescribing a client-side manual exposure call the same section said does not exist. verification.md. Added the missing wrong-variant checklist -- SKILL.md promised four symptoms and the file covered three, with the causes scattered. Reordered the always-fallback list so "is the call even reached" sits third rather than last, behind a full code audit. Narrowed the local-testing section to the QA allowlist and marked the other two options as customer-owned rollout configuration, matching the scope boundary the parent now holds. migration.md. The clarifying blockquote contradicted the instruction it annotated. Kept the two-call API sequence, which the public migration playbook specifies, and rewrote the note to explain why both paths are real. Added the bucketing-key mapping step -- the file named a mismatched key as the top parity failure but never told anyone to map it -- and gave Part 1 a concrete import payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Skills ship snippets that agents paste verbatim, so a fence that does not parse is a defect in the product, not a typo in a doc. Review found two such fences that had been through authoring and human reading without anyone running them. Extracts fenced blocks from plugins/**/*.md and syntax-checks the languages a parser exists for: JavaScript as an ES module (so top-level await is allowed), Python, and Go. Verified against the pre-fix sdk-snippets.md, where it exits 1 on both known-bad fences. Deliberately reports what it could NOT check. A green run on a stripped runner would otherwise be indistinguishable from a green run that verified nothing, which is worse than no check at all -- so a missing toolchain emits a warning naming it and the count of unverified fences, and the summary prints verified-of-checkable rather than a bare pass. This is a syntax gate only. It will not catch undefined identifiers, wrong argument order, or a snippet that parses and is still wrong -- the Go redeclaration in the same review was a type error and passes this check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The init snippets were written from inference rather than from the SDKs, and three of the five client fences would have failed silently rather than loudly — the worst outcome for a file whose whole purpose is to prevent silent flag failures. Verified every call shape against the SDK source and fixed: - Swift: initialize(options:) is the only overload taking options, and token is required on MixpanelOptions. Also moved off the deprecated featureFlagsEnabled/featureFlagsContext pair. - React Native: the token belongs to the constructor, and featureFlagsOptions is the 5th positional argument of init(), not the 4th. In the old form the token landed in optOutTrackingDefault, so the SDK opted out of tracking and flags never enabled. - Python: LocalFlagsConfig declares polling_interval_in_seconds, not poll_interval, so the old snippet raised TypeError. api_host takes no scheme. - Node: trackExposureEvent takes a SelectedVariant, so suppress on getVariant rather than getVariantValue. - Android: guarded the checked JSONException that JSONObject.put throws. Node and Python both have isEnabled/is_enabled; the divergence table claimed neither did. Neither takes a fallback parameter, so both join that row instead. Python's suppression keyword genuinely differs by evaluation mode — local_flags takes report_exposure, remote_flags takes reportExposure — which the SDK marks with an explicit noqa. Swift, Flutter and React Native init now pass context, so the two rules stated above the fences are demonstrated rather than just asserted. Also trims the frontmatter description from 1136 to 1019 characters, back under the 1024 limit it had crossed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audit delegates its whole diagnostic path to verification.md, so a cause missing there is a cause the skill cannot find. Three were missing, and each is among the cheapest checks in its section: - Ingestion lag was documented only inside the verification loop, not in the zero-exposures checklist an audit actually lands on. A zero that is minutes old is not yet a zero. - An archived flag stops SDK evaluation outright and is hidden from default listings, so it serves the fallback forever while the key still looks correct. It appeared nowhere in the file. - QA allowlist entries pin a per-user variant rather than bucketing. That makes the test account the likeliest source of a "wrong variant" report, and pinned testers skew the split hardest at low volume. The "ordered cheapest-first" claim was strictly false: ruling out whether the call site is reached means instrumenting a build, and it sits ahead of five static reads. Reordering would bury the most decisive check, so the claim now describes what the ordering actually optimises for — cost against likelihood — in both the checklist and the Audit section that promises it. Also names the wrong-variant checklist in Audit, which listed three of the four, and fixes the runtime-property seam: passing custom_properties at init is this skill's code, while the rule targeting on it is the customer's configuration. The previous wording handed both away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five facts were stated in three or four places each, in near-identical prose. Duplication of this kind does not just cost tokens — it drifts. The Python suppression keyword was wrong in two files at once last week and needed two edits to fix. Each fact now has one owner that carries the mechanism, and every other site states only the consequence and links: - Flags off by default, and the init context rules -> the init section of sdk-snippets.md. SKILL.md kept the trigger sentence and dropped the restated mechanism. - Remote vs local evaluation limits -> the divergence table, with the body section no longer repeating the same tradeoff two lines apart. - Server-side exposure semantics -> exposure-correctness.md, which has a whole section on it. sdk-snippets.md and migration.md now point there instead of restating the rule. - Per-call suppression availability -> sdk-snippets.md. The duplicated matrix in exposure-correctness.md is now the prose gate that file actually needs, which is why the table was copied in the first place. - Flutter persistence on iOS and Android only -> the divergence table, which already said it. Terse checklist items in verification.md are left alone; a checklist has to be actionable without following a link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Part 2 told a migrating reader to create the experiment and then create the flag referencing it. Three sibling files say the experiment path auto-creates and links its backing flag, and that direct flag creation rejects the experiment type outright. The second call therefore either fails or produces the unlinked orphan the sentence was warning about — once per experiment across a scripted bulk run. The two-call sequence came from the public migration playbook, which I could not resolve to confirm. Rather than keep an unverifiable instruction behind a note that never said what happens to the auto-created flag, Part 2 now routes by object type: gates and configs create a flag directly, experiments create the experiment and read the auto-created flag's key back off it. If the playbook turns out to describe a real migration-only endpoint, the cost of this wording is one extra read; the cost of the reverse error is a duplicate flag per experiment. Also settles terminology the file used inconsistently: Dynamic Config casing, "old provider" versus "old vendor", and "runtime evaluation", which collided with the separate remote-versus-local evaluation concept and is really runtime-property targeting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three defects in the worked example, found by checking it against the Python SDK rather than reasoning about it: - from mixpanel import SelectedVariant is an ImportError. The package root re-exports only LocalFlagsConfig and RemoteFlagsConfig; the type lives in mixpanel.flags.types. - The example tracked exposure on whatever get_variant returned, including the fallback. _track_exposure has no fallback guard, so this emits $experiment_started with a null variant name — manufacturing the exact events the same file says never occur, and breaking the diagnostic that treats zero exposures and always-fallback as the same symptom. The caller has to guard it, so the example now does. - The Node comment described getVariantValue, which returns a raw value, where trackExposureEvent needs the variant object. The store-key rule was the contradiction's other half, and the prose was the wrong half. It required keying on assignment key + flag key, but exposure is dispatched per distinct_id, so deduping on a coarser key like company_id would record one exposure for an entire company and silently drop every other user in it. The rule now says distinct_id and explains why the bucketing key is not the right unit here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chenxing-mixpanel
changed the title
Mechanical cleanup ahead of a review re-run. - SKILL.md referred to "the three facts above", "see below", and "Quick Start's first four steps". The ordinal one breaks silently the moment a step is inserted; all three now name their target. - Step 7 restated the Handing off table it sits above. It now points at it, and the Scope line at the top stops being the third statement of the same boundary. - SKILL.md carried flat product claims with no source line while all four references had one. Added. - sdk-snippets.md declared routine init out of scope immediately before fifty lines of routine init. The fences are worth keeping — flags being off by default is the top failure mode and the opt-in line is not derivable — so the scope sentence is what was wrong. - "Sticky variants" drove the remote-versus-local decision in three places and was defined nowhere. Glossed at first use. - The OpenFeature section was missing from the Contents list, so an agent reading the index would never learn it exists. - exposure-correctness.md promised that unconfirmed claims are marked. Four are not, so the note no longer implies the absence of a hedge means verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clears the four Major findings blocking the review-skill checkbox. Migrate mode routed straight from the Mode Selection table into migration.md, around the only confirmation gate in the skill, and then prescribed the two least reversible operations it has: bulk-ingesting historical exposures into a production project, and scripted bulk creation of flags whose variant assignment key is immutable once enabled. Single-flag creation earned a gate; a several-hundred-flag batch did not. Both parts now require a preview and one explicit yes for the batch — row count, timestamp range, $insert_id derivation and a rendered sample row for the import; name, key, type, value type and assignment key per object for the recreation. Quick Start step 1 says experiment-backed flags must be created through the experiment path or they orphan, and step 3 then said "create the flag" with type as a confirmed field and no branch for Experiment. An agent that routed to Experiment met two contradictory instructions and would produce the orphan step 1 warns about. Step 3 now branches first. migration.md already handled this; the main flow did not. Finishes the de-duplication the previous pass did only partially: flags off by default, the two init-context rules, the local-evaluation limits and the Java/Ruby suppression warning each now have one owner, with the shared rhetorical phrasing removed from the copies rather than left in place. Also restores the "verify current" hedge that consolidation stripped from the local-evaluation claim in SKILL.md while leaving it on the owning row in sdk-snippets.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aims Five of the six Majors from the last review, plus the trims that hold. - The Mode Selection table advertised Quick Start as "verify -> enable", but step 6 says verification needs the flag already enabled, and the flow ends at hand off, not enable. An agent reading the table planned a step in the wrong place that the body then tells it not to perform. - The cheapest-first ordering rule governed all four diagnostic checklists until a previous commit deleted it from SKILL.md and left only the copy inside the fallback checklist. It now sits above all four. - exposure-correctness.md is now sole owner of the exposure-firing rules; sdk-snippets.md keeps the API shapes and points for behaviour. - SKILL.md and migration.md justified the same guardrail with incompatible mechanisms — one said direct creation orphans the flag, the other said it is rejected outright. Reconciled into one hedged sentence, with migration.md pointing at it. - Hedged the claim that repeat exposures don't inflate the denominator. It is the sole justification for letting a customer skip deduplication, and it was stated flat. - Reworded two product-API names in migration.md to stay engine-agnostic. Persistence policies and the OpenFeature section are trimmed to what is not derivable: the default and the TTL, and the provider list. The per-platform method-name matrix is deliberately kept. It reads as routine API reference, but it is the opposite: JavaScript uses is_enabled/get_variant_value while React Native uses isEnabled/getVariantValue, Ruby carries a trailing ?, and Go, Java and Ruby have no full-variant getter at all. None of that follows from language convention, and guessing it wrong is exactly the failure this file exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 10-platform method grid is gone, but none of its non-derivable facts are. They are now two rows in the Cross-SDK divergences table, which is where they always belonged: the section opens by calling itself "the reason this file exists — everything an agent will get wrong by generalising from another SDK", and these were sitting outside it, diluted by six rows that just restate each language's own conventions. The facts kept: JavaScript uses is_enabled/get_variant_value while React Native uses isEnabled/getVariantValue, two casings inside one language; Ruby's trailing ?; Java's get<Mode>() accessor call where others use a property; and no full-variant getter at all on Go, Java or Ruby. Also closes the rest of the review findings: - exposure-correctness.md:38 asserted flat what :88 hedges — that repeat exposures don't inflate the denominator — so an agent reading the dedupe section never saw the caveat. It now points at the hedge. - The exposure-firing rules left in sdk-snippets.md are gone, making exposure-correctness.md the actual sole owner rather than the nominal one. - The Mode Selection rows disagreed with their own bodies on membership: Quick Start omitted "route the type", which decides whether a flag gets created at all, and Add a Flag omitted flag creation entirely. - The immutability rule was stated three times inside migration.md; it now lives once, where the decision is made. - Hedged the QA-allowlist identity-snapshot claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit stripped "once the flag is enabled" from two of the three assignment-key statements in migration.md while leaving the third intact, turning one rule into two contradictory ones — locked at creation in two places, locked at enable in the third. Restored, and migration.md:75 now defers to the one statement that owns it. SKILL.md was conflating two genuinely different locks under one word: the flag key is immutable after creation, the variant assignment key once the flag is enabled. The confirmation gate called them "both irreversible" and now defers to the bullets that state each precisely. Server-SDK suppression availability was stated four times across two files and Python's split keyword three times. The divergence table row and exposure-correctness.md now carry the verdict and point; the per-language section owns the shapes. Separately, and found only by grepping every copy rather than the one in front of me: the Python example here still had `from mixpanel import SelectedVariant`, which raises ImportError — the package root re-exports only the two config classes. That was fixed in exposure-correctness.md several commits ago and the twin was missed, which is the same failure this commit is cleaning up after. Also defers the version-gate and sync-getter restatements to their owning sections, points the sticky-variants link at the section that actually defines the term, and hedges four unmarked product claims and defaults. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
implement-flags-and-experiments, a skill that implements Mixpanel feature flags and experiments in a customer's codebase, and wires the existing console skills to route into it.Two commits, separable for review:
ae42593— the new skill (5 files, 970 lines)references/sdk-snippets.md— all ten flag-capable SDKs with real call signatures and the minimum version each requiresreferences/exposure-correctness.md— the per-language exposure rulesreferences/verification.md— local testing, the verification loop, diagnostic checklistsreferences/migration.md— LaunchDarkly / Statsig / Optimizely / GrowthBook / OpenFeatureengine: optional, matchingtracking-implementationb5c920b— routing and the exposure gap (6 files, ~60 lines changed)manage-feature-flagsandmanage-experimentdescriptions now exclude implementation workmanage-feature-flags/references/sdk-and-exposure.mdroutes here instead of deferring to the docslaunch.md— new blocker for zero exposures on the backing flagmonitor.md— "dead exposure stream" separated from slow pacedesign.md— step 9 asks whether the implementation exists before routing to launchWhy
Nobody owned the code.
manage-feature-flagsandmanage-experimentnever open a codebase — across their 26 reference files the codebase is mentioned twice, both times as "grep for the flag key before archiving."manage-feature-flags/references/sdk-and-exposure.mddeferred per-language signatures to the docs outright, androuting-and-setup.mdnarrates implementation in the third person as someone else's job ("Engineer ships SDK code that reads the flag"). Their zero-exposures checklist bottoms out at "Is the SDK initialized in the client?" — diagnosed, unfixable, no repo access.So the step between "flag configured" and "flag evaluating in the customer's code" belonged to no skill. That is exactly where a first-time user stalls, and it's the step this skill owns — ending at a verified exposure event rather than at generated code.
The per-SDK surface is inconsistent in ways a model will confidently get wrong. This is most of the value in
sdk-snippets.md:get_variant_valueat all (you must drop toget_variant(..., report_exposure=False)); a customTrackerBuilderon Go; undocumented on Java and Ruby.isEnabledtakes a fallback on JS/Swift/Android/Flutter/RN, but not on Go/Java/Ruby.getAllVariants/get_all_variants/getAllVariantsByFlag) never fires exposure on any SDK — the cleanest way to run a live experiment that silently collects nothing.Experiments could launch into the void.
launch.md's readiness checklist was eight checks, all validating configuration, with nothing confirming that any code evaluates the backing flag. An experiment could pass every check, goACTIVE, irreversibly lock its variants/model/cohort, and accrue nothing.monitor.mdthen misdiagnosed the aftermath: its pace rule reads "pace means the design is wrong, not the experiment" and routes tosizing.md— a statistical remedy for a problem with no statistical cause. Zero times a longer window is still zero. The three edits in the second commit close this end to end: prevented at launch, caught correctly at monitor, routed to the skill that can fix it from either point.Type
Skill review
/review-skillhas not been run. This section is incomplete and the checkbox below is intentionally unchecked — please don't read it as passing./review-skillpassed (no blockers or majors)Running it before merge. Flagging one design decision likely to come up:
Testing
Verified:
scripts/check-engine-markers.shpasses; all 13 skills' frontmatter parses as valid YAMLSKILL.mdis 221 lines, within the 500-line limitENGINE.mdYOUR_PROJECT_TOKENplaceholders, no real credentials/docs/featureflagsNot yet done —
/review-skill, and an end-to-end trigger test in a real customer-shaped repo. The prompts below are what the skill is designed to handle; they have not been confirmed against a live session, and this section should be treated as incomplete until they are.manage-feature-flagsmanage-experimentThe last two matter as much as the first four: this skill and the two console skills now cross-route, and the failure mode to watch for is over-triggering on configuration requests.
Two docs bugs found along the way
Out of scope here, worth separate tickets:
mixpanel.flags.trackExposureEvent('test', variant)— Node has nomixpanel.flagsnamespace (it'slocal_flags/remote_flags) and the user-context argument is missing. As written it throws.python-flagsusesreport_exposure=Falsefor local evaluation andreportExposure=Falsefor remote on the same page. One is wrong.