Added error handling #76
Conversation
|
/review-me |
|
Review in progress, it will be posted as soon as it is complete. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Error handling, improved you have. But room for refinement, there still is. Custom exceptions from ToolError inherit, they should. DRY principles, follow more closely you must. Common functions for repeated logic, extract you should. Specific exceptions, catch you must, instead of broad. Configuration file for hardcoded values, use you should. Return values from new functions, handle in calling code you must. Exit codes, document their meanings you should. These changes, make your code more robust and maintainable, they will.
Changes requested, I have. Significant improvements in error handling, seen I have. But critical issues, still present they are. Inherit from ToolError, custom exceptions must. DRY principles, violated in multiple places they are. Specific exceptions, catch you should, instead of broad. These issues, address you must, before merging this pull request we can.
Final Evaluation: REQUEST_CHANGES
| class AnthropicError(Exception): | ||
| """Custom exception for anthropic-related errors.""" | ||
| pass |
There was a problem hiding this comment.
Custom exception, created you have. But inherit from ToolError, it should. Consistency across the project, maintain we must.
| { | ||
| "role": "user", | ||
| "content": [ | ||
| { | ||
| "type": "text", | ||
| "text": prompt, | ||
| } | ||
| ], | ||
| } | ||
| ], |
There was a problem hiding this comment.
Duplicate code from chat_completion function, I see. Common function for message creation, extract you should. DRY principle, follow we must.
src/app.py
Outdated
| except GithubException as e: | ||
| logger.log.error(f"GitHub API error: {str(e)}") | ||
| raise | ||
| except Exception as e: | ||
| logger.log.error(f"Unexpected error in run method: {str(e)}") | ||
| raise |
There was a problem hiding this comment.
Logging before re-raising, wise it is. But consider, you should, if re-raising always necessary it is. Handle errors gracefully without stopping execution, you might.
| try: | ||
| comments = [comment.model_dump(exclude_none=True) for comment in comments or []] | ||
| logger.log.debug(f"Submitting review: {comments}") | ||
| review = pull_request.create_review( | ||
| body=body, | ||
| comments=comments, | ||
| event="COMMENT", | ||
| ) | ||
| logger.log.info(f"Review submitted successfully: {review.id}") | ||
| return True | ||
| except github.GithubException as e: | ||
| logger.log.error(f"GitHub API error while submitting review: {e}") | ||
| return False | ||
| except Exception as e: | ||
| logger.log.error(f"Unexpected error while submitting review: {e}") | ||
| return False |
There was a problem hiding this comment.
Return value, added you have to submit_review function. In calling code, handle this return value, you should. Important for error handling and user feedback, it is.
| cfg = config.from_env() | ||
| pr = code.pull_request.get_pr(cfg.github) | ||
| context = code.pull_request.build_pr_context(pr) | ||
| logger.log.debug(f"Context built successfully: {context.title}") | ||
| builder = ai.prompt.Builder(context) | ||
| assistant = ai.assistant.Assistant(cfg.llm, builder) | ||
|
|
||
| app = App(pr, context, assistant, debug=cfg.debug) | ||
|
|
||
| asyncio.run(app.run()) | ||
| except github.GithubException as e: | ||
| logger.log.critical(f"Exit with GithubException: {e}") | ||
| sys.exit(69) | ||
| except InvalidSeverityError as e: | ||
| logger.log.error(f"Exit with InvalidSeverityError: {e}") | ||
| sys.exit(255) | ||
| except ToolError as e: | ||
| logger.log.error(f"Exit with ToolError: {e}") | ||
| sys.exit(7) | ||
| except AnthropicError as e: | ||
| logger.log.errror(f"Exit with AnthropicError: {e}") | ||
| sys.exit(8) | ||
| except PromptError as e: | ||
| logger.log.error(f"Exit with PromptError: {e}") | ||
| sys.exit(135) | ||
| except ValueError as e: |
There was a problem hiding this comment.
Exit codes, specific they are. Document their meanings, you should. In comments or separate documentation, explain what each exit code represents. Understanding errors easier, this will make.
| logger.log.critical(f"Exit with Unexpected Exception: {e}") | ||
| if 'pr' in locals(): | ||
| code.pull_request.post_comment( | ||
| pr, | ||
| f"Sorry, I couldn't review your code because:\n" | ||
| f"```{traceback.format_exc()}```", | ||
| ) |
There was a problem hiding this comment.
Generic exception handling, broad it is. Consider more specific exception types, you should. Unexpected errors, easier to diagnose this will make.
|
/review-me |
|
Review in progress, it will be posted as soon as it is complete. |
There was a problem hiding this comment.
Error handling, improved you have, but consistent and specific it must be. DRY principle, follow you should. Catch specific exceptions, not broad ones. Log full tracebacks for debugging. Custom exceptions, create and use wisely. Refactor common error handling logic into helper functions. Large try-except blocks, break into smaller, focused ones. Standardize error handling patterns across all files. Return values on errors, reconsider you must - raise exceptions instead. Exit codes and log levels, map to exception types for consistency.
Request changes, I must. Significant issues in error handling consistency and specificity, there are. Improve code quality and maintainability, these changes will. Approve cannot I, until addressed these concerns are.
Final Evaluation: REQUEST_CHANGES
| class AnthropicError(Exception): | ||
| """Custom exception for anthropic-related errors.""" | ||
| pass |
There was a problem hiding this comment.
Custom exception, created you have. Wise decision, it is. But consider, you must, adding more specific error messages or attributes. Debugging, easier it will make.
| except (APIError, APITimeoutError, APIConnectionError) as e: | ||
| logger.log.error(f"Anthropic API error in chat_completion: {str(e)}") | ||
| raise AnthropicError(f"Anthropic API error: {str(e)}") | ||
| except Exception as e: | ||
| logger.log.error(f"Unexpected error in chat_completion: {str(e)}") |
There was a problem hiding this comment.
Error handling, improved it is. But log the full exception traceback, you should. Valuable information for debugging, it provides.
| except ToolError as e: | ||
| logger.log.error(f"Error in overview: {str(e)}") | ||
| raise | ||
| except Exception as e: | ||
| logger.log.error(f"Unexpected error in overview: {str(e)}") | ||
| raise |
There was a problem hiding this comment.
Generic Exception catching, broad it is. Specific exceptions, catch you should. More precise error handling, leads to better debugging.
| try: | ||
| completion = await tool_completion( | ||
| system_prompt=self._builder.render_template( | ||
| name="file-review", | ||
| prefix="system", | ||
| persona=self._persona, | ||
| ), | ||
| prompt=self._builder.render_template( | ||
| name="file-review", | ||
| prefix="user", | ||
| observations=observations, | ||
| file_context=context, | ||
| ), | ||
| model=self._model_name, | ||
| tools=[TOOLS["post_feedback"]], | ||
| tool_override="post_feedback", | ||
| ) | ||
| response = schema.FileReviewResponseModel(**completion) | ||
|
|
||
| return code.review.comment.extract_comments( | ||
| response=response, | ||
| file_context=context, | ||
| severity_limit=severity_limit, | ||
| ) | ||
| return code.review.comment.extract_comments( | ||
| response=response, | ||
| file_context=context, | ||
| severity_limit=severity_limit, |
There was a problem hiding this comment.
Similar structure to 'overview' method, I see. Refactor common error handling logic, you could. DRY principle, follow we must.
| try: | ||
| overview = await self._assistant.overview(self._context) | ||
| status = overview.initial_assessment.status | ||
| if status != code.review.model.Status.REVIEW_REQUIRED: | ||
| code.pull_request.submit_review( | ||
| pull_request=self._pr, | ||
| body=overview.initial_assessment.summary, | ||
| ) | ||
| return | ||
|
|
||
| observations = overview.observations | ||
| file_contexts = overview.file_contexts | ||
| logger.log.debug( | ||
| f"Files to review: \n" | ||
| f"- {"\n- ".join([context.path for context in file_contexts])}" | ||
| ) | ||
|
|
||
| prioritized_comments, remaining_comments = await self._review_files( | ||
| observations=observations, | ||
| contexts=file_contexts, | ||
| ) | ||
|
|
||
| feedback = await self._assistant.get_feedback( | ||
| prioritized_comments=prioritized_comments, | ||
| remaining_comments=remaining_comments, | ||
| ) | ||
| return | ||
|
|
||
| observations = overview.observations | ||
| file_contexts = overview.file_contexts | ||
| logger.log.debug( | ||
| f"Files to review: \n" | ||
| f"- {"\n- ".join([context.path for context in file_contexts])}" | ||
| ) | ||
|
|
||
| prioritized_comments, remaining_comments = await self._review_files( | ||
| observations=observations, | ||
| contexts=file_contexts, | ||
| ) | ||
|
|
||
| feedback = await self._assistant.get_feedback( | ||
| prioritized_comments=prioritized_comments, | ||
| remaining_comments=remaining_comments, | ||
| ) | ||
| logger.log.info(f"Overall Feedback: {feedback.evaluation}") | ||
|
|
||
| if self._debug: | ||
| logger.log.debug("Running in debug, no review submitted") | ||
| return | ||
|
|
||
| code.pull_request.submit_review( | ||
| pull_request=self._pr, | ||
| body=f"{feedback.overall_comment}\n\n" | ||
| f"{feedback.justification}\n" | ||
| f"Final Evaluation: {feedback.evaluation}", | ||
| comments=prioritized_comments, | ||
| ) | ||
| logger.log.info(f"Overall Feedback: {feedback.evaluation}") | ||
|
|
||
| if self._debug: | ||
| logger.log.debug("Running in debug, no review submitted") | ||
| return | ||
|
|
||
| if not code.pull_request.submit_review( | ||
| pull_request=self._pr, | ||
| body=f"{feedback.overall_comment}\n\n" | ||
| f"{feedback.justification}\n" | ||
| f"Final Evaluation: {feedback.evaluation}", | ||
| comments=prioritized_comments, | ||
| ): | ||
| code.pull_request.post_comment(self._pr, "Couldn't post review") | ||
| except GithubException as e: | ||
| logger.log.error(f"GitHub API error: {str(e)}") | ||
| except Exception as e: | ||
| logger.log.error(f"Unexpected error in run method: {str(e)}") |
There was a problem hiding this comment.
Large try-except block, problematic it is. Specific error handling for different operations, implement you should. Separate functions for overview, review, and feedback submission, create you must. Easier to maintain and debug, this approach will be.
| raise | ||
|
|
||
|
|
||
| def build_pr_context(pull_request: PullRequest) -> model.PullRequestContextModel: | ||
| files = pull_request.get_files() | ||
| context = model.PullRequestContextModel( | ||
| title=pull_request.title, | ||
| description=pull_request.body or "", | ||
| commit_messages=[ | ||
| commit.commit.message for commit in pull_request.get_commits() | ||
| ], | ||
| review_comments=[ | ||
| comment.body for comment in pull_request.get_review_comments() | ||
| ], | ||
| issue_comments=[comment.body for comment in pull_request.get_issue_comments()], | ||
| patches={ | ||
| file.filename: model.FilePatchModel( | ||
| filename=file.filename, | ||
| diff=file.patch or "", | ||
| hunks=[] if not file.patch else parse_diff(file.patch), | ||
| ) | ||
| for file in files | ||
| }, | ||
| added_files=[file.filename for file in files if file.status == "added"], | ||
| modified_files=[file.filename for file in files if file.status == "modified"], | ||
| deleted_files=[file.filename for file in files if file.status == "removed"], | ||
| ) | ||
| return context | ||
| def build_pr_context(pull_request: PullRequest) -> Optional[model.PullRequestContextModel]: | ||
| try: | ||
| files = pull_request.get_files() | ||
| context = model.PullRequestContextModel( | ||
| title=pull_request.title, | ||
| description=pull_request.body or "", | ||
| commit_messages=[ | ||
| commit.commit.message for commit in pull_request.get_commits() | ||
| ], | ||
| review_comments=[ | ||
| comment.body for comment in pull_request.get_review_comments() | ||
| ], | ||
| issue_comments=[comment.body for comment in pull_request.get_issue_comments()], | ||
| patches={ | ||
| file.filename: model.FilePatchModel( | ||
| filename=file.filename, | ||
| diff=file.patch or "", | ||
| hunks=[] if not file.patch else parse_diff(file.patch), | ||
| ) | ||
| for file in files | ||
| }, | ||
| added_files=[file.filename for file in files if file.status == "added"], | ||
| modified_files=[file.filename for file in files if file.status == "modified"], | ||
| deleted_files=[file.filename for file in files if file.status == "removed"], | ||
| ) | ||
| logger.log.info(f"PR Context built successfully: {context.title}") | ||
| return context | ||
| except github.GithubException as e: | ||
| logger.log.critical(f"GitHub API error while building PR context: {e}") | ||
| return None | ||
| except Exception as e: | ||
| logger.log.critical(f"Unexpected error while building PR context: {e}") |
There was a problem hiding this comment.
Return None on error, risky it is. Force caller to handle, we should. Raise custom exception, better approach it would be. Logging before return, redundant it seems. Move to exception handler, you could.
| except github.GithubException as e: | ||
| logger.log.critical(f"Exit with GithubException: {e}") | ||
| sys.exit(69) | ||
| except InvalidSeverityError as e: | ||
| logger.log.error(f"Exit with InvalidSeverityError: {e}") | ||
| sys.exit(255) | ||
| except ToolError as e: | ||
| logger.log.error(f"Exit with ToolError: {e}") | ||
| sys.exit(7) | ||
| except AnthropicError as e: | ||
| logger.log.errror(f"Exit with AnthropicError: {e}") | ||
| sys.exit(8) | ||
| except PromptError as e: | ||
| logger.log.error(f"Exit with PromptError: {e}") | ||
| sys.exit(135) | ||
| except ValueError as e: |
There was a problem hiding this comment.
Consistent error handling, you lack. For each exception type, log level and exit code differ. A mapping of exception types to exit codes and log levels, define you should. Consistency and maintainability, it improves.
| logger.log.critical(f"Exit with Unexpected Exception: {e}") | ||
| if 'pr' in locals(): | ||
| code.pull_request.post_comment( | ||
| pr, | ||
| f"Sorry, I couldn't review your code because:\n" | ||
| f"```{traceback.format_exc()}```", | ||
| ) |
There was a problem hiding this comment.
General exception handler, too broad it is. Specific exceptions, catch you should. Unexpected issues, harder to debug they become.
There was a problem hiding this comment.
Yeah, so I'm struggling with this a little. On one hand, I see what you're trying to do, but I'm not sure this is the way...
The two big patterns I'm seeing here are
- literally wrap the entire function in a try/except block and attempt to catch all possible errors in one place
- Re-raising errors using custom exception types
The first pattern is kinda brutal, and ideally we should not be throwing try/except blocks all over the place. I think the goal should be to have as few of them as possible, and make sure that we're really thinking about how the control flow would be affected by catching or re-raising an exception. Lots of try/except blocks can make execution very unpredictable, because any function could be catching a raised exception. We would do better to isolate the potential failures and think carefully about how those should be handled. I don't just want to throw try/except blocks at everything to cover for bad design.
The second pattern worries me because, while I appreciate the attempt to make the error traces appropriate for this application, it can also obfuscate things that are easier to see if from the actual exception thrown by the underlying code. You should look at the raise error from exception syntax at the very least.
No description provided.