-
Notifications
You must be signed in to change notification settings - Fork 1
Avoid steering pytest installation commands #634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||||||||
| import shlex | ||||||||||||||||||||||||||||||||||||
| from dataclasses import dataclass | ||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||||||||
|
|
@@ -27,7 +28,20 @@ | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| # Matches commands invoking pytest (pytest, python -m pytest, py.test, etc.) | ||||||||||||||||||||||||||||||||||||
| _PYTEST_ROOT_PATTERN = re.compile(r"\b(pytest|py\.test)(?:\b|\.py\b)", re.IGNORECASE) | ||||||||||||||||||||||||||||||||||||
| _PYTEST_ROOT_PATTERN = re.compile( | ||||||||||||||||||||||||||||||||||||
| r"^(pytest|py\.test)(?:$|\.py$|\.exe$|\.bat$)", | ||||||||||||||||||||||||||||||||||||
| re.IGNORECASE, | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| def _token_invokes_pytest(token: str) -> bool: | ||||||||||||||||||||||||||||||||||||
| """Return True when the token represents a pytest executable.""" | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if not token: | ||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| executable_name = Path(token).name | ||||||||||||||||||||||||||||||||||||
| return bool(_PYTEST_ROOT_PATTERN.fullmatch(executable_name)) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| DEFAULT_STEERING_MESSAGE = ( | ||||||||||||||||||||||||||||||||||||
|
|
@@ -91,6 +105,56 @@ def _normalize_whitespace(command: str) -> str: | |||||||||||||||||||||||||||||||||||
| return " ".join(command.strip().split()) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| def _split_command_tokens(command: str) -> list[str]: | ||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||
| return shlex.split(command, posix=True) | ||||||||||||||||||||||||||||||||||||
| except ValueError: | ||||||||||||||||||||||||||||||||||||
| return command.split() | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
Comment on lines
+108
to
+114
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Windows path tokenization bug: shlex POSIX parsing mangles backslashes. For commands like Apply a Windows-aware fallback in def _split_command_tokens(command: str) -> list[str]:
- try:
- return shlex.split(command, posix=True)
- except ValueError:
- return command.split()
+ # Prefer Windows parsing when an absolute Windows path is present (e.g., C:\...).
+ # This avoids POSIX backslash-escaping mangling Windows paths.
+ if re.search(r'(?i)\b[A-Z]:\\', command):
+ try:
+ return shlex.split(command, posix=False)
+ except ValueError:
+ pass
+ try:
+ return shlex.split(command, posix=True)
+ except ValueError:
+ return command.split()Please add a handler-level test for 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| def _command_invokes_pytest(command: str) -> bool: | ||||||||||||||||||||||||||||||||||||
| tokens = _split_command_tokens(command) | ||||||||||||||||||||||||||||||||||||
| if not tokens: | ||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| separators = {"&&", ";", "||", "|"} | ||||||||||||||||||||||||||||||||||||
| last_separator_index = -1 | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| for index, token in enumerate(tokens): | ||||||||||||||||||||||||||||||||||||
| if token in separators: | ||||||||||||||||||||||||||||||||||||
| last_separator_index = index | ||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if not _token_invokes_pytest(token): | ||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| segment_start = last_separator_index + 1 | ||||||||||||||||||||||||||||||||||||
| segment_tokens = tokens[segment_start : index + 1] | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if _segment_represents_installation(segment_tokens[:-1]): | ||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| def _segment_represents_installation(tokens: list[str]) -> bool: | ||||||||||||||||||||||||||||||||||||
| if not tokens: | ||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| installation_keywords = { | ||||||||||||||||||||||||||||||||||||
| "install", | ||||||||||||||||||||||||||||||||||||
| "add", | ||||||||||||||||||||||||||||||||||||
| "remove", | ||||||||||||||||||||||||||||||||||||
| "uninstall", | ||||||||||||||||||||||||||||||||||||
| "update", | ||||||||||||||||||||||||||||||||||||
| "upgrade", | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return any(token.lower() in installation_keywords for token in tokens) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| def _looks_like_full_suite(command: str) -> bool: | ||||||||||||||||||||||||||||||||||||
| """Determine if the pytest command targets the entire suite. | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
@@ -101,7 +165,7 @@ def _looks_like_full_suite(command: str) -> bool: | |||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| normalized = _normalize_whitespace(command) | ||||||||||||||||||||||||||||||||||||
| if not _PYTEST_ROOT_PATTERN.search(normalized): | ||||||||||||||||||||||||||||||||||||
| if not normalized: | ||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| tokens = normalized.split() | ||||||||||||||||||||||||||||||||||||
|
|
@@ -110,7 +174,7 @@ def _looks_like_full_suite(command: str) -> bool: | |||||||||||||||||||||||||||||||||||
| # Identify index where pytest command appears and inspect subsequent tokens. | ||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||
| pytest_index = next( | ||||||||||||||||||||||||||||||||||||
| i for i, tok in enumerate(tokens) if _PYTEST_ROOT_PATTERN.search(tok) | ||||||||||||||||||||||||||||||||||||
| i for i, tok in enumerate(tokens) if _token_invokes_pytest(tok) | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| except StopIteration: | ||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||
|
|
@@ -254,14 +318,16 @@ def _extract_pytest_command(self, context: ToolCallContext) -> str | None: | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| command = _extract_command(arguments) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if normalized_tool_name in shell_tools: | ||||||||||||||||||||||||||||||||||||
| if command and normalized_tool_name in shell_tools: | ||||||||||||||||||||||||||||||||||||
| if not _command_invokes_pytest(command): | ||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||
| return command | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| # Some providers map pytest directly as function name | ||||||||||||||||||||||||||||||||||||
| if _PYTEST_ROOT_PATTERN.search(tool_name): | ||||||||||||||||||||||||||||||||||||
| if _PYTEST_ROOT_PATTERN.fullmatch(tool_name): | ||||||||||||||||||||||||||||||||||||
| return command or tool_name | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if command and _PYTEST_ROOT_PATTERN.search(command): | ||||||||||||||||||||||||||||||||||||
| if command and _command_invokes_pytest(command): | ||||||||||||||||||||||||||||||||||||
| prefix = tool_name | ||||||||||||||||||||||||||||||||||||
| if prefix: | ||||||||||||||||||||||||||||||||||||
| return f"{prefix} {command}".strip() | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated regex now insists that each token be exactly
pytest/py.test(viafullmatch), so_extract_pytest_commandignores commands like./.venv/bin/pytestorC:\venv\Scripts\pytest.exe. These are common ways to invoke pytest from virtual environments and were previously matched because the pattern searched forpytestanywhere in the token. After this change, full‑suite runs executed via a path skip the steering logic entirely, defeating the handler’s purpose.Useful? React with 👍 / 👎.