fix: correct rule YAML format and improve PR creation error handling #32
Conversation
- Fix rule generation to use parameters directly instead of validators array - Use correct parameter names: source_patterns/test_patterns for related_tests validator - Use min_description_length validator for PR body validation - Wrap rules in rules: top-level key in YAML output - Improve PR creation error handling with branch existence checks - Add structured logging for all failure points in PR creation flow - Validate PR URL before returning response to prevent 404 errors
Summary of ChangesHello @naaa760, 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 reliability and correctness of the system's rule generation and pull request creation functionalities. It rectifies an issue where generated rule YAML was malformed, leading to engine incompatibility, and addresses silent failures during PR creation by implementing robust error handling, branch existence checks, and detailed logging, ensuring a more stable and predictable workflow. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
|
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (11.9%) is below the target coverage (80.0%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #32 +/- ##
=======================================
- Coverage 33.3% 33.2% -0.2%
=======================================
Files 85 85
Lines 5048 5082 +34
=======================================
+ Hits 1684 1688 +4
- Misses 3364 3394 +30 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![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 pull request refactors how default recommendations are defined in src/agents/repository_analysis_agent/nodes.py, moving from explicit validators and actions to a more parameterized structure for rules like "include or update tests" and "require description in PR body," and also updates the YAML rendering logic to produce a structured list of rules. Additionally, it enhances the proceed_with_pr API endpoint in src/api/recommendations.py by adding comprehensive structured logging for various failure scenarios and improving branch creation logic to gracefully handle cases where a branch already exists. The create_git_ref function in src/integrations/github/api.py was updated to return detailed ref data and include more robust error handling for branch creation, specifically addressing a bug where the response body was consumed twice in certain 422 error cases, leading to incomplete error logs.
| if response.status == 422: | ||
| error_data = await response.json() | ||
| if "already exists" in error_data.get("message", "").lower(): | ||
| # Branch exists - verify it's the same SHA | ||
| existing_ref = await self.get_git_ref_sha(repo_full_name, ref_clean, installation_id, user_token) | ||
| if existing_ref == sha: | ||
| logger.info(f"Branch {ref_clean} already exists with same SHA, continuing") | ||
| return {"ref": f"refs/heads/{ref_clean}", "object": {"sha": sha}} |
There was a problem hiding this comment.
There is a bug in the error handling logic. If the response status is 422, await response.json() is called. However, if the idempotency check fails (i.e., the branch exists with a different SHA), the code falls through to line 792 and attempts to read the response body again with await response.text(), which will fail because the body has already been consumed. This leads to incomplete error logs.
To fix this, you should handle the non-idempotent 422 error case completely within its block by logging the error_data and returning None, preventing the fall-through.
| if response.status == 422: | |
| error_data = await response.json() | |
| if "already exists" in error_data.get("message", "").lower(): | |
| # Branch exists - verify it's the same SHA | |
| existing_ref = await self.get_git_ref_sha(repo_full_name, ref_clean, installation_id, user_token) | |
| if existing_ref == sha: | |
| logger.info(f"Branch {ref_clean} already exists with same SHA, continuing") | |
| return {"ref": f"refs/heads/{ref_clean}", "object": {"sha": sha}} | |
| if response.status == 422: | |
| error_data = await response.json() | |
| if "already exists" in error_data.get("message", "").lower(): | |
| # Branch exists - verify it's the same SHA | |
| existing_ref = await self.get_git_ref_sha(repo_full_name, ref_clean, installation_id, user_token) | |
| if existing_ref == sha: | |
| logger.info(f"Branch {ref_clean} already exists with same SHA, continuing") | |
| return {"ref": f"refs/heads/{ref_clean}", "object": {"sha": sha}} | |
| logger.error(f"Failed to create branch {ref_clean}: {response.status} - {error_data}") | |
| return None |
There was a problem hiding this comment.
Is this still the case? Should we push a follow up commit to resolve this? @naaa760
- Fix bug where response.json() consumes body, then response.text() fails - Handle 422 error cases completely within their block - Log error_data instead of attempting to read consumed response body - Prevents fall-through to response.text() call
| - type: warn | ||
| parameters: | ||
| message: "Please include or update tests for code changes." | ||
| parameters: |
There was a problem hiding this comment.
Do these hardcoded parameters limit the range of cases the agent can support? For example, can the agent enforce a rule that prevents a specific member from making changes to a specific path or module (e.g. /auth)? @naaa760
|
@naaa760 please check the pre-commit checks |
- Replace hardcoded file patterns with language-specific patterns - Add _get_language_specific_patterns() to adapt patterns to repository language - Document limitation: author+path restrictions require combined validators - Change strategy from 'static' to 'hybrid' since we use repository analysis Addresses code review feedback about hardcoded parameters limiting flexibility.
3 participants
Problem
validators:array format not supported by rule engineSolution
parameters:directly (removedvalidators:array)source_patterns/test_patternsforrelated_tests,min_description_lengthfor PR body)rules:top-level keyFiles Changed
src/agents/repository_analysis_agent/nodes.py- Fixed rule generationsrc/api/recommendations.py- Improved error handlingsrc/integrations/github/api.py- Enhanced branch creation