Conversation
|
✅ 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:b6c5b5a7
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:b6c5b5a7 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:b6c5b5a7
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:b6c5b5a7
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:b6c5b5a7
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:b6c5b5a7 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:b6c5b5a7
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:b6c5b5a7Patch 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:b6c5b5a7 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:b6c5b5a7Robusta 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:b6c5b5a7 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:b6c5b5a7 |
📂 Previous Runs📜 Run @ 093bcee (#22844785556)✅ Results of HolmesGPT evalsAutomatically triggered by commit 093bcee 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:
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Output | Cached | Non-cached | Reasoning | Max output | Compactions |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ❌ | 09_crashpod | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 101_loki_historical_logs_pod_deleted | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 111_pod_names_contain_service | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 112_find_pvcs_by_uuid | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 12_job_crashing | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 176_network_policy_blocking_traffic_no_runbooks | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 227_count_configmaps_per_namespace[0] | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 24_misconfigured_pvc | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 43_current_datetime_from_prompt | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 61_exact_match_counting | 0.0s | — | — | — | — | — | — | — | — | — | — | — |
| Total | 0.0s avg | — avg | — avg | — | — | — | — | — | — | — | — | — |
Benchmark comparison unavailable: No ci-benchmark experiments found
Benchmark Comparison Details
Baseline: latest ci-benchmark experiment on master
Status: No ci-benchmark experiments found
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
⚠️ 10 Failures Detected
⚠️ Eval Results (with failures)
Automatically triggered by commit a5e0d29 on branch async-call-llm
Results of HolmesGPT evals
- ask_holmes: 0/10 test cases were successful, 10 regressions
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Compactions |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ❌ | 09_crashpod | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 101_loki_historical_logs_pod_deleted | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 111_pod_names_contain_service | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 112_find_pvcs_by_uuid | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 12_job_crashing | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 176_network_policy_blocking_traffic_no_runbooks | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 227_count_configmaps_per_namespace[0] | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 24_misconfigured_pvc | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 43_current_datetime_from_prompt | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 61_exact_match_counting | — | — | — | — | — | — | — | — | — | — | — | — | — |
| Total | — avg | — avg | — avg | — | — | — | — | — | — | — | — | — | — |
Benchmark Comparison Details
Baseline: latest ci-benchmark experiment on master
Status: Success - 73 test/model combinations loaded
Benchmark experiment:
- ci-benchmark-23102181491 (created: 2026-03-15)
No benchmark data available for comparison.
Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run.
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
⚠️ 10 Failures Detected
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref async-call-llm -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
id: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
id |
Eval ID / pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals (applies to both automatic runs and /eval comments):
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
evals-model-<name> |
Override the model (use model list name, e.g. sonnet-4.5) |
Examples: evals-tag-easy, evals-id-09_crashpod, evals-model-sonnet-4.5
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fast, frontend, grafana, hard, integration, kafka, kubernetes, leaked-information, logs, loki, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, runbooks, slackbot, storage, toolset-limitation, traces, transparency
🤖 Valid models
deepseek-chat, deepseek-r1-reasoner, deepseek-reasoner, deepseek-v3.2-chat, gemini-3-flash-preview, gemini-3-pro-preview, gemini-3.1-pro-preview, gpt-4.1, gpt-5.2-high-reasoning, gpt-5.3-codex, gpt-5.4, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, qwen-next-80B-instruct, qwen-next-80B-thinking, sonnet-4.5, sonnet-4.6
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref async-call-llm -f markers=regression -f filter=
WalkthroughThis PR introduces asynchronous execution patterns throughout the core LLM and tool-calling infrastructure, converts the FastAPI server chat endpoint to async with streaming support, adds configuration and utility helpers, and applies extensive formatting improvements across the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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. 📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
There was a problem hiding this comment.
Pull request overview
This PR migrates the Holmes LLM integration from synchronous LiteLLM calls to async (litellm.acompletion) to improve concurrency in server deployments, while keeping the CLI usable by wrapping async calls with asyncio.run().
Changes:
- Converted core LLM completion + tool-calling flows to async/await.
- Updated server
/api/chathandler to async and updated non-streaming call path to await async LLM calls. - Made conversation compaction and input context window limiting async to match the new async LLM API.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
server.py |
Makes /api/chat async and awaits async message calls. |
holmes/main.py |
Wraps async LLM calls in CLI commands via asyncio.run(). |
holmes/interactive.py |
Adapts interactive execution to call async ai.call() via asyncio.run(). |
holmes/core/truncation/input_context_window_limiter.py |
Converts context window limiting to async and awaits compaction. |
holmes/core/truncation/compaction.py |
Converts compaction to async and awaits llm.completion(). |
holmes/core/transformers/llm_summarize.py |
Calls async completion from the summarizer via asyncio.run(). |
holmes/core/tool_calling_llm.py |
Converts prompt/messages/call/call_stream APIs to async. |
holmes/core/llm.py |
Converts LLM.completion() / DefaultLLM.completion() to async and uses litellm.acompletion. |
Comments suppressed due to low confidence (3)
holmes/core/llm.py:136
LLM.completionis now async, but the repo still containsLLMsubclasses with a synchronousdef completion(...)(e.g.,tests/conftest.py:118,examples/custom_llm.py:35). Those will now be awaited byToolCallingLLMand fail at runtime. Update these implementations toasync def completion(...)(or provide a sync adapter) as part of this migration.
async def completion(
self,
messages: List[Dict[str, Any]],
tools: Optional[List[Dict[str, Any]]] = [],
tool_choice: Optional[Union[str, dict]] = None,
response_format: Optional[Union[dict, Type[BaseModel]]] = None,
temperature: Optional[float] = None,
drop_params: Optional[bool] = None,
stream: Optional[bool] = None,
) -> Union[ModelResponse, CustomStreamWrapper]:
pass
holmes/core/llm.py:134
- The abstract
LLM.completionstill uses a mutable default (tools: ... = []). This can create shared state across calls/implementations and is inconsistent withDefaultLLM.completion(which defaults toNone). UseNoneas the default and normalize inside the method.
async def completion(
self,
messages: List[Dict[str, Any]],
tools: Optional[List[Dict[str, Any]]] = [],
tool_choice: Optional[Union[str, dict]] = None,
response_format: Optional[Union[dict, Type[BaseModel]]] = None,
temperature: Optional[float] = None,
drop_params: Optional[bool] = None,
stream: Optional[bool] = None,
holmes/core/tool_calling_llm.py:395
- Making
prompt_call/messages_callasync is a breaking API change: there are still synchronous call sites in the repo (e.g. tests and examples) that callai.messages_call(...)withoutawait/asyncio.run, which will now return a coroutine and fail at runtime. Either update those call sites in this PR or provide backward-compatible sync wrappers (e.g.,messages_call_sync/prompt_call_sync).
async def prompt_call(
self,
system_prompt: str,
user_prompt: str,
response_format: Optional[Union[dict, Type[BaseModel]]] = None,
trace_span=DummySpan(),
request_context: Optional[Dict[str, Any]] = None,
) -> LLMResult:
messages = [
{"role": "system", "content": system_prompt},
{"role": "user", "content": user_prompt},
]
return await self.call(
messages,
response_format=response_format,
user_prompt=user_prompt,
trace_span=trace_span,
request_context=request_context,
)
async def messages_call(
self,
messages: List[Dict[str, str]],
response_format: Optional[Union[dict, Type[BaseModel]]] = None,
trace_span=DummySpan(),
request_context: Optional[Dict[str, Any]] = None,
) -> LLMResult:
return await self.call(
messages,
response_format=response_format,
trace_span=trace_span,
request_context=request_context,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @app.post("/api/chat") | ||
| def chat(chat_request: ChatRequest, http_request: Request): | ||
| async def chat(chat_request: ChatRequest, http_request: Request): | ||
| try: |
There was a problem hiding this comment.
chat was changed to async def, but it’s passed into ScheduledPromptsExecutor as chat_function. The executor calls self.chat_function(...) synchronously (no await), so it will now receive a coroutine instead of a ChatResponse and scheduled prompts will break. Update the executor (or provide a sync wrapper) so scheduled prompts can invoke the async chat properly.
| @@ -428,7 +428,7 @@ def completion( | |||
| ] | |||
|
|
|||
| litellm_model_name = self.get_litellm_corrected_name_for_robusta_ai() | |||
| result = litellm_to_use.completion( | |||
| result = await litellm_to_use.acompletion( | |||
| model=litellm_model_name, | |||
There was a problem hiding this comment.
DefaultLLM.completion now calls litellm_to_use.acompletion(...), but BraintrustTracer.wrap_llm() currently only wraps the sync completion path (it sets acreate_fn=None and does not override acompletion). When a tracer is enabled, async completions will bypass tracing. Update the tracer wrapper to support acompletion (e.g., provide acreate_fn/an acompletion wrapper) so observability is preserved after this async migration.
| async def compact_conversation_history( | ||
| original_conversation_history: list[dict], llm: LLM | ||
| ) -> CompactionResult: |
There was a problem hiding this comment.
compact_conversation_history is now async, but there are still synchronous callers in the repo (including tests) that invoke it without await, which will now return a coroutine and break. Update those call sites or keep a sync wrapper for backward compatibility (e.g., compact_conversation_history_sync that calls asyncio.run).
| response = asyncio.run( | ||
| self._fast_llm.completion([{"role": "user", "content": full_prompt}]) | ||
| ) |
There was a problem hiding this comment.
LLMSummarizeTransformer keeps a shared _fast_llm instance and calls it via asyncio.run(...). In server mode, transformer instances are cached on Tool objects and tools are shared across requests, while tool invocations run concurrently in a thread pool—so multiple threads can call this shared _fast_llm at the same time. Since DefaultLLM.completion mutates instance state (self.args), this can lead to cross-request parameter leakage/races. Consider creating a per-call LLM instance here or guarding the _fast_llm.completion call with a lock / making DefaultLLM.completion side-effect free.
| ) | ||
| else: | ||
| try: | ||
| llm_call = ai.messages_call( | ||
| llm_call = await ai.messages_call( | ||
| messages=messages, | ||
| trace_span=chat_request.trace_span, | ||
| response_format=chat_request.response_format, |
There was a problem hiding this comment.
In this handler, ToolCallingLLM.call_stream has been converted to an async generator, but the chat_request.stream branch above still feeds ai.call_stream(...) into stream_chat_formatter, which is implemented as a normal (sync) generator and iterates with for. That combination will raise at runtime. The streaming path needs to be updated to consume an async iterable (e.g., async for) and/or make stream_chat_formatter async so StreamingResponse receives an async iterator.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
holmes/core/tool_calling_llm.py (1)
438-470:⚠️ Potential issue | 🟠 MajorEvent loop blocking in async tool-calling paths via concurrent.futures.
Both
call()andcall_stream()useconcurrent.futures.ThreadPoolExecutorwithas_completed()andfuture.result(), which block the event loop. Additionally,call_stream()calls synchronousprocess_tool_decisions()without awaiting, and callsfuture.result()in the async context. Under FastAPI, this blocks the event loop for the full tool execution duration, stalling unrelated requests.Replace
concurrent.futures.as_completed()andexecutor.submit()withasyncio.to_thread()and consume withasyncio.as_completed(). Also:call_stream()is missing a return type annotation (should beAsyncGenerator).Also applies to: 547-576, 941-945, 971-1009, 1086-1107
Additionally,
DummySpan()default arguments on lines 368, 387, 414, 792, 1095 create shared instances across calls—use per-call initialization instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 438 - 470, The async tool-calling paths block the event loop because call() and call_stream() use concurrent.futures.ThreadPoolExecutor with as_completed()/future.result() and call_stream() invokes process_tool_decisions() synchronously; fix by converting executor.submit/as_completed usage to asyncio.to_thread and consuming with asyncio.as_completed (or use asyncio.create_task + asyncio.as_completed) so you await thread-bound work without blocking, replace any future.result() calls with awaiting the asyncio tasks, and ensure process_tool_decisions() is awaited when invoked from async flows; add the correct return type AsyncGenerator to call_stream() signature; finally, remove shared-default DummySpan instances by creating new DummySpan() per call site (initialize inside functions where DummySpan was a default arg) to avoid shared mutable defaults.server.py (1)
305-312:⚠️ Potential issue | 🟠 MajorConvert streaming wrappers to handle async generators.
The recent migration to async LLM calls introduced a bug:
ai.call_stream()is now anasync deffunction that yields (an async generator), butstream_chat_formatter()and_stream_with_storage_cleanup()are still sync functions usingforandyield from. Attempting to iterate an async generator with a syncforloop raisesTypeError: 'async_generator' object is not iterableimmediately on any streaming request.Convert both
stream_chat_formatter()(inholmes/utils/stream.py) and_stream_with_storage_cleanup()(inserver.py) toasync defand useasync forinstead offor/yield from:
stream_chat_formatter: Changefor message in call_stream:toasync for message in call_stream:_stream_with_storage_cleanup: Changeyield from stream_generatorto useasync forandyield🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 305 - 312, The streaming wrappers are still synchronous and can't iterate async generators; change _stream_with_storage_cleanup and stream_chat_formatter to async functions (async def) and replace their synchronous iteration/yield logic with async iteration: in stream_chat_formatter use "async for message in call_stream" instead of "for", and in _stream_with_storage_cleanup use "async for item in stream_generator" and "yield item" (instead of yield from), keeping the finally cleanup (logging and calling storage.__exit__(None, None, None)) intact so cleanup runs after the async iteration completes.
🧹 Nitpick comments (1)
holmes/core/tool_calling_llm.py (1)
363-395: Avoid sharing a singleDummySpan()across calls.These defaults are evaluated once at import time, so every request without an explicit span reuses the same
DummySpaninstance. That gets harder to reason about now that these entry points can run concurrently. Default toNoneand create a fresh span inside each method instead.♻️ Suggested shape
async def prompt_call( self, system_prompt: str, user_prompt: str, response_format: Optional[Union[dict, Type[BaseModel]]] = None, - trace_span=DummySpan(), + trace_span=None, request_context: Optional[Dict[str, Any]] = None, ) -> LLMResult: + trace_span = trace_span or DummySpan() messages = [ {"role": "system", "content": system_prompt}, {"role": "user", "content": user_prompt}, ] async def messages_call( self, messages: List[Dict[str, str]], response_format: Optional[Union[dict, Type[BaseModel]]] = None, - trace_span=DummySpan(), + trace_span=None, request_context: Optional[Dict[str, Any]] = None, ) -> LLMResult: + trace_span = trace_span or DummySpan() return await self.call( messages, response_format=response_format, trace_span=trace_span, request_context=request_context, ) async def call( # type: ignore self, messages: List[Dict[str, str]], response_format: Optional[Union[dict, Type[BaseModel]]] = None, user_prompt: Optional[str] = None, - trace_span=DummySpan(), + trace_span=None, tool_number_offset: int = 0, request_context: Optional[Dict[str, Any]] = None, cancel_event: Optional[threading.Event] = None, ) -> LLMResult: + trace_span = trace_span or DummySpan()Also applies to: 409-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 363 - 395, The methods prompt_call and messages_call (and the other similar methods around lines 409-418) currently use a single DummySpan() instance as a default which causes shared mutable state across concurrent calls; change the signature to default trace_span: Optional[SpanType] = None and inside each method create a fresh span when trace_span is None (e.g., trace_span = DummySpan() or equivalent) before passing it to self.call; update prompt_call, messages_call and the other mentioned methods to follow this pattern so each invocation gets its own span instance.
🤖 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/core/transformers/llm_summarize.py`:
- Around line 154-156: The transformer currently calls asyncio.run() inside
transform(), which will raise RuntimeError when invoked from an existing event
loop; change the transformer to be async-aware by converting transform() into an
async def and replace asyncio.run(self._fast_llm.completion(...)) with await
self._fast_llm.completion(...) (referencing transform() and
_fast_llm.completion()), and update any call sites (e.g., where transformers are
invoked in process_tool_decisions()/call_stream()) to await the async transform;
if changing callers is infeasible, alternatively wrap the completion call with
loop.run_in_executor(...) or provide a synchronous wrapper for
_fast_llm.completion() and call that instead—ensure no use of asyncio.run()
remains.
In `@server.py`:
- Line 315: The ScheduledPromptsExecutor currently calls the async chat function
(self.chat_function(...)) synchronously in _execute_prompt, returning a
coroutine instead of running it; update _execute_prompt to execute the coroutine
and get a real response by running it in an event loop (e.g., use
asyncio.run(...) or create a new event loop and run_until_complete) when
invoking self.chat_function, then use the returned response.metadata as before;
alternatively make _execute_prompt async and await self.chat_function(...) and
ensure the background executor invokes the coroutine properly.
---
Outside diff comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 438-470: The async tool-calling paths block the event loop because
call() and call_stream() use concurrent.futures.ThreadPoolExecutor with
as_completed()/future.result() and call_stream() invokes
process_tool_decisions() synchronously; fix by converting
executor.submit/as_completed usage to asyncio.to_thread and consuming with
asyncio.as_completed (or use asyncio.create_task + asyncio.as_completed) so you
await thread-bound work without blocking, replace any future.result() calls with
awaiting the asyncio tasks, and ensure process_tool_decisions() is awaited when
invoked from async flows; add the correct return type AsyncGenerator to
call_stream() signature; finally, remove shared-default DummySpan instances by
creating new DummySpan() per call site (initialize inside functions where
DummySpan was a default arg) to avoid shared mutable defaults.
In `@server.py`:
- Around line 305-312: The streaming wrappers are still synchronous and can't
iterate async generators; change _stream_with_storage_cleanup and
stream_chat_formatter to async functions (async def) and replace their
synchronous iteration/yield logic with async iteration: in stream_chat_formatter
use "async for message in call_stream" instead of "for", and in
_stream_with_storage_cleanup use "async for item in stream_generator" and "yield
item" (instead of yield from), keeping the finally cleanup (logging and calling
storage.__exit__(None, None, None)) intact so cleanup runs after the async
iteration completes.
---
Nitpick comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 363-395: The methods prompt_call and messages_call (and the other
similar methods around lines 409-418) currently use a single DummySpan()
instance as a default which causes shared mutable state across concurrent calls;
change the signature to default trace_span: Optional[SpanType] = None and inside
each method create a fresh span when trace_span is None (e.g., trace_span =
DummySpan() or equivalent) before passing it to self.call; update prompt_call,
messages_call and the other mentioned methods to follow this pattern so each
invocation gets its own span instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7707e1b1-2e22-46d3-a552-c983665afb6b
📥 Commits
Reviewing files that changed from the base of the PR and between 39b8509 and 59e145c043443d524fc9b731b93b4818d345b36e.
📒 Files selected for processing (8)
holmes/core/llm.pyholmes/core/tool_calling_llm.pyholmes/core/transformers/llm_summarize.pyholmes/core/truncation/compaction.pyholmes/core/truncation/input_context_window_limiter.pyholmes/interactive.pyholmes/main.pyserver.py
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 (5)
server.py (1)
367-418:⚠️ Potential issue | 🟠 MajorClose
tool_result_storage()when setup fails before the response handoff.
storage.__enter__()happens beforecreate_toolcalling_llm(),get_global_instructions_for_account(), andbuild_chat_messages(), but cleanup only exists inside the non-streamingfinallyor the returned streaming generator. Any exception during setup leaks the storage context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 367 - 418, The storage context created by tool_result_storage() is entered via storage.__enter__() but only exited in the streaming return path or the non-streaming finally, so any exception during create_toolcalling_llm(), get_global_instructions_for_account(), or build_chat_messages() will leak the storage; fix by ensuring storage.__exit__(...) is called on all failure paths—either convert the pattern to a proper context manager usage (with tool_result_storage() as tool_results_dir: ...) surrounding create_toolcalling_llm, get_global_instructions_for_account, and build_chat_messages, or add a try/except/finally around those setup calls that calls storage.__exit__(exc_type, exc, tb) on exception before re-raising; reference tool_result_storage(), storage.__enter__(), storage.__exit__(), create_toolcalling_llm, and build_chat_messages to locate the change.holmes/interactive.py (1)
1419-1425:⚠️ Potential issue | 🟠 MajorRefresh the active tool executor after
/configsaves.
run_toolset_config_tui()updates the in-memoryconfig, but this session keeps using the already-constructedai.tool_executor. After a save,/toolsand subsequent LLM/tool calls can still run against stale toolset instances until the user restarts interactive mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/interactive.py` around lines 1419 - 1425, run_toolset_config_tui() mutates the in-memory config but the session keeps using the old ai.tool_executor, causing stale toolsets after a save; after the call where config is not None, recreate or refresh ai.tool_executor using the updated config (e.g., invoke the same factory/constructor used elsewhere to build a ToolExecutor from config) and preserve or reapply preloaded_toolsets (ai.tool_executor.toolsets) as needed so subsequent /tools and LLM/tool invocations use the updated toolset instances.holmes/core/tool_calling_llm.py (1)
363-390:⚠️ Potential issue | 🟡 MinorAvoid
DummySpan()as a default argument.Using mutable objects as default arguments is a Python anti-pattern. The same
DummySpaninstance is reused across all calls to these methods. Default toNoneand create a freshDummySpan()inside each method instead.Suggested fix
async def prompt_call( self, system_prompt: str, user_prompt: str, response_format: Optional[Union[dict, Type[BaseModel]]] = None, - trace_span=DummySpan(), + trace_span=None, request_context: Optional[Dict[str, Any]] = None, ) -> LLMResult: + trace_span = trace_span or DummySpan() @@ async def messages_call( self, messages: List[Dict[str, str]], response_format: Optional[Union[dict, Type[BaseModel]]] = None, - trace_span=DummySpan(), + trace_span=None, request_context: Optional[Dict[str, Any]] = None, ) -> LLMResult: + trace_span = trace_span or DummySpan() @@ async def call( # type: ignore self, messages: List[Dict[str, str]], response_format: Optional[Union[dict, Type[BaseModel]]] = None, user_prompt: Optional[str] = None, - trace_span=DummySpan(), + trace_span=None, tool_number_offset: int = 0, request_context: Optional[Dict[str, Any]] = None, cancel_event: Optional[threading.Event] = None, ) -> LLMResult: + trace_span = trace_span or DummySpan()Also applies to: 409-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 363 - 390, The methods prompt_call and messages_call (and the other method around lines 409-418) currently use DummySpan() as a default argument which reuses the same instance; change the signature of these functions to default trace_span: Optional[SpanType] = None (or trace_span=None) and inside each method do if trace_span is None: trace_span = DummySpan() before using it; update any other methods that pass trace_span similarly (the second block at 409-418) to follow the same pattern so each call gets a fresh DummySpan instance.holmes/core/llm.py (1)
126-136:⚠️ Potential issue | 🟠 MajorUpdate all
LLMsubclasses to implement asynccompletion()method.The abstract interface changed from
def completion(...)toasync def completion(...). ToolCallingLLM now awaitsself.llm.completion(...), but three concrete subclasses still have synchronous implementations:
examples/custom_llm.py:35(MyCustomLLM)tests/conftest.py:118(MockLLM)tests/core/test_feedback.py:41(MockLLM)These will fail with
TypeErrorwhen awaited. Convert all three toasync def completion(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/llm.py` around lines 126 - 136, The LLM interface changed to async, but concrete implementations still define synchronous completion methods; update MyCustomLLM (examples/custom_llm.py) and both MockLLM definitions (tests/conftest.py and tests/core/test_feedback.py) to use async def completion(...) matching the signature in holmes/core/llm.py so they can be awaited by ToolCallingLLM; keep the same parameters and return types (ModelResponse or CustomStreamWrapper), and ensure any internal calls are awaited or return awaitable results (e.g., await any sync helpers or wrap return values appropriately).tests/test_server_endpoints.py (1)
23-38:⚠️ Potential issue | 🟠 MajorFinish the async mock migration in the remaining image tests.
/api/chatawaitsai.messages_call(...)at line 402 of server.py, buttest_api_chat_with_imagesandtest_api_chat_with_images_advanced_formatstill use plainMagicMockwith synchronousside_effectfunctions. Awaiting those will fail withTypeError, causing test failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_server_endpoints.py` around lines 23 - 38, The image tests fail because mock_ai.messages_call is awaited in server.py (ai.messages_call) but the tests test_api_chat_with_images and test_api_chat_with_images_advanced_format still assign a synchronous MagicMock/side_effect; change those to use AsyncMock (or set return_value to an AsyncMock) and ensure any side_effect handlers are async def so awaiting works; update the mocks in tests/test_server_endpoints.py (mock_ai.messages_call) to mirror the working example used earlier (AsyncMock returning an object with .result, .tool_calls, .messages, .metadata) so the awaited call in server.py succeeds.
🧹 Nitpick comments (1)
holmes/utils/stream.py (1)
66-68: Restore the async generator type annotation here.This public API lost its type hints during the async migration, so mypy can no longer validate callers or yielded item types. As per coding guidelines, "Type hints required (mypy configuration in pyproject.toml)".
Proposed typing cleanup
-from typing import Generator, List, Optional, Union +from typing import AsyncIterator, Generator, List, Optional, Union @@ async def stream_chat_formatter( - call_stream, + call_stream: AsyncIterator[StreamMessage], followups: Optional[List[dict]] = None, -): +) -> AsyncIterator[str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/utils/stream.py` around lines 66 - 68, The public async generator stream_chat_formatter lost its return type hint; restore its async generator annotation by updating the signature of stream_chat_formatter to return AsyncGenerator[...] (e.g. AsyncGenerator[dict, None]) and ensure AsyncGenerator (and Optional, List if not already) are imported from typing at the top of holmes/utils/stream.py so mypy can validate callers and yielded item types.
🤖 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 `@holmes/core/llm.py`:
- Around line 126-136: The LLM interface changed to async, but concrete
implementations still define synchronous completion methods; update MyCustomLLM
(examples/custom_llm.py) and both MockLLM definitions (tests/conftest.py and
tests/core/test_feedback.py) to use async def completion(...) matching the
signature in holmes/core/llm.py so they can be awaited by ToolCallingLLM; keep
the same parameters and return types (ModelResponse or CustomStreamWrapper), and
ensure any internal calls are awaited or return awaitable results (e.g., await
any sync helpers or wrap return values appropriately).
In `@holmes/core/tool_calling_llm.py`:
- Around line 363-390: The methods prompt_call and messages_call (and the other
method around lines 409-418) currently use DummySpan() as a default argument
which reuses the same instance; change the signature of these functions to
default trace_span: Optional[SpanType] = None (or trace_span=None) and inside
each method do if trace_span is None: trace_span = DummySpan() before using it;
update any other methods that pass trace_span similarly (the second block at
409-418) to follow the same pattern so each call gets a fresh DummySpan
instance.
In `@holmes/interactive.py`:
- Around line 1419-1425: run_toolset_config_tui() mutates the in-memory config
but the session keeps using the old ai.tool_executor, causing stale toolsets
after a save; after the call where config is not None, recreate or refresh
ai.tool_executor using the updated config (e.g., invoke the same
factory/constructor used elsewhere to build a ToolExecutor from config) and
preserve or reapply preloaded_toolsets (ai.tool_executor.toolsets) as needed so
subsequent /tools and LLM/tool invocations use the updated toolset instances.
In `@server.py`:
- Around line 367-418: The storage context created by tool_result_storage() is
entered via storage.__enter__() but only exited in the streaming return path or
the non-streaming finally, so any exception during create_toolcalling_llm(),
get_global_instructions_for_account(), or build_chat_messages() will leak the
storage; fix by ensuring storage.__exit__(...) is called on all failure
paths—either convert the pattern to a proper context manager usage (with
tool_result_storage() as tool_results_dir: ...) surrounding
create_toolcalling_llm, get_global_instructions_for_account, and
build_chat_messages, or add a try/except/finally around those setup calls that
calls storage.__exit__(exc_type, exc, tb) on exception before re-raising;
reference tool_result_storage(), storage.__enter__(), storage.__exit__(),
create_toolcalling_llm, and build_chat_messages to locate the change.
In `@tests/test_server_endpoints.py`:
- Around line 23-38: The image tests fail because mock_ai.messages_call is
awaited in server.py (ai.messages_call) but the tests test_api_chat_with_images
and test_api_chat_with_images_advanced_format still assign a synchronous
MagicMock/side_effect; change those to use AsyncMock (or set return_value to an
AsyncMock) and ensure any side_effect handlers are async def so awaiting works;
update the mocks in tests/test_server_endpoints.py (mock_ai.messages_call) to
mirror the working example used earlier (AsyncMock returning an object with
.result, .tool_calls, .messages, .metadata) so the awaited call in server.py
succeeds.
---
Nitpick comments:
In `@holmes/utils/stream.py`:
- Around line 66-68: The public async generator stream_chat_formatter lost its
return type hint; restore its async generator annotation by updating the
signature of stream_chat_formatter to return AsyncGenerator[...] (e.g.
AsyncGenerator[dict, None]) and ensure AsyncGenerator (and Optional, List if not
already) are imported from typing at the top of holmes/utils/stream.py so mypy
can validate callers and yielded item types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec5b915a-ba79-496c-aaa3-c94689a8dacc
📥 Commits
Reviewing files that changed from the base of the PR and between 59e145c043443d524fc9b731b93b4818d345b36e and 093bcee.
📒 Files selected for processing (11)
holmes/core/llm.pyholmes/core/tool_calling_llm.pyholmes/core/transformers/llm_summarize.pyholmes/core/truncation/compaction.pyholmes/core/truncation/input_context_window_limiter.pyholmes/interactive.pyholmes/main.pyholmes/utils/stream.pyserver.pytests/core/transformers/test_llm_summarize.pytests/test_server_endpoints.py
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/core/transformers/llm_summarize.py
- holmes/core/truncation/input_context_window_limiter.py
- holmes/core/truncation/compaction.py
mainred
changed the title
mainred
changed the title
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)
holmes/core/tool_calling_llm.py (1)
607-621:⚠️ Potential issue | 🔴 CriticalThe async migration left this file with critical syntax errors that prevent import and a blocking synchronous call in an async context.
- Lines 611–621 (and 1116–1158) have unexpected indentation that breaks the syntax
- Line 950 contains a bare
yield eventstatement (should iterate over the events collection)process_tool_decisions()is a synchronous method called without await from the asynccall_stream()function, which will block the event loop when processing tool approvalsThe module will not import as written. Once syntax errors are fixed, approved-tool replays will still block the server event loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 607 - 621, The file has syntax/async issues: fix the bad indentation around the runbook refresh block (the block that updates tool_number_offset, re-fetches tools via self._get_tools(), logs the runbook activation, and logs the blank line after tools_to_call) so indentation is consistent and the module parses; replace the bare "yield event" (should iterate the events collection) with a proper loop that yields each event from the events iterable; and make process_tool_decisions non-blocking by either converting it to async (async def process_tool_decisions(...)) and awaiting it from call_stream, or by running the existing sync process_tool_decisions in an executor (e.g., loop.run_in_executor) from call_stream so tool approval replay does not block the event loop—update call_stream to await the async call or the run_in_executor future accordingly.
♻️ Duplicate comments (2)
server.py (1)
315-315:⚠️ Potential issue | 🔴 CriticalMaking
chat()async also breaks the scheduled-prompts callback contract.
ScheduledPromptsExecutorstill consumeschat_function(...)synchronously in its background thread. Passingchathere now returns a coroutine object instead of aChatResponse, so that path will fail as soon as it reads response fields unless the executor starts awaiting or running the callback.Verification script
#!/bin/bash set -e rg -n "async def chat|chat_function|self\.chat_function\(" \ server.py \ holmes/core/scheduled_prompts/executor.py -A6 -B2Also applies to: 427-429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` at line 315, The chat handler was changed to an async def (chat) but ScheduledPromptsExecutor expects a synchronous callback (it calls self.chat_function(...) and reads response fields), so the executor will receive a coroutine instead of a ChatResponse; either restore chat to a synchronous function that returns ChatResponse (keeping signature chat(chat_request: ChatRequest, http_request: Request) as a normal def) or modify ScheduledPromptsExecutor to detect coroutine results from chat_function and run/await them on an event loop (e.g., use asyncio.run / loop.run_until_complete or schedule in an existing loop) before accessing response fields so that calls to chat_function always yield a concrete ChatResponse. Ensure references to chat, ChatRequest/ChatResponse, and ScheduledPromptsExecutor/chat_function are updated accordingly.holmes/core/transformers/llm_summarize.py (1)
154-156:⚠️ Potential issue | 🔴 Critical
asyncio.run()here still blows up approved-tool replays.
transform()is synchronous, but approved tools are now replayed from asynccall_stream()beforetools.pycallstransform(). When that path reaches Line 154, Python raisesRuntimeError: asyncio.run() cannot be called from a running event loop. Make the transformer path async-aware, or keep approved-tool replay on a worker thread instead of nesting an event loop here.Verification script
#!/bin/bash set -e rg -n "def transform\(|asyncio.run\(|transformer_instance\.transform\(|def process_tool_decisions\(|async def call_stream" \ holmes/core/transformers/llm_summarize.py \ holmes/core/tools.py \ holmes/core/tool_calling_llm.py -A6 -B2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/transformers/llm_summarize.py` around lines 154 - 156, The synchronous transform() calls asyncio.run(self._fast_llm.completion(...)) which fails when an event loop is already running (approved-tool replay from async call_stream()); refactor by adding an async variant (e.g., async def transform_async(...)) that awaits self._fast_llm.completion(...) and move the asyncio.run call out of the async path: keep transform() as a thin sync wrapper that calls asyncio.run(transform_async(...)) for legacy synchronous callers, and update async callers (call_stream() / tools.py where approved-tool replay happens) to await transformer.transform_async(...) instead of calling transformer.transform(), removing the direct asyncio.run() invocation inside llm_summarize.py.
🤖 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/core/tool_calling_llm.py`:
- Around line 559-567: The parallel dispatch creates tasks passing the same
mutable previous_tool_calls (tool_calls) to each _invoke_llm_tool_call, allowing
race-through of prevent_overly_repeated_tool_call(); fix by
deduplicating/reserving tools before spawning tasks or performing the guard
serially: compute a deduped list or mark/reserve entries in tool_calls (the
previous_tool_calls structure) then spawn
asyncio.create_task(...asyncio.to_thread(self._invoke_llm_tool_call, ...)) only
for non-duplicate/reserved tools, or alternatively call
prevent_overly_repeated_tool_call() synchronously for each tool and skip
spawning threads for those that fail the guard (ensure changes touch the code
paths that build previous_tool_calls, the call-site that creates tasks with
asyncio.create_task/asyncio.to_thread, and the
_invoke_llm_tool_call/prevent_overly_repeated_tool_call functions).
In `@server.py`:
- Line 315: The streaming functions are trying to iterate a now-async generator
from ai.call_stream() with synchronous constructs; update stream_chat_formatter
to be async and replace its synchronous loop with an "async for message in
call_stream" loop (and await any awaits inside), and update
_stream_with_storage_cleanup to be async as well and use "async for" / "async
yield" (or "async for" collecting and then "yield") so they consume the async
generator correctly when called from async def chat; alternatively, if you
prefer to keep them sync, wrap ai.call_stream() with a synchronous bridge that
consumes the async generator and yields sync items before passing it in—apply
the change to the functions named stream_chat_formatter and
_stream_with_storage_cleanup and ensure chat passes the async generator to the
newly-async helpers.
---
Outside diff comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 607-621: The file has syntax/async issues: fix the bad indentation
around the runbook refresh block (the block that updates tool_number_offset,
re-fetches tools via self._get_tools(), logs the runbook activation, and logs
the blank line after tools_to_call) so indentation is consistent and the module
parses; replace the bare "yield event" (should iterate the events collection)
with a proper loop that yields each event from the events iterable; and make
process_tool_decisions non-blocking by either converting it to async (async def
process_tool_decisions(...)) and awaiting it from call_stream, or by running the
existing sync process_tool_decisions in an executor (e.g., loop.run_in_executor)
from call_stream so tool approval replay does not block the event loop—update
call_stream to await the async call or the run_in_executor future accordingly.
---
Duplicate comments:
In `@holmes/core/transformers/llm_summarize.py`:
- Around line 154-156: The synchronous transform() calls
asyncio.run(self._fast_llm.completion(...)) which fails when an event loop is
already running (approved-tool replay from async call_stream()); refactor by
adding an async variant (e.g., async def transform_async(...)) that awaits
self._fast_llm.completion(...) and move the asyncio.run call out of the async
path: keep transform() as a thin sync wrapper that calls
asyncio.run(transform_async(...)) for legacy synchronous callers, and update
async callers (call_stream() / tools.py where approved-tool replay happens) to
await transformer.transform_async(...) instead of calling
transformer.transform(), removing the direct asyncio.run() invocation inside
llm_summarize.py.
In `@server.py`:
- Line 315: The chat handler was changed to an async def (chat) but
ScheduledPromptsExecutor expects a synchronous callback (it calls
self.chat_function(...) and reads response fields), so the executor will receive
a coroutine instead of a ChatResponse; either restore chat to a synchronous
function that returns ChatResponse (keeping signature chat(chat_request:
ChatRequest, http_request: Request) as a normal def) or modify
ScheduledPromptsExecutor to detect coroutine results from chat_function and
run/await them on an event loop (e.g., use asyncio.run / loop.run_until_complete
or schedule in an existing loop) before accessing response fields so that calls
to chat_function always yield a concrete ChatResponse. Ensure references to
chat, ChatRequest/ChatResponse, and ScheduledPromptsExecutor/chat_function are
updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74c11b23-efb1-42b1-a3cc-27088c2e32ae
📥 Commits
Reviewing files that changed from the base of the PR and between 093bcee and b320ed896eef6454f527570f89dcd9e4325299be.
📒 Files selected for processing (10)
holmes/core/llm.pyholmes/core/tool_calling_llm.pyholmes/core/transformers/llm_summarize.pyholmes/core/truncation/compaction.pyholmes/core/truncation/input_context_window_limiter.pyholmes/interactive.pyholmes/main.pyserver.pytests/core/transformers/test_llm_summarize.pytests/test_server_endpoints.py
🚧 Files skipped from review as they are similar to previous changes (4)
- holmes/core/truncation/compaction.py
- tests/test_server_endpoints.py
- holmes/core/truncation/input_context_window_limiter.py
- holmes/interactive.py
| task = asyncio.create_task( | ||
| asyncio.to_thread( | ||
| self._invoke_llm_tool_call, | ||
| tool_to_call=t, | ||
| previous_tool_calls=tool_calls, | ||
| trace_span=trace_span, | ||
| tool_number=tool_number, | ||
| request_context=request_context, | ||
| ) |
There was a problem hiding this comment.
Parallel dispatch bypasses the duplicate-call safeguard.
Each worker gets the same previous_tool_calls=tool_calls reference before any sibling result is appended. If the model emits the same tool twice in one turn, both threads can clear prevent_overly_repeated_tool_call() and execute the external side effect twice. Reserve/dedupe the batch before spawning threads, or serialize just the guard check.
Also applies to: 1095-1104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/core/tool_calling_llm.py` around lines 559 - 567, The parallel
dispatch creates tasks passing the same mutable previous_tool_calls (tool_calls)
to each _invoke_llm_tool_call, allowing race-through of
prevent_overly_repeated_tool_call(); fix by deduplicating/reserving tools before
spawning tasks or performing the guard serially: compute a deduped list or
mark/reserve entries in tool_calls (the previous_tool_calls structure) then
spawn asyncio.create_task(...asyncio.to_thread(self._invoke_llm_tool_call, ...))
only for non-duplicate/reserved tools, or alternatively call
prevent_overly_repeated_tool_call() synchronously for each tool and skip
spawning threads for those that fail the guard (ensure changes touch the code
paths that build previous_tool_calls, the call-site that creates tasks with
asyncio.create_task/asyncio.to_thread, and the
_invoke_llm_tool_call/prevent_overly_repeated_tool_call functions).
Signed-off-by: Qingchuan Hao <qingchuan.hao@microsoft.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/plugins/toolsets/grafana/test_grafana_tempo_tools.py (1)
558-560:⚠️ Potential issue | 🟡 MinorMove method-local imports out of test bodies.
Line 558, Line 589, and Line 669 import a module inside methods. Keep imports at module scope (or use string-based monkeypatch targets).
Suggested patch
- import holmes.plugins.toolsets.grafana.toolset_grafana_tempo as tempo_module - - monkeypatch.setattr(tempo_module, "MAX_GRAPH_POINTS", 100) + monkeypatch.setattr( + "holmes.plugins.toolsets.grafana.toolset_grafana_tempo.MAX_GRAPH_POINTS", + 100, + ) @@ - import holmes.plugins.toolsets.grafana.toolset_grafana_tempo as tempo_module - - monkeypatch.setattr(tempo_module, "MAX_GRAPH_POINTS", 100) + monkeypatch.setattr( + "holmes.plugins.toolsets.grafana.toolset_grafana_tempo.MAX_GRAPH_POINTS", + 100, + ) @@ - import holmes.plugins.toolsets.grafana.toolset_grafana_tempo as tempo_module - - monkeypatch.setattr(tempo_module, "MAX_GRAPH_POINTS", 100) + monkeypatch.setattr( + "holmes.plugins.toolsets.grafana.toolset_grafana_tempo.MAX_GRAPH_POINTS", + 100, + )As per coding guidelines, "Always place Python imports at the top of the file, not inside functions or methods."
Also applies to: 589-591, 669-671
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/toolsets/grafana/test_grafana_tempo_tools.py` around lines 558 - 560, Move the method-local imports of holmes.plugins.toolsets.grafana.toolset_grafana_tempo out of the test functions and into module scope (top of the test file) or change monkeypatch targets to string-based references; specifically, remove inline imports that assign tempo_module inside test bodies and instead import tempo_module once at module level and keep monkeypatch.setattr(tempo_module, "MAX_GRAPH_POINTS", 100) (and the similar uses around the other tests) so the tests reference the same top-level tempo_module import rather than performing imports inside the functions.holmes/utils/pydantic_utils.py (1)
223-227:⚠️ Potential issue | 🟡 MinorSupport PEP 604 unions when extracting nested BaseModel types.
Line 223 checks only
origin is Union, which misses PEP 604 union syntax likeNestedModel | None(which createstypes.UnionType). This causes nested examples to degrade to"your_<field>"unexpectedly.Add
UnionTypeto the import fromtypesand update the condition:Proposed fix
+from types import UnionType +from typing import ( Annotated, Any, ClassVar, Dict, List, Optional, Tuple, Type, Union, get_args, get_origin, ) - if origin is Union: + if origin in (Union, UnionType): args = [a for a in get_args(annotation) if a is not type(None)] # noqa: E721 if len(args) == 1: return _extract_base_model_subclass(args[0]) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/utils/pydantic_utils.py` around lines 223 - 227, The code in _extract_base_model_subclass currently only detects typing.Union via "if origin is Union:" and therefore misses PEP 604 unions (types.UnionType) like "NestedModel | None"; update the check to handle both Union and UnionType (import UnionType from types) and treat UnionType the same as Union by extracting non-None args via get_args and continuing the existing logic (i.e., if len(args) == 1 return _extract_base_model_subclass(args[0]) else return None). Ensure the new import for UnionType is added where other types are imported so both union forms are recognized.server.py (1)
394-411:⚠️ Potential issue | 🟠 MajorNon-streaming path uses synchronous
ai.call()inside async endpoint, blocking the event loop.The
chat()endpoint isasync def(line 309), but the non-streaming path callsai.call()synchronously at line 395 withoutawait. Theai.call()method is a synchronous wrapper that internally drains an async generator, which will block the event loop and prevent concurrent request handling. The streaming path correctly uses the asyncai.call_stream()at line 380.Wrap the call in
asyncio.to_thread()to run it on a thread pool executor, or refactor to use an async variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 394 - 411, The non-streaming branch in async chat() calls the blocking ai.call() (while ai.call_stream() is used for streaming), which will block the event loop; run the blocking call off the event loop (e.g., wrap the ai.call(...) invocation in asyncio.to_thread(...) or refactor to an async variant) and preserve the current finally cleanup (storage.__exit__(...)) and return construction (ChatResponse(...)); ensure you await the to_thread result and keep using llm_call.result, llm_call.tool_calls, llm_call.messages, and llm_call.metadata when building the response.
🧹 Nitpick comments (6)
holmes/core/scheduled_prompts/executor.py (1)
206-211: Consider using a persistent event loop for async chat_function calls in threaded context.The current approach creates a new event loop via
asyncio.run()for each scheduled prompt execution. Since_execute_promptruns in a background thread (started at line 68), this works but has considerations:
- Overhead: Creates/destroys an event loop per prompt execution
- No sharing: Cannot reuse connections or resources across async calls
For a thread that processes multiple prompts over its lifetime, consider maintaining a persistent event loop:
♻️ Alternative: persistent loop in the executor thread
class ScheduledPromptsExecutor: def __init__( self, dal: "SupabaseDal", config: "Config", chat_function: ChatFunction, ): self.dal = dal self.config = config self.chat_function = chat_function self.running = False self.thread: Optional[threading.Thread] = None + self._loop: Optional[asyncio.AbstractEventLoop] = None # ... def _run_loop(self): + self._loop = asyncio.new_event_loop() + asyncio.set_event_loop(self._loop) while self.running: # ... + self._loop.close() + self._loop = NoneThen in
_execute_prompt:result = self.chat_function(chat_request, empty_request) if asyncio.iscoroutine(result): - response = asyncio.run(result) + response = self._loop.run_until_complete(result) else: response = result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/scheduled_prompts/executor.py` around lines 206 - 211, The use of asyncio.run() inside _execute_prompt causes a new event loop per call and prevents reusing async resources; instead create and retain a persistent event loop for the background thread that runs prompts and execute chat_function coroutines against it. Concretely: when the executor thread starts, create an event loop (asyncio.new_event_loop()) and either run it in that thread (loop.run_forever()) or keep it accessible; then in _execute_prompt detect coroutines with asyncio.iscoroutine(result) but submit them via asyncio.run_coroutine_threadsafe(result, loop) (or loop.call_soon_threadsafe with appropriate wrapper) so you reuse the same loop and connections across chat_function invocations and avoid repeatedly creating/destroying loops.holmes/toolset_config_tui.py (1)
154-156: Unusual annotation style with parentheses wrapping a comment.The
field_typeannotation uses parentheses to include an inline comment:field_type: ( str # "str" | "int" | "float" | "bool" | "enum" | "dict" | "list" | "model" )This is syntactically valid Python (the parentheses wrap the type
strwith trailing comment), and the type is correctlystr. However, this pattern is unconventional—typically a comment would go on a separate line or the type options would useLiteral[...]for type safety.💡 Consider using Literal type for self-documenting type safety
+from typing import Literal + +FieldTypeStr = Literal["str", "int", "float", "bool", "enum", "dict", "list", "model"] + `@dataclass` class ConfigFieldNode: """One row in the config tree.""" key: str - field_type: ( - str # "str" | "int" | "float" | "bool" | "enum" | "dict" | "list" | "model" - ) + field_type: FieldTypeStr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/toolset_config_tui.py` around lines 154 - 156, The annotation for field_type currently wraps the type in parentheses with an inline comment; replace this unconventional pattern by either (a) moving the explanatory comment to the previous line and using a plain annotation field_type: str, or (b) for stricter, self-documenting typing, change the annotation to use typing.Literal (e.g. field_type: Literal["str","int","float","bool","enum","dict","list","model"]) and import Literal from typing; update any usages of field_type to match the chosen annotation.holmes/core/tool_calling_llm.py (2)
638-651: Useasyncio.get_running_loop()instead of deprecatedasyncio.get_event_loop().
asyncio.get_event_loop()is deprecated in Python 3.10+ and will emit a DeprecationWarning in Python 3.12+ when called from a coroutine without a running event loop. Since this code is inside an async function, useasyncio.get_running_loop()instead.♻️ Suggested fix
if not tool_response: # Run sync tool execution in a thread pool to avoid blocking the event loop - loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop() tool_response = await loop.run_in_executor(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 638 - 651, Replace the deprecated asyncio.get_event_loop() call with asyncio.get_running_loop() in the async context where the thread-pool execution is started (the block that awaits loop.run_in_executor and assigns tool_response); specifically update the code that obtains loop before calling run_in_executor around the invocation of self._directly_invoke_tool_call so it uses asyncio.get_running_loop() to avoid deprecation warnings and ensure a running event loop is used.
330-347: Static analysis flags uninitialized variablesmessagesandtool_number_offset.Ruff reports that
messages(line 338) andtool_number_offset(line 344) are referenced before assignment in the inner_drain()function. These variables are captured from the outercall()method's parameters, but the closure captures them correctly. However, themessagesvariable is reassigned at line 402 inside the while loop, which modifies the outer scope binding.This is technically correct Python closure behavior, but consider using
nonlocal messagesto make the intent explicit and silence the linter warning.♻️ Suggested fix to clarify closure intent
async def _drain() -> LLMResult: + nonlocal messages all_tool_calls: list[dict] = [] tool_decisions: Optional[List[ToolApprovalDecision]] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 330 - 347, The inner async function _drain() reassigns outer-scope variables (messages and possibly tool_number_offset) which confuses static analysis; inside _drain(), add a nonlocal declaration (e.g., nonlocal messages, tool_number_offset) before any use or reassignment to make the closure intent explicit and silence the linter, keeping existing uses like call_stream(..., msgs=messages, tool_number_offset=tool_number_offset, ...) and the reassignment of messages later in the loop unchanged.tests/test_toolset_config_tui.py (1)
953-955: Unused variablemsgfrom tuple unpacking.The unpacked
msgvariable is never used. Prefix it with an underscore to indicate it's intentionally unused.♻️ Suggested fix
- ok, msg = save_config_to_file( + ok, _msg = save_config_to_file( config_file, "jira_server", config_dict, is_mcp=True )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_toolset_config_tui.py` around lines 953 - 955, The tuple returned by save_config_to_file is being unpacked into ok and msg, but msg is never used; update the unpacking in the test to use an underscore-prefixed name (e.g., _msg) instead of msg to indicate an intentionally unused variable. Locate the call to save_config_to_file in tests/test_toolset_config_tui.py (the line with ok, msg = save_config_to_file(...)) and change the second target to _msg so the unused variable warning is resolved.experimental/ag-ui/server-agui.py (1)
85-86: Endpointagui_chatis synchronous but contains async generator.The
agui_chatendpoint (line 86) is defined as a regulardef, but it returns aStreamingResponsewrapping an async generatorevent_generator. While FastAPI can handle this, the internal async generator callsai.call_stream()which is now an async generator.Consider making
agui_chatanasync deffor consistency with the async patterns adopted elsewhere in this PR.♻️ Suggested fix
`@app.post`("/api/agui/chat") -def agui_chat(input_data: RunAgentInput, request: Request): +async def agui_chat(input_data: RunAgentInput, request: Request):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/ag-ui/server-agui.py` around lines 85 - 86, The agui_chat endpoint is currently a synchronous def but relies on an async generator (event_generator) that calls the async generator ai.call_stream and is returned inside a StreamingResponse; change agui_chat to async def so it consistently runs in the event loop, and ensure event_generator is defined as async def and yields from ai.call_stream correctly (no blocking calls or awaits at the top-level of the endpoint), then return StreamingResponse(event_generator(), media_type="text/event-stream") as before.
🤖 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/core/llm.py`:
- Around line 689-709: The async method acompletion currently calls
litellm.acompletion() directly, so async LLM calls are not traced; update this
by using the tracer-wrapped LLM just like completion() does (i.e., call
self.tracer.wrap_llm(self.litellm) or the equivalent wrapped instance before
invoking acompletion), and if needed enhance the tracer implementation
(WrappedLiteLLM / ChatCompletionWrapper) to provide proper async support by
implementing/forwarding acompletion (or initializing ChatCompletionWrapper with
acreate_fn) so the wrapped object actually supports async operations and the
tracer observes async calls.
- Around line 237-258: The acompletion method uses a mutable default for tools
(tools: Optional[List[Dict[str, Any]]] = []); change the signature to use None
(tools: Optional[List[Dict[str, Any]]] = None) and inside acompletion set tools
= tools or [] (or list(tools) if you want a copy) before calling
self.completion, so you never share a single list instance; update any related
parameter handling passed to self.completion to use this local initialized list
and keep tool_choice/response_format/temperature/drop_params/stream unchanged.
In `@holmes/plugins/toolsets/mongodb/mongodb.py`:
- Around line 35-37: The JSONDecodeError caught in the except block that
constructs the ValueError for invalid JSON should preserve the original
exception context by using exception chaining; update the raise in that block to
re-raise ValueError(...) from e (referencing the local variables param_name and
value in the same except block where json.JSONDecodeError is handled) so the
original traceback is kept.
In `@tests/plugins/toolsets/grafana/test_grafana_tempo_tools.py`:
- Around line 554-556: Update the test function signatures to include explicit
type hints for fixtures and a return annotation -> None; specifically add typing
to test_metrics_range_with_no_step_auto_calculates(tempo_toolset, monkeypatch)
and the other two affected tests (the ones starting at the places you noted) so
parameters are typed (e.g., tempo_toolset: TempoToolset and monkeypatch:
pytest.MonkeyPatch or appropriate fixture types used in the repo) and append ->
None; also add necessary imports for those types (from pytest and your test
fixtures module or typing) so mypy passes.
---
Outside diff comments:
In `@holmes/utils/pydantic_utils.py`:
- Around line 223-227: The code in _extract_base_model_subclass currently only
detects typing.Union via "if origin is Union:" and therefore misses PEP 604
unions (types.UnionType) like "NestedModel | None"; update the check to handle
both Union and UnionType (import UnionType from types) and treat UnionType the
same as Union by extracting non-None args via get_args and continuing the
existing logic (i.e., if len(args) == 1 return
_extract_base_model_subclass(args[0]) else return None). Ensure the new import
for UnionType is added where other types are imported so both union forms are
recognized.
In `@server.py`:
- Around line 394-411: The non-streaming branch in async chat() calls the
blocking ai.call() (while ai.call_stream() is used for streaming), which will
block the event loop; run the blocking call off the event loop (e.g., wrap the
ai.call(...) invocation in asyncio.to_thread(...) or refactor to an async
variant) and preserve the current finally cleanup (storage.__exit__(...)) and
return construction (ChatResponse(...)); ensure you await the to_thread result
and keep using llm_call.result, llm_call.tool_calls, llm_call.messages, and
llm_call.metadata when building the response.
In `@tests/plugins/toolsets/grafana/test_grafana_tempo_tools.py`:
- Around line 558-560: Move the method-local imports of
holmes.plugins.toolsets.grafana.toolset_grafana_tempo out of the test functions
and into module scope (top of the test file) or change monkeypatch targets to
string-based references; specifically, remove inline imports that assign
tempo_module inside test bodies and instead import tempo_module once at module
level and keep monkeypatch.setattr(tempo_module, "MAX_GRAPH_POINTS", 100) (and
the similar uses around the other tests) so the tests reference the same
top-level tempo_module import rather than performing imports inside the
functions.
---
Nitpick comments:
In `@experimental/ag-ui/server-agui.py`:
- Around line 85-86: The agui_chat endpoint is currently a synchronous def but
relies on an async generator (event_generator) that calls the async generator
ai.call_stream and is returned inside a StreamingResponse; change agui_chat to
async def so it consistently runs in the event loop, and ensure event_generator
is defined as async def and yields from ai.call_stream correctly (no blocking
calls or awaits at the top-level of the endpoint), then return
StreamingResponse(event_generator(), media_type="text/event-stream") as before.
In `@holmes/core/scheduled_prompts/executor.py`:
- Around line 206-211: The use of asyncio.run() inside _execute_prompt causes a
new event loop per call and prevents reusing async resources; instead create and
retain a persistent event loop for the background thread that runs prompts and
execute chat_function coroutines against it. Concretely: when the executor
thread starts, create an event loop (asyncio.new_event_loop()) and either run it
in that thread (loop.run_forever()) or keep it accessible; then in
_execute_prompt detect coroutines with asyncio.iscoroutine(result) but submit
them via asyncio.run_coroutine_threadsafe(result, loop) (or
loop.call_soon_threadsafe with appropriate wrapper) so you reuse the same loop
and connections across chat_function invocations and avoid repeatedly
creating/destroying loops.
In `@holmes/core/tool_calling_llm.py`:
- Around line 638-651: Replace the deprecated asyncio.get_event_loop() call with
asyncio.get_running_loop() in the async context where the thread-pool execution
is started (the block that awaits loop.run_in_executor and assigns
tool_response); specifically update the code that obtains loop before calling
run_in_executor around the invocation of self._directly_invoke_tool_call so it
uses asyncio.get_running_loop() to avoid deprecation warnings and ensure a
running event loop is used.
- Around line 330-347: The inner async function _drain() reassigns outer-scope
variables (messages and possibly tool_number_offset) which confuses static
analysis; inside _drain(), add a nonlocal declaration (e.g., nonlocal messages,
tool_number_offset) before any use or reassignment to make the closure intent
explicit and silence the linter, keeping existing uses like call_stream(...,
msgs=messages, tool_number_offset=tool_number_offset, ...) and the reassignment
of messages later in the loop unchanged.
In `@holmes/toolset_config_tui.py`:
- Around line 154-156: The annotation for field_type currently wraps the type in
parentheses with an inline comment; replace this unconventional pattern by
either (a) moving the explanatory comment to the previous line and using a plain
annotation field_type: str, or (b) for stricter, self-documenting typing, change
the annotation to use typing.Literal (e.g. field_type:
Literal["str","int","float","bool","enum","dict","list","model"]) and import
Literal from typing; update any usages of field_type to match the chosen
annotation.
In `@tests/test_toolset_config_tui.py`:
- Around line 953-955: The tuple returned by save_config_to_file is being
unpacked into ok and msg, but msg is never used; update the unpacking in the
test to use an underscore-prefixed name (e.g., _msg) instead of msg to indicate
an intentionally unused variable. Locate the call to save_config_to_file in
tests/test_toolset_config_tui.py (the line with ok, msg =
save_config_to_file(...)) and change the second target to _msg so the unused
variable warning is resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d753070a-97b6-4ba1-929a-b95a36d875f8
📥 Commits
Reviewing files that changed from the base of the PR and between b320ed896eef6454f527570f89dcd9e4325299be and a5e0d29.
📒 Files selected for processing (121)
.claude/skills/create-eval/SKILL.md.claude/skills/create-eval/references/anti-hallucination.md.claude/skills/create-eval/references/test-case-format.mdconftest.pydocs/data-sources/builtin-toolsets/splunk-mcp.mddocs/development/evaluations/history/frontier_5_models_20260314_204516.mddocs/development/evaluations/history/results_20260122_074259.mddocs/development/evaluations/history/results_20260127_161120.mddocs/development/evaluations/history/results_20260129_094857.mddocs/development/evaluations/history/results_20260311_210836.mddocs/development/evaluations/history/results_20260315_041151.mddocs/development/evaluations/model-comparison-summary-20260315.mdexamples/custom_llm.pyexperimental/ag-ui/server-agui.pyholmes/checks/__init__.pyholmes/checks/checks_api.pyholmes/common/cli_commons.pyholmes/config.pyholmes/core/azure_token.pyholmes/core/json_schema_coerce.pyholmes/core/llm.pyholmes/core/llm_usage.pyholmes/core/openai_formatting.pyholmes/core/scheduled_prompts/executor.pyholmes/core/tool_calling_llm.pyholmes/core/tools.pyholmes/core/tools_utils/tool_context_window_limiter.pyholmes/core/tools_utils/tool_executor.pyholmes/core/truncation/compaction.pyholmes/core/truncation/input_context_window_limiter.pyholmes/interactive.pyholmes/main.pyholmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/bash/validation.pyholmes/plugins/toolsets/confluence/confluence.pyholmes/plugins/toolsets/coralogix/utils.pyholmes/plugins/toolsets/database/database.pyholmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/elasticsearch/elasticsearch.pyholmes/plugins/toolsets/grafana/base_grafana_toolset.pyholmes/plugins/toolsets/grafana/common.pyholmes/plugins/toolsets/grafana/toolset_grafana.pyholmes/plugins/toolsets/http/http_toolset.pyholmes/plugins/toolsets/inspektor_gadget.yamlholmes/plugins/toolsets/internet/internet.pyholmes/plugins/toolsets/mcp/toolset_mcp.pyholmes/plugins/toolsets/mongodb/mongodb.pyholmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/servicenow_tables/servicenow_tables.pyholmes/toolset_config_tui.pyholmes/utils/holmes_status.pyholmes/utils/holmes_sync_toolsets.pyholmes/utils/pydantic_utils.pyholmes/utils/stream.pyholmes_operator/scheduler/job_executor.pyholmes_operator/scheduler/manager.pyscripts/cli_performance_benchmark.pyserver.pytests/checks/test_checks_api.pytests/checks/test_checks_cli.pytests/config_class/conftest.pytests/config_class/test_config_load_cloud_mcp.pytests/config_class/test_source_factory.pytests/core/test_image_token_counting.pytests/core/test_llm_api_base_version.pytests/core/test_prompt.pytests/core/test_tool_memory_limit.pytests/core/test_tool_output_deduplication.pytests/integration/test_kubernetes_transformer_execution.pytests/llm/conftest.pytests/llm/fixtures/test_ask_holmes/212_large_configmap_needle/generate_configs.pytests/llm/fixtures/test_ask_holmes/231_confluence_large_page_eval/generate_large_page.pytests/llm/fixtures/test_ask_holmes/50a_logs_since_last_specific_month/test_case.yamltests/llm/fixtures/test_ask_holmes/65_health_check_followup/test_case.yamltests/llm/test_ask_holmes.pytests/llm/test_holmes_checks.pytests/llm/utils/braintrust.pytests/llm/utils/braintrust_history.pytests/llm/utils/classifiers.pytests/llm/utils/mock_dal.pytests/llm/utils/property_manager.pytests/llm/utils/reporting/github_reporter.pytests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/test_case_utils.pytests/llm/utils/test_toolset.pytests/mocks/toolset_mocks.pytests/plugins/runbooks/test_catalog.pytests/plugins/sources/test_pagerduty_source.pytests/plugins/toolsets/database/test_database.pytests/plugins/toolsets/datadog/logs/test_check_prerequisites.pytests/plugins/toolsets/datadog/metrics/test_datadog_metrics_live.pytests/plugins/toolsets/datadog/traces/test_datadog_traces_live.pytests/plugins/toolsets/grafana/test_grafana_tempo_api.pytests/plugins/toolsets/grafana/test_grafana_tempo_tools.pytests/plugins/toolsets/grafana/test_grafana_tempo_unit.pytests/plugins/toolsets/http/test_http_toolset.pytests/plugins/toolsets/test_confluence_tools.pytests/plugins/toolsets/test_elasticsearch_mtls.pytests/plugins/toolsets/test_kubernetes_transformers.pytests/plugins/toolsets/test_logging_api.pytests/plugins/toolsets/test_runbook.pytests/test_approval_workflow.pytests/test_bash_toolset_validation.pytests/test_cache.pytests/test_check_prerequisites.pytests/test_database_toolset.pytests/test_format_tags.pytests/test_header_propagation.pytests/test_http_docs.pytests/test_json_schema_coerce.pytests/test_mcp_refresh_backoff.pytests/test_mcp_toolset.pytests/test_openai_formatting.pytests/test_server_endpoints.pytests/test_tool_calling_llm.pytests/test_toolset_auto_enable.pytests/test_toolset_config_tui.pytests/utils/test_pydantic_utils.py
💤 Files with no reviewable changes (12)
- docs/data-sources/builtin-toolsets/splunk-mcp.md
- tests/core/test_tool_output_deduplication.py
- holmes/plugins/toolsets/inspektor_gadget.yaml
- holmes/plugins/toolsets/datadog/toolset_datadog_logs.py
- tests/utils/test_pydantic_utils.py
- holmes/config.py
- holmes/plugins/toolsets/datadog/toolset_datadog_traces.py
- holmes_operator/scheduler/manager.py
- holmes/common/cli_commons.py
- tests/plugins/runbooks/test_catalog.py
- tests/plugins/toolsets/test_logging_api.py
- tests/llm/utils/classifiers.py
✅ Files skipped from review due to trivial changes (58)
- docs/development/evaluations/history/frontier_5_models_20260314_204516.md
- tests/plugins/toolsets/datadog/logs/test_check_prerequisites.py
- docs/development/evaluations/model-comparison-summary-20260315.md
- scripts/cli_performance_benchmark.py
- tests/test_format_tags.py
- tests/llm/utils/reporting/terminal_reporter.py
- holmes/core/json_schema_coerce.py
- holmes/plugins/toolsets/mcp/toolset_mcp.py
- tests/plugins/toolsets/grafana/test_grafana_tempo_api.py
- holmes/core/tools_utils/tool_executor.py
- tests/core/test_tool_memory_limit.py
- tests/test_mcp_toolset.py
- tests/llm/fixtures/test_ask_holmes/50a_logs_since_last_specific_month/test_case.yaml
- tests/plugins/sources/test_pagerduty_source.py
- docs/development/evaluations/history/results_20260127_161120.md
- docs/development/evaluations/history/results_20260122_074259.md
- docs/development/evaluations/history/results_20260311_210836.md
- holmes/plugins/toolsets/datadog/toolset_datadog_general.py
- tests/llm/utils/property_manager.py
- holmes/plugins/toolsets/bash/validation.py
- .claude/skills/create-eval/SKILL.md
- tests/plugins/toolsets/datadog/traces/test_datadog_traces_live.py
- holmes/core/tools_utils/tool_context_window_limiter.py
- holmes/plugins/toolsets/grafana/toolset_grafana.py
- tests/llm/conftest.py
- holmes_operator/scheduler/job_executor.py
- holmes/core/llm_usage.py
- tests/plugins/toolsets/datadog/metrics/test_datadog_metrics_live.py
- tests/plugins/toolsets/test_elasticsearch_mtls.py
- tests/test_http_docs.py
- tests/llm/utils/test_case_utils.py
- tests/plugins/toolsets/test_runbook.py
- docs/development/evaluations/history/results_20260129_094857.md
- examples/custom_llm.py
- holmes/core/openai_formatting.py
- tests/llm/utils/braintrust.py
- holmes/plugins/toolsets/prometheus/prometheus.py
- docs/development/evaluations/history/results_20260315_041151.md
- .claude/skills/create-eval/references/test-case-format.md
- tests/llm/test_ask_holmes.py
- holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py
- tests/llm/utils/reporting/github_reporter.py
- tests/llm/fixtures/test_ask_holmes/231_confluence_large_page_eval/generate_large_page.py
- holmes/plugins/toolsets/coralogix/utils.py
- tests/llm/fixtures/test_ask_holmes/212_large_configmap_needle/generate_configs.py
- tests/test_openai_formatting.py
- holmes/core/tools.py
- tests/plugins/toolsets/grafana/test_grafana_tempo_unit.py
- tests/core/test_llm_api_base_version.py
- holmes/plugins/toolsets/confluence/confluence.py
- tests/llm/fixtures/test_ask_holmes/65_health_check_followup/test_case.yaml
- holmes/plugins/toolsets/init.py
- tests/integration/test_kubernetes_transformer_execution.py
- tests/config_class/test_config_load_cloud_mcp.py
- holmes/plugins/toolsets/http/http_toolset.py
- holmes/plugins/toolsets/database/database.py
- tests/test_tool_calling_llm.py
- .claude/skills/create-eval/references/anti-hallucination.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_server_endpoints.py
- holmes/interactive.py
- holmes/main.py
- holmes/core/truncation/input_context_window_limiter.py
| async def acompletion( | ||
| self, | ||
| messages: List[Dict[str, Any]], | ||
| tools: Optional[List[Dict[str, Any]]] = [], | ||
| tool_choice: Optional[Union[str, dict]] = None, | ||
| response_format: Optional[Union[dict, Type[BaseModel]]] = None, | ||
| temperature: Optional[float] = None, | ||
| drop_params: Optional[bool] = None, | ||
| stream: Optional[bool] = None, | ||
| ) -> Union[ModelResponse, CustomStreamWrapper]: | ||
| """Async version of completion. Subclasses may override for true async. | ||
| Default implementation falls back to sync completion.""" | ||
| return self.completion( | ||
| messages=messages, | ||
| tools=tools, | ||
| tool_choice=tool_choice, | ||
| response_format=response_format, | ||
| temperature=temperature, | ||
| drop_params=drop_params, | ||
| stream=stream, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Mutable default argument for tools parameter.
Using a mutable default ([]) can lead to unexpected behavior if the list is accidentally mutated. Use None and initialize inside the method.
🛠️ Suggested fix
async def acompletion(
self,
messages: List[Dict[str, Any]],
- tools: Optional[List[Dict[str, Any]]] = [],
+ tools: Optional[List[Dict[str, Any]]] = None,
tool_choice: Optional[Union[str, dict]] = None,
response_format: Optional[Union[dict, Type[BaseModel]]] = None,
temperature: Optional[float] = None,
drop_params: Optional[bool] = None,
stream: Optional[bool] = None,
) -> Union[ModelResponse, CustomStreamWrapper]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def acompletion( | |
| self, | |
| messages: List[Dict[str, Any]], | |
| tools: Optional[List[Dict[str, Any]]] = [], | |
| tool_choice: Optional[Union[str, dict]] = None, | |
| response_format: Optional[Union[dict, Type[BaseModel]]] = None, | |
| temperature: Optional[float] = None, | |
| drop_params: Optional[bool] = None, | |
| stream: Optional[bool] = None, | |
| ) -> Union[ModelResponse, CustomStreamWrapper]: | |
| """Async version of completion. Subclasses may override for true async. | |
| Default implementation falls back to sync completion.""" | |
| return self.completion( | |
| messages=messages, | |
| tools=tools, | |
| tool_choice=tool_choice, | |
| response_format=response_format, | |
| temperature=temperature, | |
| drop_params=drop_params, | |
| stream=stream, | |
| ) | |
| async def acompletion( | |
| self, | |
| messages: List[Dict[str, Any]], | |
| tools: Optional[List[Dict[str, Any]]] = None, | |
| tool_choice: Optional[Union[str, dict]] = None, | |
| response_format: Optional[Union[dict, Type[BaseModel]]] = None, | |
| temperature: Optional[float] = None, | |
| drop_params: Optional[bool] = None, | |
| stream: Optional[bool] = None, | |
| ) -> Union[ModelResponse, CustomStreamWrapper]: | |
| """Async version of completion. Subclasses may override for true async. | |
| Default implementation falls back to sync completion.""" | |
| return self.completion( | |
| messages=messages, | |
| tools=tools, | |
| tool_choice=tool_choice, | |
| response_format=response_format, | |
| temperature=temperature, | |
| drop_params=drop_params, | |
| stream=stream, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 240-240: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/core/llm.py` around lines 237 - 258, The acompletion method uses a
mutable default for tools (tools: Optional[List[Dict[str, Any]]] = []); change
the signature to use None (tools: Optional[List[Dict[str, Any]]] = None) and
inside acompletion set tools = tools or [] (or list(tools) if you want a copy)
before calling self.completion, so you never share a single list instance;
update any related parameter handling passed to self.completion to use this
local initialized list and keep
tool_choice/response_format/temperature/drop_params/stream unchanged.
| async def acompletion( | ||
| self, | ||
| messages: List[Dict[str, Any]], | ||
| tools: Optional[List[Dict[str, Any]]] = None, | ||
| tool_choice: Optional[Union[str, dict]] = None, | ||
| response_format: Optional[Union[dict, Type[BaseModel]]] = None, | ||
| temperature: Optional[float] = None, | ||
| drop_params: Optional[bool] = None, | ||
| stream: Optional[bool] = None, | ||
| ) -> Union[ModelResponse, CustomStreamWrapper]: | ||
| kwargs = self._prepare_completion_kwargs( | ||
| messages, | ||
| tools, | ||
| tool_choice, | ||
| response_format, | ||
| temperature, | ||
| drop_params, | ||
| stream, | ||
| ) | ||
| result = await litellm.acompletion(**kwargs) | ||
| return self._validate_completion_result(result) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if the tracer wrapper supports async methods
rg -n "wrap_llm|acompletion" holmes/core/tracing.py -A5 -B2Repository: HolmesGPT/holmesgpt
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Check the completion() method around line 685 to verify the tracer wrapping claim
sed -n '670,690p' holmes/core/llm.py | cat -nRepository: HolmesGPT/holmesgpt
Length of output: 976
🏁 Script executed:
#!/bin/bash
# Get more context on BraintrustTracer.wrap_llm and ChatCompletionWrapper
sed -n '252,280p' holmes/core/tracing.py | cat -nRepository: HolmesGPT/holmesgpt
Length of output: 1306
🏁 Script executed:
#!/bin/bash
# Check if ChatCompletionWrapper supports async methods
rg -n "ChatCompletionWrapper|acompletion" holmes/core/tracing.py -A10Repository: HolmesGPT/holmesgpt
Length of output: 699
🏁 Script executed:
#!/bin/bash
# Check if WrappedLiteLLM has any acompletion method or if there's async tracer support elsewhere
rg -n "acompletion|acreate_fn" holmes/core/tracing.pyRepository: HolmesGPT/holmesgpt
Length of output: 103
Async completion calls lack tracer support.
The synchronous completion() method wraps litellm with the tracer (line 685), but acompletion() calls litellm.acompletion() directly without tracer wrapping. Async LLM calls won't be observed, creating observability gaps.
The tracer's wrap_llm() creates a WrappedLiteLLM wrapper that only overrides completion() and relies on __getattr__ for other methods. The underlying ChatCompletionWrapper is initialized with acreate_fn=None, meaning it has no async support. To fix this, the tracer implementation needs to be enhanced to support async operations, not just the call site.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/core/llm.py` around lines 689 - 709, The async method acompletion
currently calls litellm.acompletion() directly, so async LLM calls are not
traced; update this by using the tracer-wrapped LLM just like completion() does
(i.e., call self.tracer.wrap_llm(self.litellm) or the equivalent wrapped
instance before invoking acompletion), and if needed enhance the tracer
implementation (WrappedLiteLLM / ChatCompletionWrapper) to provide proper async
support by implementing/forwarding acompletion (or initializing
ChatCompletionWrapper with acreate_fn) so the wrapped object actually supports
async operations and the tracer observes async calls.
| raise ValueError( | ||
| f"Invalid JSON for '{param_name}': {e}. Received: {value[:200]}" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Ruff B904 for explicit exception chaining in this file.
ruff check --select B904 holmes/plugins/toolsets/mongodb/mongodb.py
# Show the relevant block for quick inspection.
rg -n -A4 -B2 'except json\.JSONDecodeError as e:' holmes/plugins/toolsets/mongodb/mongodb.pyRepository: HolmesGPT/holmesgpt
Length of output: 722
Add exception chaining to preserve the original error context.
The json.JSONDecodeError is caught at line 34 but re-raised as ValueError without exception chaining, which loses traceback context and triggers Ruff B904.
🔧 Proposed fix
except json.JSONDecodeError as e:
raise ValueError(
f"Invalid JSON for '{param_name}': {e}. Received: {value[:200]}"
- )
+ ) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise ValueError( | |
| f"Invalid JSON for '{param_name}': {e}. Received: {value[:200]}" | |
| ) | |
| except json.JSONDecodeError as e: | |
| raise ValueError( | |
| f"Invalid JSON for '{param_name}': {e}. Received: {value[:200]}" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 35-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/plugins/toolsets/mongodb/mongodb.py` around lines 35 - 37, The
JSONDecodeError caught in the except block that constructs the ValueError for
invalid JSON should preserve the original exception context by using exception
chaining; update the raise in that block to re-raise ValueError(...) from e
(referencing the local variables param_name and value in the same except block
where json.JSONDecodeError is handled) so the original traceback is kept.
| def test_metrics_range_with_no_step_auto_calculates( | ||
| self, tempo_toolset, monkeypatch | ||
| ): |
There was a problem hiding this comment.
Add type hints to the updated test method signatures.
The touched test methods still use untyped parameters and no return annotations; please add explicit fixture types and -> None.
Suggested patch
- def test_metrics_range_with_no_step_auto_calculates(
- self, tempo_toolset, monkeypatch
- ):
+ def test_metrics_range_with_no_step_auto_calculates(
+ self,
+ tempo_toolset: GrafanaTempoToolset,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
@@
- def test_metrics_range_with_small_step_gets_adjusted(
- self, tempo_toolset, monkeypatch
- ):
+ def test_metrics_range_with_small_step_gets_adjusted(
+ self,
+ tempo_toolset: GrafanaTempoToolset,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
@@
- def test_metrics_range_step_adjustment_various_ranges(
- self, tempo_toolset, monkeypatch
- ):
+ def test_metrics_range_step_adjustment_various_ranges(
+ self,
+ tempo_toolset: GrafanaTempoToolset,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:As per coding guidelines, "Type hints required in Python files (mypy configuration in pyproject.toml)."
Also applies to: 585-587, 665-667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/plugins/toolsets/grafana/test_grafana_tempo_tools.py` around lines 554
- 556, Update the test function signatures to include explicit type hints for
fixtures and a return annotation -> None; specifically add typing to
test_metrics_range_with_no_step_auto_calculates(tempo_toolset, monkeypatch) and
the other two affected tests (the ones starting at the places you noted) so
parameters are typed (e.g., tempo_toolset: TempoToolset and monkeypatch:
pytest.MonkeyPatch or appropriate fixture types used in the repo) and append ->
None; also add necessary imports for those types (from pytest and your test
fixtures module or typing) so mypy passes.
Summary
This PR migrates all LLM API calls from synchronous to asynchronous using
litellm.acompletion. This enables better performance when handling multiple requests at the same time.What Changed
Core LLM Layer:
LLM.completion()now usesasync/awaitwithlitellm.acompletionIntegration Points:
/api/chatendpoint is now async (better for concurrent requests)asyncio.run()to wrap async callsAsyncMockfor async methodsBenefits
Backward Compatibility
CLI commands work exactly the same - they wrap async calls with
asyncio.run()so there's no change in behavior for single-command usage.Files Modified
holmes/core/llm.py- Async LLM completionholmes/core/tool_calling_llm.py- Async tool calling with asyncio tasksholmes/core/truncation/- Async context limiting and compactionserver.py- Async FastAPI endpointholmes/main.py,holmes/interactive.py- Async wrappers for CLISummary by CodeRabbit
Release Notes
New Features
Refactor
supports_additional_system_promptflag.Tests