refactor: remove dead shared rate-limit handlers - #3140
Conversation
|
/agentic_review |
There was a problem hiding this comment.
🟡 Changes recommended
The new AST-based import-boundary test only checks top-level nodes (tree.body) and can miss nested imports, weakening the intended regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes unreachable PyGithub-specific rate-limit handling from the shared diff-processing layer (pr_processing.py), relying on providers to surface the project-level RateLimitExceeded exception, and adds regression tests to ensure that exception propagates through the shared diff entry points.
Changes:
- Removed the module-scope
RateLimitExceededExceptionimport and the three deadexcept RateLimitExceededExceptionhandlers inpr_agent/algo/pr_processing.py. - Added unit tests asserting
RateLimitExceededpropagation throughget_pr_diff,get_pr_diff_multiple_patchs, andget_pr_multi_diffs. - Added a regression test intended to ensure the shared diff-processing module no longer imports PyGithub’s
RateLimitExceededException.
File summaries
| File | Description |
|---|---|
| pr_agent/algo/pr_processing.py | Removes unused PyGithub import and unreachable rate-limit exception handlers in the shared diff path. |
| tests/unittest/test_pr_processing_core.py | Adds regression coverage for project-level rate-limit propagation and import-boundary enforcement. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/agentic_review |
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for claiming #3139 and turning the direct option round within the hour. The tests pinning project-level RateLimitExceeded propagation through all three shared entry points are exactly the guard this removal needed, and the full AST walk answers Copilot's note.
Summary
RateLimitExceededExceptionimport from the shared diff-processing path.RateLimitExceededexception.GithubProvider.Closes #3139
Why this does not change runtime behavior
GithubProvider._get_diff_files()already converts the PyGithub exception to the project-levelRateLimitExceededexception, andGithubProvider.get_diff_files()retries that project exception before it returns. The shared handlers only logged and re-raisedRateLimitExceededException, so they were unreachable for GitHub and for every other provider. Removing them leaves the actual retry and conversion behavior in the provider where it already occurs; other exceptions still propagate normally from the shared path.Verification
PYTHONPATH=. uv run pytest tests/unittest -q— 4232 passed, 1 skipped, 1 xfailed.uv run ruff check pr_agent/algo/pr_processing.py tests/unittest/test_pr_processing_core.py— passed.uv run pre-commit run --files pr_agent/algo/pr_processing.py tests/unittest/test_pr_processing_core.py— passed.git diff --check— passed.中文说明
本 PR 删除共享 diff 路径中永远匹配不到的 PyGithub 限流异常导入、三个 handler 和过时的 Raises 文档。GitHub provider 原有的重试与项目级
RateLimitExceeded转换保持不变,因此不改变运行时行为;新增测试覆盖三个共享入口的项目级异常传播及 provider-agnostic 导入边界。