Skip to content

fix(flags): reject empty variant keys for new variants - #118803

Merged
trunk-io[bot] merged 2 commits into
masterfrom
haacked/flags-reserve-empty-variant-key
Oct 16, 2026
Merged

trunk-io[bot] merged 2 commits into
masterfrom
haacked/flags-reserve-empty-variant-key

Conversation

@haacked

@haacked haacked commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • A multivariate flag with an empty "" variant key shows that variant's traffic as "no response" in flag calls charts, breakdowns and SQL.
  • flag_evaluations stores '' for both a "" variant and a missing response.
  • HogQL reads '' as NULL on a materialized events column, and fix(flags): read flag_evaluations response sentinels as NULL #117930 does the same for flag_evaluations.
  • The API only checks that a variant key is a string, so API, MCP and Terraform writes can still create a "" variant.

Changes

  • A write that adds a "" variant key now fails with "Variant keys can't be empty. Enter a key for each variant."
  • The rule follows fix(flags): reserve null as a variant key for new variants #118224, which reserved "null" the same way: "" joins RESERVED_NEW_VARIANT_KEYS.
  • Create rejects "". An update rejects it only when the stored flag lacks a "" variant, so existing flags with one stay editable.
  • The match is exact. A whitespace key such as " " does not read as NULL, so it stays allowed.
  • The error keeps the reserved_variant_key code. No client branches on it, and the fix is the same: choose another key. Only the message differs, because "The variant key is reserved" reads wrong.

Note

The held-key exception holds only while the #50084 validation rollout leaves structural.multivariate.variants[].key.blank unenforced, as production does today. Once that rule or * is enforced, the structural tier rejects every "" key, including one the stored flag already has. That rule has no stored-flag context. It is out of scope here.

Decisions that need no code change:

  • v2 config writes: the shared validator already rejects an empty string for a string flag's default and rule values, before the writer rules run. Adding "" to the shared set changes nothing there.
  • Frontend: validateFeatureFlagVariantKey already rejects an empty key with "Please set a key", with no exception for a saved flag. The UI already cannot save a flag that has a "" variant. That predates this PR and stays as is.

Mechanical: four API tests used "" as their example of a key that fails the structural tier but passes the serde guard. Three now use a 401-character key. The fourth needs two key violations, so it PATCHes a stored flag that already holds "". docs/internal/feature-flags/api-writes.md covers the new rule.

How did you test this code?

Ran test_filters_validation.py, test_feature_flag_config_v2_updates.py, and the TestFeatureFlagFiltersEnforcement and TestFeatureFlagFiltersMetrics classes locally. Did not run the full test_feature_flag.py; CI covers it.

Test rationale: Extended TestReservedVariantKey, the parameterized cases #118224 added for "null". New "" rows cover rejection on create (log-only and full enforcement), rejection when an update adds "", and acceptance when an update keeps a stored "" (log-only). The rejection rows also check the new message. With the old reserved set, three of the new rows fail. No new test file.

Release status

  • No feature flag controls this change

Docs update

Internal doc only (docs/internal/feature-flags/api-writes.md).

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Opus 5.5 (claude-opus-5-5)

  • Another session working on fix(flags): read flag_evaluations response sentinels as NULL #117930 handed off this task with a written brief. The user approved the PR.
  • Ran the /go pipeline with /simplify and /comment-cleanup. Invoked /improving-drf-endpoints, /writing-user-facing-copy, /writing-tests, /writing-code-comments and /writing-pr-descriptions.
  • The duplicate search (gh pr list --state open --search "empty variant key" and "reserved variant key") found no PR that already does this.
  • No customer data, tickets or logs went into the code or this description.

https://claude.ai/code/session_01PRnR48WeQWBNhWDPGLNcNL

@haacked haacked self-assigned this Oct 16, 2026
@trunk-io

trunk-io Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

@haacked
haacked requested a balanced review from Copilot October 16, 2026 20:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 6b53965: no findings.

This Standard review started from the posthog-review label. Comment @posthog review --deep for a Deep review, or @posthog help to see all commands.

@posthog posthog Bot removed the reviewhog label Oct 16, 2026
@haacked
haacked marked this pull request as ready for review October 16, 2026 20:41
@parameterai

parameterai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Risk: No findings

This change adds the empty string to the reserved variant-key set, so API/MCP/Terraform writes can no longer create a "" variant (which flag analytics store identically to a missing response), while flags that already hold one keep it; the error message is differentiated and tests/docs are updated. I traced every write surface using the shared helper — v1 serializer, v2 config writes, the cross-project copy path, and experiment flag creation — and all route through the same reserved-key check with a consistent held-key exception, so no bypass remains.

Sentinel reviewed 440f01f · Review settings

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team October 16, 2026 20:43
@posthog-project-board-bot posthog-project-board-bot Bot moved this to In Review in Feature Flags Oct 16, 2026
@haacked haacked added the stamphog Request AI approval (no full review) label Oct 16, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

The change enters API-contract and write-path territory, with owning-team authorship providing the required assurance. No unresolved substantive concerns or other refusal grounds were found.

  • Author wrote 52% of the modified lines and has 47 merged PRs in these paths (familiarity MODERATE).
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 19L, 2F substantive, 74L/5F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1b-small (74L, 5F, two-areas, fix)
stamphog 2.6.0 .stamphog/policy.yml @ 440f01f · reviewed head 440f01f

@github-project-automation github-project-automation Bot moved this from In Review to Approved in Feature Flags Oct 16, 2026
@trunk-io

trunk-io Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@trunk-io
trunk-io Bot merged commit 8007584 into master Oct 16, 2026
398 of 407 checks passed
@trunk-io
trunk-io Bot deleted the haacked/flags-reserve-empty-variant-key branch October 16, 2026 21:48
@github-project-automation github-project-automation Bot moved this from Approved to Done in Feature Flags Oct 16, 2026
@deployment-status-posthog

deployment-status-posthog Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-10-16 22:18 UTC Run
prod-us ✅ Deployed 2026-10-16 22:31 UTC Run
prod-eu ✅ Deployed 2026-10-16 22:32 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature/feature-flags Feature Tag: Feature flags stamphog Request AI approval (no full review) team/feature-flags

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants