fix(api): reject non-object conversation settings - #108162
pauldambra wants to merge 4 commits into
Conversation
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review 🦔 PostHog Review reviewed this pull requestNothing worth raising this time. Enjoy the moment:
|
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
posthog/temporal/ai_observability/run_aggregate_evaluation.py:692 |
posthog/temporal/ai_observability/run_trace_evaluation.py:928 |
38 | 203 |
posthog/api/project.py:641 |
posthog/api/team.py:1870 |
21 | 168 |
products/warehouse_sources/backend/temporal/data_imports/sources/companycam/source.py:1 |
products/warehouse_sources/backend/temporal/data_imports/sources/wix/source.py:1 |
21 | 164 |
posthog/temporal/ai_observability/run_aggregate_evaluation.py:584 |
posthog/temporal/ai_observability/run_trace_evaluation.py:855 |
24 | 135 |
products/signals/backend/scout_harness/tools/report.py:1475 |
products/signals/backend/scout_harness/tools/report.py:1647 |
21 | 133 |
products/signals/backend/scout_harness/tools/report.py:1604 |
products/signals/backend/scout_harness/tools/report.py:1754 |
30 | 118 |
posthog/temporal/ai_observability/run_session_evaluation.py:466 |
posthog/temporal/ai_observability/run_trace_evaluation.py:664 |
24 | 115 |
posthog/management/commands/backfill_hogflow_billable_action_types.py:49 |
posthog/management/commands/refresh_hog_flows.py:69 |
23 | 112 |
products/batch_exports/backend/api/batch_export.py:2092 |
products/batch_exports/backend/api/batch_export.py:2131 |
24 | 112 |
products/warehouse_sources/backend/temporal/data_imports/sources/pardot/pardot.py:2 |
products/warehouse_sources/backend/temporal/data_imports/sources/wix/wix.py:1 |
12 | 97 |
products/signals/backend/scout_harness/tools/report.py:1516 |
products/signals/backend/scout_harness/tools/report.py:1675 |
12 | 71 |
products/signals/backend/scout_harness/tools/report.py:1531 |
products/signals/backend/scout_harness/tools/report.py:1690 |
16 | 70 |
⚠️ Duplication (TypeScript) — 2 new duplicated blocks (worst 158 tokens)
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.
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/business_knowledge/frontend/scenes/settings/businessKnowledgeSettingsLogic.test.ts:10 |
products/data_catalog/frontend/certificationsLogic.test.ts:17 |
23 | 158 |
products/business_knowledge/frontend/scenes/settings/businessKnowledgeSettingsLogic.test.ts:10 |
products/data_quality/frontend/dataQualityGateLogic.test.ts:14 |
23 | 158 |
⚠️ Bundle size — 🔺 +33.8 KiB (+0.0%)
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 68.92 MiB · 🔺 +33.8 KiB (+0.0%)
| File | Size | Δ vs base |
|---|---|---|
posthog-app/_parent/products/business_knowledge/frontend/scenes/BusinessKnowledgePlaygroundScene.js |
removed | 🟢 -26.2 KiB (-100.0%) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/playground/BusinessKnowledgePlaygroundScene.js |
26.1 KiB | 🔺 +26.1 KiB (new) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/BusinessKnowledgeScene.js |
removed | 🟢 -18.7 KiB (-100.0%) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/sources/BusinessKnowledgeScene.js |
18.6 KiB | 🔺 +18.6 KiB (new) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/KnowledgeSourceScene.js |
removed | 🟢 -18.0 KiB (-100.0%) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/source/KnowledgeSourceScene.js |
18.0 KiB | 🔺 +18.0 KiB (new) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/BusinessKnowledgeSettingsScene.js |
removed | 🟢 -16.8 KiB (-100.0%) |
posthog-app/_parent/products/business_knowledge/frontend/scenes/settings/BusinessKnowledgeSettingsScene.js |
16.8 KiB | 🔺 +16.8 KiB (new) |
posthog-app/_parent/products/signals/frontend/inbox/InboxScene.js |
488.4 KiB | 🔺 +13.6 KiB (+2.9%) |
render-query/src/render-query/render-query.js |
20.16 MiB | 🔺 +6.4 KiB (+0.0%) |
posthog-app/_parent/products/ai_observability/frontend/evaluations/AIObservabilityEvaluation.js |
87.1 KiB | 🔺 +6.0 KiB (+7.4%) |
posthog-app/_parent/products/workflows/frontend/Broadcasts/BroadcastScene.js |
67.3 KiB | 🔺 +3.7 KiB (+5.9%) |
posthog-app/_parent/products/stamphog/frontend/scenes/StamphogDigestsScene/StamphogDigestsScene.js |
9.5 KiB | 🔺 +1.6 KiB (+20.7%) |
posthog-app/_parent/products/stamphog/frontend/scenes/StamphogScene/StamphogScene.js |
22.1 KiB | 🔺 +1.1 KiB (+5.4%) |
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.57 MiB · 22 files | no change | █████████░ 85.5% 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.51 MiB · 629 files | 🔺 +1.6 KiB (+0.0%) | █████████░ 87.2% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.34 MiB · 2,339 files | 🔺 +2.7 KiB (+0.0%) | █████████░ 88.0% of 8.34 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 |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 216.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 |
| 88.5 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.4 KiB | src/scenes/scenes.ts |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 272.2 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 216.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 |
| 100.5 KiB | src/lib/api.ts |
| 98.5 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 88.5 KiB | src/products.tsx |
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.16 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.16 MiB · 19 files | 🔺 +148 B (+0.0%) | ████░░░░░░ 37.7% of 5.72 MiB |
| Deferred (lazy) | 2.10 MiB · 44 files | 🔺 +528 B (+0.0%) | 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 |
|---|---|
| 800.5 KiB | dist/toolbar/toolbar-app-QUJ43CJ4.css |
| 651.7 KiB | dist/toolbar/chunk-chunk-ACEXZHU5.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-CV2VU6SQ.js |
| 138.3 KiB | dist/toolbar/chunk-chunk-VP2W3YP2.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-PZ4OESK6.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-O7ZEOSI5.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-P6OO55L7.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 — 🔺 +650.3 KiB (+0.1%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 946.90 MiB · 🔺 +650.3 KiB (+0.1%)
⚠️ MCP snapshots — 1 updated (1 modified, 0 added, 0 deleted)
Snapshots: MCP unit test snapshots updated
Changes: 1 snapshots (1 modified, 0 added, 0 deleted)
What this means:
- Snapshots have been automatically updated to match current output
Next steps:
- Review the changes to ensure they're intentional
- If unexpected, investigate what caused the output to change
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 ignored due to path filters (2)
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughBoth Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Responses for settings containing legacy non-object JSON could contradict the generated object-or-null type. The risk is limited to such records, whose current presence is unconfirmed. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change rejects malformed settings without an identified expansion of access or newly introduced security issue. Older saved values, if present, could still appear in responses that now promise an object or null. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
|
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
7acfb9a to
8e2c76c
Compare
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
[Robot] Note 🤖 Automated comment by QA Swarm — not written by a human Verdict: decision pending (round 3 @ bb5942b)The request schema now matches the object-or-null validation. The two schema findings are fixed. One response compatibility question remains open. Key findings
ConvergenceBoth original schema findings identified the same missing request contract. One fix covers both serializers. Reviewer summaries
Validation: 36 focused API tests, OpenAPI generation, frontend and MCP type checks, Ruff, and commit/push hooks passed. The automatic MCP snapshot update matches the schema change. Previous roundsRound 1 at 8e2c76c: no input-validation defects found. Two later bot findings requested a typed API schema. Automated by QA Swarm — not a human review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e2c76c741
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
| @extend_schema_field(OpenApiTypes.OBJECT) | ||
| class ConversationsSettingsField(serializers.JSONField): |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Keep response schemas accurate for existing settings
Issue description
This field annotation narrows both request and response schemas to objects or null. But JSONField can still return any stored JSON value, and these endpoints accepted arrays and some scalars before this change. Existing records with those values can therefore produce responses that violate the newly generated object-or-null response type.
Why we think it's a valid issue
- Checked: Compared the PR changes with the base version and traced
ConversationsSettingsFieldthroughTeamSerializerandProjectBackwardCompatSerializerto the project and team response paths. - Found:
@extend_schema_field(OpenApiTypes.OBJECT)annotates the shared field atposthog/api/team.py:1346-1348. Both serializers use it atposthog/api/team.py:1368-1373andposthog/api/project.py:632-637. The new validators reject non-dictionaries on input atposthog/api/team.py:1894-1898andposthog/api/project.py:653-657, but the model stores this setting in a nullablemodels.JSONFieldatposthog/models/team/team.py:446. Response serialization does not apply those input validators, and the base serializer previously exposed the value asunknownin the generated schema. - Impact: Previously accepted non-object values can remain in stored settings and be returned by these endpoints, despite the narrowed response schema. This is a reachable API compatibility mismatch, so the response schema should remain broad or the stored values should be cleaned before narrowing it.
Suggested fix
Keep the object-or-null constraint on request schemas, but preserve a response schema that matches stored values until existing records are cleaned up. Alternatively, clean up invalid stored values before narrowing the response contract.
Prompt to fix with AI (copy-paste)
## Context
@posthog/api/team.py#L1346-1347
<issue_description>
This field annotation narrows both request and response schemas to objects or null. But `JSONField` can still return any stored JSON value, and these endpoints accepted arrays and some scalars before this change. Existing records with those values can therefore produce responses that violate the newly generated object-or-null response type.
</issue_description>
<issue_validation>
- **Checked:** Compared the PR changes with the base version and traced `ConversationsSettingsField` through `TeamSerializer` and `ProjectBackwardCompatSerializer` to the project and team response paths.
- **Found:** `@extend_schema_field(OpenApiTypes.OBJECT)` annotates the shared field at `posthog/api/team.py:1346-1348`. Both serializers use it at `posthog/api/team.py:1368-1373` and `posthog/api/project.py:632-637`. The new validators reject non-dictionaries on input at `posthog/api/team.py:1894-1898` and `posthog/api/project.py:653-657`, but the model stores this setting in a nullable `models.JSONField` at `posthog/models/team/team.py:446`. Response serialization does not apply those input validators, and the base serializer previously exposed the value as `unknown` in the generated schema.
- **Impact:** Previously accepted non-object values can remain in stored settings and be returned by these endpoints, despite the narrowed response schema. This is a reachable API compatibility mismatch, so the response schema should remain broad or the stored values should be cleaned before narrowing it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Keep the object-or-null constraint on request schemas, but preserve a response schema that matches stored values until existing records are cleaned up. Alternatively, clean up invalid stored values before narrowing the response contract.
</potential_solution>
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Small, contained validation tightening on one project-settings field with parameterized tests for both serializers. The one open bot thread is about response-schema typing for legacy non-object values, which is a minor compatibility nit and does not affect runtime.
- Author wrote 0% of the modified lines and has 92 merged PRs in these paths (familiarity MODERATE).
- Non-blocking: the open ReviewHog thread on posthog/api/team.py notes that the generated response type now says object-or-null, while any legacy stored array or scalar values would still serialize as-is. This is likely rare and only affects typings.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 20L, 2F substantive, 108L/7F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (108L, 7F, cross-cutting, fix) |
| stamphog 2.3.0 | .stamphog/policy.yml @ bb5942b · reviewed head bb5942b |
This branch was successfully deployed
1 participant
[Robot] Prepared by PostHog Desktop.
Problem
Project settings accept conversation settings with the wrong JSON type.
Changes
Return a validation error for arrays and scalar values. Keep objects and null valid.
This fixes existing behavior before the module split in #99248.
How did you test this code?
Parameterized serializer tests cover both project and environment endpoints. The focused tests ran against both module layouts. The full type check covers the combined fixes.
Release status
Automatic notifications
Docs update
No existing document under
docs/covers this validation detail.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Desktop / Codex, GPT-6
Tools: Git, GitHub CLI, hogli, pytest, Ruff, mypy, and CodeRabbit.
Skills: stacking-prs, improving-drf-endpoints, writing-tests, writing-code-comments, writing-user-facing-copy, setting-up-devbox, running-ci-preflight, reviewing-with-coderabbit, writing-pr-descriptions.
Each existing bug has its own layer below the refactor. No matching open fix appeared in the PR search.
The combined CodeRabbit review found two deployment tradeoffs in the membership layer. That PR records the decisions.
Created with PostHog Desktop