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 |
Gilbert09
added
the
stamphog
Talyn App
|
Hey @Gilbert09! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_import_data.py:139 |
products/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_import_data.py:263 |
49 | 185 |
products/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_import_data.py:150 |
products/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_import_data.py:357 |
37 | 152 |
products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v2/pipeline.py:100 |
products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/pipeline.py:138 |
32 | 140 |
✅ 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 — 5% of added code lines are comments (23 of 492)
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 |
|---|---|---|
products/warehouse_sources/backend/models/external_data_schema.py |
5 | 30 |
products/warehouse_sources/backend/temporal/data_imports/sources/common/cursor.py |
5 | 106 |
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/xmin_cursor.py |
4 | 20 |
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/source.py |
3 | 16 |
products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/pipeline.py |
2 | 21 |
products/warehouse_sources/backend/temporal/data_imports/sources/common/typings.py |
2 | 9 |
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/postgres.py |
1 | 19 |
products/warehouse_sources/backend/temporal/data_imports/workflow_activities/import_data_sync.py |
1 | 13 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Backend coverage — 97.0% of changed backend lines covered — 6 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 97.0% (273 / 279)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/common/typings.py |
66.7% | 37, 40 |
products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v2/pipeline.py |
75.0% | 94 |
products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/pipeline.py |
80.0% | 128 |
products/warehouse_sources/backend/temporal/data_imports/sources/common/cursor.py |
97.0% | 79, 99 |
🤖 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 36591553337 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 52.8% |
1,411 / 2,673 |
batch_exports |
████████████████░░░░ 81.2% |
21,528 / 26,502 |
cdp |
██████████████████░░ 88.2% |
4,545 / 5,155 |
mcp_analytics |
██████████████████░░ 89.2% |
5,038 / 5,651 |
product_tours |
██████████████████░░ 89.3% |
1,331 / 1,491 |
dashboards |
██████████████████░░ 89.5% |
6,904 / 7,714 |
data_warehouse |
██████████████████░░ 90.0% |
14,027 / 15,589 |
notebooks |
██████████████████░░ 90.2% |
15,287 / 16,945 |
signals |
██████████████████░░ 90.2% |
57,816 / 64,069 |
cohorts |
██████████████████░░ 90.4% |
8,420 / 9,316 |
streamlit_apps |
██████████████████░░ 90.6% |
2,623 / 2,895 |
managed_warehouse |
██████████████████░░ 91.0% |
10,252 / 11,263 |
tasks |
██████████████████░░ 91.1% |
75,146 / 82,503 |
data_modeling |
██████████████████░░ 91.4% |
10,554 / 11,543 |
exports |
██████████████████░░ 91.6% |
9,680 / 10,562 |
engineering_analytics |
██████████████████░░ 91.7% |
11,032 / 12,030 |
business_knowledge |
██████████████████░░ 92.2% |
7,684 / 8,330 |
conversations |
███████████████████░ 92.5% |
28,734 / 31,062 |
early_access_features |
███████████████████░ 92.6% |
1,332 / 1,439 |
stamphog |
███████████████████░ 92.8% |
8,109 / 8,742 |
canvas |
███████████████████░ 92.8% |
6,873 / 7,405 |
approvals |
███████████████████░ 93.0% |
3,974 / 4,271 |
mcp_registry |
███████████████████░ 93.1% |
1,670 / 1,794 |
notifications |
███████████████████░ 93.2% |
1,145 / 1,229 |
slack_app |
███████████████████░ 93.2% |
13,733 / 14,735 |
error_tracking |
███████████████████░ 93.2% |
16,359 / 17,547 |
surveys |
███████████████████░ 93.3% |
6,571 / 7,040 |
autoresearch |
███████████████████░ 93.6% |
8,481 / 9,061 |
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 |
ai_observability |
███████████████████░ 94.5% |
24,473 / 25,896 |
workflows |
███████████████████░ 94.6% |
15,222 / 16,093 |
wizard |
███████████████████░ 94.7% |
6,150 / 6,496 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
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,081 / 26,352 |
marketing_analytics |
███████████████████░ 95.3% |
19,566 / 20,528 |
posthog_ai |
███████████████████░ 95.3% |
2,488 / 2,610 |
experiments |
███████████████████░ 95.4% |
32,457 / 34,020 |
actions |
███████████████████░ 95.5% |
756 / 792 |
logs |
███████████████████░ 95.5% |
15,399 / 16,130 |
data_catalog |
███████████████████░ 95.5% |
4,401 / 4,606 |
tracing |
███████████████████░ 95.6% |
3,536 / 3,699 |
replay_vision |
███████████████████░ 95.6% |
27,675 / 28,939 |
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,113 / 7,386 |
revenue_analytics |
███████████████████░ 96.4% |
1,876 / 1,946 |
user_interviews |
███████████████████░ 96.5% |
2,867 / 2,971 |
feature_flags |
███████████████████░ 96.5% |
25,588 / 26,509 |
warehouse_sources |
███████████████████░ 97.2% |
459,764 / 472,790 |
data_quality |
████████████████████ 97.6% |
7,587 / 7,774 |
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 |
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.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
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.
Stamphog refused this pull request at the prerequisites gate, because the branch has merge conflicts. The other gates (deny-list, size, and tier) passed, so the conflicts are the only reason for the refusal.
To move forward, rebase or merge the base branch into this one, resolve the conflicts, and push the result. If you'd like a human to look at it in the meantime, ask a reviewer directly.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✗ | merge conflicts present |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 425L, 12F substantive, 1 binary; 855L/22F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (855L, 22F, two-areas, feat) |
| stamphog 2.3.0 | .stamphog/policy.yml @ unknown · reviewed head fdf37cc |
Rebase the source cursor implementation onto current master, preserve keyset resume behavior, and package the common source tests to avoid pytest module collisions.
fdf37cc to
0cd5d73
Compare
test_source_cursor.py already holds the same tests, and common/test/__init__.py keeps its module name clear of the Cursor source's test_cursor.py.
fuziontech
left a comment
fuziontech
left a comment
There was a problem hiding this comment.
Automated review generated on behalf of @fuziontech.
APPROVE. I did not find a blocking correctness, security, data-loss, or outage issue in the diff.
Non-blocking follow-ups:
test_cursor.pyandtest_source_cursor.pyare byte-for-byte duplicates; keep one to avoid maintaining the same suite twice.- The generic staged-cursor promotion replaces the current stored cursor without a second source-specific merge. If overlapping runs can promote out of order, consider a monotonic merge at promotion time to prevent an older cursor from moving the watermark backward. For xmin this appears recoverable as rereading, so it is not a release gate.
Focused tests could not run in this environment because hogli and pytest are unavailable.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe change introduces a shared source-cursor manager and adds cursor loading, staging, and persistence to warehouse imports. PostgreSQL XMIN state moves from dedicated schema fields to a serialized Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 489d1e3c-19fe-404e-96a4-28f4d85786aa
📒 Files selected for processing (21)
.agents/skills/implementing-warehouse-sources/SKILL.mdproducts/warehouse_sources/backend/management/commands/reset_wrapped_xmin_cursors.pyproducts/warehouse_sources/backend/models/external_data_schema.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/common/extract.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v2/pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/lanes.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/test_pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/test/__init__.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/test/test_source_cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/typings.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/postgres.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/source.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/test_postgres.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/xmin_cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_end_to_end.pyproducts/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_import_data.pyproducts/warehouse_sources/backend/temporal/data_imports/workflow_activities/import_data_sync.pyproducts/warehouse_sources/backend/tests/management/test_reset_wrapped_xmin_cursors.pyproducts/warehouse_sources/backend/tests/test_models.py
💤 Files with no reviewable changes (1)
- products/warehouse_sources/backend/temporal/data_imports/sources/postgres/test_postgres.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/xmin_cursor.py-24-28 (1)
24-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve partial legacy xmin state.
The field mapping is correct, but
xmin_cursor_from_legacyrejects legacy state that the previous path accepted. The previous_capture_xmin_ceilingtreated a missingxmin_num_wraparoundas epoch0. The current parser returns no cursor, andPostgresSourcethen performs a full scan.Keep
xmin_last_valueas the required field. Default the epoch to0and derive a missingceiling_xid8.Suggested fix
def xmin_cursor_from_legacy(sync_type_config: Mapping[str, Any]) -> XminCursor | None: ceiling_xid, ceiling_xid8, num_wraparound = (sync_type_config.get(key) for key in XMIN_LEGACY_KEYS) - if not isinstance(ceiling_xid, int) or not isinstance(ceiling_xid8, int) or not isinstance(num_wraparound, int): + if not isinstance(ceiling_xid, int): return None + if not isinstance(num_wraparound, int): + num_wraparound = 0 + if not isinstance(ceiling_xid8, int): + ceiling_xid8 = (num_wraparound << 32) | ceiling_xid return XminCursor(ceiling_xid=ceiling_xid, ceiling_xid8=ceiling_xid8, num_wraparound=num_wraparound)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 57fba4a1-c9ae-4312-b3a3-2d06824c745f
📒 Files selected for processing (21)
.agents/skills/implementing-warehouse-sources/SKILL.mdproducts/warehouse_sources/backend/management/commands/reset_wrapped_xmin_cursors.pyproducts/warehouse_sources/backend/models/external_data_schema.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/common/extract.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v2/pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/lanes.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/test_pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/test/__init__.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/test/test_source_cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/typings.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/postgres.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/source.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/test_postgres.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/xmin_cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_end_to_end.pyproducts/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_import_data.pyproducts/warehouse_sources/backend/temporal/data_imports/workflow_activities/import_data_sync.pyproducts/warehouse_sources/backend/tests/management/test_reset_wrapped_xmin_cursors.pyproducts/warehouse_sources/backend/tests/test_models.py
💤 Files with no reviewable changes (2)
- products/warehouse_sources/backend/temporal/data_imports/sources/common/test/init.py
- products/warehouse_sources/backend/temporal/data_imports/sources/postgres/test_postgres.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/warehouse_sources/backend/tests/management/test_reset_wrapped_xmin_cursors.py-90-90 (1)
90-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that reset removes the cursor keys.
_ceiling_xid(wrapped) is Nonealso passes ifsource_cursorremains stored with a null ceiling. The reset contract requires the key to be deleted. Assert thatsource_cursorand the legacy XMIN keys are absent after the live run; apply the same check to the explicitly selected schema at Line 110.
🧹 Nitpick comments (1)
products/warehouse_sources/backend/temporal/data_imports/sources/common/cursor.py (1)
120-140: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDiscard malformed payloads when the cursor constructor raises something other than
TypeError.
_cursor_from_payloadcatches onlyTypeError. If a cursor class validates its fields in__post_init__and raisesValueError, every run fails for that schema. The legacy fallback does not help, because the payload key is present. Catch(TypeError, ValueError)so that a bad stored cursor is discarded and the run continues.Proposed fix
- except TypeError: + except (TypeError, ValueError):
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 75a799b3-93d8-460d-83c4-37ee0b2a5230
📒 Files selected for processing (21)
.agents/skills/implementing-warehouse-sources/SKILL.mdproducts/warehouse_sources/backend/management/commands/reset_wrapped_xmin_cursors.pyproducts/warehouse_sources/backend/models/external_data_schema.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/common/extract.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v2/pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/lanes.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/test_pipeline.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/test/__init__.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/test/test_source_cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/common/typings.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/postgres.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/source.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/test_postgres.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/postgres/xmin_cursor.pyproducts/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_end_to_end.pyproducts/warehouse_sources/backend/temporal/data_imports/tests/e2e/test_import_data.pyproducts/warehouse_sources/backend/temporal/data_imports/workflow_activities/import_data_sync.pyproducts/warehouse_sources/backend/tests/management/test_reset_wrapped_xmin_cursors.pyproducts/warehouse_sources/backend/tests/test_models.py
💤 Files with no reviewable changes (2)
- products/warehouse_sources/backend/temporal/data_imports/sources/common/test/init.py
- products/warehouse_sources/backend/temporal/data_imports/sources/postgres/test_postgres.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.
Suppress the UUIDv7 rule for the existing ExternalDataSchema UUIDT primary key; changing persisted primary-key generation is outside this cursor change.
Problem
SourceResponsefields, threesync_type_configkeys, and a pipeline step that names each one. Every new source of this kind would add the same again.Changes
CursorSource[CursorT]mixin insources/common/cursor.py. The source loads the stored cursor and stages the next one. The framework stores it.sync_type_config["source_cursor"], as{kind, data}. The kind stops a stored cursor of another shape from loading.use_stored_cursorsdecision, so sources do not re-implement it. A reset also deletes the key.XminCursor. Schemas that still hold the old xmin keys read them until their next sync writessource_cursor, so no data migration runs.SourceResponsefields,advance_xmin_state, and the xmin model accessors and writers.reset_wrapped_xmin_cursorsreads and clears both storage forms.implementing-warehouse-sourcesskill gains a short section on the mixin.How did you test this code?
test_cursor.py(new) catches a loader that accepts another kind's cursor, crashes on a field a newer deploy added, or ignores the legacy keys.test_models.pycatches a source cursor lost in staging: one staged beside the watermark promotes with it, and a displaced run's cursor is parked and still promotes. A reset clears both storage forms.test_pipeline.pycatches the cursor being stored before the final-batch notification, or a zero-batch run never storing it.test_import_data.pycatches a cursor that survives a reset or a revive, and checks that a legacy-key cursor loads when neither applies.test_reset_wrapped_xmin_cursors.pynow runs against both storage forms.products/warehouse_sourcesandproducts/data_warehouse. A repo-wide run ran out of memory on the dev machine.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Automatic notifications
Docs update
None.