Repository navigation
feat(stamphog): wait for and count PostHog Review through a backend evidence hook - #115425
trunk-io[bot] merged 15 commits into
Conversation
PostHog Review · Standard · reviewed
|
| Step | Status | Result |
|---|---|---|
| Prepare the diff | done | 40 files |
| Main review and lenses | done | 6 issues |
| Merge and cap | done | 6 → 0 |
| Publish | done | Nothing to post |
This pull request is large, so the review ran in 4 parts with less depth than usual.
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/develocity/source.py:1 |
products/warehouse_sources/backend/temporal/data_imports/sources/logrocket/source.py:1 |
21 | 157 |
products/review_hog/backend/reviewer/tools/github_meta.py:368 |
products/review_hog/backend/reviewer/tools/github_meta.py:518 |
16 | 115 |
products/warehouse_sources/backend/temporal/data_imports/sources/humanitec/humanitec.py:14 |
products/warehouse_sources/backend/temporal/data_imports/sources/logrocket/logrocket.py:27 |
11 | 85 |
products/warehouse_sources/backend/temporal/data_imports/sources/logrocket/logrocket.py:25 |
products/warehouse_sources/backend/temporal/data_imports/sources/loops/loops.py:13 |
11 | 85 |
products/review_hog/backend/temporal/__init__.py:2 |
products/review_hog/backend/temporal/workflow.py:83 |
33 | 72 |
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
⚠️ Comment density — 6% of added code lines are comments (105 of 1762)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
products/review_hog/backend/head_review_status.py |
14 | 182 |
products/stamphog/backend/tests/test_integration.py |
14 | 315 |
products/stamphog/backend/facade/review_evidence.py |
12 | 50 |
products/stamphog/backend/temporal/workflow.py |
11 | 34 |
products/review_hog/backend/facade/review_status_types.py |
7 | 25 |
products/review_hog/backend/tests/test_head_review_status.py |
6 | 246 |
products/review_hog/backend/receivers.py |
4 | 29 |
products/stamphog/backend/tasks/tasks.py |
4 | 94 |
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 — 76 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ██████████████████░░ 91.0% (846 / 922)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/review_hog/backend/temporal/client.py |
40.0% | 106–108, 110–111, 114–120 |
products/review_hog/backend/temporal/activities.py |
57.4% | 665–667, 675–677, 2506–2511, 2515–2522, 2536, 2544, 2563 |
products/review_hog/backend/reviewer/tools/github_meta.py |
66.7% | 254–256, 362, 369, 371–372 |
products/review_hog/backend/facade/review_status_types.py |
68.8% | 18, 20, 22, 24–25 |
products/review_hog/backend/reviewer/tools/github_client.py |
82.1% | 182, 186, 228–230 |
products/review_hog/backend/reviewer/status_comment.py |
85.2% | 765, 768, 784–785 |
products/review_hog/backend/temporal/workflow.py |
86.5% | 1163–1165, 1190–1191 |
products/stamphog/backend/tasks/tasks.py |
88.2% | 1737, 1743, 1747, 1749, 1753, 1798 |
products/review_hog/backend/head_review_status.py |
93.5% | 42, 44, 87, 124, 128, 167 |
products/stamphog/backend/temporal/activities.py |
95.0% | 757, 810, 1647 |
🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 191531296898004 -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 |
demo |
███████████░░░░░░░░░ 55.8% |
1,506 / 2,699 |
data_tools |
████████████░░░░░░░░ 61.2% |
90 / 147 |
warehouse_sources_queue |
█████████████░░░░░░░ 66.8% |
1,922 / 2,878 |
aeo |
███████████████░░░░░ 76.3% |
617 / 809 |
batch_exports |
████████████████░░░░ 80.1% |
21,420 / 26,728 |
github_commands |
█████████████████░░░ 83.7% |
977 / 1,167 |
apm |
█████████████████░░░ 84.1% |
1,306 / 1,553 |
engineering_analytics |
█████████████████░░░ 84.9% |
11,355 / 13,374 |
mcp_analytics |
█████████████████░░░ 85.7% |
4,844 / 5,651 |
ml_inference |
█████████████████░░░ 86.3% |
524 / 607 |
warehouse_suggestions |
█████████████████░░░ 87.3% |
2,252 / 2,580 |
posthog_ai |
██████████████████░░ 87.7% |
4,226 / 4,817 |
notebooks |
██████████████████░░ 88.4% |
15,201 / 17,196 |
today |
██████████████████░░ 88.6% |
2,866 / 3,233 |
cdp |
██████████████████░░ 88.7% |
5,081 / 5,729 |
mcp_registry |
██████████████████░░ 89.0% |
1,596 / 1,794 |
product_tours |
██████████████████░░ 89.1% |
1,337 / 1,500 |
dashboards |
██████████████████░░ 89.4% |
7,322 / 8,194 |
webmcp |
██████████████████░░ 89.6% |
240 / 268 |
cohorts |
██████████████████░░ 89.6% |
8,458 / 9,438 |
signals |
██████████████████░░ 90.0% |
69,565 / 77,329 |
data_modeling |
██████████████████░░ 90.1% |
10,856 / 12,043 |
ai_training |
██████████████████░░ 90.4% |
349 / 386 |
tasks |
██████████████████░░ 90.6% |
80,813 / 89,188 |
visual_review |
██████████████████░░ 90.7% |
9,842 / 10,856 |
data_warehouse |
██████████████████░░ 90.8% |
14,831 / 16,339 |
canvas |
██████████████████░░ 91.0% |
7,804 / 8,580 |
exports |
██████████████████░░ 91.0% |
9,763 / 10,732 |
business_knowledge |
██████████████████░░ 91.0% |
8,791 / 9,656 |
autoresearch |
██████████████████░░ 91.1% |
10,436 / 11,453 |
managed_warehouse |
██████████████████░░ 91.2% |
11,053 / 12,114 |
wizard |
██████████████████░░ 91.8% |
6,008 / 6,548 |
streamlit_apps |
██████████████████░░ 91.9% |
3,139 / 3,415 |
conversations |
██████████████████░░ 92.0% |
29,714 / 32,284 |
stamphog |
██████████████████░░ 92.5% |
8,593 / 9,291 |
early_access_features |
███████████████████░ 92.6% |
1,339 / 1,446 |
managed_migrations |
███████████████████░ 92.6% |
1,630 / 1,760 |
alerts |
███████████████████░ 92.9% |
7,246 / 7,797 |
review_hog |
███████████████████░ 92.9% |
16,502 / 17,754 |
surveys |
███████████████████░ 93.1% |
6,626 / 7,118 |
web_analytics |
███████████████████░ 93.1% |
24,096 / 25,877 |
notifications |
███████████████████░ 93.2% |
1,152 / 1,236 |
approvals |
███████████████████░ 93.4% |
4,526 / 4,848 |
cross_project_dashboards |
███████████████████░ 93.4% |
880 / 942 |
slack_app |
███████████████████░ 93.5% |
14,854 / 15,882 |
context_layer |
███████████████████░ 93.7% |
3,409 / 3,640 |
marketing_analytics |
███████████████████░ 93.8% |
20,072 / 21,402 |
workflows |
███████████████████░ 93.8% |
17,061 / 18,188 |
messaging |
███████████████████░ 93.8% |
4,468 / 4,762 |
billing_alerts |
███████████████████░ 93.9% |
2,090 / 2,226 |
customer_analytics |
███████████████████░ 93.9% |
26,079 / 27,773 |
mcp_store |
███████████████████░ 93.9% |
9,008 / 9,593 |
experiments |
███████████████████░ 94.1% |
33,981 / 36,101 |
ai_observability |
███████████████████░ 94.2% |
26,434 / 28,069 |
replay_vision |
███████████████████░ 94.3% |
30,070 / 31,884 |
legal_documents |
███████████████████░ 94.3% |
2,289 / 2,427 |
logs |
███████████████████░ 94.5% |
15,850 / 16,770 |
actions |
███████████████████░ 94.6% |
973 / 1,029 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
endpoints |
███████████████████░ 94.8% |
9,333 / 9,847 |
skills |
███████████████████░ 94.8% |
7,061 / 7,445 |
growth |
███████████████████░ 94.8% |
11,268 / 11,880 |
metrics |
███████████████████░ 95.0% |
6,733 / 7,089 |
annotations |
███████████████████░ 95.0% |
816 / 859 |
tracing |
███████████████████░ 95.2% |
3,631 / 3,815 |
data_catalog |
███████████████████░ 95.5% |
4,419 / 4,628 |
access_control |
███████████████████░ 95.6% |
7,881 / 8,245 |
product_analytics |
███████████████████░ 95.7% |
28,723 / 29,999 |
alerts_platform |
███████████████████░ 95.8% |
5,013 / 5,234 |
revenue_analytics |
███████████████████░ 95.9% |
1,878 / 1,959 |
data_quality |
███████████████████░ 95.9% |
8,569 / 8,933 |
feature_flags |
███████████████████░ 96.2% |
26,932 / 27,997 |
warehouse_sources |
███████████████████░ 96.4% |
458,614 / 475,748 |
security |
███████████████████░ 96.7% |
1,452 / 1,501 |
pulse |
███████████████████░ 97.4% |
2,023 / 2,078 |
analytics_platform |
████████████████████ 98.0% |
2,775 / 2,833 |
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.
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
c05cd45 to
c07f2f1
Compare
c07f2f1 to
db1f524
Compare
|
Risk: No findings This delta tightens the external-review evidence contract: a completed PostHog Review turn now also reports as skipped the files of every pipeline chunk that no review unit finished, and stamphog retries (rather than skips) the post-WAIT re-review dispatch when an evidence provider that previously reported pending now errors. Both changes move only in the conservative direction — more skipped paths disqualifies evidence as approval assurance, and the retry keeps the PR at WAIT — and I found no path where either enables an earlier or unwarranted approval. Sentinel reviewed |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
f8e8bac to
72a5681
Compare
Stamphog escalated a one-line change for "risky territory without independent assurance" while PostHog Review ran a Deep review of the same head and found nothing. Two gaps: nothing made stamphog wait for that review, and only current-head GitHub reviews counted as assurance, while a clean ReviewHog turn posts no review at all. - Add a provider contract in the stamphog facade (facade/review_status.py): (repository, pr_number, head_sha) -> ReviewStatus | None, per head, with pending / completed / failed. ReviewHog registers it at app-ready, same pattern as inbox_hooks.py, so stamphog never imports review_hog. - The bot-wait poll reads the status every round and stores it on the run. A pending review keeps the run waiting within the existing budget; if it is still pending at the end, the engine returns WAIT, never APPROVE. - After any wait, re-read reviews, discussion and threads (refresh_review_evidence, behind the stamphog-refresh-after-wait patch). The context fetch runs before the wait, so the engine used to judge stale evidence next to a fresh status. - A completed review of the current head counts as current-head assurance in _cap_assurance_facts, and shows in the trusted assurance line and the posted verdict. It comes from server metadata, never PR text. Published findings are review threads, so the unresolved-thread rule still applies. - No provider, a provider error, another head, or failed: behave as before. - Drop posthog[bot] from the trusted reactor sets again. The backend status replaces its 👀; the reaction stays as a cue for people.
Stamphog now asks a registered provider where the automated review of the head it judges stands, so it can wait for a running review and count a finished one as assurance. ReviewHog is that provider. - head_review_status.py answers per head, not per PR: PRStatusLookup reads "queued" for the whole PR, which would make stamphog wait on a review of another commit. - Pending: a request for the head waits in the PR's review queue, or a turn on the head runs (report ACTIVE, fresh, head_sha matches). Publishing turns stay ACTIVE until publish, so pending holds through the publish window and the findings are on the PR before stamphog reads "completed". - Waiting requests exist only in the queue workflow, so the queue gets a requested_heads query (replay-safe, no new commands). A request with no head counts unless a turn already runs on another head, since that request is most likely the running turn. Temporal errors raise, and stamphog treats a raising provider as "no review known". - Completed: newest counted turn for the head (turn marker, or completed_head_sha for the latest turn), with Standard/Deep, the valid findings count and whether that turn published at the head. - Failed: a failed review outcome marker for the head with no later counted turn, or one on the counted turn itself (failed publish). - No owning project for the repository: None. - The provider registers next to the inbox hook through a thin wrapper that imports the lookup on first use, keeping temporalio and the LLM SDKs off the django.setup() path.
Deep reviews take longer than stamphog's wait budget, so a stamphog run usually ends in WAIT while the Deep review is still going, and nothing ran stamphog again afterwards. The PR stayed unapproved until a push or a manual re-request. - Stamphog facade: request_rereview_after_external_review(repository, pr_number, head_sha) queues a Celery task. The task acts only when the PR's latest run is COMPLETED with a WAIT verdict at that head, the head is still the live head, the run is not an inbox run, and the webhook gates pass again (open, payload pre-filters, trigger label unless the run was manual, author write access). Everything else is a no-op. - The run is created through _queue_review_at_head with replacing_run_id, so the head dedupe skips the WAIT run but still collapses repeated nudges into one run. The new run keeps manual/trigger provenance and is marked rereview_after_external_review in its output. No new trigger enum, so no OpenAPI change. - ReviewHog: nudge_stamphog_activity runs after publish (posted or clean) and after the failure is recorded, behind the reviewhog-nudge-stamphog-2026-10 patch. Fail-open at both the activity and the workflow level, so a stamphog problem never fails a turn. - Only WAIT is handled. ESCALATE caused by missing assurance is not detectable from stored fields without parsing issue text, so it is left alone (TBD if it turns out to matter).
…ewers The status hook was shaped around one reviewer. Other review products should be able to plug in the same way, and other PostHog products (Inbox, desktop, MCP pr-status) need PostHog Review's per-head truth without going through stamphog. - review_hog owns a public read API: facade/review_status.py review_status_for_head(repository, pr_number, head_sha) -> HeadReviewStatus | None, with the contract in facade/contracts.py. HeadReviewStatusLookup stays the implementation behind it. - Stamphog's facade/review_status.py becomes facade/review_evidence.py, a registry of providers returning ReviewEvidence | None. Every poll asks all of them: any pending one keeps the run waiting, each completed one on the current head counts as assurance with its own line, and a raising provider is logged and skipped without hiding the others. Stored as run.output["review_evidence"], a list. - review_hog registers review_evidence_for_stamphog, a thin adapter that calls its own facade and names the reviewer "PostHog Review". Still imported lazily to keep the setup import budget. - Stamphog code no longer names ReviewHog outside docs. - Move the re-review request body into tasks/tasks.py and re-export it from facade/tasks.py, which product:lint requires.
A facade/contracts.py module moves a product onto the strict product lint, and review_hog does not meet those rules yet (no tsconfig.json, backend/tasks.py location). The types keep their content under a neutral module name.
Review findings on the status hook, all on the ReviewHog side: - A waiting request without a head (a label review) queued behind a turn on an older head was read as that running turn, so stamphog went ahead while a review of the live head was still queued. The queue query now returns the active and the waiting requests apart; a waiting headless request counts as pending for the live head. - An eval or CLI turn (run_review without publish) with valid findings reported COMPLETED, so stamphog counted a review nobody posted. The turn marker now records whether the turn publishes, and only publishing turns produce COMPLETED. A clean publishing turn still counts. Markers from before the field count only when their turn posted. - A failure marker from an earlier attempt of the same run index made a successful retry read as FAILED. A failure older than the turn's newest marker (written when the last attempt started) is now ignored. - The turn marker also records the base branch and the files PRFilter left out (from the turn's PR snapshot), and the evidence contract carries them as base_ref and skipped_paths, so stamphog can drop a review of another base or of fewer files. - nudge_stamphog_activity swallowed dispatch errors, so Temporal never retried them. They now raise; the workflow still catches the final ActivityError. No migration: the new fields live in the JSON content of existing artefacts and default for older rows.
…d files Two gaps in how the engine counted a completed external review as whole-PR assurance: - A retarget keeps the head SHA but changes the diff, so a review of the old base still counted. Completed evidence now counts only when its base_ref matches the PR's current base; an unknown base never counts. - ReviewHog skips some files (lock, generated, minified, and test files on the pipeline design). The review was still added as assurance for a risky area inside such a file. _cap_assurance_facts now skips the automatic assurance when a risky area names a skipped path, and the assurance line says how many files the review skipped. The server stores base_ref and skipped_paths from the provider answer next to the other evidence fields.
…n reliable Review findings on the stamphog side of the PostHog Review hook: - Evidence refresh: when the first poll already found a completed external review, the workflow did not wait and so skipped refresh_review_evidence. The engine then counted the review over threads read before it finished. The poll now also reports reviews_completed, and the refresh runs after any wait or when a review completed. Same patch id, so old histories replay unchanged. - Re-run dedupe: after a re-run itself ended in WAIT, the next nudge excluded only that run and found the original WAIT as covering the head, so nothing queued. _queue_review_at_head now takes revisit_wait, which ignores every delivered WAIT at the head and still dedupes against a live run. - Retries: rereview_after_external_review retries GitHub and Temporal errors with backoff (rate limits honor retry_after, 60s floor). A re-run left QUEUED by a failed workflow start counts as recoverable, so a retry or a later nudge restarts it instead of stopping at it. - Lost nudge: a nudge that arrives while the run is still in flight finds no WAIT and does nothing, and the run then posts WAIT from its stale pending snapshot. post_verdict now asks the providers once more after it saves a WAIT that waited on a pending review, and queues the re-review when that review has finished. Best-effort, since the verdict is already posted.
fetch_pr_files grew a third return value for the skipped paths, so the devex semgrep rule asks for named fields instead of a positional tuple.
…he same merge base Review feedback on the external review evidence path. All four gaps could let stamphog approve on assurance that did not hold, or leave a PR at WAIT. - Coverage: the engine withheld assurance only when a skipped path showed up verbatim in the model's free-text risky area, which misses basenames and abbreviations. A completed review now counts only when it skipped no file. ReviewHog also reports files GitHub sent without a patch (large files) as skipped, since the review never saw their changes. The verdict line says why such a review does not count. - Provider errors: a provider that errors after it reported pending in the same run keeps its pending answer (marked provider_error), so the run ends in WAIT instead of approving. An error before any answer still adds no evidence. - Stale base: evidence was bound to the base branch name. ReviewHog now reads the merge base on every PR turn, stores it in the turn marker and sends it as base_sha. The engine compares it with the merge base stamphog computed for the run; unknown or different means no assurance. base.sha is not used because it moves on every merge to the base branch. - WAIT recovery: the re-review dispatch after a WAIT ran inside post_verdict and swallowed Celery errors, so a broker outage left the PR at WAIT. It is now its own activity with the activity retry policy, behind the stamphog-rereview-activity patch; the workflow catches the final failure because the verdict is already posted.
A binary file shows 0 changed lines and no patch, so the review never sees it. Counting it as reviewed would let stamphog take the review as full coverage. Only a pure rename has no change to review.
72a5681 to
dfec747
Compare
The post-WAIT recheck read the providers with no earlier evidence, so a provider error looked like a finished review and queued a re-review. That re-review starts without the pending answer, and with continued errors it could approve while the external review still runs. The recheck now passes the run's own evidence, so an error keeps the review pending and the reviewer's completion nudge stays the trigger.
ReviewHog's nudge can land before the WAIT is saved and find nothing to re-review. If the recheck then hits a provider error, returning success leaves the PR at WAIT until another trigger. Raising lets Temporal retry the recheck while the pending evidence stays in place.
…ted turn A pipeline turn fans out one unit per (perspective, chunk) plus a blind-spot unit per chunk, and the failure floor only fails the run when more than 70% of the units fail. Every unit on one chunk can fail and the turn still completes. The head status then reported empty skipped_paths, and stamphog reads a completed review with empty skipped_paths as whole-PR assurance, even though the files in that chunk were never read. - Add the files of every chunk in the head's chunk set that has no persisted perspective or blind-spot result to skipped_paths. - Any reviewer arm counts as a read; single-agent passes do not. - Single-agent Standard turns are left alone: their main session reviews the whole diff and its failure fails the turn, so lens failures leave no file unread. - A pipeline turn without a chunk set at the head (older or pruned data) keeps the marker's skipped_paths, so no gaps are invented.
|
😎 Stack merged successfully - details. |
Problem
Changes
Stamphog and PostHog Review run in the same backend, so stamphog now asks for the review state directly. The dependency still points one way: review_hog → stamphog facade. Stamphog imports nothing from review_hog.
flowchart LR RH{{PostHog Review}} -- registers provider at startup --> REG[stamphog evidence registry] SH{{Stamphog review run}} -- asks per head --> REG REG -- pending / completed / failed --> SH RH -- turn finished: re-review a WAIT --> NUDGE[stamphog facade nudge] NUDGE --> SH classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class RH,SH phBlue; class REG,NUDGE phGray;facade/review_evidence.py). Any reviewer can register a provider. A provider answers for one PR commit:pending,completed(kind, findings count, posted or not), orfailed.review_status_for_head(...)as a public facade read, so other products can use it later. It answers per commit, from:requested_headsquery on the per-PR queue workflow);request_rereview_after_external_reviewon stamphog's facade. Stamphog re-runs only if its latest run on that exact head ended in WAIT and the head is still the PR's head. Repeated nudges create at most one run.posthog[bot]comes back off stamphog's 👀 lists from feat(review_hog): react while reviewing, drop the Standard status comment #115294. The 👀 stays a cue for people only.stamphog-refresh-after-waitandreviewhog-nudge-stamphog-2026-10.Warning
This changes when stamphog approves. A PR in risky territory can now pass on a completed PostHog Review instead of escalating, and a still-running review always ends as WAIT.
Not covered, and accepted:
How did you test this code?
Test rationale:
product:lint stamphogpasses.product:lint review_hogreports two existing issues on master (tsconfig.json, thebackend/tasks.pylocation), unchanged here.Not checked: a live run of both bots on a real PR.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Docs update
products/stamphog/AGENTS.md, the stamphog engine README, andproducts/review_hog/ARCHITECTURE.mddescribe the contract and its direction.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5
/writing-dataclasses,/writing-tests,/writing-code-comments,/writing-user-facing-copy,/writing-pr-descriptions.