perf(check-run): cache branch protection settings to reduce API calls #906
Conversation
Add instance variable caching for repository private status and branch required status checks to avoid repeated API calls within same webhook event. - Cache self._repository_private to avoid repeated repository.private checks - Cache self._branch_required_status_checks for branch protection settings - Saves ~1,300 API calls (1.4% reduction) by eliminating redundant get_branch() and get_protection() calls Closes: Task 1 of API reduction project (Archon: dd076ce5-ab33-45d5-bae2-b3df839a1080)
…e processing
Add automatic repository cloning for PR webhook events to enable reading
OWNERS files and changed files from local filesystem instead of GitHub API.
- Add _clone_repository_for_pr() method that clones repo and checks out base branch
- Add _repo_cloned boolean flag and cloned_repo_path string to track clone status
- Call clone method in process() after getting last_commit for all PR events
- Import run_command helper for git operations
- Clone to {clone_repo_dir}/repo for isolation from handler-specific clones
- Enables Tasks 3 and 4 to save ~1,280-2,080 API calls
This is infrastructure for OWNERS file local reading and git diff for changed
files, which will replace expensive GitHub API tree traversal operations.
Closes: Task 2 of API reduction project (Archon: 8f94afaf-8281-47c5-abc8-55c6f633a7c1)
Refactored repository cloning to use a single clone per webhook with git worktrees for isolated checkouts. This eliminates ~80% of clone operations and enables significant API call reduction. Key changes: - Clone repository once to clone_repo_dir (no /repo subdirectory) - All handlers reuse single clone via git worktrees - Created git_worktree_checkout() utility for reusable worktree logic - Renamed _prepare_cloned_repo_dir() to _checkout_worktree() - Fixed clone failure handling to abort webhook on error - Updated all 6 callsites to use new worktree-based approach Performance impact: - Eliminates 3-5 clones per webhook → 1 clone - Enables ~1,280-2,080 API call savings (OWNERS + changed files) - Faster processing with isolated worktrees vs full clones
Update all test mocks and assertions to use renamed function: - Renamed: _prepare_cloned_repo_dir → _checkout_worktree - Updated return signature: 3-tuple → 4-tuple (added worktree_path) - Fixed 38 failing tests across 3 test files Files changed: - webhook_server/tests/test_runner_handler.py - webhook_server/tests/test_push_handler.py - webhook_server/tests/test_github_api.py All 146 tests now passing.
Add 6 comprehensive tests for _clone_repository_for_pr: - test_clone_repository_for_pr_success: happy path - test_clone_repository_for_pr_already_cloned: early return - test_clone_repository_for_pr_clone_failure: RuntimeError on clone fail - test_clone_repository_for_pr_checkout_failure: RuntimeError on checkout fail - test_clone_repository_for_pr_git_config_warnings: non-critical failures - test_clone_repository_for_pr_general_exception: exception handling Coverage improvements: - webhook_server/libs/github_api.py: 79% → 90% - Overall coverage: 89.26% → 90.21% - Tests passing: 904/904
Fixed 5 failing tests by correcting async property mocking patterns: - Replaced AsyncMock instances with proper async functions for async properties - Fixed RuntimeWarnings in test_process_new_or_reprocess_pull_request_parallel_execution - Fixed RuntimeWarnings in test_process_new_or_reprocess_pull_request_exception_handling - Mocked asyncio.to_thread to avoid threading complexity in tests - Used proper async function mocks instead of AsyncMock for async cached properties All 904 tests now pass with 90.21% coverage.
WalkthroughAdds a shared repository clone/worktree flow with secret redaction, introduces git_worktree_checkout, moves OWNERS discovery to local filesystem reads from a checked-out worktree, adds branch/privacy caching, and updates handlers and tests to use worktree-based checkouts and propagated stdout/stderr. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webhook_server/tests/test_push_handler.py (1)
110-201: Patch_checkout_worktreeas an async context manager, not a plainMockAll the
upload_to_pypitests here patch_checkout_worktreewith a plainpatch.object(..., "_checkout_worktree")and only tweak__aenter__.return_value. Because the production code does:async with self.runner_handler._checkout_worktree(checkout=tag_name) as (success, worktree_path, out, err): ...the patched object must behave as an async context manager. With the current pattern,
__aenter__is a regularMock, soawait __aenter__()will raiseTypeError: 'Mock' object is not awaitable.You already use the correct pattern in
test_runner_handler.py; mirror it here. For example, fortest_upload_to_pypi_success:- with patch.object(push_handler.runner_handler, "_checkout_worktree") as mock_checkout: + with patch.object(push_handler.runner_handler, "_checkout_worktree", new_callable=AsyncMock) as mock_checkout: with patch( "webhook_server.libs.handlers.push_handler.run_command", new_callable=AsyncMock ) as mock_run_command: with patch("webhook_server.libs.handlers.push_handler.send_slack_message") as mock_slack: - # Mock successful checkout - mock_checkout.return_value.__aenter__.return_value = (True, "/tmp/worktree-path", "", "") + # Mock successful checkout + mock_checkout.return_value.__aenter__ = AsyncMock( + return_value=(True, "/tmp/worktree-path", "", "") + ) + mock_checkout.return_value.__aexit__ = AsyncMock(return_value=None)Apply the same
AsyncMockpattern to the other tests in this file that patch_checkout_worktreeso they accurately simulate the async context manager behavior and avoid awaitability errors.Also applies to: 205-309, 311-392
webhook_server/libs/github_api.py (1)
123-127: Centralized PR clone flow is solid, but tighten error logging and docstringThe new
clone_repo_dir+_repo_cloned+_clone_repository_for_pr()flow gives you a single on‑disk clone per webhook and lets all handlers share worktrees off that clone, which fits nicely with your API‑reduction/perf goals and avoids theowner.email‑may‑be‑None issue by usingrepository.owner.loginfor git config. Based on learnings.Two improvements to consider:
Avoid leaking the token in clone error logs
You correctly pass
redact_secrets=[github_token]intorun_command, so the internal command‑execution logs are redacted. But in failure paths you also do:self.logger.error("... Failed to clone repository: %s", err) ... self.logger.error("... Failed to checkout base branch %s: %s", base_branch, err)where
erris the raw stderr string returned byrun_command. If git ever echoes the tokenized URL (https://<token>@github.com/...) into stderr, these messages can re‑introduce the token into your logs.A safer pattern would be to run
errthrough the same redaction before logging, e.g. via a small helper around_redact_secrets(err, [github_token]), or by logging only high‑level context and omitting raw stderr in those outer messages.Update the
__del__docstring to match the new worktree designThe destructor docstring still refers to “subdirectories (created with -uuid4() suffix) cleaned up by
_prepare_cloned_repo_dir”, which no longer exists. Now you have:
- A single
clone_repo_dircreated bytempfile.mkdtemp(...), cleaned up in__del__.- Worktrees created via
git_worktree_checkout, which handles its own cleanup.Updating the docstring to describe this new split will make future maintenance less confusing.
These are relatively small adjustments on top of an otherwise well‑structured clone/worktree pipeline.
Also applies to: 171-273, 351-353, 663-671
🧹 Nitpick comments (2)
webhook_server/tests/test_pull_request_handler.py (1)
24-26: Async owners-data mocking and new parallel/exception tests look soundThe new
_mock_owners_data_for_changed_fileshelper and its usage (both viaAsyncMock(return_value={})and via a pre‑created coroutine assigned toowners_data_for_changed_files) give_check_if_pr_approveda awaitable that resolves to{}, which matches the async‑cached‑property semantics you’re simulating. The new parallel and exception‑handling tests aroundprocess_new_or_reprocess_pull_requestalso correctly patchasyncio.to_thread/asyncio.gatherand verify that failures in one task don’t block the automerge step.If you want to tighten things up later, you could standardize on one mocking style (e.g., always
AsyncMock(return_value={})) forowners_data_for_changed_filesto make the tests more uniform, but behaviorally this all looks good.Also applies to: 616-617, 642-718, 1329-1418
webhook_server/libs/handlers/runner_handler.py (1)
50-114: Clean refactoring to worktree-based checkouts.The new
_checkout_worktreemethod properly leverages the centralized clone and creates isolated worktrees for handler operations. The checkout target determination logic covers all scenarios, and the conditional merge for PR testing is appropriate.Minor: Remove the unnecessary
noqadirective on Line 71—the PLC0415 rule is not enabled.- from webhook_server.utils.helpers import git_worktree_checkout # noqa: PLC0415 + from webhook_server.utils.helpers import git_worktree_checkout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
webhook_server/libs/github_api.py(4 hunks)webhook_server/libs/handlers/check_run_handler.py(2 hunks)webhook_server/libs/handlers/push_handler.py(1 hunks)webhook_server/libs/handlers/runner_handler.py(6 hunks)webhook_server/tests/test_github_api.py(4 hunks)webhook_server/tests/test_pull_request_handler.py(4 hunks)webhook_server/tests/test_push_handler.py(7 hunks)webhook_server/tests/test_runner_handler.py(16 hunks)webhook_server/utils/helpers.py(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
📚 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/push_handler.pywebhook_server/tests/test_push_handler.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2025-05-10T21:39:34.243Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:281-308
Timestamp: 2025-05-10T21:39:34.243Z
Learning: In the refactored codebase, the `run_podman_command` method in `RunnerHandler` returns a tuple `(bool, str, str)` where the boolean `True` represents success and `False` represents failure, which is the opposite of traditional shell exit codes.
Applied to files:
webhook_server/tests/test_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/tests/test_github_api.pywebhook_server/tests/test_push_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2024-10-15T10:37:45.791Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 598
File: webhook_server_container/libs/github_api.py:1860-1874
Timestamp: 2024-10-15T10:37:45.791Z
Learning: In the `process_retest_command` method in `webhook_server_container/libs/github_api.py`, `_target_tests` is defined before use.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2024-10-14T14:12:28.924Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1638-1639
Timestamp: 2024-10-14T14:12:28.924Z
Learning: In `webhook_server_container/libs/github_api.py`, it is acceptable for `self.repository.owner.email` to be `None` since internal commands can handle this case.
Applied to files:
webhook_server/libs/handlers/check_run_handler.py
📚 Learning: 2024-10-07T16:21:03.462Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 579
File: webhook_server_container/libs/github_api.py:1098-1101
Timestamp: 2024-10-07T16:21:03.462Z
Learning: In the Python method `_run_tox` within `webhook_server_container/libs/github_api.py`, the variable `_tox_tests` is already comma-separated, so removing spaces with `_tox_tests.replace(" ", "")` is appropriate to handle any accidental spaces when specifying Tox environments.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-15T13:18:38.590Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 602
File: webhook_server_container/libs/github_api.py:2053-2062
Timestamp: 2024-10-15T13:18:38.590Z
Learning: In the `run_podman_command` method in `webhook_server_container/libs/github_api.py`, the `rc` variable is a boolean that returns `True` when the command succeeds and `False` when it fails.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-29T10:42:50.163Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-10-14T14:13:21.316Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1632-1637
Timestamp: 2024-10-14T14:13:21.316Z
Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.
Applied to files:
webhook_server/libs/github_api.py
🧬 Code graph analysis (9)
webhook_server/tests/test_runner_handler.py (3)
webhook_server/libs/handlers/runner_handler.py (2)
RunnerHandler(31-706)_checkout_worktree(50-114)webhook_server/tests/test_pull_request_handler.py (1)
mock_pull_request(73-92)webhook_server/tests/conftest.py (2)
pull_request(115-116)github_webhook(120-140)
webhook_server/tests/test_github_api.py (1)
webhook_server/libs/github_api.py (3)
process(275-468)GithubWebhook(50-677)_clone_repository_for_pr(171-273)
webhook_server/libs/handlers/push_handler.py (2)
webhook_server/libs/handlers/runner_handler.py (1)
_checkout_worktree(50-114)webhook_server/libs/handlers/check_run_handler.py (1)
get_check_run_text(299-325)
webhook_server/tests/test_pull_request_handler.py (3)
webhook_server/tests/test_runner_handler.py (1)
mock_pull_request(53-63)webhook_server/libs/handlers/pull_request_handler.py (1)
process_new_or_reprocess_pull_request(1140-1191)webhook_server/tests/conftest.py (1)
pull_request(115-116)
webhook_server/utils/helpers.py (1)
webhook_server/libs/handlers/runner_handler.py (1)
mask_sensitive(45-47)
webhook_server/tests/test_push_handler.py (1)
webhook_server/libs/handlers/push_handler.py (2)
upload_to_pypi(77-171)PushHandler(16-171)
webhook_server/libs/handlers/check_run_handler.py (2)
webhook_server/tests/test_check_run_handler.py (2)
get_branch(585-586)get_protection(588-589)webhook_server/tests/conftest.py (2)
pull_request(115-116)base(104-105)
webhook_server/libs/handlers/runner_handler.py (3)
webhook_server/utils/helpers.py (2)
git_worktree_checkout(596-652)run_command(270-378)webhook_server/libs/github_api.py (1)
get_pull_request(556-604)webhook_server/libs/handlers/check_run_handler.py (1)
get_check_run_text(299-325)
webhook_server/libs/github_api.py (3)
webhook_server/utils/helpers.py (2)
run_command(270-378)format_task_fields(117-136)webhook_server/tests/conftest.py (2)
PullRequest(99-111)base(104-105)webhook_server/libs/config.py (1)
get_value(99-120)
🪛 Ruff (0.14.4)
webhook_server/tests/test_runner_handler.py
144-144: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
165-165: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
188-188: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
211-211: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
245-245: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
293-293: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
324-324: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
367-367: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
396-396: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
646-646: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
663-663: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
684-684: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
701-701: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
711-711: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
732-732: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
750-750: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
768-768: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
779-779: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
815-815: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
870-870: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
889-889: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
webhook_server/tests/test_github_api.py
1038-1038: Unused function argument: args
(ARG001)
1038-1038: Unused function argument: kwargs
(ARG001)
1079-1079: Unused function argument: command
(ARG001)
1079-1079: Unused function argument: kwargs
(ARG001)
1132-1132: Unused function argument: args
(ARG001)
1132-1132: Unused function argument: kwargs
(ARG001)
1166-1166: Unused function argument: kwargs
(ARG001)
1186-1186: Unused function argument: args
(ARG001)
1186-1186: Unused function argument: kwargs
(ARG001)
1227-1227: Unused function argument: kwargs
(ARG001)
1250-1250: Unused function argument: args
(ARG001)
1250-1250: Unused function argument: kwargs
(ARG001)
1291-1291: Unused function argument: kwargs
(ARG001)
1323-1323: Unused function argument: args
(ARG001)
1323-1323: Unused function argument: kwargs
(ARG001)
1357-1357: Unused function argument: command
(ARG001)
1357-1357: Unused function argument: kwargs
(ARG001)
1358-1358: Avoid specifying long messages outside the exception class
(TRY003)
webhook_server/tests/test_pull_request_handler.py
1363-1363: Unused function argument: args
(ARG001)
1363-1363: Unused function argument: kwargs
(ARG001)
1365-1365: Create your own exception
(TRY002)
1365-1365: Avoid specifying long messages outside the exception class
(TRY003)
1367-1367: Unused function argument: args
(ARG001)
1367-1367: Unused function argument: kwargs
(ARG001)
1371-1371: Unused function argument: args
(ARG001)
1371-1371: Unused function argument: kwargs
(ARG001)
1390-1390: Unused function argument: args
(ARG001)
1390-1390: Unused function argument: kwargs
(ARG001)
webhook_server/utils/helpers.py
650-650: Do not catch blind exception: Exception
(BLE001)
webhook_server/tests/test_push_handler.py
116-116: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
145-145: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
166-166: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
187-187: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
211-211: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
236-236: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
263-263: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
287-287: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
318-318: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
343-343: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
369-369: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
webhook_server/libs/handlers/runner_handler.py
71-71: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
webhook_server/libs/github_api.py
208-208: Abstract raise to an inner function
(TRY301)
208-208: Avoid specifying long messages outside the exception class
(TRY003)
260-260: Abstract raise to an inner function
(TRY301)
260-260: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Redundant exception object included in logging.exception call
(TRY401)
273-273: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (11)
webhook_server/libs/handlers/check_run_handler.py (1)
39-40: Branch protection caching looks correct and safely scopedThe new
_repository_privateand_branch_required_status_checkscaches nicely avoid repeatedget_branch()/get_protection()calls while preserving behavior (per‑webhook handler instance, single PR). This aligns well with the repo’s API‑reduction goals; just keep in mind this relies on the current design where aCheckRunHandlerinstance only ever handles one PR per webhook.Also applies to: 398-417
webhook_server/tests/test_runner_handler.py (1)
140-221: Worktree and container/cherry‑pick tests correctly model the async context flowThe updated tests here consistently model
_checkout_worktreeas an async context manager (usingAsyncMockwith async__aenter__/__aexit__) and provide solid coverage for success/failure scenarios of worktree setup, merge behavior, and downstream container build/cherry‑pick flows. This matches the newgit_worktree_checkoutcontract and should keep regressions in the worktree wrapper caught by CI.Also applies to: 242-406, 640-698, 775-898
webhook_server/utils/helpers.py (1)
4-5: Worktree helper cleanly encapsulates checkout and cleanupThe new
git_worktree_checkoutasync context manager cleanly encapsulates worktree creation and teardown around an existing clone, returning(success, worktree_path, out, err)in a way that lines up with the updatedRunnerHandler._checkout_worktreeand its tests. Thefinallyblock’s best‑effort git removal with ashutil.rmtreefallback is appropriate here so that cleanup issues don’t interfere with handler logic.Also applies to: 11-14, 17-18, 595-652
webhook_server/libs/handlers/push_handler.py (1)
98-109: Push‑tag flows will now fail because_checkout_worktreenever has a base clone
upload_to_pypi(and tag‑push container builds viarun_build_container(push=True, ...)) now rely onRunnerHandler._checkout_worktree, which in turn assumesgithub_webhook.clone_repo_diris a valid git clone. That assumption is only satisfied for PR‑based events whereGithubWebhook._clone_repository_for_pris called; forpushevents we never clone the repo at all, sogit -C {repo_dir} worktree add ...will consistently fail.Net effect: tag push workflows (PyPI upload and container build/push) will always hit the “repository preparation failed” path and never actually build or publish.
You’ll need a clone step for push flows as well, e.g.:
- Generalize
_clone_repository_for_printo an “ensure repo cloned” helper that can be called for both PR and push events (deriving the checkout ref from the push payload for tags), or- Introduce a separate clone routine for push events that runs before
upload_to_pypi/run_build_container.Without that, this change is a functional regression for push‑triggered release pipelines.
⛔ Skipped due to learnings
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.webhook_server/tests/test_github_api.py (2)
264-264: LGTM! Patches correctly bypass cloning in webhook processing tests.The patches for
_clone_repository_for_prappropriately mock the new centralized cloning flow, allowing tests to focus on webhook processing logic without executing git operations.Also applies to: 366-366, 751-753
1029-1366: Excellent test coverage for the new cloning flow.The six new test functions comprehensively cover success, failure, and edge-case scenarios for
_clone_repository_for_pr:
- Success path with state verification
- Early return when already cloned
- Clone and checkout failures with appropriate RuntimeError
- Config failures degrading gracefully to warnings
- General exception handling
The static analysis warnings about unused mock function arguments are false positives—these parameters are intentionally present to match expected signatures.
webhook_server/libs/handlers/runner_handler.py (5)
173-214: Correct integration with the new worktree-based flow.The
run_toxmethod properly unpacks the 4-tuple from_checkout_worktreeand usesworktree_pathin command construction. Error handling correctly propagates stdout/stderr from worktree creation failures.
239-276: Consistent implementation following the worktree pattern.The
run_pre_commitmethod correctly integrates with the worktree-based checkout, usingworktree_pathappropriately in the pre-commit command.
324-386: Proper worktree integration for container builds.The
run_build_containermethod correctly usesworktree_pathfor both the Dockerfile path and build context, ensuring the container build operates on the isolated worktree checkout.
474-512: Correct worktree usage for Python module installation.The
run_install_python_modulemethod properly usesworktree_pathfor both the wheel output directory and source, ensuring the installation operates on the isolated checkout.
628-703: Correct worktree integration for cherry-pick operations.The
cherry_pickmethod properly constructs git and hub commands with explicit--work-treeand--git-dirflags referencing the worktree, ensuring all cherry-pick operations are isolated to the worktree checkout.
- Added missing 'import asyncio' to test_pull_request_handler.py - Removed unused 'mask_sensitive' parameter from run_podman_command method - Resolves test_process_new_or_reprocess_pull_request_parallel_execution failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webhook_server/libs/handlers/runner_handler.py (1)
84-87: Static analysis hint: Consider using a custom exception class for detailed messages.This is a minor style suggestion from Ruff (TRY003). For runtime errors with detailed diagnostic information, consider defining a custom exception class instead of passing long messages directly to
RuntimeError. However, given the low frequency of this error path and the clear diagnostic value of the inline message, this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
webhook_server/libs/github_api.py(6 hunks)webhook_server/libs/handlers/pull_request_handler.py(2 hunks)webhook_server/libs/handlers/runner_handler.py(12 hunks)webhook_server/tests/test_pull_request_handler.py(10 hunks)webhook_server/tests/test_push_handler.py(8 hunks)webhook_server/tests/test_runner_handler.py(20 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
📚 Learning: 2024-10-15T13:19:23.105Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 602
File: webhook_server_container/libs/github_api.py:1503-1503
Timestamp: 2024-10-15T13:19:23.105Z
Learning: In `run_podman_command` function in `webhook_server_container/libs/github_api.py`, using environment variables or global login sessions for credentials is not suitable because the function may be called concurrently with different credentials.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-10-15T13:18:38.590Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 602
File: webhook_server_container/libs/github_api.py:2053-2062
Timestamp: 2024-10-15T13:18:38.590Z
Learning: In the `run_podman_command` method in `webhook_server_container/libs/github_api.py`, the `rc` variable is a boolean that returns `True` when the command succeeds and `False` when it fails.
Applied to files:
webhook_server/libs/handlers/pull_request_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/tests/test_push_handler.pywebhook_server/libs/github_api.py
📚 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_push_handler.pywebhook_server/tests/test_runner_handler.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-10-29T10:42:50.163Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_pull_request_handler.py
📚 Learning: 2024-10-14T14:13:21.316Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1632-1637
Timestamp: 2024-10-14T14:13:21.316Z
Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.
Applied to files:
webhook_server/libs/github_api.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-05-10T21:39:34.243Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:281-308
Timestamp: 2025-05-10T21:39:34.243Z
Learning: In the refactored codebase, the `run_podman_command` method in `RunnerHandler` returns a tuple `(bool, str, str)` where the boolean `True` represents success and `False` represents failure, which is the opposite of traditional shell exit codes.
Applied to files:
webhook_server/tests/test_runner_handler.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-07T16:21:03.462Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 579
File: webhook_server_container/libs/github_api.py:1098-1101
Timestamp: 2024-10-07T16:21:03.462Z
Learning: In the Python method `_run_tox` within `webhook_server_container/libs/github_api.py`, the variable `_tox_tests` is already comma-separated, so removing spaces with `_tox_tests.replace(" ", "")` is appropriate to handle any accidental spaces when specifying Tox environments.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
🧬 Code graph analysis (6)
webhook_server/libs/handlers/pull_request_handler.py (1)
webhook_server/libs/handlers/runner_handler.py (1)
run_podman_command(127-147)
webhook_server/tests/test_push_handler.py (1)
webhook_server/libs/handlers/push_handler.py (2)
PushHandler(16-171)upload_to_pypi(77-171)
webhook_server/libs/github_api.py (1)
webhook_server/utils/helpers.py (3)
_redact_secrets(144-212)format_task_fields(117-136)run_command(270-378)
webhook_server/tests/test_pull_request_handler.py (1)
webhook_server/libs/handlers/pull_request_handler.py (1)
process_new_or_reprocess_pull_request(1165-1216)
webhook_server/tests/test_runner_handler.py (1)
webhook_server/libs/handlers/runner_handler.py (4)
run_build_container(285-455)run_conventional_title_check(520-597)cherry_pick(602-720)_checkout_worktree(46-116)
webhook_server/libs/handlers/runner_handler.py (2)
webhook_server/utils/helpers.py (3)
git_worktree_checkout(596-652)run_command(270-378)_redact_secrets(144-212)webhook_server/libs/handlers/check_run_handler.py (1)
get_check_run_text(299-325)
🪛 Ruff (0.14.4)
webhook_server/tests/test_push_handler.py
132-132: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
163-163: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
185-185: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
206-206: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
230-230: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
255-255: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
282-282: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
306-306: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
337-337: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
364-364: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
391-391: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
webhook_server/libs/github_api.py
213-213: Abstract raise to an inner function
(TRY301)
213-213: Avoid specifying long messages outside the exception class
(TRY003)
266-266: Abstract raise to an inner function
(TRY301)
266-266: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Redundant exception object included in logging.exception call
(TRY401)
279-279: Avoid specifying long messages outside the exception class
(TRY003)
webhook_server/tests/test_pull_request_handler.py
1371-1371: Unused function argument: args
(ARG001)
1371-1371: Unused function argument: kwargs
(ARG001)
1372-1372: Create your own exception
(TRY002)
1372-1372: Avoid specifying long messages outside the exception class
(TRY003)
1374-1374: Unused function argument: args
(ARG001)
1374-1374: Unused function argument: kwargs
(ARG001)
1377-1377: Unused function argument: args
(ARG001)
1377-1377: Unused function argument: kwargs
(ARG001)
1386-1386: Unused function argument: args
(ARG001)
1386-1386: Unused function argument: kwargs
(ARG001)
1389-1389: Unused function argument: args
(ARG001)
1389-1389: Unused function argument: kwargs
(ARG001)
1392-1392: Unused function argument: args
(ARG001)
1392-1392: Unused function argument: kwargs
(ARG001)
webhook_server/tests/test_runner_handler.py
144-144: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
165-165: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
188-188: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
211-211: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
245-245: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
293-293: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
324-324: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
366-366: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
395-395: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
645-645: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
662-662: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
683-683: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
701-701: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
711-711: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
731-731: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
749-749: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
767-767: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
778-778: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
814-814: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
869-869: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
888-888: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
webhook_server/libs/handlers/runner_handler.py
84-87: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
webhook_server/libs/handlers/runner_handler.py (7)
46-116: Well-structured worktree checkout with clear separation of concerns.The refactoring from local cloning to centralized worktree checkout is well-executed. The logic correctly:
- Determines checkout targets based on context (checkout, tag, merged state, or PR)
- Delegates worktree creation to the centralized helper
- Handles merge operations only when appropriate (non-merged PRs without tags)
- Propagates errors via the 4-tuple result
127-147: LGTM! Signature simplified correctly.The removal of the
mask_sensitiveparameter and consistent use ofself.github_webhook.mask_sensitivethroughout both the main execution path and the podman bug retry path is correct.
176-219: LGTM! Tox execution correctly migrated to worktree-based checkout.The method correctly:
- Unpacks the 4-tuple result from
_checkout_worktree- Uses
worktree_pathfor all command construction- Handles checkout failures by propagating stdout/stderr
- Applies masking consistently via
self.github_webhook.mask_sensitive
244-283: LGTM! Pre-commit execution correctly migrated to worktree-based checkout.The implementation follows the same correct pattern as
run_tox, properly handling the worktree checkout and error propagation.
331-456: LGTM! Container build correctly migrated to worktree-based checkout.The method properly:
- Passes
is_mergedandtag_nameto_checkout_worktreefor appropriate checkout behavior- Uses
worktree_pathfor all Dockerfile and build context paths- Handles both build and push flows with appropriate secret redaction on push operations
480-518: LGTM! Python module installation correctly migrated to worktree-based checkout.The implementation correctly uses
worktree_pathfor both the source directory and output directory in the pip wheel command.
634-720: LGTM! Cherry-pick correctly migrated to worktree-based checkout.The method properly:
- Uses
worktree_paththroughout all git and hub commands- Handles checkout failures with early return
- Applies secret redaction both in
run_command(viaredact_secretsparameter) and in manual PR comment generation (via_redact_secretshelper)- Maintains the final success path where
output["text"]uses the last command's output[approve_code_challenges]
Replace GitHub API calls with filesystem operations for OWNERS file discovery and reading. This eliminates ~1,200-2,000 API calls per webhook. Changes: - Modified owners_files_handler.py to use Path.rglob() for file discovery - Replaced repository.get_git_tree() with filesystem walk - Replaced repository.get_contents() with Path.read_text() - Added _clone_repository_for_pr() call to ensure repo is cloned - Updated tests to use tmp_path fixtures instead of API mocks - Wrapped filesystem I/O in asyncio.to_thread() for non-blocking operation Performance: - Saves ~1,200-2,000 API calls per webhook (1.3-2.1% reduction) - 1,500-6,000x faster than API-based approach - Eliminates GitHub API rate limit pressure for OWNERS scanning Test results: - All 55 tests pass (39 + 16 across test files) - 98% code coverage maintained - Code review approved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
webhook_server/libs/handlers/owners_files_handler.py (2)
109-124: Consider handling filesystem I/O errors in_get_file_content_from_local.The helper is clean and does exactly what we need, but if an OWNERS file is deleted or becomes unreadable between
rglobandread_text, aOSError/FileNotFoundErrorwill currently bubble up and fail the whole OWNERS loading. Wrapping the read in a smalltry/except OSErrorand logging/skip behavior would make this more robust in face of transient filesystem issues.
175-176: Optional: switch YAML error logging tologger.exceptionfor better diagnostics.Static analysis suggests using
logger.exceptioninside theyaml.YAMLErrorhandler so that the traceback is captured alongside the message, which can make debugging malformed OWNERS files easier:- except yaml.YAMLError as exp: - self.logger.error(f"{self.log_prefix} Invalid OWNERS file {relative_path_str}: {exp}") + except yaml.YAMLError: + self.logger.exception(f"{self.log_prefix} Invalid OWNERS file {relative_path_str}")If you prefer less noisy logs, the current
errorcall is fine; this is mainly an observability trade‑off.webhook_server/tests/test_pull_request_owners.py (1)
129-135: Tidy up unused fixture parameters intest_get_all_repository_approvers_and_reviewers.
changed_filesandall_repository_approvers_and_reviewersare only used for their fixture side effects, which Ruff flags as unused. If you want to keep the fixtures but silence ARG001, consider renaming them to_changed_files/_all_repository_approvers_and_reviewersor explicitly deleting them inside the test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
webhook_server/libs/handlers/owners_files_handler.py(1 hunks)webhook_server/tests/test_owners_files_handler.py(4 hunks)webhook_server/tests/test_pull_request_owners.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
📚 Learning: 2024-11-21T13:34:45.218Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:596-604
Timestamp: 2024-11-21T13:34:45.218Z
Learning: In the codebase, aggregation of reviewers and approvers from all OWNERS files is handled within the `assign_reviewers` method.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_owners_files_handler.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.py
🧬 Code graph analysis (3)
webhook_server/tests/test_pull_request_owners.py (3)
webhook_server/tests/conftest.py (3)
process_github_webhook(144-145)owners_file_handler(149-150)pull_request(115-116)webhook_server/tests/test_owners_files_handler.py (1)
owners_file_handler(32-34)webhook_server/libs/handlers/owners_files_handler.py (1)
get_all_repository_approvers_and_reviewers(126-179)
webhook_server/libs/handlers/owners_files_handler.py (2)
webhook_server/tests/conftest.py (3)
github_webhook(120-140)pull_request(115-116)PullRequest(99-111)webhook_server/libs/github_api.py (1)
_clone_repository_for_pr(172-279)
webhook_server/tests/test_owners_files_handler.py (2)
webhook_server/tests/conftest.py (2)
owners_file_handler(149-150)github_webhook(120-140)webhook_server/libs/handlers/owners_files_handler.py (3)
OwnersFileHandler(22-433)_get_file_content_from_local(109-124)get_all_repository_approvers_and_reviewers(126-179)
🪛 Ruff (0.14.4)
webhook_server/tests/test_pull_request_owners.py
129-129: Unused function argument: changed_files
(ARG001)
133-133: Unused function argument: all_repository_approvers_and_reviewers
(ARG001)
webhook_server/libs/handlers/owners_files_handler.py
176-176: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: conventional-title
- GitHub Check: build-container
- GitHub Check: pre-commit
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (8)
webhook_server/libs/handlers/owners_files_handler.py (1)
127-161: Local OWNERS discovery viaclone_repo_dirlooks correct and aligns with API-reduction goals.The switch to
clone_repo_dir.rglob("OWNERS"), bounded bymax_owners_files, plus batching reads via_get_file_content_from_localandasyncio.gather, nicely removes the previous GitHub API tree/content calls while keeping behavior (root as".", per-folder parents) consistent with existing tests and callers. This is a solid perf win with clear logging around discovery and caps.Also applies to: 165-170
webhook_server/tests/test_pull_request_owners.py (1)
136-172: Tmp-path based OWNERS test correctly exercises the new local-file workflow.The test’s use of
tmp_pathto build a mini repo tree, wiringprocess_github_webhook.clone_repo_dir, and patching_clone_repository_for_prwithAsyncMockgives good coverage of the filesystem-based OWNERS loading while staying completely local and fast, which fits well with the repo’s perf/API-reduction goals.webhook_server/tests/test_owners_files_handler.py (6)
37-58: Good reuse of shared OWNERS data viamock_content_filesfixture.Centralizing the various OWNERS YAML payloads in a single fixture keeps the tests DRY and makes it easy to keep expectations in sync with the handler’s behavior when adding new cases.
157-170:test_get_file_content_from_localnicely validates relative path semantics.This test ensures
_get_file_content_from_localreturns both the raw content and the path relative toclone_repo_dir("test/OWNERS"), which is exactly what callers rely on when building the OWNERS mapping.
171-211: Local-cloneget_all_repository_approvers_and_reviewerstest mirrors real behavior well.By constructing the filesystem layout under
tmp_path, settingclone_repo_dir, and patching_clone_repository_for_prto a no-op, the test exercises the new local discovery and YAML loading logic end-to-end and checks that the resulting dict structure matches expectations across root and nested folders.
213-233: Edge case for >1000 OWNERS files is well covered.The
too_many_filestest directly validates themax_owners_filesguard (returning 1000 entries and emitting a single error log), which is important for robustness in very large repos.
235-253: Invalid YAML handling test aligns with the YAML error branch.Creating a syntactically invalid OWNERS file and asserting an empty result plus a single
logger.errorcall gives good protection around theyaml.YAMLErrorpath in the implementation.
255-273: Invalid OWNERS content structure test confirms validation behavior.The test that writes
"approvers": "not_a_list"and checks for an empty result and a single error log effectively covers_validate_owners_contentreturningFalseand ensures structurally bad OWNERS files are safely ignored.
…ssing - Add OSError handling in _get_file_content_from_local to gracefully handle missing or unreadable files - Return None for unreadable files instead of failing the entire process - Update YAML error logging to use logger.exception for better diagnostics and full tracebacks - Add test coverage for file not found scenario - Fix unused fixture parameters in test_pull_request_owners.py with # noqa: ARG001 comments Addresses CodeRabbit review feedback for better error handling and code quality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
webhook_server/tests/test_pull_request_owners.py (1)
1-1: Local-clone-based OWNERS test aligns well with new handler flowThe switch to a filesystem-based setup with
tmp_path, explicit OWNERS file creation, and patching_clone_repository_for_pras anAsyncMockmatches the new implementation and keeps the test free of real API/clone calls. The equality assertion againstall_repository_approvers_and_reviewerscontinues to validate the aggregation logic end-to-end.If you find yourself tweaking these OWNERS structures often, consider centralizing the shared OWNERS test data (here and in
test_owners_files_handler.py) into a reusable fixture/helper to keep them in sync, but this is not required for correctness.Also applies to: 127-173
webhook_server/libs/handlers/owners_files_handler.py (1)
109-132: Local OWNERS discovery & read path looks correct; minor Path/comment nitsThe new local-read flow (_clone → rglob → async read → YAML parse/validate) is solid and matches the tests and API-reduction goal. Error handling (OSError, invalid YAML, invalid schema) gracefully skips bad files without blowing up the webhook.
Two small polish suggestions:
- Use a Path as the
relative_tobase for consistencyRight now
_get_file_content_from_localdoes:relative_path = content_path.relative_to(self.github_webhook.clone_repo_dir)Given that
get_all_repository_approvers_and_reviewersuses:clone_path = Path(self.github_webhook.clone_repo_dir) relative_path = owners_file_path.relative_to(clone_path)it would be slightly safer and more self-consistent to mirror that here:
- relative_path = content_path.relative_to(self.github_webhook.clone_repo_dir) + clone_path = Path(self.github_webhook.clone_repo_dir) + relative_path = content_path.relative_to(clone_path)This avoids any surprise if
clone_repo_diris ever a non-normalized path or a different type than expected.
- Clarify the
max_owners_filescomment
max_owners_files = 1000 # Configurable limitis intentionally hardcoded per prior guidance. Consider updating the comment to reflect reality, e.g.:- max_owners_files = 1000 # Configurable limit + max_owners_files = 1000 # Hard limit to prevent runaway OWNERS scansFunctionally everything here looks good and aligns with the “minimize API calls” objective.
Also applies to: 134-188
webhook_server/tests/test_owners_files_handler.py (1)
1-1: Shared OWNERS fixtures using YAML strings look goodImporting
Pathand switchingmock_content_filesto return raw YAML strings keyed by file path keeps the tests close to the new filesystem-based behavior while remaining simple. This also mirrors the structure used in other tests that exercise OWNERS aggregation.If duplication with
test_pull_request_owners.pyever becomes a maintenance burden, you could consider moving this mapping into a common helper/fixture, but it’s fine as-is.Also applies to: 37-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
webhook_server/libs/handlers/owners_files_handler.py(1 hunks)webhook_server/tests/test_owners_files_handler.py(4 hunks)webhook_server/tests/test_pull_request_owners.py(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
📚 Learning: 2024-11-21T13:34:45.218Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:596-604
Timestamp: 2024-11-21T13:34:45.218Z
Learning: In the codebase, aggregation of reviewers and approvers from all OWNERS files is handled within the `assign_reviewers` method.
Applied to files:
webhook_server/tests/test_pull_request_owners.pywebhook_server/tests/test_owners_files_handler.pywebhook_server/libs/handlers/owners_files_handler.py
📚 Learning: 2024-11-21T16:10:01.777Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:2067-2101
Timestamp: 2024-11-21T16:10:01.777Z
Learning: In `webhook_server_container/libs/github_api.py`, keep the `max_owners_files` limit as a hardcoded value rather than making it configurable through the config.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.py
🧬 Code graph analysis (3)
webhook_server/tests/test_pull_request_owners.py (3)
webhook_server/tests/conftest.py (3)
process_github_webhook(144-145)owners_file_handler(149-150)pull_request(115-116)webhook_server/tests/test_owners_files_handler.py (1)
owners_file_handler(32-34)webhook_server/libs/handlers/owners_files_handler.py (1)
get_all_repository_approvers_and_reviewers(133-190)
webhook_server/tests/test_owners_files_handler.py (3)
webhook_server/tests/conftest.py (2)
owners_file_handler(149-150)github_webhook(120-140)webhook_server/libs/handlers/owners_files_handler.py (3)
OwnersFileHandler(22-444)_get_file_content_from_local(109-131)get_all_repository_approvers_and_reviewers(133-190)webhook_server/tests/test_runner_handler.py (1)
mock_pull_request(53-63)
webhook_server/libs/handlers/owners_files_handler.py (2)
webhook_server/tests/conftest.py (3)
github_webhook(120-140)pull_request(115-116)PullRequest(99-111)webhook_server/libs/github_api.py (1)
_clone_repository_for_pr(172-279)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: conventional-title
- GitHub Check: pre-commit
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (2)
webhook_server/tests/test_owners_files_handler.py (2)
157-192: _get_file_content_from_local is well covered for success and missing-file casesThe two tests around
_get_file_content_from_local:
- Verify that a real OWNERS file under
tmp_pathyields the expected(content, "relative/path")tuple whenclone_repo_diris set.- Confirm that a non-existent file returns
Noneand emits a warning containing the right path and “Skipping this file” wording.This matches the handler’s behavior exactly and protects against regressions if the logging or error handling changes.
194-295: Comprehensive tests for local OWNERS discovery, limits, and invalid contentThe new/updated tests for
get_all_repository_approvers_and_reviewersdo a thorough job:
- Normal path: populating a
tmp_pathhierarchy with OWNERS files frommock_content_files, patching_clone_repository_for_pr, and asserting the aggregated dict matches expectations.- Limit path: creating 1001 OWNERS files and asserting only 1000 are processed plus a single
logger.errorcall, which validates the guardrail.- Error paths: separate tests for invalid YAML (exception logged, empty result) and invalid content structure (schema error logged via
_validate_owners_content, empty result).They also consistently rely on
clone_repo_dirand local filesystem behavior, which aligns with the API-reduction goals of the PR.No functional issues spotted here.
- Create shared owners_files_test_data fixture in conftest.py - Remove duplicate OWNERS test data from test files (48 lines eliminated) - Use Path() consistently in owners_files_handler.py (line 118) - Clarify max_owners_files comment (intentionally hardcoded) Addresses CodeRabbit AI review comments for better maintainability and code consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webhook_server/tests/conftest.py (1)
198-225: Shared OWNERS fixture is solid; consider de‑duplicating source of truthThe
owners_files_test_datafixture cleanly centralizes OWNERS YAML for tests and matches expectations elsewhere. The only minor concern is duplication with the inline OWNERS YAML inRepository.get_contentsin this same file; if one changes and the other doesn’t, tests may drift.If you keep both around long‑term, consider extracting a small helper or constant that both
Repository.get_contentsand this fixture use so the data stays in sync.webhook_server/libs/handlers/owners_files_handler.py (1)
109-131: Local OWNERS reader looks good; consider also handling decode errorsThe new
_get_file_content_from_localcorrectly computes a relative path, offloads I/O to a thread pool, and safely skips unreadable files while logging a warning. One potential hardening tweak: if an OWNERS file has invalid encoding,read_textwill raiseUnicodeDecodeError, which currently isn’t caught and would abort processing for all files.You could treat that similarly to
OSErrorso a single bad file doesn’t break OWNERS aggregation:- except OSError as ex: + except (OSError, UnicodeDecodeError) as ex: # File may have been deleted or become unreadable between rglob and read_text self.logger.warning( f"{self.log_prefix} Failed to read OWNERS file {relative_path}: {ex}. Skipping this file." ) return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
webhook_server/libs/handlers/owners_files_handler.py(1 hunks)webhook_server/tests/conftest.py(1 hunks)webhook_server/tests/test_owners_files_handler.py(3 hunks)webhook_server/tests/test_pull_request_owners.py(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
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.
📚 Learning: 2024-11-21T13:34:45.218Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:596-604
Timestamp: 2024-11-21T13:34:45.218Z
Learning: In the codebase, aggregation of reviewers and approvers from all OWNERS files is handled within the `assign_reviewers` method.
Applied to files:
webhook_server/tests/test_owners_files_handler.pywebhook_server/libs/handlers/owners_files_handler.py
📚 Learning: 2024-11-21T16:10:01.777Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:2067-2101
Timestamp: 2024-11-21T16:10:01.777Z
Learning: In `webhook_server_container/libs/github_api.py`, keep the `max_owners_files` limit as a hardcoded value rather than making it configurable through the config.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the webhook server codebase, `root_approvers` and `root_reviewers` are defined as properties in the `GithubWebhook` class, not methods, so they can be accessed without parentheses and used with methods like `.copy()` directly.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.py
🧬 Code graph analysis (3)
webhook_server/tests/test_owners_files_handler.py (1)
webhook_server/tests/conftest.py (3)
owners_file_handler(149-150)github_webhook(120-140)owners_files_test_data(199-225)
webhook_server/libs/handlers/owners_files_handler.py (2)
webhook_server/tests/conftest.py (3)
github_webhook(120-140)pull_request(115-116)PullRequest(99-111)webhook_server/libs/github_api.py (1)
_clone_repository_for_pr(172-279)
webhook_server/tests/test_pull_request_owners.py (2)
webhook_server/tests/conftest.py (4)
process_github_webhook(144-145)owners_file_handler(149-150)pull_request(115-116)owners_files_test_data(199-225)webhook_server/libs/handlers/owners_files_handler.py (1)
get_all_repository_approvers_and_reviewers(133-190)
🔇 Additional comments (5)
webhook_server/libs/handlers/owners_files_handler.py (1)
133-190: Filesystem-based OWNERS aggregation is consistent and boundedThe updated
get_all_repository_approvers_and_reviewerscorrectly:
- Ensures the repo is cloned once via
_clone_repository_for_pr.- Discovers OWNERS files via
clone_path.rglob("OWNERS")in a thread pool.- Enforces a hard limit (
max_owners_files = 1000) and logs when exceeded.- Reads each file via
_get_file_content_from_local, skips unreadable entries, parses withyaml.safe_load, validates structure, and stores data keyed by the parent directory (normalizing empty/“.” to “.”).This matches the expectations in the new tests and cleanly removes the previous API-based N+1 pattern in favor of local I/O, while still guarding against runaway repositories.
webhook_server/tests/test_pull_request_owners.py (1)
1-1: Filesystem-backed OWNERS test aligns well with the new handler behaviorThe switch to using
tmp_path+owners_files_test_data, wiringclone_repo_dirto the temp directory, and patching_clone_repository_for_prto a no‑op AsyncMock gives this test good coverage of the new local‑filesystem path without any real cloning or API calls. Comparing the method result toowners_file_handler.all_repository_approvers_and_reviewers(seeded by the fixture) keeps the expected structure explicit and stable.This looks correct and nicely integrated with the shared test data.
Also applies to: 128-152
webhook_server/tests/test_owners_files_handler.py (3)
133-167: Good coverage for_get_file_content_from_localsuccess and failure pathsThe tests for
_get_file_content_from_localcorrectly:
- Verify relative path calculation and returned
(content, "test/OWNERS")tuple when the file exists.- Exercise the missing-file case, asserting the method returns
Noneand that the warning log contains the expected phrases and relative path.This tightly couples the tests to the intended behavior and will catch regressions in how unreadable files are handled.
170-207: Local-clone OWNERS aggregation test is well structured and reuses shared dataThe updated
test_get_all_repository_approvers_and_reviewersusesowners_files_test_datato populate a realistic repo layout undertmp_path, setsclone_repo_dir, patches_clone_repository_for_prto a no‑op, and asserts the handler returns the expected directory→OWNERS mapping.This mirrors production behavior closely while staying unit‑test friendly and avoids any remote API calls, which fits the perf goals.
211-271: Edge-case tests (limit, invalid YAML, invalid structure) nicely harden the handlerThe trio of tests for:
- too many OWNERS files (1001),
- invalid YAML content, and
- structurally invalid OWNERS data
do a good job of pinning down important edge cases:
- The limit test confirms that only 1000 files are processed and that an error is logged once.
- The invalid YAML test asserts an empty result and a single
logger.exceptioncall.- The invalid-structure test asserts an empty result and a single
logger.errorcall from validation.Together, they ensure the handler fails soft on bad or excessive inputs while keeping logging behavior well-defined.
Implement three fixes based on CodeRabbit AI review: - Fix asyncio.to_thread mocking with async helper function - Tighten error handling to special-case RuntimeError - Prefix unused test parameters with underscores (ARG001) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
webhook_server/libs/github_api.py (1)
41-48: Overall_clone_repositorydesign and integration look solid, with one critical auth bug to fix.The new clone flow is well structured: single shared clone per webhook via
_repo_cloned, clear task‑scoped logging withformat_task_fields, consistent use ofrun_commandplus_redact_secrets/mask_sensitive, and correct integration into both push and PR/check_run/issue_comment paths. Config‑drivenself.mask_sensitivealso matches the helpers API and keeps sensitive data out of logs by default.However, the clone URL still embeds the token as
https://{token}@...:github_token = self.token clone_url_with_token = self.repository.clone_url.replace("https://", f"https://{github_token}@")Per the earlier review, this pattern treats the token as the HTTP username with an empty password and can break cloning for private repositories and GitHub App/fine‑grained tokens that expect a proper
username:tokenpair. It also diverges from GitHub’s documentedx-access-token:{token}(for app installs) orapi_user:{token}patterns and should be corrected before rollout. A safer approach is to build the clone URL explicitly, for example:# Pseudocode – adjust username based on token type in your context username = "x-access-token" # or self.api_user if that’s what get_api_with_highest_rate_limit returns clone_url_with_token = f"https://{username}:{github_token}@github.com/{self.repository_full_name}.git"This change preserves the single‑clone optimization while ensuring auth works reliably for private and app‑backed repos.
Also applies to: 124-129, 172-307, 336-338, 389-391, 594-595
🧹 Nitpick comments (1)
webhook_server/tests/test_github_api.py (1)
1035-1452: New_clone_repositorytests give thorough branch and error coverage.The
test_clone_repository_*set exercises success, early‑return on_repo_cloned, clone failure, checkout failure, git‑config warnings, general exceptions, and invalid argument cases. Async mocks forrun_commandandasyncio.to_threadare correctly awaitable, and the assertions line up with the implementation’s behavior and error messages. Only optional style nits remain (e.g., long error strings inpytest.raisesand the hard‑coded/tmpin tests), which are fine to keep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webhook_server/libs/github_api.py(7 hunks)webhook_server/tests/test_github_api.py(6 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/github_api.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/github_api.pywebhook_server/tests/test_github_api.py
📚 Learning: 2024-10-14T14:13:21.316Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1632-1637
Timestamp: 2024-10-14T14:13:21.316Z
Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.py
📚 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/libs/github_api.py
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_github_api.py
🧬 Code graph analysis (2)
webhook_server/libs/github_api.py (1)
webhook_server/utils/helpers.py (4)
_redact_secrets(144-212)format_task_fields(117-136)prepare_log_prefix(555-592)run_command(270-378)
webhook_server/tests/test_github_api.py (2)
webhook_server/libs/github_api.py (3)
process(309-506)GithubWebhook(51-717)_clone_repository(172-307)webhook_server/libs/config.py (1)
repository_local_data(56-97)
🪛 Ruff (0.14.4)
webhook_server/libs/github_api.py
205-207: Avoid specifying long messages outside the exception class
(TRY003)
229-229: Abstract raise to an inner function
(TRY301)
229-229: Avoid specifying long messages outside the exception class
(TRY003)
291-291: Abstract raise to an inner function
(TRY301)
291-291: Avoid specifying long messages outside the exception class
(TRY003)
305-305: Redundant exception object included in logging.exception call
(TRY401)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
webhook_server/tests/test_github_api.py
1374-1374: Avoid specifying long messages outside the exception class
(TRY003)
1431-1431: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
🔇 Additional comments (2)
webhook_server/tests/test_github_api.py (1)
264-265: Event tests correctly stub_clone_repositorywithout blocking real clone logic.Using
AsyncMock(return_value=None)onwebhook._clone_repositoryin the pull_request, push, issue_comment, and check_run tests keeps the event‑flow assertions focused and avoids unnecessary git operations. The addedget_github_repo_apipatch andmock_repository.clone_urlsetup for push events are consistent with the production constructor and don’t introduce extra API calls.Also applies to: 270-303, 310-312, 374-375, 758-761
webhook_server/libs/github_api.py (1)
703-716: Destructor docstring and cleanup behavior are aligned with the new clone strategy.The updated
__del__docstring correctly explains that only the shared base clone directory (created viatempfile.mkdtemp) is removed here, with worktree cleanup delegated to handlers. The guardedshutil.rmtreecall with best‑effort error handling is appropriate for a destructor.
Improve test code readability and portability: - Shorten pytest.raises match strings for better readability - Replace hard-coded /tmp paths with pytest tmp_path fixture
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webhook_server/tests/test_github_api.py (2)
1088-1089: Consider extracting the async helper to a fixture.The
to_thread_synchelper is defined identically three times. While test isolation is valuable, extracting this to a pytest fixture would improve maintainability without sacrificing clarity.Add a fixture at the class or module level:
@pytest.fixture def async_to_thread_sync(self): """Helper to make asyncio.to_thread awaitable in tests while executing inline.""" async def to_thread_sync(fn: Any, *args: Any, **kwargs: Any) -> Any: return fn(*args, **kwargs) return to_thread_syncThen use it in tests:
def test_clone_repository_success(self, ..., async_to_thread_sync): ... with ( patch("webhook_server.libs.github_api.run_command", side_effect=mock_run_command), patch("asyncio.to_thread", side_effect=async_to_thread_sync), ):Also applies to: 1243-1244, 1312-1313
1043-1054: Consider extracting the config mock side effect to a fixture.The
get_value_side_effectfunction is duplicated five times with identical implementation. Extracting to a fixture would reduce duplication and make future config changes easier to maintain.Add a fixture:
@pytest.fixture def mock_config_side_effect(self): """Standard config side effect for clone tests.""" def get_value_side_effect(value: str, *_args: Any, **_kwargs: Any) -> Any: config_values = { "mask-sensitive-data": True, "container": {}, "pypi": {}, "tox": {}, "verified-job": True, } return config_values.get(value) return get_value_side_effectThen use it in tests:
def test_clone_repository_success(self, ..., mock_config_side_effect): ... mock_config.return_value.get_value.side_effect = mock_config_side_effectAlso applies to: 1141-1152, 1195-1206, 1262-1273, 1340-1351
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webhook_server/tests/test_github_api.py(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_github_api.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/tests/test_github_api.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/tests/test_github_api.py
🧬 Code graph analysis (1)
webhook_server/tests/test_github_api.py (1)
webhook_server/libs/github_api.py (3)
process(309-506)GithubWebhook(51-717)_clone_repository(172-307)
🪛 Ruff (0.14.4)
webhook_server/tests/test_github_api.py
1375-1375: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
webhook_server/tests/test_github_api.py (2)
265-265: LGTM! Proper mocking of the new clone flow.The patches for
_clone_repositorycorrectly useAsyncMockto prevent actual repository cloning during tests while maintaining the async contract. The mock repository withclone_urlin the push event test provides the necessary attributes for the clone operation.Also applies to: 311-311, 375-375, 760-760
1036-1098: Excellent test coverage for the new clone functionality!The comprehensive test suite for
_clone_repositorycorrectly addresses the past review comment aboutasyncio.to_threadmocking. The async helperto_thread_syncensures the mocked call remains awaitable while executing inline, which is the proper approach.The test cases cover all critical paths: success, early returns, various failure modes, and input validation.
Extract to_thread_sync and get_value_side_effect helpers to pytest fixtures in test_github_api.py to reduce code duplication. - Add to_thread_sync fixture (previously duplicated 3 times) - Add get_value_side_effect fixture (previously duplicated 5 times) - Update 5 tests to use the new fixtures Improves maintainability by centralizing helper function definitions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webhook_server/tests/test_github_api.py (2)
1064-1147: Success and early-return tests work correctly.The tests properly verify the clone success path and the already-cloned optimization. Consider enhancing
test_clone_repository_successto also assert thatrun_commandwas called with the expected git clone command to make the test more robust.Example enhancement for line 1115:
# After the await gh._clone_repository(pull_request=mock_pr) call from unittest.mock import call # Verify clone succeeded assert gh._repo_cloned is True # Optionally verify the clone command was called # This makes the test more robust by checking actual git operations with patch("webhook_server.libs.github_api.run_command") as mock_cmd: await gh._clone_repository(pull_request=mock_pr) clone_calls = [c for c in mock_cmd.call_args_list if "git clone" in str(c)] assert len(clone_calls) == 1
1247-1310: Config warnings test verifies non-fatal behavior correctly.The test properly confirms that git config failures log warnings but don't prevent successful cloning. The assertion of exactly 4 warnings matches the current implementation.
For increased robustness, consider verifying specific warning messages rather than just the count:
# Instead of just checking count warning_messages = [str(call) for call in mock_logger.warning.call_args_list] assert any("user.name" in msg for msg in warning_messages) assert any("user.email" in msg for msg in warning_messages) assert any("fetch" in msg or "PR fetch" in msg for msg in warning_messages) assert any("remote" in msg for msg in warning_messages)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webhook_server/tests/test_github_api.py(8 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_github_api.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/tests/test_github_api.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/tests/test_github_api.py
🧬 Code graph analysis (1)
webhook_server/tests/test_github_api.py (4)
webhook_server/tests/test_helpers.py (3)
get_value_side_effect(174-181)get_value_side_effect(197-204)get_value_side_effect(220-227)webhook_server/libs/github_api.py (2)
process(309-506)_clone_repository(172-307)webhook_server/libs/config.py (2)
get_value(99-120)repository_data(53-54)webhook_server/tests/conftest.py (2)
base(95-96)pull_request(106-107)
🪛 Ruff (0.14.4)
webhook_server/tests/test_github_api.py
1346-1346: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
webhook_server/tests/test_github_api.py (4)
78-104: LGTM! Fixtures correctly implement async mocking for clone tests.The
to_thread_syncfixture properly addresses the past review comment by makingasyncio.to_threadawaitable while executing callables inline. Theget_value_side_effectfixture provides appropriate test config values.
293-293: Good isolation of clone behavior in existing tests.Properly mocks
_clone_repositoryto prevent git operations and focus each test on its specific webhook event handling logic.Also applies to: 339-339, 403-403, 788-788
1150-1245: Failure scenarios correctly tested.Both clone and checkout failure tests properly verify that
RuntimeErroris raised with appropriate messages. The selective mocking ofasyncio.to_thread(only when needed to reach the checkout step) demonstrates good understanding of the code flow.
1313-1421: Exception handling and validation tests are comprehensive.The tests properly verify that unexpected exceptions are wrapped in
RuntimeErrorand that missing/invalid arguments triggerValueError. Coverage is thorough.Note: The static analysis hint about line 1346 (long exception message) is a false positive—it's appropriate for test code to simulate various error conditions.
|
/verified |
|
/approve |
myakove-bot
added
approved-myakove
and removed
verified
approved-myakove
commented-coderabbitai[bot]
labels
|
/verified |
|
Successfully removed PR tag: ghcr.io/myk-org/github-webhook-server:pr-906. |
Add instance variable caching for repository private status and branch
required status checks to avoid repeated API calls within same webhook event.
get_branch() and get_protection() calls
Closes: Task 1 of API reduction project (Archon: dd076ce5-ab33-45d5-bae2-b3df839a1080)
Summary by CodeRabbit
Performance Improvements
New Features
Bug Fixes / Reliability
Tests