fix(container): resolve worktree conflict when building merged PRs#909
fix(container): resolve worktree conflict when building merged PRs#909
Conversation
When a PR is merged, the repository is cloned with the target branch (e.g., main) already checked out. Attempting to create a worktree for the same branch failed with "fatal: 'main' is already used by worktree". Changes: - Check if target branch is already checked out before creating worktree - Use main clone directly when branch matches (no worktree needed) - Add fail-fast error handling to abort when repo preparation fails - Remove unused container_utils.py and its test (duplicate code) Fixes container build failures for merged PRs (hook 706b7b50-c3e3-11f0-92cc-fcdca537e918)
Added 4 new tests to cover the worktree conflict detection logic: - test_checkout_worktree_branch_already_checked_out - test_checkout_worktree_branch_already_checked_out_with_origin_prefix - test_checkout_worktree_different_branch - test_run_build_container_prepare_failure Coverage increased from 89.88% to 90.10% (above 90% requirement)
WalkthroughRefactoring of worktree and container build logic: added a pre-check in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
webhook_server/libs/handlers/runner_handler.py (1)
93-114: Branch reuse optimization in_checkout_worktreelooks good; consider tightening normalization & freshness checksThe short‑circuit on matching
current_branchvscheckout_targetis a clean way to avoid the “already used by worktree” error and matches the merged‑PR use case well.A couple of small nits you might want to consider:
- To avoid surprising behavior if a branch name itself contains
"origin/"somewhere in the middle, it’s slightly safer to strip only a leading prefix instead of a blindreplace:target = checkout_target.replace("origin/", "")
if checkout_target.startswith("origin/"):target = checkout_target[len("origin/") :]else:target = checkout_target
- When we reuse
repo_dirinstead of going throughgit_worktree_checkout, we also skip whatever fetch/refresh logic that helper might encapsulate. Please double‑check thatclone_repo_diris always kept up to date before calling_checkout_worktreein the merged‑branch path so we don’t accidentally build/test against a stale base branch.webhook_server/tests/test_runner_handler.py (2)
900-975: New_checkout_worktreetests nicely pin the branch‑reuse behavior; you could also assert worktree creation/avoidance explicitlyThese tests do a good job of exercising:
- reuse of the main clone when
checkout="main"and whencheckout="origin/main", and- falling back to
git_worktree_checkoutwhen targeting a different branch.If you want to make them even more robust against regressions, consider patching
webhook_server.utils.helpers.git_worktree_checkoutin the “branch already checked out” cases and asserting it is not called, while keeping the existing assertion intest_checkout_worktree_different_branchthat it is called. That would explicitly lock in the intended contract of “reuse main clone vs create worktree”.
976-1011: Container build prepare‑failure test is good; avoid hard‑coding source line numbers in commentsThe new
test_run_build_container_prepare_failurecleanly verifies that:
- we mark the build check as in‑progress,
- we record a failure when checkout fails, and
- we do not invoke
run_podman_command.The in‑body comment referencing “lines 385‑392” in the implementation is brittle, though; it will drift as the file evolves. I’d rephrase it to describe the behavior (“tests early return on repository preparation failure”) without tying it to specific line numbers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
webhook_server/libs/handlers/runner_handler.py(2 hunks)webhook_server/tests/test_container_utils.py(0 hunks)webhook_server/tests/test_runner_handler.py(1 hunks)webhook_server/utils/container_utils.py(0 hunks)
💤 Files with no reviewable changes (2)
- webhook_server/utils/container_utils.py
- webhook_server/tests/test_container_utils.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/tests/test_runner_handler.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
🧬 Code graph analysis (2)
webhook_server/tests/test_runner_handler.py (1)
webhook_server/libs/handlers/runner_handler.py (2)
_checkout_worktree(46-138)run_build_container(307-479)
webhook_server/libs/handlers/runner_handler.py (2)
webhook_server/utils/helpers.py (1)
run_command(270-378)webhook_server/libs/handlers/check_run_handler.py (1)
set_container_build_failure(196-197)
🪛 Ruff (0.14.5)
webhook_server/tests/test_runner_handler.py
962-962: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
972-972: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
1001-1001: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
🔇 Additional comments (1)
webhook_server/libs/handlers/runner_handler.py (1)
384-392: Early return on checkout failure inrun_build_containeris a solid fail‑fast improvementThe updated path correctly:
- logs and populates
output["text"],- records a build‑failure check only when
pull_requestandset_checkare truthy,- and then unconditionally returns so we don’t attempt a container build with a failed checkout.
This also keeps the existing production pattern where
push=Trueis paired withset_check=False, so push‑only flows still avoid check‑run updates while now safely aborting on repo prep failure. Based on learnings.
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
2 participants
When a PR is merged, the repository is cloned with the target branch
(e.g., main) already checked out. Attempting to create a worktree for
the same branch failed with "fatal: 'main' is already used by worktree".
Changes:
Fixes container build failures for merged PRs (hook 706b7b50-c3e3-11f0-92cc-fcdca537e918)
Summary by CodeRabbit
Performance Improvements
Bug Fixes