trunk-merge/pr-107852/9584165b-5116-41fc-b89e-1b471442e0bc - #107949
Closed
trunk-io[bot] wants to merge 69 commits into
Closed
trunk-io[bot] wants to merge 69 commits into
trunk-io[bot] wants to merge 69 commits into
Conversation
A ModelSerializer builds every concrete column as writable unless read_only_fields says otherwise, so a self-filling DateTimeField reaches the API as a field a client can overwrite. FeatureFlagSerializer carried that hole on created_at and last_called_at for years. The repo invariant walks the URL conf with drf-spectacular's EndpointEnumerator, collects the ModelSerializer behind every POST, PUT and PATCH route, and fails on a writable DateTimeField whose model column declares auto_now, auto_now_add or a default. The nine fields that violate it today are in ALLOWED_WRITABLE, each with a one-line reason. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
Reading only the view class's serializer_class missed every viewset that picks a write serializer in get_serializer_class or names one with @extend_schema(request=...), so a server-owned timestamp on such a serializer passed CI. The guard now resolves the serializer per action against a throwaway request, and also reads the decorator. It enumerates routes with DRF's EndpointEnumerator instead of the drf-spectacular subclass, because the subclass runs preprocess_exclude_path_format, which drops every INTERNAL and undocumented route. Those routes still accept writes. This widens coverage from 205 to 290 serializers and found MyNotificationsSerializer.created_at. A view or serializer the guard cannot read no longer disappears in silence. Both failures are collected and asserted against UNCHECKED, an explicit list that needs a reason per entry. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
No serializer in the repo hides a violation behind get_serializer_class today, so the repo-wide sweep stays green if that resolution narrows back to the class attribute. A direct case on a viewset that only overrides get_serializer_class closes that gap. The docstring and the failure message said read_only_fields clears any field. DRF ignores read_only_fields for a field the serializer declares, so following that advice on a declared field leaves the violation in place. Both now say which fix applies to which kind of field. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
Calling get_serializer_class against a synthesized request returned the one serializer that matched that request, so a view branching on a header or on request data showed the guard a single arm. InsightViewSet selects MCPInsightSerializer only when x-posthog-client is mcp, so that serializer went unchecked. A plain APIView has no callback.actions at all, so its dynamic choice and its @extend_schema(request=...) body were both skipped. The guard now reads every get_serializer_class in the MRO as an AST and resolves each returned expression, including conditional arms, lookup tables, deferred imports and locals. Discovery goes from 290 serializers to 323, a strict superset, and nothing is left unresolved. UNCHECKED is now empty. A bare default=None no longer counts as server ownership: it means nullable, and it flagged timestamps a client is supposed to set, such as a batch export's start_at. A column named the way audit and telemetry columns are named now counts instead, which is the only signal a plain nullable column gives. That is what makes FeatureFlag.last_called_at enforceable, and it found four more real violations, including GroupTypeMapping.created_at, whose model sets it in code. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
ty rejected the list[ast.expr] annotation in the timestamp guard. list is invariant in its type parameter, so the list[Name | Attribute | Subscript] that ast.AnnAssign.target produces is not assignable to it. Sequence is covariant and accepts both branches. mypy accepted the annotation, so a single-file mypy run missed this. ty check, mypy --cache-fine-grained . and ruff now all run repo-wide before the push. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
…es them The guard claimed to read @extend_schema(request=...) but looked for a _spectacular_annotation attribute, which this version of drf-spectacular does not set. The reader returned nothing on every route, so every serializer declared only as a request body went unchecked, including the create, update and partial_update bodies that four viewsets declare through @extend_schema_view. drf-spectacular does not keep the declaration as data. It builds a schema class that closes over `request` and stores it in the handler's kwargs, so the value is read out of that closure. Discovery goes from 323 serializers to 792, and stays at zero unresolved. The violation set and the allowlist are unchanged, so the coverage that was missing held no violation. A declaration is also not always a class: inline_serializer returns an instance, many=True wraps the real serializer in a ListSerializer, and a media-type mapping holds one declaration per content type. All of those now normalize to classes. Two tests pin the mechanism. One decorates a local viewset with extend_schema_view and asserts the reader still finds the declared serializer, so a drf-spectacular upgrade that moves the closure fails loudly instead of narrowing discovery in silence. The other pins instance, many=True and mapping normalization. vars() is no longer called on the result of a sys.modules lookup that can be None. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
Every serializer a view mentioned was attributed to every one of its write routes, so a serializer the view only hands to list or retrieve was reported as writable on a write route. InsightViewSet returns InsightBasicSerializer only when the action is list or retrieve, and its last_modified_at sat in the allowlist as a hole an owning team could not act on. Each return is now paired with the actions that have to hold to reach it, and a route only sees the returns its own action can reach. The constraint is read from a plain self.action comparison, following a self.is_x() guard one level, because InsightViewSet keeps the check in a helper rather than in the branch test. An unrecognised guard constrains nothing, and an else inherits the enclosing constraint rather than the complement, so an unreadable branch widens the search instead of narrowing it. A return the resolver cannot read no longer passes silently when it is a call. super().get_serializer_class() is the one call the MRO walk already covers; every other one, such as a return choose_serializer(self.action) factory, now reports through UNCHECKED. No such factory exists today, so this only holds the line. Discovery goes from 792 serializers to 771 and the allowlist from 11 entries to 10. The dropped entry was the false positive; no violation was lost. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
The guard asserted that every violation appears in ALLOWED_WRITABLE, but never the reverse, so an entry survived its field being made read-only, renamed, or dropped. The larger hole is that a change narrowing discovery removed fields from the sweep without failing anything: the test only walks what discovery returns, so fields it stops reaching simply go unchecked. test_allowlist_entries_still_match_a_violation closes both, over ALLOWED_WRITABLE and UNCHECKED. Verified against both: a bogus entry, and a simulated narrowing of discovery. This assertion existed earlier on the branch and was lost in the rewrite that made serializer resolution static. The pull request description claimed it was still protecting the list; it was not, and the claim is corrected. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
The guard inferred server ownership from the column: auto_now, a non-null default, or a name that looked like an audit column. Both signals answer the wrong question. A non-null default is evidence of self-filling, not of ownership, so published_at = DateTimeField(default=timezone.now) reads as server-owned while a client is meant to set it. A naming convention guesses in both directions at once. Ownership is no longer inferred. Every writable DateTimeField a write route exposes must appear in REVIEWED_WRITABLE with a reason a person wrote, and a field missing from that list fails the build. A reason starting with TODO marks a hole the owning team still has to close; the rest record a value a client is supposed to set. The list holds all 64 such fields. Classifying them found 10 server-owned holes the naming rule missed, doubling the count from 10 to 20: Dashboard.last_refresh, which the refresh path writes; Survey.current_iteration_start_date, which the iteration task advances; the batch export backfill's finished_at and adjusted_start_at; SessionRecording.start_time and end_time, derived from ingested events; and the replay evaluation's started_at and completed_at. None matched the regex. Three fields on BatchExportRunSerializer are recorded as response-only, because that viewset is read-only and never deserializes a request body through the serializer. The two unit tests that pinned the heuristic covered code that no longer exists, so they are gone. The stale-entry test now also catches a future attempt to reintroduce inference: a filter that skipped a field would leave its entry unmatched. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
The guard asserted that every writable timestamp appears in the registry, but never the reverse, so an entry survived its field being made read-only, renamed, or dropped. The larger hole is that a change narrowing discovery removed fields from the sweep without failing anything: the test only walks what discovery returns, so fields it stops reaching simply go unchecked. test_reviewed_entries_still_match_a_writable_field closes both. A stale entry fails, and so does a discovery change that drops a reviewed field, because that field's entry no longer matches. Verified against both: a bogus entry, and a simulated reintroduction of ownership inference. This assertion existed earlier on the branch and was lost in the rewrite that made serializer resolution static. Two pull request descriptions claimed it was still protecting the list; it was not, and the claims are corrected. The module docstring now also states the two shapes the guard cannot see, so "every writable timestamp is reviewed" is not read as broader than it is: a serializer with no Meta.model has no column to read ownership from, and nested writable serializers are not traversed. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
…ss fields Copilot review found that three registry reasons described the wrong owner. Each of these is a command field: the client sends an instruction and the server derives the stored instant, so calling them client-set was wrong. - HealthIssueSerializer.snoozed_until takes only a relative duration such as "7d", and SnoozeDurationField.to_internal_value computes the instant. - AlertSerializer.snoozed_until takes a relative date string that update() resolves through relative_date_parse. - ActionSerializer.pinned_at is discarded on pin and replaced with the server's own now(); only the null that unpins comes from the client. They are now TODO entries marked server-derived. The API should not offer any of them as a writable timestamp, whatever the help text says. The same batched reason had been applied to four other snooze fields, and those were checked individually: the logs, vision, billing and ticket serializers all assign the client value directly, so their reasons stand. A writable DateTimeField whose source resolves to no model column was skipped in silence, which left a third hole in the "every writable timestamp is reviewed" guarantee beyond the two the docstring names. Such a field still takes a client value on a write route, so it is now collected and needs a reason. That surfaced three: the HogFunction optimistic-concurrency guard, and the batch import window. The BatchExportRunSerializer entries keep their place but not their reason. The runs viewset is read-only for CRUD, yet it carries POST cancel and retry actions, so the serializer is reached from a write route; those actions take no request body. Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
A discovery mechanism that finds nothing on every route is indistinguishable from one with nothing to find. The sweep keeps passing, the registry keeps matching, and the coverage quietly shrinks. Nothing in this file caught that. test_every_discovery_path_finds_something requires each mechanism to contribute at least one serializer across the whole repository. Discovery is now grouped by the mechanism that names each serializer rather than merged into one set, because a bucket holding several mechanisms stays non-empty while one of them is dead: killing the return resolution left the locally-bound imports to keep its bucket populated, and killing the serializer_class read left the lookup tables to keep its. Each of the five was disabled in turn and the test named exactly the one disabled. The five snooze reasons that shared one batched sentence now each state the mechanism that decides ownership, so a reader can check the claim by grepping for what it names instead of trusting the sentence. The claims themselves are unchanged and were verified individually in the previous commit; only the wording is now falsifiable. Logs and vision still share a reason because they share an implementation, which one grep confirms for both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 2dd6a39a-f92c-45a1-a73e-418b3b1e50b1
…s disabled A broken CDC source paused only its capture schedule, so each table's schedule kept starting runs that failed or hit the billing limit until someone repaired the source. Repair CDC already unpauses the table schedules. Disable CDC turned the tables off with a bulk update that skips the schedule pause, so the former CDC tables kept syncing, and billing, on their old schedule with no sync method. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…changes anything A pause that failed after the reset left the table syncing while a retry returned already_disabled. Pausing first fails the request while CDC is still on, so the retry repeats the whole disable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… change buffer Capture has no billing check, so while the limit blocks every load it keeps reading the slot and buffering changes that expire after 14 days. The slot sweeper now marks such a source broken once a table has been blocked longer than the buffer keeps files and the team is still over the limit. A PostHog-managed slot with auto-drop on is dropped and the schedules paused, as the critical-lag safety net does; any other slot is left to its owner and capture keeps advancing it. Repair CDC re-snapshots every table once the team is back under the limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d keep the sweep going on errors A table can go longer without a load for other reasons, so the time since its last load overstated how long billing had blocked it. The block now starts at the source's first billing-blocked job after its last job with another outcome. A failing billing check no longer ends the sweep; that source still gets its lag check and the next sweep retries it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and scope the billing block to the blocked tables Two review findings on the billing-expiry stop. drop_resources is best-effort: it logs a refused drop (an active slot, a missing grant) instead of raising. Marking the source broken on the strength of that call alone paused capture behind a live slot that nothing would advance, so the customer's WAL would grow. The stop now checks the slot is gone through a new adapter.slot_exists, and raises otherwise, which leaves the source running for the next sweep to retry. The billing block was measured from every job of the source. A sibling table that fails, or a non-billable run that skips the billing check and completes, reset the clock and could defer the stop indefinitely. The job history is now read for the blocked CDC tables only. Co-Authored-By: Claude <noreply@anthropic.com>
One window per source still let a completed job on one blocked table reset the clock for the others, so a table whose changes had already expired kept capturing. Each blocked table's run is now read from its own job history, and the source stops as soon as any of them is blocked past buffer retention. Co-Authored-By: Claude <noreply@anthropic.com>
Base PR #106917 was squash-merged. The conflicted files take master's version, because this branch never changed them itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… stop failed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 updated Run: 1b866784-4c88-4fe2-94e4-8af73246385f Co-authored-by: arthurdedeus <54866778+arthurdedeus@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…account editor Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…d clear kept-slot markers Capture records a Failed job on every CDC table when it fails, and those rows restarted a table's billing block, so a source whose capture failed now and then was never stopped. Only a table's own sync runs count now. A kept slot's billing_limit_expired marker never cleared, so its tables stayed Failed after they re-snapshotted on their own. The sweeper lifts it once the team is back under the limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Classify a raw 400 Bad Request from the Fillout API as non-retryable, matching the pattern every other REST-based source already uses for this status code. Fillout's `get_non_retryable_errors()` only listed 401 and 403, so a 400 fell through to the generic path and retried (and re-reported) for the whole activity budget instead of stopping after a bounded number of attempts. Generated-By: PostHog Desktop Task-Id: b61b7cd5-4a73-4882-bc19-87fe16afd9a7
The campaign and flow performance reports paginate over POST requests, which the shared HTTP session's automatic Retry-After-aware retry only covers for GET/HEAD/OPTIONS. A 429 on one of these reports fell back to blind exponential backoff (capped at 30s across 5 attempts), which can exhaust the retry budget before Klaviyo's own cooldown elapses. _fetch_page now reads the Retry-After header on a 429 and the retry's wait function uses it when present, capped at 120s, falling back to the existing exponential backoff otherwise. Generated-By: PostHog Desktop Task-Id: 906f64a9-7e63-429a-8f4a-35cb342c4c0d
Addresses CodeRabbit review on #107911: a negative delta-seconds value clamped to 0.0, which would drive instant retries against a still-active rate limit instead of falling back to exponential backoff. Also parameterizes the 429 propagation test to cover POST (the reporting endpoints' pagination method), not just GET. Generated-By: PostHog Desktop Task-Id: 906f64a9-7e63-429a-8f4a-35cb342c4c0d
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe changes add account editing and event-stream setup to customer analytics, including return navigation and save-state handling. CDC cleanup now checks billing-retention expiry and verifies slot drops. Information-schema queries support selected schema-split aliases, and the serializer timestamp guard requires explicit review records. Klaviyo retries honor valid Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Resolve the account data-loss window, Klaviyo retry delay, and information-schema lookup issues before merging. The remaining findings are narrower but should also be addressed. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Account edits can overwrite a concurrent change to email-matching settings, and a CDC source can be marked stopped before every cleanup or schedule-pause step succeeds. The affected operations are scoped to an account or CDC source, and the inspected server-side membership controls remain in place. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Description checkExplanation The description only documents Trunk Merge batching, included pull requests, dependencies, and the base commit. It does not provide the required Problem, Changes, testing, release status, notifications, docs update, or agent context sections. Resolution Replace or supplement the Trunk Merge text with a standalone description that follows the repository template. Explain the user problem, summarize observable and mechanical changes, list automated tests and unverified checks, select exactly one release-status option, record changelog and docs-update decisions, and complete or remove the agent context section as applicable.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/customerAnalyticsAccountSceneLogic.ts-441-446 (1)
441-446: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIgnore validation failures in
submitAccountFormFailure.An empty or oversized name fails
accountFormvalidation. WithenableFormOnSubmit, pressing Enter can dispatchsubmitAccountFormFailurewithout calling the form submit handler. The listener then shows a save-error toast and captures a validation error as an exception.🐛 Suggested fix
- submitAccountFormFailure: ({ error }) => { + submitAccountFormFailure: ({ error }) => { + if (values.accountFormHasErrors) { + return + } lemonToast.error("Couldn't save the account. Try again.")
🧹 Nitpick comments (4)
products/warehouse_sources/backend/temporal/data_imports/sources/klaviyo/test_klaviyo.py (1)
345-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a concrete type for the test payload.
The test creates a fixed JSON body, so
dict[str, Any]hides its shape. Usedict[str, object] | Noneforjson_body. As per coding guidelines, “Annotate every signature, avoidAny.”Source: Coding guidelines
posthog/hogql/database/schema/test/test_information_schema.py (1)
375-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate both new test signatures.
Add
-> Nonetotest_schema_split_filter_finds_qualified_tablesandtest_bare_system_table_name_without_schema_matches_nothing. As per coding guidelines, “Annotate every signature.”Also applies to: 381-381
Source: Coding guidelines
posthog/test/repo_invariants/test_serializer_timestamp_guard.py (2)
95-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClassify
snoozed_untilas reviewed client input.
HealthIssueSerializer.snoozed_untilis explicitly writable.SnoozeDurationFieldvalidates a bounded relative duration and converts it to the stored datetime. TheTODOprefix marks unresolved ownership, so it can prompt an incorrect read-only follow-up.Suggested fix
- "posthog.api.health_issue.HealthIssueSerializer.snoozed_until": "TODO: server-derived. SnoozeDurationField accepts only a relative duration such as '7d' and computes the stored instant", + "posthog.api.health_issue.HealthIssueSerializer.snoozed_until": "Client supplies a validated relative duration such as '7d'; SnoozeDurationField computes the stored instant",
499-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject blank review reasons.
The registry contract requires a human-written reason for every writable timestamp. The current assertion checks only the key, so an entry with an empty string passes. Add a check that rejects blank reasons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 0d50c8b6-cb3e-42f4-a095-a9057144285a
📒 Files selected for processing (42)
Dockerfiledocs/internal/cdc-buffered-ingress-runbook.mddocs/internal/customer-analytics/account-sidebar.mdfrontend/snapshots.ymlfrontend/src/lib/components/ActivityLog/activityLogLogic.test.setup.tsfrontend/src/products.tsxposthog/hogql/database/schema/information_schema.pyposthog/hogql/database/schema/test/test_information_schema.pyposthog/test/repo_invariants/test_serializer_timestamp_guard.pyproducts/customer_analytics/frontend/components/Accounts/AGENTS.mdproducts/customer_analytics/frontend/components/Accounts/accountEmailMatching.tsproducts/customer_analytics/frontend/components/Accounts/accountMeetingsLogic.tsproducts/customer_analytics/frontend/components/Accounts/constants.tsproducts/customer_analytics/frontend/components/EventStream/AccountEventStreamMembership.tsxproducts/customer_analytics/frontend/components/EventStream/AccountEventStreamSetupBanner.tsxproducts/customer_analytics/frontend/components/EventStream/AccountEventStreamToggle.tsxproducts/customer_analytics/frontend/components/EventStream/eventStreamLogic.tsproducts/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/AccountEditModal.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/AccountEventStreamModal.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/AccountSidebar.test.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/AccountSidebar.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/customerAnalyticsAccountSceneLogic.test.tsproducts/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/customerAnalyticsAccountSceneLogic.tsproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/CustomerAnalyticsConfigurationScene.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/customerAnalyticsConfigurationSceneLogic.tsproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/customerAnalyticsConfigurationSceneUtils.test.tsproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/customerAnalyticsConfigurationSceneUtils.tsproducts/customer_analytics/manifest.tsxproducts/tasks/backend/sandbox/images/Dockerfile.sandbox-baseproducts/warehouse_sources/backend/temporal/data_imports/cdc/activities.pyproducts/warehouse_sources/backend/temporal/data_imports/cdc/adapters.pyproducts/warehouse_sources/backend/temporal/data_imports/cdc/billing_expiry.pyproducts/warehouse_sources/backend/temporal/data_imports/cdc/broken.pyproducts/warehouse_sources/backend/temporal/data_imports/cdc/tests/test_cleanup_orphan_slots.pyproducts/warehouse_sources/backend/temporal/data_imports/cdc/tests/test_extract_activity.pyproducts/warehouse_sources/backend/temporal/data_imports/cdc/tests/test_metrics.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/fillout/source.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/fillout/tests/test_fillout_source.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/klaviyo/klaviyo.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/klaviyo/test_klaviyo.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/adapter.pyservices/mcp/src/templates/execute-sql-prompt.md
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.
| if not schemas: | ||
| return {} | ||
| visible = _visible_table_names(database) | ||
| bare_names = allowed.difference(visible) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle bare-name collisions across schemas. A visible view named insights blocks the system.insights alias and can itself match a bare-name-only query. The catalog lookup and prompt guidance must distinguish these cases.
posthog/hogql/database/schema/information_schema.py#L380-L380: determine ambiguity within the requested schema so a view in another schema does not suppress the system-table alias.services/mcp/src/templates/execute-sql-prompt.md#L25-L25: replace the unconditional “zero rows” claim with guidance to use a qualified name or explicit schema filter.
📍 Affects 2 files
posthog/hogql/database/schema/information_schema.py#L380-L380(this comment)services/mcp/src/templates/execute-sql-prompt.md#L25-L25
| table_label = "data_catalog_enriched_tables" if data_catalog_enrichment_requested else "tables" | ||
| if aliases: | ||
| # table_catalog and table_name both carry the table's name. | ||
| table_rows = _relabel_table_names(table_rows, aliases, (0, 2)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the queryable table identity in table_catalog.
For table_schema = 'public' AND table_name = 'ai_events', this relabels both fields to ai_events. The catalog’s only qualified identifier, posthog.ai_events, disappears from the result, while bare FROM ai_events does not resolve. Keep the qualified identifier in table_catalog and update its field description to distinguish it from the alias in table_name.
Comment on lines
+299
to
+332
| const projectId = String(props.projectId) | ||
| const openedValues = values.accountEditorOpenedValues | ||
| const changedPropertyKeys = ACCOUNT_ID_FIELDS.map(({ key }) => key).filter( | ||
| (key) => formValues[key] !== openedValues[key] | ||
| ) | ||
| const name = formValues.name.trim() | ||
| const currentAccount = await accountsRetrieve(projectId, values.account.id) | ||
| // Sending only edited fields keeps concurrent edits to the other fields. | ||
| const listCleaners = { email_domains: cleanDomains, known_emails: cleanEmails } | ||
| const cleanedLists = { | ||
| email_domains: cleanDomains(formValues.email_domains), | ||
| known_emails: cleanEmails(formValues.known_emails), | ||
| } | ||
| const changedListKeys = (['email_domains', 'known_emails'] as const).filter( | ||
| (key) => !objectsEqual(cleanedLists[key], listCleaners[key](openedValues[key])) | ||
| ) | ||
| const changedProperties = { | ||
| ...Object.fromEntries( | ||
| changedPropertyKeys | ||
| .filter( | ||
| (key) => key !== 'stripe_customer_id' || currentAccount.properties?.stripe_customer_id | ||
| ) | ||
| .map((key) => [key, formValues[key].trim() || null]) | ||
| ), | ||
| ...Object.fromEntries(changedListKeys.map((key) => [key, cleanedLists[key]])), | ||
| } | ||
| const updatedAccount = await accountsPartialUpdate(projectId, values.account.id, { | ||
| ...(name !== openedValues.name ? { name } : {}), | ||
| properties: { ...currentAccount.properties, ...changedProperties } as PatchedAccountApiProperties, | ||
| }) | ||
| actions.loadAccountSuccess(updatedAccount) | ||
| posthog.capture(AccountsEvents.AccountEdited, { | ||
| name_changed: name !== openedValues.name, | ||
| changed_fields: [...changedPropertyKeys, ...changedListKeys], |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '280,340p' products/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/customerAnalyticsAccountSceneLogic.ts
rg -n 'updateAccount|update.*account|patch.*account' products/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/customerAnalyticsAccountSceneLogic.tsRepository: PostHog/posthog
Length of output: 3720
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- imports and client references ---'
sed -n '1,80p' products/customer_analytics/frontend/scenes/CustomerAnalyticsAccountScene/customerAnalyticsAccountSceneLogic.ts
rg -n --glob '!node_modules' 'accountsPartialUpdate|accountsRetrieve' frontend products posthog | head -80
printf '%s\n' '--- account API declarations/implementations ---'
rg -n --glob '!node_modules' 'partial.*update|PartialUpdate|Account.*ViewSet|Account.*Serializer|accounts/.+properties|properties.*account' products/customer_analytics posthog frontend | head -160Repository: PostHog/posthog
Length of output: 41337
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- generated client and schema ---'
sed -n '670,750p' products/customer_analytics/frontend/generated/api.ts
sed -n '340,410p' products/customer_analytics/frontend/generated/api.zod.ts
rg -n --glob '!node_modules' --glob '!*.map' 'class .*Account|def partial_update|partial_update.*account|Accounts.*ViewSet|Account.*Serializer|account.*partial_update|accounts.*partial' products/customer_analytics posthog | head -240
printf '%s\n' '--- account source filenames ---'
fd -i 'account' products/customer_analytics | head -160Repository: PostHog/posthog
Length of output: 41233
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- route and serializer references ---'
rg -n --glob '*.py' --glob '*.yaml' --glob '*.yml' \
'api/projects/.+accounts|accounts/(?P|<|:)|AccountViewSet|PatchedAccount|AccountSerializer|AccountUpdate|properties.*validated_data|validated_data.*properties|partial_update' \
products/customer_analytics posthog | head -300
printf '%s\n' '--- product backend layout ---'
find products/customer_analytics/backend -maxdepth 3 -type f | sort | sed -n '1,220p'
printf '%s\n' '--- account route registration ---'
rg -n --glob '*.py' --glob '*.yaml' 'account_urls|include\(.*account|accounts.*router|router.*account|Account' products/customer_analytics/backend products/customer_analytics | head -220Repository: PostHog/posthog
Length of output: 41357
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- account symbols in presentation views ---'
rg -n -F 'Account' products/customer_analytics/backend/presentation/views/views.py products/customer_analytics/backend/presentation/views/serializers.py products/customer_analytics/backend/presentation/views/urls.py products/customer_analytics/backend/routes.py products/customer_analytics/backend/presentation/views/__init__.py
printf '%s\n' '--- properties handling in account API files ---'
rg -n -F 'properties' products/customer_analytics/backend/presentation/views/views.py products/customer_analytics/backend/presentation/views/serializers.py products/customer_analytics/backend/facade/api.py products/customer_analytics/backend/models/account.py
printf '%s\n' '--- relevant view source ---'
sed -n '1,260p' products/customer_analytics/backend/presentation/views/views.py
printf '%s\n' '--- relevant serializers source ---'
sed -n '1,260p' products/customer_analytics/backend/presentation/views/serializers.py
printf '%s\n' '--- URL source ---'
cat -n products/customer_analytics/backend/presentation/views/urls.pyRepository: PostHog/posthog
Length of output: 42014
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- account viewset update path ---'
sed -n '1700,2160p' products/customer_analytics/backend/presentation/views/views.py
printf '%s\n' '--- account serializer declarations and validation ---'
rg -n 'class AccountSerializer|def validate_properties|properties_provided|class AccountPropertiesField' products/customer_analytics/backend/presentation/views/serializers.py products/customer_analytics/backend/presentation/views/views.py
sed -n '980,1100p' products/customer_analytics/backend/presentation/views/serializers.py
printf '%s\n' '--- facade account update definitions ---'
rg -n 'def (create|update)_account|UpdateAccountInput|properties_provided|AccountPropertiesValidationError|_properties|properties =' products/customer_analytics/backend/facade products/customer_analytics/backend/models/account.pyRepository: PostHog/posthog
Length of output: 40106
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- model property accessors ---'
sed -n '70,112p' products/customer_analytics/backend/models/account.py
printf '%s\n' '--- facade update implementation ---'
sed -n '3478,3550p' products/customer_analytics/backend/facade/api.py
sed -n '3590,3675p' products/customer_analytics/backend/facade/api.py
printf '%s\n' '--- update input contract ---'
sed -n '1198,1240p' products/customer_analytics/backend/facade/contracts.pyRepository: PostHog/posthog
Length of output: 10200
Preserve concurrent account properties during save.
If another client changes a property after accountsRetrieve returns but before accountsPartialUpdate, this flow sends the stale value from currentAccount.properties. AccountViewSet.partial_update passes that object to update_account, which replaces _properties wholesale. Saving one field can therefore revert a later unrelated change.
Use a property-patch contract that atomically applies only changedProperties to the latest stored properties. Do not send the fetched full snapshot to this replacement API.
Comment on lines
+31
to
+35
| # Cap how long a single in-function retry waits so a long fixed-window Retry-After doesn't pin a | ||
| # worker thread; if the window is longer, the attempts exhaust and Temporal retries the whole | ||
| # activity later from saved page state. | ||
| MAX_RETRY_AFTER_SECONDS = 120 | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'def make_tracked_session|make_tracked_session\s*=' products posthog commonRepository: PostHog/posthog
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- transport outline ---'
ast-grep outline products/warehouse_sources/backend/temporal/data_imports/sources/common/http/transport.py
printf '%s\n' '--- transport factory and adapter definitions ---'
sed -n '1,260p' products/warehouse_sources/backend/temporal/data_imports/sources/common/http/transport.py
printf '%s\n' '--- Klaviyo relevant definitions and call path ---'
ast-grep outline products/warehouse_sources/backend/temporal/data_imports/sources/klaviyo/klaviyo.py
sed -n '1,180p' products/warehouse_sources/backend/temporal/data_imports/sources/klaviyo/klaviyo.py
printf '%s\n' '--- retry/session usages ---'
rg -n -C 4 'make_tracked_session|HTTPAdapter|Retry\(' products/warehouse_sources/backend/temporal/data_imports posthog commonRepository: PostHog/posthog
Length of output: 45644
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Klaviyo retry and fetch path ---'
sed -n '280,380p' products/warehouse_sources/backend/temporal/data_imports/sources/klaviyo/klaviyo.py
printf '%s\n' '--- Klaviyo session construction and callers ---'
rg -n -C 8 'make_tracked_session|_fetch_page\(' products/warehouse_sources/backend/temporal/data_imports/sources/klaviyo/klaviyo.py
printf '%s\n' '--- urllib3 dependency declarations ---'
rg -n -i 'urllib3|requests' pyproject.toml uv.lock requirements*.txt products/warehouse_sources/pyproject.toml 2>/dev/null || true
printf '%s\n' '--- installed urllib3 retry contract ---'
python3 - <<'PY'
import inspect
from urllib3.util.retry import Retry
print("urllib3 module:", Retry.__module__)
print("DEFAULT_BACKOFF_MAX:", Retry.DEFAULT_BACKOFF_MAX)
print("respect_retry_after_header default:", inspect.signature(Retry).parameters["respect_retry_after_header"].default)
print(inspect.getsource(Retry.sleep))
print(inspect.getsource(Retry.sleep_for_retry))
PYRepository: PostHog/posthog
Length of output: 22560
Disable adapter retries for the Klaviyo source session.
make_tracked_session() applies DEFAULT_RETRY when no policy is passed. That policy retries GET requests with status 429 three times and honors Retry-After. BoundedRetry caps each transport wait at 120 seconds, but session.get() can perform multiple such waits before _fetch_page() applies its Tenacity cap.
Suggested fix
from tenacity import RetryCallState, retry, retry_if_exception_type, stop_after_attempt, wait_exponential_jitter
+from urllib3.util.retry import Retry
...
- session = make_tracked_session()
+ session = make_tracked_session(retry=Retry(total=0))
trunk-io
Bot
deleted the
trunk-merge/pr-107852/9584165b-5116-41fc-b89e-1b471442e0bc
branch
September 28, 2026 19:28
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request was created and is being managed by Trunk Merge.
This pull request is based on the master branch at SHA 40eeb716559d19325eb82b381663dd07ba9c8020.
See more details about each PR in the batch here:
When CI completes, this pull request will be closed automatically.
Pull Requests Being Tested
This pull request is testing a batch with the changes from pull requests 107852, 107738, 107911, and 107338 - batching documentation.
Dependencies
This pull request depends on the changes from pull requests 107910, 107934, 106952, 94686, and 104080.