Repository navigation
feat(flags): skip events writes for flag evaluations only orgs - #110254
Conversation
|
😎 Merged successfully - details. |
🤖 CI report
|
| Function | Location | Complexity | Limit |
|---|---|---|---|
StaffFlagEvaluationsModeModal |
products/feature_flags/frontend/staff/StaffFlagEvaluationsModeModal.tsx:26 |
12 | 10 |
✅ Duplication (Python) — clean
New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
🚨 Comment density — 10% of added code lines are comments (55 of 547)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
nodejs/src/ingestion/common/steps/event-processing/fork-flag-evaluations-step.ts |
23 | 56 |
nodejs/src/ingestion/common/steps/event-processing/fork-flag-evaluations-step.test.ts |
8 | 145 |
nodejs/src/ingestion/common/flag-evaluations/flag-evaluations-service.ts |
6 | 21 |
products/feature_flags/frontend/staff/featureFlagsStaffToolsLogic.test.ts |
4 | 20 |
nodejs/tests/ingestion/flag-evaluations-e2e.test.ts |
3 | 41 |
products/feature_flags/backend/facade/enums.py |
3 | 7 |
nodejs/src/ingestion/config.ts |
2 | 4 |
posthog/settings/feature_flags.py |
2 | 2 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Bundle size — 🔺 +1.1 KiB (+0.0%)
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 70.71 MiB · 🔺 +1.1 KiB (+0.0%)
| File | Size | Δ vs base |
|---|---|---|
posthog-app/_parent/products/feature_flags/frontend/staff/FeatureFlagsStaffToolsScene.js |
60.3 KiB | 🔺 +1.1 KiB (+1.8%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.62 MiB · 22 files | no change | █████████░ 87.9% of 1.84 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
3.58 MiB · 630 files | no change | █████████░ 88.8% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.76 MiB · 2,486 files | no change | █████████░ 93.0% of 8.34 MiB |
dashboard scenesrc/scenes/dashboard/Dashboard.tsx |
11.67 MiB · 4,475 files | no change | █████████░ 86.6% of 13.48 MiB |
project home scenesrc/scenes/project-homepage/ProjectHomepage.tsx |
14.44 MiB · 5,365 files | no change | █████████░ 87.9% of 16.44 MiB |
events scenesrc/scenes/activity/explore/EventsScene.tsx |
10.90 MiB · 4,134 files | no change | █████████░ 86.3% of 12.64 MiB |
replay detail scenesrc/scenes/session-recordings/detail/SessionRecordingDetail.tsx |
13.79 MiB · 5,056 files | no change | █████████░ 87.7% of 15.72 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 854 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 219.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 92.0 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 28.7 KiB | src/scenes/scenes.ts |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 219.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 109.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.0 KiB | src/products.tsx |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Largest files eagerly shipped from src/scenes/dashboard/Dashboard.tsx
| Size | File |
|---|---|
| 315.5 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 219.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 109.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Largest files eagerly shipped from src/scenes/project-homepage/ProjectHomepage.tsx
| Size | File |
|---|---|
| 315.5 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 219.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 109.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Largest files eagerly shipped from src/scenes/activity/explore/EventsScene.tsx
| Size | File |
|---|---|
| 315.5 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 219.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 109.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Largest files eagerly shipped from src/scenes/session-recordings/detail/SessionRecordingDetail.tsx
| Size | File |
|---|---|
| 315.5 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 219.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 109.9 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.19 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.19 MiB · 19 files | no change | ████░░░░░░ 38.3% of 5.72 MiB |
| Deferred (lazy) | 2.10 MiB · 44 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 830.5 KiB | dist/toolbar/toolbar-app-KOLJZZVO.css |
| 657.2 KiB | dist/toolbar/chunk-chunk-Z6UICKYF.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-VJB3SXU4.js |
| 138.2 KiB | dist/toolbar/chunk-chunk-YRDJ3DU2.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-KU7GIXWC.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-O24ZCDY5.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-AC27PX7U.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +8.6 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 966.10 MiB · 🔺 +8.6 KiB (+0.0%)
ℹ️ MCP UI apps size — 33 app(s), 17633.1 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 597.9 KB | 199.3 KB |
| action | 454.1 KB | 199.3 KB |
| action-list | 564.2 KB | 199.3 KB |
| cohort | 453.1 KB | 199.3 KB |
| cohort-list | 563.2 KB | 199.3 KB |
| email-template | 452.9 KB | 199.3 KB |
| error-details | 469.6 KB | 199.3 KB |
| error-issue | 454.5 KB | 199.3 KB |
| error-issue-list | 564.8 KB | 199.3 KB |
| experiment | 561.3 KB | 199.3 KB |
| experiment-list | 564.9 KB | 199.3 KB |
| experiment-results | 566.3 KB | 199.3 KB |
| feature-flag | 566.8 KB | 199.3 KB |
| feature-flag-list | 570.5 KB | 199.3 KB |
| feature-flag-testing | 457.3 KB | 199.3 KB |
| inline-scan | 453.6 KB | 199.3 KB |
| insight-actors | 562.3 KB | 199.3 KB |
| invite-email-preview | 452.3 KB | 199.3 KB |
| llm-costs | 559.3 KB | 199.3 KB |
| session-recording | 455.3 KB | 199.3 KB |
| survey | 454.7 KB | 199.3 KB |
| survey-global-stats | 561.9 KB | 199.3 KB |
| survey-list | 564.9 KB | 199.3 KB |
| survey-stats | 561.9 KB | 199.3 KB |
| trace-span | 453.5 KB | 199.3 KB |
| trace-span-list | 564.1 KB | 199.3 KB |
| vision-observation-list | 563.3 KB | 199.3 KB |
| workflow | 453.4 KB | 199.3 KB |
| workflow-list | 563.5 KB | 199.3 KB |
| loops-review | 457.8 KB | 199.3 KB |
| query-results | 774.1 KB | 199.3 KB |
| render-ui | 858.1 KB | 199.3 KB |
| visual-review-snapshots | 457.9 KB | 199.3 KB |
|
[High risk] Changes event ingestion pipeline to skip writing events for flag-evaluation-only organizations. The PR appears safe to merge, but the staff warning should accurately describe when events writes stop. Reviews (1) · Last reviewed commit: "feat(flags): skip events writes for flag..." |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Experiment-reader compatibility and cache-consistent downgrades need resolution before approval.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds ingestion support for organizations storing flag calls only in flag_evaluations, alongside the planned reader migrations.
Changes:
- Suppresses events-table copies only after queuing flag-evaluation rows.
- Adds a rollback switch, outcome metrics, and oversized-row warnings.
- Extends tests and updates mode documentation.
| File | Description |
|---|---|
products/feature_flags/frontend/staff/StaffFlagEvaluationsModeModal.tsx |
Updates staff warning text. |
products/feature_flags/backend/flag_evaluations_mode.py |
Updates downgrade documentation only. |
products/feature_flags/backend/facade/enums.py |
Documents ingestion behavior and rollback. |
posthog/settings/feature_flags.py |
Updates the default-mode comment only. |
nodejs/tests/ingestion/flag-evaluations-e2e.test.ts |
Tests routing, person updates, and exposure copies. |
nodejs/tests/helpers/sql.ts |
Stores test modes in organization config. |
nodejs/src/ingestion/pipelines/analytics/event-subpipeline.ts |
Updates the fork failure comment only. |
nodejs/src/ingestion/config.ts |
Adds the ingestion rollback switch. |
nodejs/src/ingestion/common/steps/event-processing/fork-flag-evaluations-step.ts |
Filters queued calls and reports outcomes. |
nodejs/src/ingestion/common/steps/event-processing/fork-flag-evaluations-step.test.ts |
Tests suppression, fallback, and warnings. |
nodejs/src/ingestion/common/steps/event-processing/create-event-step.ts |
Passes team context downstream. |
nodejs/src/ingestion/common/steps/event-processing/create-event-step.test.ts |
Adds team context to fixtures. |
nodejs/src/ingestion/common/flag-evaluations/flag-evaluations-service.ts |
Gates suppression by mode and rollback. |
nodejs/src/ingestion/common/flag-evaluations/flag-evaluations-service.test.ts |
Tests rollback configuration wiring. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const eventsToEmit = input.eventsToEmit.filter((toEmit) => !mappableEvents.includes(toEmit)) | ||
| return Promise.resolve(ok({ ...input, eventsToEmit }, [settled])) |
There was a problem hiding this comment.
Escalated. The gap is real, and closing it needs a design decision across the feature flags and experiments products.
- For a mode-2 team, experiments that resolve to $feature_flag_called stop counting new exposures, and so do the session readers.
- The PR handles this only as a rollout prerequisite, and that check runs only once. It does not cover an org created on mode 2 that starts an experiment later while experiment-exposure-event is off for it.
- Options: (a) keep the process gate and make sure experiment-exposure-event is on for every mode-2 org, (b) block the mode change in code, (c) hold the merge until experiments read flag_evaluations.
- Options (b) and (c) are cross-product changes that are out of scope for an unattended fix, and option (a) is a rollout policy choice. A maintainer should pick one.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughIngestion now stops Priority: ➖ Normal Merge Risk: 🔵 Low · up to The ingestion logic looks sound, but the staff-facing and API descriptions of mode 2 may still overstate when flag-call events stop being written to the events table. They also omit the conditions under which writes continue. Fix the wording before merge or accept it as a follow-up. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Eligible flag calls now depend on one destination for persistence. A permanent delivery failure can lose those calls, and restoring event writes does not recover previously missing data. Tenant routing and experiment-exposure copies are preserved, but reader migration and deployment ordering still require confirmation. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
services/mcp/src/api/generated.ts-100005-100005 (1)
100005-100005: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBoth descriptions overstate mode 2’s event-write behavior. A
$feature_flag_calledcall is removed fromeventsonly after itsflag_evaluationsrow is queued. Calls without a queued row still go toevents.
services/mcp/src/api/generated.ts#L100005-L100005: qualify the target-mode description with the row-queued condition.services/mcp/src/api/generated.ts#L100085-L100085: qualify the stored-mode description with the same condition.products/feature_flags/backend/api/staff_team_config.py-114-115 (1)
114-115: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the rollback exception in each unconditional mode-2 description.
INGESTION_FLAG_EVALUATIONS_ONLY_DISABLED=trueis passed toFlagEvaluationsService. For stored mode 2,stopsEventsWritesForthen returns false, so ingestion can keep writing$feature_flag_calledevents without changing the stored mode.Update the API help text, both modal warnings, parity guidance,
posthog/settings/feature_flags.py,products/feature_flags/backend/flag_evaluations_mode.py, and the affected generated MCP descriptions. State that mode-2 suppression and the events-table gap apply only when the rollback flag is false. Keep the existing management-command documentation andproducts/feature_flags/backend/facade/enums.pycomment, which already describe the fallback. The existing Usage tab override is separate and does not document ingestion behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 73b76ec2-91aa-4844-b419-6b3463df32c3
⛔ Files ignored due to path filters (2)
products/feature_flags/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/feature_flags/frontend/generated/api.zod.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
nodejs/src/ingestion/common/steps/event-processing/fork-flag-evaluations-step.test.tsnodejs/src/ingestion/config.tsposthog/models/flag_evaluations/PARITY_CHECK.mdproducts/feature_flags/backend/api/staff_team_config.pyproducts/feature_flags/frontend/staff/StaffFlagEvaluationsModeModal.tsxproducts/feature_flags/frontend/staff/featureFlagsStaffToolsLogic.test.tsproducts/feature_flags/frontend/staff/featureFlagsStaffToolsLogic.tsservices/mcp/src/api/generated.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/feature_flags/frontend/staff/StaffFlagEvaluationsModeModal.tsx-23-23 (1)
23-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQualify the stopped-write claims to calls with valid flag keys.
nodejs/src/ingestion/common/steps/event-processing/fork-flag-evaluations-step.tsfilters onlymappableEvents. Calls without a valid flag key remain ineventsToEmitand can still be written toevents. State this exception in both descriptions.Also applies to: 119-120
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 93aa0765-dc76-4c87-857c-fa50c8ff8388
📒 Files selected for processing (1)
products/feature_flags/frontend/staff/StaffFlagEvaluationsModeModal.tsx
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
products/feature_flags/frontend/staff/StaffFlagEvaluationsModeModal.tsx-121-121 (1)
121-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit the “only to
flag_evaluations” claim to queued rows.The routing change keeps a
$feature_flag_calledineventswhen noflag_evaluationsrow is queued. The warning can therefore mislead staff about event-table coverage for allowlisted teams. Clarify that ingestion omits only calls whose rows were successfully queued.services/mcp/src/api/generated.ts-100005-100005 (1)
100005-100005: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winQualify the mode-2 event-write behavior in both descriptions.
These descriptions imply that mode 2 always stops
$feature_flag_calledwrites toevents. Suppression applies only when the ingestion allowlist admits the team and the rollback setting is off. In addition, ingestion removes a call only after it queues the correspondingflag_evaluationsrow; paths that queue no row still write the call toevents. State these conditions so API consumers do not treat mode 2 as a guarantee of complete event suppression.Also applies to: 100085-100085
products/feature_flags/backend/api/staff_team_config.py-115-116 (1)
115-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBoth descriptions overstate when mode 2 suppresses event writes. Ingestion removes a call from
eventsonly after it queues the correspondingflag_evaluationsrow. Calls on paths that queue no row can still be written toevents. State this condition in both descriptions. The PR objective confirms this routing behavior.
products/feature_flags/backend/api/staff_team_config.py#L115-L116: Clarify that the event write stops only after the correspondingflag_evaluationsrow is queued.products/feature_flags/backend/api/staff_team_config.py#L175-L176: Apply the same clarification to the mutation help text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 0e3b1e6d-7290-4f9f-955e-f668edea587a
⛔ Files ignored due to path filters (2)
products/feature_flags/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/feature_flags/frontend/generated/api.zod.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
nodejs/src/ingestion/common/steps/event-processing/fork-flag-evaluations-step.test.tsproducts/feature_flags/backend/api/staff_team_config.pyproducts/feature_flags/frontend/staff/StaffFlagEvaluationsModeModal.tsxservices/mcp/src/api/generated.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.
|
Risk: Low · 1 low The changes since the last review are documentation text only (enums.py, generated api.schemas.ts, staff modal copy), plus one behavior flip in nodejs/src/ingestion/config.ts that defaults PERSON_MERGE_TOMBSTONE_TEAM_ALLOWLIST from '' to '*'. The tombstone path is pre-existing code that scrubs person properties on delete, so the default flip introduces no new security exposure; no new findings. Sentinel reviewed |
There was a problem hiding this comment.
This review was skipped because it would exceed your organization's monthly flex usage limit. Raise the limit in billing settings or wait until the next billing period resets limits.
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
| ) | ||
| count = 0 | ||
| for experiment in experiments: | ||
| exposure_event, _ = get_exposure_event_and_property( |
There was a problem hiding this comment.
A crafted experiment exposure_criteria value crashes the staff flag-evaluations-mode tool with a 500
The new count loop runs strict pydantic parsing on each stored exposure_criteria with no exception handling, so one experiment row whose criteria hold a value the write path does not validate makes the whole staff mode-change preview/apply request fail.
How:
- An org member creates or edits an experiment with
exposure_criteria = {"multiple_variant_handling": "nonsense", "filterTestAccounts": true}. - The write validator (
products/experiments/backend/experiment_service.py:669-722) only checks unknown keys,filterTestAccounts, and theexposure_config/activation_configshapes — it never value-checks other known fields, so the row is stored. count_running_experiments_on_feature_flag_calledcallsget_exposure_event_and_property, whosenormalize_to_exposure_criteria(exposure_query_logic.py:155) runsExperimentExposureCriteria.model_validateon it, which raises aValidationErroron the invalid enum value.- The exception escapes
set_organization_flag_evaluations_mode(flag_evaluations_mode.py:115), so the staff endpoint returns 500 for that organization, and a bulkset_flag_evaluations_moderun aborts.
Fix: Wrap the per-experiment resolution in a try/except and treat an unparseable row conservatively (count it and log), so one bad row cannot block the mode change.
React with 👍 if useful or 👎 if not
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
stamphog did not approve this pull request because the prerequisites gate failed: the branch has merge conflicts. The deny-list, size and tier gates all passed, so the conflicts are the only thing blocking it here. To move forward, the author should resolve the merge conflicts and push the update. Because the change touches ingestion, staff tooling and experiments across 28 files, it should then go to a human reviewer.
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✗ | merge conflicts present |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 272L, 15F substantive, 698L/28F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (698L, 28F, cross-cutting, feat) |
| stamphog 2.3.1 | .stamphog/policy.yml @ unknown · reviewed head e00622c |
For organizations on FLAG_EVALUATIONS_ONLY, the fork step removes each $feature_flag_called event it queued for flag_evaluations from eventsToEmit. The $experiment_exposure copy and any call the fork did not queue still reach events. INGESTION_FLAG_EVALUATIONS_ONLY_DISABLED keeps the events writes for every team on that mode. Claude-Session: https://claude.ai/code/session_012LLLo3Hxxc1Vhne69sC9Hi
The staff mode modal said ingestion stops the events writes for every team in a FLAG_EVALUATIONS_ONLY organization. Teams outside the ingestion allowlist keep those writes, and so does every team while INGESTION_FLAG_EVALUATIONS_ONLY_DISABLED is on. The mode description and both warnings now say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4950f313-3930-4d55-a98c-4b8204bf8d1e
The staff mode preview and the set_flag_evaluations_mode command count each organization's running experiments that started before the experiment exposure cutoff. Those experiments count exposures on $feature_flag_called in events, so moving the organization to FLAG_EVALUATIONS_ONLY stops them. Claude-Session: https://claude.ai/code/session_012LLLo3Hxxc1Vhne69sC9Hi
The mode 2 warning counts running experiments whose exposure event resolves to $feature_flag_called, using the exposure query's own rule, and shows the count per organization in the staff modal and the command output. Claude-Session: https://claude.ai/code/session_012LLLo3Hxxc1Vhne69sC9Hi
e00622c to
e231b72
Compare
There was a problem hiding this comment.
Approved.
This touches the ingestion path, but the author has strong familiarity and is on the feature-flags team. Nothing changes until an org is moved to the mode, and an env kill switch restores the events writes. The experiment-exposure concern was handled with staff preview and command warnings plus a stated rollout prerequisite, and the remaining open thread is a low-severity crash in a staff-only tool.
- Author wrote 79% of the modified lines and has 53 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
- Unresolved Copilot thread: experiments started before the exposure cutoff still read $feature_flag_called from events. The PR only warns staff and gates the rollout; it does not block the mode change in code.
- Unresolved Parameter note: count_running_experiments_on_feature_flag_called has no exception handling, so one experiment row with unparseable exposure_criteria would 500 the staff mode tool. Only staff tooling calls it. Consider wrapping it in try/except and counting the row conservatively.
- For FLAG_EVALUATIONS_ONLY teams, a flag_evaluations produce that fails after the step returns loses the call. The PR documents this and counts it in produce_failed, with an alert behind it.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 269L, 15F substantive, 695L/28F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (695L, 28F, cross-cutting, feat) |
| stamphog 2.3.1 | .stamphog/policy.yml @ e231b72 · reviewed head e231b72 |


Problem
FLAG_EVALUATIONS_ONLYmode still get every$feature_flag_calledevent written to theeventstable.eventswrites.flag_evaluationsin feat(flags): read flag last_called_at for data freshness #110143, feat(flags): count projects tab evaluations from flag_evaluations #110148, feat(flags): read flag calls from flag_evaluations on the activity page #110149 and feat(flags): read last_called_at sync calls from flag_evaluations behind a setting #110150. This PR ships after them.$feature_flag_calledfromeventswhile theexperiment-exposure-eventflag gates$experiment_exposure. chore(experiments): drop the exposure-event rollout flag check #106333 removes that flag, so on Cloud every experiment started after the cutoff counts exposures on$experiment_exposure, which still reachesevents. Experiments started before the cutoff keep reading$feature_flag_called.Changes
FLAG_EVALUATIONS_ONLY, a$feature_flag_calledcall now lands only inflag_evaluations.eventsToEmitonly after it queued that call'sflag_evaluationsrow.events: a team outside the ingestion allowlist, a call without a flag key, a mapping error, or a lane without the fork.$experiment_exposurecopy still reachesevents, and person processing still runs, because both happen before the fork.INGESTION_FLAG_EVALUATIONS_ONLY_DISABLED=truemakes ingestion treat the mode asREAD_FLAG_EVALUATIONSfor every team. One config change restores theeventswrites.ingestion_flag_evaluations_events_totalgains two outcomes:flag_evaluations_onlyandlost_message_too_large.message_size_too_largeingestion warning that emit-event would have sent.teamon to later steps, so the fork reads the mode without a team lookup. All three pipelines that use create-event already hadteam.eventsgap when it would lower an organization fromFLAG_EVALUATIONS_ONLY.FLAG_EVALUATIONS_ONLY, the staff preview andset_flag_evaluations_modeshow each organization's running experiments that count exposures on$feature_flag_called, and warn that those exposures stop for teams in the ingestion allowlist. A new experiments facade function counts them with the exposure query's own resolver, so it follows chore(experiments): drop the exposure-event rollout flag check #106333 once that lands.FlagEvaluationsModedocstrings, the staff APIhelp_textand the parity check guide now describe the real behavior. These are text changes only, so there is no Django migration.Warning
For these teams the
flag_evaluationsrow is the only copy of the call. A produce that fails after the step returns loses the call.produce_failedcounts it, and theFlagEvaluationsForkProduceFailingalert fires on it. A fallback write toeventsis out of scope.Note
The CDP events consumer reads the same
eventstopic. A destination or workflow triggered on$feature_flag_calledstops seeing calls from these organizations.Rollout prerequisites
FlagEvaluationsForkStalledoutcome regex includesflag_evaluations_onlyandlost_message_too_large, and the fork runbooks say a produce failure loses the call for these teams and nameINGESTION_FLAG_EVALUATIONS_ONLY_DISABLED.FLAG_EVALUATIONS_NEW_ORG_MODE, chore(experiments): drop the exposure-event rollout flag check #106333 merges or theexperiment-exposure-eventflag reaches every team, so new orgs' experiments count on$experiment_exposure. This PR itself can merge before either. After that, only an experiment started before the cutoff still reads$feature_flag_calledfromevents, so no organization running one moves toFLAG_EVALUATIONS_ONLY. The staff preview and the command warn about such experiments.FLAG_EVALUATIONS_ONLYyet in either region, so the deploy itself stops no organization'seventswrites. Each organization moved to that mode afterwards stops them when it moves.After deploy: move one internal test organization to
FLAG_EVALUATIONS_ONLYwith the staff tool, verify, then setFLAG_EVALUATIONS_NEW_ORG_MODE.How did you test this code?
Test rationale: The fork step tests extend the existing
fork-flag-evaluations-step.test.ts. They catch a call that leaveseventswhen it should not, an$experiment_exposurecopy that leaves with it, and a kill switch that does nothing. The service test catchescreateFlagEvaluationsServicedropping the env kill switch, which the step tests cannot see because they build the service directly. One case inflag-evaluations-e2e.test.tschecks the whole pipeline for a mode-2 team: the person update lands,flag_evaluationsgets the row, andeventsholds only the exposure copy.test-new-events-schemalabel. The Python changes are docstrings and comments, and Backend CI does not run Node ingestion.Release status
The stored organization mode gates the behavior, and only organizations already on
FLAG_EVALUATIONS_ONLYsee it.Automatic notifications
Docs update
None. No doc under
docs/covers how ingestion writes these events.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5 (Sonnet 5.5 for comment cleanup and the description)
/gopipeline. Session: https://claude.ai/code/session_012LLLo3Hxxc1Vhne69sC9Hi/writing-tests,/writing-code-comments,/writing-user-facing-copy,/simplify,/reviewing-with-coderabbit,/writing-pr-descriptions. The pipeline-step and pipeline-result doctor agents reviewed the ingestion step.TeamManager.getTeaminside the fork. Simplify and the step doctor both pointed at create-event droppingteam, so create-event now passes it through.--deepand reported no findings./review-coderunner stopped at synthesis after all eight reviewers reported, because it wanted a production query. The fixes in the second commit come from those reports. The experiments gap and the charts changes are listed under Rollout prerequisites.