fix: validate executable filename before spawning in resolve_environment (Fixes #364)#365
Merged
karthiknadig merged 1 commit intomainfrom Feb 27, 2026
Merged
fix: validate executable filename before spawning in resolve_environment (Fixes #364)#365karthiknadig merged 1 commit intomainfrom
karthiknadig merged 1 commit intomainfrom
Conversation
Performance Report (Linux) ✅
Legend
|
Test Coverage Report (Linux)
Coverage unchanged. |
Performance Report (Windows) ✅
Legend
|
Test Coverage Report (Windows)
Coverage unchanged. |
There was a problem hiding this comment.
Pull request overview
This PR adds filename validation to prevent spawning non-Python executables in the resolve_environment() function, fixing issue #364 where Jupyter kernel spec helper scripts were being executed with Python arguments, causing ~13 second delays and confusing error logs.
Changes:
- Made
is_python_executable_name()function public inpet-python-utilsto enable validation of executable filenames - Added early validation check in
resolve_environment()to reject files that don't match Python executable naming patterns (python, python3, python3.11, etc.) before attempting to spawn them
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/pet-python-utils/src/executable.rs | Made is_python_executable_name() function public to allow reuse for validation |
| crates/pet/src/resolve.rs | Added import of is_python_executable_name and early validation check to reject non-Python executables before spawning |
Comments suppressed due to low confidence (1)
crates/pet/src/resolve.rs:63
- Consider adding a test case to verify that non-Python executables (like bash scripts) are properly rejected by resolve_environment. This would help prevent regression of the fix for issue #364. The test could create a temporary non-Python file and verify that resolve_environment returns None with an appropriate warning.
if executable.is_file() && !is_python_executable_name(&executable) {
warn!(
"Path {:?} does not look like a Python executable, skipping resolve",
executable
);
return None;
}
DonJayamanne
approved these changes
Feb 26, 2026
Performance Report (macOS)
Legend
|
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
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.
Adds validation in
resolve_environment()to check that the executable filename matches a Python executable pattern before proceeding with the locator chain or spawning. Previously, any file (e.g., Jupyter kernel spec bash wrapper scripts) would be spawned with-c "import sys;...", wasting ~13 seconds and executing arbitrary non-Python executables.is_python_executable_name()public inpet-python-utilsto reuse existing regex patternswarn!log inresolve_environment()when filename doesn't matchFixes #364