Repository navigation
fix(config): report the setting name for invalid integer values - #11149
XXXXXQ-0206 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/poetry/console/commands/config.py" line_range="389" />
<code_context>
+ # Some validators (e.g. those that parse integers) raise instead
+ # of returning False; surface them like any other invalid value.
+ valid = False
+ if not valid:
raise RuntimeError(f'"{value}" is an invalid value for {key}')
</code_context>
<issue_to_address>
**Invalid integers inject terminal controls**
When a CI caller passes an invalid integer containing terminal escape sequences to `poetry config`, `_handle_single_value` now treats the validator’s `ValueError` as invalid input and reaches the direct `f"{value}"` error at `config.py:390`; the CLI emits embedded terminal control characters, allowing the supplied value to manipulate terminal output.
Escape terminal control characters in the displayed value before constructing the error message.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: src/poetry/console/commands/config.py:389
| # Some validators (e.g. those that parse integers) raise instead | ||
| # of returning False; surface them like any other invalid value. | ||
| valid = False | ||
| if not valid: |
There was a problem hiding this comment.
🟡 Medium · Invalid integers inject terminal controls
When a CI caller passes an invalid integer containing terminal escape sequences to poetry config, _handle_single_value now treats the validator’s ValueError as invalid input and reaches the direct f"{value}" error at config.py:390; the CLI emits embedded terminal control characters, allowing the supplied value to manipulate terminal output.
Escape terminal control characters in the displayed value before constructing the error message.
Prompt for AI agents
In `src/poetry/console/commands/config.py` at line 389:
**Invalid integers inject terminal controls**
When a CI caller passes an invalid integer containing terminal escape sequences to `poetry config`, `_handle_single_value` now treats the validator’s `ValueError` as invalid input and reaches the direct `f"{value}"` error at `config.py:390`; the CLI emits embedded terminal control characters, allowing the supplied value to manipulate terminal output.
Escape terminal control characters in the displayed value before constructing the error message.
What does this PR do?
Invalid values for the three integer settings now report the same uniform error as every other setting, instead of the raw
int()conversion error.Why
ConfigCommand._handle_single_value()produces"{value}" is an invalid value for {key}for validators that returnFalse; that message is pinned bytests/console/commands/test_config.py::test_cannot_set_invalid_value. The validators forrequests.max-retries,installer.max-workersandsolver.min-release-ageare lambdas callingint(), so they raiseValueErrorand bypass that message - the user sawinvalid literal for int() with base 10: 'abc'without knowing which setting failed.This change treats a raising validator as an invalid value, keeping the pinned message unchanged:
The environment-variable path gets the same treatment in
Config.get():POETRY_REQUESTS_MAX_RETRIES=abcnow raisesPoetryError: Invalid value for POETRY_REQUESTS_MAX_RETRIES: 'abc'instead of the unrelatedValueErrormessage.Tests
test_cannot_set_invalid_integer_value(three parametrized settings) andtest_invalid_integer_from_environmentboth fail onmainwithValueErrorand pass with this change.Related
No duplicate open or closed PR covers this path: exact-error-text searches return nothing. Other open PRs touch the same files in different regions (#11057 in
console/commands/config.pynear line 140; #11115, #11116 and #11120 inconfig.pynear lines 334-342, 248 and 427), so merge order should not conflict.Resolves #11148