Repository navigation
fix(flags): honor cohort not_in - #431
Conversation
Commit signatures resolvedGitHub now verifies every commit signature in this PR. No further action is needed. |
f3cdf81 to
328dc79
Compare
Local evaluation treated a cohort not_in filter as membership, and a release condition with negation set still required the property to match.
328dc79 to
d7a19d6
Compare
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review. Reproduced blocking findings from the requested review.
An invalid regex is a non-match for both regex and not_regex. Condition negation was complementing that failure and enabling the flag for every value.
|
@marandaneto invalid regex stays a non-match under negation, so a bad pattern no longer turns the condition on. Can you review again? |
marandaneto
left a comment
There was a problem hiding this comment.
Follow-up on the cross-SDK comparison: release-condition negation changes legacy backend semantics.
|
also missing changeset |
Cohort not_in still excludes members. A negation field on a legacy release condition is left unused, matching the backend.
|
@marandaneto the changeset is in |
|
Unrelated to this PR, so feel free to ignore it here. I applied for the Technical Support Engineer role today. The careers form returned an application-limit error, so I sent the application to careers@posthog.com instead. Working through the mobile SDKs over the last two months, I kept running into the same kinds of gaps: surveys behaving differently on Flutter, iOS and Android, opt-out not fully stopping replay or integrations, and beforeSend not covering every capture path on Flutter web. I have ideas for dealing with the patterns behind those and not only the individual bugs, and I'd like the chance to work on them properly. Would you be open to a short chat about it, or to pointing me to the right person on the talent team? Happy to do it async, wherever suits you. |
ioannisj
left a comment
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Nothing here blocks the merge. The title and description still mention release-condition negation, and not_in has no test for a cohort that can't be resolved locally, both nits.
Manoel is looking into possible changes
Why the release-condition negation change was removedWe removed that part to preserve compatibility with the legacy backend semantics, independently of the unsupported-cohort-group discussion. In the legacy This is separate from negation inside dynamic-cohort leaves, which remains supported and was not removed. The cohort The release-condition behavior was removed in |
we've received your application and we'll be in contact if its a good fit. |
💡 Motivation and Context
operator: "not_in"as membership, so a person in the cohort still matched the release condition. Apply the inverse only after membership resolves successfully.inand an omitted cohort operator remain membership checks. Legacy release conditions continue to ignore thenegationfield; negative operators express negative conditions.No public API changes. Unsupported-group composition is outside this PR's scope.
💚 How did you test it?
go test -v -count=1 -timeout=90s -run '^TestCohortNotInAndConditionNegation$' .go build ./...go vet ./...go test -race -count=1 -timeout=5m ./...git diff --checkand Go formatting checks.Only root-module tests changed in the review follow-up; nested-module checks were not rerun locally.
📝 Checklist
If releasing new changes
.changeset/flag-cohort-not-in.md.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
The original implementation is by @o-mid. Pi, directed by @marandaneto and using Go, Git, and GitHub CLI, added the two unresolved-
not_inregression cases and corrected the PR metadata. This review follow-up makes no production behavior changes. Human review is required.