fix(server): wrap ModelContext initialization errors #317
Conversation
Problem: ModelContext(capabilities) can raise ValueError/SDK errors which bubble out of handle_call_tool as 500 responses. Impact: Users receive raw stack traces and server skips fallback model logic when a model is misconfigured or restricted. Solution: Catch ModelContext exceptions, log them, and rethrow ToolExecutionError with structured output so callers see actionable messages.
Summary of ChangesHello @DragonFSKY, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and user experience of the server by implementing comprehensive error handling for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello @DragonFSKY, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and user experience of the server by addressing critical error handling deficiencies in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that addresses a critical gap in error handling. Wrapping the ModelContext initialization in a try...except block to catch both specific and general exceptions significantly improves the server's robustness and the user experience. The use of ToolExecutionError and preserving the exception chain are great practices. I've added a couple of minor suggestions to enhance code consistency and maintainability.
| except ValueError as exc: | ||
| # Handle validation errors (e.g., restricted models, invalid format) | ||
| logger.error(f"Model context validation failed for '{model_name}': {exc}") | ||
| error_output = ToolOutput( | ||
| status="error", | ||
| content=str(exc), | ||
| content_type="text", | ||
| metadata={"tool_name": name, "requested_model": model_name}, | ||
| ) | ||
| raise ToolExecutionError(error_output.model_dump_json()) from exc | ||
| except Exception as exc: | ||
| # Handle unexpected errors (e.g., provider failures, SDK errors) | ||
| logger.exception("Model context setup failed for %s", model_name) | ||
| error_message = ( | ||
| f"Unable to initialize model context for '{model_name}'. " | ||
| "Please choose a different model or check provider configuration." | ||
| ) | ||
| error_output = ToolOutput( | ||
| status="error", | ||
| content=error_message, | ||
| content_type="text", | ||
| metadata={"tool_name": name, "requested_model": model_name}, | ||
| ) | ||
| raise ToolExecutionError(error_output.model_dump_json()) from exc |
There was a problem hiding this comment.
The logic for creating a ToolOutput and raising a ToolExecutionError is duplicated in both the ValueError and Exception blocks. To improve maintainability and adhere to the Don't Repeat Yourself (DRY) principle, consider refactoring this common pattern into a private helper function. This would centralize the error response creation, making the code cleaner and easier to modify in the future.
| raise ToolExecutionError(error_output.model_dump_json()) from exc | ||
| except Exception as exc: | ||
| # Handle unexpected errors (e.g., provider failures, SDK errors) | ||
| logger.exception("Model context setup failed for %s", model_name) |
There was a problem hiding this comment.
For consistency with the logging style used elsewhere in this file (including the ValueError handler you've added), it's recommended to use an f-string here instead of a C-style format string.
| logger.exception("Model context setup failed for %s", model_name) | |
| logger.exception(f"Model context setup failed for '{model_name}'") |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of unhandled exceptions during ModelContext initialization. By introducing a try...except block, you've made the server more resilient and user-friendly, preventing crashes and providing structured error messages. The distinction between ValueError and other exceptions is well-handled. I have one suggestion to refactor the exception handling blocks to reduce code duplication and improve maintainability.
| except ValueError as exc: | ||
| # Handle validation errors (e.g., restricted models, invalid format) | ||
| logger.error(f"Model context validation failed for '{model_name}': {exc}") | ||
| error_output = ToolOutput( | ||
| status="error", | ||
| content=str(exc), | ||
| content_type="text", | ||
| metadata={"tool_name": name, "requested_model": model_name}, | ||
| ) | ||
| raise ToolExecutionError(error_output.model_dump_json()) from exc | ||
| except Exception as exc: | ||
| # Handle unexpected errors (e.g., provider failures, SDK errors) | ||
| logger.exception("Model context setup failed for %s", model_name) | ||
| error_message = ( | ||
| f"Unable to initialize model context for '{model_name}'. " | ||
| "Please choose a different model or check provider configuration." | ||
| ) | ||
| error_output = ToolOutput( | ||
| status="error", | ||
| content=error_message, | ||
| content_type="text", | ||
| metadata={"tool_name": name, "requested_model": model_name}, | ||
| ) | ||
| raise ToolExecutionError(error_output.model_dump_json()) from exc |
There was a problem hiding this comment.
The except ValueError and except Exception blocks contain duplicated logic for creating the ToolOutput and raising the ToolExecutionError. To make the code more concise and maintainable (following the DRY principle), you could combine these into a single except Exception block and use isinstance(exc, ValueError) to handle the differences in logging and error message content.
This refactoring also makes the logging style consistent by using an f-string for logger.exception, matching the logger.error call.
except Exception as exc:
if isinstance(exc, ValueError):
# Handle validation errors (e.g., restricted models, invalid format)
logger.error(f"Model context validation failed for '{model_name}': {exc}")
error_content = str(exc)
else:
# Handle unexpected errors (e.g., provider failures, SDK errors)
logger.exception(f"Model context setup failed for '{model_name}'")
error_content = (
f"Unable to initialize model context for '{model_name}'. "
"Please choose a different model or check provider configuration."
)
error_output = ToolOutput(
status="error",
content=error_content,
content_type="text",
metadata={"tool_name": name, "requested_model": model_name},
)
raise ToolExecutionError(error_output.model_dump_json()) from exc
…ons#317) - Add try/except error handling for ModelContext initialization - Convert stack traces to friendly structured error messages - Session no longer crashes when model initialization fails
1 participant
fix(server): wrap ModelContext initialization errors
Summary
Fixes uncaught exceptions during ModelContext initialization that cause 500 Internal Server Errors and session crashes when users request invalid/restricted models.
Problem
The
handle_call_tool()function createsModelContextwithout exception handling:Exception Sources:
Impact:
Root Cause
ModelContextinitialization can raiseValueError(validation) orException(SDK failures), but these are not caught. The errors propagate as 500 errors with stack traces, providing poor user experience.Solution
Wrap
ModelContextinitialization in try/except to convert exceptions into structuredToolExecutionError:Benefits:
Test Plan
Test Case 1: Restricted Model
Test Case 2: Invalid Format
Test Case 3: Provider Failure
Changes
File Modified:
server.py(Lines 844-878)Before (8 lines):
After (35 lines):
Key Improvements:
_ = model_context.capabilitiesfrom excRelated Issues
Fixes unhandled exceptions in model context initialization (main branch).
Severity: High
Priority: P1
Checklist
./code_quality_checks.sh