Repository navigation
Conversation
) Splits PR #2042 into two — this is the **green** half. Just the system-prompt change. Pair with #2125 (red) which contains the evals. ## What this PR changes `holmes/plugins/prompts/generic_ask.jinja2`: expands the existing `* You are running on cluster X` bullet into a multi-cluster-aware procedure. **Old:** ```jinja2 {% if cluster_name -%} * You are running on cluster {{ cluster_name }}. {%- endif %} ``` **New:** distinguishes kubectl-bound data (only the local cluster) from external observability toolsets (Elasticsearch / Datadog / Loki / etc., which may contain data from many clusters). When the user names a cluster / region / env other than the local one, Holmes: 1. Investigates using external toolsets — does NOT refuse. 2. Verifies each finding's cluster against the user's named cluster (via the data's own `cluster` / `region` / `environment` / `kubernetes.cluster.name` field, or the index/source name). 3. If matched → investigates normally with that cluster's findings. 4. If not matched → states plainly that no data exists for the requested cluster, labels what was found in adjacent clusters, suggests pointing Holmes at the right data source / agent. Two common topologies are explicitly called out: - one Holmes per cluster (kubectl + external mostly scoped to that cluster) - one Holmes with a global observability backend covering many clusters Holmes doesn't know upfront which one applies — it must look at what the data actually contains. ## Companion PR This turns #2125's evals green: - 254, 19 — name disambiguation - 259 — wrong cluster, local-only data - 260 — global topology, remote cluster data - 261 — time-window gap - 262 — ambiguous cluster reference - 263 — region-suffixed sibling services - 264 — wrong env, same region - 265 — labeling discipline - 266 — toolset-disabled vs no-data - 267 — cluster-name alias (must NOT over-correct) - 268/269/270 — kubectl-only / mixed-source / namespace-collision Local validation across 3 iterations × 3 models (Sonnet 4.5 / Opus 4.6 / Opus 4.7) on the ES-based subset was clean. CI on #2125 will demonstrate the failing state; merging this on top turns those failures green. --- _Generated by [Claude Code](https://claude.ai/code/session_015scgyQbqJD23heqct31Ger)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Improvements** * Enhanced cluster awareness to ensure observability data is correctly attributed to your specified cluster and prevent misinterpretation of cross-cluster data. * Improved transparency when exact entity data is unavailable, with clearer reporting of similarly-named alternatives for user verification. * Refined troubleshooting guidance to better support iterative investigation workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
The eval report posted on CI/CD and GitHub Actions (evals_report.md) now includes a column listing the bash commands HolmesGPT tried to run that were denied. During evals there is no interactive approver and the bash toolset enforces an allow/deny list, so any command that is not pre-approved is effectively denied. - Add extract_denied_commands() to pull denied bash commands out of an LLMResult (deny-list/hard-coded blocks and approval-required rejections). - Capture them as a 'denied_commands' user property in test_ask_holmes.py. - Propagate them through the conftest results collection. - Render a new 'Denied commands' column (with a Total count) in the report. - Add unit tests covering extraction, formatting, and report rendering. Signed-off-by: Claude <noreply@anthropic.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Evaluation reports now include a "Denied commands" column and a summary warning when blocked bash commands are present. * **Tests** * Added comprehensive tests for extracting and rendering denied commands, edge cases, formatting, and report aggregation. * **Chores** * Test reporting updated to collect and surface denied-command data from test runs. * **Fixtures** * Added a regression fixture covering a denied bash command scenario. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Automated weekly benchmark results from CI. **Benchmark Type**: fast-benchmark **Command**: ./run_benchmarks_local.py --benchmark-type fast-benchmark --models opus-4.7,opus-4.6,sonnet-4.6,haiku-4.5,gpt-5.4,gemini-3.1-pro-preview,qwen-next-80B-instruct,qwen-next-80B-thinking,deepseek-r1-reasoner,deepseek-v3.2-chat,gpt-5.3-codex Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: moshemorad <moshemorad12340@gmail.com>
Mirrors the TodoWrite pattern: lets the investigation track competing root-cause hypotheses (proposed/investigating/supported/refuted) so the model weighs evidence for each candidate instead of latching onto the loudest signal. Adds Hypothesis model, hypothesis_formatter, the HypothesisWrite tool, and investigator-instruction guidance on distinguishing the real root cause from surrounding infra noise. Signed-off-by: Claude <noreply@anthropic.com>
…-2PpZb-green-hypothesis
Adds an enable_hypothesis test-case flag mirroring enable_todo. The core_investigation toolset (TodoWrite + HypothesisWrite) is dropped unless a test opts into at least one tool, and its tool list is filtered so the LLM only ever sees the tools the test enabled. Turns on both flags for the 271 root-cause-noise eval so Holmes can track competing infra-vs-application hypotheses while investigating. Updates the core_investigation unit test for the second tool. Signed-off-by: Claude <noreply@anthropic.com>
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.
|
Important Review skippedToo many files! This PR contains 232 files, which is 82 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (232)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds hypothesis-driven root-cause tracking: a Hypothesis model and formatter, a HypothesisWrite tool wired into CoreInvestigationToolset, test infra toggles, and updated investigation fixtures and scripts to exercise hypothesis-enabled workflows. ChangesHypothesis-driven root-cause tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
|
✅ 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:9a539088b
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:9a539088b me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:9a539088b
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:9a539088b
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:9a539088b
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:9a539088b me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:9a539088b
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:9a539088bPatch 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:9a539088b \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:9a539088bRobusta 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:9a539088b \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:9a539088b |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
holmes/plugins/toolsets/investigator/core_investigation.py (1)
30-44: ⚡ Quick winConsider validating that statement is non-empty.
The function allows
statement=""(line 38) when the LLM omits it, but an empty statement is semantically invalid for a hypothesis. While this mirrors the pattern inparse_tasks(line 54 allows empty content), it may lead to confusing tool output.Options:
- Log a warning when statement is empty
- Raise a validation error
- Accept the current defensive behavior if the formatter and LLM prompt handle it
🤖 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/investigator/core_investigation.py` around lines 30 - 44, parse_hypotheses currently allows empty hypothesis statements; update parse_hypotheses to validate that the computed statement is non-empty before constructing/appending a Hypothesis: if the statement is empty, emit a warning (use the module logger or logging.warning) that includes the generated id (uuid4()) and skip appending that item (alternatively you can raise a ValueError if you prefer strict validation); keep the existing use of Hypothesis and HypothesisStatus when the statement is present and mirror the parse_tasks handling if needed.
🤖 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.
Nitpick comments:
In `@holmes/plugins/toolsets/investigator/core_investigation.py`:
- Around line 30-44: parse_hypotheses currently allows empty hypothesis
statements; update parse_hypotheses to validate that the computed statement is
non-empty before constructing/appending a Hypothesis: if the statement is empty,
emit a warning (use the module logger or logging.warning) that includes the
generated id (uuid4()) and skip appending that item (alternatively you can raise
a ValueError if you prefer strict validation); keep the existing use of
Hypothesis and HypothesisStatus when the statement is present and mirror the
parse_tasks handling if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42a4e522-8ac7-4d25-aee6-294b6efe730e
📒 Files selected for processing (9)
holmes/core/hypothesis_formatter.pyholmes/plugins/toolsets/investigator/core_investigation.pyholmes/plugins/toolsets/investigator/investigator_instructions.jinja2holmes/plugins/toolsets/investigator/model.pytests/llm/fixtures/test_ask_holmes/271_root_cause_buried_in_infra_noise/test_case.yamltests/llm/test_ask_holmes.pytests/llm/utils/test_case_utils.pytests/llm/utils/test_toolset.pytests/plugins/toolsets/test_core_investigation.py
…ed events) Replaces the injected fake AWS-CNI/Karpenter events (which strong models correctly dismiss as test scaffolding) with an entirely real, cluster- verifiable landscape: - task_pods.yaml + run_real.sh actually run the 3 index-to-es task pods (they schedule, pull, start, then fail at the app layer and exit non-zero), then delete them -- so kubectl logs is unavailable but the genuine Scheduled/Pulled/Started events remain as proof they ran. - noise_pods.yaml adds real unrelated noise: Pending/FailedScheduling ingest-workers + ErrImagePull/ImagePullBackOff stream-processors. - generate_events.sh (the fake-event injector) is removed. Also broadens expected_output criterion 2 to accept either 'point to the Airflow task logs' OR a correct direct diagnosis (indexer cannot reach any Elasticsearch endpoint), since in a realistic cluster a strong model can discover that cause; the anti-bug assertions (do not blame the infra noise) are unchanged. Skips empty-statement hypotheses in parse_hypotheses (CodeRabbit nitpick). Verified on a live k3s cluster: green on opus-4.8; on opus-4.6 the RCA guidance lifts pass rate 33%->67% (n=3), with the HypothesisWrite tool adding no measurable lift on top of the guidance. Signed-off-by: Claude <noreply@anthropic.com>
## Summary This PR adds a uniform `firing` boolean field to issue data returned by `get_issue_data()`, enabling the LLM to easily determine whether an alert is currently active or resolved without having to infer state from raw timestamps. ## Changes ### Core Implementation - **`holmes/core/supabase_dal.py`**: Added logic to compute and expose a `firing` field in `get_issue_data()`: - For Prometheus alerts: uses the explicit `firing` column from the GroupedIssues table - For all other sources: derives firing state from `ends_at` (null = firing, timestamp = resolved) - Ensures all callers see a consistent `firing` field regardless of alert source ### Test Coverage - **`tests/core/test_supabase_dal.py`**: Added comprehensive test suite `TestGetIssueDataFiring` with 4 test cases: - Non-Prometheus alerts with `ends_at=None` → `firing=True` - Non-Prometheus alerts with `ends_at` set → `firing=False` - Prometheus alerts using explicit `firing` flag from GroupedIssues - Verification that explicit Prometheus `firing` values are preserved ### Documentation - **`holmes/plugins/toolsets/robusta/robusta_instructions.jinja2`**: Updated LLM instructions to: - Explain the `firing` field semantics - Clarify the relationship between `firing` and `ends_at` - Instruct the LLM to always explicitly state alert state and not assume alerts are still firing ## Implementation Details The solution handles the architectural difference between alert sources: - Prometheus stores explicit firing state in GroupedIssues (re-fetched when source is "prometheus") - Other sources (Kubernetes, etc.) only have `ends_at` timestamps, so firing state is computed as `ends_at is None` - The uniform `firing` field is added only if not already present, preserving explicit values from GroupedIssues https://claude.ai/code/session_01Ki4X5AMkzed6cdKCiEC6dJ Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Multi-instance support added: configure multiple named instances, list/discover instances, and route calls to selected instances; flat single-instance configs remain compatible. * **Improvements** * Unified config-driven request and health handling with enhanced auth options and clearer error/health reporting across several data sources (Elasticsearch, Grafana, Loki/Tempo, Prometheus, etc.). * **Documentation** * New multi-instance guide and per-tool “Multiple Instances” examples; navigation updated. * **Tests** * Many new and revised end-to-end and unit tests covering multi-instance behavior and routing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: avi@robusta.dev <avi@robusta.dev> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a “Which setup do I need?” chooser and renamed single-cluster guidance for clarity. * New Multiple Clusters section: end-to-end guidance for using one Holmes instance with many clusters (per-cluster kubeconfigs, mounting/override, verification, and UI routing). * Clarified multi-cluster context handling and tooling requirements; updated Per-User Auth snippet formatting. * Wrapped Common Use Cases examples in fenced bash command blocks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Roi Glinik <groi.tech@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Documentation-only change, no behavior change. The `llm_summarize` transformer predates the spill-to-disk mechanism, is disabled by default, and never worked well in practice: summarization is lossy (the original tool output is unrecoverable afterwards) and it adds latency and cost to every large tool call. Modern models do better working from the full data spilled to disk. This marks it as legacy in the module/class docstrings and adds a warning admonition to `docs/development/transformers.md`, so future contributors don't build on it and users don't enable it expecting good results. Kept for backwards compatibility with existing configs that reference it. Part of a series of targeted fixes removing old-model data limitations from built-in tools (see #2167, #2168, #2169). https://claude.ai/code/session_01BwJeGAGBLoby5rShhQADwq --- _Generated by [Claude Code](https://claude.ai/code/session_01BwJeGAGBLoby5rShhQADwq)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added notices marking the `llm_summarize` transformer as a legacy feature that is disabled by default. This feature is not recommended for new configurations due to lossy summarization, increased latency, and associated costs. Documentation has been updated to recommend the spill-to-disk mechanism as the preferred alternative for handling oversized tool results. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Automated weekly benchmark results from CI. **Benchmark Type**: fast-benchmark **Command**: ./run_benchmarks_local.py --benchmark-type fast-benchmark --models opus-4.6,opus-4.7,opus-4.8,gpt-5.4,gpt-5.5 --iterations 5 --------- Signed-off-by: avi@robusta.dev <avi@robusta.dev> Co-authored-by: avi@robusta.dev <avi@robusta.dev> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Avi <97387909+Avi-Robusta@users.noreply.github.com>
…secrets in… (#2179) Fixing regression tests - Re-add `regression` tag to 254/259/260. - Add ELASTICSEARCH_URL/ELASTICSEARCH_API_KEY to eval-master.yaml, mirroring eval-regression.yaml and eval-benchmarks.yaml. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated regression evaluation workflow to include additional infrastructure credentials. * **Tests** * Added regression test tags to multiple test cases to strengthen test coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: avi@robusta.dev <avi@robusta.dev> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (fixes Confluence eval 210) (#2177) ## Summary `MultiInstanceToolset` mirrors the child's `llm_instructions` from the unconfigured template in `__init__` and never refreshes it after running the children's `prerequisites_callable`. Toolsets that build their instructions at prerequisite time — Confluence, where the content depends on the configured endpoint (base URL, gateway routing, whitelisted paths) — therefore rendered **no usage section in the system prompt** at all. The visible symptom: the `210_confluence_skill_fetch` eval failed on every CI run. The trace shows the LLM was never told the Atlassian gateway base URL, so it guessed request URLs (`https://api.atlassian.com/wiki/api/v2/pages/...`, `https://api.atlassian.com/ex/confluence/wiki/rest/api/content/...` — both missing the `/ex/confluence/<cloud_id>` prefix), every guess was rejected by the endpoint whitelist, and it fell back to `fetch_webpage` (an explicit FAIL condition of the eval). ## Fix After the wrapper runs its children's prerequisites, mirror the children's runtime-built `llm_instructions` onto the wrapper: - single (flat) instance → the child's instructions verbatim - multiple instances → per-instance sections labelled `### Instance \`<name>\`` - no child builds instructions → keep the template-derived static instructions (other wrapped toolsets unaffected) ## Validation - 4 new unit tests in `tests/plugins/toolsets/test_multi_instance.py` (26/26 pass) - `210_confluence_skill_fetch` passes locally with opus-4.6; the rendered system prompt now contains the full gateway base URL including the cloud id - The `evals-id-210_confluence_skill_fetch` label is set on this PR so CI runs the eval here https://claude.ai/code/session_01VLkU3Rj1FoXXpkmNAyS6vo --- _Generated by [Claude Code](https://claude.ai/code/session_01VLkU3Rj1FoXXpkmNAyS6vo)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Multi-instance toolsets now correctly propagate runtime-generated instructions from child instances to the wrapper, with per-instance organization when multiple are configured. * **Tests** * Added comprehensive test coverage for LLM instruction propagation in multi-instance toolset configurations, including single-instance, multi-instance, and offline instance scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
…server (#2182) ## Summary Two server-side tracing improvements for driving Holmes via the HTTP API: - **Opt-in per-request experiment routing**: API callers can route a request's trace spans into a named tracing experiment via the `X-Braintrust-Experiment` header. This is gated behind `HOLMES_ALLOW_PER_REQUEST_EXPERIMENT=true` (off by default), so external callers cannot influence experiment routing unless the operator explicitly enables it. Without an open experiment, `server_tracer.start_trace` has no tracing context to attach to and returns a no-op span — server-side spans are silently dropped. This lets a driver (e.g. an eval harness firing many `/api/chat` calls) group all spans for one logical run under a dedicated experiment name. - **Token/cost metrics on the investigation span**: attach `prompt_tokens`, `completion_tokens`, `total_tokens`, and `total_cost` as metrics on the investigation span in the non-streaming chat path. Tracing backends derive their token/cost columns from span metrics; without these the columns stay empty even though the span itself is recorded. ## Notes - The experiment context is process-global (Braintrust tracks a current experiment per process), so per-request routing assumes one logical client per server process — e.g. a server spawned per run. Repeating the current name is a no-op; a new name switches the experiment. https://claude.ai/code/session_019R4cF5Xvdc9xS33DCkyGuH --- _Generated by [Claude Code](https://claude.ai/code/session_019R4cF5Xvdc9xS33DCkyGuH)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Opt-in per-request experiment routing via request headers to group tracing spans into named experiments. * Automatic recording of LLM token usage and cost metrics onto investigation traces for improved observability and reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
#2187) ## Problem The eval correctness judge runs with `use_cot`, so its rationale and verdict share a single JSON tool call. autoevals' default `max_tokens=512` truncates that JSON when the rationale is long (large evaluation outputs), and autoevals raises `json.decoder.JSONDecodeError: Unterminated string` with no error handling: ``` tests/llm/utils/property_manager.py:234: in update_test_results correctness_eval = evaluate_correctness( tests/llm/utils/classifiers.py:229: in evaluate_correctness correctness_eval = classifier( .venv/.../autoevals/llm.py:240: in _process_response args = json.loads(tool_call["function"]["arguments"]) E json.decoder.JSONDecodeError: Unterminated string starting at: line 1 column 12 (char 11) ``` The crash happens before any score, cost, or token metric is recorded, so the test shows as failed with completely empty metric columns even when Holmes answered correctly. `95_skill_memory_leak_detection` hit this in two consecutive CI runs on #2175 (its long memory-leak investigation produces a long judge rationale). ## Fix - Raise the judge's `max_tokens` to 4096 so the rationale + verdict fit. - Retry the classifier call up to 3 times on `JSONDecodeError` for genuinely transient truncation. Test-infra only; no product code touched. https://claude.ai/code/session_01VLkU3Rj1FoXXpkmNAyS6vo --- _Generated by [Claude Code](https://claude.ai/code/session_01VLkU3Rj1FoXXpkmNAyS6vo)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved reliability of LLM evaluation by adding automatic retry handling for transient classifier failures. * Reduced evaluation truncation by increasing token capacity for evaluation responses. * Decreased flakiness of correctness scoring, leading to more stable evaluation results. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
## Summary Fixes #2102 (ROB-267): setting `OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf` had no effect — the gRPC exporter was always used, blocking OTLP/HTTP-only backends like self-hosted Langfuse. ### Root cause `holmes/core/otel_tracing.py` hardcoded the gRPC OTLP exporters and never read `OTEL_EXPORTER_OTLP_PROTOCOL`, even though [the docs](https://holmesgpt.dev/reference/opentelemetry/#environment-variables) list `http/protobuf` as supported. Additionally, the `otel` poetry group (installed in the server image via `--with otel`) didn't include `opentelemetry-exporter-otlp-proto-http` at all. ### Changes - Select gRPC or HTTP exporters at runtime based on `OTEL_EXPORTER_OTLP_PROTOCOL` (default: `grpc`, unchanged behavior). Semantics mirror the OTel Python SDK's own auto-configuration (`_get_exporter_entry_point`): default to gRPC, raise on unsupported values (e.g. `http/json`, which the Python SDK doesn't ship). - For OTLP/HTTP, append per-signal paths (`/v1/traces`, `/v1/metrics`) to the base endpoint per the OTel spec (skipped if already present), and default to port 4318 instead of 4317. `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT` is used verbatim (it's the spec's per-signal var). - Add `opentelemetry-exporter-otlp-proto-http` to the `otel` poetry group so the Docker image ships the HTTP exporter. Lock change is minimal (content-hash only — the package was already locked via the dev group). - Docs: note about base-URL + auto-appended signal paths for `http/protobuf`. ### Testing - 9 new unit tests in `tests/test_otel_tracing.py` (written first, failed before the fix): protocol selection, signal-path appending, no double-append, 4318 default, invalid-protocol error, env normalization, end-to-end tracer wiring. All 30 tests in the file pass. - Verified end-to-end on a live AKS cluster: deployed an OTel collector with an **HTTP-only** OTLP receiver (no gRPC port — replicating the Langfuse constraint), pointed Holmes at it with `OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf`, ran investigations via `/api/chat`, and confirmed full traces (`holmesgpt.investigation` → `gen_ai.chat` → tool spans) and metrics arriving at the collector, plus visualized in Jaeger. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * OTLP exporter now supports both gRPC and HTTP/protobuf, selectable at runtime via environment variables. * **Documentation** * Updated the OpenTelemetry reference diagram to reflect dual-protocol OTLP transport. * **Tests** * Added coverage for OTLP protocol selection, HTTP endpoint path construction (including overrides and no double-appending), environment parsing, and invalid protocol handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Mohse Morad <moshemorad12340@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Problem A customer reported Holmes responses getting cut off mid-answer. Their chat metadata showed: ```json "max_completion_tokens_per_call": 4096, "finish_reason": "length", "max_output_tokens": 64000, "max_tokens": 1000000 ``` `completion_tokens` landed at exactly 4096 with `finish_reason: "length"` — the model hit a hard 4096 output cap, even though Holmes computed (and reported) a 64000-token output budget. **Root cause:** `get_maximum_output_token()` is used to reserve output space during input budgeting and compaction (`input_context_window_limiter.py`, `compaction.py`), but it was never sent on the actual request — `DefaultLLM.completion()` passed no `max_tokens` to litellm. litellm then falls back to provider defaults. For Anthropic-family models, litellm resolves the default from its cost map; when the model name isn't in the map (proxy aliases, custom gateways — the same situation that makes users configure `max_context_size` by hand), it falls back to `DEFAULT_ANTHROPIC_CHAT_MAX_TOKENS = 4096`. Long answers get silently truncated while the metadata claims a 64000 budget. ## Fix `DefaultLLM.completion()` now always sends an explicit `max_tokens`, using the same value input budgeting already reserves. Precedence: 1. Explicit `max_tokens` / `max_completion_tokens` in model args — always wins (and a user-set `max_completion_tokens` blocks injection so no conflicting pair is sent). 2. `OVERRIDE_MAX_OUTPUT_TOKEN` env var — now actually reaches the request instead of only affecting compaction math. 3. Computed: `min(64000, context_window / 5)`, capped by the model's `max_output_tokens` from litellm's cost map when known. `max_tokens: null` / `max_completion_tokens: null` config sentinels are stripped, mirroring the existing `temperature: null` handling (PR #698 semantics). Provider safety: litellm 1.83.7 translates `max_tokens` per provider — `max_completion_tokens` for OpenAI o-series/gpt-5 reasoning models, `maxTokens` for Bedrock converse, `maxOutputTokens` for Gemini — so sending it is safe across providers (verified against the pinned litellm). ## Changes - `holmes/core/llm.py` — inject `max_tokens` in `DefaultLLM.completion()` (all three call sites benefit: agentic loop, compaction, fast-model summarization) - `tests/core/test_llm_completion_max_tokens.py` — new behavior-matrix tests, including a reproduction of the customer scenario (unknown model + `max_context_size: 1000000` → `max_tokens: 64000`, not 4096) - `tests/core/test_llm_completion_temperature.py`, `tests/core/test_llm_completion_cache_control.py` — test helpers that bypass `__init__` now set `max_context_size` - `docs/reference/context-management.md` — document the output token limit and its resolution order ## Testing - `tests/core/test_llm_completion_max_tokens.py` — 8 new tests, all pass - Full non-LLM suite: **2487 passed, 91 skipped** (skips are missing-credential environment skips, pre-existing) ## Note for operators Models unknown to litellm's cost map now receive a computed `max_tokens` instead of none. If the computed value exceeds what the upstream model actually supports, the provider may reject the request with an explicit error instead of silently truncating at its default — set `OVERRIDE_MAX_OUTPUT_TOKEN` or `max_tokens` in model args to the correct value for the model. https://claude.ai/code/session_01LPu4dG5LMjRgQgpBUsWhKw --- _Generated by [Claude Code](https://claude.ai/code/session_01LPu4dG5LMjRgQgpBUsWhKw)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * LLM completion requests now consistently include an explicit output-token budget to help prevent mid-response truncation. * Output-token cap resolution is updated with clearer precedence and correct behavior when `max_tokens` and `max_completion_tokens` are both present. * **Documentation** * Context management docs now include an “Output Token Limit” section explaining the enforced cap and its resolution order. * **Tests** * Added coverage to verify output-token limit injection, stripping, environment overrides, and model-specific capping. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
## Summary Add automatic retry logic for transient infrastructure errors when updating conversation status in Supabase, while ensuring that MISMATCH errors (indicating the conversation was reassigned) are not retried and propagate immediately. ## Changes - **Retry mechanism**: Wrapped the RPC call in a `@retry` decorator that retries up to 3 times with exponential backoff (0.5s-2s) for transient errors like DNS failures, cache overflows, and 5xx gateway errors - **Selective retry**: Uses `retry_if_not_exception_type(ConversationReassignedError)` to skip retries for MISMATCH errors, allowing the worker to exit cleanly when a conversation has been reassigned - **Error handling**: Converts MISMATCH errors to `ConversationReassignedError` exceptions that bypass retry logic, while other exceptions are retried and eventually logged as failures - **Test coverage**: Added three new tests covering: - Successful retry after transient error - Exhaustion of retries returning False (not raising) - MISMATCH errors not being retried (immediate propagation) ## Implementation Details - Extracted the RPC call into an inner `_update()` function decorated with `@retry` - MISMATCH detection logic remains unchanged (case-insensitive string matching) - Outer try-except preserves the original behavior: `ConversationReassignedError` is re-raised, other exceptions are logged and return False - Updated log message to clarify that failures occur "after retries" https://claude.ai/code/session_01L6SMoR2U3Rbg1spsjnxAAR <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Bug Fixes * Conversation status updates are now more resilient to temporary network disruptions through intelligent automatic retry logic with exponential backoff, significantly improving robustness of critical background operations. * Improved error handling for conflicting or reassigned conversation states, ensuring the system responds appropriately without attempting unnecessary retries and maintains proper graceful shutdown behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
… publishing, caller headers (#2180) ## Overview Lets a Holmes instance (caller) run cluster-local tools (kubectl, in-cluster Prometheus, bash) on another cluster's Holmes (executor) through relay's platform-mcp. Design: `relay/docs/design/2026-06-10_remote-tool-execution.md`. Counterpart PRs: robusta-dev/relay#585 (platform-mcp + design), robusta-dev/robusta-storage#246 (RemoteToolCalls table + RPCs). ## Changes **Publishing (executor side)** - `Toolset.expose_remotely` (default `True` for `kubernetes/core`, `kubernetes/logs`, `bash`; everything else opt-in) and `Toolset.is_core` marker (`core_investigation`/TodoWrite, `skills`/fetch_skill, `robusta_platform_mcp`) that user config can never unset - `holmes_sync_toolsets` publishes `meta.remote_tools` (openai-format tool schemas + holmes version + llm_instructions + exposed_instances), excluding restricted and whole-tool approval-required tools - HolmesStatus heartbeat from the periodic refresh loop (`refresh_holmes_status` preserves the verified realtime flag) so `updated_at` acts as a liveness signal **Execution (executor side)** - `ToolCallWorker`: claims `RemoteToolCalls` rows (`claim_tool_calls` RPC), runs exactly one tool per row in a dedicated `TOOL_CALLER_MAX_CONCURRENT=10` pool — version guard, is_core/exposure/instance checks, pre-approved-only approval handling (approval-needed bash commands are denied immediately), 1MB uncompressed cap, gzip over 100k, no disk writes — and posts the result atomically via `post_remote_tool_call_result` - `RealtimeManager` renamed `RealtimeWorker`: owns all generic realtime plumbing (connection, auth refresh, reconnection) and routes broadcasts to `ConversationWorker.claim_pending_conversations()` / `ToolCallWorker.claim_pending_tool_calls()` **Caller side** - `RobustaPlatformMCPToolset` always sends `X-Robusta-Holmes-Version` / `X-Robusta-User-Id`; a `RobustaPlatformMCPTool` subclass enriches `request_context` with the per-call `tool_call_id`/`max_token_count` (the single-tool token budget is a function of the caller's LLM and cannot be derived on the executor) - `refresh_platform_mcp_tools()` re-discovers the dynamic tool surface each refresh cycle and patches the live ToolExecutor — comparing full schemas, not just names (the `agent_name` enum grows when a cluster joins while the tool name stays the same) ## Tests performed - 73 unit tests green (`test_realtime_manager`, `test_worker_lifecycle`, `test_holmes_sync_toolsets`) - Live e2e (relay `dev/local-stack/remote-tools-e2e/`, staging Supabase): **117/117 checks with 10 Holmes instances** — LLM-driven multi-cluster aggregation from two different callers, broadcast-wake latencies of 0.1–0.2s with polling pinned at 300s, 10-call concurrency burst, executor kill → timeout → restart recovery, remote bash pre-approved vs denied, compression, unicode fidelity https://claude.ai/code/session_019yTHwWArrsh9yJpephyJVR --- _Generated by [Claude Code](https://claude.ai/code/session_019yTHwWArrsh9yJpephyJVR)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added cross-cluster remote tool execution with a dedicated worker, plus environment-based controls for concurrent claims and maximum remote result size/compression behavior. * Enabled publishing and remote execution eligibility metadata for selected toolsets with per-instance locality filtering. * Added periodic refresh for platform remote-tool discovery. * **Behavior Changes** * Updated realtime routing so both conversation processing and remote tool-call execution wake consistently. * Introduced clearer “core/internal” vs “remotely exposable” rules for toolsets and instances; improved Holmes status heartbeat refresh. * **Tests** * Added unit tests and LLM fixtures covering remote execution, result serialization/compression boundaries, routing/locality, and relay header sanitization. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
## Summary
Moves retry logic from the event publisher layer into the Supabase DAL,
centralizing transient error handling and distinguishing between
retryable infrastructure errors and non-retryable semantic errors
(conversation reassignment).
## Key Changes
- **`supabase_dal.py`**:
- Added bounded exponential backoff retry (3 attempts, 0.5-2.0s delays)
to `claim_conversations()` for transient infrastructure errors (DNS,
5xx, proxy cache overflows)
- Added bounded exponential backoff retry (3 attempts, 0.5-2.0s delays)
to `post_conversation_events()` with selective retry logic:
- Retries all exceptions except `ConversationReassignedError`
- Detects "mismatch" errors (assignee/request_sequence/status guard
failures) and promotes them to `ConversationReassignedError` without
retry
- Allows caller to distinguish between transient failures (retry at
higher level) and reassignment (exit cleanly)
- Updated error log messages to indicate retries have been exhausted
- **`event_publisher.py`**:
- Removed local retry decorator and `_TransientPostError` helper class
- Simplified `_post_with_retry()` to call DAL directly and handle only
`ConversationReassignedError` (which now comes from DAL)
- Removed `RetryError` handling from `_flush()` since DAL now handles
all retries
- Added defensive mismatch detection in event publisher as fallback
- **`test_dal_contract.py`**:
- Added 6 new tests covering retry behavior:
-
`test_post_conversation_events_retries_transient_error_then_succeeds()`:
Verifies transient errors are retried
- `test_post_conversation_events_raises_after_exhausting_retries()`:
Verifies exhausted retries re-raise
- `test_post_conversation_events_does_not_retry_mismatch()`: Verifies
mismatch errors fail fast without retry
- `test_claim_conversations_retries_transient_error_then_succeeds()`:
Verifies transient errors are retried
- `test_claim_conversations_returns_empty_after_exhausting_retries()`:
Verifies exhausted retries return empty list
## Implementation Details
- Retry logic uses `tenacity` library with `retry_if_exception_type()` /
`retry_if_not_exception_type()` for selective retry
- `claim_conversations()` returns empty list on failure (non-raising) to
allow poll cycles to continue
- `post_conversation_events()` re-raises after retries so caller can
decide whether to retain events or fail
- Lazy import of `ConversationReassignedError` in DAL avoids circular
dependency with conversations_worker
- Exponential backoff parameters (0.5-2.0s) prevent thundering herd on
infrastructure recovery
https://claude.ai/code/session_01DXaVPZejKoNwZsPTKoaxaZ
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
## Release Notes
* **Improvements**
* Enhanced resilience when posting and retrieving conversation events,
including safe handling of unexpected response shapes.
* Improved mismatch/reassignment behavior to ensure the correct error is
raised and that event posting doesn’t lose pending work on transient
failures.
* Added broader retry handling across key conversation-related RPC
operations while avoiding retries for reassignment mismatch scenarios.
* **Tests**
* Expanded unit test coverage for retry and mismatch/error handling
behavior.
* Added an in-process integration test that verifies conversation
completion through simulated transient backend failures, plus updated
integration test configuration to use real services.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Signed-off-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
### Problem
When HolmesGPT's configuration needs to change (toolsets, models,
runbooks), the only option today is to restart the entire server
process. This causes downtime, drops in-flight requests, and is slow in
environments where the container takes time to initialize (loading
toolsets, connecting to databases, etc.).
This is especially painful in production Kubernetes/Docker deployments
where config files are mounted via ConfigMaps or bind mounts and updated
frequently.
### Changes
- **`holmes/config.py`**: Add `reload_toolsets()` and `reload_models()`
methods with thread-safe locking. Add lock protection to
`llm_model_registry` lazy property.
- **`holmes/admin/admin_api.py`** *(new)*: FastAPI sub-app mounted at
`/api/admin` with three POST endpoints:
- `POST /api/admin/reload/toolsets` — re-reads config YAML, rebuilds
toolsets/runbooks
- `POST /api/admin/reload/models` — re-reads `model_list.yaml`, rebuilds
model registry
- `POST /api/admin/reload` — reloads everything
- **`server.py`**: Mount admin sub-app (2 lines)
- **`tests/test_config_reload.py`** *(new)*: Unit + integration tests
### Example response
```json
{
"status": "ok",
"component": "all",
"detail": "50 toolsets (15 enabled), 4 models",
"counts": {
"toolsets_total": 50,
"toolsets_enabled": 15,
"runbooks": 3,
"models_loaded": 4
}
}
```
### Test plan
- [x] Unit tests for `reload_toolsets()` (reset state, YAML pickup,
no-config edge case)
- [x] Unit tests for `reload_models()` (registry reset, model count)
- [x] Integration tests for all 3 endpoints (200 happy path, 500 error
path)
- [x] Manual testing against live Docker deployment with bind-mounted
config
### Notes
- Admin endpoints are unauthenticated, matching existing API patterns.
- Reload is safe during active requests — in-flight requests use old
config, new requests pick up reloaded config.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added three unauthenticated admin POST endpoints: /api/admin/reload
(all), /api/admin/reload/toolsets (toolsets/runbooks), and
/api/admin/reload/models (model registry). Each returns operation counts
and human-readable details; failures include error detail. Endpoints are
only active when the admin API is enabled in server config.
* **Documentation**
* API reference updated with endpoint details, example 500 payload for
model reload, and a note advising network-level access restriction.
* **Tests**
* Added tests for reload operations, endpoint responses, and error
propagation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Signed-off-by: theTibi <tkorocz@gmail.com>
Co-authored-by: moshemorad <moshemorad12340@gmail.com>
…sign-in (#2193) ## Summary Improve error handling during Supabase authentication to detect and clearly communicate firewall/egress policy issues that block connections to the Robusta platform. When Holmes fails to connect during sign-in, it now distinguishes between DNS failures, firewall blocks, and genuine authentication errors, providing actionable guidance to users. <img width="1047" height="496" alt="firewallconnectionresetlog" src="https://github.com/user-attachments/assets/0fd3a533-cefc-4859-b58e-f5bb0012b2cd" /> ## Key Changes - **New Exception Classes**: Added `SupabaseConnectionException` for connection reset/refused/timeout errors that indicate firewall blocks, distinct from `SupabaseDnsException` for DNS resolution failures - **Enhanced Error Detection**: Updated `SupabaseDal.sign_in()` to classify connection errors by examining exception types and error messages, detecting patterns like "connection reset by peer", "connection refused", "connection timed out", and errno codes (104, 111) - **User-Friendly Messaging**: Connection exceptions now include: - Clear explanation that the issue is almost always an outbound firewall/egress policy - Specific guidance to allowlist `*.robusta.dev` for HTTPS (port 443) - A diagnostic curl command targeting the configured platform URL's health endpoint - Link to troubleshooting documentation - **Actionable Logging**: Added WARNING-level log message before raising connection exceptions (not ERROR, to avoid spurious Sentry alerts) with the same guidance - **Preserved Auth Errors**: Genuine authentication errors (e.g., invalid credentials) propagate unchanged without wrapping, so users see the actual auth problem - **Documentation**: Added troubleshooting section to docs explaining the firewall block scenario, how to confirm it with curl, and how to fix it ## Implementation Details - Connection error detection uses both exception type checks (`ConnectionError`, `TimeoutError`) and string pattern matching on error messages to catch various ways httpx and other libraries surface network failures - The health check URL is dynamically constructed from the configured platform URL to match the user's region/deployment - Logging is kept at WARNING level (not ERROR) to prevent false Sentry alerts for infrastructure issues outside the application's control - All three exception types (DNS, connection, auth) are tested with comprehensive unit tests covering success and failure paths https://claude.ai/code/session_01KVrzw7n3HtrfbSEZtRXZby <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Documentation** * Added a new troubleshooting item for startup failures when outbound firewall/egress resets connections, including the typical traceback, HTTPS (443) allowlisting guidance for Robusta subdomains (including `sp`), and a simple `kubectl`/`curl` connectivity check. * Updated `robusta-region` documentation and behavior to rewrite `sp.robusta.dev` in addition to existing Robusta subdomains. * **Improvements** * Enhanced sign-in handling for connection-related failures with clearer firewall guidance and a troubleshooting reference. * **Tests** * Added coverage for sign-in error classification (connection/reset vs DNS vs credential errors). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
operator mode improvements <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Triggered Health Checks (alpha): automatic, per-deployment rollout health checks (image/template changes) with configurable delay and cooldown; optional inline one-time checks for CI/CD gating. * **Documentation** * Comprehensive Triggered Health Checks guide added and linked from the operator overview. * Deployment verification docs rewritten to compare automatic triggers vs. inline gating and include timing/usage tips. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: arik <alon.arik@gmail.com>
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Removed Features** * Removed tool command editing capability during tool approval workflows. Tools now execute with their originally proposed commands without modification options. * **Bug Fixes** * Improved backward compatibility to silently handle legacy approval payloads containing command override values, preventing validation errors from older clients. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Roi Glinik <groi.tech@gmail.com>
…ProtocolError retry) (#2200) ## Summary (ROB-4017) Hardens the Supabase DAL client against intermittent connection failures (dropped `HolmesStatus` upserts, dropped conversation claims) caused by a shared client under concurrency and by Supabase's edge closing idle keep-alive connections. This is a **single PR** consolidating what were two stacked branches for the same ticket — the base client-wiring and the `RemoteProtocolError` retry follow-on. It now targets `master` directly and supersedes #2122 (which can be closed). ## Changes - **Force HTTP/1.1** for the Supabase client. httpcore's *sync* HTTP/2 connection isn't thread-safe, and one `SupabaseDal` client is shared across the conversation worker, realtime callbacks and request threads — under concurrency the HTTP/2 framing corrupts and calls fail with `RemoteProtocolError: Server disconnected`. - **Honor the environment CA bundle** (`SSL_CERT_FILE` / `REQUESTS_CA_BUNDLE`) via an explicit `SSLContext`, rather than letting our own httpx client fall back to certifi (which breaks TLS verification behind an intercepting proxy). Passes an `SSLContext`, not a path string, since httpx has deprecated `verify=<str>`. - **Retry `RemoteProtocolError` at the transport** (`SupabaseRetryTransport`, an `httpx.HTTPTransport` subclass using tenacity's `@retry`). Even on HTTP/1.1, Supabase's edge closes idle keep-alives, so a reused connection fails with "Server disconnected" *before* the request reaches Supabase — safe to replay on a fresh connection. Hardening at the transport covers every sub-client (postgrest, auth, storage, realtime) uniformly, with no backoff (a reaped socket just needs a fresh connection). - **`CLAUDE.md`**: documents the convention to prefer `tenacity` for retries over hand-rolled loops. ## Tests Deterministic, no network: `test_supabase_dal_retry.py` pins the retry contract (retry-then-succeed, budget exhaustion, non-retryable passthrough); `test_supabase_dal_transport.py` pins the client wiring (transport built, `http2` off, `verify` as `SSLContext`, client forwarded to postgrest). Mirrors the relay-side fix (ROB-4012). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_015thi2RsgXWp8BzuDCfxNzG <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Improved Supabase connectivity by retrying transient remote-protocol disconnects (with retry attempt logging) while avoiding retries for non-transient failures. * Unified HTTP client and TLS certificate verification behavior across the data layer. * **Tests** * Added coverage for retry success, retry exhaustion, and immediate failure for non-retriable errors. * Expanded tests to confirm correct transport and TLS configuration. * **Documentation** * Updated contribution guidance to prefer tenacity-based retries over custom retry loops. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
…ill evals (271-282) (#2175) ## What Improve the quality of the skills Holmes suggests via the UI's skill-suggestion frontend tool, and build the eval infrastructure to measure it end-to-end. Companion frontend PR: robusta-dev/robusta-frontend (SuggestRunbooks → SuggestSkills rename + new description/prompt snippet). **Problem:** the suggestions produced by the old `SuggestRunbooks` tool were incident memories (root causes, resource names, timestamps) that don't generalize. The goal is durable **data-source know-how** — the kind of notes customers hand-write in toolset `llm_instructions` today: index patterns, exact field/label names, `.keyword` quirks, query recipes, pitfalls. ## Eval framework - `frontend_tools` field on `test_case.yaml`: injects client-defined tools through the same `inject_frontend_tools()` path the server uses for `/api/chat`, including the client's `additional_system_prompt` snippet. The shared definition under iteration lives in `tests/llm/fixtures/shared/skill_suggestion_tool.yaml` and mirrors the frontend. - **Closed-loop replay** (ported from `claude/consolidated-skills-per-domain` and adapted): `memories_generated` hard assertions, `rerun_with_memory` (captured suggestions are rendered as SKILL.md files and a second pass must fetch them and still answer correctly), `replay_user_prompt`, `expected_replay_output`, `replay_forbidden_tools`, `pre_loaded_skills_path`, `expected_skill_count`. - Judge sees emitted suggestions as a structured `# Suggested Skills` block; GitHub report gains Skill Generated / Skills Read columns, inline `[replay]` rows, and replay-vs-primary cost/token stats. `TestStatus.passed` now also requires pytest success. - Unit tests for the new helpers. ## Evals - **271–274** (new): ES schema quirks, k8s topology mapping, Prometheus label conventions, and a no-spam negative test. Anti-memorization canary pattern: a unique error code must appear in the answer but NOT in the suggested skill. - **275–282**: ported from `claude/consolidated-skills-per-domain` (their 261–268), renumbered and adapted to `frontend_tools` injection. 269 was intentionally not ported (depends on the `skill_domain` consolidation implementation, which this PR does not adopt). - **No-spam guards** on baseline evals (09, 12, 24, 43, 51, 61, 176, 227, 243): tool injected as in production, zero suggestions allowed. 101/112 are injection-only — under the current spec, capturing Loki label conventions / a fixture-specific annotation key is compliant capture, not spam. ## Key findings (opus-4.6, all results in eval comments once CI runs) - Holmes' core prompt ("gather information with tools, then respond") suppresses any end-of-investigation tool call: 0/6 trigger rate. The system-prompt snippet must explicitly carve out the workflow `investigate → SuggestSkills → final answer`: 6/6 after the change. - 10/12 skill evals pass closed-loop, including bad-skill resilience (a confidently-wrong pre-loaded skill doesn't derail the answer) and all baselines. - Two evals are tagged `hard` with status notes and are **expected red**: - `272`: the k8s topology-mapping scenario triggers only ~25% (mapping found via routine listing — no failed query, no schema lookup). - `278`: on replay the agent fetches AND cites the captured skill but still re-verifies mappings in parallel, failing the strict no-re-exploration bar. The same verify-first habit is what makes the bad-skill eval pass, so this is a deliberate, visible trade-off rather than a bug. ## How to run ``` poetry run pytest -m "llm and skills" --no-cov ``` The `evals-tag-skills` label on this PR makes the CI eval run include the skills-tagged evals alongside the regression set. https://claude.ai/code/session_01VLkU3Rj1FoXXpkmNAyS6vo --- _Generated by [Claude Code](https://claude.ai/code/session_01VLkU3Rj1FoXXpkmNAyS6vo)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Closed-loop skill-suggestion workflow: capture suggested skills during runs, materialize them as replayable skills, and validate replay behavior with no‑spam guards. * **Tests** * Added/updated many evaluation fixtures for Elasticsearch, Prometheus, Kubernetes, Loki covering skill-suggestion, schema/metric/label quirks, and no-spam scenarios. * New unit tests for frontend-tool loading, skill-suggestion utilities, and test-result classification. * **Chores** * Test harness, reporting, and logging extended to record and report suggestion/memory and replay metrics. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
## Summary Reorganizes Kubernetes permissions documentation by consolidating content from the dedicated reference page into the Kubernetes toolset documentation, reducing duplication and improving information discoverability. ## Key Changes - **Moved permissions content** from `docs/reference/kubernetes-permissions.md` to `docs/data-sources/builtin-toolsets/kubernetes.md` under a new "Permissions" section - **Replaced reference page** with a redirect to the new location in the toolset documentation - **Reorganized subsections** in the toolset page: - "How HolmesGPT Inherits Permissions" (with updated relative links) - "Adaptive Behavior" - "Recommended Permissions" - "Adding Permissions for Additional Resources" (now a subsection with heading hierarchy adjustments) - "Using an Existing ServiceAccount" (moved from reference page with both Holmes and Robusta Helm chart examples) - **Updated cross-references** in `docs/data-sources/permissions.md` redirect to point to the new section anchor ## Implementation Details - Preserved all original content and formatting (important notes, code examples, links) - Adjusted relative paths in moved content to account for new location depth - Maintained heading hierarchy consistency within the toolset page - Added both Holmes and Robusta Helm chart configuration examples for ServiceAccount usage - Updated redirect script to use correct anchor link https://claude.ai/code/session_014WeVkooay551s9TScq36CM <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded Kubernetes toolset permissions guidance, clearly stating the built-in toolset is read-only by default and cannot create/update/delete Kubernetes resources. * Added details on how permissions are derived in local vs in-cluster execution, including adaptive behavior when permissions are missing and recommended default read coverage. * Included updated instructions for using existing ServiceAccounts (with chart configuration examples). * Improved navigation by reorganizing headings and updating a separate permissions page with a redirect to the new permissions anchor. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
…2178) ## Summary Restructured the custom skills documentation to improve clarity and usability by organizing content around deployment methods (Holmes Helm Chart, Robusta Helm Chart, Holmes CLI) rather than skill loading approaches. This makes it easier for users to find instructions relevant to their specific Holmes deployment. ## Key Changes - **Reorganized top-level structure**: Replaced skill-loading-method tabs (Inline, Self-mounted ConfigMap/Secret, GitHub repo) with deployment-method tabs (Holmes Helm Chart, Robusta Helm Chart, Holmes CLI) - **Nested skill loading approaches**: Within each deployment method, users now see tabs for different ways to load skills (GitHub Repository, Inline in Helm Values, ConfigMap/Secret) - **Added deployment method selector**: New introductory section explaining the three main Holmes deployment options and noting that selection is remembered across the site - **Improved GitHub repo instructions**: Clarified the alpha status and added step numbers (1, 2, 3) for better readability - **Consolidated CLI documentation**: Moved CLI and Python SDK instructions under a single "Holmes CLI" section with clearer guidance on using GitHub repos locally - **Updated Robusta-specific examples**: Ensured all Robusta examples reference `generated_values.yaml` and use `robusta-holmes` deployment name consistently - **Removed redundant content**: Eliminated duplicate examples and consolidated similar patterns across deployment methods ## Implementation Details - Maintained all existing configuration examples and technical details - Preserved the alpha warning for GitHub repo support - Kept the note about Holmes scanning paths up to 2 levels deep for `SKILL.md` files - All code examples remain functionally identical, just reorganized for better discoverability https://claude.ai/code/session_01HBwGKqEMxNTqmqnUPrPGKs <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Refreshed Holmes 0.26.0+ custom-skill reference: clearer built-in skills notes, simplified overview, and updated “Loading Custom Skills” guidance across Holmes Helm, Robusta Helm, and Holmes CLI/SDK. * Expanded advanced ConfigMap/Secret instructions, including `<skill-name>/SKILL.md` layout, merge behavior, and update timing. * **New Features** * Added a gated “Which Holmes are you running?” deployment picker that persists selections and syncs them with the URL. * **Style** * Added styling for the deployment picker overlay and its gated/enhanced states. * **Chores** * Wired the deployment picker script into the MkDocs site. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Evals were running on every PR commit (eval-regression.yaml on pull_request synchronize) and every push to master (eval-master.yaml), which was the main CI eval cost driver even when no one looked at the results. Switch both to on-demand: - eval-regression.yaml: trigger only on the `labeled` pull_request event instead of opened/synchronize/reopened/labeled, and skip by default in the automatic path unless the PR carries an evals-* label (evals-tag-*, evals-id-*, evals-model-*). /eval comments and workflow_dispatch are unaffected, so on-demand runs still work. - eval-master.yaml: drop the push-to-master trigger; refresh the baseline on demand via workflow_dispatch. The weekly benchmark cron (eval-benchmarks.yaml) is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BzLq3Ubqo1TTonzkUASL3X Signed-off-by: Claude <noreply@anthropic.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Evaluation workflows changed from automatic execution on every change to opt-in via manual triggers and pull request labels. * Baseline evaluation refresh now requires explicit manual workflow dispatch instead of automatically triggering on master branch changes. * Pull request evaluation runs now default to skipped, requiring explicit label opt-in or manual dispatch to execute. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
## Summary - Fix `OAuthToolConnector._evict_expired_token`: was gated to `DiskTokenStore` (CLI only). In server mode (`DalTokenStore`), the expired Supabase row was never deleted → frontend kept rendering "Failed" with no clickable recovery path → user stuck. - Clear stale `_user_tools[user][toolset]` on the same 401 branch. Without this, `apply_user_tools` keeps substituting the now-dead tool list for the `_connect` placeholder, 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 - [x] New unit tests in `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]` — confirms `delete_token` is now called for the DAL store - `test_401_deletes_token_in_both_stores[DiskTokenStore]` — confirms disk-store behaviour is preserved - `test_401_clears_stale_user_tools` — confirms `_user_tools` cleanup - [x] Live verified on a real cluster against Atlassian's hosted MCP (`https://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. - [x] All 121 adjacent OAuth tests still pass (`tests/test_mcp_oauth.py tests/core/tools_utils/`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved OAuth authentication error handling for 401/403 failures by clearing expired tokens and removing any stale, per-user tools associated with the affected toolset. This prevents the app from substituting dead tools after an auth error, allowing the UI to recover cleanly and return to a fresh login state. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Roi Glinik <groi.tech@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every tool call marked pending_approval gets a server-minted HS256 JWT
bound to {tool_call_id, tool_name, args_hash}. The resume path refuses
to execute any pending_approval whose ticket doesn't verify.
Surface:
- holmes/utils/approval_tickets.py — JWT mint/verify, canonical args
hash, signing-key loader. HOLMES_APPROVAL_SIGNING_KEY env var (used
as-is, no decoding); ephemeral fallback when unset.
- holmes/core/models.py — PendingToolApproval gains approval_ticket.
- holmes/core/tool_calling_llm.py — mint at the pending_approval site;
verify before executing tool_decisions; on failure synthesize a denial
decision so the existing deny pipeline produces a TOOL_RESULT with ERROR
status and the LLM explains the rejection to the user in chat. No new
SSE event types, no relay shim, no FE-side wire changes.
- server.py — one-line INFO log when the env var is set.
- docs/reference/environment-variables.md — new
HOLMES_APPROVAL_SIGNING_KEY section with openssl key-gen snippet and
30-day TTL note.
Tests:
- tests/test_approval_tickets.py — mint/verify round-trip, expiry,
tampering, alg=none rejection, canonicalization, key-loader fallback.
- tests/test_approval_ticket_security.py — security regressions for
forged pending_approval, tampered args, cross-call ticket reuse, and the
happy-path round-trip.
- tests/test_edit_command_removed.py — adapted to attach a real ticket
to the synthetic pending_approval message.
Replay protection (jti + redemption cache) and multi-replica caveats are
intentionally out of scope here.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added signed approval tokens to gate resuming tool execution, binding
approvals to the specific tool call and its arguments for stronger
tamper protection.
* Approval tokens automatically expire after 30 days.
* **Bug Fixes**
* Improved rejection handling to ensure invalid or tampered approvals
are safely denied and never executed; orphaned pending approvals are
cleaned up.
* **Documentation**
* Documented `HOLMES_APPROVAL_SIGNING_KEY` for configuring the HMAC
signing key used for tool approvals.
* **Tests**
* Added security and unit test coverage for token signing, verification
failures, and replay/tampering scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Signed-off-by: Roi Glinik <groi.tech@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: moshemorad <moshemorad12340@gmail.com>
## Problem When the holmes **conversations-worker** runs an investigation and it fails — e.g. a Robusta-AI account hits its token/rate limit and relay returns 429 — the worker collapsed the exception into a generic `error_code: 5000` "An internal error occurred". The rate-limit reason was lost, so the UI couldn't tell the user they were rate limited. The live SSE path (`stream_chat_formatter`) already handled this; the worker path didn't. ## Fix - Catch chat-execution exceptions in `_run_chat_and_publish`, where `ai.llm.is_robusta_model` is in scope. - Route **all** rate limits (`_is_rate_limit_error` — litellm `RateLimitError` + Bedrock throttling) to `error_code 5204 "Rate limit exceeded"`, matching the live SSE path. Shared `RATE_LIMIT_ERROR_CODE` constant added in `holmes/utils/stream.py`. - For **Robusta-AI (relay) models only**, attach the full upstream error as a `raw_error` field on the error event — it originates from our own backend (carries `robusta_error_code`, e.g. 4829 `OPENAI_LIMIT_REACHED`, plus the support message) and is safe to surface. User-defined model errors stay generic; no provider-error leakage. Linear: ROB-448 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced error reporting to optionally include detailed error messages for Robusta-AI models, improving troubleshooting visibility and error diagnostics. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary The built-in AWS MCP addon images ([robusta-dev/holmes-mcp-integrations#30](robusta-dev/holmes-mcp-integrations#30), merged) were migrated off `supergateway`/SSE to run `awslabs.aws-api-mcp-server`'s **native MCP Streamable HTTP transport**, released as tag `2.1.0`. The native server serves only at `/mcp`. The chart still pointed at the old `2.0.1` image and generated an MCP URL with no `/mcp` path and no transport mode, so Holmes' streamable-http client hit `:8000` root and got `404`/`406` — it couldn't connect. ## Changes - **`helm/holmes/templates/toolset-config.yaml`** — in the `aws_api` MCP server config, append `/mcp` to the `url` and add `"mode" "streamable-http"`. - **`helm/holmes/values.yaml`** — bump both image tags under `mcpAddons.aws`: `aws-api-mcp-server` and `multiAccount.image` (`multi-aws-api-mcp-server`) from `2.0.1` → `2.1.0`. The `tcpSocket` health probes, ServiceAccount/IRSA, and `READ_OPERATIONS_ONLY` (read-only mode) env are left unchanged. ## Verification ``` $ helm template r ./helm/holmes --set mcpAddons.aws.enabled=true | grep -A5 aws_api aws_api: config: icon_url: https://raw.githubusercontent.com/gilbarbara/logos/.../aws.svg mode: streamable-http url: http://r-aws-mcp-server.default.svc.cluster.local:8000/mcp description: AWS API MCP Server - comprehensive AWS service access... ``` Rendered URL ends in `/mcp`, `mode: streamable-http` is present, and the rendered image tag is `aws-api-mcp-server:2.1.0`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01P2aD44gnGDWY68YNzpCkuq --- _Generated by [Claude Code](https://claude.ai/code/session_01P2aD44gnGDWY68YNzpCkuq)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** - Upgraded AWS MCP container images to version 2.1.0 for enhanced reliability - Updated AWS MCP server configuration to use optimized HTTP streaming mode <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
…rg-2PpZb-green-hypothesis Signed-off-by: Claude <noreply@anthropic.com> # Conflicts: # tests/llm/utils/test_case_utils.py
…criteria master consumed test numbers through 282, so 271_root_cause_buried_in_infra_noise collided with new 271_* evals. Renumber to 283 (dir + app-283 namespace). Also simplify expected_output to two short boolean criteria capturing only the core of the bug: (1) recognizes the task pods actually ran (Scheduled/Started events) so the failure is application-level, and (2) does not blame the unrelated scheduling/image-pull noise. Drops overfit-y requirements (literal pod-id token, Airflow-logs-specific wording) so a genuinely correct answer can't fail on phrasing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFEABQjccJGLsb9mZHoRpm Signed-off-by: Claude <noreply@anthropic.com>
…d events) The previous realistic noise (impossible nodeSelector, bogus image tag on unrelated apps) was too obviously disconnected from the task failure, and opus-4.6 solved the eval without help. Rebuild the temptation of the original injected-events fixture using only real cluster mechanisms: - Noise pods share the log-archival pipeline naming (ingest-worker, compactor, forwarder) so the whole DAG stack looks broken. - ingest-workers: real "FailedScheduling: Insufficient cpu/memory" via oversized resource requests (capacity-crisis flavor). - compactors: real ErrImagePull from unresolvable registry.internal, whose error message contains "dial tcp: lookup registry.internal: no such host" (DNS/network-outage flavor, pattern-matching the indexer's connection failure). - forwarders: real FailedCreatePodSandBox via a gVisor RuntimeClass whose runsc handler is not installed (the notorious event reason from the original bug, produced honestly). Every noise failure mode is start-blocking, and the task pods provably Scheduled/Pulled/Started, so none of it can be the cause. Criteria stay two boolean bullets; bullet 2 now also covers FailedCreatePodSandBox and states the exclusion principle. after_test cleans up the cluster-scoped RuntimeClass. All three event types verified live on k3s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFEABQjccJGLsb9mZHoRpm Signed-off-by: Claude <noreply@anthropic.com>
Summary
Stacked on #2150. That PR adds the RED eval
271_root_cause_buried_in_infra_noise, which reproduces the bug "infrastructure noise prioritized over application-level root cause": a failed AirflowKubernetesPodOperatortask whose pods provably ran (Scheduled/Pulled/Started events for7q4w9z-1/2/3) but were deleted on finish, surrounded by loud-but-unrelated AWS-CNI IP-exhaustion / Karpenter node-churn events. Holmes wrongly blames the infra noise.This PR is the attempt to make Holmes reason correctly on that scenario by giving the investigation a way to track competing root-cause hypotheses, mirroring how
TodoWriteoptionally tracks tasks.Changes
New
HypothesisWritetool (holmes/plugins/toolsets/investigator/)Hypothesismodel +HypothesisStatusenum (proposed / investigating / supported / refuted) inmodel.pyhypothesis_formatter.py— renders a "CURRENT ROOT-CAUSE HYPOTHESES" block (mirrorstodo_tasks_formatter)HypothesisWriteToolregistered alongsideTodoWriteToolin thecore_investigationtoolsetinvestigator_instructions.jinja2— adds guidance on distinguishing the real root cause from surrounding noise (determine whether the failing workload actually ran; scope infra events to the affected object; record a tempting-but-wrong explanation as a refuted hypothesis; prefer an honest "pod ran, app logs unavailable, check<source>" over blaming the loudest infra signal)Eval harness wiring
enable_hypothesistest-case flag mirroringenable_todo. Thecore_investigationtoolset is dropped unless a test opts into at least one of its tools, and its tool list is filtered so the LLM only sees the tools the test enabled.271_root_cause_buried_in_infra_noiseopts into both flags so Holmes can weigh infra-vs-application hypotheses.Status / honesty
The hypothesis tool makes Holmes's reasoning on this scenario markedly better (it explicitly weighs and refutes the infra-noise explanation instead of latching onto it), but on its own it does not reliably flip the eval to green — see discussion on #2150. This PR is the experiment; CI (real KIND, opus-4.6) on
271_*is the authoritative measurement.Non-LLM unit tests for
core_investigation/todo_writepass locally.https://claude.ai/code/session_01TFEABQjccJGLsb9mZHoRpm
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests