Repository navigation
feat(signals): build stacked report plans from research layers - #107675
trunk-io[bot] merged 14 commits into
Conversation
Research can return an optional layers plan. The ready transition creates one child report per layer, linked part_of the plan and depends_on its predecessor. A layer starts when its dependency has a pull request, and its run starts on the dependency's head branch, which stays protected as the PR base. Refs #102776 Generated-By: PostHog Desktop Task-Id: f89582b0-1f18-402d-a466-428c8f13356d
|
😎 Merged successfully - details. |
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). |
|
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 (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughResearch output can include validated dependent pull-request layers. The ready transition creates child reports for those layers and copies selected parent judgments. Layer startup checks dependency pull-request state and existing implementation runs. When a dependency has an eligible pull request and an automation branch, autostart uses that branch as the task base. Pull-request sync schedules dependent-layer starts after report completion. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to An invalid optional layer value can prevent an otherwise valid report from being produced. Normalize non-list values or otherwise handle this failure before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Dependent layers can start without consistently using the branch they depend on, and one requested-start path may leave the first layer waiting. The review did not establish a new cross-project access path or credential escalation, but these gaps can undermine the intended boundaries of automated work. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
products/signals/backend/stack_plan.py |
18 | 242 |
products/signals/backend/report_generation/research.py |
9 | 80 |
products/signals/backend/temporal/summary.py |
4 | 30 |
products/tasks/backend/models.py |
3 | 8 |
products/tasks/backend/temporal/process_task/activities/start_agent_server.py |
3 | 5 |
products/signals/backend/temporal/agentic/report.py |
2 | 4 |
products/tasks/backend/facade/api.py |
2 | 7 |
products/tasks/backend/temporal/process_task/activities/tests/test_start_agent_server.py |
2 | 18 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Backend coverage — 95.0% of changed backend lines covered — 11 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 95.0% (253 / 264)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/models.py |
50.0% | 1360 |
products/tasks/backend/facade/api.py |
66.7% | 8768 |
products/signals/backend/temporal/summary.py |
80.0% | 892, 1086 |
products/signals/backend/stack_plan.py |
93.3% | 81, 98, 185–186, 189, 231, 233 |
🤖 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 36435530003 -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 |
███████████░░░░░░░░░ 52.9% |
1,413 / 2,673 |
data_tools |
████████████░░░░░░░░ 61.2% |
90 / 147 |
aeo |
██████████████░░░░░░ 70.5% |
467 / 662 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
batch_exports |
████████████████░░░░ 81.2% |
21,466 / 26,439 |
apm |
█████████████████░░░ 84.1% |
1,306 / 1,553 |
ml_inference |
█████████████████░░░ 87.2% |
482 / 553 |
cdp |
██████████████████░░ 88.2% |
4,545 / 5,155 |
mcp_analytics |
██████████████████░░ 88.9% |
4,910 / 5,523 |
product_tours |
██████████████████░░ 89.3% |
1,331 / 1,491 |
dashboards |
██████████████████░░ 89.5% |
6,855 / 7,657 |
data_warehouse |
██████████████████░░ 89.9% |
13,915 / 15,478 |
signals |
██████████████████░░ 90.0% |
55,916 / 62,118 |
notebooks |
██████████████████░░ 90.2% |
15,287 / 16,945 |
cohorts |
██████████████████░░ 90.4% |
8,420 / 9,316 |
streamlit_apps |
██████████████████░░ 90.6% |
2,623 / 2,895 |
managed_warehouse |
██████████████████░░ 90.9% |
10,215 / 11,234 |
data_modeling |
██████████████████░░ 91.4% |
10,504 / 11,498 |
exports |
██████████████████░░ 91.7% |
9,675 / 10,555 |
engineering_analytics |
██████████████████░░ 91.7% |
11,032 / 12,030 |
tasks |
██████████████████░░ 91.9% |
74,710 / 81,316 |
ai_training |
██████████████████░░ 92.2% |
356 / 386 |
business_knowledge |
██████████████████░░ 92.2% |
7,682 / 8,328 |
conversations |
███████████████████░ 92.5% |
28,734 / 31,062 |
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,974 / 4,271 |
mcp_registry |
███████████████████░ 93.1% |
1,670 / 1,794 |
error_tracking |
███████████████████░ 93.1% |
15,864 / 17,033 |
notifications |
███████████████████░ 93.2% |
1,145 / 1,229 |
slack_app |
███████████████████░ 93.2% |
13,677 / 14,674 |
stamphog |
███████████████████░ 93.2% |
7,885 / 8,456 |
surveys |
███████████████████░ 93.3% |
6,571 / 7,040 |
context_layer |
███████████████████░ 93.8% |
3,373 / 3,595 |
web_analytics |
███████████████████░ 93.9% |
21,648 / 23,051 |
alerts |
███████████████████░ 94.0% |
8,539 / 9,082 |
billing_alerts |
███████████████████░ 94.1% |
2,094 / 2,226 |
ai_observability |
███████████████████░ 94.3% |
22,516 / 23,871 |
mcp_store |
███████████████████░ 94.4% |
8,940 / 9,472 |
wizard |
███████████████████░ 94.7% |
6,150 / 6,496 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
workflows |
███████████████████░ 94.8% |
14,332 / 15,113 |
review_hog |
███████████████████░ 95.0% |
11,507 / 12,119 |
endpoints |
███████████████████░ 95.1% |
9,206 / 9,681 |
annotations |
███████████████████░ 95.1% |
817 / 859 |
customer_analytics |
███████████████████░ 95.1% |
24,897 / 26,167 |
legal_documents |
███████████████████░ 95.2% |
2,311 / 2,427 |
marketing_analytics |
███████████████████░ 95.3% |
19,209 / 20,161 |
posthog_ai |
███████████████████░ 95.3% |
2,488 / 2,610 |
logs |
███████████████████░ 95.4% |
15,285 / 16,022 |
experiments |
███████████████████░ 95.4% |
32,458 / 34,023 |
growth |
███████████████████░ 95.4% |
9,811 / 10,282 |
actions |
███████████████████░ 95.5% |
756 / 792 |
data_catalog |
███████████████████░ 95.5% |
4,401 / 4,606 |
tracing |
███████████████████░ 95.6% |
3,517 / 3,680 |
autoresearch |
███████████████████░ 95.7% |
8,481 / 8,865 |
messaging |
███████████████████░ 95.8% |
3,798 / 3,963 |
skills |
███████████████████░ 95.8% |
6,972 / 7,274 |
replay_vision |
███████████████████░ 95.9% |
27,153 / 28,310 |
product_analytics |
███████████████████░ 96.0% |
28,470 / 29,647 |
access_control |
███████████████████░ 96.3% |
7,112 / 7,386 |
revenue_analytics |
███████████████████░ 96.4% |
1,876 / 1,946 |
user_interviews |
███████████████████░ 96.5% |
2,859 / 2,963 |
feature_flags |
███████████████████░ 96.5% |
25,543 / 26,460 |
warehouse_sources |
███████████████████░ 97.2% |
454,068 / 467,020 |
data_quality |
████████████████████ 97.6% |
7,587 / 7,774 |
security |
████████████████████ 97.9% |
1,202 / 1,228 |
links |
████████████████████ 97.9% |
234 / 239 |
metrics |
████████████████████ 98.0% |
4,084 / 4,166 |
analytics_platform |
████████████████████ 98.3% |
2,783 / 2,832 |
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.
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 adds a new autonomous-agent execution path that decides which GitHub branch the self-driving PR bot pushes into and protects as its PR base — get the branch-protection logic wrong and the automation can push into or overwrite the wrong branch in production. That's security/production-impacting automation territory, it has zero reviews or reactions, the PR is cross-team (touches team-posthog-desktop-owned launch code without that team's sign-off), and the author is a machine user so familiarity/ownership give no independent assurance. Tests are solid but the operating rules require an actual review or owning-team sign-off here, not just test coverage.
- No reviews, comments, or reactions provide independent assurance for a change that alters which branch the autonomous PR agent treats as protected/base — a bug here could let the automation push into or overwrite an unrelated branch.
- Cross-team change: modifies task-launch/branch-protection code alongside signals-owned files, with no sign-off from the owning team for the tasks-side change.
- Author is a machine user (posthog[bot]) so no familiarity/ownership signal is available to substitute for review.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 392L, 10F substantive, 636L/14F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (636L, 14F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ a433d39 · reviewed head a433d39 |
Generated-By: PostHog Desktop Task-Id: f89582b0-1f18-402d-a466-428c8f13356d
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.
Cross-team change (author on neither owning team, no STRONG familiarity signal for a bot author) that rewrites branch-protection logic governing where an autonomous coding agent is allowed to push during automated PR creation — a real production-safety surface (wrong branch protection could let an agent corrupt another PR's branch). No human or completed bot review exists yet (CoderRabbit hit its rate limit, the in-house PostHog Review pass is still mid-run at step 3/6), so there is no independent assurance to rely on for this risky-territory piece.
- No completed independent review: CodeRabbit rate-limited, PostHog Review (reviewhog) still in progress (3/4 passes), and no human review
- Cross-team change: task-launch branch-protection files are owned by @PostHog/team-posthog-desktop while the PR is scoped from signals; author (posthog[bot]) is not on either owning team
- Changes what branch an autonomous agent's launch treats as protected/base, which is a production data-integrity-sensitive path (wrong branch protection could let an automated run push into a sibling PR's branch)
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 396L, 10F substantive, 640L/14F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (640L, 14F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 64e5466 · reviewed head 64e5466 |
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/signals/backend/temporal/summary.py-1085-1088 (1)
1085-1088: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIsolate layer-start failures from parent autostart evaluation.
start_unblocked_layers_of_planperformsincoming_linksandassociated_task_runslookups outside the per-layer exception handler. If either lookup raises,maybe_autostart_implementation_activityfails and Temporal retries the entire activity. For a plan parent,maybe_autostart_from_report_artefactsrecords theplan_parentskip before the layer lookup, so each retry can create anotherautostart_skipartefact and emit anothersignals_autostart_skippedevent. Catch layer-start failures separately so they do not retry the parent evaluation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 29cf1ea2-8b89-40e5-a4ce-34306b4454c3
📒 Files selected for processing (14)
products/signals/ARCHITECTURE.mdproducts/signals/backend/auto_start.pyproducts/signals/backend/receivers.pyproducts/signals/backend/report_generation/research.pyproducts/signals/backend/stack_plan.pyproducts/signals/backend/tasks.pyproducts/signals/backend/temporal/agentic/report.pyproducts/signals/backend/temporal/summary.pyproducts/signals/backend/test/test_auto_start.pyproducts/signals/backend/test/test_stack_plan.pyproducts/tasks/backend/facade/api.pyproducts/tasks/backend/models.pyproducts/tasks/backend/temporal/process_task/activities/start_agent_server.pyproducts/tasks/backend/temporal/process_task/activities/tests/test_start_agent_server.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 0 remain after this review.
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
PostHog Review
Found 3 must fix, 8 should fix, 3 consider.
Other findings (outside the changed lines)
Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.
Schedule waiting layers from canonical PR changes
Priority: should_fix | File: products/signals/backend/receivers.py:153-157 | Category: bug
Issue description
This is the only path that schedules dependent layers, and it runs on a TaskRun output save. Attaching a PR to a report through claim_report does not save a task run. A later PR state update also does not save one. A child that waits on either change can remain idle with no further event to start it.
Why we think it's a valid issue
- Checked: Every caller of
schedule_dependent_layersandstart_dependent_stack_layers, every caller oflink_pull_request(products/signals/backend/pull_requests.py:195,products/signals/backend/report_assignments.py:298,409,487),claim_reportand the claim endpoint (products/signals/backend/views.py:1869-1890), the webhook state syncupdate_assignments_for_pull_request(report_assignments.py:456-500), andTaskRun.update_output_atomic(products/tasks/backend/models.py:2654-2660). - Found:
receivers.py:157is the only caller ofschedule_dependent_layers, and it runs only on aTaskRunsave that includesoutput. The plan's settle point (start_unblocked_layers_of_plan) runs one time, at the ready transition. - Found:
claim_reportlinks PRs throughlink_pull_request(report_assignments.py:409) and saves noTaskRun. It servesPOST .../claim(views.py:1869-1890, scopetask:write). A person or an external coding agent uses that endpoint to claim an inbox report and attach its PR. - Found:
update_assignments_for_pull_requestchanges the PR state from GitHub webhooks for all linked PRs (report_assignments.py:456-500) and does not schedule dependents either. For a task-created PR, the claim in the finding is only partly true._record_run_pr_statecallsupdate_output_atomic, which callssave(update_fields=["output", ...]), so the receiver still fires. The gap is for PRs that no task run created. - Impact: Suppose a person or an external agent implements a layer and attaches the PR through the claim endpoint. The layers that depend on it pass
_dependencies_have_pull_requests, but no event runsstart_dependent_layersfor them. They stayREADYand idle until something else re-evaluates auto-start for that child, such as a reviewer edit. The stack stalls and records noautostart_skiprow, so nothing shows why.
Suggested fix
Schedule dependent layers after commit when a PR link is attached or its canonical state changes. Keep the task-output hook for task-created PRs, and use the same readiness check in each path.
| ] | ||
| return start_unblocked_layers(team_id=team_id, report_ids=dependent_ids) | ||
|
|
||
|
|
||
| def schedule_dependent_layers(*, team_id: int, report_ids: list[str]) -> None: | ||
| """After commit, queue a start of the layers that wait on these reports.""" | ||
| from products.signals.backend.tasks import start_dependent_stack_layers # noqa: PLC0415 |
There was a problem hiding this comment.
Stop waiting layers when the plan fails re-research
Issue description
A later research pass can mark the parent plan FAILED after a safety rejection or reset it to POTENTIAL as not actionable. Its existing children remain READY with their earlier judgments. When a lower layer opens a PR, this path still starts the next child because it checks the dependency PR but not the parent verdict.
Why we think it's a valid issue
- Checked: The re-research branches in
products/signals/backend/temporal/summary.py:362-392(safety rejection) andsummary.py:466-483(not actionable),mark_report_failed_activity(summary.py:1104-1113),reset_report_to_potential_activity(summary.py:1250-1262), every reader ofReportLinkKind.PART_OF, the auto-start link gates_evaluate_link_gates(products/signals/backend/auto_start.py:575-619), the status check under the auto-start lock (auto_start.py:750), and_fetch_source_references(auto_start.py:422-429). - Found: A
READYplan parent is re-researched when new signals re-promote it. The safety judge runs over all of the parent's signals. A rejection callsmark_report_failed_activityand aNOT_ACTIONABLEverdict callsreset_report_to_potential_activity. Both activities lock and change only the parent row. No code cascades the new status or judgments to the children. - Found:
create_layer_reportscopies the parent's safety, actionability, priority, repository and reviewer artefacts onto each child one time (stack_plan.py:61-75,107-114). On a later run it returns early when layers already exist (stack_plan.py:90-91), so the children keep the earlier approval. - Found: The only readers of
PART_OFarestack_plan.py,plan_rollup.py, the research context, and theplan_parentgate atauto_start.py:614. That gate reads incoming links and stops only the parent._evaluate_link_gateschecksduplicate_of,depends_onPR presence, andplan_parent. It never reads the status of the outgoingpart_oftarget. The lock check atauto_start.py:750reads only the child's own status. When a lower layer's PR syncs,start_dependent_layers→start_unblocked_layers→maybe_autostart_from_report_artefactsstarts the nextREADYchild. - Impact: After the pipeline marks a plan
FAILEDor resets it toPOTENTIAL, each lower-layer PR still starts the next layer. Each start opens an implementation run and a PR for work that the pipeline now rejects, which costs credits and reviewer time. The same gap applies when a person suppresses or deletes the plan parent. A fix only instart_unblocked_layersis incomplete, because other artefact writes also call auto-start directly on the child. - Priority: Lowered to
should_fix. The layer's prompt does not carry the rejected content: its summary comes from the earlier, approved research, and_fetch_source_referencesreads only the child's own signals, and a child has none. The trigger also needs a plan to be re-promoted while its stack is still landing. The result is unwanted PRs that a person still reviews. There is no data loss and no bypass of injection screening.
Suggested fix
Check the current part_of parent's status before an unstarted child can auto-start. Enforce that check in the locked auto-start gate too, so a reviewer edit cannot bypass it.
Prompt to fix with AI (copy-paste)
## Context
@products/signals/backend/stack_plan.py#L219-225
<issue_description>
A later research pass can mark the parent plan `FAILED` after a safety rejection or reset it to `POTENTIAL` as not actionable. Its existing children remain `READY` with their earlier judgments. When a lower layer opens a PR, this path still starts the next child because it checks the dependency PR but not the parent verdict.
</issue_description>
<issue_validation>
- **Checked:** The re-research branches in `products/signals/backend/temporal/summary.py:362-392` (safety rejection) and `summary.py:466-483` (not actionable), `mark_report_failed_activity` (`summary.py:1104-1113`), `reset_report_to_potential_activity` (`summary.py:1250-1262`), every reader of `ReportLinkKind.PART_OF`, the auto-start link gates `_evaluate_link_gates` (`products/signals/backend/auto_start.py:575-619`), the status check under the auto-start lock (`auto_start.py:750`), and `_fetch_source_references` (`auto_start.py:422-429`).
- **Found:** A `READY` plan parent is re-researched when new signals re-promote it. The safety judge runs over all of the parent's signals. A rejection calls `mark_report_failed_activity` and a `NOT_ACTIONABLE` verdict calls `reset_report_to_potential_activity`. Both activities lock and change only the parent row. No code cascades the new status or judgments to the children.
- **Found:** `create_layer_reports` copies the parent's safety, actionability, priority, repository and reviewer artefacts onto each child one time (`stack_plan.py:61-75`, `107-114`). On a later run it returns early when layers already exist (`stack_plan.py:90-91`), so the children keep the earlier approval.
- **Found:** The only readers of `PART_OF` are `stack_plan.py`, `plan_rollup.py`, the research context, and the `plan_parent` gate at `auto_start.py:614`. That gate reads incoming links and stops only the parent. `_evaluate_link_gates` checks `duplicate_of`, `depends_on` PR presence, and `plan_parent`. It never reads the status of the outgoing `part_of` target. The lock check at `auto_start.py:750` reads only the child's own status. When a lower layer's PR syncs, `start_dependent_layers` → `start_unblocked_layers` → `maybe_autostart_from_report_artefacts` starts the next `READY` child.
- **Impact:** After the pipeline marks a plan `FAILED` or resets it to `POTENTIAL`, each lower-layer PR still starts the next layer. Each start opens an implementation run and a PR for work that the pipeline now rejects, which costs credits and reviewer time. The same gap applies when a person suppresses or deletes the plan parent. A fix only in `start_unblocked_layers` is incomplete, because other artefact writes also call auto-start directly on the child.
- **Priority:** Lowered to `should_fix`. The layer's prompt does not carry the rejected content: its summary comes from the earlier, approved research, and `_fetch_source_references` reads only the child's own signals, and a child has none. The trigger also needs a plan to be re-promoted while its stack is still landing. The result is unwanted PRs that a person still reviews. There is no data loss and no bypass of injection screening.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Check the current `part_of` parent's status before an unstarted child can auto-start. Enforce that check in the locked auto-start gate too, so a reviewer edit cannot bypass it.
</potential_solution>
There was a problem hiding this comment.
Escalated. The gap is real, but the fix is a design choice that a person must make.
- Today no layer start path reads the plan's status, so layers of a failed or reset plan can still start.
- Option A: an auto-start gate that holds a report when its
part_ofparent is not ready. This needs a new skip reason and an inbox label. - Option A also holds children of scout-written
part_ofparents. Those parents are not stack plans, so their current behavior would change. - Option B: cascade the plan's failed or potential verdict to its unstarted layers in the summary workflow. Thread 15 raises the same root cause.
- Decide which parent statuses stop a layer (failed, potential, archived, deleted, in progress) and which option to use.
| team_id=team_id, | ||
| status=SignalReport.Status.READY, | ||
| title=layer.title, | ||
| summary=_layer_summary(parent=parent, layer=layer, index=index, total=len(layers)), | ||
| promoted_at=now, | ||
| # Born directly in a visible status without `transition_to`, which stamps this for | ||
| # pipeline reports, so the daily report limit counts the layer from creation. | ||
| first_visible_at=now, | ||
| ) | ||
| child_id = str(child.id) | ||
| for judgment in judgments: | ||
| SignalReportArtefact.append_status( | ||
| team_id=team_id, | ||
| report_id=child_id, | ||
| content=judgment, | ||
| attribution=attribution, | ||
| reevaluate_autostart=False, |
There was a problem hiding this comment.
Preserve the source issue for each child report
Issue description
Each child is created without the parent's source signals or a source reference. The auto-start prompt and PR Origin section look up issue references by the child's report ID. They therefore cannot link a layer's PR to the GitHub issue that produced the plan.
Why we think it's a valid issue
- Checked:
fetch_source_references_for_reportinproducts/signals/backend/signal_metadata.py:301-340, its two readers (_fetch_source_references→_build_autostart_task_descriptioninproducts/signals/backend/auto_start.py:350-420,1510-1529, andPullRequestOrigin.for_reportinproducts/signals/backend/pr_origin.py:262-276), the Origin section render (pr_origin.py:290-310), the tracker-issue path inproducts/signals/backend/tracker_issues.py:253-300, and child creation inproducts/signals/backend/stack_plan.py:95-136. - Found: Both readers select signals by
metadata.report_idfor the report's own id.create_layer_reportsgives a child no signals and no stored source reference. For a layer, both lookups return nothing. - Found: With no references, the task description drops the
Source issues:line and the instruction that the work comes from the source issue. The PR footer also dropsaddressing [issue](auto_start.py:361-373). The backend-written Origin section then renders onlyInbox reportand theTask started byline. It has no source type, noIssues:line, and noFirst signaldate (pr_origin.py:290-303). - Found: Tracker issues are not affected. Each child gets its own tracker issue when it starts (
tracker_issues.py:285-300), and a tracker issue is a separate thing from the source GitHub issue. - Impact: The plan parent never runs because of the
plan_parentgate. So no PR in a stack links the GitHub issue that asked for the stack, and the implementation agent does not get that issue's URL, only the layer's short scope. A reviewer who works only in GitHub cannot trace a layer PR to its issue. The only path is the inbox report link, then the plan report, both behind a PostHog login. This happens for every stacked PR. - Priority: Lowered to
consider. The loss is traceability and context, and nothing breaks. The PR still opens and stacks, the Origin section omits the missing facts instead of stating wrong ones, and the inbox link still leads to the plan and its signals.
Suggested fix
Make the source-reference readers follow a child's part_of link to its plan, or store an explicit source reference for each child. Keep the child's own report link while carrying the original issue into its task and PR.
Prompt to fix with AI (copy-paste)
## Context
@products/signals/backend/stack_plan.py#L92-108
<issue_description>
Each child is created without the parent's source signals or a source reference. The auto-start prompt and PR Origin section look up issue references by the child's report ID. They therefore cannot link a layer's PR to the GitHub issue that produced the plan.
</issue_description>
<issue_validation>
- **Checked:** `fetch_source_references_for_report` in `products/signals/backend/signal_metadata.py:301-340`, its two readers (`_fetch_source_references` → `_build_autostart_task_description` in `products/signals/backend/auto_start.py:350-420`, `1510-1529`, and `PullRequestOrigin.for_report` in `products/signals/backend/pr_origin.py:262-276`), the Origin section render (`pr_origin.py:290-310`), the tracker-issue path in `products/signals/backend/tracker_issues.py:253-300`, and child creation in `products/signals/backend/stack_plan.py:95-136`.
- **Found:** Both readers select signals by `metadata.report_id` for the report's own id. `create_layer_reports` gives a child no signals and no stored source reference. For a layer, both lookups return nothing.
- **Found:** With no references, the task description drops the `Source issues:` line and the instruction that the work comes from the source issue. The PR footer also drops `addressing [issue]` (`auto_start.py:361-373`). The backend-written Origin section then renders only `Inbox report` and the `Task started by` line. It has no source type, no `Issues:` line, and no `First signal` date (`pr_origin.py:290-303`).
- **Found:** Tracker issues are not affected. Each child gets its own tracker issue when it starts (`tracker_issues.py:285-300`), and a tracker issue is a separate thing from the source GitHub issue.
- **Impact:** The plan parent never runs because of the `plan_parent` gate. So no PR in a stack links the GitHub issue that asked for the stack, and the implementation agent does not get that issue's URL, only the layer's short scope. A reviewer who works only in GitHub cannot trace a layer PR to its issue. The only path is the inbox report link, then the plan report, both behind a PostHog login. This happens for every stacked PR.
- **Priority:** Lowered to `consider`. The loss is traceability and context, and nothing breaks. The PR still opens and stacks, the Origin section omits the missing facts instead of stating wrong ones, and the inbox link still leads to the plan and its signals.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Make the source-reference readers follow a child's `part_of` link to its plan, or store an explicit source reference for each child. Keep the child's own report link while carrying the original issue into its task and PR.
</potential_solution>
There was a problem hiding this comment.
Escalated. Layer PRs could link the source issue, but the fix changes what the agent is told, so a person must decide.
- A layer has no signals of its own, so its task prompt and PR Origin section omit the plan's source issue.
- If a layer inherits the issue, the agent is told the work comes from that issue. The issue covers the whole stack, so the agent could work past its layer.
- The same fallback would also give scout-made
part_ofchildren their parent's issues. - Decide which Origin fields a layer inherits, and how the prompt keeps the agent inside the layer scope.
A blank layer title made a ready child report that auto-start never runs, so every later layer stalled. The plan validator now keeps the report a single pull request, the same fallback it uses for a plan outside the layer bounds, instead of failing the presentation turn. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: d5eddc27-0145-40f4-a736-7f2d74c32350
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.
An independent agent reviewer (PostHog Review) found 3 must-fix and 8 should-fix issues on this new PR-stacking automation, with none of the 15 inline threads resolved — including a security-tagged reviewer-attribution bug that could let a task run under the wrong colleague's identity, a gap that lets dismissed layers auto-start anyway, and a branch-protection marker that isn't carried through task resume, letting a stacked run push into another branch. I independently confirmed these against the diff (e.g. the stackable-PR-state check omits the 'unknown' state treated as live elsewhere, and the layer-start check never verifies report status).
- Reviewer attribution on child layer reports uses task/system attribution instead of the parent's, so auto-start can treat a user-selected reviewer list as pipeline-trusted and start a task attributed to a named colleague (security).
- start_unblocked_layers/start_dependent_layers check only for an existing task run and a dependency PR, not report status, so a person-dismissed layer can still be auto-started once its dependency opens a PR.
- _STACKABLE_PR_STATES (OPEN, DRAFT) excludes the 'unknown' PR state that has_open_or_merged_pull_request treats as live elsewhere, so an unknown-state dependency lets the next layer start on the default branch instead of waiting, breaking the stack.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 399L, 10F substantive, 664L/14F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (664L, 14F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 72aff37 · reviewed head 72aff37 |
A depends_on link can join reports in two repositories of one team. The dependency's automation branch exists only in its own repository, so a run in the other repository failed to check it out and never opened a pull request. The lookup now uses the branch only when the dependency's open or draft pull request is in the report's repository, and the report otherwise keeps its configured base. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: d5eddc27-0145-40f4-a736-7f2d74c32350
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.
Multiple must-fix bugs raised by the automated reviewer remain unresolved on the current diff and are independently confirmed by reading the code: the stack-base-branch resume paths don't carry the new protection marker forward (a security-relevant branch-protection gap), the dependency-PR-state check used to gate starting a layer is inconsistent with the branch-selection check (unknown PR state can still start work on the wrong base), and dismissed/suppressed layers aren't excluded from auto-start when a dependency's PR opens. This is risky territory (auth/security branch protection, billing-exemption inheritance, task launch activities) with no approving review — only bot COMMENTED reviews listing unaddressed bugs — so there is no independent assurance to rely on.
- Unresolved must-fix: stack_base_branch is not propagated through the task resume paths in facade/api.py, so a resumed stacked run can lose branch protection and push into the dependency's branch.
- Unresolved must-fix: dependency_head_branch only treats OPEN/DRAFT as stackable while the earlier _dependencies_have_pull_requests gate also allows UNKNOWN PR state, so a layer can start on the wrong (default) base without actually stacking, defeating the stacking guarantee.
- Unresolved must-fix: start_unblocked_layers/start_dependent_layers do not check SignalReport.status, so a layer a person dismissed can still be auto-started once its dependency opens a PR.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 409L, 10F substantive, 694L/14F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (694L, 14F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 1485d2e · reviewed head 1485d2e |
…eports Start only ready layers, treat an unknown dependency PR state as stackable, skip inheriting a user-edited reviewer list, and carry stack_base_branch through a resume. Generated-By: PostHog Desktop Task-Id: f89582b0-1f18-402d-a466-428c8f13356d
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 a large, cross-team change to autostart, branch-protection, and billing-exemption logic (risky territory), and the automated reviewer left 3 must_fix and 8 should_fix findings — including a security-flagged reviewer-attribution issue and a branch-protection bypass on resumed runs — all still unresolved on the current head.
- Unresolved must_fix from @PostHog[bot]: 'Carry the protected stack base into resumed runs' — resumed runs drop stack_base_branch, letting the launch fall back to protecting the wrong branch so signed writes can land on the dependency's branch.
- Unresolved must_fix (security) from @PostHog[bot]: 'Preserve reviewer attribution when creating layers' — child reports copy suggested reviewers with the pipeline's own attribution, so auto-start can start a task under a named colleague's identity rather than the user's.
- Unresolved must_fix from @PostHog[bot]: 'Do not start a stacked layer when the dependency PR state is unknown' — a dependency PR in an unknown state can cause the next layer to start on the wrong base branch, defeating the stacking guarantee.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 431L, 10F substantive, 732L/14F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (732L, 14F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 0926319 · reviewed head 0926319 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
products/signals/backend/test/test_stack_plan.py (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the three changed test signatures.
The three test methods omit return annotations. The Python guideline requires annotations on every signature. Add
-> Noneto satisfy the requirement and improve strict type-checking coverage. This is a code-quality issue, not a runtime failure.Suggested fix
- def test_blank_layer_text_is_no_plan(self, _name: str, blank: dict[str, str]): + def test_blank_layer_text_is_no_plan(self, _name: str, blank: dict[str, str]) -> None: - def test_an_archived_layer_does_not_start_when_its_dependency_opens_a_pull_request(self): + def test_an_archived_layer_does_not_start_when_its_dependency_opens_a_pull_request(self) -> None: - ): + ) -> None:
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: ec45fa92-8c46-45fb-9322-1ad927944c9d
📒 Files selected for processing (5)
products/signals/backend/auto_start.pyproducts/signals/backend/report_generation/research.pyproducts/signals/backend/stack_plan.pyproducts/signals/backend/test/test_stack_plan.pyproducts/tasks/backend/facade/api.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
PostHog Review
Found 1 must fix, 2 should fix, 2 consider.
Other findings (outside the changed lines)
Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.
Start plan layers after a requested research run
Priority: consider | File: products/signals/backend/temporal/summary.py:1071-1088 | Category: bug
Issue description
The requested-implementation path skips start_unblocked_layers_of_plan. If requested research creates a new plan, it tries to start an implementation task on the parent instead. The plan_parent gate blocks that task, but start_requested_implementation treats the returned AutostartSkip as a successful creation. No child starts, and the workflow reports success.
Why we think it's a valid issue
- Checked:
maybe_autostart_implementation_activity(summary.py:1060-1088),start_requested_implementation(auto_start.py:905-1060),_create_implementation_task_if_absentand_evaluate_link_gates(auto_start.py:575-619,auto_start.py:700-834),AutostartSkip(artefact_schemas.py:768), every caller that setsrequested_implementation_user_id, andmanagement/commands/rerun_signal_report.py. - Found: In the requested branch (
summary.py:1071-1082), onlystart_requested_implementationruns.start_unblocked_layers_of_planis called only in theelsebranch (summary.py:1086). A requested research pass that returnslayersstill creates the children through_write_stack_layers, but no code starts them. - Found: With no linked implementation task,
start_requested_implementationcalls_create_implementation_task_if_absent, whose return type isbool | AutostartSkip. Theplan_parentgate returns anAutostartSkipbefore any write. The caller checks onlyif not created:(auto_start.py:1010-1012).AutostartSkipis a pydanticBaseModelwith no__bool__, so it is truthy (verified:bool(Model())isTrue). The function returns"created", the activity succeeds, and it writes no skip row. - Found: With a linked task, the function takes the
run_taskresume branch (auto_start.py:1015-1050), which does no link-gate check. The plan's old task resumes with the whole change, and the layers still do not start. - Found: The only producer of the requested path is the operator management command
rerun_signal_report(rerun_signal_report.py:157-161). No API view setsrequested_implementation_user_id. The truthy-AutostartSkiphandling existed before this PR for thedepends_onandduplicate_ofgates. This PR makes theplan_parentcase reachable. - Impact: An operator rerun whose research turns a report into a new plan reports success, but it starts no layers (or it resumes the plan's own task). No later PR event starts the first layer, so the whole stack stays idle until a person presses Implement on a layer.
- Priority: Lowered to
consider. Only a staff management command reaches this path, and only when that rerun's research splits the report into layers for the first time. The layers stay visible and can start by hand.
Suggested fix
When requested research produces a plan, start its unblocked child layers instead of requesting an implementation task on the parent. Handle AutostartSkip explicitly so a blocked task cannot be reported as created. Add a test for a requested run that produces a new plan.
| # just stored: written earlier it would name a metric the report does not have yet, | ||
| # and written later it could survive a rollback that took the metric with it. | ||
| _write_research_checks(report, input) | ||
| _write_stack_layers(report, input) |
There was a problem hiding this comment.
Stop waiting layers when their plan becomes unsafe or stale
Issue description
Layer reports keep copies of the plan's judgments. If new signals cause the plan to fail a later safety review or require human input, its existing children remain ready. A lower layer's PR sync can then start a waiting child using the old judgments, even though the plan no longer permits autonomous work.
Why we think it's a valid issue
- Checked:
start_unblocked_layersandstart_dependent_layersinproducts/signals/backend/stack_plan.py,_evaluate_link_gatesinproducts/signals/backend/auto_start.py:575-619, the re-research branches inproducts/signals/backend/temporal/summary.py:362-505,mark_report_failed_activity(summary.py:1104-1115),plan_rollup.py, andfetch_source_references_for_reportinsignal_metadata.py:301. - Found:
start_unblocked_layersfilters only on the child's own status (status=SignalReport.Status.READY), an existing implementation run, and dependency PRs. It never reads the plan report._evaluate_link_gateschecksduplicate_of,depends_on, and incomingpart_of(plan_parent). It has no check on the status of an outgoingpart_oftarget. - Found: A READY plan still collects signals and re-researches at each signal bucket. On that pass the plan can move to FAILED on a safety reject (
summary.py:373-391), to POTENTIAL on NOT_ACTIONABLE (summary.py:468-485), or to PENDING_INPUT on REQUIRES_HUMAN_INPUT (summary.py:486-505). No code path pushes this change to the children.create_layer_reportscopies the judgments once,_INHERITED_STATUS_TYPESis never refreshed, andplan_rollup.pyonly rolls child completion up to the plan. A person who dismisses the plan in the inbox gets the same result, because nothing cascades from the plan to its children. - Found: The safety angle is weaker than the title says. A child has no signals of its own, so
_fetch_source_references(team_id, child_id)returns nothing from the new unsafe signals. The child's task runs on the scope from the earlier pass, which passed the safety review. - Impact: After the plan's latest pass (or a person) withdraws autonomy, the next PR sync of a lower layer still queues
start_dependent_stack_layers. The next layer then starts on its stale copied judgments and opens an autonomous, billable PR for a plan that is no longer READY. The window lasts for the whole stack, and gets longer when a quota or daily-limit block holds a layer until a later PR event. - Priority: Lowered to
should_fix. The trigger needs an in-flight stack and a flipped plan verdict or plan dismissal. The new unsafe signal content does not reach the child's run, so this is a wrong autonomous start and a wasted billable PR, not a safety bypass.
Suggested fix
Before starting a waiting layer, check that its plan is still ready and eligible. Pause unstarted children when re-research changes the plan's safety or actionability decision. Refresh their judgments before allowing them to start again.
Prompt to fix with AI (copy-paste)
## Context
@products/signals/backend/temporal/summary.py#L956
<issue_description>
Layer reports keep copies of the plan's judgments. If new signals cause the plan to fail a later safety review or require human input, its existing children remain ready. A lower layer's PR sync can then start a waiting child using the old judgments, even though the plan no longer permits autonomous work.
</issue_description>
<issue_validation>
- **Checked:** `start_unblocked_layers` and `start_dependent_layers` in `products/signals/backend/stack_plan.py`, `_evaluate_link_gates` in `products/signals/backend/auto_start.py:575-619`, the re-research branches in `products/signals/backend/temporal/summary.py:362-505`, `mark_report_failed_activity` (`summary.py:1104-1115`), `plan_rollup.py`, and `fetch_source_references_for_report` in `signal_metadata.py:301`.
- **Found:** `start_unblocked_layers` filters only on the child's own status (`status=SignalReport.Status.READY`), an existing implementation run, and dependency PRs. It never reads the plan report. `_evaluate_link_gates` checks `duplicate_of`, `depends_on`, and incoming `part_of` (plan_parent). It has no check on the status of an outgoing `part_of` target.
- **Found:** A READY plan still collects signals and re-researches at each signal bucket. On that pass the plan can move to FAILED on a safety reject (`summary.py:373-391`), to POTENTIAL on NOT_ACTIONABLE (`summary.py:468-485`), or to PENDING_INPUT on REQUIRES_HUMAN_INPUT (`summary.py:486-505`). No code path pushes this change to the children. `create_layer_reports` copies the judgments once, `_INHERITED_STATUS_TYPES` is never refreshed, and `plan_rollup.py` only rolls child completion up to the plan. A person who dismisses the plan in the inbox gets the same result, because nothing cascades from the plan to its children.
- **Found:** The safety angle is weaker than the title says. A child has no signals of its own, so `_fetch_source_references(team_id, child_id)` returns nothing from the new unsafe signals. The child's task runs on the scope from the earlier pass, which passed the safety review.
- **Impact:** After the plan's latest pass (or a person) withdraws autonomy, the next PR sync of a lower layer still queues `start_dependent_stack_layers`. The next layer then starts on its stale copied judgments and opens an autonomous, billable PR for a plan that is no longer READY. The window lasts for the whole stack, and gets longer when a quota or daily-limit block holds a layer until a later PR event.
- **Priority:** Lowered to `should_fix`. The trigger needs an in-flight stack and a flipped plan verdict or plan dismissal. The new unsafe signal content does not reach the child's run, so this is a wrong autonomous start and a wasted billable PR, not a safety bypass.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Before starting a waiting layer, check that its plan is still ready and eligible. Pause unstarted children when re-research changes the plan's safety or actionability decision. Refresh their judgments before allowing them to start again.
</potential_solution>
There was a problem hiding this comment.
Escalated with thread 3. The gap is real, and the fix needs the same design decision.
- No layer start path reads the plan's status, so a layer can start after its plan fails, needs input, or is dismissed.
- Option A: an auto-start gate on the
part_ofparent's status. This needs a new skip reason, and it also affects scout-madepart_ofchildren. - Option B: pause the unstarted layers in the summary workflow when the plan's verdict changes. Refresh their copied judgments when the plan becomes ready again.
- Decide A or B, and which plan statuses (failed, potential, pending input, archived) stop a layer.
The warm resume path copied the other protected branch stamps but not stack_base_branch, so a warmed successor of a stacked layer launched without the marker and protected the dependency PR's base instead of the dependency branch. Cover both resume paths in tests. Generated-By: PostHog Desktop Task-Id: f2cda6c1-9a0d-4deb-8ad2-f2d4dc239864
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.
The PR's own automated reviewer (posthog[bot]) surfaced multiple substantive, still-unresolved bugs — several tagged must_fix, one tagged security (reviewer identity can be spoofed onto a task), several around billing-exemption leakage and branch-protection gaps that could let automated pushes land on the wrong branch — and only 1 of 16 review threads shows as resolved. The change also enters billing and security-sensitive territory (billing exemptions, reviewer/task attribution, protected branch selection) with no clean, unresolved-concern-free independent approval to rely on.
- Unresolved must_fix: reviewer attribution can be lost when copying suggested reviewers to child layers, letting a task start under a named colleague's identity (security).
- Unresolved must_fix: dependency PR state 'unknown' is treated as ineligible for branch lookup but accepted by the gate check, letting a layer start on the wrong base branch.
- Unresolved must_fix: dismissed/suppressed child reports can still be auto-started once their dependency opens a PR.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 433L, 10F substantive, 748L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (748L, 16F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 1993b60 · reviewed head 1993b60 |
A layer must not inherit a reviewer list a person edited on the plan, because the copy loses the editor and auto-start could then run the layer as a named colleague. The skip already exists in _inherited_judgments; this locks it in. Generated-By: PostHog Desktop Task-Id: f2cda6c1-9a0d-4deb-8ad2-f2d4dc239864
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.
Multiple must-fix/should-fix issues raised by the automated reviewer on this diff are still unresolved (only 1 of 16 threads fixed) — including a case where a user's explicit dismissal of a layer can be overridden by the new auto-start path, billing exemptions not carrying over to child reports, and reviewer-attribution copying that can misattribute a task's actions to a named colleague. This is autonomous-task/billing/authorization territory with no independent human sign-off, so it can't go through on its own.
- Unresolved must_fix: dismissed layers can still auto-start once their dependency opens a PR, bypassing an explicit user dismissal (stack_plan.py).
- Unresolved must_fix: dependency PR state 'unknown' is treated as eligible for branch stacking, which can push the next layer onto the wrong base branch.
- Unresolved must_fix: child layer reports don't inherit the parent plan's billing exemption, so exempt system/scout plans can produce billable child PRs.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 433L, 10F substantive, 776L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (776L, 16F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ fd1d574 · reviewed head fd1d574 |
A layer was created with no billing exemption. The scout policy matches only the report the scout emitted, and the plan never runs, so every layer PR of a PostHog-system report was billable. Stamp the plan's stored or policy exemption on each layer at creation, before any run exists. Generated-By: PostHog Desktop Task-Id: f2cda6c1-9a0d-4deb-8ad2-f2d4dc239864
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 large, cross-team change to the autonomous self-driving pipeline (which base branch protects automated agent pushes, when child PRs auto-start) with 13 unresolved review threads, several must-fix/should-fix bugs, and no APPROVED review; critically, the author's own reply on the "plan fails re-research but its already-created children keep starting" finding says "the gap is real, but the fix is a design choice that a person must make" — an explicit, unaddressed safety gap in autonomous PR creation that only a human can resolve.
- Author-acknowledged unresolved gap: a plan's children stay READY and can still auto-start work after the plan itself is later marked unsafe/failed or reset to not-actionable during re-research — flagged must/should-fix, explicitly deferred by the bot author to a human design decision, not fixed in this diff.
- Several other should_fix/must_fix findings from the automated reviewer (unknown PR state allowed as a stacking base, no binding of the chosen base branch to a specific PR/run, failed layer starts treated as success with no retry path, daily report-quota not re-checked when layers appear) remain open per the 13 unresolved inline threads.
- No APPROVED review and no human/agent sign-off clearing the risky parts — all reviews are COMMENTED, and the bot's own resolution tracker shows most threads still open ('1 left for you').
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 438L, 10F substantive, 816L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (816L, 16F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 7f000a9 · reviewed head 7f000a9 |
start_unblocked_layers logged a failed layer start and returned, so the settle-point activity never retried the first layer and the plan stayed blocked. Keep trying the sibling layers, then raise once so the activity retry policy runs. A retry skips layers that already started. Generated-By: PostHog Desktop Task-Id: f2cda6c1-9a0d-4deb-8ad2-f2d4dc239864
A re-research of a report that already opened its own PR could return layers, and the first layer then started beside that PR. The plan_parent gate blocked any replacement of the parent PR. Skip layer creation when the report already has an open, draft, unknown or merged PR, so it stays on its single PR. Generated-By: PostHog Desktop Task-Id: f2cda6c1-9a0d-4deb-8ad2-f2d4dc239864
start_dependent_stack_layers never retried, so a transient failure left a layer waiting for the next PR event, which may come only at merge when the layer can no longer stack. Retry up to three times with backoff; start_unblocked_layers now raises after trying every sibling, and a retry skips layers that already started. Generated-By: PostHog Desktop Task-Id: f2cda6c1-9a0d-4deb-8ad2-f2d4dc239864
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.
The PR's own reviewer (posthog[bot]) flagged a must-fix bug — child layer reports don't inherit the parent plan's billing exemption, so exempt work becomes billable — plus several should-fix correctness gaps (layers keep starting after their plan fails safety review; stack base branch isn't verified against the actual PR/run), and the author's replies concede each gap is real but defer the fix to a human design decision rather than addressing it.
- Unaddressed must-fix: layer child reports don't copy billing_exempt_reason, so an exempt plan's layers can become billable work (products/signals/backend/stack_plan.py / temporal/summary.py).
- Unaddressed should-fix: a layer can still start after its parent plan fails safety review or is reset to not-actionable, using stale judgments.
- Unaddressed should-fix: dependency PR branch selection for stack_base_branch doesn't verify the PR belongs to the correct task run/repo, risking protecting the wrong branch on launch.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 453L, 10F substantive, 863L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (863L, 16F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ ceabc02 · reviewed head ceabc02 |
A layer with a missing scope, a title over the length cap, or a non-integer dependency failed pydantic validation before the plan fallback ran. The presentation turn has no retry, so the whole research output was lost. A before-mode validator now validates each raw layer and drops the plan, so the report continues as a single pull request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 5d69987a-d959-4e85-b268-fb417109fca4
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 a large, cross-team automation feature (stacked PR plans with branch-protection changes for automated task launches), and the PostHog Review bot found multiple must-fix/should-fix correctness gaps (stale-plan layers still starting, unbound stack base branch, dropped source-issue context) that the author's own replies acknowledge as real but explicitly defer as unresolved design decisions for a human to make.
- 5 unresolved inline review threads on products/signals/backend/stack_plan.py and temporal/summary.py flag real bugs (layers of a failed/reset plan still start; dependency_head_branch can bind to a stale/unrelated branch for the protected PR base; requested-research path never starts plan layers) that are acknowledged as valid in the author's own replies but left unfixed pending a human design decision.
- Author (posthog[bot]) is not on either owning team (@PostHog/team-self-driving, @PostHog/team-posthog-desktop) and this is a cross-team change, removing the owning-team assurance path.
- No approving review with no unresolved concerns exists on the current head — both CodeRabbit and the PostHog Review bot reviews are on older commits and raised issues that remain open.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 474L, 10F substantive, 886L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (886L, 16F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 301fc59 · reviewed head 301fc59 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
products/signals/backend/test/test_stack_plan.py (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the changed test signatures. Add
-> Noneat each site:
products/signals/backend/test/test_stack_plan.py#L75-L75: annotatetest_malformed_layer_is_no_plan.products/signals/backend/test/test_stack_plan.py#L183-L183: annotatetest_layers_inherit_only_a_pipeline_reviewer_list.products/signals/backend/test/test_stack_plan.py#L212-L212: annotatetest_layers_keep_the_plan_billing_exemption.products/signals/backend/test/test_stack_plan.py#L241-L241: annotatetest_a_report_with_its_own_pull_request_gets_no_layers.products/signals/backend/test/test_stack_plan.py#L266-L266: annotatetest_a_failed_layer_start_raises_after_its_siblings_are_tried.products/tasks/backend/tests/test_facade.py#L1206-L1206: annotatetest_run_task_resume_carries_self_driving_branch_stamp.As per coding guidelines, “Annotate every signature.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 70430efb-eb38-4d27-b33c-b36794d6015d
📒 Files selected for processing (7)
products/signals/backend/report_generation/research.pyproducts/signals/backend/stack_plan.pyproducts/signals/backend/tasks.pyproducts/signals/backend/test/test_stack_plan.pyproducts/tasks/backend/facade/api.pyproducts/tasks/backend/tests/test_facade.pyproducts/tasks/backend/tests/test_warm_facade.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 2 remain after this review.
| if not isinstance(v, list): | ||
| return v |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Drop non-list layer plans before field validation.
If the presentation returns "layers": null or an object, this branch passes the value to Pydantic. Validation then fails before layers_form_a_plan can discard the optional plan. The valid title and summary do not settle. Return [] for non-list values too.
…cked-report-plans-739d04 Generated-By: PostHog Desktop Task-Id: a618b307-4024-440c-8d2b-b895338a61a8
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 builds new orchestration for the self-driving agent pipeline — data model changes, automated task-launch branch selection, and safety-gate logic controlling when autonomous coding agents keep working — which is risky territory. The automated reviewer flagged several should_fix bugs on exactly that surface (notably: a plan marked unsafe/failed after re-research can still have its remaining layers auto-started using stale judgments) that the author explicitly acknowledged as real but left unresolved as "a person must decide," and these 5 threads remain unresolved on the current head alongside the one human approval.
- andrewm4894 reviewed the current head.
- Unresolved should_fix: layers of a plan that later fails safety review or is reset to not-actionable can still auto-start with stale judgments (products/signals/backend/temporal/summary.py, stack_plan.py) — author reply confirms the gap is real and defers the fix.
- Unresolved should_fix: dependent layers scheduled only via TaskRun output save, so PRs attached through claim_report or webhook-only state updates never trigger the start of waiting layers (products/signals/backend/receivers.py).
- Unresolved should_fix: stack base branch selection accepts any open PR on the dependency without verifying it belongs to the current run, so the automated launch can protect a stale/unrelated branch (products/signals/backend/stack_plan.py).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 474L, 10F substantive, 886L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (886L, 16F, single-area, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 148fbd4 · reviewed head 148fbd4 |
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/tasks/backend/facade/api.py-8374-8374 (1)
8374-8374: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch
stack_base_branchbefore reusing a warm run.
stable_selectionomitsstack_base_branch, soSandboxWarmer.warmcan reuse a run with a missing or different marker when the previous run has one. The reused run keeps that incorrect state._resolve_protected_base_branchthen falls back to the GitHub PR lookup instead of protecting the stored stack base, which can select the wrong PR base.Suggested fix
"pr_authorship_mode", "github_credential_source", + "stack_base_branch", }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 6d67471c-eed7-4ad6-82b7-4a9f1db6195a
📒 Files selected for processing (4)
products/signals/ARCHITECTURE.mdproducts/tasks/backend/facade/api.pyproducts/tasks/backend/models.pyproducts/tasks/backend/tests/test_facade.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Patch the team callback's imported function so lazy loading cannot retain a mock in the personal callback. Keep the existing personal integration assertions. Generated-By: PostHog Desktop Task-Id: a618b307-4024-440c-8d2b-b895338a61a8
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 touches automated task-launch/branch-protection logic and auto-start orchestration (risky, CI/deploy-adjacent territory), and multiple bot reviewers (CodeRabbit, posthog[bot]) flagged concrete, still-unfixed bugs on the current head — including a Major finding that a null/non-list "layers" value from the LLM will raise a validation error instead of degrading gracefully, and a warm-run-reuse path that doesn't match on the new stack_base_branch key, so a stale branch can be reused as the PR base. The author's own replies on two other threads explicitly say the gaps are real but "a person must decide" the fix, i.e. acknowledged, unresolved design gaps in an automated code-push pipeline.
- CodeRabbit (major, unresolved): research.py's
layersbefore-validator only special-cases non-list values by no-op, so alayers: null/object from research output reaches Pydantic'slist[ReportLayer]field validation and raises instead of falling back to no-plan. - posthog[bot] should_fix (unresolved, author replied 'Escalated — a person must decide'): stack_plan.py's dependency_head_branch/receivers.py only reschedule waiting layers on a TaskRun output save; PRs attached via claim_report or webhook-only updates never trigger schedule_dependent_layers, so a waiting layer can stall forever.
- posthog[bot] should_fix (unresolved, author replied 'Escalated'): no layer-start path reads the parent plan's status, so a plan that fails safety review or is reset to potential after re-research can still have its already-READY children start using stale inherited judgments.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 474L, 10F substantive, 896L/17F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (896L, 17F, two-areas, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 5b3146e · reviewed head 5b3146e |
|
/trunk merge |
| if incoming_links(team_id=team_id, report_id=parent.id, kinds=(ReportLinkKind.PART_OF,)): | ||
| return [] | ||
| if has_open_or_merged_pull_request(team_id=team_id, report_ids=[str(parent.id)]): | ||
| logger.info("signals.stack_plan.layers_skipped_for_own_pull_request", report_id=str(parent.id), team_id=team_id) | ||
| return [] |
There was a problem hiding this comment.
Do not create layers while the parent implementation is running
Issue description
The guard checks for a parent pull request, but not an implementation task that has started and has not opened a pull request. If new signals cause re-research during that window, the report can become a plan and start its first layer. The parent task can then open a second pull request for overlapping work.
Why we think it's a valid issue
- Checked: The parent guards in
create_layer_reports(products/signals/backend/stack_plan.py:103-107),has_open_or_merged_pull_request(products/signals/backend/typed_report_links.py:180-200), the READY re-research gate inassign_and_emit_signal_activity(products/signals/backend/temporal/grouping.py:869-893), the settle step that starts the first layers (products/signals/backend/temporal/summary.py:1084-1088), the child's link gates_evaluate_link_gates(products/signals/backend/auto_start.py:575-620), and the supersession checks (products/signals/backend/supersession.py:330-342). - Found: A READY report re-researches when a new signal brings it to its next research bucket. The gate at grouping.py:870-893 checks only the bucket and the signal count. It does not check for a running implementation task or an active claim. The supersession code (supersession.py:330-342, auto_start.py:646-661) exists because re-research while an implementation is live is an expected path.
- Found:
has_open_or_merged_pull_requestreads only theSignalReportPullRequestrows that are attached to the report (typed_report_links.py:199-200). A run that started and has not attached a PR yet reads as "no work".create_layer_reportsthen creates the children. - Found: At settle, the parent gets the
plan_parentskip, andstart_unblocked_layers_of_planstarts layer 0, which has no dependencies (summary.py:1084-1088). The child's link gates check onlyduplicate_of,depends_on, andplan_parent(auto_start.py:586-619). No gate checks the running task of thepart_ofparent. - Found: The docstring at stack_plan.py:97-98 says that the layers must not repeat the parent's own PR work. The code keeps this rule only after the PR is attached, and not while the run is in flight.
- Impact: A concrete trigger exists: a report auto-starts, and during the run a signal reaches the next bucket (default
1,2,4,10). The re-research returnslayerswhen the earlier pass did not. This can happen for reports that were researched before this PR, or when the model splits the work on a later pass. The old task then opens a full-scope PR on the parent, and layer 0 opens an overlapping PR. The later layers stack on top of layer 0. The result is duplicate PRs and wasted runs for the same work, which the design means to prevent.
Suggested fix
Before creating layers, use the parent's associated implementation tasks to check for a nonterminal run. Keep the report on its existing implementation path while that run is active.
Prompt to fix with AI (copy-paste)
## Context
@products/signals/backend/stack_plan.py#L103-107
<issue_description>
The guard checks for a parent pull request, but not an implementation task that has started and has not opened a pull request. If new signals cause re-research during that window, the report can become a plan and start its first layer. The parent task can then open a second pull request for overlapping work.
</issue_description>
<issue_validation>
- **Checked:** The parent guards in `create_layer_reports` (products/signals/backend/stack_plan.py:103-107), `has_open_or_merged_pull_request` (products/signals/backend/typed_report_links.py:180-200), the READY re-research gate in `assign_and_emit_signal_activity` (products/signals/backend/temporal/grouping.py:869-893), the settle step that starts the first layers (products/signals/backend/temporal/summary.py:1084-1088), the child's link gates `_evaluate_link_gates` (products/signals/backend/auto_start.py:575-620), and the supersession checks (products/signals/backend/supersession.py:330-342).
- **Found:** A READY report re-researches when a new signal brings it to its next research bucket. The gate at grouping.py:870-893 checks only the bucket and the signal count. It does not check for a running implementation task or an active claim. The supersession code (supersession.py:330-342, auto_start.py:646-661) exists because re-research while an implementation is live is an expected path.
- **Found:** `has_open_or_merged_pull_request` reads only the `SignalReportPullRequest` rows that are attached to the report (typed_report_links.py:199-200). A run that started and has not attached a PR yet reads as "no work". `create_layer_reports` then creates the children.
- **Found:** At settle, the parent gets the `plan_parent` skip, and `start_unblocked_layers_of_plan` starts layer 0, which has no dependencies (summary.py:1084-1088). The child's link gates check only `duplicate_of`, `depends_on`, and `plan_parent` (auto_start.py:586-619). No gate checks the running task of the `part_of` parent.
- **Found:** The docstring at stack_plan.py:97-98 says that the layers must not repeat the parent's own PR work. The code keeps this rule only after the PR is attached, and not while the run is in flight.
- **Impact:** A concrete trigger exists: a report auto-starts, and during the run a signal reaches the next bucket (default `1,2,4,10`). The re-research returns `layers` when the earlier pass did not. This can happen for reports that were researched before this PR, or when the model splits the work on a later pass. The old task then opens a full-scope PR on the parent, and layer 0 opens an overlapping PR. The later layers stack on top of layer 0. The result is duplicate PRs and wasted runs for the same work, which the design means to prevent.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Before creating layers, use the parent's associated implementation tasks to check for a nonterminal run. Keep the report on its existing implementation path while that run is active.
</potential_solution>
Problem
Refs #102776
Origin
Changes
Why: the issue's final layer. The pipeline must turn a phased issue into ordered, stacked PRs instead of overlapping work.
layersplan (2 to 6 layers). A dependency always points at an earlier layer, so the order stays acyclic.part_ofthe plan anddepends_onits dependency.stack_base_branchrun state key makes the launch protect that branch as the PR base. Without it, the launch would see the branch heads an open PR, protectmaster, and let the layer push into the layer below.Note
Nothing changes for reports without
layers. The layers appear as normal reports in the inbox list.Not in this PR, left as follow-ups under #102776: stack-wide supersession, server-side GitHub stack linking, and the collapsed inbox list.
How did you test this code?
test_stack_plan.py(new): layer validation, child creation and links, idempotent re-research, first-layer start, a dependent layer that starts only after its dependency opens a PR, no double start, and base branch selection for open, draft, and merged dependencies.test_auto_start.py: thedepends_on_with_open_prcase now asserts the task starts on the dependency branch withstack_base_branchand the base instruction.test_start_agent_server.py: a stacked run protects its own start branch, and a moved run falls back to the open-PR lookup.hogli lint:tachpasses.Release status
Automatic notifications
Docs update
Updated
products/signals/ARCHITECTURE.mdwith a "Stacked plans" section and corrected the note on blocked reports.🤖 Agent context
Autonomy: Fully autonomous
Agent: PostHog Desktop, Claude Opus 5.5 (
claude-opus-5-5)writing-tests,writing-pr-descriptions. The local CodeRabbit pass is skipped in cloud runs.master, so the tasks-side state key was added.Created with PostHog Desktop from this inbox report.
🤖 Generated with Claude Code