Repository navigation
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 |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Limit details: You’ve used all 12 included reviews currently available. 📝 WalkthroughWalkthroughWhen a HogFlow is created, the save signal schedules a team action report to run after the transaction commits. The report includes the workflow ID, origin product, and action count. Reporting errors are logged and suppressed. Tests cover deferred reporting, captured creation values, reporting failures, and rolled-back transactions. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Workflow creation reporting is best-effort after commit. The repository’s configured and inspected runtime paths do not establish a non-default database trigger, so no actionable merge-blocking risk remains. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new reporting step uses the workflow’s team identity and sends limited metadata through the existing analytics path. Reporting failures do not undo saved workflows. Delivery remains best effort, and the reviewed path showed no expansion of access privileges. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate 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: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/workflows/backend/models/hog_flow/hog_flow.py-242-242 (1)
242-242: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBind workflow creation reporting to the save database.
When
HogFlowis saved on a non-default database, this callback can run before that transaction commits and report a creation that later rolls back. The team lookup also uses the default database. Pass the signal’susingalias to both operations.🐛 Suggested fix
-def report_workflow_created(team_id: int, workflow_id: str, origin_product: str | None, actions_count: int) -> None: +def report_workflow_created( + team_id: int, workflow_id: str, origin_product: str | None, actions_count: int, using: str +) -> None: ... - team = Team.objects.get(id=team_id) + team = Team.objects.using(using).get(id=team_id) ... -def hog_flow_saved(sender, instance: HogFlow, created, update_fields=None, **kwargs): +def hog_flow_saved(sender, instance: HogFlow, created, using, update_fields=None, **kwargs): ... - transaction.on_commit(lambda: report_workflow_created(team_id, workflow_id, origin_product, actions_count)) + transaction.on_commit( + lambda: report_workflow_created(team_id, workflow_id, origin_product, actions_count, using), + using=using, + )
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
46566e06-7ce2-4161-8126-627be6b6c069
📒 Files selected for processing (2)
products/workflows/backend/models/hog_flow/hog_flow.pyproducts/workflows/backend/test/test_hog_flow.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.
|
Note 🤖 GPT-6.1 Sol responding on behalf of Michael @coderabbitai No current workflow creation writes to an alternate database: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n -C3 --type py 'class ReplicaRouter\b|def db_for_write\b' .
ast-grep outline products/workflows/backend/models/hog_flow/hog_flow.py --match 'hog_flow_saved|report_workflow_created' --view expanded
rg -n -C3 --type py '\.using\s*\(' products/workflowsLength of output: 4700 🏁 Script executed: #!/bin/bash
ast-grep outline posthog/dbrouter.py --match ReplicaRouter --view expanded
sed -n '30,42p' posthog/dbrouter.py
sed -n '215,270p' products/workflows/backend/models/hog_flow/hog_flow.pyLength of output: 3512
Thanks for the clarification. ✏️ Learnings added
You are interacting with an AI system. |
|
Risk: No findings This increment adds a capability-gated Kafka replay worker that rebuilds and re-queues invocations from the internal DLQ topic, refactors the workflow-proposal views onto team-scoped facade functions, and adds a post-commit analytics event on workflow creation. I traced every read and write path in the refactor and the replay worker and found no new security issues: proposal lookups, locks, and resolves all filter on team_id, and the replay worker routes customer bytes through the same masking, quota, and team-resolution guards as normal pipeline processing. Sentinel reviewed |
1089e2f to
8eea816
Compare
8eea816 to
7dbc048
Compare
|
Note 🤖 GPT-6.1 Sol responding on behalf of Michael Replaced by the draft fork PR #358 in its native fork stack. @meikelmosby will start migrating the stack from the fork into PostHog/posthog for upstream review and CI. Closing this master-targeted copy to keep the handoff clear. |
Problem
The Workflows team cannot build a team activation funnel from every saved workflow.
Note
Merge risk: low. Analytics runs after commit and contains failures; each creation adds one team lookup.
Changes
workflows workflow created.The change affects analytics only. The UI stays the same.
Before:
flowchart LR A[Create workflow] --> B[Persist workflow] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class A phYellow; class B phGray;After:
flowchart LR A[Create workflow] --> B[Persist workflow] --> C[Commit] --> D[Team creation event] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class A,D phYellow; class B phGray; class C phBlue;How did you test this code?
Test rationale: Model tests catch duplicate events, changed metadata, incorrect groups, rollback reporting and analytics failures.
The existing receiver tests cover worker reloads, so analytics needs assertions at the persisted-model seam.
Recorded local verification
.codex/with-flox hogli test products/workflows/backend/test/test_hog_flow.py -qHosted backend CI needs maintainer approval. Live analytics delivery remains unchecked.
Release status
Automatic notifications
Docs update
No existing document covers these analytics events.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: T3 Code / Codex, GPT-6.1 Sol.
Skills:
tdd,writing-tests,hogli,django-startup-time,writing-pr-descriptions,writing-voice,running-ci-preflight,reviewing-with-coderabbit,qa-swarm,merge-risk,unslop.QA strengthened the metadata, group and failure tests. CodeRabbit's import finding prompted the module-level import.
Local CodeRabbit skipped review because its CLI was signed out.