Repository navigation
feat(feature-flags)!: Evaluate ordered targeting rules - #1717
stanleyphu wants to merge 2 commits into
Conversation
[Medium impact] The PR appears safe to merge, with a non-blocking gap in the response-type migration notes. Findings
Fix with agent prompt### Issue 1
src/feature-flags/interfaces/flag-poll-response.interface.ts:undefined-56
`FlagPollResponse` now includes a v2 envelope without a string index signature. Existing TypeScript code that accepts this exported type and indexes it by flag slug will no longer compile. The README describes the `getFlag()` and change-event migration, but not this additional type change, leaving those callers without migration guidance.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds v2 polling and ordered rules, so the first matching rule can serve either
|
| flags: Record<string, FlagPollEntryV2>; | ||
| } | ||
|
|
||
| export type FlagPollResponse = FlagPollResponseV1 | FlagPollResponseV2; |
There was a problem hiding this comment.
Response alias breaks indexing
FlagPollResponse now includes a v2 envelope without a string index signature. Existing TypeScript code that accepts this exported type and indexes it by flag slug will no longer compile. The README describes the getFlag() and change-event migration, but not this additional type change, leaving those callers without migration guidance.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/feature-flags/interfaces/flag-poll-response.interface.ts
Line: 56
Comment:
**Response alias breaks indexing**
`FlagPollResponse` now includes a v2 envelope without a string index signature. Existing TypeScript code that accepts this exported type and indexes it by flag slug will no longer compile. The README describes the `getFlag()` and change-event migration, but not this additional type change, leaving those callers without migration guidance.
**Knowledge Base Used:**
- [Feature flag evaluation](https://app.greptile.com/workos/-/custom-context/knowledge-base/workos/workos-node/-/docs/feature-flags.md)
- [SDK initialization and public entrypoints](https://app.greptile.com/workos/-/custom-context/knowledge-base/workos/workos-node/-/docs/sdk-foundation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
The type change is intentional as part of the major SDK update, but agreed that we’re missing migration guidance here. I’ll add a note to the README’s configuration migration section to use FlagPollResponseV1 for legacy maps, or FlagPollResponseV2 with .flags[slug] for v2.
There was a problem hiding this comment.
That migration note addresses the concern. Please make the distinction explicit: legacy flat maps should use FlagPollResponseV1, while v2 consumers should access entries through FlagPollResponseV2.flags[slug]; then callers can update their indexing without relying on the union’s shape.
|
How do we plan to roll this out? I imagine now that we have the v2 version of the SDK API shipped, we can just ship this without waiting for the full rollout of targeting rules.
However, this would break existing targeting, so I'm wondering when the right order to ship this SDK change would be |
BREAKING CHANGE: getFlag() and change-event snapshots now expose v2 rule entries with off_value and rules instead of legacy targets.
c0860ed to
c9ac1cd
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
⚓ Shipyard review suggestionSuggested team: applied-ai, based on recent contributors to the changed files (@nicknisi).
Links open Shipyard for confirmation. Choosing a reviewer overrides availability; “Pick for me” skips known absences. Shipyard Analysis⏳ Generating analysis... ⚓ Explore the analysis in depth in Shipyard · 🔄 Re-run analysis |
|
⚓ Review requested from @nicknisi for team Shipyard Analysis⏳ Generating analysis... ⚓ Explore the analysis in depth in Shipyard · 🔄 Re-run analysis |
Why
The runtime's enable-only evaluator cannot honor an ordered rule that serves false. I added v2 polling and first-match-wins evaluation, with legacy responses normalized before the cache swap so older APIs and bootstrap data keep working. Unknown rules/operators do not match; unparseable served values use the caller's default.
This changes the configuration returned by
getFlag()and change-event snapshots fromtargetstorulesplusoff_value. The breaking-change marker and README call out that migration.isEnabled()keeps its existing signature.Not covered by CI
payload_version=2and a future bootstrap envelope was cached as two flags. Both pass now.codex/auth-7096-poll-payloads. The relay forwarded live API payloads; credentials stayed inside the testing app.Proof
The live poll requested
/sdk/feature-flags?payload_version=2and loaded 16 flags. For the disposablepoll-payload-smokeflag, whose user exclusion precedes its organization inclusion:The last result confirms compatibility with v1's enable-only projection. The v2 SDK preserves the exclusion.
Fixes AUTH-7097