trunk-merge/pr-107620/99443364-8fe2-496e-b447-5610198465e4 - #107626
trunk-io[bot] wants to merge 289 commits into
Conversation
…from the proposals api
…he workflow history
A config value and a subsection name resolve escapes differently. A value keeps the escaped character of \" and \\; a subsection name does the same, but it also drops a backslash that precedes anything else. Git therefore reads [remote "ori\gin"] as the remote origin and [remote "tab\there"] as tabthere. I checked both against real Git. The header decoder kept that backslash, so the name did not match origin and an earlier remote, such as upstream, won the release instead. The subsection name now has its own decoder. Two cases cover it: the name a dropped escape produces, and the remote selection that follows from it. Both fail without the change. Generated-By: PostHog Desktop Task-Id: 7c27427d-8fcc-4bc7-b7a2-0946a5a9538f
…-templates-ai-first-new
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe pull request adds HogQL catalog traversal metadata, configurable calendar sync intervals, and AI-first workflow and email-template creation flows. It also updates sandbox request validation, MCP loop-tool filtering, Fleetio imports, template edit notifications, and the CLI release version and changelog. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Fleetio imports of service entry line items can fail when a service entry is deleted during a sync. Resolve that before merging. A malformed sandbox URL can also cause a server error instead of a clear rejection. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new flows have meaningful security boundaries. The direct sandbox route has restrictive origin and redirect controls, but the new email preview can display saved HTML whose external-resource behavior is not fully established. Calendar configuration also depends on the administrator check applying to the same project being updated. 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 explains the Trunk Merge test setup, base commit, tested pull request, and dependencies, but it does not follow the repository template. It omits the Problem, Changes, testing details, Release status, notifications, Docs update, and Agent context sections. Resolution Replace or supplement the Trunk Merge text with a completed repository-template description. State the user-facing problem, observable changes, automated tests and untested areas, select exactly one release-status option, record changelog and docs decisions, and complete Agent context or remove it if no agent authored the change.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (7)
posthog/hogql/catalog_traversal.py-186-204 (1)
186-204: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winCache the
Noneresult for unserializable canonical tables.When
serialize_fieldsraises,_canonical_namereturnsNonewithout writing tocanonical_name_cache. Every later call for the same table runs the full serialization again and incrementscanonical_unserializableagain._resolve_chainat Line 247 can call_canonical_nameonce per chain step, so a single broken table can cost many repeated serializations. The repeated counts also inflate the aggregate warning.Proposed fix
except Exception: self.omissions["canonical_unserializable"] += 1 + self.canonical_name_cache[id(target)] = None return Noneproducts/customer_analytics/backend/facade/api.py-4269-4279 (1)
4269-4279: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate
interval_minutesin the write path.
update_calendar_sync_intervalstores any integer without checking it.get_calendar_sync_intervalsilently maps values outsideALLOWED_SYNC_INTERVALSback to 60. Any caller that bypasses the serializer can therefore persist a value that the API reports as 60, while the coordinator reads the stored value. Reject values outsideALLOWED_SYNC_INTERVALShere, or confirm that the coordinator usesget_calendar_sync_interval.Proposed fix
from products.customer_analytics.backend.logic.calendar_sync import ( # noqa: PLC0415 + ALLOWED_SYNC_INTERVALS, SYNC_INTERVAL_CONFIG_KEY, update_calendar_sync_config, ) + if interval_minutes not in ALLOWED_SYNC_INTERVALS: + raise ValueError("unsupported calendar sync interval") try:docs/internal/hogql-language-service.md-168-168 (1)
168-168: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHyphenate the compound adjective.
Change "feature flagged" to "feature-flagged".
Source: Linters/SAST tools
products/customer_analytics/backend/logic/calendar_sync.py-148-150 (1)
148-150: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA failure marker can mask the original exception.
mark_calendar_sync_failedraisesIntegration.DoesNotExistif the integration is deleted during the sync. In that case, the new exception replaces the original error. The temporal layer then cannot classify the original error, such asCalendarSyncErrororGoogleWorkspaceEgressBudgetExhausted. Catch errors from the marker so that the original exception is re-raised.Fix
except Exception: - mark_calendar_sync_failed(integration_id, team_id) + try: + mark_calendar_sync_failed(integration_id, team_id) + except Integration.DoesNotExist: + pass raiseproducts/tasks/backend/presentation/views/api.py-3469-3469 (1)
3469-3469: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject invalid Hogland ports instead of raising.
If
connection.sandbox_urlcontains a nonnumeric or out-of-range port, the delegated check raisesValueErrorwhen it readstarget.port. The command endpoint calls this check before its forwardingtryblock, so the request returns a server error instead of the intended 400. Catch invalid-port errors insideis_hogland_sandbox_urland returnFalse. Python 3.13 documents thisportbehavior. (docs.python.org)products/workflows/frontend/Workflows/newWorkflowHandoff.ts-19-19 (1)
19-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the missing project ID.
String(projectLogic.findMounted()?.values.currentProjectId)becomes"undefined"or"null"whenprojectLogicis unmounted or the project has not loaded. The request then goes to/api/projects/undefined/hog_flows/. That request fails, and the handoff shows the "could not be opened" toast. Returnnullearly when the ID is missing.Proposed fix
- const projectId = String(projectLogic.findMounted()?.values.currentProjectId) + const currentProjectId = projectLogic.findMounted()?.values.currentProjectId + if (currentProjectId == null) { + return null + } + const projectId = String(currentProjectId)products/warehouse_sources/backend/temporal/data_imports/sources/COVERAGE_GAPS_APPENDIX.md-3227-3227 (1)
3227-3227: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the Fleetio inventory when marking gaps complete.
These checked entries add five streams, but the “Today (9)” line still lists only the original nine. Change that line to “Today (14)” and include the five new stream names.
🧹 Nitpick comments (2)
products/customer_analytics/backend/presentation/views/serializers.py (1)
1488-1495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared constant for allowed intervals.
The literal
(5, 15, 30, 60)repeatsALLOWED_SYNC_INTERVALSfromlogic/calendar_sync.py. If the two lists diverge, the API can accept values thatget_calendar_sync_intervalthen silently maps to 60. Presentation cannot importlogicdirectly. Expose the tuple through the facade, then use aChoiceFieldor validate against that tuple.products/customer_analytics/backend/temporal/calendar_sync.py (1)
122-143: 🚀 Performance & Scalability | 🔵 TrivialThe coordinator loads every Google Calendar config every 5 minutes.
The collector filters in Python. Each run iterates over all
google-calendarintegrations and reads the completeconfigJSON for each one. The run frequency increased from hourly to every 5 minutes, which multiplies this full scan by 12. This load is acceptable at the current scale. As the number of integrations grows, move the due-time filter into SQL or storenext_sync_atin an indexed column.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 26017d25-9452-4cfe-85d2-cd042c65f9b8
⛔ Files ignored due to path filters (4)
cli/Cargo.lockis excluded by!**/*.lockproducts/customer_analytics/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/customer_analytics/frontend/generated/api.tsis excluded by!**/generated/**products/customer_analytics/frontend/generated/api.zod.tsis excluded by!**/generated/**
📒 Files selected for processing (76)
cli/.sampo/changesets/event-hash-ignores-chunk-file-names.mdcli/.sampo/changesets/section-aware-git-config.mdcli/CHANGELOG.mdcli/Cargo.tomldocs/internal/hogql-language-service.mdfrontend/src/lib/components/EmailPreviewThumbnail/EmailPreviewThumbnail.test.tsxfrontend/src/lib/components/EmailPreviewThumbnail/EmailPreviewThumbnail.tsxfrontend/src/lib/constants.tsxfrontend/src/scenes/max/aiFirstCreate/AiFirstCreateScene.tsxfrontend/src/scenes/max/aiFirstCreate/aiFirstHandoffLogic.test.tsfrontend/src/scenes/max/aiFirstCreate/aiFirstHandoffLogic.tsfrontend/src/scenes/max/aiFirstCreate/aiFirstMode.tsposthog/api/services/query.pyposthog/api/test/test_query_service.pyposthog/hogql/catalog_traversal.pyposthog/hogql/language_service.pyposthog/hogql/test/test_language_service.pyproducts/customer_analytics/backend/facade/api.pyproducts/customer_analytics/backend/facade/contracts.pyproducts/customer_analytics/backend/logic/calendar_sync.pyproducts/customer_analytics/backend/presentation/views/serializers.pyproducts/customer_analytics/backend/presentation/views/views.pyproducts/customer_analytics/backend/temporal/calendar_sync.pyproducts/customer_analytics/backend/test/test_calendar_sync.pyproducts/customer_analytics/backend/test/test_views.pyproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/calendar/CalendarSyncConfig.tsxproducts/customer_analytics/frontend/scenes/CustomerAnalyticsConfigurationScene/calendar/calendarSyncLogic.tsproducts/customer_analytics/mcp/tools.yamlproducts/messaging/backend/api/message_templates.pyproducts/messaging/backend/api/test/test_message_templates.pyproducts/posthog_ai/frontend/api/logics.tsproducts/posthog_ai/frontend/api/tools.tsproducts/posthog_ai/frontend/components/composer/AttachedContextBar.test.tsxproducts/posthog_ai/frontend/components/composer/AttachedContextBar.tsxproducts/posthog_ai/frontend/components/tool/widgets/CreateNotebookWidget.tsxproducts/posthog_ai/frontend/components/tool/widgets/extractors.tsproducts/posthog_ai/frontend/types/contextTypes.tsproducts/posthog_ai/frontend/utils/getToolOutputRecord.tsproducts/tasks/backend/facade/api.pyproducts/tasks/backend/presentation/views/api.pyproducts/tasks/backend/tests/test_api.pyproducts/tasks/backend/tests/test_sandbox_url_validation.pyproducts/tasks/mcp/tools.yamlproducts/warehouse_sources/backend/temporal/data_imports/sources/COVERAGE_GAPS_APPENDIX.mdproducts/warehouse_sources/backend/temporal/data_imports/sources/fleetio/canonical_descriptions.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/fleetio/fleetio.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/fleetio/settings.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/fleetio/tests/test_fleetio.pyproducts/workflows/frontend/MessagingTabActions.tsxproducts/workflows/frontend/TemplateLibrary/MessageTemplate.tsxproducts/workflows/frontend/TemplateLibrary/MessageTemplatesTable.tsxproducts/workflows/frontend/TemplateLibrary/NewTemplateAgent.tsxproducts/workflows/frontend/TemplateLibrary/TemplateStartingPointCard.tsxproducts/workflows/frontend/TemplateLibrary/messageTemplateLogic.test.tsproducts/workflows/frontend/TemplateLibrary/messageTemplateLogic.tsproducts/workflows/frontend/TemplateLibrary/newTemplateAgentLogic.test.tsproducts/workflows/frontend/TemplateLibrary/newTemplateAgentLogic.tsproducts/workflows/frontend/TemplateLibrary/newTemplateHandoff.test.tsproducts/workflows/frontend/TemplateLibrary/newTemplateHandoff.tsproducts/workflows/frontend/TemplateLibrary/templateAgentContext.test.tsproducts/workflows/frontend/TemplateLibrary/templateAgentContext.tsproducts/workflows/frontend/Workflows/NewWorkflowAgent.tsxproducts/workflows/frontend/Workflows/WorkflowScene.tsxproducts/workflows/frontend/Workflows/newWorkflowAgentLogic.tsproducts/workflows/frontend/Workflows/newWorkflowHandoff.test.tsproducts/workflows/frontend/Workflows/newWorkflowHandoff.tsproducts/workflows/frontend/Workflows/newWorkflowLogic.tsproducts/workflows/frontend/Workflows/workflowAgentContext.tsservices/mcp/schema/generated-tool-definitions.jsonservices/mcp/schema/tool-definitions-all.jsonservices/mcp/schema/tool-definitions.jsonservices/mcp/scripts/generate-tools.tsservices/mcp/scripts/yaml-config-schema.tsservices/mcp/src/api/generated.tsservices/mcp/tests/unit/tool-filtering.test.tstach.toml
💤 Files with no reviewable changes (3)
- cli/.sampo/changesets/section-aware-git-config.md
- cli/.sampo/changesets/event-hash-ignores-chunk-file-names.md
- products/workflows/frontend/Workflows/newWorkflowAgentLogic.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.
| ) | ||
| @patch("products.tasks.backend.presentation.views.api.internal_requests_session") | ||
| @patch("products.tasks.backend.presentation.views.api.http_requests.post") | ||
| def test_command_to_hogland_sandbox_bypasses_egress_proxy(self, mock_post, mock_session_factory): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Annotate the new test signatures. The four new test methods omit annotations required by the Python guideline.
products/tasks/backend/tests/test_api.py#L14299-L14299: annotate the mock parameters andNonereturn type.products/tasks/backend/tests/test_api.py#L14326-L14326: add theNonereturn type.products/tasks/backend/tests/test_api.py#L14349-L14349: annotate the mock parameters andNonereturn type.products/tasks/backend/tests/test_sandbox_url_validation.py#L44-L44: annotateurl,expected, and theNonereturn type.
As per coding guidelines: “Annotate every signature.”
📍 Affects 2 files
products/tasks/backend/tests/test_api.py#L14299-L14299(this comment)products/tasks/backend/tests/test_api.py#L14326-L14326products/tasks/backend/tests/test_api.py#L14349-L14349products/tasks/backend/tests/test_sandbox_url_validation.py#L44-L44
Source: Coding guidelines
| path_version="v2", | ||
| incremental_fields=[], | ||
| primary_keys=["service_entry_id", "id"], | ||
| fanout=DependentEndpointConfig( |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect the fan-out response-action contract without running repository code.
ast-grep outline products/warehouse_sources/backend/temporal/data_imports/sources/common/rest_source/fanout.py --match 'DependentEndpointConfig|build_dependent_resource'
rg -n -C 5 'child_response_actions|response_actions|status_code|404' products/warehouse_sources/backend/temporal/data_imports/sources/common/rest_sourceRepository: PostHog/posthog
Length of output: 42004
Configure 404 handling for Fleetio child requests.
When Fleetio deletes a service entry after the parent list request, the child request can return 404. This fan-out does not configure child_response_actions, so the 404 is not ignored on the API-parent path and can fail the import. Configure a child action that ignores only 404 responses.
| session.send.side_effect = responses | ||
| return urls | ||
|
|
||
| def _line_items(self, manager: mock.MagicMock, **kwargs: Any): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Annotate the new test signatures.
Add a SourceResponse return type to _line_items. Annotate the patched MockSession parameters in the three new tests. As per coding guidelines, “Annotate every signature.”
Also applies to: 325-325, 356-358, 368-368
Source: Coding guidelines
|
|
14 participants
This pull request was created and is being managed by Trunk Merge.
This pull request is based on the master branch at SHA 69a4765903776a5e70d4633590673395b1eaec2f.
See more details here.
When CI completes, this pull request will be closed automatically.
Pull Requests Being Tested
This pull request is testing the changes from pull request 107620.
Dependencies
This pull request depends on the changes from pull requests 107389, 101857, 107409, 102336, 107079, and 104978.