feat: Add experimental AG-UI supported chat endpoint and PPL query assist - #1035
Conversation
|
|
WalkthroughAdds an experimental AG-UI integration: a FastAPI SSE AG-UI server, a React ExampleOps front-end with ChatAssistant and observability pages, OpenSearch PPL query-assist toolset and templates, tests, Holmes config wiring for AG-UI tool executor/tool-calling, Docker copy, and documentation/env files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Front-end ChatAssistant
participant FE as Front-end MainContent
participant AG as AG-UI Server (FastAPI)
participant HL as Holmes (LLM + Tool Executor)
participant DS as Data Sources (Prometheus/OpenSearch)
UI->>AG: POST /api/agui/chat (RunAgentInput) [SSE]
AG-->>UI: RUN_STARTED
AG->>HL: Build runner (messages + toolset)
HL-->>AG: Stream TEXT tokens
AG-->>UI: TEXT_MESSAGE_START / TEXT_MESSAGE_CONTENT / TEXT_MESSAGE_END
alt LLM emits tool call
HL-->>AG: TOOL_CALL_START (execute_promql_query / execute_ppl_query)
AG-->>UI: TOOL_CALL_START (forwarded)
UI->>FE: onExecutePromQLQuery / onExecutePPLQuery
FE->>DS: Query Prometheus/OpenSearch
DS-->>FE: Results
FE-->>UI: TOOL_CALL_END (result)
UI-->>AG: TOOL_CALL_END (ack/result)
end
HL-->>AG: RUN_FINISHED / RUN_ERROR
AG-->>UI: RUN_FINISHED / RUN_ERROR
sequenceDiagram
autonumber
participant UI as ChatAssistant
participant FE as MainContent
participant PR as Prometheus
participant OS as OpenSearch
UI->>FE: onExecutePromQLQuery(query)
FE->>PR: /api/v1/query_range (PromQL)
PR-->>FE: Time-series data
FE-->>UI: GraphData (GraphVisualization)
UI->>FE: onExecutePPLQuery(query)
FE->>OS: _plugins/_ppl (PPL)
OS-->>FE: Logs data
FE-->>UI: LogData (LogsVisualization)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (20)
holmes/config.py (1)
293-304: Add docstring for consistency.The method
create_agui_toolcalling_llmlacks a docstring, while similar methods likecreate_console_toolcalling_llm(line 280-291) include documentation. Adding a docstring would improve consistency and maintainability.Apply this diff to add a docstring:
def create_agui_toolcalling_llm( self, dal: Optional["SupabaseDal"] = None, model: Optional[str] = None, tracer=None, ) -> "ToolCallingLLM": + """ + Creates a ToolCallingLLM instance configured for AG-UI usage. + + Args: + dal: Optional SupabaseDal instance for data access + model: Optional model key to override the default model + tracer: Optional tracer for observability + + Returns: + ToolCallingLLM configured with AG-UI toolsets + """ tool_executor = self.create_agui_tool_executor(dal) from holmes.core.tool_calling_llm import ToolCallingLLM return ToolCallingLLM( tool_executor, self.max_steps, self._get_llm(model, tracer) )tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py (1)
286-315: Schema consistency: query should be string OR object/array, not string-with-items.The test enforces
parameters["query"].type == "string"and also expects nesteditems.properties. That mixes JSON-schema concepts (items belong to arrays). Consider either:
- keep
type: "string"and removeitems; or- change to
type: "object"/"array"and keepitems/properties.This will make tools more interoperable (e.g., UI autogeneration).
Based on learnings
experimental/ag-ui/front-end/src/components/MainContent.tsx (6)
4-4: Avoid duplicating ObservabilityPage type.Import the shared type from App.tsx (or a common types module) to prevent drift.
Apply this diff:
-import GraphVisualization from './GraphVisualization'; -import LogsVisualization from './LogsVisualization'; -type ObservabilityPage = 'metrics' | 'logs' | 'traces'; +import GraphVisualization from './GraphVisualization'; +import LogsVisualization from './LogsVisualization'; +import type { ObservabilityPage } from '../App';
41-43: Remove unused refs.
currentQueryRefandabortControllerRefare defined but never used.Apply this diff:
- const currentQueryRef = React.useRef<string>(''); - const abortControllerRef = React.useRef<AbortController | null>(null);
121-151: Toggle loading state for metrics fetcher.
loadingMetricsis never set, so the spinner never shows. Add set/unset like labels/values.Apply this diff:
const fetchMetricsCount = React.useCallback(async () => { if (selectedPage !== 'metrics' || prometheusStatus !== 'connected') return; - try { + setLoadingMetrics(true); + try { const response = await fetch(`${prometheusUrl}/api/v1/label/__name__/values`, { method: 'GET', signal: AbortSignal.timeout(10000), // 10 second timeout }); @@ } catch (error) { console.error('Error fetching metrics:', error); setAvailableMetrics([]); - } + } finally { + setLoadingMetrics(false); + } }, [prometheusUrl, selectedPage, prometheusStatus]);
92-119: Toggle loading state for indices fetcher.
loadingIndicesis never set; enable it for better UX feedback.Apply this diff:
const fetchIndicesCount = React.useCallback(async () => { if (selectedPage !== 'logs' || opensearchStatus !== 'connected') return; - try { + setLoadingIndices(true); + try { const response = await fetch(`${opensearchUrl}/_cat/indices?format=json&h=index`, { method: 'GET', headers: getOpensearchHeaders(), signal: AbortSignal.timeout(10000), // 10 second timeout }); @@ } catch (error) { console.error('Error fetching indices count:', error); setAvailableIndices([]); - } + } finally { + setLoadingIndices(false); + } }, [opensearchUrl, selectedPage, opensearchStatus, getOpensearchHeaders]);
257-259: Use DOM-safe timeout types in React.
NodeJS.Timeoutcan cause TS friction in browsers. PreferReturnType<typeof setTimeout>.Apply this diff:
- const prometheusRetryTimeoutRef = React.useRef<NodeJS.Timeout | null>(null); - const opensearchRetryTimeoutRef = React.useRef<NodeJS.Timeout | null>(null); + const prometheusRetryTimeoutRef = React.useRef<ReturnType<typeof setTimeout> | null>(null); + const opensearchRetryTimeoutRef = React.useRef<ReturnType<typeof setTimeout> | null>(null); @@ - let safetyTimeout: NodeJS.Timeout | null = null; + let safetyTimeout: ReturnType<typeof setTimeout> | null = null; @@ - let interval: NodeJS.Timeout | null = null; + let interval: ReturnType<typeof setInterval> | null = null;Also applies to: 381-381, 515-515
356-367: IncludeprometheusStatusin callback deps to avoid stale reads.The guard
if (prometheusStatus === 'checking' ...)can use stale value without it in deps.Apply this diff:
- const checkPrometheusConnection = React.useCallback(async (isRetry = false, force = false) => { + const checkPrometheusConnection = React.useCallback(async (isRetry = false, force = false) => { @@ - }, [prometheusUrl, selectedPage]); + }, [prometheusUrl, selectedPage, prometheusStatus]);Also applies to: 435-435
experimental/ag-ui/front-end/src/components/LogsVisualization.tsx (2)
36-46: Support numeric/epoch timestamps too.PPL rows may return epoch seconds/millis. Add number handling for better UX.
Example:
if (type === 'timestamp') { const toDate = (v: any) => typeof v === 'number' ? new Date(v > 1e12 ? v : v * 1000) : new Date(String(v)); const date = toDate(value); if (!isNaN(date.getTime()) && date.getTime() > 0) { return <span className="timestamp-value">{date.toLocaleString()}</span>; } }
198-207: Harden CSV export for quotes/newlines.Escape quotes and wrap fields containing comma, quote, or newline to avoid malformed CSV.
Apply this diff:
- ...reorderedDatarows.map(row => - row.map(cell => - typeof cell === 'string' && cell.includes(',') - ? `"${cell.replace(/"/g, '""')}"` - : String(cell || '') - ).join(',') - ) + ...reorderedDatarows.map(row => + row.map(cell => { + const raw = String(cell ?? ''); + const needsWrap = /[",\n\r]/.test(raw); + const escaped = raw.replace(/"/g, '""'); + return needsWrap ? `"${escaped}"` : escaped; + }).join(',') + )experimental/ag-ui/server.py (3)
83-89: CORS: avoid*withallow_credentials=True.Browsers block
Access-Control-Allow-Credentials: truewith wildcard origins. Either setallow_credentials=Falseor specify explicit origins from config/env.Example:
allowed = os.getenv("AGUI_CORS_ORIGINS", "").split(",") if os.getenv("AGUI_CORS_ORIGINS") else ["http://localhost:3000"] app.add_middleware( CORSMiddleware, allow_origins=allowed, allow_credentials=True, allow_methods=["*"], allow_headers=["*"], )
398-401: Return a proper JSON list for models.Avoid double-encoding with
json.dumps.Apply this diff:
-@app.get("/api/model") -def get_model(): - return {"model_name": json.dumps(config.get_models_list())} +@app.get("/api/model") +def get_model(): + return {"model_names": config.get_models_list()}
197-209: Preserve exception context on HTTP errors.Use
raise ... from eand prefer conversion flags.Apply this diff:
- yield encoder.encode( + yield encoder.encode( RunErrorEvent( type=EventType.RUN_ERROR, - message=f"Agent encountered an error: {str(e)}" + message=f"Agent encountered an error: {e!s}" ) ) if isinstance(e, AuthenticationError): - raise HTTPException(status_code=401, detail=e.message) + raise HTTPException(status_code=401, detail=e.message) from e elif isinstance(e, litellm.exceptions.RateLimitError): - raise HTTPException(status_code=429, detail=e.message) + raise HTTPException(status_code=429, detail=e.message) from e else: - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from eexperimental/ag-ui/front-end/src/components/ChatAssistant.tsx (1)
433-437: Use AbortController for fetch timeouts (timeout option is ignored by browsers).
fetchdoesn’t support atimeoutoption. UseAbortSignal.timeoutor a controller.Apply this diff:
- const response = await fetch(modelUrl, { - method: 'GET', - timeout: 5000 - } as any); + const response = await fetch(modelUrl, { + method: 'GET', + signal: (AbortSignal as any).timeout ? (AbortSignal as any).timeout(5000) : undefined + } as any); @@ - const response = await fetch(modelUrl, { - method: 'GET', - timeout: 5000 - } as any); + const response = await fetch(modelUrl, { + method: 'GET', + signal: (AbortSignal as any).timeout ? (AbortSignal as any).timeout(5000) : undefined + } as any);Also applies to: 449-453
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (3)
1-17: Imports/logger cleanup and type hints.
- Add module logger instead of root logging.
- Import ToolInvokeContext for typing.
- Remove unused imports (uuid4, formatters, Task, TaskStatus, toolset_name_for_one_liner).
-import logging -import os -from typing import Any, Dict -from uuid import uuid4 +import logging +import os +from typing import Any, Dict @@ -from holmes.core.todo_tasks_formatter import format_tasks from holmes.core.tools import ( StructuredToolResult, StructuredToolResultStatus, Tool, ToolParameter, Toolset, ToolsetTag, + ToolInvokeContext, ) -from holmes.plugins.toolsets.investigator.model import Task, TaskStatus -from holmes.plugins.toolsets.utils import toolset_name_for_one_liner + +logger = logging.getLogger(__name__)As per coding guidelines
52-58: Ruff hint: use explicit conversion flag and module logger.
- Prefer
logger.exception(...)overlogging.exception(...).- Replace
str(e)with{e!s}(RUF010).- except Exception as e: - logging.exception(f"error using {self.name} tool") - return StructuredToolResult( + except Exception as e: + logger.exception("error using %s tool", self.name) + return StructuredToolResult( status=StructuredToolResultStatus.ERROR, - error=f"Failed to process tasks: {str(e)}", + error=f"Failed to process query: {e!s}", params=params, )Based on static analysis hints
60-63: Optional: Improve one-liner clarity.Consider quoting the query and omitting the unnecessary parentheses.
- return f"OpenSearchQueryToolset: Query ({query})" + return f'OpenSearchQueryAssist: query="{query}"'holmes/plugins/toolsets/opensearch/opensearch_query_assist_instructions.jinja2 (1)
31-33: Clarity: “CURRENT USE QUERY” → “CURRENT QUERY”.Minor wording correction.
-IMPORTANT: YOU CAN USE THE CURRENT USE QUERY TO HELP ENHANCE/MODIFY/FIX/SUGGEST VALID QUERY USING THE SAME INDEX PATTERN +IMPORTANT: YOU CAN USE THE CURRENT QUERY TO HELP ENHANCE/MODIFY/FIX/SUGGEST A VALID QUERY USING THE SAME INDEX PATTERNexperimental/ag-ui/front-end/src/components/GraphVisualization.tsx (2)
75-84: Remove debug logs; they spam console in production.Drop
console.logcalls. Optionally gate behind a debug flag.- console.log('Metric data for series', index, ':', metric); @@ - console.log('Legend layout check:', { - seriesCount, - maxLabelLength, - hasLongLabels, - sampleLabels: datasets.slice(0, 3).map(d => d.label) - }); @@ - console.log('Should use vertical legend:', shouldBeVertical);Also applies to: 224-229, 241-243
151-212: Optional: memoize chart options for perf.Use
useMemoso options aren’t rebuilt every render.- const chartOptions: ChartOptions<'line'> = { + const chartOptions: ChartOptions<'line'> = React.useMemo(() => ({ responsive: true, maintainAspectRatio: false, @@ - }; + }), []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
experimental/ag-ui/front-end/public/holmesgpt-logo.pngis excluded by!**/*.pngexperimental/ag-ui/front-end/resources/holmesgpt-logo.pngis excluded by!**/*.pngexperimental/ag-ui/front-end/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
experimental/ag-ui/README.md(1 hunks)experimental/ag-ui/front-end/.env.example(1 hunks)experimental/ag-ui/front-end/.gitignore(1 hunks)experimental/ag-ui/front-end/.nvmrc(1 hunks)experimental/ag-ui/front-end/README.md(1 hunks)experimental/ag-ui/front-end/package.json(1 hunks)experimental/ag-ui/front-end/public/index.html(1 hunks)experimental/ag-ui/front-end/src/App.css(1 hunks)experimental/ag-ui/front-end/src/App.tsx(1 hunks)experimental/ag-ui/front-end/src/components/ChatAssistant.css(1 hunks)experimental/ag-ui/front-end/src/components/ChatAssistant.tsx(1 hunks)experimental/ag-ui/front-end/src/components/ErrorBoundary.tsx(1 hunks)experimental/ag-ui/front-end/src/components/GraphVisualization.css(1 hunks)experimental/ag-ui/front-end/src/components/GraphVisualization.tsx(1 hunks)experimental/ag-ui/front-end/src/components/LogsVisualization.css(1 hunks)experimental/ag-ui/front-end/src/components/LogsVisualization.tsx(1 hunks)experimental/ag-ui/front-end/src/components/MainContent.tsx(1 hunks)experimental/ag-ui/front-end/src/index.css(1 hunks)experimental/ag-ui/front-end/src/index.tsx(1 hunks)experimental/ag-ui/front-end/tsconfig.json(1 hunks)experimental/ag-ui/server.py(1 hunks)holmes/config.py(2 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_query_assist.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_query_assist_instructions.jinja2(1 hunks)pyproject.toml(1 hunks)tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2holmes/plugins/toolsets/opensearch/opensearch_query_assist.pyholmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/opensearch/opensearch_query_assist_instructions.jinja2
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/opensearch/opensearch_query_assist.pyholmes/plugins/toolsets/__init__.pyexperimental/ag-ui/server.pytests/plugins/toolsets/opensearch/test_opensearch_query_assist.pyholmes/config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers/tags
Files:
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
🧬 Code graph analysis (6)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (4)
holmes/core/todo_tasks_formatter.py (1)
format_tasks(6-51)holmes/core/tools.py (7)
StructuredToolResult(79-103)StructuredToolResultStatus(52-76)Tool(173-365)ToolParameter(155-161)Toolset(534-768)ToolsetTag(143-146)_load_llm_instructions(763-768)holmes/plugins/toolsets/investigator/model.py (2)
Task(12-15)TaskStatus(6-9)holmes/plugins/toolsets/utils.py (1)
toolset_name_for_one_liner(232-236)
holmes/plugins/toolsets/__init__.py (1)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (1)
OpenSearchQueryAssistToolset(65-86)
experimental/ag-ui/front-end/src/components/MainContent.tsx (1)
experimental/ag-ui/front-end/src/App.tsx (1)
ObservabilityPage(7-7)
experimental/ag-ui/server.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (5)
Config(46-505)load_from_env(168-201)dal(117-120)create_agui_toolcalling_llm(293-304)get_models_list(501-505)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py (2)
holmes/core/tools.py (5)
StructuredToolResult(79-103)StructuredToolResultStatus(52-76)ToolParameter(155-161)ToolsetTag(143-146)Toolset(534-768)holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (6)
PplQueryAssistTool(19-62)OpenSearchQueryAssistToolset(65-86)_invoke(38-58)get_parameterized_one_liner(60-62)get_example_config(79-80)_reload_instructions(82-86)
holmes/config.py (3)
holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(17-76)holmes/core/toolset_manager.py (1)
list_console_toolsets(317-332)holmes/core/tool_calling_llm.py (1)
ToolCallingLLM(260-1109)
🪛 Biome (2.1.2)
experimental/ag-ui/front-end/src/App.css
[error] 596-596: Expected a qualified rule, or an at rule but instead found '/'.
Expected a qualified rule, or an at rule here.
(parse)
[error] 597-597: expected , but instead found /
Remove /
(parse)
[error] 1197-1197: Expected a qualified rule, or an at rule but instead found '/'.
Expected a qualified rule, or an at rule here.
(parse)
[error] 1198-1198: expected , but instead found /
Remove /
(parse)
[error] 1408-1408: Expected a qualified rule, or an at rule but instead found '/'.
Expected a qualified rule, or an at rule here.
(parse)
[error] 1409-1409: expected , but instead found /
Remove /
(parse)
[error] 597-597: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 597-597: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 597-597: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 597-597: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 1198-1198: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 1198-1198: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 1198-1198: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 1409-1409: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 1409-1409: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 1409-1409: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 1409-1409: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 1409-1409: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
🪛 dotenv-linter (3.3.0)
experimental/ag-ui/front-end/.env.example
[warning] 4-4: [UnorderedKey] The HOLMES_PORT key should go before the PORT key
(UnorderedKey)
[warning] 12-12: [UnorderedKey] The REACT_APP_OPENSEARCH_PASSWORD key should go before the REACT_APP_OPENSEARCH_URL key
(UnorderedKey)
🪛 Ruff (0.13.3)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py
22-36: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
39-39: Unused method argument: user_approved
(ARG002)
56-56: Use explicit conversion flag
Replace with conversion flag
(RUF010)
experimental/ag-ui/server.py
1-1: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
93-93: Unused function argument: request
(ARG001)
98-98: Redefinition of unused agui_chat from line 93
(F811)
201-201: Use explicit conversion flag
Replace with conversion flag
(RUF010)
205-205: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
207-207: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
209-209: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
277-277: f-string without any placeholders
Remove extraneous f prefix
(F541)
283-283: Local variable title is assigned to but never used
Remove assignment to unused variable title
(F841)
294-299: Consider moving this statement to an else block
(TRY300)
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
268-268: Local variable current_file is assigned to but never used
Remove assignment to unused variable current_file
(F841)
🔇 Additional comments (5)
holmes/plugins/toolsets/__init__.py (2)
47-47: LGTM!The import follows the established pattern for other toolset imports and is correctly placed at the module level as required by the coding guidelines.
97-97: LGTM!The toolset is correctly instantiated and added to the list, following the same pattern as other OpenSearch-related toolsets.
experimental/ag-ui/front-end/package.json (1)
6-7: No action needed: @Ag-ui packages are published on npm. Both @ag-ui/client@0.0.38 and @ag-ui/core@0.0.38 exist and are accessible.experimental/ag-ui/front-end/src/App.tsx (1)
38-46: ...Also applies to: 60-68, 101-111, 123-131
experimental/ag-ui/front-end/src/components/ErrorBoundary.tsx (1)
13-50: Solid error boundary implementation.State handling, fallback UI, and reset flow look good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
experimental/ag-ui/README.md (5)
9-14: Fix heading level sequencing.
### Known Limitationsjumps straight from the H1 title to an H3, tripping MD001 and making the outline harder to scan. Promote it to an H2 so heading levels increase one at a time.
20-21: Adjust nested list indentation.The sub-bullet under “Data Sources” is indented three spaces, so markdownlint flags MD007. Remove one space (two-space indent total) to keep consistent list formatting.
25-29: Specify language for the shell snippet.The fenced block lacks a language tag, triggering MD040. Add
bash(orshell) after the opening backticks so the commands render with syntax highlighting.
44-55: Add language annotation to the.envexample.This code fence also needs a language identifier for MD040 compliance. Use something like
inito highlight the key/value pairs.
56-59: Tag the frontend command block with a language.Same MD040 issue here—add
bashto the opening backticks for the npm commands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experimental/ag-ui/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
experimental/ag-ui/README.md
9-9: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
* chore: update README
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
experimental/ag-ui/server.py (4)
1-15: Past issues remain unresolved: import order and env var name.The previous review identified critical issues in the certificate initialization block that have not been addressed:
- Line 1: Unused
noqadirective should be removed- Line 9: Environment variable name mismatch (
CERTIFICATEvs.ADDITIONAL_CERTIFICATE)- Lines 4-5: Networked imports before certificate setup can initialize clients that don't trust the custom certificate
92-94: Past issue remains unresolved: function name collision.The health endpoint defines a function named
agui_chatwhich conflicts with the POST endpoint handler on line 98. This causes F811 (function redefinition) and therequestparameter is unused.
187-191: Past issue remains unresolved: f-string syntax error.The f-string on line 189 has nested double quotes causing a syntax error, and the code unsafely assumes the result structure without type checking.
276-306: Past issues remain unresolved: timeseries parsing and unused variable.The previous review identified issues in
_parse_timeseries_datathat have not been addressed:
- Line 280: Assumes
result_data["data"]is a JSON string without checking type- Lines 284, 290:
titlevariable is computed but never used in the return statement (line 302 usesdescriptioninstead)- Line 284: Unnecessary f-string prefix
🧹 Nitpick comments (4)
experimental/ag-ui/README.md (1)
9-9: Optional: Improve markdown formatting for consistency.The markdown linter suggests a few formatting improvements:
- Line 9: Use
##instead of###(heading levels should increment by one)- Line 20: Adjust list indentation to 2 spaces
- Lines 25 and 47: Add language identifiers to code blocks (e.g., ````bash`)
Also applies to: 20-20, 25-29, 47-58
experimental/ag-ui/server.py (3)
197-210: Improve exception handling: add cause chain and explicit conversion.The exception handling lacks proper chaining, making debugging harder. Also, line 202 should use an explicit conversion flag.
Apply this diff:
except Exception as e: logging.error(f"Error in /api/agui/chat: {e}", exc_info=True) yield encoder.encode( RunErrorEvent( type=EventType.RUN_ERROR, - message=f"Agent encountered an error: {str(e)}" + message=f"Agent encountered an error: {e!s}" ) ) if isinstance(e, AuthenticationError): - raise HTTPException(status_code=401, detail=e.message) + raise HTTPException(status_code=401, detail=e.message) from e elif isinstance(e, litellm.exceptions.RateLimitError): - raise HTTPException(status_code=429, detail=e.message) + raise HTTPException(status_code=429, detail=e.message) from e else: - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from e
228-235: Add None check for robustness.
_should_execute_suggested_queryiterates overfrontend_toolswithout checking if it's None, which could raise a TypeError.Apply this diff:
def _should_execute_suggested_query(backend_tool_name: str, frontend_tools: list) -> bool: + if not frontend_tools: + return False for fe_tool_name in frontend_tools: if "execute_prometheus" in fe_tool_name and backend_tool_name in ( "execute_prometheus_range_query", "execute_prometheus_instant_query"):
170-186: Consider configuration-driven tool mapping.The backend-to-frontend tool name mapping is hard-coded with string literals. If backend tool names evolve or new query tools are added, this code will require manual updates.
Consider externalizing this mapping to a configuration dict or registry:
BACKEND_TO_FRONTEND_TOOL_MAP = { "opensearch_ppl_query_assist": "execute_ppl_query", "execute_prometheus_range_query": "execute_promql_query", "execute_prometheus_instant_query": "execute_promql_query", } # Then use: front_end_query_tool = BACKEND_TO_FRONTEND_TOOL_MAP.get(tool_name) if front_end_query_tool: # invoke toolThis aligns with the TODO on line 164 about automating front-end tool discovery.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
experimental/ag-ui/README.md(1 hunks)experimental/ag-ui/server.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server.py
🧬 Code graph analysis (1)
experimental/ag-ui/server.py (5)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (3)
Config(46-505)dal(117-120)create_agui_toolcalling_llm(293-304)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
🪛 markdownlint-cli2 (0.18.1)
experimental/ag-ui/README.md
9-9: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
20-20: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.13.3)
experimental/ag-ui/server.py
1-1: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
93-93: Unused function argument: request
(ARG001)
98-98: Redefinition of unused agui_chat from line 93
(F811)
202-202: Use explicit conversion flag
Replace with conversion flag
(RUF010)
206-206: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
208-208: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
210-210: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
284-284: f-string without any placeholders
Remove extraneous f prefix
(F541)
290-290: Local variable title is assigned to but never used
Remove assignment to unused variable title
(F841)
301-306: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (1)
experimental/ag-ui/server.py (1)
83-89: Verify CORS configuration for production readiness.The CORS middleware allows all origins (
"*") with credentials enabled. While this may be acceptable for local development and experimental features, it poses security risks if deployed beyond localhost.Confirm this is intentionally permissive for the experimental AG-UI demonstration, or consider restricting
allow_originsto specific domains if this module will be exposed to broader networks.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
experimental/ag-ui/server.py (8)
5-8: Fix environment variable name mismatch.The code reads
os.environ.get("CERTIFICATE")but should useADDITIONAL_CERTIFICATEto match the variable name and documentation conventions.Apply this diff:
-ADDITIONAL_CERTIFICATE: str = os.environ.get("CERTIFICATE", "") +ADDITIONAL_CERTIFICATE: str = os.environ.get("ADDITIONAL_CERTIFICATE", "")
16-16: Remove unused import.
update_holmes_status_in_dbis imported but never used in this module.Apply this diff:
-from holmes.utils.holmes_status import update_holmes_status_in_db import logging
36-36: Remove unused import.
holmes_sync_toolsets_statusis imported but never used in this module.Apply this diff:
from holmes.core.models import ( ChatRequest, ) -from holmes.utils.holmes_sync_toolsets import holmes_sync_toolsets_status from fastapi.middleware.cors import CORSMiddleware
93-96: Remove unused parameter.The
requestparameter is declared but never used in the health endpoint.Apply this diff:
@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): +def agui_chat_health(): return JSONResponse(content="ok")
188-192: Fix f-string quoting and handle non-string data safely.The f-string at line 190 contains nested quotes that break syntax, and the code assumes
result['data']is a string without validation.Apply this diff:
if not front_end_tool_invoked: + # Build a safe snippet from result->data + result_obj = chunk.data.get('result', {}) + data_field = result_obj.get('data', '') + if not isinstance(data_field, str): + try: + data_field = json.dumps(data_field) + except Exception: + data_field = str(data_field) + snippet = data_field[:200] async for event in _stream_agui_text_message_event( - message=f"🔧 {tool_name} result:\n{chunk.data.get('result', {}).get('data', '')[0:200]}..." + message=f"🔧 {tool_name} result:\n{snippet}..."): - ): yield encoder.encode(event)
220-227: Remove unused function.
_remove_empty_user_messagesis defined but never invoked anywhere in the module.If this function should sanitize incoming messages, integrate it into
_agui_input_to_holmes_chat_requestwhere messages are processed. Otherwise, remove it.
278-308: Improve data parsing robustness and fix unused variable.The code assumes
result_data["data"]is always a JSON string (line 282) but it may already be a dict. Additionally,titleis computed at line 286-292 but the return statement usesdescriptioninstead (line 304).Apply this diff:
# Handle different Prometheus response formats prometheus_data = result_data result_type = "unknown" if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") + raw = result_data["data"] + try: + parsed = json.loads(raw) if isinstance(raw, str) else raw + except Exception: + parsed = {} + prometheus_data = parsed.get("data", parsed) result_type = prometheus_data.get("resultType", "unknown") # Generate a meaningful title - title = f"Prometheus Query Results" + title = "Prometheus Query Results" if query: # Truncate long queries for display display_query = query if len(query) <= 50 else query[:47] + "..." title = f"Prometheus: {display_query}" elif tool_name: title = f"{tool_name} Results" # Prepare metadata metadata = { "timestamp": int(time.time()), "source": "Prometheus", "result_type": result_type, "description": description, "query": query } return { - "title": description, + "title": title if title else (description or "Prometheus Query Results"), "query": query, "data": prometheus_data, "metadata": metadata }
362-363: Add bounds check to prevent IndexError.
_is_tool_result_messageaccessesinput_data.messages[-1]without verifying the list is non-empty, which will raise anIndexErrorif called with no messages.Apply this diff:
def _is_tool_result_message(input_data: RunAgentInput) -> bool: - return input_data.messages[-1].role == "tool" + return len(input_data.messages) > 0 and input_data.messages[-1].role == "tool"holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2 (1)
74-79: This issue was previously flagged and remains unaddressed.The HTTP request example is still malformed:
- Missing space between HTTP verb and endpoint (
PUT_pluginsshould bePUT _plugins)- JSON body concatenated on the same line instead of appearing on separate line(s)
Apply the previously suggested fix:
-PUT_plugins/_query/settings{"transient":{"plugins":{"ppl":{"enabled":"false"}}}} +PUT _plugins/_query/settings +{"transient":{"plugins":{"ppl":{"enabled":"false"}}}}
🧹 Nitpick comments (4)
experimental/ag-ui/README.md (2)
29-33: Add language identifier to code block.Specify
bashas the language for proper syntax highlighting.Apply this diff:
-``` +```bash git clone git@github.com:open-telemetry/opentelemetry-demo.git cd opentelemetry-demo docker compose up -d--- `55-66`: **Add language identifier to code block.** Specify the language (e.g., `env` or `bash`) for proper syntax highlighting. Apply this diff: ```diff -``` +```env # AG-UI Agent Configuration HOLMES_PORT=5050 AGENT_URL=http://localhost:${HOLMES_PORT}holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2 (2)
95-1616: Consider reducing placeholder code duplication.The generic PPL example
source=my_index | where field1 > 10 | fields field1, field2appears in approximately 100+ snippets throughout this template, making the file unnecessarily large (1616 lines) with limited instructional variety.While consistent examples across documentation pages may be intentional, consider:
- Consolidating redundant snippets into a single comprehensive PPL syntax reference
- Providing diverse examples that demonstrate different PPL commands (stats, dedup, join, eval, etc.) rather than repeating the same basic where/fields pattern
- Prioritizing the unique, detailed examples (like the subsearch examples at lines 354-381, 1365-1396) that genuinely illustrate advanced PPL features
This would improve the signal-to-noise ratio for LLM context consumption and make maintenance easier.
1-1616: Consider standardizing language identifiers.The LANGUAGE field uses various identifiers inconsistently:
- "PPL" (line 9)
- "ppl" (line 48)
- "OpenSearch PPL" (line 100)
- "OpenSearch API" (line 796)
- "OpenSearch Dashboards" (line 1292)
While this variation may intentionally reflect different contexts (CLI vs API vs Dashboard), standardizing to a minimal set (e.g., "PPL" for queries, "JSON" for API requests, "bash" for shell) would improve consistency and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
experimental/ag-ui/README.md(1 hunks)experimental/ag-ui/front-end/src/App.css(1 hunks)experimental/ag-ui/front-end/src/components/GraphVisualization.css(1 hunks)experimental/ag-ui/server.py(1 hunks)holmes/config.py(3 hunks)holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_query_assist_instructions.jinja2(1 hunks)tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
- holmes/plugins/toolsets/opensearch/opensearch_query_assist_instructions.jinja2
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/config.pyexperimental/ag-ui/server.py
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2
🧬 Code graph analysis (2)
holmes/config.py (3)
holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(17-76)holmes/core/toolset_manager.py (1)
list_console_toolsets(317-332)holmes/core/tool_calling_llm.py (1)
ToolCallingLLM(260-1109)
experimental/ag-ui/server.py (5)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (5)
Config(46-506)load_from_env(169-202)dal(118-121)create_agui_toolcalling_llm(294-305)get_models_list(502-506)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
🪛 markdownlint-cli2 (0.18.1)
experimental/ag-ui/README.md
9-9: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.13.3)
experimental/ag-ui/server.py
94-94: Unused function argument: request
(ARG001)
204-204: Use explicit conversion flag
Replace with conversion flag
(RUF010)
208-208: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
210-210: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
212-212: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
286-286: f-string without any placeholders
Remove extraneous f prefix
(F541)
292-292: Local variable title is assigned to but never used
Remove assignment to unused variable title
(F841)
303-308: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (3)
holmes/config.py (3)
98-98: LGTM!The separate cache field
_agui_tool_executorcorrectly resolves the cache collision issue identified in previous reviews, ensuring AG-UI and server executors are cached independently.
246-261: LGTM!The AG-UI tool executor factory follows the established pattern from
create_console_tool_executorand correctly uses the dedicated cache field. Therefresh_status=Trueparameter ensures toolset status is current for AG-UI endpoints.
294-305: LGTM!The AG-UI toolcalling LLM factory correctly delegates to
create_agui_tool_executorand follows the pattern established bycreate_console_toolcalling_llm.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (8)
holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2 (1)
77-77: Formatting improvement: Consider separating JSON bodies onto new lines for REST examples.The major issue (missing space between HTTP verb and endpoint) has been addressed. However, for improved readability and adherence to REST API documentation conventions, consider placing JSON request bodies on separate lines in the HTTP examples at lines 77, 261, and 405.
Example for line 77:
-PUT _plugins/_query/settings{"transient":{"plugins":{"ppl":{"enabled":"false"}}}} +PUT _plugins/_query/settings +{"transient":{"plugins":{"ppl":{"enabled":"false"}}}}Also applies to: 261-261, 405-405
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (2)
38-40: CRITICAL: Fix _invoke signature to match Tool API.The
_invokesignature is incompatible with the Tool API contract.Tool.invoke(see holmes/core/tools.py lines 212-235) calls_invoke(params=..., context=...), but this implementation acceptsuser_approvedinstead, which will raise aTypeErrorat runtime.Apply this diff:
+from holmes.core.tools import ( + StructuredToolResult, + StructuredToolResultStatus, + Tool, + ToolInvokeContext, + ToolParameter, + Toolset, + ToolsetTag, +) + class PplQueryAssistTool(Tool): # ... def _invoke( - self, params: dict, user_approved: bool = False + self, params: dict, context: ToolInvokeContext ) -> StructuredToolResult:As per coding guidelines
22-36: Fix parameter schema inconsistencies and mutable default.Multiple issues with the parameter definition:
Schema inconsistency:
queryis declared astype="string"but includesitemsand nestedproperties(id, content, status), which are only valid for arrays or objects. For a simple string parameter, remove theitemsfield.Copy-paste artifacts: The nested properties (id, content, status) match the Task model structure from the investigator toolset, not a PPL query string. This confirms these are leftover from copying another tool.
Mutable default: The parameters dict should use
Field(default_factory=...)to avoid sharing mutable defaults across instances (RUF012).Apply this diff:
+from pydantic import Field + class PplQueryAssistTool(Tool): name: str = "opensearch_ppl_query_assist" description: str = "Generate valid OpenSearch Piped Processing Language (PPL) queries to suggest to users for execution" - parameters: Dict[str, ToolParameter] = { - "query": ToolParameter( - description="valid OpenSearch Piped Processing Language (PPL) query to suggest to users for execution", - type="string", - required=True, - items=ToolParameter( - type="object", - properties={ - "id": ToolParameter(type="string", required=True), - "content": ToolParameter(type="string", required=True), - "status": ToolParameter(type="string", required=True), - }, - ), - ), - } + parameters: Dict[str, ToolParameter] = Field( + default_factory=lambda: { + "query": ToolParameter( + description="Valid OpenSearch Piped Processing Language (PPL) query to suggest to users for execution", + type="string", + required=True, + ), + } + )As per coding guidelines
experimental/ag-ui/server.py (5)
16-16: Remove unused imports.Lines 16 and 36 import
update_holmes_status_in_dbandholmes_sync_toolsets_statusbut neither is used in this module.Also applies to: 36-36
188-192: Fix f-string syntax and handle non-string result data.Line 190 has a syntax error: double quotes inside an f-string that uses double quotes. Also,
chunk.data.get('result', {}).get('data', '')may not return a string.The previous review comment on lines 188-193 provided a detailed fix for this issue.
207-212: Chain exceptions to preserve traceback.When re-raising exceptions inside an
exceptblock, useraise ... from eto maintain the exception chain and traceback information.This was previously flagged in the review of lines 207-212.
268-289: Robust timeseries parsing and correct title usage.Line 272 assumes
result_data["data"]is a JSON string, but it may already be a dict. Also, thetitlevariable computed at line 277 (wait, I don't see it in current code - let me recheck)... Actually looking at the code, line 285 usesdescriptionas title, which is fine. But line 272 could fail if data is already parsed.The previous review comment on lines 268-289 addresses the JSON parsing issue.
94-94: Remove unused parameter.The
requestparameter is not used in this handler.Apply this diff:
@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): +def agui_chat_health(): return JSONResponse(content="ok")
🧹 Nitpick comments (7)
experimental/ag-ui/front-end/src/components/MainContent.tsx (6)
4-4: Import ObservabilityPage type instead of duplicating.The
ObservabilityPagetype is defined inApp.tsx(line 6 per relevant snippets) and should be imported rather than redeclared here to maintain a single source of truth.Apply this diff:
import React, { useState } from 'react'; import GraphVisualization from './GraphVisualization'; import LogsVisualization from './LogsVisualization'; -type ObservabilityPage = 'metrics' | 'logs' | 'traces'; +import { ObservabilityPage } from '../App';
42-42: Remove unused abortControllerRef.The
abortControllerRefis declared but never used. The query functions useAbortSignal.timeout()directly instead of leveraging this ref for cancellation.If you don't plan to implement manual query cancellation, remove this line:
- const abortControllerRef = React.useRef<AbortController | null>(null);
159-159: Consider making the series limit configurable.The hardcoded
limit=1000on the Prometheus/seriesendpoint may be insufficient for large deployments with many label combinations. Consider making this configurable or implementing pagination.
356-435: Simplify connection checking to avoid race conditions.The
checkPrometheusConnectionlogic is complex with multiple safety mechanisms (concurrent check prevention, safety timeouts, stuck-state monitoring) that suggest underlying race condition concerns. IncludingprometheusStatusin the dependency array (line 435) while also setting it creates potential for unexpected behavior.Consider:
- Removing
prometheusStatusfrom dependencies and using a ref to track check state- Simplifying the retry logic
- Removing the monitoring useEffect (lines 494-503) which is a workaround for complexity
Example refactor using a ref for check state:
const checkInProgressRef = React.useRef(false); const checkPrometheusConnection = React.useCallback(async (isRetry = false, force = false) => { if (selectedPage !== 'metrics') { setPrometheusStatus('connected'); return; } if (checkInProgressRef.current && !force) { return; } checkInProgressRef.current = true; // ... rest of logic try { // ... fetch logic } finally { checkInProgressRef.current = false; } }, [prometheusUrl, selectedPage]); // Remove prometheusStatus from deps
570-572: Consider making time range and step configurable.The hardcoded 1-hour time range and 1-minute step may not suit all use cases. Consider making these configurable via props or UI controls to allow users to adjust the query window and granularity.
1168-1232: Extract data type detection logic to reduce duplication.The data type detection logic (lines 1168-1232) is duplicated in the maximized modal section (lines 1270-1313). This increases maintenance burden and the risk of inconsistencies.
Consider extracting this into a helper function:
const detectVisualizationType = (data: any): 'logs' | 'metrics' | 'unsupported' => { if ((data.schema && data.datarows) || (data.data && data.data.schema && data.data.datarows)) { return 'logs'; } if ((data.result !== undefined) || (data.data && data.data.result !== undefined)) { return 'metrics'; } return 'unsupported'; }; const structureData = (data: any, type: 'logs' | 'metrics', query: string, page: string) => { if (data.title) return data; return { title: type === 'logs' ? 'Logs Visualization' : 'Metrics Visualization', query, data, metadata: { timestamp: Date.now() / 1000, source: type === 'logs' ? 'OpenSearch PPL' : 'Prometheus' } }; }; // Then in JSX: const vizType = detectVisualizationType(currentResult.data); const structuredData = structureData(currentResult.data, vizType, currentResult.query, selectedPage); {vizType === 'logs' && <LogsVisualization data={structuredData} />} {vizType === 'metrics' && <GraphVisualization data={structuredData} />} {vizType === 'unsupported' && <div className="unsupported-data">...</div>}holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (1)
60-62: Consider consistent naming in one-liner output.The one-liner returns "OpenSearchQueryToolset" but the actual toolset name is "opensearch/query_assist" (line 70). For consistency, consider either:
- Using the imported
toolset_name_for_one_linerutility (though this would require keeping that import), or- Simplifying to match the pattern used by other toolsets
Example simplified approach:
def get_parameterized_one_liner(self, params: Dict) -> str: query = params.get("query", "") - return f"OpenSearchQueryToolset: Query ({query})" + return f"PPL Query: {query[:50]}{'...' if len(query) > 50 else ''}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
experimental/ag-ui/front-end/src/components/MainContent.tsx(1 hunks)experimental/ag-ui/server.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_query_assist.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2holmes/plugins/toolsets/opensearch/opensearch_query_assist.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server.pyholmes/plugins/toolsets/opensearch/opensearch_query_assist.py
🧬 Code graph analysis (3)
experimental/ag-ui/server.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (5)
Config(46-506)load_from_env(169-202)dal(118-121)create_agui_toolcalling_llm(294-305)get_models_list(502-506)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
experimental/ag-ui/front-end/src/components/MainContent.tsx (1)
experimental/ag-ui/front-end/src/App.tsx (1)
ObservabilityPage(7-7)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (4)
holmes/core/todo_tasks_formatter.py (1)
format_tasks(6-51)holmes/core/tools.py (7)
StructuredToolResult(79-103)StructuredToolResultStatus(52-76)Tool(173-365)ToolParameter(155-161)Toolset(534-768)ToolsetTag(143-146)_load_llm_instructions(763-768)holmes/plugins/toolsets/investigator/model.py (2)
Task(12-15)TaskStatus(6-9)holmes/plugins/toolsets/utils.py (1)
toolset_name_for_one_liner(232-236)
🪛 Ruff (0.13.3)
experimental/ag-ui/server.py
94-94: Unused function argument: request
(ARG001)
204-204: Use explicit conversion flag
Replace with conversion flag
(RUF010)
208-208: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
210-210: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
212-212: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
284-289: Consider moving this statement to an else block
(TRY300)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py
22-36: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
39-39: Unused method argument: user_approved
(ARG002)
56-56: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (1)
experimental/ag-ui/server.py (1)
343-344: LGTM! Bounds check added.The function now properly checks for non-empty messages before accessing the last element, preventing potential IndexError.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
experimental/ag-ui/server.py (4)
16-16: Remove unused imports (previously flagged).Lines 16 and 36 import
update_holmes_status_in_dbandholmes_sync_toolsets_statusrespectively, but neither is used in this module.Also applies to: 36-36
222-223: Fix PromQL tool name matching (previously flagged).Line 222 checks for
"execute_prometheus"but the frontend uses"execute_promql_query". This mismatch prevents the frontend tool from being invoked.Apply this diff:
- if "execute_prometheus" in fe_tool_name and backend_tool_name in ( + if "execute_promql" in fe_tool_name and backend_tool_name in ( "execute_prometheus_range_query", "execute_prometheus_instant_query"): return True
268-273: Add safe JSON parsing for timeseries data (previously flagged).Line 272 assumes
result_data["data"]is a JSON string without checking. It may already be a dict, causingjson.loads()to fail.Apply this diff:
prometheus_data = result_data result_type = "unknown" if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") + raw = result_data["data"] + try: + parsed = json.loads(raw) if isinstance(raw, str) else raw + except Exception: + parsed = {} + prometheus_data = parsed.get("data", parsed) result_type = prometheus_data.get("resultType", "unknown")
357-359: Avoid mutating input messages (previously flagged).Lines 357-359 assign
msg_tmp = msgthen modifymsg_tmp.role, which mutates the original message ininput_data.messages.Apply this diff:
elif msg.role == "tool": - msg_tmp = msg - msg_tmp.role = "assistant" - non_system_messages.append(msg_tmp) + from copy import copy + msg_copy = copy(msg) + msg_copy.role = "assistant" + non_system_messages.append(msg_copy)
🧹 Nitpick comments (3)
experimental/ag-ui/server.py (3)
94-94: Remove unusedrequestparameter.The
requestparameter is not used in the health check handler.Apply this diff:
@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): +def agui_chat_health(): return JSONResponse(content="ok")
59-59: Add type hints for better type safety.Several functions lack complete type hints, which are required per coding guidelines for mypy compliance:
- Line 59:
init_logging()missing return type-> None- Line 126:
event_generator(message_history)missing param typemessage_history: listand return type- Line 308:
_invoke_front_end_tool(..., tool_call_args: dict)should specifydictcontents or useDict[str, Any], missing return type- Line 325:
_stream_agui_text_message_event(...)missing return typeExample improvements:
def init_logging() -> None: ... async def event_generator(message_history: list) -> AsyncGenerator[bytes, None]: ... async def _invoke_front_end_tool( tool_call_id: str, tool_call_name: str, tool_call_args: Dict[str, Any] ) -> AsyncGenerator[Union[ToolCallStartEvent, ToolCallArgsEvent, ToolCallEndEvent], None]: ... async def _stream_agui_text_message_event( message: str ) -> AsyncGenerator[Union[TextMessageStartEvent, TextMessageContentEvent, TextMessageEndEvent], None]: ...Also applies to: 126-126, 308-308, 325-325
84-90: Consider restricting CORS origins for production use.The current CORS configuration allows all origins (
allow_origins=["*"]), which is acceptable for experimental/demo purposes but should be tightened before production deployment.For production, specify allowed origins explicitly:
app.add_middleware( CORSMiddleware, allow_origins=["https://your-frontend-domain.com"], # Restrict to known origins allow_credentials=True, allow_methods=["*"], allow_headers=["*"], )Or read from configuration:
allowed_origins = os.environ.get("ALLOWED_ORIGINS", "*").split(",") app.add_middleware( CORSMiddleware, allow_origins=allowed_origins, ... )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experimental/ag-ui/server.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server.py
🧬 Code graph analysis (1)
experimental/ag-ui/server.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (5)
Config(46-506)load_from_env(169-202)dal(118-121)create_agui_toolcalling_llm(294-305)get_models_list(502-506)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
🪛 Ruff (0.13.3)
experimental/ag-ui/server.py
94-94: Unused function argument: request
(ARG001)
204-204: Use explicit conversion flag
Replace with conversion flag
(RUF010)
284-289: Consider moving this statement to an else block
(TRY300)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
experimental/ag-ui/server.py (4)
214-221: Fix frontend tool name matching.Line 216 checks for
"execute_prometheus"but the frontend uses"execute_promql_query". This mismatch prevents proper query execution detection.Apply this diff:
def _should_execute_suggested_query(backend_tool_name: str, frontend_tools: list) -> bool: for fe_tool_name in frontend_tools: - if "execute_prometheus" in fe_tool_name and backend_tool_name in ( + if "execute_promql" in fe_tool_name and backend_tool_name in ( "execute_prometheus_range_query", "execute_prometheus_instant_query"): return True elif "execute_ppl" in fe_tool_name and backend_tool_name == "execute_ppl_query": return True return FalseBased on past review comments
347-353: Avoid mutating original input messages.Lines 351-353 assign
msg_tmp = msgthen modifymsg_tmp.role, which mutates the original message object ininput_data.messages. This creates unexpected side effects.Apply this diff:
elif msg.role == "tool": - msg_tmp = msg - msg_tmp.role = "assistant" - non_system_messages.append(msg_tmp) + from copy import copy + msg_copy = copy(msg) + msg_copy.role = "assistant" + non_system_messages.append(msg_copy)Based on past review comments
262-283: Add robust JSON parsing for timeseries data.Line 266 assumes
result_data["data"]is always a JSON string and callsjson.loads()unconditionally. If it's already a dict or other type, this will raise an exception.Apply this diff:
# Handle different Prometheus response formats prometheus_data = result_data result_type = "unknown" if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") - result_type = prometheus_data.get("resultType", "unknown") + raw = result_data["data"] + try: + parsed = json.loads(raw) if isinstance(raw, str) else raw + except Exception: + parsed = {} + prometheus_data = parsed.get("data", parsed) + result_type = prometheus_data.get("resultType", "unknown")Based on past review comments
188-192: Add type safety for data slicing.The code assumes
chunk.data.get('result', {}).get('data', '')returns a string or sliceable type. Ifdatais a dict, list, or other non-string type, the slice operation[0:200]may fail or produce unexpected results.Apply this diff:
if not front_end_tool_invoked: + result_obj = chunk.data.get('result', {}) + data_field = result_obj.get('data', '') + if not isinstance(data_field, str): + try: + data_field = json.dumps(data_field) + except Exception: + data_field = str(data_field) + snippet = data_field[:200] async for event in _stream_agui_text_message_event( - message=f"🔧 {tool_name} result:\n{chunk.data.get('result', {}).get('data', '')[0:200]}..." + message=f"🔧 {tool_name} result:\n{snippet}..." ): yield encoder.encode(event)Based on past review comments
🧹 Nitpick comments (5)
experimental/ag-ui/README.md (2)
29-33: Add language identifier to code block.The fenced code block is missing a language specifier, which prevents proper syntax highlighting and may affect documentation rendering tools.
Apply this diff:
-``` +```bash git clone git@github.com:open-telemetry/opentelemetry-demo.git cd opentelemetry-demo docker compose up -d--- `55-66`: **Add language identifier to code block.** The environment configuration code block is missing a language specifier. Use `bash` or `env` to enable syntax highlighting. Apply this diff: ```diff -``` +```bash # AG-UI Agent Configuration HOLMES_PORT=5050 AGENT_URL=http://localhost:${HOLMES_PORT} # Prometheus Configuration REACT_APP_PROMETHEUS_URL=http://localhost:9090 # OpenSearch Configuration REACT_APP_OPENSEARCH_URL=http://localhost:9200 REACT_APP_OPENSEARCH_USER=user REACT_APP_OPENSEARCH_PASSWORD=pass</blockquote></details> <details> <summary>experimental/ag-ui/server.py (3)</summary><blockquote> `16-16`: **Remove unused import.** `update_holmes_status_in_db` is imported but never used in this module. Apply this diff: ```diff -from holmes.utils.holmes_status import update_holmes_status_in_db import loggingBased on past review comments
36-36: Remove unused import.
holmes_sync_toolsets_statusis imported but never used in this module.Apply this diff:
-from holmes.utils.holmes_sync_toolsets import holmes_sync_toolsets_status from fastapi.middleware.cors import CORSMiddlewareBased on past review comments
93-95: Remove unused parameter.The
requestparameter is not used in the health check handler.Apply this diff:
@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): +def agui_chat_health(): return JSONResponse(content="ok")Based on past review comments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
experimental/ag-ui/README.md(1 hunks)experimental/ag-ui/server.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_query_assist.py(1 hunks)tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server.pyholmes/plugins/toolsets/opensearch/opensearch_query_assist.py
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py
🧬 Code graph analysis (2)
experimental/ag-ui/server.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (5)
Config(46-506)load_from_env(169-202)dal(118-121)create_agui_toolcalling_llm(294-305)get_models_list(502-506)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (1)
holmes/core/tools.py (8)
StructuredToolResult(79-103)StructuredToolResultStatus(52-76)Tool(173-365)ToolParameter(155-161)Toolset(534-768)ToolsetTag(143-146)ToolInvokeContext(164-170)_load_llm_instructions(763-768)
🪛 markdownlint-cli2 (0.18.1)
experimental/ag-ui/README.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.13.3)
experimental/ag-ui/server.py
94-94: Unused function argument: request
(ARG001)
204-204: Use explicit conversion flag
Replace with conversion flag
(RUF010)
278-283: Consider moving this statement to an else block
(TRY300)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py
32-32: Unused method argument: context
(ARG002)
48-48: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (1)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (1)
1-78: LGTM!The toolset implementation correctly follows the Tool API and HolmesGPT toolset patterns:
- Proper import structure and minimal dependencies
- Correct
_invokesignature withcontext: ToolInvokeContextparameter- Appropriate parameter schema (string type for PPL query)
- Clean error handling with proper status and error messages
- Template-based instruction loading via
_reload_instructionsThe unused
contextparameter (static analysis hint on line 32) is normal for simple tools that don't require context—keeping it maintains API consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
experimental/ag-ui/server.py (5)
89-91: Remove unusedrequestparameter.The
requestparameter is not used in this health check handler and should be removed for clarity.Apply this diff:
@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): +def agui_chat_health(): return JSONResponse(content="ok")
210-217: Fix tool name matching for query execution.Two issues in the tool name matching logic:
- Line 212 checks for
"execute_prom"but the frontend uses"execute_promql_query"(visible in the frontend code).- Line 215 checks
backend_tool_name == "execute_ppl_query"but line 172 shows the actual backend tool name is"opensearch_ppl_query_assist".Apply this diff:
def _should_execute_suggested_query(backend_tool_name: str, frontend_tools: list) -> bool: for fe_tool_name in frontend_tools: - if "execute_prom" in fe_tool_name and backend_tool_name in ( + if "execute_promql" in fe_tool_name and backend_tool_name in ( "execute_prometheus_range_query", "execute_prometheus_instant_query"): return True - elif "execute_ppl" in fe_tool_name and backend_tool_name == "execute_ppl_query": + elif "execute_ppl" in fe_tool_name and backend_tool_name == "opensearch_ppl_query_assist": return True return False
184-189: Fix f-string syntax error and add type safety.Line 186 has two critical issues:
- The f-string uses inner double quotes which breaks the syntax:
f"🔧 {tool_name} result:\n{chunk.data.get("result", {}).get("data", "")[0:200]}..."- It assumes
datais a string/list without type checking, which can cause runtime errors if the data is a dict or other type.Apply this diff:
if not front_end_tool_invoked: + # Safely extract and format result data + result_obj = chunk.data.get('result', {}) if hasattr(chunk, 'data') else {} + data_field = result_obj.get('data', '') + if not isinstance(data_field, str): + try: + data_field = json.dumps(data_field) + except Exception: + data_field = str(data_field) + snippet = data_field[:200] async for event in _stream_agui_text_message_event( - message=f"🔧 {tool_name} result:\n{chunk.data.get('result', {}).get('data', '')[0:200]}..." + message=f"🔧 {tool_name} result:\n{snippet}..." ): yield encoder.encode(event)
258-279: Add type safety for timeseries data parsing.Line 262 calls
json.loads(result_data["data"])without checking if the data is already parsed. Ifresult_data["data"]is already a dict, this will raise aTypeError.Apply this diff:
# Handle different Prometheus response formats prometheus_data = result_data result_type = "unknown" if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") + raw = result_data["data"] + try: + parsed = json.loads(raw) if isinstance(raw, str) else raw + except Exception: + parsed = {} + prometheus_data = parsed.get("data", parsed) result_type = prometheus_data.get("resultType", "unknown")
343-349: Avoid mutating input messages.Lines 347-349 assign
msg_tmp = msgthen modifymsg_tmp.role, which mutates the original message object ininput_data.messages. This can cause unexpected side effects for the caller.Apply this diff:
elif msg.role == "tool": - msg_tmp = msg - msg_tmp.role = "assistant" - non_system_messages.append(msg_tmp) + # Create a new message object to avoid mutating the original + from copy import copy + msg_copy = copy(msg) + msg_copy.role = "assistant" + non_system_messages.append(msg_copy)
🧹 Nitpick comments (1)
experimental/ag-ui/server.py (1)
195-202: Consider using conversion flag in f-string.Line 200 uses
str(e)inside the f-string. Python's f-string conversion flags provide a more idiomatic way to achieve this.Apply this diff:
yield encoder.encode( RunErrorEvent( type=EventType.RUN_ERROR, - message=f"Agent encountered an error: {str(e)}" + message=f"Agent encountered an error: {e!s}" ) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
experimental/ag-ui/README.md(1 hunks)experimental/ag-ui/server.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server.py
🧬 Code graph analysis (1)
experimental/ag-ui/server.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (5)
Config(46-506)load_from_env(169-202)dal(118-121)create_agui_toolcalling_llm(294-305)get_models_list(502-506)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
🪛 Ruff (0.13.3)
experimental/ag-ui/server.py
90-90: Unused function argument: request
(ARG001)
200-200: Use explicit conversion flag
Replace with conversion flag
(RUF010)
274-279: Consider moving this statement to an else block
(TRY300)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
experimental/ag-ui/server.py (1)
346-349: Avoid mutating input message objects.Assigning
msg_tmp = msgand then modifyingmsg_tmp.role = "assistant"mutates the original message object ininput_data.messages, which can cause unexpected side effects.Apply this diff to create a proper copy:
elif msg.role == "tool": - msg_tmp = msg - msg_tmp.role = "assistant" - non_system_messages.append(msg_tmp) + from copy import copy + msg_copy = copy(msg) + msg_copy.role = "assistant" + non_system_messages.append(msg_copy)Alternatively, construct a new message object to avoid any shared references.
experimental/ag-ui/front-end/src/components/MainContent.tsx (1)
794-848: Refactor to eliminate code duplication.The triggered query execution logic (lines 800-843) duplicates the query routing and result handling already implemented in
handleExecuteQuery(lines 701-775). This duplication increases maintenance burden and risks inconsistencies between the two code paths.As noted in past review comments, refactor to reuse the existing query execution logic. Wrap
handleExecuteQueryinuseCallbackso it can be safely called from the effect:const handleExecuteQuery = React.useCallback(async () => { if (!query.trim() || isExecuting) return; // ... existing implementation ... }, [query, isExecuting, selectedPage, /* other dependencies */]); React.useEffect(() => { if (triggerQuery && triggerQuery.trim() && triggerQuery !== processedTriggerQuery.current) { processedTriggerQuery.current = triggerQuery; setQuery(triggerQuery); setTimeout(() => { handleExecuteQuery(); if (onQueryTriggered) { onQueryTriggered(); } }, 100); } }, [triggerQuery, selectedPage, onQueryTriggered, handleExecuteQuery]);
🧹 Nitpick comments (6)
experimental/ag-ui/server.py (4)
90-90: Remove unusedrequestparameter.The
requestparameter is declared but never used in the health check handler.As per static analysis hints
Apply this diff:
@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): +def agui_chat_health(): return JSONResponse(content="ok")
186-186: Safely handle non-string result data before slicing.The code assumes
chunk.data.get('result', {}).get('data', '')returns a string or list that can be sliced with[0:200]. Ifdatais a complex object (dict, custom type), this will raise aTypeError.Apply this diff to safely extract and format the result snippet:
if not front_end_tool_invoked: + # Safely extract result data + result_obj = chunk.data.get('result', {}) + data_field = result_obj.get('data', '') + if not isinstance(data_field, str): + try: + data_field = str(data_field) + except Exception: + data_field = '' + snippet = data_field[:200] if data_field else '' async for event in _stream_agui_text_message_event( - message=f"🔧 {tool_name} result:\n{chunk.data.get('result', {}).get('data', '')[0:200]}..." + message=f"🔧 {tool_name} result:\n{snippet}..." ): yield encoder.encode(event)
200-200: Use explicit string conversion flag in f-string.Replace
f"{str(e)}"withf"{e!s}"for cleaner, more idiomatic string conversion.As per static analysis hints
Apply this diff:
yield encoder.encode( RunErrorEvent( type=EventType.RUN_ERROR, - message=f"Agent encountered an error: {str(e)}" + message=f"Agent encountered an error: {e!s}" ) )
232-295: Enhance robustness of timeseries data parsing.At line 262,
json.loads(result_data["data"])assumesresult_data["data"]is a JSON string. If it's already a dict (which is common), this will raise aTypeError. The code should check the type before parsing.Apply this diff to safely handle both string and dict formats:
# Handle different Prometheus response formats prometheus_data = result_data result_type = "unknown" if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") + raw_data = result_data["data"] + try: + parsed = json.loads(raw_data) if isinstance(raw_data, str) else raw_data + except (json.JSONDecodeError, TypeError): + parsed = {} + prometheus_data = parsed.get("data", parsed) if isinstance(parsed, dict) else {} result_type = prometheus_data.get("resultType", "unknown")experimental/ag-ui/front-end/src/components/MainContent.tsx (2)
29-1334: Consider splitting into smaller components.This component is 1336 lines and handles multiple concerns: connection management, query execution, data fetching, three different explorers, visualizations, and modal management. Consider extracting smaller, focused components:
ConnectionStatusBar(lines 895-943)PrometheusSeriesExplorer(lines 945-1051)OpenSearchIndicesExplorer(lines 1053-1102)QueryInputSection(lines 1104-1151)ResultsVisualization(lines 1153-1257)This would improve maintainability, testability, and readability.
1179-1327: Consider extracting duplicated visualization rendering logic.The logic for detecting data type and rendering the appropriate visualization (GraphVisualization vs LogsVisualization) is duplicated between the inline view (lines 1182-1246) and the modal view (lines 1284-1327).
Consider extracting to a helper function:
const renderVisualization = (data: any, query: string) => { const isLogsData = (data.schema && data.datarows) || (data.data && data.data.schema && data.data.datarows); const isMetricsData = (data.result !== undefined) || (data.data && data.data.data.result !== undefined); if (isLogsData) { return ( <LogsVisualization data={data.title ? data : { title: selectedPage === 'logs' ? 'Logs Visualization' : 'Data Visualization', query: query, data: data, metadata: { timestamp: Date.now() / 1000, source: 'OpenSearch PPL' } }} /> ); } else if (isMetricsData) { return ( <GraphVisualization data={data.title ? data : { title: selectedPage === 'metrics' ? 'Metrics Visualization' : 'Data Visualization', query: query, data: data, metadata: { timestamp: Date.now() / 1000, source: 'Prometheus' } }} /> ); } else { return ( <div className="unsupported-data"> <span className="error-icon">⚠️</span> <div className="error-text">Unsupported data format</div> <div className="error-details"> <pre>{JSON.stringify(data, null, 2)}</pre> </div> </div> ); } };Then use
{renderVisualization(currentResult.data, currentResult.query)}in both locations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
experimental/ag-ui/front-end/src/components/MainContent.tsx(1 hunks)experimental/ag-ui/server.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server.py
🧬 Code graph analysis (2)
experimental/ag-ui/front-end/src/components/MainContent.tsx (1)
experimental/ag-ui/front-end/src/App.tsx (1)
ObservabilityPage(7-7)
experimental/ag-ui/server.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (5)
Config(46-506)load_from_env(169-202)dal(118-121)create_agui_toolcalling_llm(294-305)get_models_list(502-506)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
🪛 Ruff (0.13.3)
experimental/ag-ui/server.py
90-90: Unused function argument: request
(ARG001)
200-200: Use explicit conversion flag
Replace with conversion flag
(RUF010)
274-279: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (2)
experimental/ag-ui/server.py (1)
210-217: LGTM: Flexible frontend tool name matching.The substring check
"execute_prom" in fe_tool.nameappropriately matches bothexecute_prometheus_*andexecute_promql_*frontend tool variants, providing good flexibility for tool name variations.experimental/ag-ui/front-end/src/components/MainContent.tsx (1)
553-558: IncludeonQueryUpdatein the effect’s dependency array
onQueryUpdateis stable (wrapped inuseCallback([])), and the existingisUpdatingFromParentref prevents feedback loops.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py (2)
136-150: Remove or rename redundant test.This test is functionally identical to
test_invoke_with_valid_query(lines 39-53) and doesn't verify anything specific about user approval behavior. The test name suggests it should test approval logic, but it simply invokes the tool with default parameters.Consider either:
- Removing this test as duplicate coverage, or
- Renaming it to something like
test_invoke_with_default_behaviorif you want to keep it for clarity
332-396: LGTM! Solid integration test coverage.The integration tests effectively verify:
- Tool and toolset interoperability
- Configuration consistency across components
- Template loading behavior with proper filesystem mocking
Optional enhancement: Consider adding a test that triggers the exception handler in
PplQueryAssistTool._invoke(lines 47-51 in the source). Currently, all tests result in SUCCESS status. You could add a test that mocks an internal failure to ensure the error handling path works correctly.experimental/ag-ui/front-end/src/components/ChatAssistant.tsx (6)
230-355: Reduce duplication in query execution handlers.The
execute_promql_queryandexecute_ppl_queryhandlers (lines 230-292 and 293-355) have nearly identical logic differing only in the callback and message text.Extract a common handler:
const handleQueryExecution = ( toolCallId: string, accumulatedArgsString: string, queryType: 'PromQL' | 'PPL', callback?: (query: string) => void, pageName: string ) => { try { let args: any = {}; if (accumulatedArgsString) { try { args = JSON.parse(accumulatedArgsString); } catch (parseError) { console.warn('Could not parse accumulated args as JSON:', parseError); args = {}; } } console.log(`Execute ${queryType} query parsed args:`, args); if (args.query && callback) { callback(args.query); const successMessage: ChatMessage = { id: 'tool-' + toolCallId, text: `✅ Navigated to ${pageName} page and executed query: \`${args.query}\``, sender: 'assistant', timestamp: new Date() }; setMessages(prev => prev.map(msg => msg.id === 'tool-' + toolCallId ? successMessage : msg ) ); } else { const errorDetails = []; if (!args.query) errorDetails.push('query parameter'); if (!callback) errorDetails.push('callback function'); throw new Error(`Missing: ${errorDetails.join(', ')}. Args: ${JSON.stringify(args)}`); } } catch (error) { console.error(`Error executing ${queryType} query:`, error); const errorMessage: ChatMessage = { id: 'tool-' + toolCallId, text: `❌ Failed to execute ${queryType} query: ${error instanceof Error ? error.message : 'Unknown error'}`, sender: 'assistant', timestamp: new Date() }; setMessages(prev => prev.map(msg => msg.id === 'tool-' + toolCallId ? errorMessage : msg ) ); } };Then replace both blocks with:
} else if (toolCallInfo?.name === 'execute_promql_query') { handleQueryExecution( params.event.toolCallId, accumulatedArgsString, 'PromQL', onExecutePromQLQuery, 'Metrics' ); } else if (toolCallInfo?.name === 'execute_ppl_query') { handleQueryExecution( params.event.toolCallId, accumulatedArgsString, 'PPL', onExecutePPLQuery, 'Logs' ); }
68-68: Extract magic numbers to named constants.Multiple magic numbers throughout the code reduce readability and maintainability. Consider extracting them to named constants.
Add these constants at the top of the component:
const TIMEOUTS = { INITIAL_MESSAGE_DELAY: 100, // Line 68 CONNECTION_CHECK_DELAY: 500, // Line 669 FETCH_TIMEOUT: 5000, // Used in checkConnection and fetchModel RUN_AGENT_TIMEOUT: 120000, // Line 831 MAX_RECONNECT_DELAY: 30000, // Line 592 } as const; const UI_CONSTRAINTS = { MIN_PANEL_WIDTH: 250, // Line 691 MAX_PANEL_WIDTH_RATIO: 0.8, // Line 692 DEFAULT_PANEL_WIDTH: 500, // Line 39 } as const;Then replace the numeric literals with the named constants throughout the code.
Also applies to: 592-592, 669-669, 691-692, 831-831
64-64: Consider conditional logging for production.Extensive console logging throughout the component may expose sensitive information in production and adds noise.
Wrap debug logs in a conditional or create a debug utility:
const DEBUG = process.env.NODE_ENV === 'development'; const debugLog = (...args: any[]) => { if (DEBUG) { console.log(...args); } }; const debugError = (...args: any[]) => { if (DEBUG) { console.error(...args); } };Then replace:
console.log(...)withdebugLog(...)- Keep
console.error(...)for actual errors, or usedebugError(...)for debug-only errorsAlso applies to: 79-79, 96-96, 103-103, 133-133, 171-171, 179-179, 185-185, 199-199, 243-243, 306-306
14-14: Improve type safety for better error detection.Several uses of loose types (
any,Object) reduce TypeScript's ability to catch errors.Consider defining proper types:
interface GraphData { title: string; data: { result?: Array<{ metric: Record<string, string>; values: Array<[number, string]>; }>; }; query?: string; metadata?: Record<string, any>; } interface ChatMessage { id: string; text?: string; sender: 'user' | 'assistant'; timestamp: Date; type?: 'text' | 'graph' | 'error'; graphData?: GraphData; // Instead of 'any' error?: { title: string; description: string; retryable?: boolean; }; } // For the subscriber, use proper AG-UI types if available from the package interface AgentEventSubscriber { onRunStartedEvent?: (params: { event: any }) => void; onRunFinishedEvent?: (params: { event: any }) => void; // ... other event handlers } const subscriber: AgentEventSubscriber = { // ... };Also applies to: 90-90
753-765: Consider input validation for robustness.User input is sent directly to the backend with only whitespace checks. While this may be acceptable for an internal observability tool, consider whether additional validation is needed based on your security requirements.
If the backend doesn't have input validation, you might want to add:
- Maximum length checks
- Character allowlist/blocklist
- Sanitization of special characters if they could cause issues with the backend
Example:
const MAX_MESSAGE_LENGTH = 10000; const sanitizeInput = (input: string): string => { return input.trim().slice(0, MAX_MESSAGE_LENGTH); }; const messageContent = sanitizeInput(userMessage.text || ''); if (messageContent.length === 0) { throw new Error('Cannot send empty message'); }
35-1045: Consider component decomposition for maintainability.This component handles multiple responsibilities: agent lifecycle management, connection management, message handling, UI rendering, and error recovery. For long-term maintainability, consider splitting into smaller components or hooks.
Potential structure:
useAgentConnectionhook: agent initialization, connection status, reconnection logicuseMessageHandlerhook: message sending, receiving, tool call handlingChatMessagescomponent: message rendering logicChatInputcomponent: input field, history navigationChatAssistantcomponent: orchestrates the aboveThis would make each piece easier to test and maintain, though it may be premature optimization for an experimental feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
experimental/ag-ui/front-end/src/components/ChatAssistant.tsx(1 hunks)tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers/tags
Files:
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
🧬 Code graph analysis (1)
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py (2)
holmes/core/tools.py (6)
StructuredToolResult(79-103)StructuredToolResultStatus(52-76)ToolParameter(155-161)ToolsetTag(143-146)ToolInvokeContext(164-170)Toolset(534-768)holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (6)
PplQueryAssistTool(16-54)OpenSearchQueryAssistToolset(57-78)_invoke(32-50)get_parameterized_one_liner(52-54)get_example_config(71-72)_reload_instructions(74-78)
🔇 Additional comments (4)
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py (3)
1-15: LGTM! Imports are correctly placed and organized.All imports are at the top of the file as required by the coding guidelines, and they're well-organized with standard library imports followed by project imports.
214-330: LGTM! Comprehensive toolset testing.The test coverage is thorough:
- Initialization and configuration validation
- Template path construction with proper mocking of filesystem operations
- Inheritance verification
- Tool configuration consistency checks
The use of mocking for path operations (lines 240-273, 275-302) properly isolates the tests from filesystem dependencies.
398-399: LGTM! Standard pytest entry point.The main block allows direct execution of this test file, which is a helpful convenience pattern.
experimental/ag-ui/front-end/src/components/ChatAssistant.tsx (1)
1-34: LGTM!Import structure and type definitions are well-organized and appropriate for the component's functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
experimental/ag-ui/server.py (3)
368-374: Message mutation still present.Lines 372-374 assign
msg_tmp = msgthen mutatemsg_tmp.role, which modifies the original message object ininput_data.messages. This was noted in a previous review comment but remains unaddressed.If this is intentional (e.g., the input messages are not reused), no action needed. Otherwise, create a copy before mutation.
Apply this diff to avoid mutating the original:
elif msg.role == "tool": - msg_tmp = msg - msg_tmp.role = "assistant" - non_system_messages.append(msg_tmp) + from copy import copy + msg_copy = copy(msg) + msg_copy.role = "assistant" + non_system_messages.append(msg_copy)
182-193: Add type safety for result data slicing.Line 188 assumes
chunk.data.get('result', {}).get('data', '')returns a string, but it could be a dict, list, or other type. Direct string slicing[0:200]will raiseTypeErrorfor non-string types.Apply this diff to safely handle different data types:
if not front_end_tool_invoked: # TODO [FUTURE]: Render "TodoWrite" tool_name results prettier. Use code block for now. # Ideally using TOOL_STEP events. if tool_name == "TodoWrite": tool_message = _format_todo_write(data=chunk.data) else: - tool_message = f"🔧 {tool_name} result:\n{chunk.data.get('result', {}).get('data', '')[0:200]}..." + result_obj = chunk.data.get('result', {}) + data_field = result_obj.get('data', '') + if not isinstance(data_field, str): + try: + data_field = json.dumps(data_field) + except Exception: + data_field = str(data_field) + snippet = data_field[:200] + tool_message = f"🔧 {tool_name} result:\n{snippet}..."
283-304: Add type check before JSON parsing.Line 287 calls
json.loads(result_data["data"])without verifying whether the data is a string. If it's already a dict (which is common in tool responses), this will raiseTypeError.Apply this diff to safely handle both string and dict types:
# Handle different Prometheus response formats prometheus_data = result_data result_type = "unknown" if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") + raw_data = result_data["data"] + try: + parsed = json.loads(raw_data) if isinstance(raw_data, str) else raw_data + except (json.JSONDecodeError, TypeError): + parsed = {} + prometheus_data = parsed.get("data", parsed) result_type = prometheus_data.get("resultType", "unknown")
🧹 Nitpick comments (4)
experimental/ag-ui/server.py (4)
80-86: CORS wildcard acceptable for experimental code.The
allow_origins=["*"]setting is appropriate for this experimental AG-UI demo. For production deployments, restrict to specific origins.
89-91: Consider removing unused parameter and standardizing response format.The
requestparameter is unused. For consistency, consider:
- Removing the parameter.
- Returning a structured response:
{"status": "ok"}instead of plain"ok".Apply this diff:
@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): - return JSONResponse(content="ok") +def agui_chat_health(): + return JSONResponse(content={"status": "ok"})
200-207: Optional: Use f-string conversion flag for cleaner code.The static analysis hint suggests using the
!sconversion flag instead of explicitstr()call.Apply this diff:
yield encoder.encode( RunErrorEvent( type=EventType.RUN_ERROR, - message=f"Agent encountered an error: {str(e)}" + message=f"Agent encountered an error: {e!s}" ) )
299-320: Optional: Move return statement to else block.Static analysis suggests restructuring the try/except for better clarity by moving the successful return into an else block.
Apply this diff:
except Exception as e: logging.error(f"Error parsing timeseries data: {e}", exc_info=True) - # Return a fallback structure - return { + else: + return { "title": description, "query": query, "data": prometheus_data, "metadata": metadata } - - except Exception as e: - logging.error(f"Error parsing timeseries data: {e}", exc_info=True) - # Return a fallback structure - return { + + # Fallback for exception case + return { "title": "Prometheus Query Results (Parse Error)", "query": data.get("query", ""), "data": { "result": [] }, "metadata": { "timestamp": int(time.time()), "source": "Prometheus", "error": str(e) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experimental/ag-ui/server.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server.py
🧬 Code graph analysis (1)
experimental/ag-ui/server.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(20-22)StreamEvents(11-17)holmes/config.py (5)
Config(46-506)load_from_env(169-202)dal(118-121)create_agui_toolcalling_llm(294-305)get_models_list(502-506)holmes/core/conversations.py (1)
build_chat_messages(306-398)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(867-1109)
🪛 Ruff (0.13.3)
experimental/ag-ui/server.py
90-90: Unused function argument: request
(ARG001)
205-205: Use explicit conversion flag
Replace with conversion flag
(RUF010)
299-304: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (5)
experimental/ag-ui/server.py (5)
215-232: LGTM! Clean TODO formatting implementation.The status icon mapping and formatting logic is well-structured and readable.
235-242: LGTM! Frontend tool matching logic is correct.The substring checks for
"execute_prom"and"execute_ppl"correctly match the frontend tool naming patterns.
323-355: LGTM! Clean event streaming helper implementations.The
_invoke_front_end_tooland_stream_agui_text_message_eventasync generators correctly yield AG-UI protocol events.
406-408: LGTM! Model endpoint correctly returns list.The double JSON encoding issue was fixed; the endpoint now returns the model list directly for FastAPI to serialize.
411-419: LGTM! Uvicorn configuration is appropriate.The logging configuration and server setup are correct for this experimental server.
|
@kylehounslow can you please fix the poetry lock issue, so we can merge it? |
done! |
Thanks for the review! AG-UI server README covers Prometheus setup here and I've added a link to configuring HolmesGPT on OpenSearch Dashboards here |
Yeah, I saw that Not a must obviously - just will help users see the value faster |
|
@kylehounslow can you please check the pre-commit failures? |
On it. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2 (3)
75-82: Separate JSON body onto new line.The JSON request body is still concatenated on the same line as the HTTP request. This issue was flagged in a previous review but remains unresolved.
Apply this diff:
-PUT _plugins/_query/settings{"transient":{"plugins":{"ppl":{"enabled":"false"}}}} +PUT _plugins/_query/settings +{"transient":{"plugins":{"ppl":{"enabled":"false"}}}}
253-262: Separate JSON body onto new line.The JSON request body is concatenated on the same line as the endpoint. This issue was flagged in a previous review but remains unresolved.
Apply this diff:
-POST _plugins/_ppl/_explain?format=simple{"query":"source=state_country | where country = 'USA' OR country = 'England' | stats count() by country"} +POST _plugins/_ppl/_explain?format=simple +{"query":"source=state_country | where country = 'USA' OR country = 'England' | stats count() by country"}
397-413: Separate JSON body onto new line.The JSON request body is concatenated on the same line as the endpoint. This issue was flagged in a previous review but remains unresolved.
Apply this diff:
-POST _plugins/_ppl/_explain{"query":"source=state_country | where country = 'USA' OR country = 'England' | stats count() by country"} +POST _plugins/_ppl/_explain +{"query":"source=state_country | where country = 'USA' OR country = 'England' | stats count() by country"}experimental/ag-ui/server-agui.py (4)
90-91: Drop unusedrequestparameter to satisfy Ruff.
agui_chat_healthno longer uses the request object, so Ruff (ARG001) still fails. Remove the unused argument so the health check builds cleanly.-@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): - return JSONResponse(content="ok") +@app.get("/api/agui/chat/health") +def agui_chat_health() -> JSONResponse: + return JSONResponse(content="ok")
213-220: Guard tool result formatting against non-string payloads.Slicing
chunk.data.get('result', {}).get('data', '')assumes a string; Prometheus/other tools often return dicts, so this raisesTypeErrorand aborts the stream. Coerce to a string safely before slicing.- if tool_name == "TodoWrite": - tool_message = _format_todo_write(data=chunk.data) - else: - tool_message = f"🔧 {tool_name} result:\n{chunk.data.get('result', {}).get('data', '')[0:200]}..." + if tool_name == "TodoWrite": + tool_message = _format_todo_write(data=chunk.data) + else: + result_payload = chunk.data.get("result", {}) if hasattr(chunk, "data") else {} + data_field = result_payload.get("data", "") + if isinstance(data_field, (dict, list)): + data_snippet = json.dumps(data_field) + else: + data_snippet = str(data_field) + tool_message = f"🔧 {tool_name} result:\n{data_snippet[:200]}..."
321-339: Handle dict payloads when parsing Prometheus timeseries.
json.loads(result_data["data"])blows up when"data"is already a dict (the typical case). That throws, triggers the fallback, and the UI loses charts. Parse conditionally and only call.geton dicts.- if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") - result_type = prometheus_data.get("resultType", "unknown") + if "data" in result_data: + raw_payload = result_data["data"] + try: + parsed_payload = json.loads(raw_payload) if isinstance(raw_payload, str) else raw_payload + except (TypeError, json.JSONDecodeError): + logging.warning("Failed to parse Prometheus data payload", exc_info=True) + parsed_payload = {} + prometheus_data = parsed_payload.get("data", parsed_payload) + if isinstance(prometheus_data, dict): + result_type = prometheus_data.get("resultType", "unknown") + else: + result_type = "unknown"
394-400: Avoid mutating inbound AG‑UI messages.Reassigning
msg_tmp = msgand then changingmsg_tmp.rolemutates the original entry ininput_data.messages, corrupting upstream state. Copy the message before modifying the role.- elif msg.role == "tool": - msg_tmp = msg - msg_tmp.role = "assistant" - non_system_messages.append(msg_tmp) + elif msg.role == "tool": + msg_copy = deepcopy(msg) + msg_copy.role = "assistant" + non_system_messages.append(msg_copy)(remember to add
from copy import deepcopywith the other imports below the certificate guard)experimental/ag-ui/front-end/src/App.css (1)
496-503: Scope the spinner variants so they stop clobbering each otherAll three
.loading-spinnerblocks share the same selector, so the final (14 px white) definition overrides the earlier grey 20 px version that the results placeholder depends on. On a white card the spinner now becomes almost invisible. Please consolidate into a single base rule plus scoped modifiers (e.g.,.results-section .loading-spinner,.show-indices-btn .loading-spinner) or rename the variants so each context keeps its intended size/contrast.Example fix:
-.loading-spinner { - width: 20px; - height: 20px; - border: 2px solid #e9ecef; - border-top: 2px solid #6B7280; - border-radius: 50%; - animation: spin 1s linear infinite; -} - -.loading-spinner { - width: 16px; - height: 16px; - border: 2px solid rgba(255, 255, 255, 0.3); - border-top: 2px solid white; - border-radius: 50%; - animation: spin 1s linear infinite; -} - -.loading-spinner { - width: 14px; - height: 14px; - border: 2px solid rgba(255, 255, 255, 0.3); - border-top: 2px solid white; - border-radius: 50%; - animation: spin 1s linear infinite; -} +.loading-spinner { + width: 20px; + height: 20px; + border: 2px solid #e9ecef; + border-top: 2px solid #6B7280; + border-radius: 50%; + animation: spin 1s linear infinite; +} + +.show-indices-btn .loading-spinner { + width: 16px; + height: 16px; + border-color: rgba(255, 255, 255, 0.3); + border-top-color: #fff; +} + +.indices-discovery-section .loading-spinner { + width: 14px; + height: 14px; + border-color: rgba(255, 255, 255, 0.3); + border-top-color: #fff; +}Also applies to: 984-991, 1162-1169
experimental/ag-ui/front-end/src/components/MainContent.tsx (4)
1-5: Reuse the shared ObservabilityPage type instead of re-declaring it
ObservabilityPagealready lives inApp.tsx. Duplicating the union here risks the two definitions drifting apart. Please import the existing type instead of redefining it.-import React, { useState } from 'react'; -import GraphVisualization from './GraphVisualization'; -import LogsVisualization from './LogsVisualization'; -type ObservabilityPage = 'metrics' | 'logs' | 'traces'; +import React, { useState } from 'react'; +import type { ObservabilityPage } from '../App'; +import GraphVisualization from './GraphVisualization'; +import LogsVisualization from './LogsVisualization';
40-43: Drop the unused abortControllerRef (it fails noUnusedLocals)
abortControllerRefis declared but never read. With the default TypeScript settings (noUnusedLocals: true) this is a build error. Please remove the declaration (and any related code if you bring it back later).- const currentQueryRef = React.useRef<string>(''); - const abortControllerRef = React.useRef<AbortController | null>(null); + const currentQueryRef = React.useRef<string>('');
370-449: Include prometheusStatus in the checkPrometheusConnection closureBecause
prometheusStatusisn’t in theuseCallbackdeps, the closure freezes it at'checking'. The guard at Line 377 then short-circuits future checks, so periodic health probes never run unless you force them. AddprometheusStatusto the dependency list (and adjust callers as needed) so the logic sees the latest status.- const checkPrometheusConnection = React.useCallback(async (isRetry = false, force = false) => { + const checkPrometheusConnection = React.useCallback(async (isRetry = false, force = false) => { … - }, [prometheusUrl, selectedPage]); + }, [prometheusUrl, selectedPage, prometheusStatus]);
520-539: Update the interval effect dependenciesThis effect schedules
checkPrometheusConnection/checkOpensearchConnection, but the dependency array ignores both callbacks. When the callbacks change (e.g., after addingprometheusStatusto the dependency list) the interval keeps calling the stale version. Add both callbacks to the dependency array so the interval is torn down and rebuilt with the current logic.- }, [selectedPage]); // Only depend on selectedPage + }, [selectedPage, checkPrometheusConnection, checkOpensearchConnection]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
Dockerfile(1 hunks)experimental/ag-ui/README.md(1 hunks)experimental/ag-ui/front-end/.gitignore(1 hunks)experimental/ag-ui/front-end/README.md(1 hunks)experimental/ag-ui/front-end/public/index.html(1 hunks)experimental/ag-ui/front-end/src/App.css(1 hunks)experimental/ag-ui/front-end/src/App.tsx(1 hunks)experimental/ag-ui/front-end/src/components/ChatAssistant.css(1 hunks)experimental/ag-ui/front-end/src/components/ChatAssistant.tsx(1 hunks)experimental/ag-ui/front-end/src/components/ErrorBoundary.tsx(1 hunks)experimental/ag-ui/front-end/src/components/GraphVisualization.css(1 hunks)experimental/ag-ui/front-end/src/components/GraphVisualization.tsx(1 hunks)experimental/ag-ui/front-end/src/components/LogsVisualization.css(1 hunks)experimental/ag-ui/front-end/src/components/LogsVisualization.tsx(1 hunks)experimental/ag-ui/front-end/src/components/MainContent.tsx(1 hunks)experimental/ag-ui/front-end/src/index.css(1 hunks)experimental/ag-ui/front-end/src/index.tsx(1 hunks)experimental/ag-ui/front-end/tsconfig.json(1 hunks)experimental/ag-ui/server-agui.py(1 hunks)holmes/config.py(3 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_query_assist.py(1 hunks)tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- Dockerfile
- experimental/ag-ui/front-end/src/index.css
- experimental/ag-ui/front-end/public/index.html
- holmes/plugins/toolsets/init.py
- experimental/ag-ui/front-end/README.md
- experimental/ag-ui/front-end/src/components/GraphVisualization.css
- experimental/ag-ui/front-end/.gitignore
- experimental/ag-ui/front-end/src/index.tsx
- experimental/ag-ui/README.md
- experimental/ag-ui/front-end/tsconfig.json
- experimental/ag-ui/front-end/src/App.tsx
- experimental/ag-ui/front-end/src/components/LogsVisualization.css
- experimental/ag-ui/front-end/src/components/ChatAssistant.css
- holmes/config.py
🧰 Additional context used
📓 Path-based instructions (4)
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/opensearch/opensearch_ppl_query_docs.jinja2holmes/plugins/toolsets/opensearch/opensearch_query_assist.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/opensearch/opensearch_query_assist.pytests/plugins/toolsets/opensearch/test_opensearch_query_assist.pyexperimental/ag-ui/server-agui.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers/tags
Files:
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py
🧠 Learnings (1)
📚 Learning: 2025-10-08T00:03:11.993Z
Learnt from: kylehounslow
PR: robusta-dev/holmesgpt#1035
File: experimental/ag-ui/front-end/src/components/ChatAssistant.tsx:118-482
Timestamp: 2025-10-08T00:03:11.993Z
Learning: The directory `experimental/ag-ui/front-end/` contains demo/example front-end code for AG-UI demonstration purposes and does not require thorough code review.
Applied to files:
experimental/ag-ui/front-end/src/components/ChatAssistant.tsx
🧬 Code graph analysis (4)
experimental/ag-ui/front-end/src/components/MainContent.tsx (1)
experimental/ag-ui/front-end/src/App.tsx (1)
ObservabilityPage(7-7)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (1)
holmes/core/tools.py (8)
StructuredToolResult(79-103)StructuredToolResultStatus(52-76)Tool(173-365)ToolParameter(155-161)Toolset(534-768)ToolsetTag(143-146)ToolInvokeContext(164-170)_load_llm_instructions(763-768)
tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py (2)
holmes/core/tools.py (6)
StructuredToolResult(79-103)StructuredToolResultStatus(52-76)ToolParameter(155-161)ToolsetTag(143-146)ToolInvokeContext(164-170)Toolset(534-768)holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (6)
PplQueryAssistTool(16-51)OpenSearchQueryAssistToolset(54-77)_invoke(31-47)get_parameterized_one_liner(49-51)get_example_config(68-69)_reload_instructions(71-77)
experimental/ag-ui/server-agui.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(25-27)StreamEvents(15-22)holmes/config.py (5)
Config(46-522)load_from_env(172-205)dal(121-124)create_agui_toolcalling_llm(297-308)get_models_list(518-522)holmes/core/conversations.py (1)
build_chat_messages(307-399)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(888-1128)
🪛 Ruff (0.13.3)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py
31-31: Unused method argument: context
(ARG002)
45-45: Use explicit conversion flag
Replace with conversion flag
(RUF010)
experimental/ag-ui/server-agui.py
90-90: Unused function argument: request
(ARG001)
233-233: Use explicit conversion flag
Replace with conversion flag
(RUF010)
334-339: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (5)
holmes/plugins/toolsets/opensearch/opensearch_query_assist.py (2)
31-47: LGTM—unusedcontextparameter is required by API contract.The static analysis warning about the unused
contextparameter (ARG002) is a false positive. The_invokesignature must match the abstract method in theToolbase class (holmes/core/tools.py:231), which requirescontext: ToolInvokeContexteven if not used by this particular implementation.
54-77: LGTM—toolset follows standard pattern.The toolset initialization and template loading implementation correctly follow the established pattern. The
_reload_instructionsmethod properly constructs an absolute file path and formats it as afile://URI for the template loader.tests/plugins/toolsets/opensearch/test_opensearch_query_assist.py (3)
17-211: Excellent test coverage for PplQueryAssistTool.The test suite comprehensively covers:
- Tool initialization and parameter configuration
- Various invoke scenarios (valid, empty, missing, complex, list, None queries)
- One-liner generation for different input types
- Clear AAA (Arrange-Act-Assert) structure
213-329: Well-structured toolset tests with appropriate mocking.The test suite properly validates:
- Toolset initialization properties and configuration
- Template path construction with detailed mocking
- Inheritance and interface conformance
- Proper isolation with targeted mocks for filesystem operations
331-395: Strong integration test coverage.The integration tests effectively validate:
- Tool-toolset interaction and data flow
- Configuration consistency across components
- Template loading behavior with appropriate mocking
- End-to-end functionality
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
experimental/ag-ui/server-agui.py (1)
318-323: Robustly parse timeseries data that may already be a dict.Line 322 calls
json.loads(result_data["data"])assuming it's a JSON string, but the data field may already be a parsed dict or list. This will raiseTypeErrorif it's not a string.Apply this diff to handle both cases:
# Handle different Prometheus response formats prometheus_data = result_data result_type = "unknown" if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") + raw_data = result_data["data"] + # Parse if string, otherwise use as-is + if isinstance(raw_data, str): + try: + parsed = json.loads(raw_data) + except json.JSONDecodeError: + logging.warning(f"Failed to parse data field as JSON: {raw_data}") + parsed = {} + else: + parsed = raw_data + prometheus_data = parsed.get("data", parsed) if isinstance(parsed, dict) else parsed result_type = prometheus_data.get("resultType", "unknown")
🧹 Nitpick comments (3)
experimental/ag-ui/server-agui.py (3)
89-91: Remove unused request parameter.The
requestparameter is not used in this health check endpoint.Apply this diff:
-def agui_chat_health(request: Request): +def agui_chat_health(): return JSONResponse(content={"status": "ok"})
55-71: Consider adding type hints to helper functions.As per coding guidelines, type hints are required. Several helper functions lack return type annotations:
init_logging()(line 55)_format_todo_write(data)(line 242)_parse_query(data)(line 275)_should_graph_timeseries_data(tool_name)(line 282)_is_tool_result_message(input_data)(line 383)Example for
init_logging:def init_logging() -> None: ...Example for
_is_tool_result_message:def _is_tool_result_message(input_data: RunAgentInput) -> bool: ...Also applies to: 242-255, 275-279, 282-287, 383-384
233-233: Optional: Address static analysis style suggestions.Two minor style improvements suggested by static analysis:
- Line 233: Use explicit conversion flag instead of
str(e):message=f"Agent encountered an error: {e!s}",
- Lines 334-339: Move the return statement to an
elseblock after theexceptfor clearer structure (TRY300).Also applies to: 334-339
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experimental/ag-ui/server-agui.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server-agui.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: kylehounslow
PR: robusta-dev/holmesgpt#1035
File: experimental/ag-ui/front-end/src/components/ChatAssistant.tsx:118-482
Timestamp: 2025-10-08T00:03:11.993Z
Learning: The directory `experimental/ag-ui/front-end/` contains demo/example front-end code for AG-UI demonstration purposes and does not require thorough code review.
🧬 Code graph analysis (1)
experimental/ag-ui/server-agui.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(25-27)StreamEvents(15-22)holmes/config.py (5)
Config(46-522)load_from_env(172-205)dal(121-124)create_agui_toolcalling_llm(297-308)get_models_list(518-522)holmes/core/conversations.py (1)
build_chat_messages(307-399)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(888-1128)
🪛 Ruff (0.13.3)
experimental/ag-ui/server-agui.py
90-90: Unused function argument: request
(ARG001)
233-233: Use explicit conversion flag
Replace with conversion flag
(RUF010)
334-339: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (1)
experimental/ag-ui/server-agui.py (1)
94-239: Well-structured AG-UI integration.The main endpoint successfully translates HolmesGPT outputs into AG-UI events with clear separation:
- Input conversion (lines 105-120)
- Event streaming with proper async generators
- Conditional front-end tool invocation based on tool types
- Comprehensive error handling
The integration with
config.create_agui_toolcalling_llmandbuild_chat_messagesleverages existing Holmes infrastructure effectively.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
experimental/ag-ui/server-agui.py (1)
210-221: Fix unsafe data slicing to prevent TypeError.Line 216 slices
chunk.data.get('result', {}).get('data', '')directly, which will raiseTypeErrorifdatais a dict, list (non-string sequence), or other non-string object. Previous reviews flagged this issue; it remains unaddressed.Apply this diff to safely extract and format the data:
if not front_end_tool_invoked: # TODO [FUTURE]: Render "TodoWrite" tool_name results prettier. Use code block for now. # Ideally using TOOL_STEP events. if tool_name == "TodoWrite": tool_message = _format_todo_write(data=chunk.data) else: - tool_message = f"🔧 {tool_name} result:\n{chunk.data.get('result', {}).get('data', '')[0:200]}..." + # Safely extract and format result data + result_obj = chunk.data.get('result', {}) + data_field = result_obj.get('data', '') + if isinstance(data_field, (dict, list)): + try: + data_str = json.dumps(data_field) + except Exception: + data_str = str(data_field) + else: + data_str = str(data_field) if data_field is not None else '' + snippet = data_str[:200] + tool_message = f"🔧 {tool_name} result:\n{snippet}..." async for event in _stream_agui_text_message_event( message=tool_message
🧹 Nitpick comments (5)
experimental/ag-ui/server-agui.py (5)
74-87: Restrict CORS origins before production deployment.The current CORS configuration allows all origins (
allow_origins=["*"]), which is acceptable for an experimental/demo server but poses security risks in production. Before deploying to production, restrictallow_originsto specific trusted domains.
90-92: Remove unusedrequestparameter.The
requestparameter is not used in the health check endpoint. Remove it to clean up the function signature.Apply this diff:
@app.get("/api/agui/chat/health") -def agui_chat_health(request: Request): +def agui_chat_health(): return JSONResponse(content="ok")
259-273: Consider more robust tool matching for production.The current string-based matching (
"execute_prom" in fe_tool.name) works for the experimental/demo context but may be fragile if tool names change. For production deployment, consider using explicit tool identifiers or enums.
445-447: Consider renaming response key for clarity.The response key
"model_name"is singular but contains a list of models. Consider renaming it to"model_names"or"models"for better clarity.Apply this diff:
@app.get("/api/model") def get_model(): - return {"model_name": config.get_models_list()} + return {"models": config.get_models_list()}
450-460: Consider multiprocess deployment for production scaling.The current setup runs a single uvicorn process, which is appropriate for experimental/demo purposes. For production deployment with higher load, consider using uvicorn's built-in multiprocess manager (
--workersflag) or deploying behind a reverse proxy with multiple workers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experimental/ag-ui/server-agui.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server-agui.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: kylehounslow
PR: robusta-dev/holmesgpt#1035
File: experimental/ag-ui/front-end/src/components/ChatAssistant.tsx:118-482
Timestamp: 2025-10-08T00:03:11.993Z
Learning: The directory `experimental/ag-ui/front-end/` contains demo/example front-end code for AG-UI demonstration purposes and does not require thorough code review.
🧬 Code graph analysis (1)
experimental/ag-ui/server-agui.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(25-27)StreamEvents(15-22)holmes/config.py (5)
Config(46-522)load_from_env(172-205)dal(121-124)create_agui_toolcalling_llm(297-308)get_models_list(518-522)holmes/core/conversations.py (1)
build_chat_messages(307-399)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(888-1128)
🪛 Ruff (0.13.3)
experimental/ag-ui/server-agui.py
91-91: Unused function argument: request
(ARG001)
234-234: Use explicit conversion flag
Replace with conversion flag
(RUF010)
335-340: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (15)
experimental/ag-ui/server-agui.py (15)
1-11: LGTM! Certificate setup pattern is correct.The certificate setup before network imports is correct, and the environment variable name
CERTIFICATEmatches the official HolmesGPT documentation as confirmed in previous review discussions.
12-53: LGTM! Import organization follows best practices.All imports are properly placed after the certificate setup block, and the necessary dependencies for the AG-UI server are correctly imported.
56-72: LGTM! Logging configuration is well-structured.The colored logging setup with environment-driven log levels and httpx noise reduction is appropriate for a development/debugging server.
95-104: LGTM! Endpoint setup and early return logic is correct.The accept header parsing and early return for unsupported tool result messages is appropriate. The bounds check in
_is_tool_result_messagewas correctly added in a previous iteration.
106-121: LGTM! Chat request validation and message building is correct.The conversion from AG-UI input to Holmes chat request, validation of the ask field, and message building using the existing
build_chat_messagesfunction is properly implemented.
138-161: LGTM! AI message processing is correctly implemented.The event type extraction with safety checks and content streaming via
_stream_agui_text_message_eventis properly implemented.
162-166: LGTM! Start tool event processing is correct.The tool name extraction and message formatting for tool start events is properly implemented.
229-236: LGTM! Error handling is appropriately implemented.The try-except block properly catches exceptions during streaming, logs with traceback, and emits a
RUN_ERRORevent to inform the client. This is the correct pattern for handling errors in async generators.
238-240: LGTM! StreamingResponse setup is correct.The
StreamingResponsewith the event generator and encoder-determined content type is properly configured for streaming AG-UI events.
243-256: LGTM! TodoWrite formatting is well-implemented.The function safely extracts todo items and formats them with status icons for clear presentation to users.
276-280: LGTM! Query parsing is correctly implemented.The function safely extracts the query field using nested
.get()calls with appropriate defaults.
283-288: LGTM! Timeseries data check is correctly implemented.The function clearly checks for Prometheus tools and includes a comment explaining the current limitation. This is appropriate for the experimental scope.
357-370: LGTM! Front-end tool invocation is correctly implemented.The function properly yields the sequence of AG-UI tool call events (START, ARGS, END) with correct serialization of arguments.
373-381: LGTM! Text message streaming is correctly implemented.The function generates a unique message ID and properly yields the sequence of AG-UI text message events with correct structure.
384-385: LGTM! Tool result message check is correctly implemented.The function properly checks the list length before accessing the last element, preventing
IndexErroras addressed in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
experimental/ag-ui/server-agui.py (2)
210-217: Tool result slicing will crash on non-string payloads.
chunk.data["result"]["data"]is often a dict/list; slicing it raisesTypeError, so TOOL_RESULT streaming breaks. Convert to a string safely before truncating.- else: - tool_message = f"🔧 {tool_name} result:\n{chunk.data.get('result', {}).get('data', '')[0:200]}..." + else: + raw_data = chunk.data.get("result", {}).get("data", "") + if not isinstance(raw_data, str): + try: + raw_data = json.dumps(raw_data) + except Exception: # pragma: no cover + raw_data = str(raw_data) + tool_message = ( + f"🔧 {tool_name} result:\n{raw_data[:200]}..." + if raw_data + else f"🔧 {tool_name} result:" + )
320-340: Handle non-string Prometheus payloads beforejson.loads.
result_data["data"]is already a dict for many Prometheus responses; callingjson.loadson it throwsTypeError, so time-series parsing breaks. Please normalize the payload first and keep a safe fallback:- prometheus_data = result_data - result_type = "unknown" - if "data" in result_data: - prometheus_data = json.loads(result_data["data"]).get("data") - result_type = prometheus_data.get("resultType", "unknown") + prometheus_data = result_data + result_type = "unknown" + if "data" in result_data: + raw = result_data["data"] + try: + parsed = json.loads(raw) if isinstance(raw, str) else raw + except Exception: + logging.warning("Failed to parse Prometheus data", exc_info=True) + parsed = {} + prometheus_data = parsed.get("data", parsed) + if isinstance(prometheus_data, dict): + result_type = prometheus_data.get("resultType", "unknown")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
experimental/ag-ui/server-agui.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
experimental/ag-ui/server-agui.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: kylehounslow
PR: robusta-dev/holmesgpt#1035
File: experimental/ag-ui/front-end/src/components/ChatAssistant.tsx:118-482
Timestamp: 2025-10-08T00:03:11.993Z
Learning: The directory `experimental/ag-ui/front-end/` contains demo/example front-end code for AG-UI demonstration purposes and does not require thorough code review.
🧬 Code graph analysis (1)
experimental/ag-ui/server-agui.py (6)
holmes/utils/cert_utils.py (1)
add_custom_certificate(29-40)holmes/utils/stream.py (2)
StreamMessage(25-27)StreamEvents(15-22)holmes/config.py (5)
Config(46-522)load_from_env(172-205)dal(121-124)create_agui_toolcalling_llm(297-308)get_models_list(518-522)holmes/core/conversations.py (1)
build_chat_messages(307-399)holmes/core/models.py (1)
ChatRequest(232-233)holmes/core/tool_calling_llm.py (1)
call_stream(888-1128)
🪛 Ruff (0.13.3)
experimental/ag-ui/server-agui.py
1-1: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
91-91: Unused function argument: request
(ARG001)
234-234: Use explicit conversion flag
Replace with conversion flag
(RUF010)
335-340: Consider moving this statement to an else block
(TRY300)
|
mypy and ruff fixes have been applied. Looks like things are passing on my forked repo workflows: https://github.com/kylehounslow/holmesgpt/actions/runs/18390467516 |
|
Thanks @kylehounslow ! |
3 participants
Summary
This PR introduces an experimental AG-UI chat server for HolmesGPT. The primary use-case is to support AI-powered data exploration and root-cause-analysis capabilities directly within observability platforms like OpenSearch Dashboards. It also adds experimental support for query assist with OpenSearch Piped Processing Language (PPL).
Resolves #889
Why is this change necessary?
Platforms like OpenSearch Dashboards are integrating AI-powered data exploration and problem diagnosis capabilities directly into their core user experience, making open-source observability more intelligent and automated (see RFCs here and here). By implementing AG-UI support, HolmesGPT can be deployed as a modular, context-aware agent that seamlessly integrates with the frontend architecture. This design approach minimizes implementation overhead while maintaining deep integration with the platform's UI components and data structures.
Issues
Summary of Changes
experimental/ag-ui/server.pyconfig.py(loads same toolsets as existing console)experimental/ag-ui/front-end/pyproject.tomlNotes for Reviewers
experimental/ag-ui/front-end/is for demonstration purposes only. Feel free to skim this or omit altogether for this review.experimental/ag-ui/server.pyholmes/plugins/toolsets/opensearch/opensearch_query_assist.pyTesting
Unit Testing
Functional Testing
An example observability front-end was built to demonstrate the AG-UI functionality.
Setup details are at
experimental/ag-ui/README.md. Start a prometheus server and opensearch cluster (e.g. using opentelemetry-demo) and: