fix: graceful error handling for empty/invalid Fara model responses#484
fix: graceful error handling for empty/invalid Fara model responses#484majiayu000 wants to merge 2 commits intomicrosoft:mainfrom
Conversation
Fixes microsoft#453 - Add validation for empty model responses with helpful error message - Add check for missing <tool_call> tag with debugging info - Handle case where <tool_call> is not followed by newline - Add check for empty tool call content - Improve error messages to help users diagnose model compatibility issues - Add comprehensive unit tests for _parse_thoughts_and_action method Signed-off-by: majiayu000 <[email protected]>
- Add Anthropic Claude as a model provider option in the UI - New AnthropicModelConfigForm component for configuring Claude models - Support for claude-sonnet-4, claude-3-5-sonnet, claude-3-5-haiku, claude-3-opus - Backend validation for Anthropic provider aliases - Updated autogen-ext dependency to include anthropic extra - Improve Fara web surfer error handling (PR microsoft#484) - Add validation for empty/whitespace-only model responses - Check for required <tool_call> tag before parsing - Handle tool calls with or without newline delimiters - Provide descriptive error messages with response previews - Fall back gracefully from JSON to ast.literal_eval parsing
There was a problem hiding this comment.
Pull request overview
This pull request fixes issue #453 where the Fara integration crashes with an IndexError when using LM Studio instead of vLLM, caused by empty or malformed model responses. The fix adds comprehensive validation and error handling to gracefully handle these cases with helpful error messages for users.
Changes:
- Added validation for empty/whitespace-only model responses with descriptive error messages
- Added check for missing
<tool_call>tag with response preview for debugging - Made the parser more flexible to handle
<tool_call>tags not followed by newlines - Added validation for empty tool call content
- Improved exception handling and error messages with response truncation
- Added 10 comprehensive unit tests covering all validation branches and edge cases
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_fara_web_surfer.py | New test file with 10 unit tests covering all validation logic, edge cases, and error handling in the parsing method |
| src/magentic_ui/agents/web_surfer/fara/_fara_web_surfer.py | Enhanced _parse_thoughts_and_action method with validation checks, improved parsing flexibility, and better error messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| action = json.loads(action_text) | ||
| except json.decoder.JSONDecodeError: | ||
| self.logger.error(f"Invalid action text: {action_text}") | ||
| self.logger.warning( | ||
| f"JSON decode failed, trying ast.literal_eval: {action_text}" | ||
| ) | ||
| action = ast.literal_eval(action_text) | ||
|
|
||
| return thoughts, action | ||
| except ValueError: | ||
| raise |
There was a problem hiding this comment.
The exception handling here could mask errors from ast.literal_eval. When ast.literal_eval raises a ValueError (e.g., due to malformed syntax), it will be re-raised at line 243 without the additional context added at lines 248-251. Consider catching specific exceptions from ast.literal_eval or restructuring to ensure all parsing failures include the helpful error message with response preview.
| if len(tmp) < 2: | ||
| response_preview = ( | ||
| f"{message[:200]}..." if len(message) > 200 else message | ||
| ) | ||
| raise ValueError( | ||
| f"Could not parse tool call from response: {response_preview}" | ||
| ) |
There was a problem hiding this comment.
This condition at line 216 is unreachable. Since we already verified that "<tool_call>" exists in the message at line 203, splitting by "<tool_call>" at line 215 will always produce at least 2 parts. This check serves no practical purpose and could be removed to reduce code complexity.
| if len(tmp) < 2: | |
| response_preview = ( | |
| f"{message[:200]}..." if len(message) > 200 else message | |
| ) | |
| raise ValueError( | |
| f"Could not parse tool call from response: {response_preview}" | |
| ) |
|
Tested: ============================= test session starts ============================== tests/test_fara_web_surfer.py ........... [100%] ============================== 11 passed in 2.81s ============================== (11 passed) |
1 participant
Summary
<tool_call>tag with debugging info<tool_call>is not followed by newlineTest plan
_parse_thoughts_and_actionmethodpoe fmt- formatting passespoe lint- linting passespoe pyright- type checking passes