Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR makes non-functional improvements by tightening a threading excepthook type annotation, exposing the libclinic.cpp submodule as part of the public API for tooling, and updating Dependabot configuration to auto-rebase its PRs. Class diagram for updated type hints and libclinic public APIclassDiagram
class libregrtest_utils {
+orig_threading_excepthook: Callable[threading_ExceptHookArgs, None] | None
}
class threading_ExceptHookArgs {
<<dataclass-like>>
}
class libclinic {
+__all__: ["Block", "Command", "ClinicError", "format_escape", "CPPFile", "cpp", "FormatCounterFormatter", "NULL"]
}
class cpp {
}
libregrtest_utils ..> threading_ExceptHookArgs : uses
libclinic o-- cpp : exposes_submodule
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded Dependabot rebase automation; narrowed a threading excepthook type annotation; exported a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. 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 left some high level feedback:
- If
libclinic.cppis meant to be part of the public API, make sure it is actually imported inTools/clinic/libclinic/__init__.py(not just listed in__all__), otherwisefrom libclinic import cppwill fail at runtime. - Now that
orig_threading_excepthookis annotated withthreading.ExceptHookArgs, consider updating theregrtest_threading_excepthooksignature to acceptthreading.ExceptHookArgsas well to keep the typing consistent end-to-end.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If `libclinic.cpp` is meant to be part of the public API, make sure it is actually imported in `Tools/clinic/libclinic/__init__.py` (not just listed in `__all__`), otherwise `from libclinic import cpp` will fail at runtime.
- Now that `orig_threading_excepthook` is annotated with `threading.ExceptHookArgs`, consider updating the `regrtest_threading_excepthook` signature to accept `threading.ExceptHookArgs` as well to keep the typing consistent end-to-end.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Lib/test/test_tools/test_compute_changes.py`:
- Around line 56-57: The assignment "f = candidate" is not indented under the
if-statement checking "candidate.exists()", causing a SyntaxError; fix it by
indenting "f = candidate" so it is inside the "if candidate.exists():" block
(and ensure any subsequent uses of "f" handle the case where the if-body doesn't
run), locating the lines with the "candidate.exists()" check and the "f =
candidate" assignment in test_compute_changes.py to apply the indentation
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e428cec0-e4a1-41f2-b799-17c0ba5105f1
📒 Files selected for processing (1)
Lib/test/test_tools/test_compute_changes.py
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.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Lib/test/support/os_helper.py`:
- Around line 547-565: Preserve the original path before calling
tempfile.mkdtemp and narrow the broad except to specific exceptions so the
warning logs the failing original path and not the newly-created temp path:
store original_path = path immediately after the OSError handler begins, replace
the bare "except Exception:" with a more specific exception (e.g., OSError or
tempfile-specific exceptions) around tempfile.mkdtemp, and update the logging
call to reference original_path and include exc/exception info; ensure
dir_created is only set True after mkdtemp succeeds and that exc is what's
logged.
- Around line 576-579: The cleanup block around rmtree(path) currently swallows
all exceptions; change it to catch Exception as e and log a warning/debug
message including the path and exception details (e.g., using the module logger
or Python's logging) instead of silently passing, so failures like
permission/disk issues are visible during CI; keep the cleanup attempt but
record the error from rmtree(path) for diagnostics.
- Around line 555-571: The except Exception block that logs a warning when
creating the temporary directory fails should not allow the function to proceed
to yield an invalid path; after logging the warning (in the branch where quiet
is True) re-raise the caught exception instead of falling through so callers
never receive a non-existent path. Locate the except Exception handling around
os.mkdir(path) / tempfile.mkdtemp() (the block that logs via
logging.getLogger(__name__).warning and references path, exc, dir_created) and
add a raise (or re-raise the captured exception) immediately after the warning
so the generator does not reach the yield of path when directory creation
failed.
- Around line 528-531: The sanitization currently runs two regexes on the
variable path and strips ':' causing Windows drive letters to become invalid;
remove the redundant non-ASCII replacement (the first re.sub on path) and update
the second regex to allow colons so drive letters are preserved (e.g. change
r'[^A-Za-z0-9._\\/-]' to r'[^A-Za-z0-9._\\/:-]'), leaving the rest of the
allowed characters intact; ensure these edits are applied where path is
sanitized so absolute Windows paths like "C:\temp\test" keep their drive letter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 350dbd16-2e0a-45e5-bd24-b63011b4b444
📒 Files selected for processing (1)
Lib/test/support/os_helper.py
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Refactor path sanitization and error handling in os_helper.py.
| # Prevent empty or broken names | ||
| if not path.strip(): | ||
| path = None | ||
| except BaseException: |
| path = tempfile.mkdtemp(prefix="test_python_") | ||
| dir_created = True | ||
| path = os.path.realpath(path) | ||
| except BaseException as exc: |
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[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.
Summary
This pull request makes several non-functional improvements to Python's source and tooling.
Changes included
Add precise type hint for
orig_threading_excepthookLib/test/libregrtest/utils.pyExpose
libclinic.cppand update__all__Tools/clinic/libclinic/__init__.pycppand include"cpp"in__all__.libclinic.cppas part of the public API.Enable explicit auto-rebasing in Dependabot configuration
.github/dependabot.ymlrebase-strategy: "auto"to make Dependabot PRs rebase automatically.Impact
Testing
Notes
Commits
orig_threading_excepthooklibclinic.cppand update__all__for mypySummary by Sourcery
Clarify typing around test threading exception hooks, expose an internal Clinic submodule as part of its public API, and adjust Dependabot behavior for automated rebasing.
New Features:
Enhancements:
CI:
Summary by CodeRabbit