OAuth MCP: delete expired token row on 401 so users can re-auth - #2211
Merged
Merged
Conversation
Previously _evict_expired_token only deleted from DiskTokenStore (CLI), not DalTokenStore (server), so on 401 the Supabase row stayed and the frontend kept showing "Failed" forever with no clickable recovery path. The stale per-user tool list also remained, causing the LLM to keep calling dead tools instead of falling back to the _connect placeholder. Both branches now run on 401: delete the persisted token row regardless of store, and clear _user_tools[user][toolset] so apply_user_tools re-exposes the placeholder for fresh OAuth. The justification for the original DAL gate (another cluster might have refreshed in parallel) doesn't hold: by the time a 401 reaches here, the background refresh loop has already failed; the stored token is dead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Contributor
📂 Previous Runs
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Skill Generated | Skills Read | Compactions | Denied commands | Src |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ✅ | 09_crashpod | 26.2s | 3 | 7 | $0.2095 | 57,342 | 55,825 | 21,119 | 1,517 | 668 | 33,911 | 21,914 | 158 | — | — | — | — | src |
| ✅ | 101_loki_historical_logs_pod_deleted | 62.3s | 5 | 11 | $0.2971 | 102,850 | 99,456 | 23,184 | 3,394 | 1,221 | 75,777 | 23,679 | 771 | — | — | — | — | src |
| ✅ | 112_find_pvcs_by_uuid | 16.9s | 2 | 2 | $0.1737 | 37,666 | 36,711 | 20,165 | 955 | 517 | 16,543 | 20,168 | 293 | — | — | — | — | src |
| ✅ | 12_job_crashing | 30.2s | 4 | 7 | $0.2101 | 75,132 | 73,668 | 20,004 | 1,464 | 423 | 53,090 | 20,578 | 152 | — | — | — | — | src |
| ✅ | 176_network_policy_blocking_traffic_no_skills | 36.7s | 5 | 9 | $0.2364 | 97,701 | 96,085 | 21,446 | 1,616 | 412 | 73,944 | 22,141 | 198 | — | — | — | — | src |
| ✅ | 227_count_configmaps_per_namespace[0] | 20.0s | 3 | 6 | $0.1671 | 53,004 | 52,299 | 18,866 | 705 | 439 | 33,429 | 18,870 | 30 | — | — | — | — | src |
| ✅ | 243_pod_names_contain_service | 38.4s | 3 | 7 | $0.2074 | 56,533 | 54,867 | 20,205 | 1,666 | 659 | 33,889 | 20,978 | 283 | — | — | — | — | src |
| ✅ | 24_misconfigured_pvc | 35.0s | 5 | 10 | $0.2358 | 97,215 | 95,554 | 21,192 | 1,661 | 394 | 73,652 | 21,902 | 94 | — | — | — | — | src |
| ✅ ✍️ | 254_elasticsearch_dr_test_log_check | 73.0s | 10 | 12 | $0.3101 | 156,078 | 152,416 | 19,577 | 3,662 | 928 | 132,496 | 19,920 | 239 | 1 | — | — | — | src |
| ✅ ✍️ | 259_wrong_cluster_logs_confusion | 84.0s | 7 | 9 | $0.2943 | 115,998 | 112,129 | 19,946 | 3,869 | 997 | 92,175 | 19,954 | 742 | 1 | — | — | — | src |
| ✅ ✍️ | 260_global_es_remote_cluster_logs | 84.3s | 11 | 13 | $0.3515 | 180,250 | 175,998 | 21,068 | 4,252 | 914 | 154,011 | 21,987 | 657 | 1 | — | — | — | src |
| ✅ | 43_current_datetime_from_prompt | 6.8s | 1 | — | $0.1170 | 16,653 | 16,530 | 16,530 | 123 | 123 | 0 | 16,530 | 79 | — | — | — | — | src |
| ✅ | 51_logs_summarize_errors | 22.1s | 3 | 2 | $0.1768 | 53,711 | 52,809 | 19,431 | 902 | 539 | 33,374 | 19,435 | 33 | — | — | — | — | src |
| ✅ | 61_exact_match_counting | 12.1s | 2 | 1 | $0.1310 | 33,588 | 33,371 | 16,861 | 217 | 148 | 16,507 | 16,864 | 30 | — | — | — | — | src |
| Total | 39.1s avg | 4.6 avg | 7.4 avg | $3.1176 | 1,133,721 | 1,107,718 | 23,184 | 26,003 | 1,221 | 822,798 | 284,920 | 3,759 | 3 | — | — | — |
Skills mechanism stats
- Evals that emitted at least one memory: 3
- Replays attempted: 0
- Replays where the agent loaded the captured skill: 0/0
- Replays that answered correctly: 0/0
- Mean replay vs primary delta (per-row average): — cost, — tokens (n=0)
Benchmark Comparison Details
Master baseline: latest master-* experiment (post-merge regression eval)
Status: 14 test/model combinations loaded
- master-27824784319 (created: 2026-06-19)
Benchmark baseline: latest ci-benchmark experiment on master
Status: 257 test/model combinations loaded
- ci-benchmark-27891966896 (created: 2026-06-21)
Time comparison (seconds):
| Test case | This branch | master (3d ago) | Δ vs master | benchmark (2d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 26.2s | 29.4s | ↓11% | 26.7s | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 62.3s | 46.4s | ↑34% | 636.1s | ↓90% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 16.9s | 15.6s | ±0% | 13.1s | ↑28% |
| 12_job_crashing (opus-4.6) 📄 | 30.2s | 25.1s | ↑20% | 24.4s | ↑24% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 36.7s | 32.4s | ↑13% | 31.9s | ↑15% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 20.0s | 14.5s | ↑37% | 14.5s | ↑37% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 38.4s | 29.0s | ↑32% | 27.5s | ↑40% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 35.0s | 30.6s | ↑15% | 27.5s | ↑27% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 73.0s | 68.8s | ±0% | 71.5s | ±0% |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 84.0s | 84.2s | ±0% | 68.0s | ↑24% |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 84.3s | 77.8s | ±0% | 61.5s | ↑37% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 6.8s | 3.8s | ↑79% | 3.2s | ↑113% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 22.1s | 18.6s | ↑19% | 15.1s | ↑47% |
| 61_exact_match_counting (opus-4.6) 📄 | 12.1s | 7.3s | ↑64% | 6.4s | ↑88% |
| Total (all, n=14) | 39.1s | 34.5s | — | 73.4s | — |
| Comparable (m=14, b=14) | 39.1s | 34.5s | ↑13% | 73.4s | ↓47% |
Cost comparison:
| Test case | This branch | master (3d ago) | Δ vs master | benchmark (2d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | $0.2095 | $0.2304 | ±0% | $0.2309 | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | $0.2971 | $0.2757 | ±0% | $3.4626 | ↓91% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | $0.1737 | $0.1732 | ±0% | $0.1728 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | $0.2101 | $0.2092 | ±0% | $0.2117 | ±0% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | $0.2364 | $0.2539 | ±0% | $0.2740 | ↓14% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | $0.1671 | $0.1674 | ±0% | $0.1674 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | $0.2074 | $0.1998 | ±0% | $0.2143 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | $0.2358 | $0.2365 | ±0% | $0.2533 | ±0% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | $0.3101 | $0.3342 | ±0% | $0.3623 | ↓14% |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | $0.2943 | $0.3685 | ↓20% | $0.3328 | ↓12% |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | $0.3515 | $0.3688 | ±0% | $0.3134 | ↑12% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | $0.1170 | $0.1170 | ±0% | $0.1171 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | $0.1768 | $0.1730 | ±0% | $0.1725 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | $0.1310 | $0.1310 | ±0% | $0.1310 | ±0% |
| Total (all, n=14) | $0.2227 | $0.2313 | — | $0.4583 | — |
| Comparable (m=14, b=14) | $0.2227 | $0.2313 | ±0% | $0.4583 | ↓51% |
Total tokens comparison:
| Test case | This branch | master (3d ago) | Δ vs master | benchmark (2d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 57,342 | 78,851 | ↓27% | 78,756 | ↓27% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 102,850 | 101,178 | ±0% | 3,011,791 | ↓97% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 37,666 | 37,684 | ±0% | 37,638 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 75,132 | 75,000 | ±0% | 75,218 | ±0% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 97,701 | 102,465 | ±0% | 105,366 | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 53,004 | 53,013 | ±0% | 53,028 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 56,533 | 55,789 | ±0% | 57,318 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 97,215 | 97,234 | ±0% | 85,370 | ↑14% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 156,078 | 166,485 | ±0% | 194,499 | ↓20% |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 115,998 | 183,807 | ↓37% | 133,446 | ↓13% |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 180,250 | 186,227 | ±0% | 159,503 | ↑13% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 16,653 | 16,651 | ±0% | 16,656 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 53,711 | 53,405 | ±0% | 53,519 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 33,588 | 33,592 | ±0% | 33,589 | ±0% |
| Total (all, n=14) | 80,980 | 88,670 | — | 292,550 | — |
| Comparable (m=14, b=14) | 80,980 | 88,670 | ±0% | 292,550 | ↓72% |
Cached tokens comparison:
| Test case | This branch | master (3d ago) | Δ vs master | benchmark (2d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 33,911 | 54,605 | ↓38% | 54,541 | ↓38% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 75,777 | 75,268 | ±0% | 2,832,791 | ↓97% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 16,543 | 16,543 | ±0% | 16,543 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 53,090 | 53,078 | ±0% | 53,099 | ±0% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 73,944 | 76,957 | ±0% | 77,290 | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 33,429 | 33,434 | ±0% | 33,429 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 33,889 | 33,823 | ±0% | 34,060 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 73,652 | 73,495 | ±0% | 58,453 | ↑26% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 132,496 | 141,125 | ±0% | 168,108 | ↓21% |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 92,175 | 156,613 | ↓41% | 106,957 | ↓14% |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 154,011 | 158,508 | ±0% | 135,238 | ↑14% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 33,374 | 33,377 | ±0% | 33,380 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 16,507 | 16,507 | ±0% | 16,507 | ±0% |
| Total (all, n=14) | 58,771 | 65,952 | — | 258,600 | — |
| Comparable (m=13, b=13) | 63,292 | 71,026 | ↓11% | 278,492 | ↓77% |
Turns comparison:
| Test case | This branch | master (3d ago) | Δ vs master | benchmark (2d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 3 | 4 | ↓25% | 4 | ↓25% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 5 | 5 | ±0% | 41 | ↓88% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | 2 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 4 | 4 | ±0% | 4 | ±0% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 5 | 5 | ±0% | 5 | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 3 | 3 | ±0% | 3 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 3 | 3 | ±0% | 3 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 5 | 5 | ±0% | 4 | ↑25% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 10 | 10 | ±0% | 12 | ↓17% |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 7 | 11 | ↓36% | 8 | ↓12% |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 11 | 11 | ±0% | 10 | ↑10% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 1 | 1 | ±0% | 1 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 3 | 3 | ±0% | 3 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 2 | 2 | ±0% | 2 | ±0% |
| Total (all, n=14) | 4.6 | 4.9 | — | 7.3 | — |
| Comparable (m=14, b=14) | 4.6 | 4.9 | ±0% | 7.3 | ↓37% |
Tool calls comparison:
| Test case | This branch | master (3d ago) | Δ vs master | benchmark (2d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 7 | 8 | ↓12% | 8 | ↓12% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 11 | 10 | ↑10% | 91 | ↓88% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | 2 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 7 | 7 | ±0% | 7 | ±0% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 9 | 10 | ↓10% | 11 | ↓18% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 6 | 6 | ±0% | 6 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 7 | 7 | ±0% | 8 | ↓12% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 10 | 10 | ±0% | 9 | ↑11% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 12 | 13 | ±0% | 15 | ↓20% |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 9 | 13 | ↓31% | 11 | ↓18% |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 13 | 15 | ↓13% | 12 | ±0% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 2 | 2 | ±0% | 2 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 1 | 1 | ±0% | 1 | ±0% |
| Total (all, n=14) | 6.9 | 8.0 | — | 14.1 | — |
| Comparable (m=13, b=13) | 7.4 | 8.0 | ±0% | 14.1 | ↓48% |
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref worktree-mcp-oauth-token-expiry -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
id: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
id |
Eval ID / pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals (applies to both automatic runs and /eval comments):
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
evals-model-<name> |
Override the model (use model list name, e.g. sonnet-4.5) |
Examples: evals-tag-easy, evals-id-09_crashpod, evals-model-sonnet-4.5
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, conversation_worker, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fable-not-opus, fast, frontend, grafana, hard, images, integration, kafka, kubernetes, leaked-information, logs, loki, manual, mcp, medium, metrics, multi-cluster, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, skills, slackbot, storage, token-limit, toolset-limitation, traces, transparency, victorialogs
🤖 Valid models
deepseek-chat, deepseek-r1-reasoner, deepseek-reasoner, deepseek-v3.2-chat, fable-5, gemini-3-flash-preview, gemini-3-pro-preview, gemini-3.1-pro-preview, gpt-4.1, gpt-5.2-high-reasoning, gpt-5.3-codex, gpt-5.4, gpt-5.5, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, opus-4.7, opus-4.8, qwen-next-80B-instruct, qwen-next-80B-thinking, sonnet-4.5, sonnet-4.6
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref worktree-mcp-oauth-token-expiry -f markers=regression -f filter=
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesOAuth 401 Recovery Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:e3d603eca
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:e3d603eca me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:e3d603eca
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:e3d603eca
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:e3d603eca
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:e3d603eca me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:e3d603eca
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:e3d603ecaPatch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:e3d603eca \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:e3d603ecaRobusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:e3d603eca \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:e3d603eca |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@holmes/core/tools_utils/oauth_tool_connector.py`:
- Around line 301-302: The except Exception clause at line 301 is flagged by
Ruff as BLE001 (blind exception catching). Add a focused noqa comment to the
except Exception line (e.g., `# noqa: BLE001`) to document that this
intentionally broad catch is by design for best-effort cleanup and token
eviction, which suppresses the lint warning without changing the behavior.
- Around line 311-315: The issue is that when removing a toolset within the lock
in the toolset removal logic, the code removes entries from _user_tools by
toolset.name but removes entries from _user_tool_to_toolset using object
identity comparison (ts is toolset). This causes stale tool_name to toolset
mappings to persist if the toolset was stored as a different object instance
with the same name. Fix this by changing the identity comparison in the
_user_tool_to_toolset cleanup loop to a name-based comparison instead, so that
the removal logic matches the approach used for _user_tools (comparing by
toolset.name rather than object identity).
In `@tests/core/tools_utils/test_oauth_tool_connector_eviction.py`:
- Line 76: The line 76 in test_oauth_tool_connector_eviction.py violates Ruff's
E702 rule because multiple statements are separated by semicolons on a single
line. Split the chained assignments for stale_tool into separate lines: first
create the MagicMock instance, then assign the name property to "k8s_list_pods"
on a new line, and finally assign the toolset property on another new line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8c68536-c2ac-4d08-ae76-c5faadc9e77f
📒 Files selected for processing (2)
holmes/core/tools_utils/oauth_tool_connector.pytests/core/tools_utils/test_oauth_tool_connector_eviction.py
A reloaded toolset is a new instance with the same name. Identity-only purge left stale tool→toolset entries behind. Match by name so eviction survives instance churn. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Avi-Robusta
approved these changes
Jun 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OAuthToolConnector._evict_expired_token: was gated toDiskTokenStore(CLI only). In server mode (DalTokenStore), the expired Supabase row was never deleted → frontend kept rendering "Failed" with no clickable recovery path → user stuck._user_tools[user][toolset]on the same 401 branch. Without this,apply_user_toolskeeps substituting the now-dead tool list for the_connectplaceholder, so the LLM keeps calling dead tools.The old "another cluster may have refreshed in parallel" justification for the DAL gate doesn't survive scrutiny: by the time a 401 reaches
_evict_expired_token, the background refresh loop has already failed against this stored token. The row is genuinely dead.Test plan
tests/core/tools_utils/test_oauth_tool_connector_eviction.py— red on master, green with the fix:test_401_deletes_token_in_both_stores[DalTokenStore]— confirmsdelete_tokenis now called for the DAL storetest_401_deletes_token_in_both_stores[DiskTokenStore]— confirms disk-store behaviour is preservedtest_401_clears_stale_user_tools— confirms_user_toolscleanuphttps://mcp.atlassian.com/v1/mcp): forced expiry via Supabase, rolled patched image → eviction path fires → row deleted → FE flips "Failed" → "Login" → re-auth → 31 Atlassian tools rediscover → recovery loop closed.tests/test_mcp_oauth.py tests/core/tools_utils/).🤖 Generated with Claude Code
Summary by CodeRabbit