-
Notifications
You must be signed in to change notification settings - Fork 14
fix: repository analysis caching and PR creation 404 issues #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: repository analysis caching and PR creation 404 issues #34
Conversation
…tion - Cache key now includes authentication context to prevent different users/installations from sharing cached results - Repository analysis now fails completely when it can't access GitHub data, instead of returning generic rules - Failed analyses clear their cache entries to ensure fresh attempts - Added validation to ensure repository language is determined before generating rules - ADDED BULLETPROOF PR URL VALIDATION: * Validates PR URL is proper GitHub format (https://github.com/owner/repo/pull/number) * Validates PR number is a positive integer * Validates URL matches expected pattern before returning success * NO MORE 404 REDIRECTS - only returns valid, working GitHub PR URLs
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 addresses critical issues in both repository analysis and pull request creation workflows. It refines the caching mechanism for repository analysis to ensure that recommendations are always specific to the authenticated user and the repository, preventing the premature return of generic rules. Concurrently, it fortifies the PR creation process by introducing comprehensive URL and PR number validation, guaranteeing that users are redirected to valid GitHub pull request pages after creation. These changes collectively enhance the reliability and accuracy of the system's interactions with GitHub. 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 (20.7%) is below the target coverage (80.0%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #34 +/- ##
=======================================
- Coverage 32.9% 32.9% -0.1%
=======================================
Files 85 85
Lines 5122 5163 +41
=======================================
+ Hits 1689 1699 +10
- Misses 3433 3464 +31 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses the reported issues with repository analysis caching and pull request creation. The introduction of authentication context into cache keys and robust URL validation for PRs are excellent fixes. I've identified one critical issue where the cache-clearing mechanism on failure is not working as intended due to a bug in how ttl=0 is handled, which could prevent fresh analysis on retries. Additionally, I have a couple of medium-severity suggestions to improve code style and remove redundancy. Overall, these are great improvements to the system's reliability.
| error=result.message, | ||
| ) | ||
| # Clear any cached results for this repository to ensure fresh analysis on retry | ||
| await set_cache(cache_key, None, ttl=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a critical issue with this cache clearing attempt. The set_cache function checks if ttl:, which evaluates to False for ttl=0. Consequently, this call caches a None value for the default duration (1 hour) instead of clearing the entry, preventing fresh analysis on retries. A proper fix would be in set_cache to check if ttl is not None:. As a workaround within this file, you can use a very small positive TTL to ensure near-immediate expiration.
| await set_cache(cache_key, None, ttl=0) | |
| await set_cache(cache_key, None, ttl=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naaa760 any thougths?
| Currently, validators like `author_team_is` and `file_patterns` operate independently. | ||
| """ | ||
| import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naaa760 import should on top
| final_pr_url = pr.get("html_url", "") | ||
| final_pr_number = pr.get("number") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables final_pr_url and final_pr_number are redundant. The variables pr_url and pr_number were already assigned from the same source on lines 276-277 and have been validated. You can remove these lines and use pr_url and pr_number in the subsequent logic (lines 322-333, 342-343, and 347) to avoid duplication and improve clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naaa760 here too...
|
Also, invalid rules schema still exists here |
Fixed repository analysis and PR creation issues
Repository Analysis Problem:
Repository Analysis Solution:
PR Creation Problem:
PR Creation Solution:
Result:
Testing: All tests pass, fixes ensure proper analysis and 404-free PR creation.