feat(core,cli): emit the canary decision reason alongside the assignment

[vanceingalls]

Why now

The calibration contract deferred this deliberately: "Emitting a reason property is skipped — add it only if this check comes back dirty."

It came back dirty. The first fleet read (n = 27,226 non-CI installs on v0.7.92+) found 304 installs (1.08%) reporting both values for a canary whose percentage never moved. The genuinely anomalous ones could not be separated from a developer toggling HF_CANARY_*, because the assignment is identical in both cases — "true" is "true" whether it came from a cohort roll or a forced override.

resolveCanary has computed the reason all along; canaryEventProperties dropped it on the floor.

What changes

Every canary now emits canary_reason_<name> beside its $feature/canary-<name> assignment.

Deliberately outside the $feature/ namespace. PostHog treats those as flag values, and a non-boolean there would corrupt the flag's own breakdowns — the exact analysis surface the assignment property exists to feed.

Two values pay for themselves immediately

Beyond override attribution, of the six wire values (telemetry_opt_out never emits by construction):

• excluded — CI installs. These are excluded from percentage enrolment but still emit "false", so today they must be dropped by joining on is_ci. Conflating them with out_of_cohort is what made the first accuracy read look like a significant failure — 9.22% against a 10% target, z = −2.56 — when the non-CI population was on target at 9.49%. This kills that whole class of error.
• no_unit_id — the fails-closed corner when bucketSeed is missing.

Verification

Mutation-tested at both layers: dropping the reason in the CLI binding fails 6 tests, dropping it in the core helper fails 3.

New coverage pins the case this exists for — that a forced override and a genuine cohort roll produce an identical assignment and are told apart only by the reason — plus that CI reports excluded rather than out_of_cohort, and that no reason ever lands inside the $feature/ namespace.

Two existing exact-shape assertions updated rather than loosened.

Suites: core 1674, cli 2474, studio 3478, studio-server 416. Lint clean.

What this does not do

It will not settle the shared-identity question behind the residual 38 unexplained installs — it reports in_cohort/out_of_cohort on both sides of a flip, i.e. what was decided, not why the seeds diverged. That needs bucket_seed_origin or the already-merged identity_persistence reaching a release. This removes the larger, cheaper ambiguity first.

Where 38 comes from (it is not in #3030 — flagged in review, reconciling here): #3030's table reports 70 installs in the >1h post-settled bucket. A later root-cause pass split those 70 by whether the assignment tracks the CI flag within each install:

	installs
false events are exactly the CI ones → the documented exclude: is_ci rule meeting a shared distinct_id	32
mixed	1
never CI → genuinely unexplained	38

So 70 is the pre-attribution count and 38 is what survives removing the CI artifact. That pass also falsified the Docker/container hypothesis — flipped installs are less containerized than baseline (4.23% vs 12.98%), while CI is enriched 15.6×. Neither number is in #3030 because that PR was withdrawn to an empty diff; the record lives in the repo's internal plans/ handoff.

🤖 Generated with Claude Code

[miga-heygen]

Review: canary decision reason

This is the exact action item PR #3030 documented: "Emitting a reason property was deliberately skipped — to be added only if this check came back dirty. It did." Clean delivery.

What I verified

The reason was always computed but dropped. evaluateCanary in core already returns CanaryDecision { enabled, reason, bucket? } with the full seven-way classification. canaryEventProperties was destructuring only enabled and discarding the rest. The fix is one line in the CLI binding — passing reason through.

Namespace separation. The reason lives under canary_reason_<name>, deliberately outside $feature/. PostHog treats $feature/ values as flag states and expects booleans — a string like "forced_on" there would corrupt the flag's own breakdowns. Verified: the test on line 133 asserts no reason ever lands inside the $feature/ prefix.

Key sanitization. canaryReasonKey replaces [^A-Za-z0-9]+ with _ — safe for PostHog property names. No collision risk in practice: canary names are registered in a controlled registry.

The classification covers every evaluateCanary exit:

Path	enabled	reason
override === true	true	forced_on
override === false	false	forced_off
pct <= 0	false	out_of_cohort
pct >= 100	true	in_cohort
input.exclude (CI)	false	excluded
no unitId	false	no_unit_id
bucket < pct	true	in_cohort
bucket >= pct	false	out_of_cohort

Plus telemetry_opt_out from the CLI's telemetry-disabled path. Every exit produces a reason. No gaps.

Backwards compatibility. reason is optional in canaryFeatureProperties — callers without the full decision (e.g. Studio, which receives decisions from CLI) keep working. When undefined, no reason key is emitted.

Why two values pay for themselves

• excluded — replaces the manual is_ci join that anyone writing a canary query had to remember. The first accuracy read looked like a significant failure (9.22% vs 10% target, z = −2.56) because CI installs were in the denominator. With excluded, they're filterable at the property level.
• no_unit_id — surfaces the fails-closed corner case (missing bucketSeed) that was previously invisible.

Test coverage

• Core (6 new): reason alongside assignment, namespace isolation, forced-vs-rolled disambiguation, excluded for CI, omitted when not supplied, key sanitization
• CLI (4 new): reason reaches event properties, forced override reported as forced_on, CI as excluded, telemetry opt-out
• Existing (2 updated): shape assertions include reason properties

The critical test is on line 142: forced override and genuine cohort roll produce the same assignment but different reasons. That's the distinction the calibration read couldn't make.

CI mostly pending (only WIP so far). No blocking concerns.

— Miga

[miguel-heygen]

Exact-head review at 076657a63994a911deba65cd4d54131d5997e1d1.

Strengths

• packages/cli/src/telemetry/canary.ts:195 carries the already-computed decision reason through the only binding that previously discarded it, keeping assignment and attribution derived from the same cached decision.
• packages/core/src/canary.ts:215 keeps reason optional, so the Studio caller remains backward-compatible while full-decision callers can emit the companion property.
• packages/core/src/canary.test.ts:395 and packages/cli/src/telemetry/canary.test.ts:248 positively pin the actual failure mode: identical boolean assignments remain distinguishable as forced vs cohort-derived, and reason values stay outside the PostHog $feature/ namespace.

Audited: all four changed files; all production canaryFeatureProperties callers; CanaryReason and registry-name contracts; existing review/comments; exact-head check runs.

No blocking findings. GitHub Actions has only reported WIP plus skipped Mintlify at this head during the current Actions outage, so this approval is on code merits, not a claim that the normal matrix ran.

Verdict: APPROVE
Reasoning: The change is contract-compatible, the telemetry namespace is correct, and the tests pin the ambiguity this PR is meant to remove.

— Magi

[james-russo-rames-d-jusso]

Reviewed at 076657a.

Clean fix for the resolveCanary-computes-but-drops-it plumbing gap I called out on #3030. Wire shape is right: reason lives outside the $feature/ namespace so the flag column stays boolean and PostHog's own breakdown surface stays uncorrupted, and companion key canary_reason_<name> sits alongside $feature/canary-<name> on every event. canaryFeatureProperties extension is cleanly backward-compatible (reason?: CanaryReason optional, existing zero-arg callers keep working).

Tests exercise the four operationally interesting values (in_cohort, forced_on, excluded, telemetry_opt_out) plus the load-bearing "same assignment, different reason" pair — that's the specific ambiguity this whole PR exists to close. Mutation-tested at both layers per PR body, with 6 CLI + 3 core failures when the reason gets dropped again — the right kind of pin.

Findings inline — one blocker on the Studio-side parallel that this PR doesn't touch (same anti-pattern, verified still present at packages/studio/src/telemetry/canary.ts:264-266), and one small nit on missing no_unit_id emission-layer coverage.

Cardinality: bounded. Each canary contributes two properties ($feature/canary-<name> boolean + canary_reason_<name> closed enum of 6 wire values) per event. Current 3 canaries → 6 event properties per event, no blowup even at 10x. Reason values are a closed set from the enum, no per-user data flows through — no PII/secret leak surface. canaryReasonKey's [^A-Za-z0-9]+ → _ sanitizer is defensively wider than the registry's kebab-lowercase assertion needs, which is fine.

PR-body reconciliation nit (not code, so not inline): PR body cites "the residual 38 unexplained installs", but #3030's diff at its current head lists the >1h bucket at 70 and total flips at 304. Where 38 comes from is not in either doc. If it's a post-attribution number (e.g. after joining on #3065's identity_persistence, or a re-run with a narrower filter), a one-line footnote reconciling with the #3030 flip table would spare the next reader chasing the mismatch. Small.

Coupling with #3030: loose. This PR can land first, then #3030 rebases (DIRTY against main) and points at the merged emission by number — that's the cleaner ordering because the doc's "next step" language then references a merged, not-hypothetical, artifact. Alternatively #3030 lands with its independence-contradiction fix first and this lands right after; no hard ordering in code, either sequence works.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🔴 The identical anti-pattern is still live in Studio. packages/studio/src/telemetry/canary.ts:264-266 reads:

export function canaryEventProperties(): Record<string, string> {
  return canaryFeatureProperties(
    CANARIES.map((c) => ({ name: c.name, enabled: resolveCanary(c.name).enabled })),
  );
}

That is the exact shape this PR just fixed on the CLI side. Studio events emit $feature/canary-<name> without a companion canary_reason_<name>, so:

1. Any flip observed via a Studio-emitted event stays unattributable — override vs cohort roll blurs together in the same way it did CLI-side pre-feat(core,cli): emit the canary decision reason alongside the assignment #3074, in a surface that shares the bucket seed with the CLI when CLI-launched.
2. The re-read query the docs(canary): record the first fleet read — check 3 is dirty #3030 doc plans ("re-reading this table" after emission ships) either filters to CLI-only sources — blind to Studio flips — or accepts partial coverage on the surface most likely to expose a shared-__HF_CLI_BUCKET_SEED-with-diverging-CLI-anonymousId pattern (the closest wire signal to the shared-distinct_id Docker hypothesis).
3. Cross-surface canaries: de-parallel-router is CLI-only today, but the calibration canaries themselves emit on Studio events when a CLI-launched Studio inherits the seed. Asymmetric attribution here means the flip counts observed CLI-side and Studio-side won't reconcile.

Fix here — the Studio change is the same three-line shape as the one below, and the tests parallel canary.test.ts cleanly. If it's genuinely a follow-up (Studio event volume is smaller and Vance may want to isolate the release blast radius), name the follow-up in the PR body so the re-read query knows to filter Studio out and does not drop attribution silently. — Rames D Jusso

[james-russo-rames-d-jusso]

🟡 The describe("reason reaches the event properties") block covers in_cohort (line 250), forced_on (260), excluded (267), telemetry_opt_out (274) — but not no_unit_id, which the PR body advertises as the second of "two values that pay for themselves immediately". resolveCanary("test-gamma") at line 217 already exercises no_unit_id at the resolver layer (when configState.bucketSeed = undefined), so the emission-layer test is one line: unset bucketSeed, unset any override, then expect(canaryEventProperties()["canary_reason_test_gamma"]).toBe("no_unit_id"). Same shape as the four existing tests — closes the "advertised in body, not asserted in tests" gap for a value the body specifically names. — Rames D Jusso

[vanceingalls]

Fixed at b3990ac78. @james-russo-rames-d-jusso — the Studio finding was a real miss on my part, thank you.

🔴 Studio still dropped the reason. Confirmed at packages/studio/src/telemetry/canary.ts:264-266, identical shape. I treated "canaryEventProperties drops the reason" as one site when there are two, one per surface, and fixed only the CLI. The function's own doc comment promised "identical shape to the CLI, so a rollout spanning both reads as one flag" — which my CLI-only change had just made false, in the same file I should have been reading.

Fixed here rather than deferred, for the reason you gave: a CLI-launched Studio adopts the CLI's decisions and shares its bucket seed, so a flip can surface on either surface. Attribution on one side only would leave the two flip counts irreconcilable — and Studio is the surface most likely to expose the shared-seed-with-diverging-id pattern, which is the open question this property exists to answer. Deferring it would have meant the re-read query silently dropping attribution on the most informative surface.

Mutation-tested: reverting the Studio binding fails 3 tests.

🟡 no_unit_id advertised but not asserted. Correct — the resolver exercised it, the emission layer didn't. Added at the emission layer, one line as you described.

PR-body reconciliation: 38 vs 70. Also correct, and worth spelling out since the number genuinely appears nowhere. #3030's table reports 70 in the >1h bucket; a later root-cause pass split those by whether the assignment tracks the CI flag within each install:

	installs
false events are exactly the CI ones → documented exclude: is_ci meeting a shared distinct_id	32
mixed	1
never CI → genuinely unexplained	38

70 is pre-attribution, 38 is what survives removing the CI artifact. That pass also falsified the Docker/container hypothesis — flipped installs are less containerized than baseline (4.23% vs 12.98%), while CI is enriched 15.6×. Neither number reached #3030 because that PR was withdrawn to an empty diff (the calibration record belongs in internal docs, not a published contributor page); the record is in the repo's plans/ handoff. PR body now carries the reconciliation.

Ordering: agreed this lands first. #3030 is withdrawn rather than rebasing, so there is no coupling left to sequence.

Cardinality/PII audit — thank you for doing it. Matches my read: two properties per canary per event, reason from a closed 6-value enum, no per-user data.

Suites: core 1674, cli 2475, studio 3480, studio-server 416. Lint clean.

One caveat on this head, per @miguel-heygen's note: GitHub Actions has scheduled nothing repo-wide since 18:02 UTC, so the only checks present are WIP and a skipped Mintlify. The matrix has not run on either head. I would not merge on my local runs alone.