Repository navigation
Remove automated investigation API and consolidate to conversational interface - #1688
Merged
Merged
Conversation
… endpoints Purge the investigate endpoint and its issue_chat follow-up endpoint from the server, keeping only the general /api/chat endpoint. This removes: - Three server endpoints: /api/investigate, /api/stream/investigate, /api/issue_chat - Issue-chat-specific models: IssueChatRequest, ToolCallConversationResult, IssueInvestigationResult, ConversationInvestigationResult, HolmesConversationHistory, HolmesConversationIssueContext, ConversationType, ConversationRequest - build_issue_chat_messages() and truncate_tool_outputs() from conversations.py - stream_investigate_formatter() from stream.py - Related tests and API documentation The investigation module (holmes/core/investigation.py), InvestigateRequest, and InvestigationResult models are preserved as they are used by the LLM eval test suite. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
…estigator Phase 2 of investigation endpoint removal: - Port all CLI investigate commands (alertmanager, jira, ticket, github, pagerduty, opsgenie) to use ToolCallingLLM directly via prompt_call() - Remove IssueInvestigator class and sections parameter from ToolCallingLLM - Remove investigation factory methods from Config - Delete investigation infrastructure: investigation.py, investigation_structured_output.py, prompt templates, LLM eval tests - Clean up all references across test utils, docs, and prompt templates https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
Contributor
📂 Previous Runs📜 Run @ 181cd33 (#22807112075)✅ Results of HolmesGPT evalsAutomatically triggered by commit 181cd33 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 61 test/model combinations loaded Benchmark experiment:
Time comparison (seconds):
Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 Run @ 4743647 (#22806905076)✅ Results of HolmesGPT evalsAutomatically triggered by commit 4743647 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 61 test/model combinations loaded Benchmark experiment:
Time comparison (seconds):
Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 Run @ 469c69e (#22806844973)✅ Results of HolmesGPT evalsAutomatically triggered by commit 469c69e on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 61 test/model combinations loaded Benchmark experiment:
Time comparison (seconds):
Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 Run @ 71ccc8d (#22806755030)✅ Results of HolmesGPT evalsAutomatically triggered by commit 71ccc8d on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 61 test/model combinations loaded Benchmark experiment:
Time comparison (seconds):
Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 Run @ bf8b253 (#22806538013)✅ Results of HolmesGPT evalsAutomatically triggered by commit bf8b253 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 61 test/model combinations loaded Benchmark experiment:
Time comparison (seconds):
Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit 573ccbf on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals in automatic regression runs:
Examples: 🏷️ Valid tags
Commands: CLI: |
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:72b09984
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:72b09984 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:72b09984
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:72b09984
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:72b09984
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:72b09984 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:72b09984
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:72b09984Patch 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:72b09984 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:72b09984Robusta 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:72b09984 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:72b09984 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
The eval-regression workflow hardcoded tests/llm/test_investigate.py in pytest args, but this file was deleted in the previous commit. This caused pytest to error with "file or directory not found", and the `|| true` masked the failure, resulting in "No eval report was generated" on the PR. Now dynamically checks which test files exist before passing them to pytest. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request removes the investigation feature from the codebase, including the Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/llm/utils/braintrust.py (1)
303-313:⚠️ Potential issue | 🟠 MajorAttributeError when
test_caselacksconversation_historyattribute.
HolmesTestCase(per its definition withextra="forbid") does not have aconversation_historyattribute. Whentest_caseis not anAskHolmesTestCase, this line will raiseAttributeError. Line 259 correctly useshasattrfor the same check.Proposed fix using hasattr guard
- elif test_case.conversation_history: # compaction test case + elif hasattr(test_case, "conversation_history") and test_case.conversation_history: # compaction test case🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/utils/braintrust.py` around lines 303 - 313, The branch using "elif test_case.conversation_history" will raise AttributeError for HolmesTestCase instances that don't have conversation_history; change the guard to use hasattr(test_case, "conversation_history") and ensure it also checks truthiness (e.g., if hasattr(test_case, "conversation_history") and test_case.conversation_history:) so only AskHolmesTestCase-like objects enter that block; update the condition in the function where the block containing format_conversation_as_markdown and expected assignment runs to use this hasattr-based check.
🧹 Nitpick comments (2)
tests/llm/utils/braintrust.py (1)
209-219: Minor: Redundant conditional afterhasattrcheck.When
hasattr(result, "result")returnsTrue,resultis necessarily truthy, making the ternary conditionif resultredundant.Simplified version
if error: - if hasattr(result, "result"): - output = result.result if result else str(error) - else: - output = str(error) + output = result.result if result and hasattr(result, "result") else str(error) scores = scores or {} else: - if hasattr(result, "result"): - output = result.result if result else "" - else: - output = "" + output = result.result if result and hasattr(result, "result") else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/utils/braintrust.py` around lines 209 - 219, The conditional that checks hasattr(result, "result") is followed by redundant ternary checks of result (e.g., output = result.result if result else ...); simplify by removing the needless `if result` branches and directly use result.result when hasattr(result, "result") is true, updating both the error and non-error branches (variables: error, result, scores, and output) so that output is assigned directly from result.result (or the appropriate fallback string when no result attribute exists) and keep the existing scores = scores or {} assignment.holmes/main.py (1)
492-496: Consider usinglogging.exceptionfor exception logging.The static analysis tool suggests using
logging.exceptioninstead oflogging.errorwithexc_info=e. While functionally similar,logging.exceptionis more idiomatic when logging exceptions.♻️ Suggested change
- logging.error("Failed to fetch issues from alertmanager", exc_info=e) + logging.exception("Failed to fetch issues from alertmanager")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 492 - 496, The except block handling source.fetch_issues() currently uses logging.error with exc_info=e; change it to use logging.exception to be more idiomatic: in the try/except around source.fetch_issues() (where issues is assigned) replace logging.error("Failed to fetch issues from alertmanager", exc_info=e) with a logging.exception call that logs the same message so the current exception info is included automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/eval-regression.yaml:
- Around line 664-669: The workflow builds TEST_FILES by appending two potential
test paths into the TEST_FILES array then sets PYTEST_ARGS using
"${TEST_FILES[@]}", but it doesn't guard for an empty TEST_FILES so pytest may
be invoked with no paths; change the workflow to check the length of TEST_FILES
(e.g., test if ${`#TEST_FILES`[@]} -eq 0) and if empty, fail the job early with a
clear message (or exit 1) before constructing/using PYTEST_ARGS and running
pytest with EVAL_MARKER_EXPR; apply the same guard where TEST_FILES/PYTEST_ARGS
is repeated later in the file.
In `@holmes/main.py`:
- Around line 630-632: Wrap uses of result.result (an Optional[str]) with a
null-safe expression before calling .replace(), e.g., compute a safe string like
safe_text = result.result or "" (or a clear fallback like "<no result>") and
call safe_text.replace(...); update the three occurrences shown (the
console.print(Markdown(...)) call where Rule() and f"AI analysis of {issue.url}"
are printed) and apply the same defensive fix to the other similar sites (the
occurrences referenced at lines 745, 825, 905, and 982) so Markdown(...) never
receives None and .replace() cannot raise AttributeError.
---
Outside diff comments:
In `@tests/llm/utils/braintrust.py`:
- Around line 303-313: The branch using "elif test_case.conversation_history"
will raise AttributeError for HolmesTestCase instances that don't have
conversation_history; change the guard to use hasattr(test_case,
"conversation_history") and ensure it also checks truthiness (e.g., if
hasattr(test_case, "conversation_history") and test_case.conversation_history:)
so only AskHolmesTestCase-like objects enter that block; update the condition in
the function where the block containing format_conversation_as_markdown and
expected assignment runs to use this hasattr-based check.
---
Nitpick comments:
In `@holmes/main.py`:
- Around line 492-496: The except block handling source.fetch_issues() currently
uses logging.error with exc_info=e; change it to use logging.exception to be
more idiomatic: in the try/except around source.fetch_issues() (where issues is
assigned) replace logging.error("Failed to fetch issues from alertmanager",
exc_info=e) with a logging.exception call that logs the same message so the
current exception info is included automatically.
In `@tests/llm/utils/braintrust.py`:
- Around line 209-219: The conditional that checks hasattr(result, "result") is
followed by redundant ternary checks of result (e.g., output = result.result if
result else ...); simplify by removing the needless `if result` branches and
directly use result.result when hasattr(result, "result") is true, updating both
the error and non-error branches (variables: error, result, scores, and output)
so that output is assigned directly from result.result (or the appropriate
fallback string when no result attribute exists) and keep the existing scores =
scores or {} assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 014700cc-7619-44e4-a4ef-bdf9e3a98dab
📒 Files selected for processing (81)
.github/workflows/eval-regression.yamldocs/reference/http-api.mddocs/reference/python-sdk.mdholmes/config.pyholmes/core/conversations.pyholmes/core/investigation.pyholmes/core/investigation_structured_output.pyholmes/core/models.pyholmes/core/tool_calling_llm.pyholmes/main.pyholmes/plugins/prompts/_general_instructions.jinja2holmes/plugins/prompts/_noflag_general_instructions.jinja2holmes/plugins/prompts/generic_ask_for_issue_conversation.jinja2holmes/plugins/prompts/generic_investigation.jinja2holmes/plugins/prompts/investigation_output_format.jinja2holmes/plugins/prompts/investigation_procedure.jinja2holmes/utils/stream.pyserver.pytests/core/test_prompt.pytests/llm/fixtures/test_investigate/01_oom_kill/fast_oom_deployment.yamltests/llm/fixtures/test_investigate/01_oom_kill/investigate_request.jsontests/llm/fixtures/test_investigate/01_oom_kill/issue_data.jsontests/llm/fixtures/test_investigate/01_oom_kill/resource_instructions.jsontests/llm/fixtures/test_investigate/01_oom_kill/test_case.yamltests/llm/fixtures/test_investigate/02_crashloop_backoff/investigate_request.jsontests/llm/fixtures/test_investigate/02_crashloop_backoff/issue_data.jsontests/llm/fixtures/test_investigate/02_crashloop_backoff/resource_instructions.jsontests/llm/fixtures/test_investigate/02_crashloop_backoff/test_case.yamltests/llm/fixtures/test_investigate/03_cpu_throttling/investigate_request.jsontests/llm/fixtures/test_investigate/03_cpu_throttling/issue_data.jsontests/llm/fixtures/test_investigate/03_cpu_throttling/manifest.yamltests/llm/fixtures/test_investigate/03_cpu_throttling/resource_instructions.jsontests/llm/fixtures/test_investigate/03_cpu_throttling/test_case.yamltests/llm/fixtures/test_investigate/03_cpu_throttling/toolsets.yamltests/llm/fixtures/test_investigate/05_crashpod/investigate_request.jsontests/llm/fixtures/test_investigate/05_crashpod/issue_data.jsontests/llm/fixtures/test_investigate/05_crashpod/resource_instructions.jsontests/llm/fixtures/test_investigate/05_crashpod/test_case.yamltests/llm/fixtures/test_investigate/06_job_failure/investigate_request.jsontests/llm/fixtures/test_investigate/06_job_failure/issue_data.jsontests/llm/fixtures/test_investigate/06_job_failure/resource_instructions.jsontests/llm/fixtures/test_investigate/06_job_failure/test_case.yamltests/llm/fixtures/test_investigate/07_job_syntax_error/investigate_request.jsontests/llm/fixtures/test_investigate/07_job_syntax_error/issue_data.jsontests/llm/fixtures/test_investigate/07_job_syntax_error/resource_instructions.jsontests/llm/fixtures/test_investigate/07_job_syntax_error/test_case.yamltests/llm/fixtures/test_investigate/09_high_latency/helm/Dockerfiletests/llm/fixtures/test_investigate/09_high_latency/helm/app.pytests/llm/fixtures/test_investigate/09_high_latency/helm/build.shtests/llm/fixtures/test_investigate/09_high_latency/helm/manifest.yamltests/llm/fixtures/test_investigate/09_high_latency/helm/requirements.txttests/llm/fixtures/test_investigate/09_high_latency/investigate_request.jsontests/llm/fixtures/test_investigate/09_high_latency/issue_data.jsontests/llm/fixtures/test_investigate/09_high_latency/resource_instructions.jsontests/llm/fixtures/test_investigate/09_high_latency/test_case.yamltests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/investigate_request.jsontests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/issue_data.jsontests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/resource_instructions.jsontests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/test_case.yamltests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/toolsets.yamltests/llm/fixtures/test_investigate/15_dns_resolution/investigate_request.jsontests/llm/fixtures/test_investigate/15_dns_resolution/issue_data.jsontests/llm/fixtures/test_investigate/15_dns_resolution/manifest.yamltests/llm/fixtures/test_investigate/15_dns_resolution/resource_instructions.jsontests/llm/fixtures/test_investigate/15_dns_resolution/test_case.yamltests/llm/fixtures/test_investigate/15_dns_resolution/toolsets.yamltests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/investigate_request.jsontests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/issue_data.jsontests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/manifest.yamltests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/resource_instructions.jsontests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/test_case.yamltests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/toolsets.yamltests/llm/test_investigate.pytests/llm/utils/braintrust.pytests/llm/utils/langfuse.pytests/llm/utils/property_manager.pytests/llm/utils/test_case_utils.pytests/test_ai_safety_prompt.pytests/test_investigate_structured_output.pytests/test_issue_investigator.pytests/test_server_endpoints.py
💤 Files with no reviewable changes (72)
- tests/llm/fixtures/test_investigate/05_crashpod/resource_instructions.json
- tests/llm/fixtures/test_investigate/06_job_failure/test_case.yaml
- holmes/plugins/prompts/generic_investigation.jinja2
- tests/llm/fixtures/test_investigate/01_oom_kill/test_case.yaml
- tests/llm/fixtures/test_investigate/02_crashloop_backoff/resource_instructions.json
- tests/llm/fixtures/test_investigate/15_dns_resolution/test_case.yaml
- holmes/utils/stream.py
- tests/llm/fixtures/test_investigate/07_job_syntax_error/test_case.yaml
- tests/llm/fixtures/test_investigate/15_dns_resolution/issue_data.json
- tests/llm/fixtures/test_investigate/06_job_failure/issue_data.json
- holmes/plugins/prompts/investigation_procedure.jinja2
- holmes/plugins/prompts/_general_instructions.jinja2
- tests/llm/fixtures/test_investigate/15_dns_resolution/toolsets.yaml
- tests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/issue_data.json
- tests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/investigate_request.json
- tests/llm/fixtures/test_investigate/05_crashpod/issue_data.json
- tests/llm/fixtures/test_investigate/15_dns_resolution/manifest.yaml
- tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/test_case.yaml
- holmes/core/tool_calling_llm.py
- tests/llm/fixtures/test_investigate/09_high_latency/helm/manifest.yaml
- tests/llm/fixtures/test_investigate/05_crashpod/investigate_request.json
- tests/llm/fixtures/test_investigate/09_high_latency/helm/app.py
- holmes/core/investigation.py
- tests/llm/fixtures/test_investigate/03_cpu_throttling/test_case.yaml
- tests/llm/fixtures/test_investigate/09_high_latency/helm/requirements.txt
- tests/llm/fixtures/test_investigate/02_crashloop_backoff/test_case.yaml
- tests/llm/fixtures/test_investigate/01_oom_kill/investigate_request.json
- holmes/plugins/prompts/generic_ask_for_issue_conversation.jinja2
- tests/test_ai_safety_prompt.py
- tests/llm/fixtures/test_investigate/05_crashpod/test_case.yaml
- tests/llm/fixtures/test_investigate/01_oom_kill/resource_instructions.json
- tests/llm/fixtures/test_investigate/09_high_latency/test_case.yaml
- tests/llm/fixtures/test_investigate/06_job_failure/resource_instructions.json
- tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/resource_instructions.json
- tests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/toolsets.yaml
- tests/llm/fixtures/test_investigate/07_job_syntax_error/issue_data.json
- tests/llm/fixtures/test_investigate/03_cpu_throttling/manifest.yaml
- tests/llm/fixtures/test_investigate/03_cpu_throttling/toolsets.yaml
- tests/test_investigate_structured_output.py
- tests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/test_case.yaml
- tests/llm/fixtures/test_investigate/09_high_latency/investigate_request.json
- tests/llm/fixtures/test_investigate/07_job_syntax_error/investigate_request.json
- tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/issue_data.json
- holmes/plugins/prompts/investigation_output_format.jinja2
- tests/llm/fixtures/test_investigate/02_crashloop_backoff/issue_data.json
- tests/core/test_prompt.py
- tests/llm/fixtures/test_investigate/09_high_latency/issue_data.json
- tests/llm/fixtures/test_investigate/15_dns_resolution/resource_instructions.json
- tests/llm/fixtures/test_investigate/03_cpu_throttling/investigate_request.json
- tests/llm/fixtures/test_investigate/07_job_syntax_error/resource_instructions.json
- tests/test_server_endpoints.py
- tests/llm/fixtures/test_investigate/01_oom_kill/issue_data.json
- tests/llm/fixtures/test_investigate/09_high_latency/resource_instructions.json
- tests/llm/fixtures/test_investigate/15_dns_resolution/investigate_request.json
- tests/llm/fixtures/test_investigate/09_high_latency/helm/Dockerfile
- holmes/core/conversations.py
- tests/llm/fixtures/test_investigate/01_oom_kill/fast_oom_deployment.yaml
- tests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/resource_instructions.json
- tests/llm/fixtures/test_investigate/02_crashloop_backoff/investigate_request.json
- tests/llm/fixtures/test_investigate/06_job_failure/investigate_request.json
- tests/llm/fixtures/test_investigate/09_high_latency/helm/build.sh
- tests/test_issue_investigator.py
- tests/llm/fixtures/test_investigate/03_cpu_throttling/issue_data.json
- tests/llm/fixtures/test_investigate/03_cpu_throttling/resource_instructions.json
- tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/toolsets.yaml
- tests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/manifest.yaml
- holmes/core/models.py
- tests/llm/utils/test_case_utils.py
- tests/llm/utils/langfuse.py
- tests/llm/test_investigate.py
- tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/investigate_request.json
- holmes/core/investigation_structured_output.py
…moval - Makefile: remove broken test-llm-investigate target - conftest.py: remove test_investigate from LLM_TEST_TYPES, is_llm_test(), and test type detection logic - github_reporter.py: remove dead investigate_* counters and reporting - eval-regression.yaml: remove investigate fixtures from /list command - property_manager.py, braintrust.py: update stale comments https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/llm/utils/reporting/github_reporter.py (1)
174-188:⚠️ Potential issue | 🟡 MinorRemove dead code from conftest.py that detects "investigate" test types.
The PR removes investigation functionality from
github_reporter.py, butconftest.pystill contains code at lines 810–811 that checks fortest_investigatein nodeids and assignstest_type = "investigate". Since notest_investigatetest functions exist in the codebase, this is dead code that should be removed as part of the cleanup. Additionally, the summary counting logic (lines 174–188) only processestest_type == "ask"results, while the detailed table (lines 214–278) iterates over allsorted_results. While this inconsistency is currently masked by the absence of investigate tests, removing the dead code inconftest.pywill clarify the intended behavior: only "ask" and "unknown" test types can be produced, and "unknown" results should either be filtered or explicitly handled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/utils/reporting/github_reporter.py` around lines 174 - 188, Remove the dead "investigate" test_type detection in conftest (the code that looks for "test_investigate" in nodeids and sets result["test_type"] = "investigate"); instead ensure conftest only emits "ask" or "unknown" test_type values. Then update the summary loop that iterates sorted_results and constructs TestStatus (the block using TestStatus(result)) to either filter out result["test_type"] != "ask" before counting or explicitly handle the "unknown" type (e.g., increment an unknown counter or skip), so summary counters (ask_holmes_total, ask_holmes_passed, etc.) stay consistent with the detailed table output. Ensure changes reference the same result dict keys used by TestStatus and sorted_results so behavior remains consistent.tests/llm/utils/braintrust.py (1)
334-341:⚠️ Potential issue | 🟡 MinorFix the stale
get_braintrust_url()argument docs.The docstring still lists
test_suite,test_id, andtest_name, but the function only acceptsspan_idandroot_span_id. Keeping removed parameters in the args list makes the helper look callable in ways it no longer supports. As per coding guidelines, "Write clear, concise comments that explain 'why' rather than 'what'".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/utils/braintrust.py` around lines 334 - 341, The docstring for get_braintrust_url is stale: it still documents test_suite, test_id, and test_name even though the function signature only accepts span_id and root_span_id; update the docstring to accurately describe the current parameters (span_id and root_span_id), their types/format, and the return value (the generated Braintrust URL), or remove the obsolete Args section and replace it with a concise description of what get_braintrust_url does and the two valid parameters.
🧹 Nitpick comments (2)
tests/llm/conftest.py (2)
887-893: Prefer explaining theclean_test_case_idcontract over naming the current producer file.Hardcoding
test_ask_holmes.pyin the comment will go stale as soon as another LLM test sets the same property. A short “why” comment about keeping reporting stable across parameterized nodeids is enough here.As per coding guidelines "Write clear, concise comments that explain 'why' rather than 'what'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/conftest.py` around lines 887 - 893, Replace the hardcoded reference to "test_ask_holmes.py" with a concise explanation of the clean_test_case_id contract: state that tests set request.node.user_properties.append(("clean_test_case_id", test_case.id)) to provide a stable test-case identifier without model-parameter suffixes (so reporting remains stable across parameterized nodeids), and note the contract’s caveats (it won’t be present for skipped tests or tests that fail before user_properties are set); keep the comment short and focused on this “why” explanation rather than naming the producer file.
48-48: Make test classification a single typed source of truth.
LLM_TEST_TYPESwas updated here, butis_llm_test()still hardcodes the substring and_collect_test_results_from_stats()keeps its owntest_ask_holmes/test_investigatebranches. That leaves multiple classifiers to maintain in one file, so the next rename or new eval module will drift again. A typed mapping used by both helpers would make this change complete.♻️ Suggested direction
-LLM_TEST_TYPES = ["test_ask_holmes"] +LLM_TEST_TYPE_BY_NODEID: dict[str, str] = { + "test_ask_holmes": "ask", +} def is_llm_test(nodeid: str) -> bool: """Check if a test nodeid is for an LLM test.""" - return "test_ask_holmes" in nodeid + return any(name in nodeid for name in LLM_TEST_TYPE_BY_NODEID)Then reuse the same mapping when deriving
test_typein_collect_test_results_from_stats().As per coding guidelines "Type hints are required throughout the codebase (mypy configuration in pyproject.toml)".
Also applies to: 100-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/conftest.py` at line 48, LLM test classification is duplicated: LLM_TEST_TYPES, is_llm_test(), and _collect_test_results_from_stats() each hardcode test kind strings; consolidate into a single typed mapping/enum (replace LLM_TEST_TYPES with a Mapping[str, str] or TypedDict/Enum) and have both is_llm_test() and _collect_test_results_from_stats() reference that single source to derive test_type (update function signatures and internal logic to use the mapping/enum), and add type hints for the new mapping and for is_llm_test()/_collect_test_results_from_stats() return types per project typing rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/llm/utils/braintrust.py`:
- Around line 192-199: The docstring for log_to_braintrust() claims it only
handles AskHolmesTestCase/LLMResult but the function still contains explicit
compaction branches and is invoked by compaction tests; update the contract to
match behaviour by either (A) broadening the docstring to describe both
multi-test and compaction flows (mentioning it accepts compaction inputs and how
they are compacted) or (B) remove/encapsulate the compaction-specific branches
and calls so the function truly only accepts AskHolmesTestCase/LLMResult; locate
the branching logic inside log_to_braintrust and align callers/tests accordingly
(adjust compaction tests to the new helper if you choose B).
- Line 188: Change the local import and loosened typing for the helper's
`result` parameter: move the import(s) that bring in LLMResult and
CompactionResult from inside the function to module top-level imports, then
replace the annotation `Optional[Any]` on the `result` parameter with
`Optional[Union[LLMResult, CompactionResult]]` (and add `from typing import
Optional, Union` at module scope if not present). This keeps imports compliant
with the repo rule and restores precise typing for the helper that accepts
either an LLMResult or a CompactionResult.
---
Outside diff comments:
In `@tests/llm/utils/braintrust.py`:
- Around line 334-341: The docstring for get_braintrust_url is stale: it still
documents test_suite, test_id, and test_name even though the function signature
only accepts span_id and root_span_id; update the docstring to accurately
describe the current parameters (span_id and root_span_id), their types/format,
and the return value (the generated Braintrust URL), or remove the obsolete Args
section and replace it with a concise description of what get_braintrust_url
does and the two valid parameters.
In `@tests/llm/utils/reporting/github_reporter.py`:
- Around line 174-188: Remove the dead "investigate" test_type detection in
conftest (the code that looks for "test_investigate" in nodeids and sets
result["test_type"] = "investigate"); instead ensure conftest only emits "ask"
or "unknown" test_type values. Then update the summary loop that iterates
sorted_results and constructs TestStatus (the block using TestStatus(result)) to
either filter out result["test_type"] != "ask" before counting or explicitly
handle the "unknown" type (e.g., increment an unknown counter or skip), so
summary counters (ask_holmes_total, ask_holmes_passed, etc.) stay consistent
with the detailed table output. Ensure changes reference the same result dict
keys used by TestStatus and sorted_results so behavior remains consistent.
---
Nitpick comments:
In `@tests/llm/conftest.py`:
- Around line 887-893: Replace the hardcoded reference to "test_ask_holmes.py"
with a concise explanation of the clean_test_case_id contract: state that tests
set request.node.user_properties.append(("clean_test_case_id", test_case.id)) to
provide a stable test-case identifier without model-parameter suffixes (so
reporting remains stable across parameterized nodeids), and note the contract’s
caveats (it won’t be present for skipped tests or tests that fail before
user_properties are set); keep the comment short and focused on this “why”
explanation rather than naming the producer file.
- Line 48: LLM test classification is duplicated: LLM_TEST_TYPES, is_llm_test(),
and _collect_test_results_from_stats() each hardcode test kind strings;
consolidate into a single typed mapping/enum (replace LLM_TEST_TYPES with a
Mapping[str, str] or TypedDict/Enum) and have both is_llm_test() and
_collect_test_results_from_stats() reference that single source to derive
test_type (update function signatures and internal logic to use the
mapping/enum), and add type hints for the new mapping and for
is_llm_test()/_collect_test_results_from_stats() return types per project typing
rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d85c8249-16eb-43a0-a048-40ca2c88334e
📒 Files selected for processing (6)
.github/workflows/eval-regression.yamlMakefiletests/llm/conftest.pytests/llm/utils/braintrust.pytests/llm/utils/property_manager.pytests/llm/utils/reporting/github_reporter.py
💤 Files with no reviewable changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/eval-regression.yaml
- tests/llm/utils/property_manager.py
- Move LLMResult and AskHolmesTestCase imports to module top-level - Type result parameter as Optional[Union[LLMResult, CompactionResult]] - Update docstrings to reflect both ask_holmes and compaction flows - Fix get_braintrust_url docstring listing nonexistent params - Add guard for empty TEST_FILES array in eval workflow to fail early https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
…0.0.1:29615/git/HolmesGPT/holmesgpt into claude/investigate-endpoint-audit-S5ufh
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/llm/utils/braintrust.py (1)
344-351:⚠️ Potential issue | 🟡 MinorRemove parameters from the docstring that are no longer in the signature.
get_braintrust_url()only acceptsspan_idandroot_span_id, but the docstring still liststest_suite,test_id, andtest_name. That leaves the public contract stale.As per coding guidelines, "Write clear, concise comments that explain 'why' rather than 'what'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/utils/braintrust.py` around lines 344 - 351, The docstring for get_braintrust_url is stale—remove the outdated parameters test_suite, test_id, and test_name from the docstring and update it to match the current signature (only span_id and root_span_id); rewrite the docstring to briefly describe the function's purpose and include parameter entries only for span_id and root_span_id (with their optional status) and the return value so the public contract is accurate and concise.
♻️ Duplicate comments (7)
holmes/main.py (5)
807-810:⚠️ Potential issue | 🟡 MinorApply null check for
result.resulthere as well (GitHub command).Same issue -
result.resultcould beNone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 807 - 810, The code prints Markdown(result.result...) without checking for None; update the block that calls console.print(Markdown(...)) to guard against result.result being None (same pattern as other checks) by using a conditional or coalesce to a safe string (e.g., "" or "No analysis available") before calling Markdown, referencing result.result, Markdown(...), console.print(...) and issue.url so the printed analysis never receives None.
725-730:⚠️ Potential issue | 🟡 MinorApply null check for
result.resulthere as well.Same issue as flagged above -
result.resultcould beNone.🛡️ Defensive fix
console.print(Rule()) console.print( f"[bold green]AI analysis of {issue_to_investigate.url} {prompt}[/bold green]" ) - console.print(result.result.replace("\n", "\n\n"), style="bold green") # type: ignore + analysis_text = result.result or "No analysis available" + console.print(analysis_text.replace("\n", "\n\n"), style="bold green") console.print(Rule())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 725 - 730, The print block uses result.result without a null check; update the block around console.print(result.result.replace(...)) to first test if result.result is not None (e.g., if result.result: or if result.result is not None:) and only call .replace when non-null, otherwise print a fallback message or skip that print; reference the variable result and the console.print calls in the same block so you preserve the surrounding Rule() prints.
887-890:⚠️ Potential issue | 🟡 MinorApply null check for
result.resulthere as well (PagerDuty command).Same issue -
result.resultcould beNone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 887 - 890, The print block assumes result.result is a string but it can be None; update the PagerDuty command output logic (the block that prints Rule(), console.print(f"AI analysis of {issue.url}"), and console.print(Markdown(result.result.replace("\n", "\n\n")), ... )) to null-check result.result first and use a safe fallback (e.g., skip the Markdown print, show a placeholder message like "No analysis available", or coalesce to an empty string) so you never call .replace on None; locate the variable named result in main.py and guard its .result attribute before passing it to Markdown or calling .replace.
614-616:⚠️ Potential issue | 🟡 MinorAdd null check for
result.resultbefore calling.replace().
LLMResult.resultis typed asOptional[str], so calling.replace()directly could raiseAttributeErrorif the LLM fails to produce a result.🛡️ Defensive fix
console.print(Rule()) console.print(f"[bold green]AI analysis of {issue.url}[/bold green]") - console.print(Markdown(result.result.replace("\n", "\n\n")), style="bold green") # type: ignore + analysis_text = result.result or "No analysis available" + console.print(Markdown(analysis_text.replace("\n", "\n\n")), style="bold green") console.print(Rule())The same pattern appears at lines 729, 809, 889, and 966 - apply a similar fix to those locations as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 614 - 616, Wrap uses of result.result before calling .replace() with a null-safe check and a fallback string: check if result.result is truthy (or use "result_text = result.result or '<no result>'") then call result_text.replace(...). Update the three occurrences in the same function where console.print(Markdown(result.result.replace(...))) is used (the calls printing AI analysis via console.print and Markdown) and replicate this null-safe pattern for the other locations that use LLMResult.result at the same file (the instances around the existing console.print lines and the similar usages at the other noted spots).
964-967:⚠️ Potential issue | 🟡 MinorApply null check for
result.resulthere as well (OpsGenie command).Same issue -
result.resultcould beNone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 964 - 967, The code prints Markdown(Markdown(result.result.replace("\n", "\n\n"))) without guarding against result.result being None; update the block that uses result (the variable named result in holmes/main.py) to null-check result.result before calling .replace and passing to Markdown/console.print (e.g., use a safe fallback like an empty string or "No result available" when result.result is None) so Markdown() always receives a str and avoids AttributeError.tests/llm/utils/braintrust.py (2)
188-202: 🛠️ Refactor suggestion | 🟠 MajorMove the local import to module scope and give
resulta real type.
Optional[Any]turns off checking in a shared helper, and theAskHolmesTestCaseimport inside the function still violates the repo’s Python import rule. Please import the supported types at module scope and annotateresultprecisely instead of documenting it in a comment.As per coding guidelines, "Type hints required (mypy configuration in pyproject.toml)" and "ALWAYS place Python imports at the top of the file, not inside functions or methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/utils/braintrust.py` around lines 188 - 202, Move the local import "from tests.llm.utils.test_case_utils import AskHolmesTestCase" to the top of the module and add a precise type for the result parameter instead of Optional[Any]; import the concrete LLM result type used across tests (e.g., LLMResult) at module scope and annotate the function signature as result: Optional[LLMResult] (or the actual class name used in your codebase), keeping scores: Optional[dict] and error: Optional[Exception] as-is; update imports at module scope and adjust any references inside the logging helper accordingly.
192-199:⚠️ Potential issue | 🟡 MinorKeep the docstring aligned with the compaction branch below.
This docstring now says the helper only handles
AskHolmesTestCase/LLMResult, but the function still has explicitCompactionResulthandling later on. Either narrow the implementation too, or document both supported flows here.As per coding guidelines, "Write clear, concise comments that explain 'why' rather than 'what'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/utils/braintrust.py` around lines 192 - 199, The docstring for the shared Braintrust logging helper is out of sync with the implementation: update the docstring of the function (the helper that takes eval_span) to document both supported flows or narrow the code to match the docstring; specifically either add CompactionResult (CompactionResult) to the documented accepted types along with AskHolmesTestCase and LLMResult, or remove the explicit CompactionResult handling in the function body so it only handles AskHolmesTestCase/LLMResult—adjust the narrative to explain why both flows are supported and what each path does, and reference the symbols AskHolmesTestCase, LLMResult, CompactionResult and the helper function name so reviewers can locate the changes.
🧹 Nitpick comments (3)
server.py (1)
59-61: Remove the stale historical comment.
# removed: add_runbooks_to_user_promptno longer explains anything about the current code path and will just drift over time. Dropping it keeps this import section focused on the behavior that still exists. As per coding guidelines, "Write clear, concise comments that explain 'why' rather than 'what'".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 59 - 61, Remove the stale inline comment "# removed: add_runbooks_to_user_prompt" near the import of stream_chat_formatter in server.py; locate the import statement "from holmes.utils.stream import stream_chat_formatter" and delete only the historical comment line so the import section contains current, purposeful comments only.holmes/main.py (2)
476-480: Uselogging.exceptioninstead oflogging.errorwithexc_info.When logging exceptions in an except block,
logging.exceptionis the idiomatic choice as it automatically includes exception info.♻️ Suggested fix
try: issues = source.fetch_issues() except Exception as e: - logging.error("Failed to fetch issues from alertmanager", exc_info=e) + logging.exception("Failed to fetch issues from alertmanager") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 476 - 480, Replace the logging call in the except block that handles errors from source.fetch_issues(): instead of calling logging.error(..., exc_info=e), use logging.exception("Failed to fetch issues from alertmanager") so the exception traceback is included idiomatically; locate the try/except around source.fetch_issues() in holmes/main.py and swap the logging.error call for logging.exception within that except block.
614-627: Consider extracting repeated result display logic into a helper.The pattern of printing rules, AI analysis header, markdown result, and update handling is repeated across jira, github, pagerduty, and opsgenie commands. This could be consolidated into a helper function similar to how
_investigate_issue()was extracted.This is a nice-to-have for future maintainability and not blocking for this PR.
Also applies to: 807-817, 887-897, 964-974
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 614 - 627, The block that prints rules, header, Markdown result, handles update/write_back_result and appends to results is duplicated; extract it into a helper (e.g., _display_issue_result or display_result) that accepts parameters (issue, result, update, source, console, results) and performs console.print(Rule()), header, Markdown rendering, optional source.write_back_result(issue.id, result) with console messages, and results.append({"issue": issue.model_dump(), "result": result.model_dump()}); then replace the repeated blocks in jira/github/pagerduty/opsgenie command handlers (the repeated printing/update code shown around the Rule/AI analysis/Markdown/update logic and the other mentioned ranges) with calls to that helper so behavior and side effects remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/llm/utils/braintrust.py`:
- Around line 344-351: The docstring for get_braintrust_url is stale—remove the
outdated parameters test_suite, test_id, and test_name from the docstring and
update it to match the current signature (only span_id and root_span_id);
rewrite the docstring to briefly describe the function's purpose and include
parameter entries only for span_id and root_span_id (with their optional status)
and the return value so the public contract is accurate and concise.
---
Duplicate comments:
In `@holmes/main.py`:
- Around line 807-810: The code prints Markdown(result.result...) without
checking for None; update the block that calls console.print(Markdown(...)) to
guard against result.result being None (same pattern as other checks) by using a
conditional or coalesce to a safe string (e.g., "" or "No analysis available")
before calling Markdown, referencing result.result, Markdown(...),
console.print(...) and issue.url so the printed analysis never receives None.
- Around line 725-730: The print block uses result.result without a null check;
update the block around console.print(result.result.replace(...)) to first test
if result.result is not None (e.g., if result.result: or if result.result is not
None:) and only call .replace when non-null, otherwise print a fallback message
or skip that print; reference the variable result and the console.print calls in
the same block so you preserve the surrounding Rule() prints.
- Around line 887-890: The print block assumes result.result is a string but it
can be None; update the PagerDuty command output logic (the block that prints
Rule(), console.print(f"AI analysis of {issue.url}"), and
console.print(Markdown(result.result.replace("\n", "\n\n")), ... )) to
null-check result.result first and use a safe fallback (e.g., skip the Markdown
print, show a placeholder message like "No analysis available", or coalesce to
an empty string) so you never call .replace on None; locate the variable named
result in main.py and guard its .result attribute before passing it to Markdown
or calling .replace.
- Around line 614-616: Wrap uses of result.result before calling .replace() with
a null-safe check and a fallback string: check if result.result is truthy (or
use "result_text = result.result or '<no result>'") then call
result_text.replace(...). Update the three occurrences in the same function
where console.print(Markdown(result.result.replace(...))) is used (the calls
printing AI analysis via console.print and Markdown) and replicate this
null-safe pattern for the other locations that use LLMResult.result at the same
file (the instances around the existing console.print lines and the similar
usages at the other noted spots).
- Around line 964-967: The code prints
Markdown(Markdown(result.result.replace("\n", "\n\n"))) without guarding against
result.result being None; update the block that uses result (the variable named
result in holmes/main.py) to null-check result.result before calling .replace
and passing to Markdown/console.print (e.g., use a safe fallback like an empty
string or "No result available" when result.result is None) so Markdown() always
receives a str and avoids AttributeError.
In `@tests/llm/utils/braintrust.py`:
- Around line 188-202: Move the local import "from
tests.llm.utils.test_case_utils import AskHolmesTestCase" to the top of the
module and add a precise type for the result parameter instead of Optional[Any];
import the concrete LLM result type used across tests (e.g., LLMResult) at
module scope and annotate the function signature as result: Optional[LLMResult]
(or the actual class name used in your codebase), keeping scores: Optional[dict]
and error: Optional[Exception] as-is; update imports at module scope and adjust
any references inside the logging helper accordingly.
- Around line 192-199: The docstring for the shared Braintrust logging helper is
out of sync with the implementation: update the docstring of the function (the
helper that takes eval_span) to document both supported flows or narrow the code
to match the docstring; specifically either add CompactionResult
(CompactionResult) to the documented accepted types along with AskHolmesTestCase
and LLMResult, or remove the explicit CompactionResult handling in the function
body so it only handles AskHolmesTestCase/LLMResult—adjust the narrative to
explain why both flows are supported and what each path does, and reference the
symbols AskHolmesTestCase, LLMResult, CompactionResult and the helper function
name so reviewers can locate the changes.
---
Nitpick comments:
In `@holmes/main.py`:
- Around line 476-480: Replace the logging call in the except block that handles
errors from source.fetch_issues(): instead of calling logging.error(...,
exc_info=e), use logging.exception("Failed to fetch issues from alertmanager")
so the exception traceback is included idiomatically; locate the try/except
around source.fetch_issues() in holmes/main.py and swap the logging.error call
for logging.exception within that except block.
- Around line 614-627: The block that prints rules, header, Markdown result,
handles update/write_back_result and appends to results is duplicated; extract
it into a helper (e.g., _display_issue_result or display_result) that accepts
parameters (issue, result, update, source, console, results) and performs
console.print(Rule()), header, Markdown rendering, optional
source.write_back_result(issue.id, result) with console messages, and
results.append({"issue": issue.model_dump(), "result": result.model_dump()});
then replace the repeated blocks in jira/github/pagerduty/opsgenie command
handlers (the repeated printing/update code shown around the Rule/AI
analysis/Markdown/update logic and the other mentioned ranges) with calls to
that helper so behavior and side effects remain identical.
In `@server.py`:
- Around line 59-61: Remove the stale inline comment "# removed:
add_runbooks_to_user_prompt" near the import of stream_chat_formatter in
server.py; locate the import statement "from holmes.utils.stream import
stream_chat_formatter" and delete only the historical comment line so the import
section contains current, purposeful comments only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b37b720-1be8-4bf3-9eca-b6f6d35444ec
📒 Files selected for processing (8)
holmes/config.pyholmes/core/tool_calling_llm.pyholmes/main.pyserver.pytests/llm/conftest.pytests/llm/utils/braintrust.pytests/llm/utils/property_manager.pytests/llm/utils/reporting/github_reporter.py
💤 Files with no reviewable changes (1)
- holmes/core/tool_calling_llm.py
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/config.py
- tests/llm/conftest.py
- tests/llm/utils/property_manager.py
The investigation_procedure.jinja2 template was accidentally deleted in the investigation endpoint removal, but it is used by ALL CLI commands (via generic_investigation.jinja2 -> _noflag_general_instructions.jinja2) and by the server/chat path (via _general_instructions.jinja2 when todowrite_enabled). This restores the template and its includes. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/eval-regression.yaml (1)
858-867: Consider extracting the repeated test file collection logic.This block duplicates lines 661-670. While acceptable in workflow files, you could extract this into a shared shell script (e.g.,
.github/scripts/collect-test-files.sh) to reduce maintenance burden if the file list changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/eval-regression.yaml around lines 858 - 867, Extract the duplicated test-file collection loop (the code that builds TEST_FILES and sets PYTEST_ARGS) into a shared shell script named collect-test-files.sh and replace both inline copies with a simple source/execute of that script; ensure the script exports or prints the TEST_FILES array (or writes it to stdout for command substitution) so the calling workflow can set TEST_FILES and then define PYTEST_ARGS=(--no-cov "${TEST_FILES[@]}" -s -n10 -m "$EVAL_MARKER_EXPR"), keeping the same variable names (TEST_FILES, PYTEST_ARGS) so existing references like the for loop and the PYTEST_ARGS assignment continue to work unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/llm/utils/braintrust.py`:
- Around line 212-220: The branch handling LLMResult may set output to None and
drop the exception text; change both branches to explicitly guard result.result
for None: when error is truthy set output = result.result if (result and
getattr(result, "result", None) is not None) else str(error), ensure scores =
scores or {}, and in the non-error branch set output = result.result if (result
and getattr(result, "result", None) is not None) else "". Update references
around the variables error, result, LLMResult.result, output, and scores to use
getattr(result, "result", None) checks rather than truthiness of result.result.
---
Nitpick comments:
In @.github/workflows/eval-regression.yaml:
- Around line 858-867: Extract the duplicated test-file collection loop (the
code that builds TEST_FILES and sets PYTEST_ARGS) into a shared shell script
named collect-test-files.sh and replace both inline copies with a simple
source/execute of that script; ensure the script exports or prints the
TEST_FILES array (or writes it to stdout for command substitution) so the
calling workflow can set TEST_FILES and then define PYTEST_ARGS=(--no-cov
"${TEST_FILES[@]}" -s -n10 -m "$EVAL_MARKER_EXPR"), keeping the same variable
names (TEST_FILES, PYTEST_ARGS) so existing references like the for loop and the
PYTEST_ARGS assignment continue to work unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3aa3509-52a6-40ab-8586-5f20f0adc60e
📒 Files selected for processing (2)
.github/workflows/eval-regression.yamltests/llm/utils/braintrust.py
aantn
enabled auto-merge (squash)
March 7, 2026 20:32
Investigation commands (alertmanager, jira, github, pagerduty, opsgenie) now use the same build_system_prompt() path as the ask command, with investigation-specific context passed via system_prompt_additions. This removes the separate generic_investigation.jinja2 template and the --system-prompt CLI option from all investigate subcommands. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
No test cases use expected_sections (it was part of the old investigation system). Remove the evaluate_sections function, its call site in property_manager, and the docs reference. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
…ions These timestamp variables (start_timestamp, end_timestamp, etc.) were never populated - the Issue model has no such fields. They were leftover from the old investigation template. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
….jinja2 Just a single-line f-string instead of a dedicated template file. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
holmes/main.py (1)
609-611:⚠️ Potential issue | 🟠 MajorGuard optional LLM output before rendering.
Line 611, Line 724, Line 800, Line 876, and Line 949 still call
.replace()onresult.result.LLMResult.resultis optional, so a failed/empty model response will raiseAttributeErrorand abort the command after the investigation already ran.🛡️ Representative fix
- console.print(Markdown(result.result.replace("\n", "\n\n")), style="bold green") # type: ignore + analysis_text = result.result or "No analysis available" + console.print(Markdown(analysis_text.replace("\n", "\n\n")), style="bold green")Apply the same fallback to the plain
console.print(...)site inticket.Also applies to: 720-725, 798-800, 874-876, 947-949
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 609 - 611, result.result is optional and the code calls result.result.replace(...) directly which can raise AttributeError when the LLM returned None; update each console.print that renders the LLM output (the calls that use console.print(Markdown(result.result.replace(...)))—and the similar sites in the ticket rendering) to guard the value first by coalescing result.result to a safe fallback (e.g., use (result.result or "") or a message like "No response from model") before calling .replace(), then pass that sanitized string into Markdown and console.print so rendering never dereferences None.
🧹 Nitpick comments (2)
holmes/plugins/prompts/_investigation_additions.jinja2 (1)
7-7: This instruction does not match the current caller contract.
_investigate_issue()only sends issue context, so the model never receives a dedicated triple-quoted instruction block in this flow. Either wire those instructions through the caller or drop this stale directive from the prompt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/prompts/_investigation_additions.jinja2` at line 7, The prompt contains a stale directive telling the model to obey triple-quoted extra instructions, but _investigate_issue() never provides such a block; either remove that line from the _investigation_additions.jinja2 template or update the caller that invokes _investigate_issue() so it forwards any triple-quoted instruction payload into the investigation context; locate the template entry that says "If the user provides you with extra instructions in a triple quotes section..." and either delete it or replace it with a note that callers must pass extra instructions via the _investigate_issue() payload, and if you choose wiring, modify the caller to include that payload under the investigation context key so _investigate_issue() receives it.holmes/main.py (1)
475-479: Uselogging.exceptioninside this handler.Ruff flags
logging.error(..., exc_info=e)here;logging.exception(...)is the cleaner form in anexceptblock and preserves the traceback without passing the exception object manually.♻️ Suggested cleanup
- except Exception as e: - logging.error("Failed to fetch issues from alertmanager", exc_info=e) + except Exception: + logging.exception("Failed to fetch issues from alertmanager") returnAs per coding guidelines, "Use Ruff for formatting and linting (configured in pyproject.toml)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/main.py` around lines 475 - 479, Replace the logging call inside the except block that currently uses logging.error(..., exc_info=e) with logging.exception to properly log the traceback in the except handler; locate the try/except around source.fetch_issues() (the variable issues and the call source.fetch_issues()) and change the except block to call logging.exception("Failed to fetch issues from alertmanager") and then return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/main.py`:
- Around line 727-728: The current call to
ticket_source.source.write_back_result(issue_to_investigate.id, result) performs
a destructive write on every successful run; change this so writes are opt-in:
add a CLI flag (e.g., --update or --write-back) or an explicit confirmation
check used by the investigate flow, defaulting to read-only, and only call
ticket_source.source.write_back_result when that flag/confirmation is present;
locate and guard the write in the function where issue_to_investigate is
processed (the block that currently calls console.print and write_back_result)
so the behavior remains idempotent and non-mutating by default.
In `@holmes/plugins/prompts/_investigation_additions.jinja2`:
- Around line 2-4: The template _investigation_additions.jinja2 currently
instructs using literal placeholders ('start_timestamp', 'end_timestamp',
'start_timestamp_millis', 'end_timestamp_millis') and also swaps the
millis/string guidance; update it to either remove the inline time-window
guidance or make it accurate by (a) correcting the mapping so "string format
timestamps" reference start_timestamp/start_timestamp_end and "millisecond
timestamps" reference start_timestamp_millis/end_timestamp_millis, and (b)
ensuring _investigate_issue() supplies the actual timestamp values in the render
context (e.g., add timestamp keys to the context) if you keep the guidance —
otherwise delete these lines so the template doesn't mislead callers.
---
Duplicate comments:
In `@holmes/main.py`:
- Around line 609-611: result.result is optional and the code calls
result.result.replace(...) directly which can raise AttributeError when the LLM
returned None; update each console.print that renders the LLM output (the calls
that use console.print(Markdown(result.result.replace(...)))—and the similar
sites in the ticket rendering) to guard the value first by coalescing
result.result to a safe fallback (e.g., use (result.result or "") or a message
like "No response from model") before calling .replace(), then pass that
sanitized string into Markdown and console.print so rendering never dereferences
None.
---
Nitpick comments:
In `@holmes/main.py`:
- Around line 475-479: Replace the logging call inside the except block that
currently uses logging.error(..., exc_info=e) with logging.exception to properly
log the traceback in the except handler; locate the try/except around
source.fetch_issues() (the variable issues and the call source.fetch_issues())
and change the except block to call logging.exception("Failed to fetch issues
from alertmanager") and then return.
In `@holmes/plugins/prompts/_investigation_additions.jinja2`:
- Line 7: The prompt contains a stale directive telling the model to obey
triple-quoted extra instructions, but _investigate_issue() never provides such a
block; either remove that line from the _investigation_additions.jinja2 template
or update the caller that invokes _investigate_issue() so it forwards any
triple-quoted instruction payload into the investigation context; locate the
template entry that says "If the user provides you with extra instructions in a
triple quotes section..." and either delete it or replace it with a note that
callers must pass extra instructions via the _investigate_issue() payload, and
if you choose wiring, modify the caller to include that payload under the
investigation context key so _investigate_issue() receives it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: adec9ad4-d35a-475a-8f9f-dbcc35b1dc34
📒 Files selected for processing (4)
holmes/main.pyholmes/plugins/prompts/_investigation_additions.jinja2holmes/plugins/prompts/generic_investigation.jinja2tests/test_ai_safety_prompt.py
💤 Files with no reviewable changes (2)
- tests/test_ai_safety_prompt.py
- holmes/plugins/prompts/generic_investigation.jinja2
The ticket command was unconditionally writing results back to the source system. All other investigate commands (jira, github, pagerduty, opsgenie) already have --update flags. Add the same pattern here, defaulting to read-only. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
Only included by the now-deleted generic_investigation.jinja2. No remaining references in the codebase. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
arikalon1
previously approved these changes
Mar 8, 2026
Keep dynamic test file collection from our branch (test_investigate.py was removed as part of investigation endpoint removal). Master added test_investigate.py to the static list, which no longer exists. https://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP Signed-off-by: Claude <noreply@anthropic.com>
arikalon1
approved these changes
Mar 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
This PR removes the automated investigation API endpoint (
/api/investigate) and associated infrastructure, consolidating all issue analysis functionality into the conversational chat interface. The changes simplify the codebase by eliminating duplicate investigation logic and structured output handling that was specific to the investigation workflow.Key Changes
/api/investigateendpoint andInvestigateRequest/InvestigationResultmodels from the HTTP APIholmes/core/investigation.pywhich contained theinvestigate_issues()functionholmes/core/investigation_structured_output.pyand all related structured output formatting logic (sections, JSON schema generation, markdown parsing)investigation_procedure.jinja2(task management and parallel execution rules)investigation_output_format.jinja2(structured output formatting)generic_ask_for_issue_conversation.jinja2(issue conversation system prompt)tests/llm/test_investigate.py(main investigation tests)tests/test_investigate_structured_output.py(structured output tests)tests/test_issue_investigator.py(issue investigator tests)holmes/main.pyto use the new_investigate_issue()helper function that callsToolCallingLLM.prompt_call()directly instead of the removedIssueInvestigator.investigate()methodbuild_issue_chat_messages()and related issue-specific conversation logic fromholmes/core/conversations.pyToolCallingLLMclassIssueInvestigatortype hint and related factory methods/api/investigateendpoint documentation from HTTP API referenceImplementation Details
_investigate_issue()helper inholmes/main.pyprovides a lightweight wrapper aroundToolCallingLLM.prompt_call()for the alertmanager command, replacing the removed investigation infrastructure/api/chatendpoint, which provides the same capabilities with a more unified interfacetodowrite_enabledconditional logic from prompt templates, simplifying the instruction hierarchyhttps://claude.ai/code/session_01GgDjg6RjYXmfDKSp2TCcxP
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Tests