Conversation
Signed-off-by: 석지영 <jiyeong.seok@lge.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded UTF‑8-first subprocess output decoding and helper wrappers; updated Npm, Pypi, Yarn, and PackageManager code to use decoded outputs and improved pip/virtualenv environment handling; removed package-data entry for LICENSES in pyproject.toml. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fosslight_dependency/_package_manager.py (1)
158-163:⚠️ Potential issue | 🟡 MinorRemove the invalid
0integer comparisons on the return value ofcheck_output_safe().
check_output_safe()returns decoded stdout text (never raises if successful), not an integer. The comparisonsret != 0andret == 0are always true and false respectively when comparing strings, making the else block and second if block dead code. Replace with checks for non-empty output usingoutput.strip().Proposed fix
- ret = check_output_safe(cmd, shell=True) - if ret != 0: - self.parse_dependency_tree(ret) + output = check_output_safe(cmd, shell=True) + if output.strip(): + self.parse_dependency_tree(output) else: self.set_direct_dependencies(False) - logger.warning(f"Fail to run {cmd}") + logger.warning(f"No output from {cmd}") @@ - ret = check_output_safe(cmd, shell=True) - if ret == 0: - ret_task = False - logger.error(f'Fail to run {cmd}') + _ = check_output_safe(cmd, shell=True)Also applies to: 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fosslight_dependency/_package_manager.py` around lines 158 - 163, The code incorrectly treats the return value of check_output_safe() as an integer; instead of comparing ret != 0 / ret == 0 you should check whether the returned stdout is non-empty (e.g., if output and output.strip():). Update the two call sites that use check_output_safe() in _package_manager.py (the blocks that call parse_dependency_tree(ret) and set_direct_dependencies(False)) to use a string emptiness check on the returned value (e.g., output = check_output_safe(...); if output and output.strip(): parse_dependency_tree(output) else: set_direct_dependencies(False) and logger.warning(...)). Ensure you use the same variable name (ret or output) consistently and replace both occurrences mentioned in the review.
🧹 Nitpick comments (4)
src/fosslight_dependency/package_manager/Pypi.py (3)
230-236: Apply the same refactor here for consistency.Same redundant decoding pattern as the primary block. Decode stderr once after
subprocess.runand reuse the variable.♻️ Proposed refactor
cmd_ret = subprocess.run(cmd, shell=True, stderr=subprocess.PIPE, env=env) + stderr_text = cmd_ret.stderr.decode('utf-8', errors='replace').strip() if cmd_ret.returncode != 0: ret = False - err_msg = cmd_ret.stderr.decode('utf-8', errors='replace').strip() or f"return code({cmd_ret.returncode})" - elif cmd_ret.stderr.decode('utf-8', errors='replace').strip().lower().startswith('error:'): + err_msg = stderr_text or f"return code({cmd_ret.returncode})" + elif stderr_text.lower().startswith('error:'): ret = False - err_msg = cmd_ret.stderr.decode('utf-8', errors='replace').strip() + err_msg = stderr_text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fosslight_dependency/package_manager/Pypi.py` around lines 230 - 236, In Pypi.py where subprocess.run is invoked (the cmd_ret handling block), avoid decoding stderr multiple times: after cmd_ret = subprocess.run(..., stderr=subprocess.PIPE, env=env) decode stderr once into a variable (e.g., stderr = cmd_ret.stderr.decode('utf-8', errors='replace').strip()) and then use that variable for the return-code error branch and the stderr.startswtih('error:') branch to set ret and err_msg; update the block around cmd_ret to reuse the single decoded stderr value for consistency with the primary block.
211-218: Decode stderr once and reuse to avoid redundant decoding.The stderr is decoded multiple times in this block. Decode once and store in a variable for both the error message assignment and the
'error:'prefix check.♻️ Proposed refactor
- cmd_ret = subprocess.run(cmd, shell=True, stderr=subprocess.PIPE, env=env) + cmd_ret = subprocess.run(cmd, shell=True, stderr=subprocess.PIPE, env=env) + stderr_text = cmd_ret.stderr.decode('utf-8', errors='replace').strip() if cmd_ret.returncode != 0: ret = False - err_msg = cmd_ret.stderr.decode('utf-8', errors='replace').strip() or f"return code({cmd_ret.returncode})" - elif cmd_ret.stderr.decode('utf-8', errors='replace').strip().lower().startswith('error:'): + err_msg = stderr_text or f"return code({cmd_ret.returncode})" + elif stderr_text.lower().startswith('error:'): ret = False - err_msg = cmd_ret.stderr.decode('utf-8', errors='replace').strip() + err_msg = stderr_text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fosslight_dependency/package_manager/Pypi.py` around lines 211 - 218, In the subprocess handling block around subprocess.run (look for cmd_ret variable in Pypi.py), decode cmd_ret.stderr once into a local string (e.g., stderr_str = (cmd_ret.stderr or b'').decode('utf-8', errors='replace').strip()) and then reuse stderr_str for both the return-code error assignment and the startswith('error:') check; set ret and err_msg based on stderr_str (or fallback to f"return code({cmd_ret.returncode})" when empty) instead of decoding cmd_ret.stderr multiple times.
215-218: Use the shareddecode_subprocess_output()utility for consistency across package managers.Pypi.py uses inline
decode('utf-8', errors='replace')at multiple locations (lines 215–218, 233–236, 283, 339), while Yarn.py and Npm.py consistently use the shareddecode_subprocess_output()utility from_package_manager.py. Importdecode_subprocess_outputand replace the inline decoding to maintain consistency across all package managers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fosslight_dependency/package_manager/Pypi.py` around lines 215 - 218, Replace all inline subprocess output decoding in src/fosslight_dependency/package_manager/Pypi.py with the shared utility decode_subprocess_output from _package_manager: add "from ._package_manager import decode_subprocess_output" at the top and change every occurrence of cmd_ret.stderr.decode('utf-8', errors='replace') and cmd_ret.stdout.decode('utf-8', errors='replace') (e.g., the blocks that set err_msg and ret based on cmd_ret) to use decode_subprocess_output(cmd_ret, stream='stderr') or decode_subprocess_output(cmd_ret, stream='stdout') as appropriate so decoding is consistent with Yarn/Npm modules.src/fosslight_dependency/_package_manager.py (1)
43-44: Make shell execution opt-in in the shared helpers.These wrappers centralize subprocess spawning. Keeping
shell=Trueas the default makes future string commands shell out unless every caller remembers to override it. Safer default isshell=False, with explicit opt-in at the few sites that really need a shell. All existing callers already explicitly passshell=True, so this change is safe.Proposed fix
-def run_command(cmd, shell=True, cwd=None, env=None, timeout=None, capture_output=True): +def run_command(cmd, shell=False, cwd=None, env=None, timeout=None, capture_output=True): kwargs = dict(shell=shell, capture_output=capture_output) if cwd: kwargs['cwd'] = cwd @@ -def check_output_safe(cmd, shell=True, cwd=None): +def check_output_safe(cmd, shell=False, cwd=None): result = subprocess.run(cmd, shell=shell, capture_output=True, cwd=cwd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fosslight_dependency/_package_manager.py` around lines 43 - 44, The run_command helper currently defaults to shell=True; change its signature to use shell=False and build kwargs accordingly (e.g., kwargs = dict(shell=shell, capture_output=capture_output)) so callers must opt in to shell usage; update any call sites that rely on the old default to pass shell=True explicitly and ensure run_command and its callers (function name: run_command) still pass through cwd, env, timeout as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fosslight_dependency/_package_manager.py`:
- Around line 30-40: The fallback encoding in decode_subprocess_output should
use the real system encoding via locale.getencoding() instead of
locale.getpreferredencoding(False), falling back to
locale.getpreferredencoding(False) on older Pythons; update the second try block
in decode_subprocess_output to call locale.getencoding() (with a fallback to
getpreferredencoding(False)) and then decode using that encoding, preserving the
existing error-handling branches and the final data.decode(..., errors=errors)
fallback.
---
Outside diff comments:
In `@src/fosslight_dependency/_package_manager.py`:
- Around line 158-163: The code incorrectly treats the return value of
check_output_safe() as an integer; instead of comparing ret != 0 / ret == 0 you
should check whether the returned stdout is non-empty (e.g., if output and
output.strip():). Update the two call sites that use check_output_safe() in
_package_manager.py (the blocks that call parse_dependency_tree(ret) and
set_direct_dependencies(False)) to use a string emptiness check on the returned
value (e.g., output = check_output_safe(...); if output and output.strip():
parse_dependency_tree(output) else: set_direct_dependencies(False) and
logger.warning(...)). Ensure you use the same variable name (ret or output)
consistently and replace both occurrences mentioned in the review.
---
Nitpick comments:
In `@src/fosslight_dependency/_package_manager.py`:
- Around line 43-44: The run_command helper currently defaults to shell=True;
change its signature to use shell=False and build kwargs accordingly (e.g.,
kwargs = dict(shell=shell, capture_output=capture_output)) so callers must opt
in to shell usage; update any call sites that rely on the old default to pass
shell=True explicitly and ensure run_command and its callers (function name:
run_command) still pass through cwd, env, timeout as before.
In `@src/fosslight_dependency/package_manager/Pypi.py`:
- Around line 230-236: In Pypi.py where subprocess.run is invoked (the cmd_ret
handling block), avoid decoding stderr multiple times: after cmd_ret =
subprocess.run(..., stderr=subprocess.PIPE, env=env) decode stderr once into a
variable (e.g., stderr = cmd_ret.stderr.decode('utf-8',
errors='replace').strip()) and then use that variable for the return-code error
branch and the stderr.startswtih('error:') branch to set ret and err_msg; update
the block around cmd_ret to reuse the single decoded stderr value for
consistency with the primary block.
- Around line 211-218: In the subprocess handling block around subprocess.run
(look for cmd_ret variable in Pypi.py), decode cmd_ret.stderr once into a local
string (e.g., stderr_str = (cmd_ret.stderr or b'').decode('utf-8',
errors='replace').strip()) and then reuse stderr_str for both the return-code
error assignment and the startswith('error:') check; set ret and err_msg based
on stderr_str (or fallback to f"return code({cmd_ret.returncode})" when empty)
instead of decoding cmd_ret.stderr multiple times.
- Around line 215-218: Replace all inline subprocess output decoding in
src/fosslight_dependency/package_manager/Pypi.py with the shared utility
decode_subprocess_output from _package_manager: add "from ._package_manager
import decode_subprocess_output" at the top and change every occurrence of
cmd_ret.stderr.decode('utf-8', errors='replace') and
cmd_ret.stdout.decode('utf-8', errors='replace') (e.g., the blocks that set
err_msg and ret based on cmd_ret) to use decode_subprocess_output(cmd_ret,
stream='stderr') or decode_subprocess_output(cmd_ret, stream='stdout') as
appropriate so decoding is consistent with Yarn/Npm modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 254427c9-5b92-4c33-b6c0-86f697745a06
📒 Files selected for processing (4)
src/fosslight_dependency/_package_manager.pysrc/fosslight_dependency/package_manager/Npm.pysrc/fosslight_dependency/package_manager/Pypi.pysrc/fosslight_dependency/package_manager/Yarn.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fosslight_dependency/_package_manager.py (1)
161-166:⚠️ Potential issue | 🟠 MajorFix impossible int checks against decoded stdout text.
check_output_safe()returns decoded stdout text (or raises), so checks likeret != 0/ret == 0are invalid and produce dead/misleading logic.Proposed fix
- ret = check_output_safe(cmd, shell=True) - if ret != 0: - self.parse_dependency_tree(ret) + output = check_output_safe(cmd, shell=True) + if output.strip(): + self.parse_dependency_tree(output) else: self.set_direct_dependencies(False) logger.warning(f"Fail to run {cmd}") @@ - ret = check_output_safe(cmd, shell=True) - if ret == 0: - ret_task = False - logger.error(f'Fail to run {cmd}') + check_output_safe(cmd, shell=True)Also applies to: 174-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fosslight_dependency/_package_manager.py` around lines 161 - 166, The code incorrectly treats the decoded stdout from check_output_safe() as an int and compares it to 0; update the logic in the block using check_output_safe, parse_dependency_tree, set_direct_dependencies and logger.warning to treat ret as text (use truthiness / non-empty string) and handle command failures via exceptions: call check_output_safe(cmd, shell=True) inside a try/except, on success if ret (non-empty string) call parse_dependency_tree(ret), otherwise call set_direct_dependencies(False) and logger.warning(f"Fail to run {cmd}"); apply the same fix to the similar block that appears later (the second occurrence mentioned for lines 174-177).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fosslight_dependency/_package_manager.py`:
- Around line 52-53: The current conditional uses truthiness for timeout and
therefore skips valid falsy values like 0; update the check in the block that
sets kwargs (the code referencing timeout and kwargs) to use an explicit None
check (if timeout is not None:) so timeout=0 is preserved and only None is
omitted.
- Around line 46-47: Change the default shell behavior to avoid command
injection: update run_command and check_output_safe to default shell=False (or
remove the shell parameter) and require/accept argument lists instead of shell
strings; then refactor all callers that currently pass interpolated f-strings
(e.g., calls like check_output_safe(cmd, shell=True) where cmd = f"{cmd_gradle}
allDeps") to pass an argv list (e.g., [cmd_gradle, "allDeps"]) or use
shlex.split before calling; ensure subprocess invocations inside run_command and
check_output_safe use the list directly and only set shell=True when absolutely
necessary and explicitly reviewed.
---
Outside diff comments:
In `@src/fosslight_dependency/_package_manager.py`:
- Around line 161-166: The code incorrectly treats the decoded stdout from
check_output_safe() as an int and compares it to 0; update the logic in the
block using check_output_safe, parse_dependency_tree, set_direct_dependencies
and logger.warning to treat ret as text (use truthiness / non-empty string) and
handle command failures via exceptions: call check_output_safe(cmd, shell=True)
inside a try/except, on success if ret (non-empty string) call
parse_dependency_tree(ret), otherwise call set_direct_dependencies(False) and
logger.warning(f"Fail to run {cmd}"); apply the same fix to the similar block
that appears later (the second occurrence mentioned for lines 174-177).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89b2c489-ceff-4e95-b97a-1ef5dd8c2b19
📒 Files selected for processing (2)
pyproject.tomlsrc/fosslight_dependency/_package_manager.py
💤 Files with no reviewable changes (1)
- pyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fosslight_dependency/_package_manager.py (1)
161-166:⚠️ Potential issue | 🔴 CriticalType mismatch:
check_output_safe()returns a string, not an exit code.
check_output_safe()returns decoded stdout as a string, but line 162 compares it to integer0. This comparison is alwaysTruefor any string, making theelsebranch (lines 165-166) unreachable dead code. The logic should check for non-empty output instead.Proposed fix
ret = check_output_safe(cmd, shell=True) - if ret != 0: + if ret: self.parse_dependency_tree(ret) else: self.set_direct_dependencies(False) logger.warning(f"Fail to run {cmd}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fosslight_dependency/_package_manager.py` around lines 161 - 166, The code incorrectly treats ret from check_output_safe(cmd, shell=True) as an exit code; change the conditional to check for non-empty string output (e.g., if ret: ...) so that parse_dependency_tree(ret) runs when output exists and the else branch calls self.set_direct_dependencies(False) and logger.warning(f"Fail to run {cmd}") when ret is empty or falsy; update the block that references ret, cmd, parse_dependency_tree and set_direct_dependencies accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fosslight_dependency/_package_manager.py`:
- Around line 174-177: Remove the unreachable branch that checks "if ret == 0"
after calling check_output_safe(cmd, shell=True) in _package_manager:
check_output_safe returns a decoded string (or raises CalledProcessError on
failure), so the ret == 0 condition is always false; delete the if block that
sets ret_task = False and calls logger.error(f'Fail to run {cmd}'), leaving the
call to check_output_safe and existing except handling intact to report
failures.
- Around line 40-43: The current try/except around data.decode(system_encoding)
mishandles LookupError because retrying with the same invalid system_encoding
will raise again; update the code to handle UnicodeDecodeError and LookupError
separately: keep the UnicodeDecodeError branch to retry with
data.decode(system_encoding, errors=errors), but add a LookupError branch that
falls back to a known-good encoding (e.g., 'utf-8' or 'latin-1') and decodes
with data.decode(fallback_encoding, errors=errors). Reference the existing
symbols system_encoding, data.decode(...), and errors when implementing the
separate except clauses and the fallback decode.
---
Outside diff comments:
In `@src/fosslight_dependency/_package_manager.py`:
- Around line 161-166: The code incorrectly treats ret from
check_output_safe(cmd, shell=True) as an exit code; change the conditional to
check for non-empty string output (e.g., if ret: ...) so that
parse_dependency_tree(ret) runs when output exists and the else branch calls
self.set_direct_dependencies(False) and logger.warning(f"Fail to run {cmd}")
when ret is empty or falsy; update the block that references ret, cmd,
parse_dependency_tree and set_direct_dependencies accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee7d60ac-85b5-481c-81b0-3193cff259fc
📒 Files selected for processing (2)
pyproject.tomlsrc/fosslight_dependency/_package_manager.py
💤 Files with no reviewable changes (1)
- pyproject.toml
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
1 participant
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.
Description
fix(encoding): handle non-UTF-8 system encoding in subprocess output
to safely decode subprocess output using locale-aware fallback
(UTF-8 → system encoding → replace)
to handle cp949/euc-kr and other non-UTF-8 system encodings
to fix UnicodeDecodeError in _readerthread on non-UTF-8 systems
Summary by CodeRabbit
Bug Fixes
Chores