Repository navigation
chore(tasks): turn-completed signal, run history read and state helpers - #106428
Conversation
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
products/tasks/backend/tests/test_facade_stream_notifications.py |
8 | 174 |
products/tasks/backend/facade/api.py |
5 | 202 |
products/tasks/backend/facade/task_run_signals.py |
2 | 8 |
products/tasks/backend/models.py |
2 | 48 |
products/tasks/backend/turn_completed.py |
2 | 17 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Backend coverage — 91.0% of changed backend lines covered — 25 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ██████████████████░░ 91.0% (274 / 299)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/facade/task_run_signals.py |
75.0% | 25 |
products/tasks/backend/facade/api.py |
83.5% | 709–710, 725, 727–732, 734–738, 4542–4544, 4549, 4552–4553, 4593, 4640 |
products/tasks/backend/models.py |
92.6% | 981, 985 |
🤖 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 36153200179 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
warehouse_sources_queue |
██████░░░░░░░░░░░░░░ 29.1% |
92 / 316 |
demo |
████████████░░░░░░░░ 57.8% |
1,545 / 2,673 |
data_tools |
████████████░░░░░░░░ 61.2% |
90 / 147 |
aeo |
██████████████░░░░░░ 70.5% |
467 / 662 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
batch_exports |
████████████████░░░░ 81.2% |
21,449 / 26,424 |
apm |
█████████████████░░░ 84.1% |
1,306 / 1,553 |
ml_inference |
█████████████████░░░ 86.7% |
451 / 520 |
cdp |
██████████████████░░ 88.2% |
4,548 / 5,155 |
mcp_analytics |
██████████████████░░ 88.6% |
4,704 / 5,308 |
signals |
██████████████████░░ 88.7% |
52,399 / 59,082 |
product_tours |
██████████████████░░ 89.3% |
1,331 / 1,491 |
dashboards |
██████████████████░░ 89.8% |
7,234 / 8,056 |
data_warehouse |
██████████████████░░ 89.9% |
13,738 / 15,285 |
cohorts |
██████████████████░░ 90.2% |
8,238 / 9,138 |
notebooks |
██████████████████░░ 90.2% |
15,287 / 16,945 |
streamlit_apps |
██████████████████░░ 90.7% |
2,625 / 2,895 |
managed_warehouse |
██████████████████░░ 90.9% |
10,215 / 11,234 |
data_modeling |
██████████████████░░ 91.4% |
10,491 / 11,483 |
business_knowledge |
██████████████████░░ 91.6% |
6,899 / 7,528 |
engineering_analytics |
██████████████████░░ 91.7% |
11,002 / 11,999 |
exports |
██████████████████░░ 91.8% |
9,685 / 10,555 |
tasks |
██████████████████░░ 92.1% |
74,688 / 81,124 |
ai_training |
██████████████████░░ 92.2% |
356 / 386 |
conversations |
███████████████████░ 92.5% |
28,705 / 31,025 |
early_access_features |
███████████████████░ 92.6% |
1,341 / 1,448 |
managed_migrations |
███████████████████░ 92.7% |
1,581 / 1,705 |
visual_review |
███████████████████░ 92.8% |
9,244 / 9,966 |
canvas |
███████████████████░ 92.8% |
6,877 / 7,409 |
approvals |
███████████████████░ 93.0% |
3,919 / 4,214 |
mcp_registry |
███████████████████░ 93.1% |
1,670 / 1,794 |
error_tracking |
███████████████████░ 93.1% |
15,708 / 16,874 |
notifications |
███████████████████░ 93.2% |
1,145 / 1,229 |
slack_app |
███████████████████░ 93.2% |
13,677 / 14,674 |
surveys |
███████████████████░ 93.3% |
6,571 / 7,040 |
stamphog |
███████████████████░ 93.4% |
7,716 / 8,262 |
context_layer |
███████████████████░ 93.8% |
3,373 / 3,595 |
web_analytics |
███████████████████░ 93.9% |
21,653 / 23,051 |
ai_observability |
███████████████████░ 94.0% |
20,588 / 21,892 |
alerts |
███████████████████░ 94.1% |
8,554 / 9,094 |
billing_alerts |
███████████████████░ 94.1% |
2,094 / 2,226 |
mcp_store |
███████████████████░ 94.4% |
8,940 / 9,472 |
wizard |
███████████████████░ 94.4% |
5,791 / 6,134 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
workflows |
███████████████████░ 94.8% |
13,438 / 14,170 |
review_hog |
███████████████████░ 94.9% |
11,490 / 12,109 |
annotations |
███████████████████░ 95.1% |
817 / 859 |
customer_analytics |
███████████████████░ 95.1% |
24,669 / 25,932 |
endpoints |
███████████████████░ 95.1% |
9,211 / 9,681 |
legal_documents |
███████████████████░ 95.2% |
2,311 / 2,427 |
marketing_analytics |
███████████████████░ 95.3% |
19,047 / 19,991 |
logs |
███████████████████░ 95.4% |
15,235 / 15,967 |
tracing |
███████████████████░ 95.4% |
3,483 / 3,650 |
growth |
███████████████████░ 95.4% |
9,812 / 10,282 |
actions |
███████████████████░ 95.5% |
756 / 792 |
posthog_ai |
███████████████████░ 95.5% |
2,438 / 2,553 |
messaging |
███████████████████░ 95.9% |
3,766 / 3,927 |
skills |
███████████████████░ 95.9% |
6,649 / 6,932 |
replay_vision |
███████████████████░ 96.0% |
26,750 / 27,878 |
autoresearch |
███████████████████░ 96.0% |
7,716 / 8,037 |
product_analytics |
███████████████████░ 96.2% |
28,495 / 29,617 |
revenue_analytics |
███████████████████░ 96.4% |
1,876 / 1,946 |
access_control |
███████████████████░ 96.4% |
7,122 / 7,386 |
user_interviews |
███████████████████░ 96.5% |
2,859 / 2,963 |
feature_flags |
███████████████████░ 96.5% |
25,146 / 26,046 |
experiments |
███████████████████░ 96.6% |
33,963 / 35,146 |
warehouse_sources |
███████████████████░ 97.2% |
443,964 / 456,678 |
data_quality |
████████████████████ 97.7% |
7,592 / 7,774 |
links |
████████████████████ 97.9% |
234 / 239 |
security |
████████████████████ 98.1% |
1,258 / 1,283 |
metrics |
████████████████████ 98.1% |
4,085 / 4,166 |
analytics_platform |
████████████████████ 98.3% |
2,778 / 2,827 |
data_catalog |
████████████████████ 98.3% |
3,932 / 3,999 |
pulse |
████████████████████ 98.5% |
2,043 / 2,075 |
live_debugger |
████████████████████ 99.2% |
626 / 631 |
field_notes |
████████████████████ 99.4% |
172 / 173 |
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.
|
[Medium risk] Refactors turn-completion dispatch and adds state mutation helpers. The PR should not merge until the missing-history and duplicate-notification paths are fixed. Reviews (1) · Last reviewed commit: "refactor(tasks): turn-completed signal, ..." |
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
This is risky-territory (shared task-state row locking, event ingestion, and a new cross-product history read), the author is not on either owning team, and the only reviewer signal is an agent reviewer's unresolved P1 findings — plausible data-loss (trimmed stream can wrongly stand in for the run's own log) and duplicate-notification bugs I confirmed by reading the diff — plus a discussion comment explicitly saying the PR should not merge until they're fixed.
- 👍 on the PR from greptile-apps[bot].
- Unresolved P1: read_task_run_history can misdetect a trimmed/capped Redis stream as the whole run and skip reading the current run's S3 log, silently dropping earlier conversation frames.
- Unresolved P1: publish_task_run_stream_notification writes the same persisted notification to the live stream and the log without a shared event_id, so read_task_run_history's dedup can't recognize them and returns it twice.
- Unresolved P2: update_task_state_entry compares the callback's return value to the original entry by equality; a callback that mutates a nested dict/list in place will pass the equality check and the update will be silently dropped.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 331L, 10F substantive, 536L/12F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (536L, 12F, single-area, chore) |
| stamphog 2.1.0 | .stamphog/policy.yml @ 0a9b57d · reviewed head 0a9b57d |
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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: Your plan provides up to 12 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe changes add row-locked task-state updates and facade helpers for individual state entries. They add APIs to parse task-run logs, read and merge live stream history, and publish notifications with separate live and persisted delivery results. Turn-completion handling now uses a shared dispatcher that sends a signal and push notification for eligible runs. Existing call sites use that dispatcher. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The new history read can return duplicate frames in a mixed-write case and do unnecessary work for oversized histories. These bounded risks warrant owner awareness or follow-up, but do not establish a broad failure that blocks merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new shared interfaces have task and team scoping, and the state update uses a row lock. No introduced security issue was established, but notification and recovery behavior merits attention as other products adopt the interfaces. 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 💡 1📝 Generate docstrings
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Comment |
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 (3)
products/tasks/backend/models.py-919-922 (1)
919-922: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA shallow copy drops writes that change a nested value in place.
state = dict(current)copies only the top level of the dict. Nested dicts and lists stay shared withlocked_task.state. If a mutator changes a nested value in place, the same change appears incurrent. Thenstate != currentisFalse, and the save is skipped with no error.
update_task_state_entryinproducts/tasks/backend/facade/api.pyexposes this path to other products:
- The callback receives
state.get(key), which is the shared nested object.- A caller that does
current["x"] = 1; return current, resultchangescurrentin place.value != currentis thenFalse, so the check at lines 731-732 skips the write.mutate_state_atomicalso skips the save, so the new value is lost.Deep-copy the state before you call the mutator.
🐛 Proposed fix
- current = locked_task.state or {} - state = dict(current) + current = locked_task.state or {} + state = copy.deepcopy(current) mutator(state)Add
import copyat the top of the module.products/tasks/backend/turn_completed.py-17-17 (1)
17-17: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake turn-completed signal dispatch idempotent.
dispatch_turn_completed()sendstask_run_turn_completedon every eligible call. The proxy callback, stream ingest, and sandbox relay each call this dispatcher, and retries can repeat a report. No per-turn guard preventssend_robust()from invoking receivers more than once. Add a stable turn identifier and deduplicate the dispatch, or make each receiver idempotent.products/tasks/backend/tests/test_facade_stream_notifications.py-113-124 (1)
113-124: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winStamp persisted server notifications with an event ID.
When
persist=True,publish_task_run_stream_notificationwrites the same event to the live stream and the log.build_notification_eventdoes not assignevent_id. If the stream has no user prompt,read_task_run_historyreads the current log and appends the live event, butTaskRunStreamBacklogIndex.coversreturnsFalsefor events without an ID. The notification can therefore appear twice.Assign one unique
event_idwhen building the event, then add an overlap test that stores the same notification in both inputs and expects one history entry.
🧹 Nitpick comments (1)
products/tasks/backend/models.py (1)
3174-3181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared ACP envelope builder where timestamp semantics allow.
emit_console_event,build_progress_event, andemit_sandbox_outputduplicate the envelope returned bybuild_notification_event. Route these methods through the builder. Preserve the shared timestamp inemit_conversation_cleared, either by extending the builder to accept a timestamp or by keeping that two-event assembly local.This is a maintainability refactor, not a runtime defect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 8e783df3-90f6-4914-869c-b7b046fb3bdb
📒 Files selected for processing (12)
products/tasks/backend/agent_proxy_callback.pyproducts/tasks/backend/facade/api.pyproducts/tasks/backend/facade/contracts.pyproducts/tasks/backend/facade/task_run_signals.pyproducts/tasks/backend/logic/stream/event_ingest.pyproducts/tasks/backend/models.pyproducts/tasks/backend/presentation/views/api.pyproducts/tasks/backend/push_dispatcher.pyproducts/tasks/backend/temporal/process_task/activities/relay_sandbox_events.pyproducts/tasks/backend/tests/test_facade_stream_notifications.pyproducts/tasks/backend/tests/test_turn_completed.pyproducts/tasks/backend/turn_completed.py
Included review availability: Your plan provides up to 12 included reviews per hour; 5 remain after this review.
0a9b57d to
072f558
Compare
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: 9a69ded3-d8b8-4d39-b9e9-819cfae01128
📒 Files selected for processing (4)
products/tasks/backend/facade/api.pyproducts/tasks/backend/models.pyproducts/tasks/backend/tests/test_facade_stream_notifications.pyproducts/tasks/backend/tests/test_models.py
Included review availability: Your plan provides up to 12 included reviews per hour; 0 remain after this review.
072f558 to
1ae0d25
Compare
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: 193c8578-da00-4ce6-981d-14cefb026df4
📒 Files selected for processing (3)
products/tasks/backend/facade/api.pyproducts/tasks/backend/facade/contracts.pyproducts/tasks/backend/tests/test_facade_stream_notifications.py
Included review availability: Your plan provides up to 12 included reviews per hour; 5 remain after this review.
|
Stacked PR 106430 was cancelled: the stack changed after it was queued. |
1ae0d25 to
ddc61b7
Compare
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: cfb5ae47-0d61-4a67-b920-aa641ab05161
📒 Files selected for processing (2)
products/tasks/backend/facade/api.pyproducts/tasks/backend/tests/test_facade_stream_notifications.py
Included review availability: Your plan provides up to 12 included reviews per hour; 2 remain after this review.
| TASK_RUN_STREAM_MAX_LENGTH, | ||
| ) | ||
|
|
||
| stream_entries = _read_run_stream_entries(run) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Check max_bytes before reading the Redis stream.
When the logs exceed max_bytes, this call still fetches and parses the live stream before Line 4652 returns None. A live stream can hold approximately 5,000 entries, and ingestion permits event lines up to 1 MB. Move the log-size check ahead of _read_run_stream_entries so rejected histories do not incur that transfer or allocation. (raw.githubusercontent.com)
…lpers The three turn-completed reporters share one fan-out that sends a task_run_turn_completed signal, so other products react to a finished turn without the tasks product importing them. The facade gains a single-lookup conversation history read, task state helpers on a new Task.mutate_state_atomic, and a server notification publish that goes through TaskRun so it gets presence gating and the ACP envelope. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An unstamped stream at the length cap may be a trimmed tail, so the history read still loads the run's log for it. A persisted server notification sits in both stores without an event id, and the merge now keeps one copy. Task.mutate_state_atomic hands the mutator a deep copy, so an in-place edit of a nested value is saved. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… the log Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… log append Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ddc61b7 to
feb1e38
Compare
There was a problem hiding this comment.
Not approved — escalated to a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
This is a cross-team, non-owning-team-authored change to shared task state locking and the event-ingestion/turn-completion signal path (both explicit risky territory), and the one currently-unresolved reviewer comment (fetching the live Redis stream before checking the log size cap) plus an earlier agent reviewer's "should not merge" hold mean there is no clean independent assurance on the current head.
- 👍 on the PR from greptile-apps[bot].
- Unresolved CodeRabbit comment on facade/api.py: read_task_run_history still fetches/parses the Redis stream before checking the log max_bytes cap, so a rejected (over-size) history still pays the stream read cost.
- Greptile's earlier review (on an early commit in the stack) held the PR at 'should not merge' pending missing-history/duplicate-notification fixes; later commits appear to add dedup + extensive tests for that logic, but this isn't confirmed by a current, clean re-review.
- Author is not on either owning team (team-posthog-desktop, team-self-driving) and no STRONG-familiarity signal was reported, so there's no owning-team or familiarity-based assurance to fall back on for this risky-territory change.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 407L, 10F substantive, 694L/13F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (694L, 13F, single-area, chore) |
| stamphog 2.1.0 | .stamphog/policy.yml @ feb1e38 · reviewed head feb1e38 |
|
This pull request was merged into |
Problem
Changes
Nothing is user-visible. The riskiest part is the lock change in
Task.mutate_state_atomic; the rest adds new reads.dispatch_turn_completed. It sends atask_run_turn_completedDjango signal, then the push notification.dispatch_turn_completed.read_task_run_historyreturns every frame across the run's resume chain. It merges the logs with live stream entries the logs do not cover yet.Nonebefore any download when the logs are overmax_bytes.read_task_state_entryandupdate_task_state_entryread and merge one key ofTask.state.Task.mutate_state_atomicholds the row lock for the merge and hands the mutator a deep copy, so a nested edit is saved. It usesFOR NO KEY UPDATE, so it does not wait on theKEY SHARElock that every task run write takes on the task row.mutate_state_atomic, with the same behavior.publish_task_run_stream_notificationpublishes throughTaskRun, so the frame gets presence gating and the ACP envelope. It returns whether the live stream and the log each received the frame.How did you test this code?
test_turn_completed.pycatches a regression where a failed push notification swallows the signal, or where a non-interactive run fires it.test_facade_stream_notifications.pycatches a publish that skips the log or the live stream, a duplicated server notification, and a trimmed stream that replaces the log.test_models.pycatches a nested in-place state edit that is never saved.products/tasks/backend/tests/test_api.pyalso fails on master, so it is not caused by this layer.signals (2/2), field-notesproduct shard also fails on master since feat(signals): skip scout suggestion scans on low-activity projects #104535, withTeamScopeErrorintest_scout_suggestionssetup. This layer does not touch Signals.Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Fable 5.1 wrote the original #101991. Claude Opus 5.5 split it into this stack and fixed the review findings.
🤖 Generated with Claude Code