fix: use webhook payload SHAs in list_changed_files to avoid race condition#1107
Conversation
Code Review by Qodo
Context used 1.
|
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
7ff7d45 to
92438ea
Compare
|
Code review by qodo was updated up to the latest commit 92438ea |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
Code review by qodo was updated up to the latest commit 5a40c0f |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 5a40c0f |
|
Code review by qodo was updated up to the latest commit 0afdbf7 |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
Code review by qodo was updated up to the latest commit 31f6655 |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
Code review by qodo was updated up to the latest commit c3a56ab |
…dition - Replace live PyGithub API calls with webhook payload SHAs for pull_request events - Fall back to API for non-PR events (issue_comment, check_run, etc.) - Store pr_base_sha/pr_head_sha on GithubWebhook instance during process() - Remove pull_request parameter from initialize() and list_changed_files() - Add symmetric guards for both base and head SHA validation Closes #1096
Declare pr_base_sha and pr_head_sha in __init__() with empty string defaults so mypy strict mode has clear type declarations. Remove redundant inline annotations in process() since the class-level ones cover typing.
…h safety After fetching the PR ref, check if payload SHAs exist in the clone. If not (force-push race condition), fetch them explicitly from origin. This ensures git diff in list_changed_files() always has valid SHAs.
- Validate payload SHAs match 40-char hex format before using in git commands - Check return code of git fetch for missing SHAs and log warning on failure - Helps diagnose downstream git diff failures from unreachable SHAs
Validate SHA format and type before use in git commands. Invalid SHAs are reset to empty string so the cat-file/fetch block is skipped and list_changed_files() falls back to API-sourced SHAs. Prevents TypeError on non-string payloads and unclear git diff errors from malformed SHAs.
c3a56ab to
4c96c95
Compare
|
/retest build-container |
1 similar comment
|
/retest build-container |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
Replace defensive isinstance checks on pull_request payload with a direct event type check (self.github_event == 'pull_request'). For pull_request events, base.sha and head.sha are guaranteed by the GitHub webhook spec, so no defensive checks are needed. For other events (issue_comment, check_run), fall back to the PullRequest API object as before.
|
Code review by qodo was updated up to the latest commit 14ccf9a |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
PR Summary by Qodo
Fix changed-files diff race by using webhook payload base/head SHAs
🐞 Bug fix🧪 Tests🕐 20-40 MinutesWalkthroughs
User Description
Summary
Replace live PyGithub API calls with webhook payload SHAs in
list_changed_files()to eliminate a race condition where base branch receives new commits between clone and API call.pull_requestevents (no race condition)pr_base_sha/pr_head_shaon GithubWebhook instance duringprocess()pull_requestparameter frominitialize()andlist_changed_files()Closes #1096
AI Description
Diagram
graph TD A["GitHub webhook payload"] --> B["GithubWebhook.process"] --> C["Store PR base/head SHAs"] --> D[("Local clone")] --> E["OwnersFileHandler.list_changed_files"] --> F["git diff --name-only"] B --> G["PullRequest API (fallback)"] --> CHigh-Level Assessment
Using webhook payload SHAs for pull_request events is the most reliable way to keep the local clone and diff base/head aligned and eliminate the observed race. Alternatives like always querying live PR/base refs or diffing against the current base branch would reintroduce timing drift; passing SHAs through additional parameters instead of storing on the per-request GithubWebhook instance would add plumbing without changing the core correctness.
File Changes
Bug fix (2)
Tests (2)