Conversation
- All file and directory ownerships now assigned to the core-dev team - Simplifies review process and centralizes code ownership - Ensures branch protection rules apply uniformly to the team
Updated the Dependabot configuration to run on a weekly schedule instead of monthly. This ensures dependencies are checked and updated more frequently, reducing the risk of security vulnerabilities and keeping packages more up-to-date.
Dependabot uses "auto" as the default rebasing behavior, but declaring it explicitly: - Improves configuration readability and maintainability - Protects against potential future default changes - Aligns with best practices for declarative dependency management No functional change in current behavior, but increases long-term robustness.
Add explicit import of the `cpp` submodule in Tools/clinic/libclinic/__init__.py
so that both runtime and static type checking via mypy recognize `libclinic.cpp`.
Also, include "cpp" in __all__ to mark it as part of the public API.
This fixes the mypy error in clanguage.py:
Module has no attribute "cpp" [attr-defined]
No functional behavior is changed.
In test.test_tools.test_compute_changes.TestProcessChangedFiles, the variable 'f' could be referenced before assignment if a path in LIBRARY_FUZZER_PATHS was neither a file nor a directory. This caused UnboundLocalError in CI runs. The test is now fixed by: - Initializing 'f' to None before conditional assignment. - Skipping paths that are neither valid files nor directories. - Verifying that candidate files exist before passing them to process_changed_files(). - Adding assertion messages for better CI debugging. - Preserving the working directory context with os_helper.change_cwd(basepath). This ensures the test safely iterates over all fuzzable library files and properly verifies CI fuzzing logic without triggering UnboundLocalError.
The assertion lines in test.test_tools.test_compute_changes. TestProcessChangedFiles.test_ci_fuzz_stdlib contained trailing whitespace and inconsistent formatting, which triggered lint errors. This commit: - Rewrites the assertion statements using parentheses. - Aligns arguments and adds explicit assertion messages. - Ensures the code is lint-clean while preserving the working directory context and test logic. No functional changes are introduced; this is purely a style/lint fix.
…emp()
The test helper temp_dir() may receive a directory name containing
invalid or corrupted characters during parallel test execution.
On Windows CI (e.g. Azure Pipelines), this can lead to failures such as:
FileNotFoundError: [WinError 3] The system cannot find the path specified
when os.mkdir() attempts to create directories like
"test_python_6744æ". These paths can appear when worker directory
names become mis-decoded during high parallelism test runs.
Sanitize the provided path by replacing non-ASCII and unsafe filesystem
characters with "_" before attempting to create the directory. If
directory creation still fails, fall back to tempfile.mkdtemp() to
ensure the test run can proceed.
This change preserves the existing behavior of temp_dir(), including
its logging and fork-safety semantics, while making it more robust in
CI environments.
Refactor path sanitization and error handling in os_helper.py.
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
test_extractall() creates the extraction directory using os.mkdir(). In some environments the directory may already exist from a previous test run, causing FileExistsError. Remove the directory with shutil.rmtree(..., ignore_errors=True) before creating it to ensure the test always starts with a clean extraction directory.
Removed 'ignore_errors=True' from rmtree call to ensure errors are not ignored during directory removal.
Change the `if` condition in the stale GitHub Actions workflow from `github.repository_owner == 'python'` to `github.repository_owner == 'zrsx'`. This unlocks the stale workflow for the `zrsx` account, allowing it to automatically process stale pull requests and issues for repositories owned by `zrsx`. Previously, it ran only for repositories owned by `python`.
Update the `Check labels` GitHub Actions workflow to run for repositories owned by `zrsx` instead of `python`. Changed the `if` conditions for the `label-dnm` and `label-reviews` jobs to `github.repository_owner == 'zrsx'`. This unlocks automatic checks for DO-NOT-MERGE and Unresolved review labels on pull requests in `zrsx` repositories. The workflow ensures: - PRs with the DO-NOT-MERGE label are detected. - Feature PRs (with `type-feature` label) are checked for complete review (`awaiting merge` label) if necessary.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates test tooling, CI workflows, Dependabot settings, and typing hints for improved reliability and project-local behavior. Flow diagram for GitHub Actions workflow execution based on repository ownerflowchart TD
Start["GitHub event (PR opened / scheduled run)"] --> CheckOwner{"Is github.repository_owner == 'zrsx'?"}
CheckOwner -->|Yes| RunWorkflows["Run repository-local workflows\nrequire-pr-label.yml\nstale.yml"]
CheckOwner -->|No| SkipWorkflows["Skip these workflows\n(no checks or stale handling)"]
RunWorkflows --> End["Event handling complete"]
SkipWorkflows --> End
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReassigns CODEOWNERS entries to the Changes
Sequence Diagram(s)(Skipped — changes are configuration, typing, small API exposure, and test guards; no new multi-component control flow introduced.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- You’ve added "cpp" to libclinic.all but don’t appear to bind a cpp symbol in this package; consider importing or defining libclinic.cpp so that
from Tools.clinic.libclinic import *doesn’t expose a missing attribute. - Now that orig_threading_excepthook is annotated as
Callable[[threading.ExceptHookArgs], None] | None, it would be good to updateregrtest_threading_excepthook’s signature to accept a typedargs: threading.ExceptHookArgsfor consistency and better type checking.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’ve added "cpp" to libclinic.__all__ but don’t appear to bind a cpp symbol in this package; consider importing or defining libclinic.cpp so that `from Tools.clinic.libclinic import *` doesn’t expose a missing attribute.
- Now that orig_threading_excepthook is annotated as `Callable[[threading.ExceptHookArgs], None] | None`, it would be good to update `regrtest_threading_excepthook`’s signature to accept a typed `args: threading.ExceptHookArgs` for consistency and better type checking.
## Individual Comments
### Comment 1
<location path="Lib/test/test_tools/test_compute_changes.py" line_range="60-61" />
<code_context>
+ f = candidate
elif p.is_file():
f = p
+ if f is None:
+ continue
result = process_changed_files({f})
- self.assertTrue(result.run_ci_fuzz_stdlib)
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid silently skipping all `LIBRARY_FUZZER_PATHS` and weakening the test’s guarantee
With the new `continue` when `f is None`, this test can now pass even if none of the `LIBRARY_FUZZER_PATHS` yield an existing fuzzable file, whereas before a missing file would fail the test and catch CI fuzz config issues.
To keep the test stable but still meaningful, either:
- Track whether at least one `f` was found and `self.fail`/assert after the loop if none were, or
- Fail immediately when an expected path has no fuzzable file instead of `continue`-ing.
This preserves the test’s ability to detect configuration drift rather than becoming a no-op when nothing is found.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+60
to
+61
| if f is None: | ||
| continue |
There was a problem hiding this comment.
suggestion (testing): Avoid silently skipping all LIBRARY_FUZZER_PATHS and weakening the test’s guarantee
With the new continue when f is None, this test can now pass even if none of the LIBRARY_FUZZER_PATHS yield an existing fuzzable file, whereas before a missing file would fail the test and catch CI fuzz config issues.
To keep the test stable but still meaningful, either:
- Track whether at least one
fwas found andself.fail/assert after the loop if none were, or - Fail immediately when an expected path has no fuzzable file instead of
continue-ing.
This preserves the test’s ability to detect configuration drift rather than becoming a no-op when nothing is found.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Lib/test/libregrtest/utils.py (1)
14-14: Improved type precision.The type annotation
Callable[[threading.ExceptHookArgs], None]is more accurate thanCallable[..., None]and properly documents the expected callback signature.Note: The local
import threadingat line 174 insidesetup_threading_excepthook()is now redundant sincethreadingis imported at module level. Consider removing it.♻️ Optional: Remove redundant local import
def setup_threading_excepthook() -> None: global orig_threading_excepthook - import threading orig_threading_excepthook = threading.excepthook threading.excepthook = regrtest_threading_excepthookAlso applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Lib/test/libregrtest/utils.py` at line 14, Update the type annotation for the threading exception hook from a generic Callable[..., None] to Callable[[threading.ExceptHookArgs], None] to precisely document the expected callback signature (look for the variable/parameter annotated with Callable in utils.py and in setup_threading_excepthook). Also remove the redundant local "import threading" inside setup_threading_excepthook() since threading is already imported at module scope. Ensure any type references use threading.ExceptHookArgs so the signature is accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Lib/test/libregrtest/utils.py`:
- Line 14: Update the type annotation for the threading exception hook from a
generic Callable[..., None] to Callable[[threading.ExceptHookArgs], None] to
precisely document the expected callback signature (look for the
variable/parameter annotated with Callable in utils.py and in
setup_threading_excepthook). Also remove the redundant local "import threading"
inside setup_threading_excepthook() since threading is already imported at
module scope. Ensure any type references use threading.ExceptHookArgs so the
signature is accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 881e1f22-2b00-426b-ab5a-60881b5a2583
📒 Files selected for processing (7)
.github/CODEOWNERS.github/dependabot.yml.github/workflows/require-pr-label.yml.github/workflows/stale.ymlLib/test/libregrtest/utils.pyLib/test/test_tools/test_compute_changes.pyTools/clinic/libclinic/__init__.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/CODEOWNERS (1)
651-651: Align adjacent comment with current owner mapping.Line 651 now assigns
.mailmapto@zrsx/core-dev, but the nearby comment still mentions@python/email-team. Please update the comment to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/CODEOWNERS at line 651, Update the adjacent human-readable comment near the .mailmap entry so it reflects the new owner mapping: replace any mention of `@python/email-team` with `@zrsx/core-dev` so the comment matches the actual CODEOWNERS line that assigns .mailmap to `@zrsx/core-dev`; ensure the comment references the .mailmap entry and retains original phrasing/formatting aside from the owner change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/CODEOWNERS:
- Line 135: Remove the stray merge marker "=======" from the CODEOWNERS file:
locate the literal line containing "=======" (currently present on line with the
unresolved merge artifact) and delete it so the file contains only valid
CODEOWNERS patterns and comments; ensure no other leftover merge markers (e.g.,
"<<<<<<<" or ">>>>>>>") remain and run a quick validation of the file after
editing.
---
Nitpick comments:
In @.github/CODEOWNERS:
- Line 651: Update the adjacent human-readable comment near the .mailmap entry
so it reflects the new owner mapping: replace any mention of `@python/email-team`
with `@zrsx/core-dev` so the comment matches the actual CODEOWNERS line that
assigns .mailmap to `@zrsx/core-dev`; ensure the comment references the .mailmap
entry and retains original phrasing/formatting aside from the owner change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98f17070-038d-4030-806b-92dd80c98889
📒 Files selected for processing (3)
.github/CODEOWNERS.github/dependabot.yml.github/workflows/stale.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/dependabot.yml
Collaborator
Author
|
@copilot |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
gh-37: Improve type hints, test stability, Dependabot automation, and fork CI configuration
This PR collects non-functional, backward-compatible enhancements to boost developer productivity and maintenance quality in the zrsx/cpython repository:
No runtime impact — zero changes to interpreter behavior, standard library semantics, or user-facing APIs. All modifications are confined to test support code, tooling (Clinic), configuration files, and GitHub Actions.
Detailed Changes by Category
Type Hinting & Static Analysis
orig_threading_excepthookinLib/test/libregrtest/utils.py:Callable[[threading.ExceptHookArgs], None] | None.cppsubmodule fromTools/clinic/libclinic/__init__.py(import +__all__entry) to satisfy type checkers.Test Stability & Lint Fixes
test_ci_fuzz_stdlib.UnboundLocalErrorin the same test by proper variable initialization and path validation.Dependabot & Dependency Automation
rebase-strategy: "auto".Fork CI & Organization Maintenance
@zrsx/core-devteam (replacing individual usernames).Commits Included
(Exactly the commits requested, listed chronologically with short hashes):
7ff33af — misc: unlock PR labeling workflow for zrsx repositories
7ff33af
2a42c53 — misc: unlock stale job for zrsx repositories
2a42c53
8228c84 — bpo: Fix lint issues in test_ci_fuzz_stdlib
8228c84
af67a0a — bpo: Fix UnboundLocalError in test_ci_fuzz_stdlib
af67a0a
a1d5ba9 — bpo: expose libclinic.cpp and update all for mypy
a1d5ba9
de303b6 — Fix: Add type hint for orig_threading_excepthook
de303b6
20a4948 — chore: enable explicit auto-rebasing in dependabot configuration
20a4948
87bc392 — chore(dependabot): change update schedule from monthly to weekly
87bc392
60d1043 — chore(CODEOWNERS): replace individual usernames with @zrsx/core-dev team
60d1043
Testing
./python -m test(full regrtest suite) passes cleanly.Documentation
Doc/whatsnew/orNEWS.d(orMisc/NEWS.d).Issue
Closes gh-37.
Review Guidance
Focus on:
Thanks for the reviews and feedback — excited to get this landed!
-->
Summary by Sourcery
Tighten tooling type hints and testing behavior while updating repository-specific Dependabot and CI workflow configuration.
New Features:
Bug Fixes:
CI:
Tests:
Summary by CodeRabbit
Chores
Tests
New Features