Repository navigation
refactor(alerts): reduce investigation loop complexity - #100093
trunk-io[bot] merged 2 commits into
Conversation
|
😎 Stack merged successfully - details. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
posthog/temporal/ai/anomaly_investigation/runner.py |
26 | 224 |
This check does not block merging. It updates on every push and clears when the share drops.
✅ ClickHouse migration SQL — none
No ClickHouse migrations in the latest push.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: ✅ APPROVE (round 4 @ 4b3a448)Two more commits landed since round 3: the defensive per-run state reset, and consolidating that state's initialization into Key findings
Convergence
Reviewer summaries
Previous rounds (3)round 1 @ fd6d065 — ✅ APPROVE: no correctness, security, or style issue found. Automated by QA Swarm — not a human review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd6d065d0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
pauldambra
added
the
stamphog
PostHog
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Pure internal refactor extracting helper functions from a Temporal anomaly-investigation loop; tracing the diff confirms the tool-budget, retry, salvage, and fallback control flow is preserved unchanged. The two unresolved Codex comments are a style preference (class vs. functions) and a lost explanatory comment — non-substantive per the rubric, not correctness or security issues, and this is not risky territory.
- Codex flagged (P1, unresolved): prefers the extracted loop state be encapsulated in a class per repo convention (AGENTS.md) — style preference, not a correctness issue.
- Codex flagged (P1, unresolved): the refactor dropped an explanatory comment about why LLM finalize failures are swallowed rather than left to bubble up to Temporal retries — worth restoring for future maintainers but behavior itself is unchanged.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 396L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (396L, 1F, single-area, refactor) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ f38efdb · reviewed head f38efdb |
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 1 consider. Published 2 findings (view the review). Resolved comments: 1 fixed, 1 left for you |
Addresses review feedback on #100093: threading messages, report_args_history, and tool_calls_used through several loose functions risked one path omitting or propagating the state inconsistently. Moves the extracted operations onto a private _InvestigationRunner class that owns that state, and restores the comment explaining why a failed finalize call returns a fallback report instead of raising (an exception would trigger a costly full activity retry). Generated-By: PostHog Desktop Task-Id: bd7e5ab4-ee17-419c-a28d-702360e9805a
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Internal refactor of a single Temporal worker file that preserves the LLM tool-calling/report-validation/salvage/fallback behavior verbatim (verified by diffing old vs new control flow); both prior Codex P1 concerns (extract to a class, retain retry rationale comment) are already addressed in the current head. Not risky territory (no schema, API contract, auth, billing, dependency, or CI changes), so no independent assurance is required.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 435L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (435L, 1F, single-area, refactor) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ b00a225 · reviewed head b00a225 |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Pure behavior-preserving refactor of one internal file; I traced both bot-flagged concerns against the diff and confirmed the precondition comment was preserved (reworded, relocated) and the salvage asymmetry the bot called out exists identically in the pre-refactor code, so it's not a regression this PR introduces. Not risky territory, so no independent assurance is required.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 440L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (440L, 1F, single-area, refactor) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ a1ace6f · reviewed head a1ace6f |
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Verified line-by-line that the refactor is behaviorally identical to the prior inline loop — extracted helpers and a stateful runner class reproduce every branch (tool-budget finalize, retry, salvage, fallback) exactly, including the pre-existing salvage asymmetry a bot flagged, which is unchanged from master and already triaged as out-of-scope. Tests/mypy/CI checks reported passing and an independent bot review approved twice with no outstanding correctness issues.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 428L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (428L, 1F, single-area, refactor) |
| stamphog 2.0.0 | .stamphog/policy.yml @ f07517e · reviewed head f07517e |
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Pure internal refactor of the anomaly-investigation loop into a class with helper methods; comparing old and new code shows behavior is preserved and the one real behavior bug reviewers found (tool_calls_used defaulting to 0 on the LLM-invoke-failure path) is already fixed by routing all returns through the new _result() helper. The remaining flagged issue (salvage-vs-fallback asymmetry) is unchanged from master, explicitly disclosed as preserved behavior, and the reviewer/author thread already agreed to fix it in a separate stacked PR rather than block this one — no unresolved regression, and the PR was covered by three rounds of automated review plus targeted unit tests.
- Pre-existing (not introduced here) asymmetry: the loop's LLM-invoke-failure path returns a bare fallback report without consulting report_args_history for salvage, unlike the tool-budget-exhaustion path — author and reviewer agreed to address this in a follow-up stacked PR rather than in this refactor.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 425L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (425L, 1F, single-area, refactor) |
| stamphog 2.0.0 | .stamphog/policy.yml @ 85f9d0c · reviewed head 85f9d0c |
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Pure internal refactor of the anomaly-investigation loop into a class-based structure, verified line-by-line to preserve prior control flow (retry, salvage, fallback, tool budget) exactly; it doesn't touch schemas, APIs, deps, CI, or auth. The one flagged asymmetry (LLM-invoke-failure path skipping salvage) is confirmed pre-existing on master, not introduced here, and was already surfaced by an automated reviewer and explicitly deferred to a follow-up human decision, which the author acknowledged.
- Pre-existing, non-regressing asymmetry: run()'s LLM tool-call invoke failure path returns a bare fallback report without consulting report_args_history, unlike the budget-finalize failure path which salvages. Already flagged by @PostHog[bot] and escalated separately for a human policy decision on verdict handling; not a blocker for this refactor PR but worth a prompt follow-up.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 427L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (427L, 1F, single-area, refactor) |
| stamphog 2.0.0 | .stamphog/policy.yml @ fa36843 · reviewed head fa36843 |
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Careful line-by-line comparison confirms this is a faithful extract-to-class refactor of the anomaly investigation loop with no behavioral changes; the one outstanding defect (salvage-path asymmetry) is pre-existing on master, not introduced by this diff, and was already triaged separately by the bot reviewer as out-of-scope for this PR. A prior latent state-reuse concern raised by the QA swarm review has since been fixed (tool_calls_used and report_args_history are both reset together).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 429L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (429L, 1F, single-area, refactor) |
| stamphog 2.0.0 | .stamphog/policy.yml @ 376a5f0 · reviewed head 376a5f0 |
be3d01c to
3efe755
Compare
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3efe755 to
3f05401
Compare
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3f05401 to
553cded
Compare
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0593fe1 to
bcf35a6
Compare
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
🚨 This push destroyed the test runs of 12 other PRsThe SQL insight value (pauldambra pushed to this PR while it was being tested in the Trunk merge queue. That push threw this PR out of the queue and killed the test run of every PR queued behind it. All 12 of them now have to run the full test suite again, from scratch: #88545, #100269, #103522, #104360, #105695, #106067, #106303, #106321, #106330, #106331, #106376, #106388 Once a PR enters the merge queue it is finished. Do not touch it again. Let it merge. If something still has to change, put it on a new branch and open a new PR. A push from a local checkout is blocked before this can happen, but only when the Trunk CLI is logged in. Run |
bcf35a6 to
009c948
Compare
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
009c948 to
d8e9988
Compare
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Line-by-line diff comparison shows the new _InvestigationRunner class preserves the original loop's control flow (budget-exhaustion finalize retry, tool-call handling, salvage/fallback paths) exactly — it's a mechanical extraction into helpers/methods, not a behavior change. It's internal alert-investigation tooling, not risky territory, and it already has tests plus multiple automated review passes (Codex, CodeRabbit, an internal QA bot that iterated 4 rounds and caught/fixed two real bugs along the way) converging clean with only a cosmetic nitpick outstanding.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 452L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (452L, 1F, single-area, refactor) |
| stamphog 2.1.0 | .stamphog/policy.yml @ d8e9988 · reviewed head d8e9988 |
d8e9988 to
60c58bf
Compare
Problem
Changes
How did you test this code?
uv run pytest -q posthog/temporal/tests/ai/test_anomaly_investigation_runner.py.hogli ci:preflight --strictafter stacking.Release status
Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Slack app (original PR); Codex, GPT-6 (stacking pass)
/stacking-prsand/writing-pr-descriptions.Created with PostHog from a Slack thread