fix: improve agent and Ollama error handling and validation#10385
fix: improve agent and Ollama error handling and validation#10385TejasQ wants to merge 12 commits into
Conversation
- Add statusText parameter to StreamingRequestParams type - Update performStreamingRequest to pass statusText to error handlers - Improve buildUtils error handling to show proper server error messages - Replace generic 'Network error' with specific server error details Fixes issue where 500 errors were incorrectly labeled as network errors
- Add warning when mirostat is enabled for potentially unsupported models - Add check_model_supports_mirostat method for future model validation - Improve user guidance when mirostat warnings appear in Ollama logs Helps users understand when models don't support mirostat parameters
- Fix event type mismatch: 'tool_start' -> 'on_tool_start' - Fix event type mismatch: 'tool_end' -> 'on_tool_end' - Add missing on_chain_end event handler - Add missing on_tool_error event handler Ensures tool calls are properly displayed in UI instead of only in logs
- Add validation in AgentComponent.message_response() for empty inputs - Provide clear error messages for empty message inputs Prevents validation errors when users send empty messages to agents
- Add context-aware error handling for validation and connection errors - Provide specific error types (ValueError, ConnectionError) based on error content - Improve error logging with more context Better error categorization and debugging information
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR extends error handling across frontend and backend components. Frontend API streaming now passes HTTP status text alongside status codes to error callbacks. Backend agent framework introduces specialized exception types (ValueError for validation, ConnectionError for network issues), new callback methods for chain and tool lifecycle events, and input validation for agents and Ollama components including model capability checking. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Changes span 6 files across frontend and backend with heterogeneous modifications including API signature updates, input validation patterns, specialized exception handling, callback lifecycle management, and model support detection. While individual changes are straightforward, the variety of concerns and distributed nature across components requires contextual reasoning for each area. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 error, 3 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
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 |
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/controllers/API/api.tsx (1)
315-323: Abort after invoking onError to avoid reading error bodiesCurrently, after calling onError, the function continues and tries to read the body/stream. This can mis-handle error responses and cause stray parsing.
Apply:
if (!response.ok) { if (onError) { - onError(response.status, response.statusText); + onError(response.status, response.statusText); + return; // stop processing on error } else { throw new Error("Error in streaming request."); } }
🧹 Nitpick comments (3)
src/lfx/src/lfx/components/ollama/ollama.py (2)
147-155: Good defensive validation with clear error messages.The validation correctly handles both empty and whitespace-only values with actionable error messages.
For slightly cleaner code, consider:
- if not self.model_name or not str(self.model_name).strip(): + if not (self.model_name and str(self.model_name).strip()): msg = "Model name cannot be empty. Please select a model from the dropdown or enter a valid model name." raise ValueError(msg) - if not self.base_url or not str(self.base_url).strip(): + if not (self.base_url and str(self.base_url).strip()): msg = "Base URL cannot be empty. Please provide a valid Ollama base URL (e.g., http://localhost:11434)." raise ValueError(msg)
170-175: Consider using the new capability check method.The generic warning is helpful, but you added
check_model_supports_mirostat(lines 361-396) which could make this warning conditional. Currently that method is never called.Consider checking actual model capabilities:
# Check if model supports mirostat supports_mirostat = await self.check_model_supports_mirostat(transformed_base_url, self.model_name) if not supports_mirostat: logger.warning( f"Model '{self.model_name}' may not support mirostat. " f"If you see 'invalid option provided: mirostat' errors, consider disabling mirostat." )Note: This requires
build_modelto become async or the check to be performed elsewhere.src/lfx/src/lfx/base/agents/agent.py (1)
244-255: Tool-name validation: good coveragePattern is tight and error is descriptive. Consider compiling regex once at module level to avoid recompilation on hot paths.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/frontend/src/controllers/API/api.tsx(2 hunks)src/frontend/src/utils/buildUtils.ts(2 hunks)src/lfx/src/lfx/base/agents/agent.py(1 hunks)src/lfx/src/lfx/base/agents/callback.py(5 hunks)src/lfx/src/lfx/components/agents/agent.py(1 hunks)src/lfx/src/lfx/components/ollama/ollama.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/frontend/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
src/frontend/src/**/*.{ts,tsx,js,jsx}: All frontend TypeScript and JavaScript code should be located under src/frontend/src/ and organized into components, pages, icons, stores, types, utils, hooks, services, and assets directories as per the specified directory layout.
Use React 18 with TypeScript for all UI components in the frontend.
Format all TypeScript and JavaScript code using the make format_frontend command.
Lint all TypeScript and JavaScript code using the make lint command.
Files:
src/frontend/src/controllers/API/api.tsxsrc/frontend/src/utils/buildUtils.ts
src/frontend/src/utils/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend_development.mdc)
All utility functions should be placed in the utils directory.
Files:
src/frontend/src/utils/buildUtils.ts
🧬 Code graph analysis (1)
src/lfx/src/lfx/components/ollama/ollama.py (1)
src/lfx/src/lfx/utils/util.py (1)
transform_localhost_url(119-161)
🪛 GitHub Actions: Ruff Style Check
src/lfx/src/lfx/base/agents/agent.py
[error] 230-230: Ruff: TRY003 Avoid specifying long messages outside the exception class. - Avoid placing long messages in exception raising; prefer concise messages or define a dedicated exception with a docstring.
🪛 GitHub Check: Ruff Style Check (3.13)
src/lfx/src/lfx/base/agents/agent.py
[failure] 233-233: Ruff (EM102)
src/lfx/src/lfx/base/agents/agent.py:233:39: EM102 Exception must not use an f-string literal, assign to variable first
[failure] 233-233: Ruff (TRY003)
src/lfx/src/lfx/base/agents/agent.py:233:23: TRY003 Avoid specifying long messages outside the exception class
[failure] 230-230: Ruff (EM102)
src/lfx/src/lfx/base/agents/agent.py:230:34: EM102 Exception must not use an f-string literal, assign to variable first
[failure] 230-230: Ruff (TRY003)
src/lfx/src/lfx/base/agents/agent.py:230:23: TRY003 Avoid specifying long messages outside the exception class
src/lfx/src/lfx/components/ollama/ollama.py
[failure] 388-388: Ruff (F841)
src/lfx/src/lfx/components/ollama/ollama.py:388:17: F841 Local variable parameters is assigned to but never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Lint Backend / Run Mypy (3.12)
- GitHub Check: Lint Backend / Run Mypy (3.10)
- GitHub Check: Lint Backend / Run Mypy (3.13)
- GitHub Check: Lint Backend / Run Mypy (3.11)
- GitHub Check: Run Backend Tests / LFX Tests - Python 3.10
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 5
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 1
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 3
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 4
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 2
- GitHub Check: Run Frontend Tests / Determine Test Suites and Shard Distribution
- GitHub Check: Run Backend Tests / Integration Tests - Python 3.10
- GitHub Check: Run Frontend Unit Tests / Frontend Jest Unit Tests
- GitHub Check: Test Starter Templates
- GitHub Check: Validate PR
- GitHub Check: Update Component Index
🔇 Additional comments (4)
src/lfx/src/lfx/components/ollama/ollama.py (1)
38-38: LGTM! Required fields prevent empty inputs.Making
base_urlandmodel_namerequired ensures users provide valid values and aligns with the validation logic added inbuild_model.Also applies to: 47-47
src/frontend/src/utils/buildUtils.ts (1)
346-357: Improved 5xx handling with statusText — LGTM, pending upstream return fixServer errors now surface clear messages via onBuildError. Ensure api.tsx returns after onError so streaming halts; otherwise this may still read error bodies.
Also applies to: 432-443
src/lfx/src/lfx/base/agents/callback.py (1)
31-41: Event type renaming is properly integrated—no breaking changes detectedThe review comment flags a potential breaking change, but verification shows the renamed event types ("on_chain_start", "on_tool_start", "on_tool_end") are correctly wired throughout the codebase. The event handlers in
events.pyhave mappings for exactly these prefixed names (lines 316, 323–325), and production components likecuga_agent.pyandastradb_assistant_manager.pyalready emit and consume these event types. The "agent_action" and "agent_finish" event types remain unchanged (lines 141, 164 in callback.py). No consumers expect the old unprefixed names.Likely an incorrect or invalid review comment.
src/frontend/src/controllers/API/api.tsx (1)
268-276: All StreamingRequestParams.onError call sites correctly use two-argument signatureVerification confirms both
performStreamingRequest()call sites in buildUtils.ts (lines 330 and 417) properly implement the updatedonError: (statusCode, statusText?)signature. No residual single-argument usages exist.
| # Log or handle any other exceptions with more context | ||
| error_details = str(e) | ||
| if "validation error" in error_details.lower(): | ||
| logger.error(f"Agent validation error: {error_details}") | ||
| raise ValueError(f"Agent validation failed: {error_details}") from e | ||
| if "connection" in error_details.lower() or "timeout" in error_details.lower(): | ||
| logger.error(f"Agent connection error: {error_details}") | ||
| raise ConnectionError(f"Agent connection failed: {error_details}") from e | ||
| logger.error(f"Unexpected agent error: {error_details}") | ||
| raise |
There was a problem hiding this comment.
Fix Ruff EM102/TRY003 and keep messages concise
Avoid f-strings directly in raise and long inline messages. Assign to a variable and re-raise. Also short messages satisfy TRY003.
Apply:
- except Exception as e:
- # Log or handle any other exceptions with more context
- error_details = str(e)
- if "validation error" in error_details.lower():
- logger.error(f"Agent validation error: {error_details}")
- raise ValueError(f"Agent validation failed: {error_details}") from e
- if "connection" in error_details.lower() or "timeout" in error_details.lower():
- logger.error(f"Agent connection error: {error_details}")
- raise ConnectionError(f"Agent connection failed: {error_details}") from e
- logger.error(f"Unexpected agent error: {error_details}")
- raise
+ except Exception as e:
+ # Log or handle any other exceptions with more context
+ error_details = str(e)
+ lower = error_details.lower()
+ if "validation error" in lower:
+ logger.error("Agent validation error: %s", error_details)
+ msg = "Agent validation failed"
+ raise ValueError(msg) from e
+ if "connection" in lower or "timeout" in lower:
+ logger.error("Agent connection error: %s", error_details)
+ msg = "Agent connection failed"
+ raise ConnectionError(msg) from e
+ logger.error("Unexpected agent error: %s", error_details)
+ raiseOptionally, refine classification using concrete exception types (e.g., TimeoutError, httpx.ConnectError) instead of substring checks.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Ruff Style Check
[error] 230-230: Ruff: TRY003 Avoid specifying long messages outside the exception class. - Avoid placing long messages in exception raising; prefer concise messages or define a dedicated exception with a docstring.
🪛 GitHub Check: Ruff Style Check (3.13)
[failure] 233-233: Ruff (EM102)
src/lfx/src/lfx/base/agents/agent.py:233:39: EM102 Exception must not use an f-string literal, assign to variable first
[failure] 233-233: Ruff (TRY003)
src/lfx/src/lfx/base/agents/agent.py:233:23: TRY003 Avoid specifying long messages outside the exception class
[failure] 230-230: Ruff (EM102)
src/lfx/src/lfx/base/agents/agent.py:230:34: EM102 Exception must not use an f-string literal, assign to variable first
[failure] 230-230: Ruff (TRY003)
src/lfx/src/lfx/base/agents/agent.py:230:23: TRY003 Avoid specifying long messages outside the exception class
| # Validate input is not empty | ||
| if not self.input_value or not str(self.input_value).strip(): | ||
| msg = "Message cannot be empty. Please provide a valid message." | ||
| raise ValueError(msg) | ||
|
|
There was a problem hiding this comment.
Don’t block image-only inputs
Precheck rejects empty text, but run_agent supports image-only payloads by moving images to chat_history. This will now raise on those valid cases.
Apply a guarded validation:
try:
- # Validate input is not empty
- if not self.input_value or not str(self.input_value).strip():
- msg = "Message cannot be empty. Please provide a valid message."
- raise ValueError(msg)
+ # Validate input is not empty, but allow image-only inputs
+ is_empty_text = not self.input_value or not str(self.input_value).strip()
+ if is_empty_text:
+ try:
+ to_msg = getattr(self.input_value, "to_lc_message", None)
+ lc = to_msg() if callable(to_msg) else None
+ content = getattr(lc, "content", None)
+ has_images = isinstance(content, list) and any(
+ isinstance(it, dict) and it.get("type") == "image" for it in content
+ )
+ except Exception:
+ has_images = False
+ if not has_images:
+ msg = "Message cannot be empty. Please provide a valid message."
+ raise ValueError(msg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Validate input is not empty | |
| if not self.input_value or not str(self.input_value).strip(): | |
| msg = "Message cannot be empty. Please provide a valid message." | |
| raise ValueError(msg) | |
| # Validate input is not empty, but allow image-only inputs | |
| is_empty_text = not self.input_value or not str(self.input_value).strip() | |
| if is_empty_text: | |
| try: | |
| to_msg = getattr(self.input_value, "to_lc_message", None) | |
| lc = to_msg() if callable(to_msg) else None | |
| content = getattr(lc, "content", None) | |
| has_images = isinstance(content, list) and any( | |
| isinstance(it, dict) and it.get("type") == "image" for it in content | |
| ) | |
| except Exception: | |
| has_images = False | |
| if not has_images: | |
| msg = "Message cannot be empty. Please provide a valid message." | |
| raise ValueError(msg) |
🤖 Prompt for AI Agents
In src/lfx/src/lfx/components/agents/agent.py around lines 206–210, the current
precheck always raises for empty text which incorrectly blocks valid image‑only
inputs; change the guard so you only raise ValueError when there is no textual
input AND there are no images/attachments present in the incoming payload (e.g.,
check self.images, self.attachments or chat_history for image entries).
Concretely, replace the unconditional empty-text check with a compound condition
that first trims and tests text, and only raises if that text is empty AND no
image fields exist in the agent state/payload.
| error_details = str(e) | ||
| if "validation error" in error_details.lower(): | ||
| msg = ( | ||
| f"Ollama model validation error: {error_details}. " | ||
| f"This may be due to incompatible parameters for model '{self.model_name}'. " | ||
| f"Please check the model parameters and try again." | ||
| ) | ||
| elif "connection" in error_details.lower() or "timeout" in error_details.lower(): | ||
| msg = ( | ||
| f"Unable to connect to the Ollama API: {error_details}. " | ||
| f"Please verify the base URL ({self.base_url}), ensure Ollama is running, " | ||
| f"and that the model '{self.model_name}' is pulled." | ||
| ) | ||
| else: | ||
| msg = ( | ||
| f"Unable to initialize Ollama model: {error_details}. " | ||
| f"Please verify the base URL, ensure the relevant Ollama model is pulled, and try again." | ||
| ) | ||
| raise ValueError(msg) from e |
There was a problem hiding this comment.
Inconsistent with PR objectives: network errors should raise ConnectionError.
The PR description states "specialized exception types (ValueError for validation, ConnectionError for network issues)," but lines 229-234 raise ValueError for connection/timeout errors instead of ConnectionError.
Apply this diff to align with the PR objectives:
elif "connection" in error_details.lower() or "timeout" in error_details.lower():
msg = (
f"Unable to connect to the Ollama API: {error_details}. "
f"Please verify the base URL ({self.base_url}), ensure Ollama is running, "
f"and that the model '{self.model_name}' is pulled."
)
+ raise ConnectionError(msg) from e
- else:
+ else:
msg = (
f"Unable to initialize Ollama model: {error_details}. "
f"Please verify the base URL, ensure the relevant Ollama model is pulled, and try again."
)
- raise ValueError(msg) from e
+ raise ValueError(msg) from eCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lfx/src/lfx/components/ollama/ollama.py around lines 222 to 240, the
exception handling currently raises ValueError for connection/timeout errors
which contradicts the PR objective to use specialized exceptions; change the
branch that detects "connection" or "timeout" in error_details to raise
ConnectionError(msg) instead of ValueError(msg) (preserve the constructed msg
and use exception chaining with "from e"), keep the validation branch raising
ValueError as-is, and leave the final fallback raising ValueError for other
cases.
| async def check_model_supports_mirostat(self, base_url_value: str, model_name: str) -> bool: | ||
| """Check if a specific model supports mirostat parameter. | ||
|
|
||
| Args: | ||
| base_url_value (str): The base URL of the Ollama API. | ||
| model_name (str): The name of the model to check. | ||
|
|
||
| Returns: | ||
| bool: True if the model supports mirostat, False otherwise. | ||
| """ | ||
| try: | ||
| base_url = base_url_value.rstrip("/").removesuffix("/v1") | ||
| if not base_url.endswith("/"): | ||
| base_url = base_url + "/" | ||
| base_url = transform_localhost_url(base_url) | ||
|
|
||
| show_url = urljoin(base_url, "api/show") | ||
|
|
||
| async with httpx.AsyncClient() as client: | ||
| payload = {"model": model_name} | ||
| show_response = await client.post(show_url, json=payload) | ||
| show_response.raise_for_status() | ||
| json_data = show_response.json() | ||
| if asyncio.iscoroutine(json_data): | ||
| json_data = await json_data | ||
|
|
||
| # Check if the model has parameter information | ||
| parameters = json_data.get("parameters", {}) | ||
| # Some models might not have detailed parameter info, so we'll be conservative | ||
| # and assume they support mirostat unless we know otherwise | ||
| return True | ||
|
|
||
| except (httpx.RequestError, ValueError) as e: | ||
| await logger.awarning(f"Could not check mirostat support for model {model_name}: {e}") | ||
| # If we can't check, assume it supports mirostat to avoid breaking functionality | ||
| return True |
There was a problem hiding this comment.
Method is unused and always returns True, making it ineffective.
This method has several issues:
- Never invoked: The method is not called anywhere in the codebase.
- Always returns True: Lines 391 and 396 always return True, so the check is meaningless.
- Unused variable: Line 388 assigns
parametersbut never uses it (flagged by static analysis). - No actual validation: Despite fetching model data, no actual capability checking occurs.
Consider one of these approaches:
Option 1 (recommended): Remove the method entirely if mirostat support cannot be reliably determined:
- async def check_model_supports_mirostat(self, base_url_value: str, model_name: str) -> bool:
- """Check if a specific model supports mirostat parameter.
-
- Args:
- base_url_value (str): The base URL of the Ollama API.
- model_name (str): The name of the model to check.
-
- Returns:
- bool: True if the model supports mirostat, False otherwise.
- """
- try:
- base_url = base_url_value.rstrip("/").removesuffix("/v1")
- if not base_url.endswith("/"):
- base_url = base_url + "/"
- base_url = transform_localhost_url(base_url)
-
- show_url = urljoin(base_url, "api/show")
-
- async with httpx.AsyncClient() as client:
- payload = {"model": model_name}
- show_response = await client.post(show_url, json=payload)
- show_response.raise_for_status()
- json_data = show_response.json()
- if asyncio.iscoroutine(json_data):
- json_data = await json_data
-
- # Check if the model has parameter information
- parameters = json_data.get("parameters", {})
- # Some models might not have detailed parameter info, so we'll be conservative
- # and assume they support mirostat unless we know otherwise
- return True
-
- except (httpx.RequestError, ValueError) as e:
- await logger.awarning(f"Could not check mirostat support for model {model_name}: {e}")
- # If we can't check, assume it supports mirostat to avoid breaking functionality
- return TrueOption 2: Implement actual validation logic (requires knowledge of how Ollama exposes parameter support):
# Check if mirostat is in supported parameters
supported_params = json_data.get("parameters", "").lower()
return "mirostat" in supported_params🧰 Tools
🪛 GitHub Check: Ruff Style Check (3.13)
[failure] 388-388: Ruff (F841)
src/lfx/src/lfx/components/ollama/ollama.py:388:17: F841 Local variable parameters is assigned to but never used
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
1 participant
pr.mp4
Problematic flow: ollama-agent.json
This PR fixes a couple of issues (thanks to @philnash for reporting)
New Error Reporting
I killed the Ollama server while running a flow and saw a cleaner error:
Summary by CodeRabbit
Release Notes
New Features
Improvements