Skip to content

Conversation

@xianglongfei-8888
Copy link

@xianglongfei-8888 xianglongfei-8888 commented Nov 25, 2025

sudo id -u : 'b'0''
image

System_output returns a byte string (bytes), not a string (str).
Will cause "can_sudo" to consistently return False
We need to use. decode ('utf-8 ') to convert the byte string into a string.

Summary by CodeRabbit

  • Bug Fixes
    • Improved system privilege detection to prevent incorrect checks caused by data type mismatches, ensuring more reliable root/sudo detection.

✏️ Tip: You can customize this high-level summary in your review settings.

@mr-avocado mr-avocado bot moved this to Review Requested in Default project Nov 25, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 25, 2025

Warning

Rate limit exceeded

@xianglongfei-8888 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 6 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between b1804d5 and 6d01c2f.

📒 Files selected for processing (1)
  • avocado/utils/process.py (1 hunks)

Walkthrough

The pull request modifies the can_sudo function in avocado/utils/process.py by replacing a system_output("id -u", ignore_status=True, sudo=True) call with getoutput("id -u", sudo=True). The change shifts from using a bytes-returning system_output path to a string-returning getoutput path and performs the UID check by comparing the resulting string (after strip()) to "0".

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Single file and single-function change
  • Simple call replacement that changes return type from bytes to string
  • Low logic density and no structural refactor

Areas to spot-check:

  • Confirm getoutput(..., sudo=True) preserves the same exit/ignore semantics previously relied upon (e.g., behavior on non-root or permission failures).

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title describes the problem being fixed (System_output returns bytes instead of str), which aligns with the changeset replacing system_output with getoutput to handle string results.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
avocado/utils/process.py (1)

82-82: Fix is correct; consider using getoutput() for cleaner code.

The decode is necessary since system_output returns bytes. However, this file already provides getoutput() (lines 1162-1220) which returns a string directly and handles encoding automatically, eliminating the need for manual decoding.

Apply this diff to use getoutput() for cleaner, more maintainable code:

-        if system_output("id -u", ignore_status=True, sudo=True).decode('utf-8').strip() == "0":
+        if getoutput("id -u", sudo=True).strip() == "0":

Note: getoutput() has ignore_status=True as its default, so it can be omitted.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 74b7379 and 95baa3b.

📒 Files selected for processing (1)
  • avocado/utils/process.py (1 hunks)

@xianglongfei-8888
Copy link
Author

@clebergnu Could you help review the code? Thanks!
This function needs to be called:#6241

@clebergnu clebergnu self-requested a review November 25, 2025 15:09
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @xianglongfei-8888 ,

Good catch! The result of system_output is indeed bytes and not a string. But, the better solution is to avoid forcing encoding types because the system under which the id -u command runs may be configured with a different locale. It's true that the result should be numeric only, which would always decode properly under "utf-8", but, it's a good practice to not assume that.

So, my suggestion is to use avocado.utils.process.getoutput() which uses the default encoding (from avocado.utils.atring.ENCODING).

@xianglongfei-8888
Copy link
Author

Hi @xianglongfei-8888 ,

Good catch! The result of system_output is indeed bytes and not a string. But, the better solution is to avoid forcing encoding types because the system under which the id -u command runs may be configured with a different locale. It's true that the result should be numeric only, which would always decode properly under "utf-8", but, it's a good practice to not assume that.

So, my suggestion is to use avocado.utils.process.getoutput() which uses the default encoding (from avocado.utils.atring.ENCODING).

@clebergnu Yes, you're absolutely right. Thank you for your advice

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.52%. Comparing base (74b7379) to head (6d01c2f).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6242      +/-   ##
==========================================
+ Coverage   73.48%   73.52%   +0.03%     
==========================================
  Files         206      206              
  Lines       22494    22497       +3     
==========================================
+ Hits        16530    16540      +10     
+ Misses       5964     5957       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@clebergnu clebergnu merged commit 9ac2f2d into avocado-framework:master Dec 13, 2025
64 checks passed
@github-project-automation github-project-automation bot moved this from Review Requested to Done 113 in Default project Dec 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done 113

Development

Successfully merging this pull request may close these issues.

2 participants