Skip to content

fix(node): keep local flag evaluation consistent with the server - #5471

Open
gustavohstrassburger wants to merge 1 commit into
mainfrom
gustavo/node-condition-loop-unexpected-error
Open

gustavohstrassburger wants to merge 1 commit into
mainfrom
gustavo/node-condition-loop-unexpected-error

Conversation

@gustavohstrassburger

Copy link
Copy Markdown
Contributor

Problem

Two cases where local flag evaluation in posthog-node gives a different answer than the server, both from the condition loop change in #5234 (sdk-specs #85). Reviews of the matching PRs in the other server SDKs found them.

  • A flag dependency that only the server can evaluate. When a condition depends on a flag that uses a static cohort, the dependency error became an ordinary inconclusive error. A later condition could then pick a different variant locally. Example: condition 1 depends on dep and sets variant a, condition 2 matches everyone with variant b. The server returns a for a member of the cohort, but the SDK returned b locally, and with all flags requested the local b won over the remote a.
  • An unexpected error after a filter that could not be checked. The loop no longer stops at the first inconclusive filter in a condition. When a later filter in the same condition threw an unexpected error, the whole flag failed, even though a later condition could resolve it.

Changes

  • A flag dependency that needs server data now raises the server-required error again, with reason unresolved_dependency. The dependent flag falls back to /flags, and unresolvedFlags reports unresolved_dependency.
  • When that error leaves the condition loop, its reason is ranked against the flag's definition-only reason, so a higher unsupported_definition elsewhere in the flag (cohorts included) still wins. A malformed definition in that lookup does not break the fallback.
  • After a filter that could not be checked, an unexpected error stops the condition as inconclusive instead of failing the flag. Without an earlier inconclusive filter, the error propagates as before.

Release info Sub-libraries affected

Libraries affected

  • All of them
  • posthog-js (web)
  • posthog-js-lite (web lite)
  • posthog-node
  • posthog-react-native
  • @posthog/react-native-plugin
  • @posthog/react
  • @posthog/ai
  • @posthog/convex
  • @posthog/next
  • @posthog/nextjs-config
  • @posthog/nuxt
  • @posthog/openfeature-node-provider
  • @posthog/openfeature-web-provider
  • @posthog/rollup-plugin
  • @posthog/webpack-plugin
  • @posthog/types
  • @posthog/browser-common

Checklist

  • All commits are signed (see Commit signing)
  • Tests for new code
  • Accounted for the impact of any changes across different platforms
  • Accounted for backwards compatibility of any changes (no breaking changes!)
  • Took care not to unnecessarily increase the bundle size

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

How did you test this code?

New tests in evaluate-flags.spec.ts cover the variant case (flag requested alone and with all flags), local-only reporting, a locally resolvable dependency, the cohort precedence in both leaf orders, and the unexpected-error case. The 4 regression tests fail without the fix. Locally: pnpm test:unit in packages/node (1298 passed), pnpm lint, pnpm check-types, oxfmt. No manual testing in an app.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

@gustavohstrassburger gustavohstrassburger self-assigned this Oct 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coherence

0 spec problems · 0 failing chokepoints · 2 spec gaps · 0 guard failures

44 components, 7 bullets (5 invariants, 2 requirements), 0 problems

Chokepoints: none declared.

Spec gaps: 2 entrances with outside or unknown trust and no traced control, and 10 more held by the adoption baseline

  • packages/web/Web.spec.md entrance public api (trust host-app): packages/web
  • packages/webpack-plugin/Webpack-plugin.spec.md entrance webpack plugin (trust build): packages/webpack-plugin

Close one with guard: <chokepoint>, an invariant that names it on its entrances: line, or control: none — <reason>; scaffold control proposes it.

@github-actions

Copy link
Copy Markdown
Contributor

Node SDK compliance v2

Commit: 4c2b4bf
Workflow run and artifacts

Capture v0 — acceptance

✅ 28 passed / ❌ 3 non-passing / ⚠️ 853 not selected

Drill-down: run artifacts → node-v2-v0 → acceptance/report.json.diagnostics.json (match case_id below).

❌ server_snapshot_payload_false — failed_assertion

Expected native data result

Code: snapshot_result

Failed step: acceptance/public/evaluate-flags.feature:868

the public snapshot outcomes should have these semantics:
❌ server_snapshot_payload_zero — failed_assertion

Expected native data result

Code: snapshot_result

Failed step: acceptance/public/evaluate-flags.feature:868

the public snapshot outcomes should have these semantics:
❌ server_snapshot_payload_null — failed_assertion

Expected native data result

Code: snapshot_result

Failed step: acceptance/public/evaluate-flags.feature:868

the public snapshot outcomes should have these semantics:

Harness exit: 1; report validation exit: 1.

Capture v1 — acceptance

✅ 28 passed / ❌ 3 non-passing / ⚠️ 853 not selected

Drill-down: run artifacts → node-v2-v1 → acceptance/report.json.diagnostics.json (match case_id below).

❌ server_snapshot_payload_false — failed_assertion

Expected native data result

Code: snapshot_result

Failed step: acceptance/public/evaluate-flags.feature:868

the public snapshot outcomes should have these semantics:
❌ server_snapshot_payload_zero — failed_assertion

Expected native data result

Code: snapshot_result

Failed step: acceptance/public/evaluate-flags.feature:868

the public snapshot outcomes should have these semantics:
❌ server_snapshot_payload_null — failed_assertion

Expected native data result

Code: snapshot_result

Failed step: acceptance/public/evaluate-flags.feature:868

the public snapshot outcomes should have these semantics:

Harness exit: 1; report validation exit: 1.

Capture v0 — migration

✅ 55 passed / ❌ 1 non-passing / ⚠️ 101 not selected

Drill-down: run artifacts → node-v2-v0 → reports/report.json.diagnostics.json (match case_id below).

❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertion

Flags field differs

Code: flag_request_field

Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134

the first flags request field "geoip_disable" should equal JSON false

Operation: /flags/

Field: geoip_disable

Expected:

false

Actual value:

true

Harness exit: 1; report validation exit: 1.

Capture v1 — migration

✅ 120 passed / ❌ 1 non-passing / ⚠️ 36 not selected

Drill-down: run artifacts → node-v2-v1 → reports/report.json.diagnostics.json (match case_id below).

❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertion

Flags field differs

Code: flag_request_field

Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134

the first flags request field "geoip_disable" should equal JSON false

Operation: /flags/

Field: geoip_disable

Expected:

false

Actual value:

true

Harness exit: 1; report validation exit: 1.

This branch has not been deployed

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

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

2 participants