Fix _remove to catch EnvCommandError instead of CalledProcessError#10740
Merged
radoering merged 1 commit intopython-poetry:mainfrom Feb 21, 2026
Merged
Conversation
run_pip() wraps CalledProcessError inside EnvCommandError, so the except CalledProcessError handler in _remove() was dead code. If pip exited non-zero with 'not installed' in its output, the EnvCommandError would propagate uncaught to _execute_operation, which treats it as a fatal error and aborts the entire installation. This matters when removing a package that was already uninstalled externally — e.g. via pip directly — while Poetry is performing an update.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the uninstall path in the installer executor to catch the correct exception type from pip invocations so that missing packages during removal are treated as non-fatal, and removes the now-unused import. Sequence diagram for uninstall flow with EnvCommandError handlingsequenceDiagram
actor User
participant Executor
participant Environment
participant Pip
User->>Executor: uninstall package
Executor->>Executor: _remove(package)
Executor->>Executor: run_pip("uninstall", package.name, "-y")
Executor->>Environment: run_pip("uninstall", package.name, "-y")
Environment->>Pip: pip uninstall package -y
alt package_not_installed
Pip-->>Environment: error "not installed"
Environment-->>Executor: EnvCommandError("not installed")
Executor-->>Executor: catch EnvCommandError in _remove
Executor-->>User: return 0 (treat as success)
else other_uninstall_error
Pip-->>Environment: error
Environment-->>Executor: EnvCommandError
Executor-->>Executor: _remove does not handle, rethrow
Executor->>Executor: _execute_operation handles EnvCommandError
Executor-->>User: installation aborted, shutdown set True
end
Updated class diagram for Executor uninstall error handlingclassDiagram
class Executor {
bool _shutdown
run_pip(args, kwargs) int
_remove(package) int
_execute_operation(operation) None
}
class EnvCommandError {
str message
}
class Package {
str name
}
Executor ..> EnvCommandError : raises
Executor ..> Package : removes
Executor : run_pip(args, kwargs) catches EnvCommandError
Executor : _remove(package) calls run_pip
Executor : _remove(package) catches EnvCommandError
Executor : _execute_operation(operation) may set _shutdown on EnvCommandError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Catching
EnvCommandErrorand then checking for the substring"not installed"is somewhat brittle (e.g. locale-dependent); if possible, consider using a more structured signal fromEnvCommandError(exit code, reason, or parsed stderr) rather than matching on the human-readable message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching `EnvCommandError` and then checking for the substring `"not installed"` is somewhat brittle (e.g. locale-dependent); if possible, consider using a more structured signal from `EnvCommandError` (exit code, reason, or parsed stderr) rather than matching on the human-readable message.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
radoering
approved these changes
Feb 21, 2026
Member
radoering
left a comment
There was a problem hiding this comment.
Good catch.
This might still be dead code due to changes in pip - it seems to just print a warning if the package is not installed nowadays - but it cannot hurt to keep it for robustness.
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.
Problem
Executor._remove()catchesCalledProcessError, butrun_pip()never raises that — it wraps it insideEnvCommandError:So the handler in
_remove():is dead code. When pip reports "not installed", the
EnvCommandErrorpropagates to_execute_operation, which treats it as a fatal error and setsself._shutdown = True, aborting the entire installation.This can happen during
poetry updateif a package was already removed externally (e.g.pip uninstallby hand).Fix
Catch
EnvCommandErrorinstead ofCalledProcessError, and remove the now-unused import.Summary by Sourcery
Bug Fixes: