Fixed the issue:Supports specifying port ranges - #6281
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of the Avocado framework's status server by enabling the specification of port ranges for both listening and connection URIs. This change allows the system to automatically identify and utilize an available port within a defined range, which is particularly useful in dynamic or constrained network environments. The implementation also ensures consistent configuration by synchronizing the resolved port across relevant settings, streamlining the setup process for users. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by adding support for specifying port ranges for the status server. However, the implementation of the port range resolution logic is vulnerable to a Denial of Service (DoS) due to unhandled OverflowError exceptions when port numbers are outside the valid range (0-65535), potentially crashing the Avocado runner process via malformed command-line arguments. Additionally, a high-severity issue exists with IPv6 support in the new port-finding logic, currently limited to IPv4, and a minor refactoring is suggested to remove a duplicated logging statement.
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| try: | ||
| sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| sock.bind((host, port)) |
There was a problem hiding this comment.
The current implementation for finding an open port is hardcoded to IPv4 (socket.AF_INET) and does not correctly handle IPv6 addresses (e.g., [::1]). The bind call will fail for an IPv6 literal with brackets, and the address family is incorrect.
To properly support both IPv4 and IPv6, you should use socket.getaddrinfo to determine the correct address family and prepare the host string for the bind call.
| start = int(start_s) | ||
| end = int(end_s) | ||
| if start > end: | ||
| raise ValueError( | ||
| f"Invalid port range (start > end) in status server URI: {uri}" | ||
| ) | ||
|
|
||
| last_exc = None |
There was a problem hiding this comment.
The resolve_listen_uri function is vulnerable to a Denial of Service (DoS) vulnerability. It iterates through a user-provided port range and attempts to bind a socket to each port. If the port number is outside the valid range (0-65535), socket.bind() raises an OverflowError. This exception is not caught in the calling code in avocado/plugins/runner_nrunner.py (which only handles ValueError and OSError), causing the entire Avocado runner process to crash. An attacker or user providing a port range like 127.0.0.1:65530-70000 or 127.0.0.1:-1-10 can trigger this crash. Validating the port range before entering the loop is recommended.
start = int(start_s)
end = int(end_s)
if start > end:
raise ValueError(
f"Invalid port range (start > end) in status server URI: {uri}"
)
if not (0 <= start <= 65535 and 0 <= end <= 65535):
raise ValueError(
f"Port range out of bounds (0-65535) in status server URI: {uri}"
)
last_exc = None| if ":" in self._uri: | ||
| host, port = self._uri.split(":") | ||
| host, port = self._uri.rsplit(":", 1) | ||
| port = int(port) | ||
| self._server_task = await asyncio.start_server( | ||
| self.cb, host=host, port=port, limit=limit | ||
| ) | ||
| LOG_JOB.info("Status server listening on %s", self._uri) | ||
| else: | ||
| self._server_task = await asyncio.start_unix_server( | ||
| self.cb, path=self._uri, limit=limit | ||
| ) | ||
| LOG_JOB.info("Status server listening on %s", self._uri) |
There was a problem hiding this comment.
The logging statement LOG_JOB.info(...) is duplicated in both branches of the if/else statement. You can avoid repetition by moving it after the block.
if ":" in self._uri:
host, port = self._uri.rsplit(":", 1)
port = int(port)
self._server_task = await asyncio.start_server(
self.cb, host=host, port=port, limit=limit
)
else:
self._server_task = await asyncio.start_unix_server(
self.cb, path=self._uri, limit=limit
)
LOG_JOB.info("Status server listening on %s", self._uri)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6281 +/- ##
==========================================
+ Coverage 73.48% 73.65% +0.17%
==========================================
Files 206 206
Lines 22494 22655 +161
==========================================
+ Hits 16530 16687 +157
- Misses 5964 5968 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b7f0504 to
c19157a
Compare
clebergnu
left a comment
clebergnu
left a comment
There was a problem hiding this comment.
Hi @xianglongfei-8888 , thanks for working on this.
Issue #5550 mentions that the outcome desired is to have this behavior by default, that is, it should allow users running avocado multiple times to not hit the status server port allocation. Have you looked into making the port range the default setting?
|
|
||
| last_exc = None | ||
| for port in range(start, end + 1): | ||
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) |
There was a problem hiding this comment.
This should really reuse avocado.utils.network.ports.find_free_port()
There was a problem hiding this comment.
This should really reuse avocado.utils.network.ports.find_free_port()
@clebergnu Thank you, it has been modified
88b0438 to
bf1310f
Compare
@clebergnu Thank you, it has been changed to default |
|
@richtja Could you help review the code？ Thanks |
|
Hi @xianglongfei-8888, would you mind rebasing on most recent master so we can clear the merge conflicts and possibly rerun the CI? |
@pevogam Well noted with thanks. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughStatus-server listen URIs now support port ranges. The server selects an available port, rejects reversed or unavailable ranges, and logs the concrete endpoint. The nrunner plugin uses the new default range, converts resolution errors to Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
avocado/utils/podman.py (1)
1339-1345: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
AsyncPodman.executefails when an argument is not a string.Line 1340 calls
" ".join(args)in the error path. Theuserbranch at Line 1315 already converts each argument withstr(arg), andPodman.executewas updated at Line 640 to use" ".join(str(a) for a in args). This error path was not updated. A non-string argument, for example the integer timeout used byrestart, raisesTypeErrorinstead ofPodmanException.
AsyncPodman.restartpassesf"-t={timeout}", which is a string, so the current callers are safe. New callers that pass integers are not.🐛 Proposed fix
if proc.returncode: - command_args = " ".join(args) + command_args = " ".join(str(a) for a in args)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 1339 - 1345, Update the error-path command formatting in AsyncPodman.execute to convert every args element to str before joining, matching the existing user branch and Podman.execute behavior. Preserve the existing PodmanException message and return-code handling for commands containing non-string arguments.
🟠 Major comments (22)
optional_plugins/spawner_remote/avocado_spawner_remote/__init__.py-89-97 (1)
89-97: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not block the asyncio event loop with remote commands.
session.cmd_status_output()can block for its full timeout.is_task_alive()calls it up to ten times and then callstime.sleep(1).wait_task()invokes this method from the event loop. A slow or disconnected remote host can block all runtime tasks for more than 100 seconds. Setup hooks can block the same loop forsetup_timeout.Run the blocking session calls in an executor, or make the helper asynchronous. Replace
time.sleep()withawait asyncio.sleep()after makingis_task_alive()asynchronous.Also applies to: 151-160, 186-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@optional_plugins/spawner_remote/avocado_spawner_remote/__init__.py` around lines 89 - 97, Make remote session operations non-blocking throughout run_remote_cmd, is_task_alive, wait_task, and the setup-hook flow: execute blocking session calls such as session.cmd_status_output() in an executor or convert the helpers to async, and replace time.sleep() with await asyncio.sleep() in is_task_alive. Ensure wait_task and setup hooks await the asynchronous helpers so the asyncio event loop is never blocked during remote-command timeouts.optional_plugins/spawner_remote/avocado_spawner_remote/__init__.py-197-199 (1)
197-199: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep
spawner.remote.test_timeouteffective for background tasks.The trailing
&makescmd_status_output()wait only for the launch shell. It no longer enforcesspawner.remote.test_timeoutontask-run. A hung remote task can therefore outlive this spawner-specific timeout.Enforce the timeout in the background process or track its PID and terminate it when the deadline expires.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@optional_plugins/spawner_remote/avocado_spawner_remote/__init__.py` around lines 197 - 199, Update the background task execution around RemoteSpawner.run_remote_cmd so spawner.remote.test_timeout remains effective for task-run despite the trailing “&”. Enforce the deadline within the remote background process or capture its PID and terminate it when the timeout expires, while preserving the existing command launch and status handling.avocado/plugins/runner_nrunner.py-231-235 (1)
231-235: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate the resolved port to a ranged status URI with a different host.
Line 234 only updates
run.status_server_uriwhen it exactly equalslistenorDEFAULT_SERVER_URI. A manual configuration such asrun.status_server_listen=0.0.0.0:8888-9000andrun.status_server_uri=127.0.0.1:8888-9000leaves tasks with the unresolved range. The server selects one port, but task status connections use an invalid endpoint.When
run.status_server_uricontains a range, preserve its host and replace its port specification with the port selected forresolved_listen. Validate that the configured URI range can represent that selected port.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/plugins/runner_nrunner.py` around lines 231 - 235, Update the resolved-listen handling in the runner flow to also rewrite ranged run.status_server_uri values when their host differs from run.status_server_listen: preserve the URI host, replace its port range with the port selected in resolved_listen, and validate that the configured range includes that port. Retain the existing exact-match and DEFAULT_SERVER_URI behavior, and avoid updating ranges that cannot represent the selected port.avocado/core/suite.py-374-380 (1)
374-380: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow a resume with no remaining runnables.
If every runnable is in
completed_set, Line 377 makessuite.testsempty. Lines 384-390 then raiseTestSuiteErrorfor a valid fully completed resume. Handle this case separately from unresolved references soavocado replay --resumecan complete successfully when there is nothing left to run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/core/suite.py` around lines 374 - 380, Update the resume handling around cls._runnable_name and suite.resume_start_index so an empty suite.tests after filtering completed_set is treated as a valid fully completed resume. Distinguish this from unresolved completed-test references, preserving TestSuiteError for references that do not resolve while allowing avocado replay --resume to finish successfully when all runnables were completed.avocado/plugins/runner_nrunner.py-305-309 (1)
305-309: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBind the status server before starting workers.
resolve_listen_uri()only probes a free port. Another process can bind that port beforeserve_forever()runs. Ifasyncio.start_server()then fails,status_server_taskfinishes with an unobserved exception andself.status_server.close()later callsclose()on an unset server object.Create and await the server before creating workers. Convert bind failures to
JobError. If the selected port is no longer available, retry another port in the configured range or fail the job with the bind error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/plugins/runner_nrunner.py` around lines 305 - 309, Update the status-server startup flow around resolve_listen_uri() and status_server_task to bind and await the server before creating workers. Handle asyncio.start_server() bind failures by retrying an available port within the configured range, and convert an unrecoverable bind error to JobError; only create status_server_task after successful binding and ensure cleanup never closes an uninitialized server.avocado/plugins/replay.py-171-179 (1)
171-179: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTraverse replay ancestry and reject an empty source reference.
Line 178 matches every job with a missing
job.replay.source_job_id, because every string starts with"". This adds unrelated PASS/SKIP names and can skip tests in the new replay. The scan also only finds jobs that directly referenceresults_dir; when a user resumes a replay job, it does not collect completed tests from that replay job’s source ancestors.Resolve each non-empty source reference to a canonical job directory. Walk ancestors with cycle detection. Match descendants only against those canonical chain members. Add coverage for a replay whose source is another replay and for a job with no replay source key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/plugins/replay.py` around lines 171 - 179, Update the replay job matching logic around the source reference and results directory scan: ignore empty or missing job.replay.source_job_id values, resolve each non-empty reference to a canonical job directory, and traverse its replay ancestry with cycle detection. Match descendants only against directories in that canonical ancestor chain, preserving direct results_dir and absolute-path support, and add coverage for nested replays and jobs without a replay source.avocado/utils/podman.py-1609-1611 (1)
1609-1611: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
AsyncPodman.collect_container_statsdrops theuserparameter.
Podman.collect_container_statsacceptsuserat Line 877 and forwards it toexecute. The asynchronous version has nouserparameter, and_collect_single_container_statsat Line 1661 callsexecutewithout a user. Code that runs rootless containers as a specific user cannot collect statistics through the asynchronous API.Add
user=Noneto both asynchronous methods and forward it toexecute.Also applies to: 1652-1654, 1661-1663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 1609 - 1611, Update AsyncPodman.collect_container_stats and _collect_single_container_stats to accept user=None, propagate the value through their internal call, and pass it to execute so asynchronous statistics collection supports user-specific rootless containers.avocado/utils/podman.py-852-857 (1)
852-857: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe synchronous and asynchronous
su/nohupnesting differ.The synchronous variant at Line 853 produces
nohup su - USER -c '<cmd>' > /dev/null 2>&1 &. The asynchronous variant at Line 1586 producessu - USER -c 'nohup bash -c ... &'. In the asynchronous form, the background job belongs to thesulogin shell, andsuexits when that shell exits. The lifetime of the collector and the meaning of the returnedprocess.pidtherefore differ between the two methods.Both methods document the same behavior and return
subprocess.Popen. Use one form in both.Also applies to: 1584-1590
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 852 - 857, Unify the `su`/`nohup` command nesting in the synchronous and asynchronous execution paths so both use the same process-lifetime behavior and `subprocess.Popen` PID semantics. Update the command construction around `full_command` in the shown path and its corresponding asynchronous implementation, preserving the existing user and non-user branches.avocado/utils/podman.py-98-128 (1)
98-128: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winQuote
container_idanduserin thesu -ccommand strings.
get_container_port,save_container_logs, andwait_for_vllm_startupinterpolatecontainer_idinto a shell command string thatsu -cruns. Acontainer_idvalue with shell metacharacters executes arbitrary commands as the target user. The non-user branches pass argument lists and are safe, so the risk exists only in thesubranches.Use
shlex.quotefor every interpolated value.🔒 Proposed change (apply the same pattern at Lines 195 and 295)
- f"XDG_RUNTIME_DIR={xdg_runtime_dir} podman port {container_id} {port}", + f"XDG_RUNTIME_DIR={shlex.quote(xdg_runtime_dir)} " + f"podman port {shlex.quote(str(container_id))} {int(port)}",Also applies to: 178-209, 276-310
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 98 - 128, Update the su -c command construction in get_container_port, save_container_logs, and wait_for_vllm_startup to shell-quote every interpolated container_id and user value using shlex.quote (including the XDG runtime path where interpolated). Keep the non-user subprocess argument-list branches unchanged and apply the same protection to all affected su command strings.avocado/utils/podman.py-612-628 (1)
612-628: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Podman.executeandAsyncPodman.executetreatuser="root"differently.
Podman.executeusesif user:at Line 623.AsyncPodman.executeusesif user and user != "root"at Line 1312. Withuser="root", the synchronous path runssu - root -c ...and the asynchronous path runs podman directly.su - rootstarts a login shell, which changes the environment, the working directory, and theXDG_RUNTIME_DIRvalue. The two paths therefore produce different results for the same argument.Align the synchronous condition with the asynchronous one.
🐛 Proposed fix
- if user: + if user and user != "root": podman_cmd = [self.podman_bin] + list(args)Also applies to: 1301-1329
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 612 - 628, Update the user-selection condition in Podman.execute to bypass su when user is "root", matching AsyncPodman.execute; only non-root user values should construct and run the su command, while root and absent user values should execute podman directly.avocado/utils/podman.py-846-857 (1)
846-857: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUnquoted interpolation into the
shell=Truecommand in bothcollect_container_aiu_metricsmethods. Both methods callshlex.joinfor the podman arguments, then embed the result in a larger shell string whereoutput_file,timeout, anduserare interpolated raw. A path or user name with a space or a metacharacter breaks the command or executes extra shell code.
avocado/utils/podman.py#L846-L857: applyshlex.quotetooutput_fileand touser, and casttimeouttoint.avocado/utils/podman.py#L1576-L1590: apply the sameshlex.quotetreatment tooutput_fileanduser, and casttimeouttoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 846 - 857, In both collect_container_aiu_metrics methods, update command construction to shell-quote output_file and user before interpolation and cast timeout to int. Apply these changes at avocado/utils/podman.py lines 846-857 and 1576-1590, preserving the existing timeout, tee, and privilege-switching behavior.Source: Linters/SAST tools
avocado/utils/podman.py-1223-1237 (1)
1223-1237: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe
password_stdinbranch is unreachable whenusernameandpasswordare both set.If
username,password, andpassword_stdin=Trueare all supplied, theelif username and passwordbranch at Line 1226 runs. It appends--usernamebut never appends--password-stdin.podman loginthen prompts for a password on a non-interactive stdin.Test
password_stdinbefore the username and password combination.🐛 Proposed fix
if api_key: args.extend(["--username", username or api_key_username]) args.extend(["--password", api_key]) + elif password_stdin: + if username: + args.extend(["--username", username]) + args.append("--password-stdin") elif username and password: args.extend(["--username", username]) - if not password_stdin: - args.extend(["--password", password]) - elif password_stdin: - if username: - args.extend(["--username", username]) - args.append("--password-stdin") + args.extend(["--password", password])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 1223 - 1237, Update the authentication branch ordering in the surrounding argument-construction logic so the password_stdin case is evaluated before the username-and-password case. When password_stdin is true, append --password-stdin and include username when provided, even if password is also set; preserve the existing api_key handling and fallback validation.avocado/utils/podman.py-504-581 (1)
504-581: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThe SHA256 validation is unsound.
Line 564 tests
actual_sha[:16] in sha_content. This performs a substring search of a 16-character prefix over the whole checksum file. Two problems follow:
- A truncated 64-bit prefix is not a valid integrity check.
- The prefix is not bound to the file it belongs to. The prefix of file A matches an entry for file B, so a corrupted file passes.
Also, Line 556 tests
filename in sha_content, which matches any substring, and theexcept Exceptionat Line 572 records the failure but does not setis_valid = False.Parse
SHA256SUMSinto a{filename: digest}mapping and compare the full digest.🔒 Proposed change
- with open(sha_file, "r", encoding="utf-8") as f: - sha_content = f.read() - for weight_file in weight_files: - filename = os.path.basename(weight_file) - if filename in sha_content: + expected = {} + with open(sha_file, "r", encoding="utf-8") as f: + for line in f: + parts = line.split() + if len(parts) == 2: + expected[os.path.basename(parts[1].lstrip("*"))] = parts[0] + for weight_file in weight_files: + filename = os.path.basename(weight_file) + if filename in expected: validation_messages.append(f"Validating SHA256 for: {filename}") sha256_hash = hashlib.sha256() try: with open(weight_file, "rb") as f: - for byte_block in iter(lambda: f.read(4096), b""): + for byte_block in iter(lambda: f.read(1024 * 1024), b""): sha256_hash.update(byte_block) actual_sha = sha256_hash.hexdigest() - if actual_sha[:16] in sha_content: + if actual_sha == expected[filename]: validation_messages.append(f"SHA256 VALID: {filename}") else: validation_messages.append(f"SHA256 MISMATCH: {filename}") - validation_messages.append( - f" Calculated: {actual_sha[:16]}..." - ) is_valid = False except Exception as sha_ex: validation_messages.append( f"SHA256 calculation failed for {filename}: {sha_ex}" ) + is_valid = False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 504 - 581, Update validate_model_with_sha to parse SHA256SUMS into a filename-to-full-digest mapping, then validate each weight_file only against the digest associated with its exact basename; compare the complete calculated SHA256 value rather than a truncated prefix or unrestricted substring. Mark is_valid = False when a checksum is missing, mismatched, or cannot be calculated/read, while preserving the existing validation messages.avocado/utils/podman.py-40-83 (1)
40-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle failures in
setup_user_and_groupand avoid a literal "None" password.All
subprocess.runcalls usecheck=Falseand the return codes are ignored. The caller cannot detect that user creation or group modification failed. Later container operations then fail with unrelated errors.If
passwordisNone,chpasswdreceives the literal stringusername:None. Validatepasswordbefore you callchpasswd.🛠️ Proposed change
if user_check.returncode != 0: log.info("Create user: %s", username) - subprocess.run(["useradd", "-m", username], check=False) - # Use input parameter to pass password securely to chpasswd - subprocess.run( - ["chpasswd"], input=f"{username}:{password}\n".encode(), check=False - ) + result = subprocess.run(["useradd", "-m", username], check=False) + if result.returncode != 0: + raise PodmanException(f"Failed to create user {username}") + if password is not None: + # Use input parameter to pass password securely to chpasswd + result = subprocess.run( + ["chpasswd"], input=f"{username}:{password}\n".encode(), check=False + ) + if result.returncode != 0: + raise PodmanException(f"Failed to set password for {username}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 40 - 83, Update setup_user_and_group to validate that password is not None before invoking chpasswd, and replace ignored check=False subprocess calls with failure propagation so user creation, password setup, and group membership operations raise or otherwise report errors to the caller. Preserve the existing root and add/remove branching behavior while ensuring no command receives a literal "None" password.avocado/utils/podman.py-954-997 (1)
954-997: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the UTC clock change, but drop the
stderrsample-discard rule.
datetime.datetime.utcnow()is deprecated in Python 3.12, so the replacement with timezone-aware UTC is appropriate. Skipping collection samples whenstderris non-empty drops validpodman stats --no-stream --format=jsonoutput, because diagnostic messages can go to stderr while valid JSON is still returned on stdout. Keep logging the stderr warnings, but validate samples againststdout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 954 - 997, The stats collection loop should continue parsing valid stdout even when stderr is non-empty. In the stats sampling flow, retain the stderr warning log, but remove the branch that sleeps and skips the sample; validate and process the decoded stdout JSON as before, while preserving the timezone-aware UTC timestamp change.avocado/utils/nvme.py-361-383 (1)
361-383: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not fabricate supported LBA formats when discovery fails.
get_supported_lba_formats()marks indices 0 and 1 as valid formats when the namespace query fails, but NVMe LBA format indices are only meaningful from the controller-supportedLBAFlist. An unsupportedcreate_one_ns(..., flbas=-1)can then select the fabricated entry and issuenvme create-nswith an invalid FLBAS value. Return no formats, or raiseNvmeException, when no namespace provides queryable LBA format data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/nvme.py` around lines 361 - 383, Update get_supported_lba_formats() to remove the fallback that fabricates valid formats at indices 0 and 1 when namespace discovery or querying fails. Return an empty format list, or raise NvmeException, so create_one_ns cannot select an unsupported FLBAS value from fabricated data.avocado/utils/nvme.py-298-306 (1)
298-306: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the valid
nvme attach-nscontroller option.
nvme attach-nsaccepts--controllers=...or short option-c, not-controllers=.... Replace this invalid shorthand with--controllers=so the namespace attach command is accepted and parsed as intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/nvme.py` around lines 298 - 306, Update the attach command construction in the namespace-attachment flow to use the valid `--controllers=` option instead of `-controllers=`. Preserve the existing controller identifier and command execution behavior.avocado/core/utils/entry_points.py-64-65 (1)
64-65: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDeduplicate entry points from all selection paths.
get_entry_points_for()documents deduplicated results, but lines 64-65 return_entry_points(group=group)raw. Apply the(ep.name, ep.value)deduplication after collecting entry points from both the dict branch and thegroup=branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/core/utils/entry_points.py` around lines 64 - 65, Update get_entry_points_for so entry points returned from both the dict-based selection path and the _entry_points(group=group) path are deduplicated by the (ep.name, ep.value) pair before returning. Preserve the documented result ordering and existing selection behavior.avocado/core/nrunner/runnable.py-271-272 (1)
271-272: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve schema validation for package resources outside the filesystem.
resource_files("avocado").joinpath(...)returns aTraversableresource, not always a native path. Converting it withstr()and checkingos.path.exists()can fail for archive-loaded packages; then the fallback checks only/usr/share/avocado/schemas, so JSON Schema validation can be skipped. Open the package resource through theTraversableAPI, and keep native paths for the system-wide schema fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/core/nrunner/runnable.py` around lines 271 - 272, Update the schema resolution in the runnable schema-validation flow to retain the Traversable returned by resource_files("avocado").joinpath(...) and open package resources through its Traversable API instead of converting to str() or relying on os.path.exists(). Preserve native filesystem paths for the /usr/share/avocado/schemas fallback so validation remains enabled for both packaged and system-wide schemas..github/workflows/autils_migration_announcement.yml-31-31 (1)
31-31: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReplace the archived GitHub App token action across all workflows.
tibdex/github-app-tokenis archived and read-only, so it cannot receive security fixes. Migrate the token steps toactions/create-github-app-tokenand map the inputs: replaceapp_id,installation_id, andprivate_keywith the supported inputs, then limit token permissions as needed.Locations to update:
.github/workflows/pr_announcement.yml.github/workflows/project.yml.github/workflows/autils_migration_announcement.yml.github/workflows/release.yml(both token-generator steps)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/autils_migration_announcement.yml at line 31, Replace the archived tibdex/github-app-token action with actions/create-github-app-token in .github/workflows/autils_migration_announcement.yml:31, .github/workflows/pr_announcement.yml:15, and .github/workflows/project.yml:20, and update each token step’s app_id, installation_id, and private_key inputs to the supported names while restricting permissions as needed. Apply the same migration to both token-generator steps in .github/workflows/release.yml.Source: Linters/SAST tools
.github/workflows/setup.yml-168-169 (1)
168-169: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
devel-wide-system-installationsystem-wide.This job name and step name say “system wide”, but
python3 setup.py develop --userinstalls into the user site. The uninstall step usespython3 setup.py develop --uninstallwithout--user, so it does not reliably remove the user installation either. Keep installation and uninstallation scoped together. Use system-wide installation withsudoif this job should test system-wide commands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/setup.yml around lines 168 - 169, Update the installation and corresponding uninstall commands in the devel-wide-system-installation job to use the same system-wide scope: remove --user from setup.py develop and its uninstall invocation, and run the installation with sudo as needed for system-wide access. Keep the setuptools upgrade and existing job flow unchanged..github/workflows/release.yml-45-49 (1)
45-49: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReplace the archived GitHub App token action.
tibdex/github-app-tokenis archived and no longer maintained. The release workflow passes a private key to it and uses the generated token forreleasepushes andbuild-and-publish-eggsrelease uploads. Replace the occurrences in.github/workflows/release.ymlwith a maintained action, pin the replacement to an immutable commit, preserve theinstallation-idscope, and grant the generated token only the permissions needed for the following steps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 45 - 49, Replace every tibdex/github-app-token usage in the release workflow with a maintained GitHub App token action pinned to an immutable commit. Preserve the existing app ID, private key, and installation ID inputs, using the replacement’s installation-id input, and configure the generated token with only the permissions required by the release push and build-and-publish-eggs upload steps.Source: Linters/SAST tools
🟡 Minor comments (11)
docs/source/guides/contributor/chapters/how.rst-223-228 (1)
223-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInitialize the submodule before the first
static-checkscommand.The pre-commit setup uses
static-checks/requirements.txtat Line 209. The new initialization commands appear at Lines 226-227. An existing clone can therefore fail before reaching these recovery instructions. Move the submodule commands before the firststatic-checkscommand.Line 223 also calls
static-checksa symlink. Describe it as a Git submodule instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/source/guides/contributor/chapters/how.rst` around lines 223 - 228, Update the contributor setup instructions around the first static-checks command to initialize and update the Git submodule beforehand, including the existing recursive sync and update commands. Replace the description that calls the path a symlink with wording identifying it as a Git submodule, while preserving the requirements command afterward.docs/source/quickstart/index.rst-238-249 (1)
238-249: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the setuptools quickstart warning wording and ordering.
The
pkg_resourcesissue comes fromsetuptools82+, not Python 3.11+, so the warning title should not sayPython 3.11+. Move the warning before the defaultpip3 install --user avocado-frameworkcommand, or make it clear this only applies to builds/documentation installers that also installrequirements-doc.txt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/source/quickstart/index.rst` around lines 238 - 249, The quickstart warning currently attributes the pkg_resources problem to Python 3.11+ and appears after the default installation command. Update the warning title and text to identify setuptools 82+ as the trigger, and place it before the standard pip3 install command so affected users see the workaround first; otherwise explicitly scope it to documentation/build installations that install requirements-doc.txt.avocado/utils/software_manager/distro_packages.py-91-98 (1)
91-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQuote the custom tool path when calling
process.run().
process.run()receives a string andSubProcesssplits it withshlex.split()whenshell=False, so a valid path containing whitespace can be parsed as the executable plus arguments, causing a failed version check or command injection if shell metacharacters are present. Pass the command as a shell array, or quote the path before interpolating it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/software_manager/distro_packages.py` around lines 91 - 98, Update the custom_path branch in the distro package tool lookup to pass the executable path safely to process.run(), preserving paths containing whitespace and shell metacharacters. Prefer the supported shell-argument array form, or quote custom_path before interpolation, while keeping the existing version validation and return behavior unchanged.avocado/utils/sysinfo.py-417-423 (1)
417-423: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPut
AVOCADO_SYSINFO_CONFlast in the load order.
ConfigParser.read()applies later files over earlier files. The current list makes~/.config/avocado/sysinfo.confoverride the package default,/etc/avocado/sysinfo.conf, andAVOCADO_SYSINFO_CONF. Moveos.environ.get("AVOCADO_SYSINFO_CONF")to the end if it should be the highest-priority override.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/sysinfo.py` around lines 417 - 423, Update the candidates list used by the sysinfo configuration loader so the AVOCADO_SYSINFO_CONF entry comes last, after the package, system, and user configuration paths; preserve the existing filtering and ConfigParser.read flow so the environment-provided configuration has highest priority.avocado/utils/podman.py-876-923 (1)
876-923: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
collect_container_statsblocks the caller for the wholeduration.The method loops until
durationseconds elapse and callstime.sleep(interval). Forcontainer_id="all", the loop is repeated for each container in sequence, so the total wall time isduration * number_of_containers, notduration. The docstring does not state this.Either collect the containers concurrently, or document the sequential behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 876 - 923, Update collect_container_stats to document that container_id="all" processes containers sequentially, causing total collection time to scale with duration multiplied by the number of containers; clarify this behavior in the docstring without changing the implementation.avocado/utils/podman.py-754-805 (1)
754-805: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSplitting a string command with
str.split()breaks quoted arguments.Line 795 and Line 1521 use
command.split(). An argument that contains spaces, for exampleecho "hello world", is split into separate tokens and the quotes are passed literally. Useshlex.splitfor shell-like tokenization.🐛 Proposed fix (apply at both sites)
if isinstance(command, str): - cmd_args.extend(command.split()) + cmd_args.extend(shlex.split(command))Also applies to: 1476-1531
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 754 - 805, Replace command.split() with shlex.split() in both exec_command and the other command-building path around the second occurrence, adding the required shlex import. Preserve list commands unchanged so quoted string arguments are tokenized correctly without literal quote characters.avocado/utils/podman.py-24-35 (1)
24-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd tests for the new high-risk public API functions.
PodmanandAsyncPodmanalready have some functional tests, but the new module-level functions —setup_user_and_group,get_container_port,save_container_logs,wait_for_vllm_startup,install_huggingface_cli,download_model_from_hf, andvalidate_model_with_sha— have no test coverage. Cover the shell-quoting paths, checksum parsing, anduserpropagation to prevent command-injection and validation regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 24 - 35, Add focused tests for the module-level functions setup_user_and_group, get_container_port, save_container_logs, wait_for_vllm_startup, install_huggingface_cli, download_model_from_hf, and validate_model_with_sha. Mock subprocess and filesystem/network boundaries to cover shell-quoting behavior, checksum parsing and validation, and propagation of the user argument through generated commands, including both success and relevant failure paths.avocado/utils/podman.py-1749-1853 (1)
1749-1853: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe asynchronous inference method wraps the timeout error twice.
Line 1848 raises
PodmanException("Inference request timed out after 300 seconds")inside the innerexcept asyncio.TimeoutError. The outerexcept Exceptionat Line 1850 catches thatPodmanExceptionand replaces the message withFailed to send inference request to {host}:{port}. The timeout cause is lost from the message, and the raise at Line 1848 also lacks afromclause.Re-raise
PodmanExceptionunchanged in the outer handler.🐛 Proposed fix
- except asyncio.TimeoutError: + except asyncio.TimeoutError as timeout_ex: proc.kill() await proc.wait() error_msg = "Inference request timed out after 300 seconds" LOG.error(error_msg) - raise PodmanException(error_msg) + raise PodmanException(error_msg) from timeout_ex + except PodmanException: + raise except Exception as ex:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 1749 - 1853, Update the outer exception handler in send_vllm_inference_request to re-raise existing PodmanException instances unchanged, while preserving the current wrapping and context for other exceptions. This must retain the timeout message raised by the inner asyncio.TimeoutError handler.Source: Linters/SAST tools
avocado/utils/dmesg.py-214-216 (1)
214-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch complete kernel command-line parameters.
Line 216 matches substrings.
foo=onalso matchesfoo=only. An emptyparamalso matches every command line. Split the command line into parameters and compare exact values. Add a test for prefix collisions.Proposed fix
- return param in cmdline + return param in cmdline.split()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/dmesg.py` around lines 214 - 216, Update the /proc/cmdline check to split the command line into individual parameters and test exact parameter membership, ensuring empty inputs and prefix collisions such as foo=on versus foo=only do not match. Add a focused test covering the prefix-collision case..github/workflows/prerelease.yml-20-23 (1)
20-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the Python version in the step name.
The step installs Python 3.14. Its name still states Python 3.9. Update the name so CI logs match the configured runtime.
Proposed fix
- - name: Set up Python 3.9 + - name: Set up Python 3.14🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/prerelease.yml around lines 20 - 23, Update the setup-python step name in the prerelease workflow to state Python 3.14, matching the python-version configured for actions/setup-python..github/workflows/ci.yml-284-289 (1)
284-289: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winHash the avocado-vt download configuration.
The checkout stores avocado-vt under
avocado-vt/, but the current glob starts at the workspace root withvirttest/.hashFiles()therefore matches no files and returns an empty hash. All configuration revisions use the same cache key.Proposed fix
- key: avocado-vt-images-${{ runner.os }}-${{ hashFiles('virttest/shared/downloads/*.ini') }} + key: avocado-vt-images-${{ runner.os }}-${{ hashFiles('avocado-vt/virttest/shared/downloads/*.ini') }}GitHub evaluates
hashFiles()relative toGITHUB_WORKSPACEand returns an empty string for no matches. (docs.github.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 284 - 289, Update the cache key expression in the actions/cache step to hash the avocado-vt download configuration under the checked-out avocado-vt/ directory, ensuring hashFiles matches the intended virttest/shared/downloads/*.ini files while preserving the existing runner-specific key and restore prefix.
🧹 Nitpick comments (7)
avocado/utils/sysinfo.py (1)
176-179: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid sudo checks for commands outside the allowlist.
Line 177 constructs
SysinfoCommandfor every collected command. Its constructor loads configuration and callscan_sudo()before Line 178 checksis_sudo_cmd(). This repeats sudo capability checks for every sysinfo command, including commands that cannot use sudo.Load and cache the configuration once. Check the command allowlist before the sudo capability check.
Also applies to: 412-425
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/sysinfo.py` around lines 176 - 179, Update the sysinfo command execution flow around SysinfoCommand and the corresponding logic at the additional sudo-check location to load and cache configuration once rather than constructing/loading it for every command. Evaluate is_sudo_cmd(self.cmd) first, and only call use_sudo() for commands in the sudo allowlist; preserve the existing sudo_flag and logging behavior.avocado/utils/podman.py (5)
228-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit
return Nonein the finalexceptblock.
save_container_logsdocuments a return of the log path orNone. Theexcept Exceptionhandler at Line 231 has noreturn, so the function falls through and returnsNoneimplicitly. The behavior is correct, but the exit path is inconsistent with the other handlers.♻️ Proposed change
except Exception as ex: log.warning("Failed to save container logs: %s", ex) + return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 228 - 233, Update the final except Exception handler in save_container_logs to explicitly return None after logging the failure, matching the existing timeout handler and documented return contract.
511-512: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
globandhashlibimports to the module header.Both imports are unconditional inside the function body. The module already imports
json,os, andshlexat the top.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 511 - 512, Move the unconditional glob and hashlib imports from the function body to the module-level import section alongside json, os, and shlex, and remove the now-redundant local imports.
368-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffReconsider installing packages with
pip install -Ufrom a utility module.
install_huggingface_clirunspip install -U huggingface_hub[cli]in the active environment. This mutates the host or virtual environment as a side effect of a test utility. It can upgrade transitive dependencies of Avocado itself.If the function must stay, document the side effect and consider
--useror an explicit target directory.avocado.utils.software_manageralready provides an installation abstraction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 368 - 443, Update install_huggingface_cli to avoid directly mutating the active environment with pip install -U; reuse the installation abstraction from avocado.utils.software_manager instead. If direct installation must remain, document the side effect and constrain it to an explicit user or target directory while preserving the existing verification behavior.
98-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
self.podman_bininstead of the hardcodedpodmanstring.
_Podman.__init__resolves the binary throughwhich(podman_bin or "podman")and stores it inself.podman_bin. The module-level helpersget_container_port,save_container_logs, andwait_for_vllm_startuphardcode"podman". ThePodman.get_container_port,Podman.save_container_logs,AsyncPodman.get_container_port,AsyncPodman.save_container_logs, andAsyncPodman.wait_for_vllm_startupwrappers therefore ignore a custompodman_bin.Add a
podman_binparameter to the helpers and passself.podman_binfrom the wrappers.Also applies to: 1275-1284
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 98 - 128, Update the module-level helpers get_container_port, save_container_logs, and wait_for_vllm_startup to accept a podman_bin parameter and use it for every Podman subprocess invocation instead of hardcoding "podman". Pass self.podman_bin from the corresponding Podman and AsyncPodman wrapper methods, preserving existing behavior when no custom binary is provided.
273-365: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
wait_for_vllm_startupretrieves the full log on every iteration and tracks position by line count.Each check calls
podman logswithout--tail, so the whole log is fetched everycheck_intervalseconds. For a long-running container the transferred data grows without bound.
last_log_positioncounts lines of the previous full output. If the log is rotated or truncated, the index no longer matches and new lines are skipped.Consider
--sinceor--tailfor the incremental display.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avocado/utils/podman.py` around lines 273 - 365, The wait_for_vllm_startup log polling currently fetches the entire container log each iteration and uses an invalid line-count cursor after truncation or rotation. Update the podman logs invocations to retrieve only incremental output for live-log display, using an appropriate --since or --tail strategy, and adjust the tracking logic so newly emitted lines are not skipped when logs shrink or rotate. Preserve failure and success pattern checks against the available log content..github/workflows/setup.yml (1)
189-189: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMove workflow context values out of the shell source.
zizmorflags both commands. GitHub expands${{ ... }}before the shell parses the script. If a trigger supplies attacker-controlled text, shell metacharacters can execute commands. Pass the values throughenvand expand quoted variables instead. GitHub warns that context values can contain untrusted input. (docs.github.com)Proposed safe pattern
- - run: echo "Job triggered by a ${{ github.event_name }} event on branch is ${{ github.ref }} in repository is ${{ github.repository }}, runner on ${{ runner.os }}" + - name: Log workflow context + env: + EVENT_NAME: ${{ github.event_name }} + REF: ${{ github.ref }} + REPOSITORY: ${{ github.repository }} + RUNNER_OS: ${{ runner.os }} + run: >- + printf 'Job triggered by a %s event on branch %s in repository %s, runner on %s\n' + "$EVENT_NAME" "$REF" "$REPOSITORY" "$RUNNER_OS"Also applies to: 223-223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/setup.yml at line 189, Update the workflow steps containing the job-trigger and corresponding second context log commands to pass each GitHub context value through the step’s env block, then reference the resulting environment variables with quoted shell expansions in run. Remove all direct `${{ ... }}` expressions from shell source while preserving the logged values and message.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46ca450f-628a-461b-9604-92e38b4d2afe
📒 Files selected for processing (63)
.github/dependabot.yml.github/workflows/ansible.yml.github/workflows/autils_migration_announcement.yml.github/workflows/ci.yml.github/workflows/pr_announcement.yml.github/workflows/prerelease.yml.github/workflows/project.yml.github/workflows/push_ci.yml.github/workflows/release.yml.github/workflows/setup.yml.github/workflows/vmimage.yml.github/workflows/weekly.yml.pylintrc_utilsavocado/core/__init__.pyavocado/core/extension_manager.pyavocado/core/nrunner/app.pyavocado/core/nrunner/runnable.pyavocado/core/output.pyavocado/core/status/server.pyavocado/core/suite.pyavocado/core/task/runtime.pyavocado/core/utils/eggenv.pyavocado/core/utils/entry_points.pyavocado/core/utils/path.pyavocado/core/version.pyavocado/etc/avocado/sysinfo.confavocado/plugins/exec_path.pyavocado/plugins/replay.pyavocado/plugins/resolvers.pyavocado/plugins/runner_nrunner.pyavocado/plugins/runners/exec_test.pyavocado/plugins/runners/package.pyavocado/plugins/runners/podman_image.pyavocado/utils/archive.pyavocado/utils/cpu.pyavocado/utils/disk.pyavocado/utils/dmesg.pyavocado/utils/nvme.pyavocado/utils/pci.pyavocado/utils/pmem.pyavocado/utils/podman.pyavocado/utils/software_manager/backends/zypper.pyavocado/utils/software_manager/distro_packages.pyavocado/utils/sysinfo.pydocs/source/guides/contributor/chapters/how.rstdocs/source/quickstart/index.rstman/avocado.rstoptional_plugins/html/avocado_result_html/__init__.pyoptional_plugins/resultsdb/setup.pyoptional_plugins/robot/tests/resolver.pyoptional_plugins/spawner_remote/avocado_spawner_remote/__init__.pyselftests/check.pyselftests/deployment/roles/avocado/tasks/rpm/examples.ymlselftests/deployment/roles/common/tasks/repos.ymlselftests/deployment/roles/tests/tasks/main.ymlselftests/pre_release/tests/check-copr-rpm-version.shselftests/unit/output.pyselftests/unit/safeloader_module.pyselftests/unit/test_replay_resume.pyselftests/unit/utils/cpu.pyselftests/unit/utils/pci.pyselftests/utils.pysetup.py
| if self.is_tar: | ||
| self._engine.extraction_filter = getattr( | ||
| tarfile, "fully_trusted_filter", (lambda member, path: member) | ||
| ) | ||
| self._engine.extractall(path) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)avocado/utils/archive\.py$|requirements|setup\.py|setup\.cfg|pyproject\.toml|tox\.ini|VERSION|README|rtd|README'
echo "== archive outline =="
if [ -f avocado/utils/archive.py ]; then
wc -l avocado/utils/archive.py
ast-grep outline avocado/utils/archive.py || true
fi
echo "== relevant archive lines =="
if [ -f avocado/utils/archive.py ]; then
sed -n '320,390p' avocado/utils/archive.py | cat -n -v
fi
echo "== python/tarfile filter availability and docs =="
python3 - <<'PY'
import tarfile, inspect, sys
print("python", sys.version)
for name in ["fully_trusted_filter", "data_filter", "default_filter"]:
obj = getattr(tarfile, name, None)
print(name, bool(obj), obj)
PY
echo "== python behavioral probe with crafted tar/ZIP archive names =="
python3 - <<'PY'
import tarfile, zipfile, tempfile, os, io, pathlib
def run_extractall(name, create_archive, extract_filter=None, use_tar_filter=True):
target = tempfile.mkdtemp(prefix="avocado_archive_probe_")
path = os.path.join(target, "sandbox")
os.makedirs(path)
archive = create_archive()
try:
with tarfile.open(name=name, fileobj=archive, mode=archive.mode if hasattr(archive, "mode") else None) as tar:
if use_tar_filter and extract_filter is not None:
tar.extractall(path=path, filter=extract_filter)
elif tarfile.extractall is zipfile:
# unreachable for tarfile fixture because opened through tarfile.open
pass
else:
tar.extractall(path=path)
entries = set()
for root, dirs, files in os.walk(path):
for d in dirs:
entries.add(os.path.join(root, d))
for f in files:
entries.add(os.path.join(root, f))
print(name, "ok", sorted(entries), "outside_sandbox", any(not str(x).startswith(path) for x in entries))
except Exception as e:
print(name, "error", repr(e))
print(name, "outside_sandbox after exception", False)
finally:
import shutil
shutil.rmtree(target, ignore_errors=True)
members = [
("bad-normal", tarfile.TarInfo("../../../tmp/evil.txt"), b"evil"),
("bad-link", tarfile.TarInfo("bad-link"), b"", type=tarfile.SYMTYPE, linkname="../../etc/passwd"),
]
for name, info, data in members:
buf = io.BytesIO()
with tarfile.open(fileobj=buf, mode="w") as tar:
tar.addfile(info.replace=name[5:], fileobj=io.BytesIO(data))
archive = io.BytesIO(buf.getvalue())
archive.mode = "r"
for f in [None, tarfile.data_filter, tarfile.fully_trusted_filter]:
run_extractall(f"{name}_filter={getattr(f, '__name__', str(f))}", lambda a=archive: a, f)
zip_members = ["evil.txt", "../../../tmp/e.txt"]
for zname in zip_members:
buf = io.BytesIO()
with zipfile.ZipFile(buf, "w") as z:
z.writestr(zipfile.ZipInfo(zname + "/"), "")
z.writestr(zipfile.ZipInfo(zname + "/out.txt"), "evil")
archive = io.BytesIO(buf.getvalue())
print("zip", zname, "unsupported_filter_attempt", list(zipfile.ZipFile(archive, "r").extractall.__kwdefaults__.keys()) if hasattr(zipfile.ZipFile(archive, "r").extractall, "__kwdefaults__") else "{}")
target = tempfile.mkdtemp(prefix="avocado_zip_probe_")
sandbox = os.path.join(target, "sandbox")
os.makedirs(sandbox)
with zipfile.ZipFile(archive, "r") as z:
z.extractall(path=sandbox)
entries = []
for root, dirs, files in os.walk(sandbox):
for e in dirs + files:
entries.append(os.path.join(root, e))
outside = [e for e in entries if not os.path.dirname(e).startswith(sandbox) or e.startswith(os.path.join(target, "tmp"))]
print("zip", zname, "outdir", os.listdir(sandbox), "outside", outside)
os.unlink(os.path.join(target, "tmp", "e.txt"))
os.rmdir(os.path.join(target, "tmp"))
os.rmdir(target)
PYRepository: avocado-framework/avocado
Length of output: 7743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== usages of archive/archive_extractor classes =="
rg -n "class .*Archive|Archive|_engine|\.extract|extractall|fully_trusted_filter|data_filter" avocado/utils/archive.py avocado -g '*.py' || true
echo "== dependency/version constraints around Python =="
for f in pyproject.toml setup.cfg setup.py tox.ini .python-version .tool-versions requirements.txt requirements/*.txt docs/conf.py README.md README.rst README.txt; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
done
echo "== deterministic path handling based on source =="
python3 - <<'PY'
from pathlib import Path
import ast, tarfile, zipfile
src = Path("avocado/utils/archive.py")
text = src.read_text()
tree = ast.parse(text)
print("contains fully_trusted_filter fallback:", "fully_trusted_filter" in text)
print("contains data_filter:", "data_filter" in text)
print("contains extractall call:", "extractall" in text)
PYRepository: avocado-framework/avocado
Length of output: 22452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)avocado/utils/archive\.py$|requirements|setup\.py|setup\.cfg|pyproject\.toml|tox\.ini|VERSION|README|rtd|README'
echo "== archive outline =="
if [ -f avocado/utils/archive.py ]; then
wc -l avocado/utils/archive.py
ast-grep outline avocado/utils/archive.py || true
fi
echo "== relevant archive lines =="
if [ -f avocado/utils/archive.py ]; then
sed -n '320,390p' avocado/utils/archive.py | cat -n -v
fi
echo "== usages of archive/archive_extractor classes =="
rg -n "class .*Archive|Archive|_engine|\.extract|extractall|fully_trusted_filter|data_filter" avocado/utils/archive.py avocado -g '*.py' || true
echo "== dependency/version constraints around Python =="
for f in pyproject.toml setup.cfg setup.py tox.ini .python-version .tool-frames requirements.txt requirements/*.txt docs/conf.py README.md README.rst README.txt; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
done
echo "== python/tarfile filter availability and docs =="
python3 - <<'PY'
import tarfile, sys
print("python", sys.version)
for name in ["fully_trusted_filter", "data_filter", "default_filter"]:
obj = getattr(tarfile, name, None)
print(name, bool(obj), obj and obj.__name__)
PY
echo "== deterministic path handling based on source =="
python3 - <<'PY'
from pathlib import Path
src = Path("avocado/utils/archive.py")
text = src.read_text()
print("contains fully_trusted_filter fallback:", "fully_trusted_filter" in text)
print("contains data_filter:", "data_filter" in text)
print("contains extractall call:", "extractall" in text)
PY
echo "== behavioral probe with crafted tar/ZIP archive names =="
python3 - <<'PY'
import tarfile, zipfile, tempfile, os, io
def run_tar_extract(name, archive, extract_filter):
target = tempfile.mkdtemp(prefix="avocado_archive_probe_")
path = os.path.join(target, "sandbox")
os.makedirs(path)
archive.seek(0, io.SEEK_SET)
try:
with tarfile.open(fileobj=archive, mode="r") as tar:
if extract_filter is not None:
tar.extractall(path=path, filter=extract_filter)
else:
tar.extractall(path=path)
entries = set()
for root, dirs, files in os.walk(path):
for d in dirs:
entries.add(os.path.join(root, d))
for f in files:
entries.add(os.path.join(root, f))
print("tar", name, "fs_entries", sorted(entries), "outside_sandbox", [e for e in entries if not os.path.abspath(e).startswith(os.path.abspath(path))])
except Exception as e:
print("tar", name, "error", repr(e).splitlines()[0], "outside_sandbox", False)
finally:
import shutil
shutil.rmtree(target, ignore_errors=True)
members = [
("bad-normal", tarfile.TarInfo("../../../tmp/evil.txt"), b"evil"),
("bad-link", tarfile.TarInfo("bad-link"), b"", type=tarfile.SYMTYPE, linkname="../../etc/passwd"),
]
for name, info, data in members:
buf = io.BytesIO()
with tarfile.open(fileobj=buf, mode="w") as tar:
info.name = name
tar.addfile(info, fileobj=io.BytesIO(data))
archive = io.BytesIO(buf.getvalue())
for f in [None, getattr(tarfile, "data_filter", None), getattr(tarfile, "fully_trusted_filter", None)]:
run_tar_extract(f"{name}_filter={getattr(f, '__name__', 'none')}", archive, f)
target = tempfile.mkdtemp(prefix="avocado_zip_probe_")
sandbox = os.path.join(target, "sandbox")
os.makedirs(sandbox)
for zname in ["../../../tmp/e.txt", "../../../etc/e.txt"]:
buf = io.BytesIO()
with zipfile.ZipFile(buf, "w") as z:
z.writestr(zipfile.ZipInfo(zname), "evil")
with zipfile.ZipFile(buf, "r") as z:
z.extractall(path=sandbox)
entries = []
for root, dirs, files in os.walk(sandbox):
for e in dirs + files:
entries.append(os.path.join(root, e))
outside = [e for e in entries if not os.path.abspath(e).startswith(os.path.abspath(sandbox))]
print("zip", zname, "files_in_sandbox_root", os.listdir(sandbox), "outside_sandbox", outside)
os.rmdir(sandbox)
os.rmdir(target)
PYRepository: avocado-framework/avocado
Length of output: 29967
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AVOCADO_VERSION =="
cat -n VERSION
echo "== python package metadata =="
for f in setup.cfg pyproject.toml tox.ini docs/source/conf.py; do
[ -f "$f" ] && { echo "--- $f"; rg -n "python_requires|Programming Language :: Python|python_version|python\s+3|requires-python|trove" "$f" -C 2 || true; }
done
echo "== archive imports and extraction helper =="
sed -n '1,80p' avocado/utils/archive.py | cat -n
sed -n '360,390p' avocado/utils/archive.py | cat -n
sed -n '378,450p' avocado/utils/archive.py | cat -n
echo "== Python 3.9 tarfile filter and crafted member behavior =="
python3 - <<'PY'
import tarfile, zipfile, tempfile, os, io, pathlib, shutil, importlib, pkgutil
def probe_tar(name, buf, engine_filter=None):
target = tempfile.mkdtemp(prefix="avocado_tar_probe_")
sandbox = os.path.join(target, "sandbox")
os.makedirs(sandbox)
try:
with tarfile.open(fileobj=buf, mode="r") as tar:
if engine_filter is not None:
# Mirrors the current ArchiveFile.extract implementation at line 366-369.
tar.extraction_filter = engine_filter
tar.extractall(path=sandbox)
outside = []
for root, dirs, files in os.walk(sandbox):
for e in dirs + files:
path = os.path.join(root, e)
if not os.path.abspath(path).startswith(os.path.abspath(sandbox)):
outside.append((path, os.path.islink(path), os.readlink(path) if os.path.islink(path) else None))
print("tar", name, "engine_filter", getattr(engine_filter, "__name__", str(engine_filter)), len(outside), outside)
except Exception as e:
print("tar", name, "engine_filter", getattr(engine_filter, "__name__", str(engine_filter)), "ERROR", repr(e).splitlines()[0])
finally:
shutil.rmtree(target, ignore_errors=True)
tmp = pathlib.Path("/tmp")
if not tmp.exists():
tmp.mkdir(mode=0o755, parents=True, exist_ok=True)
members = [
("bad-normal", tarfile.TarInfo("../../../tmp/avocado_e.txt"), b"evil"),
("bad-link", tarfile.TarInfo("bad-link"), b"", type=tarfile.SYMTYPE, linkname="../tmp/avocado_e.txt"),
("absolute", tarfile.TarInfo("/tmp/avocado_e.txt"), b"evil"),
("zero-size-link", tarfile.TarInfo("link"), b"", type=tarfile.SYMTYPE, linkname="../tmp/avocado_e.txt"),
]
for name, info, data in members:
buf = io.BytesIO(io.BytesIO()).getvalue() # empty buffer
buf = io.BytesIO()
with tarfile.open(fileobj=buf, mode="w") as tar:
tar.addfile(info, fileobj=io.BytesIO(data))
for filter_value in [None, tarfile.data_filter, tarfile.fully_trusted_filter]:
probe_tar(name, io.BytesIO(buf.getvalue()), filter_value)
probe_tar("tricky-continued-data-filter", io.BytesIO(tarfile.TarInfo("a").tarfile), tarfile.data_filter)
print("zip extractall kwdefaults", zipfile.ZipFile(io.BytesIO(), "r").extractall.__kwdefaults__ if hasattr(zipfile.ZipFile(io.BytesIO(), "r").extractall, "__kwdefaults__") else None)
for zname in ["../../../tmp/avocado_z.txt", "/tmp/avocado_z.txt"]:
target = tempfile.mkdtemp(prefix="avocado_zip_probe_")
sandbox = os.path.join(target, "sandbox2")
os.makedirs(sandbox)
buf = io.BytesIO()
with zipfile.ZipFile(buf, "w") as z:
z.writestr(zipfile.ZipInfo(zname), "evil")
with zipfile.ZipFile(buf, "r") as z:
z.extractall(path=sandbox)
outside = []
for root, dirs, files in os.walk(sandbox):
for e in dirs + files:
path = os.path.join(root, e)
if not os.path.abspath(path).startswith(os.path.abspath(sandbox)):
outside.append(path)
print("zip", zname, "outside", outside)
shutil.rmtree(target, ignore_errors=True)
# Remove test files used by crafted samples if present.
for p in [tmp / "avocado_e.txt", tmp / "avocado_z.txt"]:
if p.exists():
p.unlink(missing_ok=True)
PYRepository: avocado-framework/avocado
Length of output: 8615
🌐 Web query:
Python tarfile.extractall fully_trusted_filter data_filter CVE-2007-6063
💡 Result:
The vulnerability CVE-2007-4559 refers to a long-standing directory traversal flaw in the Python tarfile module's extract and extractall methods [1][2]. Because these methods historically trusted archive metadata, a maliciously crafted tar file could use ".." path components to overwrite arbitrary files outside the intended destination directory [2][3]. To address this, Python introduced the filter mechanism via PEP 706 [4][5]. This allows developers to restrict which files and paths are extracted [6][7]. Key Filters and Mechanisms: 1. 'data' filter (tarfile.data_filter): This is the recommended secure default [6][7]. It prevents directory traversal by blocking paths that escape the destination, refuses to extract dangerous file types (e.g., device files, pipes), and restricts unsafe symbolic/hard links [6][8][7]. 2. 'fully_trusted' filter (tarfile.fully_trusted_filter): This maintains the original, insecure behavior [4][9]. It honors all archive metadata without restriction and should only be used if the archive source is entirely trusted [4][9]. 3. 'tar' filter (tarfile.tar_filter): A middle ground that blocks directory traversal but remains more permissive than the 'data' filter regarding certain UNIX-specific features [10][6]. Implementation: Upgrading your Python interpreter is necessary but not sufficient, as the 'filter' argument must be explicitly used in your code [3]. You should use the 'data' filter as follows: tar.extractall(path, filter='data') To ensure protection process-wide—including for third-party libraries—you can set the filter globally [3]: tarfile.TarFile.extraction_filter = staticmethod(tarfile.data_filter) Compatibility Note: These features were introduced in Python 3.12 and backported to several older versions (e.g., 3.8.17+, 3.9.17+, 3.10.12+, 3.11.4+) [3]. You can check for availability in your environment using hasattr(tarfile, 'data_filter') [6][11]. In Python 3.14 and later, the 'data' filter will become the default [4][5].
Citations:
- 1: https://nvd.nist.gov/vuln/detail/cve-2007-4559
- 2: https://ubuntu.com/security/CVE-2007-4559
- 3: https://safeguard.sh/resources/blog/python-tarfile-extraction-path-traversal-the-15-year-old-flaw-cve-2007-4559
- 4: https://peps.python.org/pep-0706/
- 5: https://discuss.python.org/t/pep-706-filter-for-tarfile-extractall/23903
- 6: https://docs.python.org/3/library/tarfile.html
- 7: https://runebook.dev/en/docs/python/library/tarfile/tarfile.data_filter
- 8: https://github.com/python/cpython/blob/main/Doc/library/tarfile.rst
- 9: https://discuss.python.org/t/policies-for-tarfile-extractall-a-k-a-fixing-cve-2007-4559/23149
- 10: https://github.com/python/cpython/blob/bb911a2319365a4155e7398b4b7978589d8bed49/Lib/tarfile.py
- 11: https://docs.python.org/release/3.11.5/library/tarfile.html
🌐 Web query:
Python zipfile.extractall path traversal before_filter zipfile.Path traversal
💡 Result:
Python's zipfile.extractall and extract methods include built-in mitigations against path traversal (often referred to as Zip Slip) [1][2][3]. These methods sanitize file names by removing leading slashes, drive letters, and parent directory references ('..') before creating files on the filesystem [4][3]. Despite these protections, the official Python documentation maintains a warning advising against extracting archives from untrusted sources [5][6]. This is due to a few key factors: 1. Potential for Bypass or Edge Cases: While the internal sanitization logic covers many common scenarios, it may not be exhaustive against all platform-specific nuances or future bypass techniques [1][7]. 2. Zip Bombs: The module does not inherently protect against denial-of-service attacks known as "zip bombs," which can exhaust system resources (disk space, memory, or inodes) during extraction [7]. 3. Complexity of Security: The Python development team considers it best practice to keep this warning to ensure users remain cautious when handling untrusted input, even if the implementation is significantly safer than it was in older versions (e.g., prior to the 2013/Python 2.7.4 era) [8][1][7]. The zipfile.Path class, which provides an alternative way to access archive contents, does not automatically sanitize file names [5][6]. If you are using zipfile.Path to iterate and manually extract files, it is your responsibility to validate paths to prevent traversal [5][6]. Best practices for handling untrusted ZIP files: - Use zipfile.extractall or zipfile.extract for general use, as they include the standard sanitization [2][3]. - For high-security environments, manually validate that the resolved output path starts with the intended base directory using os.path.abspath and os.path.commonpath [5][9]. - Be aware of the risks beyond path traversal, such as zip bombs [7]. - Monitor for relevant CVEs in third-party libraries (e.g., python-libarchive), as vulnerabilities often arise in external wrappers rather than the core zipfile module [10][11][12].
Citations:
- 1: zipfile.extractall is safe by now python/cpython#84940
- 2: https://github.com/doyensec/Unsafe-Unpacking/blob/main/Python/README.md
- 3: https://github.com/python/cpython/blob/61892c04764e1f3a659bbd09e6373687a27d36e2/Lib/zipfile.py
- 4: https://hg.python.org/cpython/rev/249e0b47b686
- 5: https://docs.python.org/3/library/zipfile.html
- 6: https://github.com/python/cpython/blob/main/Doc/library/zipfile.rst
- 7: zipfile.extractall security warning in the docs, can it be removed yet? python/cpython#102686
- 8: https://bugs.python.org/issue6972
- 9: https://discuss.python.org/t/safe-archive-unpacking-and-path-resolution/30476
- 10: https://nvd.nist.gov/vuln/detail/CVE-2024-55587
- 11: GHSA-m5gr-86j6-99jp
- 12: Directory Traversal Vulnerability (CVE-2024-55587) smartfile/python-libarchive#42
Do not set the tar extraction filter to fully trusted.
ArchiveFile.extract() intentionally selects tarfile.fully_trusted_filter, which lets crafted tar members with .. paths, absolute names, or unsafe symlinks write outside path before extractall() runs. Only use this for trusted input. Prefer tarfile.data_filter and add a compatibility fallback that rejects traversal names and links that escape the destination.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 368-368: Calling extractall() on a zipfile.ZipFile or tarfile archive without validating member paths lets a crafted entry (e.g. "../../etc/passwd") write outside the destination directory (Zip Slip). Validate each member resolves inside the target directory, or pass a safe filter (tarfile: filter="data" / tarfile.data_filter).
Context: self._engine.extractall(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(archive-extractall-path-traversal-python)
🪛 Ruff (0.16.1)
[error] 369-369: Uses of tarfile.extractall()
(S202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avocado/utils/archive.py` around lines 365 - 369, The ArchiveFile.extract()
tar path must not select tarfile.fully_trusted_filter, since it permits members
to escape the destination. Use tarfile.data_filter when available, with a
compatibility fallback that rejects traversal, absolute paths, and symlinks or
links escaping path before calling self._engine.extractall(path).
Source: Linters/SAST tools
| # Determine retry strategy based on flbas parameter | ||
| # Only enable auto-detection fallback for default flbas=0 | ||
| should_retry_with_auto_detect = flbas == 0 | ||
|
|
||
| # Handle explicit auto-detection request (flbas=-1) | ||
| # This skips the FLBAS=0 attempt and goes straight to optimal detection | ||
| if flbas == -1: | ||
| try: | ||
| flbas = get_optimal_flbas(controller_name) | ||
| LOGGER.info( | ||
| f"Auto-detected FLBAS={flbas} for namespace creation on {controller_name}" | ||
| ) | ||
| except NvmeException as e: | ||
| LOGGER.error(f"Failed to auto-detect FLBAS: {e}") | ||
| raise NvmeException( | ||
| f"Cannot create namespace: FLBAS auto-detection failed: {e}" | ||
| ) | ||
|
|
||
| # Build and execute namespace creation command | ||
| cmd = f"nvme create-ns /dev/{controller_name} --nsze={ns_size} --ncap={ns_size} --flbas={flbas} --dps=0" | ||
| if shared_ns: | ||
| cmd = f"{cmd} -m 1" | ||
| if process.system(cmd, shell=True, ignore_status=True): | ||
| raise NvmeException(f"namespace create command failed {cmd}") | ||
|
|
||
| result = process.system(cmd, shell=True, ignore_status=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate nvme.py =="
fd -a 'nvme.py|nvme.*' . | sed 's#^\./##' | head -50
echo "== inspect relevant avocado/utils/nvme.py section =="
wc -l avocado/utils/nvme.py
sed -n '460,550p' avocado/utils/nvme.py
echo "== locate create_namespace definitions/usages =="
rg -n "def create_namespace|create_namespace|flbas" avocado/utils/nvme.py avocado -g '*.py' | head -120
echo "== inspect call sites =="
python3 - <<'PY'
import ast, pathlib
p=pathlib.Path('avocado/utils/nvme.py')
tree=ast.parse(p.read_text())
for node in ast.walk(tree):
if isinstance(node, ast.Call):
if isinstance(node.func, ast.Attribute) and node.func.attr == 'create_namespace':
print(p, node.lineno, ast.get_source_segment(p.read_text(), node))
PY
echo "== behavioral probe: process.system shell=True interpolation =="
python3 - <<'PY'
import ast
PYRepository: avocado-framework/avocado
Length of output: 9226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect shell escaping behavior for candidate unvalidated inputs =="
python3 - <<'PY'
from shlex import join
inputs = ["0", "0; command", "0 --nsze=1", "0 && id", "-1", "abc", "16", "0; rm -rf /"]
cmds = [f"nvme create-ns /dev/controller --nsze={x} --ncap={x} --flbas={x} --dps=0" for x in inputs]
for x, cmd in zip(inputs, cmds):
print(f"input={x!r} command={cmd!r}")
PYRepository: avocado-framework/avocado
Length of output: 1082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect create_namespace wrapper =="
sed -n '630,665p' avocado/utils/nvme.py
echo "== inspect create-ns usage in avocado tests/usages =="
rg -n "create-one-ns|create_one_ns|create-namespace|create_namespace|--flbas=|flbas=\[" avocado -g '*.py' | head -200
echo "== process.system implementation =="
rg -n "def system|class .*Process|shell=True|system32|subprocess|check_output" avocado/utils process.py avocado/utils/process.py -g '*.py' | head -80
echo "== targeted AST for create-one-ns signature/body =="
python3 - <<'PY'
import ast
from pathlib import Path
src = Path('avocado/utils/nvme.py').read_text()
tree = ast.parse(src)
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name == 'create_one_ns':
print(node.lineno, node.argnames)
for stmt in node.body:
if isinstance(stmt, (ast.If, ast.Try, ast.Raise, ast.Assign, ast.Call, ast.Expr)):
start, end = stmt.lineno, getattr(stmt, 'end_lineno', stmt.lineno)
print(f"{start}-{end}: {ast.dump(stmt)[:200]}")
PY
echo "== Python shell interpolation demonstration =="
python3 - <<'PY'
flbas = "0; rm -rf /"
cmd = f"nvme create-ns /dev/controller --nsze=100 --ncap=100 --flbas={flbas} --dps=0"
print(cmd)
print(repr(cmd))
PYRepository: avocado-framework/avocado
Length of output: 9341
Validate flbas before building the shell command.
create_one_ns() accepts arbitrary flbas values and interpolates them directly into process.system(..., shell=True). A caller can pass a string containing shell syntax, so reject non-integers and integers outside -1..15 before command construction.
Proposed fix
+ if (
+ isinstance(flbas, bool)
+ or not isinstance(flbas, int)
+ or not -1 <= flbas <= 15
+ ):
+ raise NvmeException("flbas must be an integer from -1 through 15")
+
# Determine retry strategy based on flbas parameter📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Determine retry strategy based on flbas parameter | |
| # Only enable auto-detection fallback for default flbas=0 | |
| should_retry_with_auto_detect = flbas == 0 | |
| # Handle explicit auto-detection request (flbas=-1) | |
| # This skips the FLBAS=0 attempt and goes straight to optimal detection | |
| if flbas == -1: | |
| try: | |
| flbas = get_optimal_flbas(controller_name) | |
| LOGGER.info( | |
| f"Auto-detected FLBAS={flbas} for namespace creation on {controller_name}" | |
| ) | |
| except NvmeException as e: | |
| LOGGER.error(f"Failed to auto-detect FLBAS: {e}") | |
| raise NvmeException( | |
| f"Cannot create namespace: FLBAS auto-detection failed: {e}" | |
| ) | |
| # Build and execute namespace creation command | |
| cmd = f"nvme create-ns /dev/{controller_name} --nsze={ns_size} --ncap={ns_size} --flbas={flbas} --dps=0" | |
| if shared_ns: | |
| cmd = f"{cmd} -m 1" | |
| if process.system(cmd, shell=True, ignore_status=True): | |
| raise NvmeException(f"namespace create command failed {cmd}") | |
| result = process.system(cmd, shell=True, ignore_status=True) | |
| if ( | |
| isinstance(flbas, bool) | |
| or not isinstance(flbas, int) | |
| or not -1 <= flbas <= 15 | |
| ): | |
| raise NvmeException("flbas must be an integer from -1 through 15") | |
| # Determine retry strategy based on flbas parameter | |
| # Only enable auto-detection fallback for default flbas=0 | |
| should_retry_with_auto_detect = flbas == 0 | |
| # Handle explicit auto-detection request (flbas=-1) | |
| # This skips the FLBAS=0 attempt and goes straight to optimal detection | |
| if flbas == -1: | |
| try: | |
| flbas = get_optimal_flbas(controller_name) | |
| LOGGER.info( | |
| f"Auto-detected FLBAS={flbas} for namespace creation on {controller_name}" | |
| ) | |
| except NvmeException as e: | |
| LOGGER.error(f"Failed to auto-detect FLBAS: {e}") | |
| raise NvmeException( | |
| f"Cannot create namespace: FLBAS auto-detection failed: {e}" | |
| ) | |
| # Build and execute namespace creation command | |
| cmd = f"nvme create-ns /dev/{controller_name} --nsze={ns_size} --ncap={ns_size} --flbas={flbas} --dps=0" | |
| if shared_ns: | |
| cmd = f"{cmd} -m 1" | |
| result = process.system(cmd, shell=True, ignore_status=True) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 524-526: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[error] 533-533: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avocado/utils/nvme.py` around lines 510 - 533, Validate flbas at the start of
create_one_ns(), before should_retry_with_auto_detect or command construction,
accepting only integers in the inclusive range -1..15 and rejecting strings or
other non-integer values. Raise the existing appropriate exception for invalid
input, while preserving the special -1 auto-detection path and normal handling
of valid values.
Source: Linters/SAST tools
| if use_jq: | ||
| curl_cmd_str = " ".join( | ||
| [f"'{arg}'" if " " in arg else arg for arg in curl_cmd] | ||
| ) | ||
| full_cmd = f"{curl_cmd_str} | jq" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Hand-rolled shell quoting in both send_vllm_inference_request methods. Both methods quote an argument only when it contains a space, and they use single quotes. payload_json contains double quotes, and prompt is caller-controlled. A prompt with a single quote ends the quoting and the remaining text runs as shell code. Use shlex.join for the argument vector.
avocado/utils/podman.py#L1067-L1071: replace the manual" ".join([f"'{arg}'" ...])construction withshlex.join(curl_cmd), and pass the result through["sh", "-c", ...]instead ofshell=True.avocado/utils/podman.py#L1806-L1809: replace the identical manual construction withshlex.join(curl_cmd)before you passfull_cmd_strtoasyncio.create_subprocess_shell.
📍 Affects 1 file
avocado/utils/podman.py#L1067-L1071(this comment)avocado/utils/podman.py#L1806-L1809
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avocado/utils/podman.py` around lines 1067 - 1071, Replace the manual
argument quoting in both send_vllm_inference_request methods: at
avocado/utils/podman.py lines 1067-1071, use shlex.join(curl_cmd) and execute
the jq pipeline via ["sh", "-c", ...] instead of shell=True; at lines 1806-1809,
use shlex.join(curl_cmd) before passing full_cmd_str to
asyncio.create_subprocess_shell. Ensure shlex is imported and preserve the
existing jq pipeline behavior.
Source: Linters/SAST tools
| def login( | ||
| self, | ||
| registry, | ||
| username=None, | ||
| password=None, | ||
| api_key=None, | ||
| api_key_username="iamapikey", | ||
| password_stdin=False, | ||
| user=None, | ||
| ): | ||
| """Login to a container registry. | ||
|
|
||
| :param str registry: Registry URL. | ||
| :param str username: Username for authentication (optional if using API key). | ||
| :param str password: Password for authentication (optional if using API key). | ||
| :param str api_key: API key for authentication (alternative to username/password). | ||
| :param str api_key_username: Username to use with API key authentication (default: "iamapikey" for IBM Cloud). | ||
| Other registries may use different conventions (e.g., "oauth2accesstoken" for GCR). | ||
| :param bool password_stdin: If True, read password from stdin. | ||
| :param str user: Optional system user to run podman command as. | ||
| :rtype: tuple with returncode, stdout, stderr. | ||
| """ | ||
| try: | ||
| args = ["login"] | ||
|
|
||
| if api_key: | ||
| args.extend(["--username", username or api_key_username]) | ||
| args.extend(["--password", api_key]) | ||
| elif username and password: | ||
| args.extend(["--username", username]) | ||
| if not password_stdin: | ||
| args.extend(["--password", password]) | ||
| elif password_stdin: | ||
| if username: | ||
| args.extend(["--username", username]) | ||
| args.append("--password-stdin") | ||
| else: | ||
| raise PodmanException( | ||
| "Must provide either api_key, username/password, or password_stdin" | ||
| ) | ||
|
|
||
| args.append(registry) | ||
| return self.execute(*args, user=user) | ||
| except PodmanException as ex: | ||
| raise PodmanException(f"Failed to login to registry {registry}.") from ex |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not pass the API key or password on the command line.
Line 1225 and Line 1229 append --password with the secret value. The full argument vector is visible in /proc/<pid>/cmdline to other users on the host. When user is set, Podman.execute also builds a single command string for su -c, so the secret is stored in the shell history of the target account and in the su process arguments.
Also, Podman.execute logs the arguments at Line 621 with LOG.debug("Executing %s", args), so the secret reaches the debug log.
Use --password-stdin for every credential path and write the secret to the process stdin.
Also applies to: 1923-1967
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avocado/utils/podman.py` around lines 1198 - 1242, Update Podman.login to
never place api_key or password in the argument list; use --password-stdin for
both credential paths and provide the selected secret through the subprocess
stdin, including when user is set. Preserve username and api_key_username
handling, and update the relevant Podman.execute flow so stdin is passed safely
without exposing the secret in command strings or debug logs; apply the same
change to the other credential-login implementation around the referenced
symbol.
| async def get_container_port(self, container_id, port=8000, user=None): | ||
| """ | ||
| Get the actual host port mapped to a container port. | ||
|
|
||
| :param container_id: Container ID | ||
| :param port: Container port to check (default: 8000) | ||
| :param user: Username if container was created by specific user | ||
| :return: Host port number or None | ||
| """ | ||
| return get_container_port(container_id, port, user, LOG) | ||
|
|
||
| async def save_container_logs( | ||
| self, container_id, log_dir, test_name="test", user=None | ||
| ): | ||
| """ | ||
| Save complete container logs to a file. | ||
|
|
||
| :param container_id: Container ID | ||
| :param log_dir: Directory to save logs | ||
| :param test_name: Test name for log file naming (default: "test") | ||
| :param user: Username if container was created by specific user | ||
| :return: Path to saved log file or None | ||
| """ | ||
| return save_container_logs(container_id, log_dir, test_name, user, LOG) | ||
|
|
||
| async def wait_for_vllm_startup( | ||
| self, | ||
| container_id, | ||
| success_pattern="Application startup complete.", | ||
| failure_pattern=None, | ||
| additional_failure_checks=None, | ||
| timeout=300, | ||
| check_interval=10, | ||
| user=None, | ||
| show_live_logs=True, | ||
| live_log_lines=10, | ||
| ): | ||
| """ | ||
| Async method: Wait for container to start by checking logs for a success pattern. | ||
|
|
||
| This is a generic async method that can be used for any container type. | ||
| The success and failure patterns can be customized based on the application. | ||
|
|
||
| :param container_id: Container ID to monitor | ||
| :param success_pattern: String pattern to look for in logs indicating successful startup | ||
| :param failure_pattern: Optional string pattern indicating startup failure (e.g., "BACKTRACE") | ||
| :param additional_failure_checks: Optional list of tuples [(pattern, case_sensitive), ...] | ||
| for additional failure detection. Example: | ||
| [("VFIO", False), ("fail", False)] checks for "VFIO" and "fail" | ||
| :param timeout: Maximum time to wait in seconds (default: 300) | ||
| :param check_interval: Time between log checks in seconds (default: 10) | ||
| :param user: Username if container was created by specific user | ||
| :param show_live_logs: If True, display recent log lines during each check (default: True) | ||
| :param live_log_lines: Number of recent log lines to display (default: 10) | ||
| :return: True if startup successful, False otherwise | ||
| """ | ||
| return wait_for_vllm_startup( | ||
| container_id, | ||
| success_pattern, | ||
| failure_pattern, | ||
| additional_failure_checks, | ||
| timeout, | ||
| check_interval, | ||
| user, | ||
| show_live_logs, | ||
| live_log_lines, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
These async def methods block the event loop.
AsyncPodman.get_container_port, AsyncPodman.save_container_logs, and AsyncPodman.wait_for_vllm_startup are coroutines, but each one calls the blocking module-level function directly. No await occurs. wait_for_vllm_startup calls time.sleep in a loop and can block for the full timeout, which defaults to 300 seconds. Every other task on the same event loop stops during that time.
avocado/plugins/runners/podman_image.py runs AsyncPodman inside asyncio.run, so an event loop is always active for this class.
Offload the calls with asyncio.to_thread.
🐛 Proposed fix
async def get_container_port(self, container_id, port=8000, user=None):
@@
- return get_container_port(container_id, port, user, LOG)
+ return await asyncio.to_thread(
+ get_container_port, container_id, port, user, LOG
+ )
@@
- return save_container_logs(container_id, log_dir, test_name, user, LOG)
+ return await asyncio.to_thread(
+ save_container_logs, container_id, log_dir, test_name, user, LOG
+ )
@@
- return wait_for_vllm_startup(
- container_id,
- success_pattern,
- failure_pattern,
- additional_failure_checks,
- timeout,
- check_interval,
- user,
- show_live_logs,
- live_log_lines,
- )
+ return await asyncio.to_thread(
+ wait_for_vllm_startup,
+ container_id,
+ success_pattern,
+ failure_pattern,
+ additional_failure_checks,
+ timeout,
+ check_interval,
+ user,
+ show_live_logs,
+ live_log_lines,
+ )Note that wait_for_vllm_startup at Line 235 also accepts a log parameter after user. The positional call at Line 2056 passes show_live_logs into the log parameter and live_log_lines into show_live_logs. The live_log_lines value is never passed. Use keyword arguments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avocado/utils/podman.py` around lines 2000 - 2066, Update
AsyncPodman.get_container_port, save_container_logs, and wait_for_vllm_startup
to await asyncio.to_thread when invoking their blocking module-level functions,
preserving each method’s current arguments and return values. In
wait_for_vllm_startup, pass arguments by keyword so the underlying log parameter
receives LOG and show_live_logs/live_log_lines map to their intended parameters.
| for _ in range(10): | ||
| status, output = RemoteSpawner.run_remote_cmd( | ||
| session, | ||
| f"pgrep -r R,S -f 'task-run -i {runtime_task.task.identifier}'", | ||
| 10, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Quote the task identifier as one shell argument.
Line 154 interpolates runtime_task.task.identifier inside single quotes. Runnable identifiers can derive from user-controlled URIs or identifier formats. A single quote in the identifier terminates the quote and permits additional remote shell syntax.
Build the complete pgrep -f pattern first. Then pass it through shlex.quote().
Proposed fix
+ pattern = f"task-run -i {runtime_task.task.identifier}"
status, output = RemoteSpawner.run_remote_cmd(
session,
- f"pgrep -r R,S -f 'task-run -i {runtime_task.task.identifier}'",
+ f"pgrep -r R,S -f {shlex.quote(pattern)}",
10,
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@optional_plugins/spawner_remote/avocado_spawner_remote/__init__.py` around
lines 151 - 155, Update the remote command construction in the run_remote_cmd
loop to build the complete pgrep pattern containing
runtime_task.task.identifier, then quote that full pattern with shlex.quote
before interpolating it into the shell command. Add the required shlex import
and preserve the existing pgrep behavior and arguments.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@avocado/core/status/server.py`:
- Around line 28-33: Update the status-server port allocation around
find_free_port and StatusServer.create_server to bind and retain the listening
socket while scanning candidate ports, rather than returning only a number.
Propagate the resolved endpoint only after a bind succeeds, and ensure the
retained socket is used by the server so concurrent processes cannot select the
same port.
In `@avocado/plugins/runner_nrunner.py`:
- Around line 205-207: Update _determine_status_server to check
run.status_server_auto and, when enabled, ignore configured endpoint values in
favor of the default status-server range for both listen and URI resolution.
After resolving the endpoint, write the same concrete value back to both
run.status_server_listen and run.status_server_uri so RuntimeTaskGraph and the
server use the identical endpoint; preserve configured values when automatic
mode is disabled.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c87a8d8-95e1-49aa-b729-0323d9112219
📒 Files selected for processing (2)
avocado/core/status/server.pyavocado/plugins/runner_nrunner.py
| port = network_ports.find_free_port( | ||
| start_port=start, | ||
| end_port=end, | ||
| address=host, | ||
| sequent=True, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bind the selected port before publishing it.
find_free_port() returns only a port number. It does not retain a listening socket. Another Avocado process can bind the selected port before StatusServer.create_server() runs. Two concurrent runs can then select the same port, and one status server fails to start.
Bind and retain the listening socket while scanning the range. Propagate the resolved endpoint only after the bind succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avocado/core/status/server.py` around lines 28 - 33, Update the status-server
port allocation around find_free_port and StatusServer.create_server to bind and
retain the listening socket while scanning candidate ports, rather than
returning only a number. Propagate the resolved endpoint only after a bind
succeeds, and ensure the retained socket is used by the server so concurrent
processes cannot select the same port.
| def _determine_status_server(self, test_suite, config_key): | ||
| if test_suite.config.get("run.status_server_auto"): | ||
| # no UNIX domain sockets on Windows | ||
| if platform.system() != "Windows": | ||
| if self.status_server_dir is None: | ||
| self.status_server_dir = tempfile.TemporaryDirectory( | ||
| prefix="avocado_" | ||
| ) | ||
| return os.path.join(self.status_server_dir.name, ".status_server.sock") | ||
| """Return listen/uri config; default is a port range so multiple runs work.""" | ||
| return test_suite.config.get(config_key) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Honor run.status_server_auto when selecting the endpoint.
When run.status_server_auto is True, this method still returns configured values. For example, a custom run.status_server_uri makes RuntimeTaskGraph send status messages to that custom URI while the server resolves run.status_server_listen. This contradicts the option help and can disconnect tasks from the status server.
When automatic mode is enabled, select the default range for both values. After resolution, update both configuration keys to the same concrete endpoint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avocado/plugins/runner_nrunner.py` around lines 205 - 207, Update
_determine_status_server to check run.status_server_auto and, when enabled,
ignore configured endpoint values in favor of the default status-server range
for both listen and URI resolution. After resolving the endpoint, write the same
concrete value back to both run.status_server_listen and run.status_server_uri
so RuntimeTaskGraph and the server use the identical endpoint; preserve
configured values when automatic mode is disabled.
Signed-off-by: xianglongfei_uniontech <xianglongfei@uniontech.com>
3 participants
Fixed the issue:Supports specifying port ranges, with automatic matching between listen and URI.
Issue: #5550
Command line:
avocado run examples/tests/passtest.py --status-server-disable-auto
--status-server-listen 127.0.0.1:8888-9000 (defalut) --status-server-uri 127.0.0.1:8888-9000 (defalut)
Use the testing program to occupy ports 8888/8889 for verification
Logs:
...
2026-03-04 15:06:58,073 avocado.job job L0291 INFO | 'run.status_server_listen': '127.0.0.1:8888-9000',
2026-03-04 15:06:58,073 avocado.job job L0291 INFO | 'run.status_server_uri': '127.0.0.1:8888-9000',
...
2026-03-04 15:06:58,439 avocado.job server L0068 INFO | Status server listening on 127.0.0.1:8890
2026-03-04 15:06:58,740 avocado.job testlogs L0132 INFO | examples/tests/passtest.py:PassTest.test: STARTED
2026-03-04 15:06:58,948 avocado.job testlogs L0138 INFO | examples/tests/passtest.py:PassTest.test: PASS
...
Signed-off-by: xianglongfei xianglongfei@uniontech.com
Summary by CodeRabbit
New Features
127.0.0.1:8888-9000.Documentation
avocado runoptions to explain automatic selection, manual configuration, supported address formats, and configuration precedence.