Skip to content

fix(experiments): keep legacy recalculation times consistent - #108172

Draft
pauldambra wants to merge 1 commit into
posthog/serialize-trace-retention-updatesfrom
posthog/normalize-legacy-experiment-times
Draft

pauldambra wants to merge 1 commit into
posthog/serialize-trace-retention-updatesfrom
posthog/normalize-legacy-experiment-times

Conversation

@pauldambra

Copy link
Copy Markdown
Member

[Robot] Prepared by PostHog Desktop.

Problem

Legacy experiment times can report minutes or seconds that the hourly schedule does not use.

Changes

Normalize both stored time fields to the supported hour in API and admin updates.

This fixes existing behavior before the module split in #99248.

How did you test this code?

Parameterized API and admin tests cover whole hours, minutes, and seconds. The focused tests ran against both module layouts. The full type check covers the combined fixes.

Release status

  • No feature flag controls this change

Automatic notifications

  • Publish to changelog?

Docs update

No existing document under docs/ covers this validation detail.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: PostHog Desktop / Codex, GPT-6

Tools: Git, GitHub CLI, hogli, pytest, Ruff, mypy, and CodeRabbit.
Skills: stacking-prs, improving-drf-endpoints, writing-tests, writing-code-comments, writing-user-facing-copy, setting-up-devbox, running-ci-preflight, reviewing-with-coderabbit, writing-pr-descriptions.
Each existing bug has its own layer below the refactor. No matching open fix appeared in the PR search.
The combined CodeRabbit review found two deployment tradeoffs in the membership layer. That PR records the decisions.


Created with PostHog Desktop

@pauldambra pauldambra self-assigned this Sep 29, 2026
@posthog

posthog Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

🦔 PostHog Review reviewed this pull request

Nothing worth raising this time. Enjoy the moment:

Someone relaxing in a sunny garden

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

⚠️ Duplication (Python) — 11 new duplicated blocks (worst 203 tokens)

New Python 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.

First copy Second copy Lines Tokens
posthog/temporal/ai_observability/run_aggregate_evaluation.py:692 posthog/temporal/ai_observability/run_trace_evaluation.py:928 38 203
products/warehouse_sources/backend/temporal/data_imports/sources/companycam/source.py:1 products/warehouse_sources/backend/temporal/data_imports/sources/wix/source.py:1 21 164
posthog/temporal/ai_observability/run_aggregate_evaluation.py:584 posthog/temporal/ai_observability/run_trace_evaluation.py:855 24 135
products/signals/backend/scout_harness/tools/report.py:1475 products/signals/backend/scout_harness/tools/report.py:1647 21 133
products/signals/backend/scout_harness/tools/report.py:1604 products/signals/backend/scout_harness/tools/report.py:1754 30 118
posthog/temporal/ai_observability/run_session_evaluation.py:466 posthog/temporal/ai_observability/run_trace_evaluation.py:664 24 115
posthog/management/commands/backfill_hogflow_billable_action_types.py:49 posthog/management/commands/refresh_hog_flows.py:69 23 112
products/batch_exports/backend/api/batch_export.py:2092 products/batch_exports/backend/api/batch_export.py:2131 24 112
products/warehouse_sources/backend/temporal/data_imports/sources/pardot/pardot.py:2 products/warehouse_sources/backend/temporal/data_imports/sources/wix/wix.py:1 12 97
products/signals/backend/scout_harness/tools/report.py:1516 products/signals/backend/scout_harness/tools/report.py:1675 12 71
products/signals/backend/scout_harness/tools/report.py:1531 products/signals/backend/scout_harness/tools/report.py:1690 16 70
⚠️ Duplication (TypeScript) — 2 new duplicated blocks (worst 158 tokens)

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.

First copy Second copy Lines Tokens
products/business_knowledge/frontend/scenes/settings/businessKnowledgeSettingsLogic.test.ts:10 products/data_catalog/frontend/certificationsLogic.test.ts:17 23 158
products/business_knowledge/frontend/scenes/settings/businessKnowledgeSettingsLogic.test.ts:10 products/data_quality/frontend/dataQualityGateLogic.test.ts:14 23 158

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: f42c76e3-cd0b-4f91-943b-6b8b5d12ae7f

📥 Commits

Reviewing files that changed from the base of the PR and between f1aaa68 and 2c4be1c.

📒 Files selected for processing (1)
  • posthog/api/test/test_project.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

When the legacy experiment recalculation time changes, the admin form and team API derive the plural schedule and then set the legacy time from that schedule. Tests cover three legacy-time inputs in both paths.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 2c4be

Both update paths keep the legacy recalculation time aligned with the schedule’s hour precision, including inputs with minutes or seconds. No actionable merge risk was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 2c4be

The change makes the legacy time agree with the hourly schedule across existing update paths. No new route or permission change was found. Compatibility with consumers of the legacy value remains the principal design consideration.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected write surface is the existing team-scoped configuration reached through project and environment actions; the changed public-API-labelled ranges only vary test inputs and do not expand route reachability.

Trust Boundaries and Controls

  • observed — Management permission remains on both API actions. The plural schedule is subject to hourly-format and schedule validation before the shared handler saves a PATCH.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the problem, user-visible change, testing approach, release status, documentation status, and agent context. The agent-context notes about a membership-layer review are unrelate…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@pauldambra
pauldambra added this pull request to stack #108171 September 29, 2026 08:52
@trunk-io

trunk-io Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

Generated-By: PostHog Desktop
Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
@pauldambra
pauldambra force-pushed the posthog/normalize-legacy-experiment-times branch from f1aaa68 to 2c4be1c Compare September 29, 2026 16:26

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant