refactor(api): split team settings module - #99248
pauldambra wants to merge 9 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
posthog/api/project.py:387 |
posthog/api/team/viewsets.py:465 |
41 | 282 |
posthog/api/project.py:533 |
posthog/api/team/integration_config.py:383 |
34 | 271 |
posthog/api/project.py:538 |
posthog/api/team/integration_config.py:388 |
32 | 213 |
posthog/api/project.py:2066 |
posthog/api/team/viewsets.py:555 |
32 | 145 |
posthog/api/project.py:644 |
posthog/api/team/team_serializer.py:581 |
15 | 115 |
posthog/api/project.py:2031 |
posthog/api/team/viewsets.py:521 |
17 | 111 |
posthog/api/project.py:1551 |
posthog/api/team/viewsets.py:190 |
19 | 97 |
posthog/api/project.py:1240 |
posthog/api/team/team_serializer.py:938 |
12 | 80 |
posthog/api/project.py:1347 |
posthog/api/team/team_serializer.py:1228 |
12 | 78 |
posthog/api/project.py:1499 |
posthog/api/team/viewsets.py:128 |
16 | 74 |
✅ 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 — 7% of added code lines are comments (218 of 2960)
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 |
|---|---|---|
posthog/api/team/team_serializer.py |
88 | 1072 |
posthog/api/team/viewsets.py |
42 | 543 |
posthog/api/team/integration_config.py |
29 | 381 |
posthog/api/team/marketing_config.py |
19 | 207 |
posthog/api/team/settings_validation.py |
16 | 218 |
posthog/api/team/team_config.py |
13 | 301 |
posthog/api/team/conversations_settings.py |
5 | 67 |
posthog/api/project.py |
3 | 30 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Backend coverage — 69.0% of changed backend lines covered — 506 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ██████████████░░░░░░ 69.0% (1,164 / 1,670)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/surveys/backend/debug.py |
0.0% | 3 |
posthog/api/team/team_permissions.py |
30.0% | 15–16, 18–21, 25, 27–29, 32–34, 36 |
posthog/api/team/viewsets.py |
39.2% | 114, 116–123, 126–128, 132–134, 139–142, 147, 150–151, 154–157, 160, 168, 177–180, 182–183, 186–187, 189, 191, 193, 196–200, 204–207, 209–214, 219, 223–224, 228–230, 236, 238–240, 242, 246, 248, 255, 266, 275–277, 286–289, 298–300, 310–311, 314, 323–324, 327, 350, 373, 383, 416, 424–425, 429, 431, 433, 440, 450, 452–454, 456–458, 461–462, 465–471, 473, 476, 483–489, 492–494, 496–501, 505, 507–509, 511, 520–523, 525–526, 528, 537, 548–552, 554–555, 557–560, 562, 571–572, 586, 597, 601–602, 620 |
posthog/api/team/team_serializer.py |
58.2% | 62, 216, 219, 223, 230, 234, 240, 244, 248, 253, 257, 292–293, 297, 327, 341, 347, 353, 377, 389, 410, 413, 415, 419, 438, 440, 442–443, 456–458, 466–467, 471, 491, 494, 499, 505, 508–509, 512–513, 523, 527, 579, 583, 586, 589–591, 595, 602–611, 613–629, 632–640, 642, 644–652, 658, 660, 664–665, 669–682, 686–691, 693–695, 697–699, 702, 706, 710, 722, 728, 755, 792, 795, 811, 830, 845–846, 852, 856, 861–862, 865–869, 871–874, 877, 879, 885–887, 889, 900, 903, 906–907, 909–910, 912–913, 915–916, 918–919, 921–922, 926–928, 933–934, 936, 943–944, 958, 968–969, 971, 974, 980, 986–989, 991, 996–997, 1007–1009, 1011, 1014–1015, 1019–1021, 1023, 1025, 1039, 1045, 1049, 1054, 1060–1061, 1063, 1066, 1071, 1073–1075, 1077, 1081, 1083, 1087, 1100, 1106–1107, 1111, 1114, 1123–1124, 1127, 1136, 1142–1143, 1145, 1147, 1151–1152, 1155, 1160, 1166–1167, 1169, 1171, 1175–1176, 1179, 1184, 1190–1191, 1193, 1195, 1199–1200, 1203–1204, 1206–1207, 1210–1211, 1215–1216, 1221, 1228–1229 |
posthog/api/team/integration_config.py |
80.6% | 238, 326, 328–333, 336–337, 342–343, 346, 386, 388–395, 397, 399–403, 405–408, 416 |
posthog/api/project.py |
83.3% | 544–545 |
posthog/api/team/marketing_config.py |
84.0% | 212, 214, 216, 218, 220, 228, 231–232, 234, 237, 240, 247, 250, 253, 256, 259 |
posthog/api/test/test_team_tracing_config.py |
87.5% | 233–234 |
posthog/api/livestream.py |
92.9% | 27, 31, 33 |
posthog/api/team/settings_validation.py |
94.3% | 34, 59, 63, 78, 168, 173, 232, 256 |
posthog/api/team/team_config.py |
96.2% | 186, 192, 335 |
posthog/api/team/live_events.py |
97.2% | 88 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 36546974920 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
warehouse_sources_queue |
██████░░░░░░░░░░░░░░ 29.1% |
92 / 316 |
demo |
███████████░░░░░░░░░ 52.9% |
1,413 / 2,673 |
data_tools |
████████████░░░░░░░░ 61.2% |
90 / 147 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
aeo |
███████████████░░░░░ 76.3% |
617 / 809 |
batch_exports |
████████████████░░░░ 81.2% |
21,476 / 26,449 |
apm |
█████████████████░░░ 84.1% |
1,306 / 1,553 |
cdp |
██████████████████░░ 88.2% |
4,545 / 5,155 |
ml_inference |
██████████████████░░ 88.4% |
509 / 576 |
mcp_analytics |
██████████████████░░ 88.9% |
4,927 / 5,540 |
product_tours |
██████████████████░░ 89.3% |
1,331 / 1,491 |
dashboards |
██████████████████░░ 89.5% |
6,855 / 7,657 |
data_warehouse |
██████████████████░░ 90.0% |
14,019 / 15,581 |
signals |
██████████████████░░ 90.1% |
56,600 / 62,839 |
notebooks |
██████████████████░░ 90.2% |
15,287 / 16,945 |
cohorts |
██████████████████░░ 90.4% |
8,420 / 9,316 |
streamlit_apps |
██████████████████░░ 90.6% |
2,623 / 2,895 |
tasks |
██████████████████░░ 91.0% |
74,108 / 81,470 |
managed_warehouse |
██████████████████░░ 91.0% |
10,252 / 11,263 |
data_modeling |
██████████████████░░ 91.4% |
10,504 / 11,498 |
exports |
██████████████████░░ 91.6% |
9,680 / 10,562 |
engineering_analytics |
██████████████████░░ 91.7% |
11,032 / 12,030 |
ai_training |
██████████████████░░ 92.2% |
356 / 386 |
business_knowledge |
██████████████████░░ 92.2% |
7,684 / 8,330 |
conversations |
███████████████████░ 92.5% |
28,734 / 31,062 |
early_access_features |
███████████████████░ 92.6% |
1,341 / 1,448 |
managed_migrations |
███████████████████░ 92.7% |
1,581 / 1,705 |
visual_review |
███████████████████░ 92.8% |
9,247 / 9,969 |
canvas |
███████████████████░ 92.8% |
6,877 / 7,409 |
approvals |
███████████████████░ 93.0% |
3,974 / 4,271 |
mcp_registry |
███████████████████░ 93.1% |
1,670 / 1,794 |
error_tracking |
███████████████████░ 93.2% |
15,928 / 17,097 |
notifications |
███████████████████░ 93.2% |
1,145 / 1,229 |
slack_app |
███████████████████░ 93.2% |
13,677 / 14,674 |
stamphog |
███████████████████░ 93.2% |
7,885 / 8,456 |
surveys |
███████████████████░ 93.3% |
6,571 / 7,040 |
context_layer |
███████████████████░ 93.9% |
3,415 / 3,638 |
web_analytics |
███████████████████░ 94.0% |
21,815 / 23,218 |
alerts |
███████████████████░ 94.0% |
8,570 / 9,114 |
billing_alerts |
███████████████████░ 94.1% |
2,094 / 2,226 |
mcp_store |
███████████████████░ 94.4% |
8,940 / 9,472 |
wizard |
███████████████████░ 94.7% |
6,150 / 6,496 |
ai_observability |
███████████████████░ 94.7% |
24,139 / 25,489 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
workflows |
███████████████████░ 94.8% |
14,332 / 15,113 |
review_hog |
███████████████████░ 95.0% |
11,507 / 12,119 |
endpoints |
███████████████████░ 95.1% |
9,206 / 9,681 |
annotations |
███████████████████░ 95.1% |
817 / 859 |
customer_analytics |
███████████████████░ 95.2% |
25,078 / 26,349 |
legal_documents |
███████████████████░ 95.2% |
2,311 / 2,427 |
marketing_analytics |
███████████████████░ 95.3% |
19,322 / 20,274 |
posthog_ai |
███████████████████░ 95.3% |
2,488 / 2,610 |
experiments |
███████████████████░ 95.4% |
32,458 / 34,023 |
actions |
███████████████████░ 95.5% |
756 / 792 |
logs |
███████████████████░ 95.5% |
15,399 / 16,130 |
data_catalog |
███████████████████░ 95.6% |
4,402 / 4,606 |
tracing |
███████████████████░ 95.6% |
3,536 / 3,699 |
replay_vision |
███████████████████░ 95.6% |
27,675 / 28,939 |
autoresearch |
███████████████████░ 95.7% |
8,481 / 8,865 |
growth |
███████████████████░ 95.7% |
11,228 / 11,734 |
messaging |
███████████████████░ 95.8% |
3,798 / 3,963 |
skills |
███████████████████░ 95.8% |
6,972 / 7,274 |
product_analytics |
███████████████████░ 96.0% |
28,470 / 29,647 |
access_control |
███████████████████░ 96.3% |
7,112 / 7,386 |
revenue_analytics |
███████████████████░ 96.4% |
1,876 / 1,946 |
user_interviews |
███████████████████░ 96.5% |
2,867 / 2,971 |
feature_flags |
███████████████████░ 96.5% |
25,560 / 26,480 |
warehouse_sources |
███████████████████░ 97.2% |
456,707 / 469,652 |
data_quality |
████████████████████ 97.6% |
7,587 / 7,774 |
security |
████████████████████ 97.9% |
1,202 / 1,228 |
links |
████████████████████ 97.9% |
234 / 239 |
metrics |
████████████████████ 98.0% |
4,084 / 4,166 |
analytics_platform |
████████████████████ 98.3% |
2,784 / 2,833 |
pulse |
████████████████████ 98.5% |
2,046 / 2,078 |
live_debugger |
████████████████████ 99.2% |
626 / 631 |
field_notes |
████████████████████ 99.4% |
172 / 173 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
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. |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6301687885
ℹ️ 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".
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (security-audit, qa-team/paul/xp lenses) as warranted Verdict: 💬 APPROVE WITH NITS (round 5 @ 457b7af)Router pass found nothing itself but the diff (7k+ lines across 20+ files, touching Team/Project permissions) tripped the mandatory-delegation line, so it split the review: a security-audit pass on the permission/authorization surface, and a combined reliability + paul + xp pass on split-correctness across the whole Key findings
Convergence
Reviewer summaries
Previous rounds (4)round 1 @ 6301687 — request changes: a stale router reference required a package-boundary fix. Automated by QA Swarm — not a human review |
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.
Gates denied this PR purely for size — a 6000+ line mechanical module split far exceeds the auto-review ceiling — and a discussion comment raises a concrete, unaddressed high-severity concern that the router may still reference a symbol from the old module location, which would break initialization. Combined with cross-team ownership and no independent human/agent approval on the current head, this needs manual review.
- Author wrote 18% of the modified lines and has 235 merged PRs in these paths (familiarity MODERATE).
- Gates denied: change is far larger than the auto-review size ceiling (6158 substantive lines vs 800).
- A QA Swarm discussion comment claims posthog/api/rest_router.py may still reference ProjectEnvironmentsViewSet from the old team.py location, which would break router initialization — not visibly resolved in the diff/comments.
- Cross-team change (team-product-analytics, team-surveys, team-web-analytics) with author not on any owning team and no approving review from an owning-team member or agent reviewer on the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✗ | too large for auto-review (6158L substantive in global — ceiling is 800L; 6158L, 17F total, 6222L/27F incl. docs/generated/snapshots) |
| tier | ✓ | T1-agent / T1d-complex (6222L, 27F, two-areas, refactor) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 6957593 · reviewed head 6957593 |
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.
Gates denied this PR for size (far beyond the auto-review ceiling), and it's a large cross-team refactor of core team/project settings code with no human review — only bot/QA-swarm comments. That combination requires human sign-off.
- Author wrote 18% of the modified lines and has 235 merged PRs in these paths (familiarity MODERATE).
- Deterministic size gate failed (6184 substantive lines vs 800 ceiling), so this PR is outside auto-review scope regardless of content.
- Cross-team change (touches team-product-analytics, team-surveys, team-web-analytics owned files) with no human reviewer approval yet — only bot/automated review comments.
- Author is not on any owning team and has only MODERATE familiarity with the touched lines, which does not substitute for team review in this large a refactor of team settings/permissions code.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✗ | too large for auto-review (6184L substantive in global — ceiling is 800L; 6184L, 17F total, 6248L/27F incl. docs/generated/snapshots) |
| tier | ✓ | T1-agent / T1d-complex (6248L, 27F, two-areas, refactor) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 6990ff1 · reviewed head 6990ff1 |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
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.
Gates denied on size, and separately this touches auth/security-sensitive surface (widget token validation, JWT scope, token revocation) with four unresolved bot-flagged security concerns that were escalated rather than fixed; author is cross-team with only moderate familiarity, so there's no independent assurance covering the risky part.
- Author wrote 18% of the modified lines and has 235 merged PRs in these paths (familiarity MODERATE).
- Project settings endpoint doesn't strip widget_public_token/integration keys the way the team endpoint does, allowing duplicate widget tokens and a 500 in widget auth (unresolved, escalated by reviewer)
- live_events_token is minted for any project:read-scoped caller even though live event data elsewhere requires query:read (unresolved, escalated)
- Live event JWTs are valid 7 days with no membership/revocation check, so removed members retain stream access (unresolved, escalated)
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✗ | too large for auto-review (6187L substantive in global — ceiling is 800L; 6187L, 17F total, 6251L/27F incl. docs/generated/snapshots) |
| tier | ✓ | T1-agent / T1d-complex (6251L, 27F, two-areas, refactor) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ c3cd0ee · reviewed head c3cd0ee |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (28)
Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe team API is split from aggregate module imports into dedicated package modules. New modules add team configuration serializers, validation, marketing analytics models, integration handlers, conversation settings, permissions, viewsets, and live-events caching. Routers, application code, tests, and mocks now reference the dedicated modules. Team serialization supports nested updates, access checks, activity logging, cache refresh, and evaluation-context settings. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The package split preserves current import wiring, with no identified merge-blocking behavior change. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@pauldambra I didn't want to commit direclty so I sent some changes here: #102991 |
lricoy
left a comment
There was a problem hiding this comment.
Approving to unblock, just some notes. I think this is long overdue actually, so I am glad we're doing it
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.
This pull request was refused by the size gate: it contains 6675 substantive lines across 17 files (6742 lines / 28 files including docs/generated/snapshots), well beyond the 800-line ceiling for automatic review. This mostly stems from posthog/api/team.py being removed (-3262 lines) and its contents redistributed into eight new modules under posthog/api/team/ (e.g. team_serializer.py at +1276, viewsets.py at +622, integration_config.py at +423), plus updates to callers and tests. To move forward, ask a human reviewer to review this refactor directly, or split it into a sequence of smaller, independently reviewable pull requests (for example, one per extracted module).
- posthog[bot] reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✗ | too large for auto-review (6675L substantive in global — ceiling is 800L; 6675L, 17F total, 6742L/28F incl. docs/generated/snapshots) |
| tier | ✓ | T1-agent / T1d-complex (6742L, 28F, two-areas, refactor) |
| stamphog 2.2.0 | .stamphog/policy.yml @ unknown · reviewed head 457b7af |
|
✅ Security review complete — No findings above the confidence threshold, in 16 min. Add the |
Piccirello
left a comment
There was a problem hiding this comment.
Only reviewed changes to posthog/auth.py
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
443a26d to
d25fca7
Compare

[Robot] Updated by PostHog Desktop.
Problem
Engineers need smaller modules to change project settings safely.
Changes
Split the team settings module into focused files. Update imports, router registration, and test mock paths.
This PR contains the module split. Separate bug-fix PRs form the eight layers below it in the native GitHub stack.
Bottom to top:
How did you test this code?
The same focused API and validation suites ran before and after the split. Both layouts passed the full mypy check.
OpenAPI generation completed with no generated-file changes. Ruff and security checks passed.
The function comparison found only module references, annotations, declaration order, and comment changes in this layer.
Release status
Automatic notifications
Docs update
No existing document under
docs/covers this internal module layout. The scope documentation change is in the token-scope layer.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Desktop / Codex, GPT-6
The review fixes now sit below this refactor, one existing bug per PR. The retention layer also corrects the lock issue introduced during review fixes.
CodeRabbit found no issues in the initial refactor review. The combined fixes review raised two deployment tradeoffs, recorded in the membership PR.
Tools: Git, GitHub CLI, hogli, pytest, mypy, Ruff, Semgrep, and CodeRabbit.
Skills: stacking-prs, improving-drf-endpoints, writing-tests, writing-code-comments, writing-user-facing-copy, running-ci-preflight, setting-up-devbox, reviewing-with-coderabbit, writing-pr-descriptions, debugging-ci-failures.
Created with PostHog Desktop