codeant-ai-for-open-source[bot] commented on code in PR #37015:
URL: https://github.com/apache/superset/pull/37015#discussion_r2676736828
##########
scripts/check-env.py:
##########
@@ -47,11 +47,13 @@ def __init__(
def get_version(self) -> Optional[str]:
try:
- version = subprocess.check_output(self.command,
shell=True).decode().strip() # noqa: S602
+ output = subprocess.check_output(self.command,
shell=True).decode().strip() # noqa: S602
Review Comment:
**Suggestion:** get_version only captures stdout, but some commands (notably
older Python versions) print their version to stderr; as written the call can
return an empty string and cause the tool to report a binary as "Not
Installed". Capture stderr as well so the version is read reliably. [possible
bug]
**Severity Level:** Critical 🚨
```suggestion
output = subprocess.check_output(self.command, shell=True,
stderr=subprocess.STDOUT).decode().strip() # noqa: S602
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a real, common issue: some binaries (notably certain Python
releases) print their version to stderr.
Adding stderr=subprocess.STDOUT to capture stderr is a minimal, correct
change that prevents false negatives
when parsing versions. It doesn't harm normal stdout cases and keeps the
existing shell behavior intact.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** scripts/check-env.py
**Line:** 50:50
**Comment:**
*Possible Bug: get_version only captures stdout, but some commands
(notably older Python versions) print their version to stderr; as written the
call can return an empty string and cause the tool to report a binary as "Not
Installed". Capture stderr as well so the version is read reliably.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
scripts/check-env.py:
##########
@@ -47,11 +47,13 @@ def __init__(
def get_version(self) -> Optional[str]:
try:
- version = subprocess.check_output(self.command,
shell=True).decode().strip() # noqa: S602
+ output = subprocess.check_output(self.command,
shell=True).decode().strip() # noqa: S602
if self.version_post_process:
- version = self.version_post_process(version)
- return version.split()[-1]
- except subprocess.CalledProcessError:
+ output = self.version_post_process(output)
Review Comment:
**Suggestion:** The post-processing callback (`version_post_process`) is
called without guarding against exceptions or non-string returns; if it raises
or returns a non-string (e.g., None) the subsequent `output.split()` will
raise, crashing the script. Validate and normalize the post-processed value (or
handle exceptions) before using `split()`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
try:
output = self.version_post_process(output)
except Exception:
# If post-processing fails, treat as not found
return None
# Ensure we have a string to split
if output is None:
return None
if not isinstance(output, str):
output = str(output)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The post-processing callback runs inside the try but only specific
subprocess exceptions are caught.
If the callback raises or returns None/non-string, you'll get an unhandled
exception (or AttributeError on split),
crashing the script. Guarding the callback and normalizing its return value
to a string (or returning None on failure)
fixes a visible bug. The suggested patch is pragmatic; catching Exception is
coarse but acceptable here.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** scripts/check-env.py
**Line:** 52:52
**Comment:**
*Possible Bug: The post-processing callback (`version_post_process`) is
called without guarding against exceptions or non-string returns; if it raises
or returns a non-string (e.g., None) the subsequent `output.split()` will
raise, crashing the script. Validate and normalize the post-processed value (or
handle exceptions) before using `split()`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
