Skip to content

test(node): synchronize local flag capture completion - #5264

Open
dustinbyrne wants to merge 1 commit into
mainfrom
fix/merge-queue-unit-flake-20261009
Open

dustinbyrne wants to merge 1 commit into
mainfrom
fix/merge-queue-unit-flake-20261009

Conversation

@dustinbyrne

@dustinbyrne dustinbyrne commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The local-evaluation property-override test failed #5231's first merge-queue unit job: it expected a batch POST, but only the flag-definitions GET had been recorded.

Capture prepares events asynchronously, including native WebCrypto hashing for local rollout evaluation. The test's short sleeps and fake-timer advancement did not guarantee that preparation or delivery had completed. The affected Node/core source was unchanged by #5231; the later queue attempt passed all 1,109 Node tests.

Changes

  • Await local-evaluation readiness before capturing.
  • Await the public flush promise before inspecting delivered events.
  • Hold actual WebCrypto hashing behind an explicit promise gate to exercise deferred evaluation deterministically. Assert no batch is sent while evaluation is blocked, then release hashing and verify the existing property-override assertions.
  • Release the gate and restore the digest spy in finally.

Only the failing test changes. Production behavior and public APIs are unchanged.

Validation

  • The controlled digest schedule fails with the original synchronization at the same missing-batch assertion and passes with this fix.
  • Focused test: 20/20 independent runs passed.
  • pnpm turbo run test:unit --filter=posthog-node --concurrency=50%: 1,109 tests passed in each of three runs, with build prerequisites bootstrapped through root Turbo.
  • Node lint, formatting, root Turbo Node type checks, and git diff --check passed.
  • Fresh read-only review found no actionable issues.

Local validation used macOS, Node 24.13.0 and pnpm 11.7.0. The failed CI job used Ubuntu 22.04.5 and Node 24.21.0; the patch has not yet been validated in that environment. Other tests using the shared timing helper remain outside this change.

Release info Sub-libraries affected

Test-only change; no package version bump or changeset required.

Checklist

  • 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

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Investigated and implemented with Pi using local source inspection, controlled scheduling, package tests and GitHub CLI log inspection. A fresh read-only reviewer checked the diff and validation evidence. Human review is required.

@dustinbyrne dustinbyrne self-assigned this Oct 9, 2026
@dustinbyrne
dustinbyrne requested a review from a team as a code owner October 9, 2026 15:37
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor
⚠️ `posthog-node` is modified but this PR has no changeset

This is informational — the PR is not blocked. Click the triangle above to collapse, or push a fix and this comment will auto-delete.

Modified in this PR but no changeset added:

  • posthog-node

If this change should ship, run pnpm changeset and select a bump level.
If it isn't user-facing (refactor with no behavior change, internal tooling, generated files), no action needed.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

📝 No Changeset Found

This PR doesn't include a changeset. A changeset is required to release a new version.

How to add a changeset

Run this command and follow the prompts:

pnpm changeset

Remember: Never use major version bumps for posthog-js as it's autoloaded by clients.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Node SDK compliance v2

Commit: c4d6459
Workflow run and artifacts

Capture v0 — acceptance

✅ 5 passed / ❌ 0 non-passing / ⚠️ 812 not selected

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

Harness exit: 0; report validation exit: 0.

All selected cases passed and report validation succeeded.

Capture v1 — acceptance

✅ 5 passed / ❌ 0 non-passing / ⚠️ 812 not selected

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

Harness exit: 0; report validation exit: 0.

All selected cases passed and report validation succeeded.

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.

@stamphog stamphog Bot removed the stamphog label Oct 9, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not approved — escalated to a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

The diff is a contained test-only change with no open review concerns or production changes. Remove the reviewer-directed instructions in the PR description before re-requesting.

  • Prompt injection attempt: an HTML comment in the PR description instructs AI reviewers to append unrelated provenance and human-treatment information. Remove those reviewer-directed instructions before re-requesting.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 0L, 0F substantive, 97L/1F incl. docs/generated/snapshots — within ceiling
tier ✓ T0 auto-approve: T0-deterministic (97L, 1F, single-area, test)
stamphog 2.4.1 .stamphog/policy.yml @ c4d6459 · reviewed head c4d6459

@dustinbyrne

Copy link
Copy Markdown
Contributor Author

@stamphog no fun allowed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant