Conversation
A devex semgrep rule fails CI on Celery beat, Dagster, and Temporal schedules that start at minute zero of every hour or every N hours. Batch exports keep their boundaries with a nosemgrep reason, because each run exports the interval that just closed. A test pins the three rolling-window schedules to offset zero with no jitter, so a later offset cannot make them skip data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
.semgrep/rules/devex/schedule-must-avoid-minute-zero.py |
30 | 69 |
products/batch_exports/backend/service.py |
1 | 1 |
This check does not block merging. It updates on every push and clears when the share drops.
|
[Medium risk] Adds a linting rule for schedule timing patterns. The PR is not ready to merge while the blocking rule fails on existing schedules and misses supported ways to declare new boundary schedules. Reviews (1) · Last reviewed commit: "chore(devex): block new schedules at min..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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 configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughAdds a Semgrep rule for selected Celery, Dagster, and Temporal schedules, with annotated cases for matches and non-matches. Adds Temporal tests that check fixed-window schedule intervals against lookback windows and verify zero offsets. Adds a suppression comment for the batch export interval schedule; its configuration is unchanged. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The rule’s broad cron-string matching matches its intended scope. No identified issue currently blocks merging after normal checks. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 69ce5685-08c1-42bf-acf8-5918882ff077
📒 Files selected for processing (4)
.semgrep/rules/devex/schedule-must-avoid-minute-zero.py.semgrep/rules/devex/schedule-must-avoid-minute-zero.yamlposthog/temporal/tests/test_schedule.pyproducts/batch_exports/backend/service.py
Included review availability: Your plan provides up to 12 included reviews per hour; 0 remain after this review.
The rule now matches a minute-zero hourly cron string wherever it is written, so a constant or environment default in another module is reported at its definition. A Temporal interval with offset None or a zero timedelta now counts as having no offset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
1 participant
Problem
Nothing stops a new schedule from starting at minute zero, so the pile-up that the other split PRs remove would come back. Split from #106458.
Changes
A devex semgrep rule fails CI on Celery beat, Dagster and Temporal schedules that start at minute zero of every hour or every N hours. Its message explains how to pick a minute or an offset, and when to keep the boundary with a stated exemption.
Batch exports keep their boundaries with an exemption, because each run exports the interval that just closed and batch export jitter already spreads the start. A test pins the rolling-window schedules (trace summarization, evaluation sampling and replay count metrics) to offset zero with no jitter, because a later start would skip data.
Warning
Merge this last. The rule scans the whole tree, so its semgrep-devex check fails until the team PRs land. On this branch it reports 24 schedules, and #106985 #106986 #106987 #106990 #106991 #106993 #106994 #106995 clear all of them.
How did you test this code?
The rule's semgrep test fixture passes. Semgrep 1.167.0 finds no violations with every split PR applied to master. The pinning test passes locally.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Automatic notifications
Docs update
None. The rule message carries the guidance.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5; Codex, GPT-6 (review fixes on #106458).
Split from #106458 by owning team: the owners.yaml resolver decided ownership, and a team gets its own PR when its owned files clear the reviewer assigner's bar of 10 lines or 3 files. The code is unchanged from the reviewed head of #106458, except that its signals test-fixture fix dropped because #106460 fixed the same bug on master. CodeRabbit CLI ran once with
--deepover all the split PRs combined on master 8475a79 and reported no findings. Skills for the split: stacking-prs, establishing-code-ownership, reviewing-with-coderabbit, writing-pr-descriptions. The original change also used qa-team, announcing-behavior-changes, writing-tests, writing-code-comments, writing-ui-components, writing-user-facing-copy, running-ci-preflight and debugging-ci-failures.