Skip to content

fix(flags): don't panic on a numeric group key in local evaluation - #332

Merged
dustinbyrne merged 1 commit into
PostHog:mainfrom
breken-ai:fix/numeric-group-key-panic
Sep 30, 2026
Merged

dustinbyrne merged 1 commit into
PostHog:mainfrom
breken-ai:fix/numeric-group-key-panic

Conversation

@breken-ai

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

Groups is map[string]interface{}, and Groups{"company": 42} is a natural way to pass a database ID. With local evaluation enabled, any group-aggregated flag then panics:

panic: interface conversion: interface {} is int, not string
github.com/posthog/posthog-go.(*FeatureFlagsPoller).computeFlagLocally  featureflags.go:1106

computeFlagLocally does groups[groupType].(string) without a check. The panic happens on the caller's goroutine in:

  • GetFeatureFlag, IsFeatureEnabled, GetFeatureFlagPayload and GetAllFlags;
  • Enqueue of a Capture with SendFeatureFlags, which evaluates every flag, so a plain capture call panics once any group flag exists.

The mixed-targeting path (#192) uses a checked assertion but then continues, so a numeric key silently skips a condition-level group condition and the flag comes back false.

The flags service accepts a JSON number as a group key and buckets it by its decimal form (hashed_identifier in rust/feature-flags/src/flags/flag_matching.rs: Value::Number(n) => n.to_string()). posthog-python and posthog-node build the hash input with string formatting, so 42 hashes as "42" there too.

The fix adds groupKeyToString. A string is used as-is. A Go number or json.Number is bucketed by its JSON encoding, the same bytes the SDK sends to /flags. Any other type returns an error from computeFlagLocally, so the flag falls back to the API instead of panicking. The mixed path uses the same helper. The assertion dates from the first local evaluation release (#6, 2022-08).

💚 How did you test it?

  • Added TestFlagNumericGroupKey. For group IDs 0–19 against the 35% rollout fixture, it checks that int, int64 and float64 keys give the same local result as the string key. It also checks that Enqueue(Capture{Groups: {"company": 42}, SendFeatureFlags: true}) doesn't panic.
  • Added a numeric-key case to TestMixedTargetingLocalEvaluation.
  • On main, the first test panics on int(0), and the mixed case returns false instead of true.
  • With the fix, both pass, and so do go test ./..., go test -race on the group/mixed tests, go vet, gofmt -l and bin/api-diff (no public API change).

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

🤖 Agent context

Autonomy: Fully autonomous

  • Found and fixed by an AI agent (Claude Code, breken-ai) while reviewing local flag evaluation against the flags service and posthog-python/posthog-node.
  • I considered making any non-string key fall back to the API. I chose to bucket numbers the way the server does, so numeric IDs keep evaluating locally and give the same answer as /flags. Bools, maps and similar keys still fall back.
  • Only automated tests were run. No manual testing against a live project.

Groups is map[string]interface{}, but computeFlagLocally asserted
groups[groupType].(string). A numeric key such as Groups{"company": 42}
panicked in GetFeatureFlag, GetAllFlags and in Enqueue of a Capture with
SendFeatureFlags, and a mixed-targeting condition skipped it silently.
The flags service accepts a string or a JSON number as a group key and
buckets the number by its decimal form, so do the same; other types fall
back to the API.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@dustinbyrne
dustinbyrne force-pushed the fix/numeric-group-key-panic branch from 8f85bab to f172107 Compare September 29, 2026 21:52
@dustinbyrne
dustinbyrne merged commit 2a26150 into PostHog:main Sep 30, 2026
28 checks passed
@dustinbyrne

Copy link
Copy Markdown
Contributor

thank you again @breken-ai!

eli-r-ph added a commit that referenced this pull request Oct 1, 2026
main's numeric group key fix (#332) added a test whose server fails on any
path it does not expect, and it accepts capture only on the legacy /batch/
path. This branch sends capture to /i/v1/analytics/events, so the test
reported an unexpected request.

The handler now answers capture first with serveCaptureOK, as the other flag
tests on this branch do, and drops the dead /batch/ branch.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants