fix: drop group_identify() calls with a missing group type or key (sdk-specs group-identify) - #835
Draft
posthog[bot] wants to merge 1 commit into
Draft
Conversation
…k-specs group-identify)
The sdk-specs `group-identify` contract has an explicit `@both` scenario -
"Group identify requires type and key" - requiring that a call without a group
key enqueue no `$groupidentify` event and record a validation warning.
Behavior step 1 says `groupType` and `groupKey` "must be present and non-empty".
`Client.group_identify()` performed no validation, so `group_identify("company",
None)` (or an empty string for either argument) enqueued a `$groupidentify`
event carrying a null/empty `$group_type` or `$group_key`, which cannot address
a group profile.
Both arguments are now validated up front and the call is dropped with a
warning, mirroring the `alias()` validation merged in #831. Valid values -
including non-string group keys - are passed through to the wire unchanged.
Generated-By: PostHog Code
Task-Id: 24b9fcad-a8a5-4f09-83e8-cd0340ba0440
Contributor
posthog-python Compliance ReportDate: 2026-08-06 07:55:24 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
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
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.
💡 Motivation and Context
Compliance gap against the cross-SDK contract in PostHog/sdk-specs.
openspec/specs/group-identify/spec.md— Requirement "Canonical group-identify behavior", Scenario "Group identify requires type and key (@both)":Mirrored in
acceptance/public/group-identify.feature(@both), and restated as Behavior step 1 of the same spec: "Validate group identity.groupTypeandgroupKeymust be present and non-empty."What was out of compliance.
Client.group_identify()went straight fromproperties = properties or {}to building the$groupidentifymessage — neither argument was checked. So all of these enqueued an event:Each returns a UUID and logs nothing. A
$groupidentifywith a null or empty group type/key cannot address a group profile, so the event is unusable on arrival — it just consumes ingestion quota and shows up as a phantom group in the project.How this fixes it. Both arguments are validated up front; a missing or empty value is dropped with a
log.warningandNoneis returned. This is the same shape as thealias()validation merged in #831 —stringify_id(...)for the emptiness test so a legitimately falsy non-string key such as0is not mistaken for "missing", and one warning naming the specific argument that was absent.Behavior change / compatibility risk: low, but not zero. Calls that previously enqueued an unusable event now return
Noneand enqueue nothing. Anyone whose code path silently passed an empty group key will see those events stop — which is the point, but it is an observable drop in event volume for that path, and a caller asserting on a non-Nonereturn would start failing. No public signature, config, or property-name changes.Deliberately not included: unlike
alias(), this does not normalize a non-stringgroup_keyto a string on the wire.$group_keyis part of the group's identity server-side, so silently turning5into"5"could split an existing group's profile — a much larger blast radius than the compliance gap being fixed here. The value is validated and passed through as-is; a test pins that.💚 How did you test it?
Added five cases in
posthog/test/test_client.py, following the conventions of the existingaliasvalidation tests:test_group_identify_without_group_type_is_dropped, parameterized overNoneand""— asserts no HTTP post,Nonereturn, and that the warning namesgroup_type.test_group_identify_without_group_key_is_dropped, same two parameters, warning namesgroup_key.test_group_identify_accepts_falsy_non_string_group_key—group_key=0is still enqueued and reaches the wire as0, not"0".Ran
posthog/test/test_client.py,test_module.py, andtest_contexts.py(200 passed), plusruff format --check,ruff check,mypy posthog/client.py, and.github/scripts/check_public_api.py(snapshot unchanged). No manual or integration testing against a live PostHog instance.📝 Checklist
.sampo/changesets/group-identify-validates-group-identity.md.If releasing new changes
sampo addto generate a changeset file — thesampoCLI isn't available in this environment, so the file was hand-written to match the format of the one merged in fix: drop alias() calls with a missing identity (sdk-specs alias) #831. Please sanity-check it.🤖 Agent context
Autonomy: Fully autonomous
Opened by the scheduled SDK Spec Compliance Enforcer loop for posthog-python, running in PostHog Code (Claude Code harness). Each run reads
PostHog/sdk-specsas the source of truth, audits the Python SDK against the contracts whoseApplicabilityisbothorserver, and opens one focused draft PR per confirmed divergence.This run swept the ~20 in-scope specs across five parallel read-only audit agents (capture/identify/alias/group-identify/before-send; the six flag getters; batcher/retry/http/flush/shutdown; local evaluation + definition loader + flag-called tracker; tracing-headers/bootstrap/logs/traces).
logsandtracesare simply not implemented in this SDK — a capability not yet ported rather than a violation — andbootstrapis a client-only concept.This finding was chosen over the alternatives because the spec states it as an explicit
@bothacceptance scenario rather than prose, the fix has direct precedent in #831 that the team already accepted, and it carries the least backward-compatibility risk of the confirmed candidates. Runners-up left for human judgement, in rough priority order:request.py:265treats only HTTP200as success, so a202/204from an ingestion proxy becomes anAPIErrorand (being classified retryable) causes duplicate batch delivery. The SDK's own v1 path already does200 <= status < 300. One-line fix, but it changes error handling on the shared/batch/+/flags/path.capture_v1.py:87lists429as terminal, so a rate-limited batch is dropped on first response whencapture_mode="v1"; theretry-queuespec names429as retryable and the default v0 lane already retries it. Fixing it makesshutdown()'s unbounded flush block through the backoff schedule under sustained rate limiting.json.loadson flag payloads intypes.py:234/:275is unguarded, so a non-JSON payload string raisesJSONDecodeErrorinto caller code fromget_feature_flag_payload/get_feature_flag_result. The tolerant_parse_flag_payloadhelper already exists but is only wired into theevaluate_flags()path.get_all_flags_and_payloads()returns payloads as raw JSON strings while every other payload surface returns parsed values. Spec-supported, but it is a public return-type change.Falselocally instead of signalling inconclusive, so the/flagsfallback the spec asks for never happens. The current behavior is a deliberate, commented cost optimization — this one probably wants resolving in the spec, not the SDK.Agent-authored, so no human co-author is claimed, and it needs human review before merge.
Created with PostHog Code