Conversation
poetry-core's PathDependency.__init__ unconditionally warns if a path/directory/file dependency's target doesn't exist, at pyproject.toml parse time, before the CLI knows which --with/--without/--only groups are active for this invocation. A path dependency in an excluded group (e.g. a dev-only local package absent from a Docker image) was getting flagged even though it was never going to be installed. The already-correct, group-aware validation in Installer (dep.validate(raise_error=not op.skipped)) still runs afterward and is unaffected: a path dependency that IS needed and IS missing still fails install/sync with a real error, exactly as before. Add a suppressed_loggers mechanism to Command, letting a command raise specific loggers above WARNING for itself unless run verbose/debug. InstallCommand (and SyncCommand, which inherits from it) uses this to suppress poetry.core.packages.path_dependency's premature warning. Logger levels are global process state, so also track which logger names are currently suppressed on Application and reset any a new command doesn't ask for, so suppression from one command doesn't leak into a later command in the same process (relevant for this repo's own test suite, and for anyone using Application programmatically). Resolves: python-poetry#10461
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When resetting previously suppressed loggers in
register_command_loggers, you currently set them to the globallevel, which can overwrite any prior per-logger configuration; consider capturing and restoring each logger’s original level (e.g., via a small mapping keyed by logger name) so that suppression is strictly temporary and does not alter existing logging configuration beyond the lifetime of the command.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When resetting previously suppressed loggers in `register_command_loggers`, you currently set them to the global `level`, which can overwrite any prior per-logger configuration; consider capturing and restoring each logger’s original level (e.g., via a small mapping keyed by logger name) so that suppression is strictly temporary and does not alter existing logging configuration beyond the lifetime of the command.
## Individual Comments
### Comment 1
<location path="src/poetry/console/application.py" line_range="593-594" />
<code_context>
+ # Mutated in place (rather than `self._suppressed_logger_names = ...`)
+ # so the update lands on the shared ClassVar instead of shadowing it
+ # with a same-named instance attribute.
+ for name in Application._suppressed_logger_names - to_suppress:
+ logging.getLogger(name).setLevel(level)
+
+ for name in to_suppress:
</code_context>
<issue_to_address>
**issue (bug_risk):** Restoring suppressed loggers to `level` may unintentionally lower externally configured log levels.
This loop sets each previously suppressed logger to the command’s `level`, which can lower a logger that was externally configured to a stricter level (e.g., CRITICAL) down to WARNING/INFO on later, less-verbose commands. To avoid overriding stricter policies, consider either tracking which loggers were temporarily bumped to ERROR and only resetting those, or resetting with something like `max(existing_level, level)` instead of always applying `level`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
register_command_loggers tracked only which logger *names* a command suppressed (Application._suppressed_logger_names: set[str]), then reset each one to the *next* command's own verbosity level once it stopped being suppressed. That silently overrides any stricter level set on that logger outside this mechanism (an embedding caller, another library, or poetry itself constructing more than one Application() in the same process) - flagged by sourcery-ai on this PR. Track each suppressed logger's actual level from just before suppression instead (_suppressed_logger_levels: dict[str, int]), and restore to that captured value. As a side effect this also stops re-suppressing a logger that's already at ERROR from an earlier command, and makes the "mutated in place" comment on this block accurate again (dict .pop()/[]= are in-place; the old set reassignment wasn't). Add a regression test that pre-sets a logger to CRITICAL, suppresses it via one command, then runs a second, non-suppressing command and asserts it's restored to CRITICAL rather than lowered to WARNING - the exact scenario the previous behavior got wrong. AI-assisted (Claude Code); found via a 10-angle adversarial review that also confirmed sourcery's comment was a real, reachable bug rather than a theoretical one. Verified with the project's own pytest/ruff/mypy before committing.
The new suppressed-logger regression test just broke CI (macOS, Python 3.13, --integration): AssertionError: assert 50 == 40. Root cause was test-order dependence, not a real bug in the fix - the existing test_application_suppressed_loggers_are_raised_above_warning suppresses SUPPRESSIBLE_LOGGER_NAME but never runs a second, non-suppressing command, so it never triggers the restore path and leaves the logger's name (with a stale captured level) sitting in Application._suppressed_logger_levels for whichever test runs next in the same pytest-xdist worker. When that next test happened to be the new one, its own capture step saw the name already tracked and skipped re-suppressing it, so the CRITICAL level it set manually was never touched. Make with_add_warn_command_plugin a yield-fixture that resets both the logger's level and its Application._suppressed_logger_levels entry before and after each test using it, instead of patching only the new test's teardown. Verified with 7 different pytest-randomly seeds (all passed) plus a full tests/console/ run (964 passed).
|
Quick note: the previous push briefly broke CI (macOS/Python 3.13, Fixed by making |
Resolves: #10461
Problem
PathDependency.__init__(in poetry-core) unconditionally logs a warning if a path/directory/file dependency's target doesn't exist, at the point the fullpyproject.tomlis parsed — before this CLI has determined which--with/--without/--onlygroups are actually active for the currentinstall/syncinvocation. So a path dependency in an excluded group (e.g. a dev-only local package that's never copied into a Docker image) gets flagged even though it was never going to be installed.The already-correct, group-aware validation in
Installer(installer.py:373,dep.validate(raise_error=not op.skipped)) still runs afterward and is unaffected — a path dependency that IS actually needed and IS missing still fails install/sync with a real error, exactly as before.Fix
Added a
suppressed_loggersmechanism toCommand(parallel to the existingloggerslistregister_command_loggersalready reads), letting a command raise specific loggers above WARNING for itself, unless run with--verbose/-vv/--debug.InstallCommand(andSyncCommand, which inherits from it) uses this to suppresspoetry.core.packages.path_dependency's premature warning.While testing, found and fixed a related issue: logger levels are global process state, so an earlier version of this raised the level but never reset it for a later command in the same process that doesn't ask for suppression.
Applicationnow tracks which logger names it's currently suppressing and resets any a new command doesn't ask for — relevant for anything running more than one command per process, this repo's own test suite included.Nothing changed in poetry-core; this is scoped entirely to how this CLI configures logging per command.
Tests
tests/console/test_application.py: new test covering default/--verbose/-vv.tests/console/commands/test_install.py: existing fixtures already cover the scenario. Ran the full suite deterministically and with randomized order (several times) specifically to rule out logger-state leakage across tests.Added tests for changed code.
Documentation: none needed, internal logging fix with no user-facing API change.
Note on process: I worked on this with AI assistance (Claude) to investigate the root cause, write the fix, and write/run the tests. I reviewed and understand the changes and I'm the one deciding what's in this PR.