Repository navigation
Conversation
The previous bash instructions used `kubectl get pods | grep Running | head -5`
as the canonical "pipe is auto-approved" example. That implicitly endorsed
piping kubectl output through awk/sort/uniq -c/wc to do grouping and counting,
which is exactly the anti-pattern that produces the customer pain reported in
this branch: instead of a single kubernetes_count / kubernetes_jq_query /
kubernetes_tabular_query call, the LLM stitched together shell pipelines that
either required approval (loops, command substitution) or produced noisy
investigation traces.
This change adds a "Tool Selection — Prefer Dedicated Tools" section at the
top of the bash instructions that:
* Tells the model to check for a dedicated K8s tool before reaching for bash.
* Enumerates the four most common anti-patterns (group-by-uniq, wc -l counts,
distinct-via-sort-u, per-resource for-loops) and points each one at the
dedicated tool that replaces it.
* Carves out the legitimate uses of bash — one-off `kubectl get` with a
specific flag, non-kubectl invocations — so the guidance does not over-rotate.
Two smaller edits below:
* The "Pipes" example is changed from `kubectl get pods | grep ...` to a
log-grep example, with a one-line reminder pointing at the new section.
* The "go ahead and use loops" line is replaced with a check-first prompt
that mentions the cost of approval prompts.
This is prompt guidance, not a behavior gate — the model can still pipe kubectl
when it judges that the right choice (e.g. when no dedicated tool fits). The
goal is to flip the default for grouping/counting/joining work back to the
dedicated tools.
Pairs with the 259/260/261/262/263/264 evals on this branch which lock in the
preference. Verification against opus-4.6 with full iteration counts is
pending — the OpenRouter weekly credit limit was hit during testing.
Signed-off-by: Claude <noreply@anthropic.com>
The previous "Tool Selection" section told the LLM what to do but not why,
so it couldn't apply the principle to novel question shapes it hadn't seen
before. Replace the generic "faster, more reliable, cleaner trace" line
with the five concrete costs of choosing bash when a dedicated tool fits:
1. Compound bash (loops, ifs, command substitution) requires explicit
user approval per call and interrupts the user mid-investigation.
Dedicated tools and simple kubectl gets are auto-approved.
2. The bash allowlist gates every pipeline segment. A pipeline is only
auto-approved if every segment (awk, sort, uniq, wc, grep, cut, ...)
is on the configured allowlist; if one isn't, the whole pipeline
blocks waiting for approval — or fails outright in deployments that
auto-deny. Dedicated tools sidestep the allowlist entirely.
3. Text parsing is fragile and fails silently. wc -l counts headers,
awk column indexing breaks under -o wide, empty awk matches look
like "0 results" rather than "bad pipeline". Dedicated tools work
on JSON via jq and return actionable errors instead of plausible
false answers.
4. Server-side filtering keeps token budgets small. kubernetes_jq_query
projects only the asked-for fields on the API server; kubectl get -A
pipes the full status of every resource into the conversation. On
real clusters with hundreds of pods, this difference is enormous
and persists across every subsequent turn until compaction.
5. Structured tool calls produce readable traces. A
kubernetes_count(kind=..., jq=...) line survives compaction with
its intent intact; a 200-character shell pipeline does not.
These are exactly the reasons opus-4.6 sometimes still picked the bash
path in our internal probes even after the first "Tool Selection" pass —
the principle was abstract and didn't connect to consequences the model
could weigh against the convenience of a one-liner. With the five costs
spelled out, the model has the information needed to make the same
trade-off on cases that aren't in the explicit anti-pattern list.
Signed-off-by: Claude <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
📂 Previous Runs
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Compactions | Denied commands | Src |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ✅ | 09_crashpod | 28.0s | 4 | 7 | $0.2085 | 72,686 | 71,233 | 19,822 | 1,453 | 668 | 50,613 | 20,620 | 107 | — | — | src |
| ✅ | 101_loki_historical_logs_pod_deleted | 41.3s | 4 | 8 | $0.2389 | 73,974 | 71,558 | 20,831 | 2,416 | 813 | 50,451 | 21,107 | 350 | — | — | src |
| ✅ | 112_find_pvcs_by_uuid | 13.7s | 2 | 2 | $0.1617 | 35,003 | 34,137 | 18,881 | 866 | 448 | 15,253 | 18,884 | 289 | — | — | src |
| ✅ | 12_job_crashing | 27.7s | 4 | 9 | $0.2283 | 77,204 | 75,508 | 21,765 | 1,696 | 454 | 53,129 | 22,379 | 132 | — | — | src |
| ✅ | 176_network_policy_blocking_traffic_no_skills | 39.7s | 5 | 13 | $0.2618 | 97,115 | 94,715 | 21,984 | 2,400 | 683 | 71,786 | 22,929 | 374 | — | — | src |
| ✅ | 227_count_configmaps_per_namespace[0] | 14.4s | 3 | 6 | $0.1568 | 49,174 | 48,459 | 17,598 | 715 | 437 | 30,857 | 17,602 | 36 | — | — | src |
| ✅ | 243_pod_names_contain_service | 25.5s | 3 | 7 | $0.1959 | 52,523 | 50,894 | 18,999 | 1,629 | 573 | 31,224 | 19,670 | 227 | — | — | src |
| ✅ | 24_misconfigured_pvc | 30.8s | 5 | 11 | $0.2338 | 92,596 | 90,821 | 20,335 | 1,775 | 576 | 69,192 | 21,629 | 117 | — | — | src |
| ✅ | 254_elasticsearch_dr_test_log_check | 61.2s | 8 | 11 | $0.2607 | 104,643 | 101,138 | 17,170 | 3,505 | 1,050 | 83,959 | 17,179 | 231 | — | — | src |
| ✅ | 259_wrong_cluster_logs_confusion | 58.7s | 8 | 11 | $0.2705 | 112,332 | 108,915 | 18,069 | 3,417 | 861 | 90,368 | 18,547 | 268 | — | — | src |
| ✅ | 260_global_es_remote_cluster_logs | 52.8s | 7 | 11 | $0.2552 | 99,077 | 95,775 | 16,772 | 3,302 | 807 | 77,865 | 17,910 | 402 | — | — | src |
| ✅ | 43_current_datetime_from_prompt | 3.8s | 1 | — | $0.1081 | 15,362 | 15,240 | 15,240 | 122 | 122 | 0 | 15,240 | 78 | — | — | src |
| ✅ | 51_logs_summarize_errors | 18.2s | 3 | 2 | $0.1621 | 49,562 | 48,776 | 17,967 | 786 | 414 | 30,805 | 17,971 | 40 | — | — | src |
| ✅ | 61_exact_match_counting | 7.7s | 2 | 1 | $0.1216 | 31,020 | 30,798 | 15,578 | 222 | 153 | 15,217 | 15,581 | 35 | — | — | src |
| Total | 30.2s avg | 4.2 avg | 7.6 avg | $2.8638 | 962,271 | 937,967 | 21,984 | 24,304 | 1,050 | 670,719 | 267,248 | 2,686 | — | — |
Benchmark Comparison Details
Master baseline: latest master-* experiment (post-merge regression eval)
Status: 11 test/model combinations loaded
- master-27089078550 (created: 2026-06-07)
Benchmark baseline: latest ci-benchmark experiment on master
Status: 185 test/model combinations loaded
- ci-benchmark-27081280174 (created: 2026-06-07)
Time comparison (seconds):
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (12h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 28.0s | 26.2s | ±0% | 41.4s | ↓32% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 41.3s | 47.6s | ↓13% | 65.7s | ↓37% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 13.7s | 10.6s | ↑30% | 19.0s | ↓28% |
| 12_job_crashing (opus-4.6) 📄 | 27.7s | 27.6s | ±0% | 42.6s | ↓35% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 39.7s | 32.6s | ↑22% | 44.3s | ↓10% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 14.4s | 13.2s | ±0% | 18.4s | ↓22% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 25.5s | 28.8s | ↓12% | 35.1s | ↓27% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 30.8s | 31.4s | ±0% | 40.6s | ↓24% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 61.2s | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 58.7s | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 52.8s | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 3.8s | 3.1s | ↑23% | 3.5s | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 18.2s | 18.8s | ±0% | 19.7s | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 7.7s | 7.1s | ±0% | 10.2s | ↓25% |
| Total (all, n=14) | 30.2s | 22.5s | — | 31.0s | — |
| Comparable (m=11, b=11) | 22.8s | 22.5s | ±0% | 31.0s | ↓26% |
Cost comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (12h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | $0.2085 | $0.2032 | ±0% | $0.3101 | ↓33% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | $0.2389 | $0.2642 | ±0% | $0.3829 | ↓38% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | $0.1617 | $0.1368 | ↑18% | $0.2047 | ↓21% |
| 12_job_crashing (opus-4.6) 📄 | $0.2283 | $0.2173 | ±0% | $0.3167 | ↓28% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | $0.2618 | $0.2524 | ±0% | $0.3179 | ↓18% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | $0.1568 | $0.1464 | ±0% | $0.2048 | ↓23% |
| 243_pod_names_contain_service (opus-4.6) 📄 | $0.1959 | $0.1872 | ±0% | $0.2662 | ↓26% |
| 24_misconfigured_pvc (opus-4.6) 📄 | $0.2338 | $0.2240 | ±0% | $0.3110 | ↓25% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | $0.2607 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | $0.2705 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | $0.2552 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | $0.1081 | $0.0110 | ↑884% | $0.1190 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | $0.1621 | $0.1488 | ±0% | $0.2018 | ↓20% |
| 61_exact_match_counting (opus-4.6) 📄 | $0.1216 | $0.1112 | ±0% | $0.1518 | ↓20% |
| Total (all, n=14) | $0.2046 | $0.1730 | — | $0.2534 | — |
| Comparable (m=11, b=11) | $0.1889 | $0.1730 | ±0% | $0.2534 | ↓25% |
Total tokens comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (12h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 72,686 | 67,778 | ±0% | 132,351 | ↓45% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 73,974 | 88,307 | ↓16% | 144,907 | ↓49% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 35,003 | 30,686 | ↑14% | 61,219 | ↓43% |
| 12_job_crashing (opus-4.6) 📄 | 77,204 | 69,487 | ↑11% | 136,570 | ↓43% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 97,115 | 108,726 | ↓11% | 113,514 | ↓14% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 49,174 | 44,988 | ±0% | 76,715 | ↓36% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 52,523 | 48,612 | ±0% | 104,698 | ↓50% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 92,596 | 68,923 | ↑34% | 132,672 | ↓30% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 104,643 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 112,332 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 99,077 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 15,362 | 13,970 | ±0% | 17,001 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 49,562 | 45,163 | ±0% | 77,120 | ↓36% |
| 61_exact_match_counting (opus-4.6) 📄 | 31,020 | 28,231 | ±0% | 52,716 | ↓41% |
| Total (all, n=14) | 68,734 | 55,897 | — | 95,408 | — |
| Comparable (m=11, b=11) | 58,747 | 55,897 | ±0% | 95,408 | ↓38% |
Cached tokens comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (12h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 50,613 | 46,700 | ±0% | 102,543 | ↓51% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 50,451 | 64,209 | ↓21% | 109,927 | ↓54% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 15,253 | 13,861 | ↑10% | 38,090 | ↓60% |
| 12_job_crashing (opus-4.6) 📄 | 53,129 | 46,249 | ↑15% | 107,265 | ↓50% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 71,786 | 84,274 | ↓15% | 82,679 | ↓13% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 30,857 | 28,072 | ±0% | 54,602 | ↓43% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 31,224 | 28,690 | ±0% | 78,754 | ↓60% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 69,192 | 46,110 | ↑50% | 103,729 | ↓33% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 83,959 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 90,368 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 77,865 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | 13,845 | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 30,805 | 28,012 | ±0% | 55,156 | ↓44% |
| 61_exact_match_counting (opus-4.6) 📄 | 15,217 | 13,825 | ↑10% | 34,481 | ↓56% |
| Total (all, n=14) | 47,908 | 37,622 | — | 69,748 | — |
| Comparable (m=10, b=10) | 41,853 | 40,000 | ±0% | 76,723 | ↓45% |
Turns comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (12h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 4 | 4 | ±0% | 6 | ↓33% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 4 | 5 | ↓20% | 6 | ↓33% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | 3 | ↓33% |
| 12_job_crashing (opus-4.6) 📄 | 4 | 4 | ±0% | 6 | ↓33% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 5 | 6 | ↓17% | 5 | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 3 | 3 | ±0% | 4 | ↓25% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 3 | 3 | ±0% | 5 | ↓40% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 5 | 4 | ↑25% | 6 | ↓17% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 8 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 8 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 7 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 1 | 1 | ±0% | 1 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 3 | 3 | ±0% | 4 | ↓25% |
| 61_exact_match_counting (opus-4.6) 📄 | 2 | 2 | ±0% | 3 | ↓33% |
| Total (all, n=14) | 4.2 | 3.4 | — | 4.5 | — |
| Comparable (m=11, b=11) | 3.3 | 3.4 | ±0% | 4.5 | ↓27% |
Tool calls comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (12h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 7 | 8 | ↓12% | 13 | ↓46% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 8 | 11 | ↓27% | 14 | ↓43% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | 4 | ↓50% |
| 12_job_crashing (opus-4.6) 📄 | 9 | 9 | ±0% | 15 | ↓40% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 13 | 12 | ±0% | 15 | ↓13% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 6 | 6 | ±0% | 9 | ↓33% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 7 | 7 | ±0% | 11 | ↓36% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 11 | 13 | ↓15% | 15 | ↓27% |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 11 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 11 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 11 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 2 | 2 | ±0% | 5 | ↓60% |
| 61_exact_match_counting (opus-4.6) 📄 | 1 | 1 | ±0% | 3 | ↓67% |
| Total (all, n=14) | 7.1 | 7.1 | — | 10.4 | — |
| Comparable (m=10, b=10) | 6.6 | 7.1 | ±0% | 10.4 | ↓37% |
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 claude/k8s-no-bash-approval-fix -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, 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, 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, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, opus-4.7, 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 claude/k8s-no-bash-approval-fix -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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe bash instruction template is expanded with a new "Tool Selection — Prefer Dedicated Tools" section that identifies anti-patterns for Kubernetes data processing in bash (counting, grouping, distinct, select, iteration) and revises guidance for approval-interrupting constructs to discourage loops and conditionals in favor of dedicated tools or batched ChangesBash Tool Instructions Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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:60a0cc11d
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:60a0cc11d me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:60a0cc11d
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:60a0cc11d
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:60a0cc11d
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:60a0cc11d me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:60a0cc11d
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:60a0cc11dPatch 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:60a0cc11d \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:60a0cc11dRobusta 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:60a0cc11d \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:60a0cc11d |
❌ Deploy Preview for holmes-docs failed. Why did it fail? →
|
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/plugins/toolsets/bash/bash_instructions.jinja2 (1)
3-6: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit failure-report requirements for bash errors.
This template should explicitly require returning rich error context on command failure (full command executed, relevant parameters/filters/time bounds, exit status, and raw stderr) so the model can self-correct consistently.
As per coding guidelines: “All toolsets MUST return detailed error messages from underlying APIs … include the exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message).”
Proposed patch
Use the `bash` tool to execute shell commands. You must provide: 1. `command`: The bash command to execute 2. `suggested_prefixes`: Array of prefixes, one per command segment +3. On failure, return detailed error context for self-correction: + - Exact command/segment executed + - Parameters/filters/time bounds used (when applicable) + - Exit status and full stderr/error message from executionAlso applies to: 91-96
🤖 Prompt for 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. In `@holmes/plugins/toolsets/bash/bash_instructions.jinja2` around lines 3 - 6, Update the bash tool template to require rich failure reporting when a shell command fails: for the `bash` tool (referencing the `command` and `suggested_prefixes` fields) ensure the failure response includes the full command executed, any relevant parameters/filters/time bounds passed, the numeric exit status, and the raw stderr output (and any runtime error message); modify the template text around the `bash` usage instructions so callers are explicitly instructed to return that detailed error context on non-zero exits and failed executions.Source: Coding guidelines
🤖 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/plugins/toolsets/bash/bash_instructions.jinja2`:
- Around line 19-20: The guidance text claiming simple `kubectl get` calls are
"auto-approved" conflicts with the allowlist gating mentioned later; update the
phrase containing "kubectl get" so it explicitly states such calls are
auto-approved only when they match the configured allowlist (or are otherwise
permitted by the allowlist policy), and mirror the same clarified wording in the
later section that references auto-approval and allowlist matching (the
paragraph that currently explains allowlist-based auto-approval) so both places
consistently state that auto-approval is conditional on an allowlist match.
---
Outside diff comments:
In `@holmes/plugins/toolsets/bash/bash_instructions.jinja2`:
- Around line 3-6: Update the bash tool template to require rich failure
reporting when a shell command fails: for the `bash` tool (referencing the
`command` and `suggested_prefixes` fields) ensure the failure response includes
the full command executed, any relevant parameters/filters/time bounds passed,
the numeric exit status, and the raw stderr output (and any runtime error
message); modify the template text around the `bash` usage instructions so
callers are explicitly instructed to return that detailed error context on
non-zero exits and failed executions.
🪄 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: 09c50c59-d96f-4b41-a045-5f97382f787d
📒 Files selected for processing (1)
holmes/plugins/toolsets/bash/bash_instructions.jinja2
CodeRabbit caught an inconsistency: the motivation section claimed "Dedicated tools and simple kubectl get calls are auto-approved" with no caveat, but later in the same file the "Auto-approved" section correctly conditions bash auto-approval on the prefix matching the configured allowlist. Reconcile the two: - Dedicated tools are auto-approved unconditionally (they're separate tools, not bash, and don't go through the allowlist). - Simple kubectl get is auto-approved only when its prefix is on the configured allowlist (which it is by default). Same prompt-engineering intent (push the model away from compound bash), just no longer overclaiming about the always-auto-approved status of kubectl. Signed-off-by: Claude <noreply@anthropic.com>
This branch has not been deployed
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
Significantly expands the bash tool instructions to guide users toward dedicated Kubernetes tools (
kubernetes_jq_query,kubernetes_tabular_query,kubernetes_count) instead of bash pipelines for aggregation, filtering, grouping, and counting operations. This improves user experience by reducing approval prompts and token usage while increasing reliability.Key Changes
New "Tool Selection" section explaining why dedicated tools should be preferred over bash for Kubernetes operations:
wc -lcounts headers,awkbreaks with column shifts)Common anti-patterns section with concrete examples of what to avoid:
kubectl get ... | awk | sort | uniq -c→ usekubernetes_countkubectl get ... | wc -l→ usekubernetes_countkubectl get ... | awk | sort -u→ usekubernetes_jq_queryforloops → use single batchedkubernetes_jq_queryorkubernetes_tabular_queryUpdated pipe example from
kubectl get pods | grep Running | head -5tocat /var/log/app.log | grep ERROR | head -20with a note warning against piping kubectl output through aggregation toolsClarified bash use cases: Single
kubectl getcalls, specific kubectl flags (-o yaml,-o wide,--show-labels), and non-kubectl tools are appropriate; grouping, counting, joining, and set operations are notRefined guidance on loops/conditionals: Added emphasis to minimize approval prompts by checking if a dedicated tool or batched
kubectl getcan accomplish the task firstMinor formatting: Changed "Requires user approval -" to "Requires user approval —" for consistency
https://claude.ai/code/session_01ShMKrLaC9Dddn41ZJM6CWW
Summary by CodeRabbit