-
Notifications
You must be signed in to change notification settings - Fork 26
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
Changes from 1 commit
f769dbf
51fe312
3ab5f48
63d19cd
45c7ff3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -48,7 +48,9 @@ async def recommend_rules( | |||||
| if not request.repository_full_name or "/" not in request.repository_full_name: | ||||||
| raise HTTPException(status_code=400, detail="Invalid repository name format. Expected 'owner/repo'") | ||||||
|
|
||||||
| cache_key = f"repo_analysis:{request.repository_full_name}" | ||||||
| # Include authentication context in cache key to ensure different access levels get different results | ||||||
| auth_context = request.installation_id or request.user_token or "anonymous" | ||||||
| cache_key = f"repo_analysis:{request.repository_full_name}:{auth_context}" | ||||||
| cached_result = await get_cache(cache_key) | ||||||
|
|
||||||
| if cached_result: | ||||||
|
|
@@ -57,6 +59,7 @@ async def recommend_rules( | |||||
| "cache_hit", | ||||||
| operation="repository_analysis", | ||||||
| subject_ids=[request.repository_full_name], | ||||||
| auth_context=auth_context, | ||||||
| cached=True, | ||||||
| ) | ||||||
| return RepositoryAnalysisResponse(**cached_result) | ||||||
|
|
@@ -85,6 +88,8 @@ async def recommend_rules( | |||||
| decision="failed", | ||||||
| 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 commentThe reason will be displayed to describe this comment to others. Learn more. There's a critical issue with this cache clearing attempt. The
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @naaa760 any thougths? |
||||||
| raise HTTPException(status_code=500, detail=result.message) | ||||||
|
|
||||||
| analysis_response = result.data.get("analysis_response") | ||||||
|
|
@@ -161,6 +166,18 @@ async def proceed_with_pr(request: ProceedWithPullRequestRequest) -> ProceedWith | |||||
| branch=request.branch_name, | ||||||
| existing_sha=existing_branch_sha, | ||||||
| ) | ||||||
| # Verify the branch points to the correct base | ||||||
| if existing_branch_sha != base_sha: | ||||||
| log_structured( | ||||||
| logger, | ||||||
| "branch_sha_mismatch", | ||||||
| operation="proceed_with_pr", | ||||||
| subject_ids=[repo], | ||||||
| branch=request.branch_name, | ||||||
| existing_sha=existing_branch_sha, | ||||||
| expected_sha=base_sha, | ||||||
| warning="Branch exists but points to different SHA than base branch", | ||||||
| ) | ||||||
| else: | ||||||
| # Create new branch | ||||||
| created_ref = await github_client.create_git_ref(repo, request.branch_name, base_sha, **auth_ctx) | ||||||
|
|
@@ -182,6 +199,15 @@ async def proceed_with_pr(request: ProceedWithPullRequestRequest) -> ProceedWith | |||||
| "The branch may already exist or you may not have permission to create branches." | ||||||
| ), | ||||||
| ) | ||||||
| log_structured( | ||||||
| logger, | ||||||
| "branch_created", | ||||||
| operation="proceed_with_pr", | ||||||
| subject_ids=[repo], | ||||||
| branch=request.branch_name, | ||||||
| base_branch=base_branch, | ||||||
| new_sha=created_ref.get("object", {}).get("sha"), | ||||||
| ) | ||||||
|
|
||||||
| file_result = await github_client.create_or_update_file( | ||||||
| repo_full_name=repo, | ||||||
|
|
@@ -209,6 +235,17 @@ async def proceed_with_pr(request: ProceedWithPullRequestRequest) -> ProceedWith | |||||
| ), | ||||||
| ) | ||||||
|
|
||||||
| commit_sha = (file_result.get("commit") or {}).get("sha") | ||||||
| log_structured( | ||||||
| logger, | ||||||
| "file_created", | ||||||
| operation="proceed_with_pr", | ||||||
| subject_ids=[repo], | ||||||
| branch=request.branch_name, | ||||||
| file_path=request.file_path, | ||||||
| commit_sha=commit_sha, | ||||||
| ) | ||||||
|
|
||||||
| pr = await github_client.create_pull_request( | ||||||
| repo_full_name=repo, | ||||||
| title=request.pr_title, | ||||||
|
|
@@ -237,16 +274,63 @@ async def proceed_with_pr(request: ProceedWithPullRequestRequest) -> ProceedWith | |||||
| ) | ||||||
|
|
||||||
| pr_url = pr.get("html_url", "") | ||||||
| if not pr_url: | ||||||
| pr_number = pr.get("number") | ||||||
| if not pr_url or not pr_number: | ||||||
| log_structured( | ||||||
| logger, | ||||||
| "pr_url_missing", | ||||||
| "pr_creation_incomplete", | ||||||
| operation="proceed_with_pr", | ||||||
| subject_ids=[repo], | ||||||
| pr_data=pr, | ||||||
| error="PR created but html_url is missing", | ||||||
| pr_url=pr_url, | ||||||
| pr_number=pr_number, | ||||||
| error="PR creation response missing required fields", | ||||||
| ) | ||||||
| raise HTTPException(status_code=500, detail="PR was created but response is incomplete") | ||||||
|
|
||||||
| # Validate the PR URL is a proper GitHub URL format | ||||||
| if not pr_url.startswith("https://github.com/") or "/pull/" not in pr_url: | ||||||
| log_structured( | ||||||
| logger, | ||||||
| "pr_url_invalid", | ||||||
| operation="proceed_with_pr", | ||||||
| subject_ids=[repo], | ||||||
| pr_url=pr_url, | ||||||
| pr_number=pr_number, | ||||||
| error="PR URL is not a valid GitHub pull request URL", | ||||||
| ) | ||||||
| raise HTTPException(status_code=500, detail="PR was created but returned invalid URL format") | ||||||
|
|
||||||
| # Validate PR number is reasonable | ||||||
| if not isinstance(pr_number, int) or pr_number <= 0: | ||||||
| log_structured( | ||||||
| logger, | ||||||
| "pr_number_invalid", | ||||||
| operation="proceed_with_pr", | ||||||
| subject_ids=[repo], | ||||||
| pr_url=pr_url, | ||||||
| pr_number=pr_number, | ||||||
| error="PR number is invalid", | ||||||
| ) | ||||||
| raise HTTPException(status_code=500, detail="PR was created but returned invalid PR number") | ||||||
|
|
||||||
| # Final validation before returning success | ||||||
| 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 commentThe reason will be displayed to describe this comment to others. Learn more. These variables
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @naaa760 here too... |
||||||
|
|
||||||
| # Double-check URL format one more time | ||||||
| expected_url_pattern = f"https://github.com/{repo}/pull/{final_pr_number}" | ||||||
| if final_pr_url != expected_url_pattern: | ||||||
| log_structured( | ||||||
| logger, | ||||||
| "pr_url_mismatch", | ||||||
| operation="proceed_with_pr", | ||||||
| subject_ids=[repo], | ||||||
| expected_url=expected_url_pattern, | ||||||
| actual_url=final_pr_url, | ||||||
| pr_number=final_pr_number, | ||||||
| warning="PR URL doesn't match expected pattern", | ||||||
| ) | ||||||
|
Comment on lines
+319
to
332
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great series of validation steps to ensure the PR URL is correct. However, in this final check, if the A mismatch here, even if the URL seems valid, is a strong indicator of a problem (e.g., an unexpected API response, a repository rename, etc.). It could lead to returning a URL that points to the wrong place or requires a redirect. To make this more robust and prevent potentially confusing behavior for the user, I recommend raising an if pr_url != expected_url_pattern:
log_structured(
logger,
"pr_url_mismatch",
operation="proceed_with_pr",
subject_ids=[repo],
expected_url=expected_url_pattern,
actual_url=pr_url,
pr_number=pr_number,
error="PR URL doesn't match expected pattern",
)
raise HTTPException(
status_code=500, detail=f"PR URL mismatch: expected {expected_url_pattern} but got {pr_url}"
)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @naaa760 please check this too ^ |
||||||
| raise HTTPException(status_code=500, detail="PR was created but URL is missing") | ||||||
|
|
||||||
| log_structured( | ||||||
| logger, | ||||||
|
|
@@ -255,11 +339,12 @@ async def proceed_with_pr(request: ProceedWithPullRequestRequest) -> ProceedWith | |||||
| subject_ids=[repo], | ||||||
| decision="success", | ||||||
| branch=request.branch_name, | ||||||
| pr_number=pr.get("number"), | ||||||
| pr_number=final_pr_number, | ||||||
| pr_url=final_pr_url, | ||||||
| ) | ||||||
|
|
||||||
| return ProceedWithPullRequestResponse( | ||||||
| pull_request_url=pr.get("html_url", ""), | ||||||
| pull_request_url=final_pr_url, | ||||||
| branch_name=request.branch_name, | ||||||
| base_branch=base_branch, | ||||||
| file_path=request.file_path, | ||||||
|
|
||||||
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.
According to PEP 8 style guidelines, imports should be placed at the top of the file. Please move
import loggingto the top level of the module to improve readability and adhere to standard Python conventions.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