refactor(e2e): remove bob_the_builder; drive remediation from a Grafana alert (provisioned outside the repo) - #33749
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
|
Greptile SummaryThis PR removes
Confidence Score: 5/5Safe to merge — the change is a clean removal of a fragile module and its hook, with no production code path affected. The only runtime change is removing a try/except block that was already fully swallowing every exception, so the suite's exit status and all other conftest behaviour are identical before and after. The new Grafana YAML is inert until provisioned and has no effect on Python tests. No application logic, SDK surface, or proxy path is touched. No files require special attention. All five changed files are in the e2e test infrastructure tree.
|
| Filename | Overview |
|---|---|
| tests/e2e/bob_the_builder.py | Deleted — the circular-dependency in-runner remediation module is fully removed with no residual references. |
| tests/e2e/conftest.py | Cleanly unwires the bob_the_builder import and remediate() call from pytest_sessionfinish; spend-log cleanup path and _E2E_TEST_RAN guard are untouched. |
| tests/e2e/grafana/alerting/alert_rules.yaml | New Grafana alert rule with 1m evaluation interval and 5m LogQL lookback (5x overlap), correct A→B→C reduce/threshold chain, env-var-referenced datasource UID, and simplified notification routing. |
| tests/e2e/grafana/alerting/contact_points.yaml | New Slack contact point; webhook URL correctly env-var-referenced, resolve messages suppressed, Go template syntax valid. |
| tests/e2e/grafana/alerting/README.md | Operational runbook for the new alerting provisioning, colocated with the YAML files it documents. |
Reviews (4): Last reviewed commit: "docs(e2e): note the alert selector must ..." | Re-trigger Greptile
Greptile SummaryThis PR removes
Confidence Score: 4/5Safe to merge — the deletion of bob_the_builder and its conftest.py hook is clean and complete, and the new Grafana provisioning files are additive infrastructure-as-code with no runtime impact on the proxy or test suite. The conftest.py edit is minimal and correct; the spend-log reset path is unaffected. The Grafana rule's LogQL window matches exactly its evaluation interval, which can cause edge-case boundary gaps where a failure logged right at an interval boundary escapes both windows — a minor but real operational concern worth a one-line fix before the alert goes live. alert_rules.yaml — the [5m] LogQL range should be widened slightly relative to the 5m evaluation interval to avoid boundary gaps.
|
| Filename | Overview |
|---|---|
| tests/e2e/bob_the_builder.py | File deleted — in-runner Devin remediation module removed as intended; no residual references remain |
| tests/e2e/conftest.py | Trailing remediation call removed from pytest_sessionfinish; spend-log reset path and _E2E_TEST_RAN guard are untouched and correct |
| tests/e2e/grafana/alerting/alert_rules.yaml | Grafana Alerting provisioning for e2e failures; query logic is correct but the [5m] LogQL window exactly matches the 5m evaluation interval, which can cause boundary gaps |
| tests/e2e/grafana/alerting/contact_points.yaml | Slack contact point provisioning; webhook correctly externalized via env var, disableResolveMessage=false will send resolved notifications to the Devin channel |
| tests/e2e/grafana/alerting/README.md | New documentation file added directly to the repo; design rationale and operational instructions are clear and accurate |
Reviews (2): Last reviewed commit: "refactor(e2e): remove bob_the_builder; d..." | Re-trigger Greptile
|
osv-scan is failing here for base drift, not this PR. It is red on essentially every open PR right now (a CVE published after the base branch last bumped its lockfile), and this PR changes no dependency files (git diff origin/litellm_internal_staging...HEAD lists no uv.lock/pyproject/requirements changes). The dependency bump belongs in its own PR on the base branch. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…na alert Delete tests/e2e/bob_the_builder.py and unwire its pytest_sessionfinish remediation call. bob was an opt-in (E2E_DEVIN_REMEDIATION=1) hook that, on a red run, collected the failing tests and called the Devin MCP devin_session_create tool through the proxy's own /mcp-rest gateway to open fix PRs. It was a circular dependency: the suite goes red most often when the proxy is unhealthy, which is exactly when the call back through /mcp-rest also failed, and it swallowed every exception so the failure was silent. It also put remediation orchestration inside the test runner and hand-rolled a sha256 node-id dedup that Grafana already does. Replace it with alerting-as-code under tests/e2e/grafana/alerting/: a Grafana rule on the E2E_RESULT failure signal the harness already ships to Loki, routed to a Slack contact point where Devin listens. Because the alert reads the already-shipped Loki signal instead of calling back through the proxy, it keeps working when the runner or proxy is degraded, and Grafana owns the grouping and repeat suppression. Resolves LIT-4551
5dbe79e to
dca50c6
Compare
|
Addressed both notes from the last review in HEAD dca50c6. Boundary-gap on the LogQL window: the rule now evaluates every 1m over the 5m lookback instead of every 5m, so windows overlap 5x and a line ingested late or near a boundary is always caught by several consecutive evaluations. Grouping and repeat_interval on notification_settings keep that to one notification. I also set disableResolveMessage: true on the Slack contact point so Devin only ever acts on a firing (red) suite, never on a recovery message. README placement: this is provisioning-adjacent operational documentation colocated with the YAML it describes, matching the existing tests/e2e/grafana/status_history_panels.md in the same directory. It is not user-facing product documentation for the docs site, so it belongs next to the config rather than in the docs repo. I verified the change end to end against a real Grafana 11.4.0 plus Loki: provisioning loads with no errors, and after pushing a failing E2E_RESULT line the rule fires per package and the Slack contact point delivers the notification with the drill-down query (payload in the PR description). @greptileai please review the current head dca50c6 |
|
The |
The rule's {service_name="litellm-e2e"} stream selector matches nothing if the
scrape does not attach that label, so the alert would silently never fire; point
at the pod-based fallback selector the status-history panels already use.
|
@greptileai please review the current head cbba2b0 |
|
The |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
45273f1
into
litellm_internal_staging
Relevant issues
Linear ticket
Resolves LIT-4551
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
This is harness-internal cleanup with no runtime LLM surface, so the proof is that the module is fully removed, the session-finish cleanup it shared a hook with is untouched, and the
E2E_RESULTsignal that downstream alerting consumes is still emitted by the harness. Captured at head82d878e20bbob existed on the base and is gone on head, with no residual references:
The
pytest_sessionfinishhook keeps its spend-log truncation path and the_E2E_TEST_RANguard that protects it; only the trailing remediation call was removed. The alerting that replaces bob reads theE2E_RESULTline the harness already ships, so the emitter is untouched:Type
🧹 Refactoring
🚄 Infrastructure
Changes
This deletes
tests/e2e/bob_the_builder.pyand unwires it from the e2e runnerbob was an opt-in (
E2E_DEVIN_REMEDIATION=1)pytest_sessionfinishstep that, on a red run, collected the failing tests plus their tracebacks and called the Devin MCPdevin_session_createtool through the LiteLLM proxy's own/mcp-restgateway to open Linear tickets and fix PRs. The only wiring was the trailing remediation block inconftest.py'spytest_sessionfinish; the spend-log reset path and its_E2E_TEST_RANguard are left as they wereThe remediation is now driven from a Grafana alert on the
E2E_RESULTlines the harness already emits into Loki, provisioned directly in Grafana rather than checked into this repo, so nothing here needs to carry the alerting configLink to Devin session: https://app.devin.ai/sessions/ef60065427004a0faf31d5385fa78736
Requested by: @yassin-berriai