Removed LSBLK dependency, fixed progress bar. (Issue #178, partially #171)#3
Merged
Merged
Conversation
Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: src/lufus/drives/find_usb.py modified: src/lufus/drives/get_usb_info.py modified: tests/test_find_usb.py modified: tests/test_flash_usb_and_find_usb_fixes.py modified: tests/test_get_usb_info.py
Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is ahead of 'origin/less-deps-feature-win-tweaks' by 1 commit. (use "git push" to publish your local commits) Changes to be committed: modified: tests/test_get_usb_info_and_detect_windows_fixes.py
Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: src/lufus/writing/flash_usb.py
Reviewer's GuideRefactors device and USB handling to remove external CLI dependencies in favor of /proc, sysfs, pyudev, and psutil; improves dd-based flash progress reporting and GUI UX (downloads dir autodetection, tooltips, dialogs); tightens Windows flashing behavior and tweaks handling; and updates tests, build, CI, versioning, and docs accordingly. Sequence diagram for dd-based USB flashing progress reportingsequenceDiagram
actor User
participant MainWindow
participant FlashWorker
participant flash_usb
participant subprocess_dd as dd_process
User->>MainWindow: click btn_start
MainWindow->>MainWindow: validate options
MainWindow->>FlashWorker: create with options
MainWindow->>FlashWorker: start()
activate FlashWorker
FlashWorker->>flash_usb: flash_usb(device_node, iso_path, scheme, progress_cb, status_cb)
activate flash_usb
flash_usb->>flash_usb: build dd_args
flash_usb->>subprocess_dd: Popen(dd_args, stderr=PIPE, env={LC_ALL="C"})
flash_usb->>MainWindow: status_cb("Flash starting...")
flash_usb->>MainWindow: progress_cb(0)
loop while dd running
subprocess_dd-->>flash_usb: stderr chunks with CR-separated progress
flash_usb->>flash_usb: buffer += chunk
flash_usb->>flash_usb: split on CR/LF
alt percent line ("NN%" or derived from bytes)
flash_usb->>MainWindow: status_cb("bytes / total bytes (NN% completed)")
flash_usb->>MainWindow: progress_cb(NN)
else dd bookkeeping or other stderr
flash_usb->>flash_usb: log.info("dd stderr: line")
end
end
subprocess_dd-->>flash_usb: exit code
flash_usb->>MainWindow: status_cb("dd exited with code X")
deactivate flash_usb
FlashWorker-->>MainWindow: flash_done(success)
deactivate FlashWorker
MainWindow->>MainWindow: on_flash_finished(success)
MainWindow->>User: show success/failure dialog, update progress bar
Sequence diagram for cancelling a flash and killing device-using processessequenceDiagram
actor User
participant MainWindow
participant FlashWorker
participant ProcFD as _procs_using_device_fd
participant ProcMnt as _procs_with_device_mounted
participant OS as os_kernel
User->>MainWindow: click btn_cancel
activate MainWindow
MainWindow->>MainWindow: device_node = state.device_node
MainWindow->>ProcFD: _procs_using_device_fd(device_node)
ProcFD->>ProcFD: stat(device_node).st_rdev
ProcFD->>OS: iterate /proc/*/fd
OS-->>ProcFD: open fds with matching st_rdev
ProcFD-->>MainWindow: list[(pid, comm)]
MainWindow->>MainWindow: log processes using device
alt FlashWorker is running
MainWindow->>FlashWorker: request_interruption()
MainWindow->>FlashWorker: wait(2000)
end
MainWindow->>ProcMnt: _procs_with_device_mounted(device_node)
ProcMnt->>ProcMnt: stat(device_node) -> major:minor
ProcMnt->>OS: read /proc/*/mountinfo
OS-->>ProcMnt: lines with matching major:minor
ProcMnt-->>MainWindow: list[(pid, comm)]
loop for each pid
MainWindow->>OS: os.kill(pid, SIGKILL)
end
MainWindow->>MainWindow: log killed pids or no processes
alt is_terminal
MainWindow->>MainWindow: termios.tcgetattr(stdin_fd)
MainWindow->>MainWindow: restore ICANON,ECHO,ISIG,ICRNL,OPOST
MainWindow->>MainWindow: termios.tcsetattr(...)
end
MainWindow->>MainWindow: reset progress bar, status text, buttons
deactivate MainWindow
Updated class diagram for GUI workers, dialogs, and Windows flashing typesclassDiagram
class AppState {
+str version
+int filesystem_index
+int cluster_size
+int quick_format
+int check_bad
+str new_label
+int flash_mode
+str iso_path
+str device_node
+bool verify_hash
+str expected_hash
+int win_hardware_bypass
+int win_microsoft_acc
+str win_local_acc
+int win_privacy
}
class FlashWorker {
+dict options
+dict _T
+run()
+progress:int
+status:str
+flash_done:bool
}
class LogWindow {
+__init__(parent)
}
class AboutWindow {
+__init__(parent)
}
class SettingsDialog {
+__init__(parent)
+accepted()
+rejected()
}
class PartitionInfo {
<<TypedDict>>
+str role
+str path
}
class WindowsFlashModule {
<<module writing.windows.flash>>
+mount_iso(iso_path) str|None
+create_partitions(drive, scheme) list~PartitionInfo~
+_copy_efi_boot_files(iso_mount, mount_efi, _status)
+_fix_efi_bootloader(efi_mount)
+_get_disk_size_sectors(drive) int
}
class UtilsModule {
<<module lufus.utils>>
+run_cmd(cmd list~str~)
+elevate_privileges()
}
class RemovedWinTweaks {
<<removed QDialog>>
+log_message(msg)
+update_winhardware()
+update_winmicrosoftacc()
+update_winlocalaccchk()
+sync_username(new_username)
+update_winprivacy()
+applywintweaks()
}
%% Relationships
FlashWorker --> AppState : reads
FlashWorker ..> WindowsFlashModule : calls
FlashWorker ..> flash_usb : calls
LogWindow --> AppState : reads version
AboutWindow --> AppState : reads version
SettingsDialog --> AppState : updates theme and preferences
WindowsFlashModule ..> PartitionInfo : returns list
WindowsFlashModule ..> UtilsModule : uses run_cmd
RemovedWinTweaks --> AppState : modified Windows tweak flags
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: src/lufus/drives/get_usb_info.py new file: tests/conftest.py modified: tests/test_find_usb.py modified: tests/test_flash_usb_and_find_usb_fixes.py modified: tests/test_get_usb_info.py modified: tests/test_get_usb_info_and_detect_windows_fixes.py
with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: .github/workflows/python-compatibility.yml
…ting" This reverts commit dc34beb. Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: src/lufus/drives/get_usb_info.py deleted: tests/conftest.py modified: tests/test_find_usb.py modified: tests/test_flash_usb_and_find_usb_fixes.py modified: tests/test_get_usb_info.py modified: tests/test_get_usb_info_and_detect_windows_fixes.py
This reverts commit f77fd1c. Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: src/lufus/writing/flash_usb.py
Owner
Author
|
@sourcery-ai review |
with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: README.md
Owner
Author
|
@sourcery-ai review |
1 similar comment
Owner
Author
|
@sourcery-ai review |
with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: .github/workflows/ci.yml modified: src/lufus/drives/get_usb_info.py modified: tests/test_find_usb.py modified: tests/test_flash_usb_and_find_usb_fixes.py modified: tests/test_get_usb_info.py modified: tests/test_get_usb_info_and_detect_windows_fixes.py Untracked files: TODO
Owner
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
get_usb_info,device.attributes.get("size")from pyudev is likely to be a bytes-like object rather than a plain string; consider normalizing/decoding the attribute (and handling non‑numeric values) before passing it toint()to avoid runtime errors on real systems. - There is quite a bit of duplicated mocking setup for
os.statandpyudevacross multiple tests; extracting a shared helper or fixture for the udev/stat mocking would make the tests easier to maintain and less error-prone. - The updated docstring in
TestLabelIsWindowsDeadBranchincludes informal language ("Idk why I don't like that, but eh, it works..."); consider reverting to a neutral description to keep tests and diagnostics professional and focused on behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `get_usb_info`, `device.attributes.get("size")` from pyudev is likely to be a bytes-like object rather than a plain string; consider normalizing/decoding the attribute (and handling non‑numeric values) before passing it to `int()` to avoid runtime errors on real systems.
- There is quite a bit of duplicated mocking setup for `os.stat` and `pyudev` across multiple tests; extracting a shared helper or fixture for the udev/stat mocking would make the tests easier to maintain and less error-prone.
- The updated docstring in `TestLabelIsWindowsDeadBranch` includes informal language (`"Idk why I don't like that, but eh, it works..."`); consider reverting to a neutral description to keep tests and diagnostics professional and focused on behavior.
## Individual Comments
### Comment 1
<location path="src/lufus/drives/get_usb_info.py" line_range="33-37" />
<code_context>
- if not size_output.isdigit():
- log.warning("Could not parse device size: %r", size_output)
+ # Size in bytes: udev attributes 'size' is in 512-byte sectors
+ size_attr = device.attributes.get("size")
+ usb_size = int(size_attr) * 512 if size_attr else 0
+
+ label = device.get("ID_FS_LABEL")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling and logging invalid or missing `size` attributes more explicitly.
The previous implementation logged a warning when `lsblk` returned a non-numeric size before defaulting to 0. In this new path, missing or non-numeric `size_attr` quietly results in 0, which can obscure size-detection issues. Please add a warning when `size_attr` is `None` or when `int(size_attr)` fails, so operators can distinguish parsing failures from genuine 0-sized devices.
```suggestion
# Size in bytes: udev attributes 'size' is in 512-byte sectors
size_attr = device.attributes.get("size")
if size_attr is None:
log.warning(
"Could not get 'size' attribute from udev for device %s; defaulting to 0 bytes",
device_node,
)
usb_size = 0
else:
try:
usb_size = int(size_attr) * 512
except (TypeError, ValueError):
log.warning(
"Could not parse udev 'size' attribute %r for device %s; defaulting to 0 bytes",
size_attr,
device_node,
)
usb_size = 0
label = device.get("ID_FS_LABEL")
```
</issue_to_address>
### Comment 2
<location path="src/lufus/drives/find_usb.py" line_range="56-61" />
<code_context>
label = None
try:
- label = subprocess.check_output(
- ["lsblk", "-d", "-n", "-o", "LABEL", device_node],
- text=True,
- timeout=5,
- ).strip()
- except (subprocess.CalledProcessError, subprocess.TimeoutExpired):
+ # Using os.stat to get device number as per requirements
+ st = os.stat(device_node)
+ device = pyudev.Devices.from_device_number(context, "block", st.st_rdev)
+ label = device.get("ID_FS_LABEL")
+ except Exception:
pass
</code_context>
<issue_to_address>
**issue (bug_risk):** The blanket `except Exception: pass` may hide real issues when querying pyudev.
Catching all exceptions here can mask persistent pyudev failures (e.g. permissions/env issues) and silently reduce behavior to always using `os.path.basename`. Please narrow the exception to the specific pyudev errors you expect, or at least log at debug level so failures in this block remain visible.
</issue_to_address>
### Comment 3
<location path=".github/workflows/python-compatibility.yml" line_range="17" />
<code_context>
strategy:
fail-fast: false
matrix:
- python-version: ["3.10", "3.11", "3.12", "3.13", "3.14-dev"]
+ python-version: ["3.10", "3.13"]
steps:
</code_context>
<issue_to_address>
**question (testing):** Reducing the Python matrix may drop coverage for supported runtimes.
This change speeds up CI but drops CI coverage for 3.11 and 3.12. If we still claim support for those versions, consider keeping at least one (e.g. 3.11 or 3.12) in the matrix to help catch compatibility issues across 3.10–3.13.
</issue_to_address>
### Comment 4
<location path="tests/test_get_usb_info.py" line_range="97-100" />
<code_context>
-def test_get_usb_info_returns_empty_when_lsblk_fails(monkeypatch) -> None:
+def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None:
mount_path = "/media/testuser/USB"
device_node = "/dev/sdb1"
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test where pyudev fails independently of os.stat to fully exercise the new error-handling path.
The existing `test_get_usb_info_returns_empty_when_pyudev_fails` only covers failures via `os.stat` raising. Since the code can also fail when `pyudev.Devices.from_device_number` or attribute access on the returned `device` raises, please add a test where `os.stat` succeeds but the pyudev lookup fails (e.g., `Devices.from_device_number` raises or returns a device without `attributes['size']`) to verify these cases also return `None`.
Suggested implementation:
```python
def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None:
```
Please add a new test function to cover the case where `os.stat` succeeds but the pyudev lookup fails independently (e.g. `Devices.from_device_number` raises). Place it near the other `get_usb_info` tests, for example directly after `test_get_usb_info_returns_empty_when_pyudev_fails`:
```python
def test_get_usb_info_returns_empty_when_pyudev_device_lookup_raises(monkeypatch) -> None:
mount_path = "/media/testuser/USB"
device_node = "/dev/sdb1"
# Partition for the mount path is found
monkeypatch.setattr(
get_usb_info_module.psutil,
"disk_partitions",
lambda *args, **kwargs: [
SimpleNamespace(mountpoint=mount_path, device=device_node)
],
)
# os.stat succeeds and yields a valid st_dev
monkeypatch.setattr(
get_usb_info_module.os,
"stat",
lambda p: SimpleNamespace(st_dev=1234),
)
# pyudev context can be constructed, but the device lookup itself fails
mock_context = object()
monkeypatch.setattr(
get_usb_info_module.pyudev,
"Context",
lambda: mock_context,
)
def failing_from_device_number(*args, **kwargs):
raise RuntimeError("pyudev failure")
monkeypatch.setattr(
get_usb_info_module.pyudev.Devices,
"from_device_number",
failing_from_device_number,
)
result = get_usb_info_module.get_usb_info(mount_path)
# When pyudev fails independently of os.stat, we should still return an empty result
assert result is None
```
If your implementation also has a separate failure path when the device is found but `device.attributes["size"]` is missing or raises, you can add a sister test to explicitly cover that:
```python
def test_get_usb_info_returns_empty_when_pyudev_missing_size_attribute(monkeypatch) -> None:
mount_path = "/media/testuser/USB"
device_node = "/dev/sdb1"
monkeypatch.setattr(
get_usb_info_module.psutil,
"disk_partitions",
lambda *args, **kwargs: [
SimpleNamespace(mountpoint=mount_path, device=device_node)
],
)
monkeypatch.setattr(
get_usb_info_module.os,
"stat",
lambda p: SimpleNamespace(st_dev=1234),
)
mock_context = object()
monkeypatch.setattr(
get_usb_info_module.pyudev,
"Context",
lambda: mock_context,
)
mock_device = SimpleNamespace()
# Simulate missing / inaccessible size attribute
mock_device.attributes = {}
monkeypatch.setattr(
get_usb_info_module.pyudev.Devices,
"from_device_number",
lambda *args, **kwargs: mock_device,
)
result = get_usb_info_module.get_usb_info(mount_path)
assert result is None
```
Adjust the final assertions (`assert result is None` vs `assert result == {}` or similar) to match the actual "empty" value returned by `get_usb_info` in your codebase, keeping it consistent with the existing `test_get_usb_info_returns_empty_when_pyudev_fails` expectations.
</issue_to_address>
### Comment 5
<location path="tests/test_get_usb_info.py" line_range="49-53" />
<code_context>
+
+ # Mock pyudev
+ mock_context = MagicMock()
+ mock_device = MagicMock()
+ mock_device.get.return_value = "lufus_USB"
+ monkeypatch.setattr(find_usb_module.pyudev, "Context", lambda: mock_context)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for malformed or non-integer pyudev size attributes.
Right now the tests only cover cases where `device.attributes['size']` is a valid integer string. Since the code does `int(size_attr) * 512 if size_attr else 0` and catches all `Exception`, please add a test where `size` is non-numeric (e.g. `'abc'`) and assert the resulting behavior (likely `None`). This will lock in the expected handling of malformed udev metadata and avoid regressions around `ValueError` handling.
</issue_to_address>
### Comment 6
<location path="tests/test_find_usb.py" line_range="66-62" />
<code_context>
-def test_find_usb_falls_back_to_dir_name_when_lsblk_fails(monkeypatch) -> None:
+def test_find_usb_falls_back_to_dir_name_when_pyudev_fails(monkeypatch) -> None:
user = "testuser"
mount_path = f"/media/{user}/NO_LABEL"
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering the fallback when pyudev returns an empty label rather than raising.
The current test only covers the exception path. Since the code also falls back to the directory name when `label` is falsy, please add a test where `Devices.from_device_number` succeeds but `device.get("ID_FS_LABEL")` returns an empty string, to explicitly cover that branch.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| python-version: ["3.10", "3.11", "3.12", "3.13", "3.14-dev"] |
There was a problem hiding this comment.
question (testing): Reducing the Python matrix may drop coverage for supported runtimes.
This change speeds up CI but drops CI coverage for 3.11 and 3.12. If we still claim support for those versions, consider keeping at least one (e.g. 3.11 or 3.12) in the matrix to help catch compatibility issues across 3.10–3.13.
Comment on lines
+97
to
100
| def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None: | ||
| mount_path = "/media/testuser/USB" | ||
| device_node = "/dev/sdb1" | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Add a test where pyudev fails independently of os.stat to fully exercise the new error-handling path.
The existing test_get_usb_info_returns_empty_when_pyudev_fails only covers failures via os.stat raising. Since the code can also fail when pyudev.Devices.from_device_number or attribute access on the returned device raises, please add a test where os.stat succeeds but the pyudev lookup fails (e.g., Devices.from_device_number raises or returns a device without attributes['size']) to verify these cases also return None.
Suggested implementation:
def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None:Please add a new test function to cover the case where os.stat succeeds but the pyudev lookup fails independently (e.g. Devices.from_device_number raises). Place it near the other get_usb_info tests, for example directly after test_get_usb_info_returns_empty_when_pyudev_fails:
def test_get_usb_info_returns_empty_when_pyudev_device_lookup_raises(monkeypatch) -> None:
mount_path = "/media/testuser/USB"
device_node = "/dev/sdb1"
# Partition for the mount path is found
monkeypatch.setattr(
get_usb_info_module.psutil,
"disk_partitions",
lambda *args, **kwargs: [
SimpleNamespace(mountpoint=mount_path, device=device_node)
],
)
# os.stat succeeds and yields a valid st_dev
monkeypatch.setattr(
get_usb_info_module.os,
"stat",
lambda p: SimpleNamespace(st_dev=1234),
)
# pyudev context can be constructed, but the device lookup itself fails
mock_context = object()
monkeypatch.setattr(
get_usb_info_module.pyudev,
"Context",
lambda: mock_context,
)
def failing_from_device_number(*args, **kwargs):
raise RuntimeError("pyudev failure")
monkeypatch.setattr(
get_usb_info_module.pyudev.Devices,
"from_device_number",
failing_from_device_number,
)
result = get_usb_info_module.get_usb_info(mount_path)
# When pyudev fails independently of os.stat, we should still return an empty result
assert result is NoneIf your implementation also has a separate failure path when the device is found but device.attributes["size"] is missing or raises, you can add a sister test to explicitly cover that:
def test_get_usb_info_returns_empty_when_pyudev_missing_size_attribute(monkeypatch) -> None:
mount_path = "/media/testuser/USB"
device_node = "/dev/sdb1"
monkeypatch.setattr(
get_usb_info_module.psutil,
"disk_partitions",
lambda *args, **kwargs: [
SimpleNamespace(mountpoint=mount_path, device=device_node)
],
)
monkeypatch.setattr(
get_usb_info_module.os,
"stat",
lambda p: SimpleNamespace(st_dev=1234),
)
mock_context = object()
monkeypatch.setattr(
get_usb_info_module.pyudev,
"Context",
lambda: mock_context,
)
mock_device = SimpleNamespace()
# Simulate missing / inaccessible size attribute
mock_device.attributes = {}
monkeypatch.setattr(
get_usb_info_module.pyudev.Devices,
"from_device_number",
lambda *args, **kwargs: mock_device,
)
result = get_usb_info_module.get_usb_info(mount_path)
assert result is NoneAdjust the final assertions (assert result is None vs assert result == {} or similar) to match the actual "empty" value returned by get_usb_info in your codebase, keeping it consistent with the existing test_get_usb_info_returns_empty_when_pyudev_fails expectations.
Comment on lines
+49
to
+53
| mock_device = MagicMock() | ||
| mock_device.attributes = {"size": str((16 * 1024**3) // 512)} | ||
| mock_device.get.return_value = "MYUSB" | ||
| monkeypatch.setattr(get_usb_info_module.pyudev, "Context", lambda: mock_context) | ||
| monkeypatch.setattr(get_usb_info_module.pyudev.Devices, "from_device_number", lambda ctx, type, num: mock_device) |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for malformed or non-integer pyudev size attributes.
Right now the tests only cover cases where device.attributes['size'] is a valid integer string. Since the code does int(size_attr) * 512 if size_attr else 0 and catches all Exception, please add a test where size is non-numeric (e.g. 'abc') and assert the resulting behavior (likely None). This will lock in the expected handling of malformed udev metadata and avoid regressions around ValueError handling.
| monkeypatch.setattr(find_usb_module.pyudev, "Context", lambda: mock_context) | ||
| monkeypatch.setattr(find_usb_module.pyudev.Devices, "from_device_number", lambda ctx, type, num: mock_device) | ||
|
|
||
| result = find_usb_module.find_usb() |
There was a problem hiding this comment.
suggestion (testing): Add a test covering the fallback when pyudev returns an empty label rather than raising.
The current test only covers the exception path. Since the code also falls back to the directory name when label is falsy, please add a test where Devices.from_device_number succeeds but device.get("ID_FS_LABEL") returns an empty string, to explicitly cover that branch.
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The pyudev-based lookups in both get_usb_info and find_usb are wrapped in broad Exception handlers; consider narrowing these to catch only expected pyudev/os errors so that unexpected bugs don’t get silently swallowed.
- There is quite a bit of duplicated mocking logic for os.stat and pyudev across multiple tests; extracting a shared helper (similar to _mock_os_stat_and_pyudev) into a common test utility would make the tests easier to maintain.
- The updated test docstring for TestLabelIsWindowsDeadBranch now includes informal wording (“Idk why…”); consider reverting to a neutral explanation to keep comments focused and professional.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pyudev-based lookups in both get_usb_info and find_usb are wrapped in broad Exception handlers; consider narrowing these to catch only expected pyudev/os errors so that unexpected bugs don’t get silently swallowed.
- There is quite a bit of duplicated mocking logic for os.stat and pyudev across multiple tests; extracting a shared helper (similar to _mock_os_stat_and_pyudev) into a common test utility would make the tests easier to maintain.
- The updated test docstring for TestLabelIsWindowsDeadBranch now includes informal wording (“Idk why…”); consider reverting to a neutral explanation to keep comments focused and professional.
## Individual Comments
### Comment 1
<location path="src/lufus/drives/get_usb_info.py" line_range="33-37" />
<code_context>
- if not size_output.isdigit():
- log.warning("Could not parse device size: %r", size_output)
+ # Size in bytes: udev attributes 'size' is in 512-byte sectors
+ size_attr = device.attributes.get("size")
+ usb_size = int(size_attr) * 512 if size_attr else 0
+
+ label = device.get("ID_FS_LABEL")
</code_context>
<issue_to_address>
**suggestion:** Consider handling missing or non-numeric `size` attributes more explicitly.
With this change, missing `size` now silently maps to `usb_size = 0`, and non-numeric `size` will raise and hit the generic `Exception` path, returning `None`. Consider explicitly handling both cases: log a clear warning when `size` is missing or non-numeric, and, where reasonable, still return a `USBDeviceInfo` with `usb_size = 0`. This preserves the previous, more predictable behavior and keeps diagnosing udev issues easier.
```suggestion
# Size in bytes: udev attribute 'size' is in 512-byte sectors
size_attr = device.attributes.get("size")
usb_size = 0
if size_attr is None:
log.warning(
"Missing 'size' attribute for device %s; assuming size 0 bytes.",
device_node,
)
else:
try:
usb_size = int(size_attr) * 512
except (TypeError, ValueError):
log.warning(
"Could not parse numeric 'size' attribute %r for device %s; "
"assuming size 0 bytes.",
size_attr,
device_node,
)
label = device.get("ID_FS_LABEL")
```
</issue_to_address>
### Comment 2
<location path="src/lufus/drives/find_usb.py" line_range="61" />
<code_context>
+ st = os.stat(device_node)
+ device = pyudev.Devices.from_device_number(context, "block", st.st_rdev)
+ label = device.get("ID_FS_LABEL")
+ except Exception:
pass
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching all exceptions here may hide unexpected failures when inspecting devices.
Previously we only suppressed `CalledProcessError`/`TimeoutExpired`, which matched the expected `lsblk` failures. With `pyudev`, `except Exception:` will now swallow any error from `os.stat` or `pyudev.Devices.from_device_number`, including programming or environment issues. Please narrow this to the specific exceptions you expect (or at least log unexpected ones) so real problems with device enumeration are visible.
</issue_to_address>
### Comment 3
<location path="tests/test_get_usb_info.py" line_range="93-97" />
<code_context>
-def test_get_usb_info_returns_empty_when_lsblk_fails(monkeypatch) -> None:
+def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None:
mount_path = "/media/testuser/USB"
device_node = "/dev/sdb1"
</code_context>
<issue_to_address>
**suggestion (testing):** Add a dedicated test for PermissionError in get_usb_info to ensure that branch is covered.
Right now only the generic exception path from `os.stat` is tested. Please add a test that explicitly makes `get_usb_info_module.os.stat` raise `PermissionError` and verifies that `get_usb_info` returns `None` without re-raising, so the dedicated handler is covered and guarded against regressions.
```suggestion
result = get_usb_info_module.get_usb_info(mount_path)
assert result["label"] == "NO_LABEL"
def test_get_usb_info_returns_none_on_permission_error(monkeypatch) -> None:
mount_path = "/media/testuser/USB"
device_node = "/dev/sdb1"
monkeypatch.setattr(
get_usb_info_module.psutil,
"disk_partitions",
lambda *args, **kwargs: [
SimpleNamespace(mountpoint=mount_path, device=device_node)
],
)
def mock_os_stat(path: str) -> None:
raise PermissionError("Permission denied")
monkeypatch.setattr(get_usb_info_module.os, "stat", mock_os_stat)
result = get_usb_info_module.get_usb_info(mount_path)
assert result is None
def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
93
to
+97
| result = get_usb_info_module.get_usb_info(mount_path) | ||
| assert result["label"] == "NO_LABEL" | ||
|
|
||
|
|
||
| def test_get_usb_info_returns_empty_when_lsblk_fails(monkeypatch) -> None: | ||
| def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None: |
There was a problem hiding this comment.
suggestion (testing): Add a dedicated test for PermissionError in get_usb_info to ensure that branch is covered.
Right now only the generic exception path from os.stat is tested. Please add a test that explicitly makes get_usb_info_module.os.stat raise PermissionError and verifies that get_usb_info returns None without re-raising, so the dedicated handler is covered and guarded against regressions.
Suggested change
| result = get_usb_info_module.get_usb_info(mount_path) | |
| assert result["label"] == "NO_LABEL" | |
| def test_get_usb_info_returns_empty_when_lsblk_fails(monkeypatch) -> None: | |
| def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None: | |
| result = get_usb_info_module.get_usb_info(mount_path) | |
| assert result["label"] == "NO_LABEL" | |
| def test_get_usb_info_returns_none_on_permission_error(monkeypatch) -> None: | |
| mount_path = "/media/testuser/USB" | |
| device_node = "/dev/sdb1" | |
| monkeypatch.setattr( | |
| get_usb_info_module.psutil, | |
| "disk_partitions", | |
| lambda *args, **kwargs: [ | |
| SimpleNamespace(mountpoint=mount_path, device=device_node) | |
| ], | |
| ) | |
| def mock_os_stat(path: str) -> None: | |
| raise PermissionError("Permission denied") | |
| monkeypatch.setattr(get_usb_info_module.os, "stat", mock_os_stat) | |
| result = get_usb_info_module.get_usb_info(mount_path) | |
| assert result is None | |
| def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None: |
SourceryAI
reviewed
May 7, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The pyudev/os.stat mocking logic in the tests (e.g. for get_usb_info and find_usb) is duplicated and slightly varied; consider extracting a shared helper or pytest fixture to centralize this setup and reduce risk of inconsistencies between tests.
- In get_usb_info, parsing the udev
sizeattribute assumes it is always a valid integer; adding a small try/except around theint(size_attr)conversion with a targeted warning would make the behaviour clearer and avoid the generic catch-all handling unexpected ValueError cases. - The new pyudev integration in find_usb wraps all errors in a broad
except Exceptionwith no logging; adding at least a debug-level log there would make it easier to diagnose situations where label lookup repeatedly fails without surfacing why.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pyudev/os.stat mocking logic in the tests (e.g. for get_usb_info and find_usb) is duplicated and slightly varied; consider extracting a shared helper or pytest fixture to centralize this setup and reduce risk of inconsistencies between tests.
- In get_usb_info, parsing the udev `size` attribute assumes it is always a valid integer; adding a small try/except around the `int(size_attr)` conversion with a targeted warning would make the behaviour clearer and avoid the generic catch-all handling unexpected ValueError cases.
- The new pyudev integration in find_usb wraps all errors in a broad `except Exception` with no logging; adding at least a debug-level log there would make it easier to diagnose situations where label lookup repeatedly fails without surfacing why.
## Individual Comments
### Comment 1
<location path="src/lufus/drives/get_usb_info.py" line_range="33-37" />
<code_context>
- if not size_output.isdigit():
- log.warning("Could not parse device size: %r", size_output)
+ # Size in bytes: udev attributes 'size' is in 512-byte sectors
+ size_attr = device.attributes.get("size")
+ usb_size = int(size_attr) * 512 if size_attr else 0
+
+ label = device.get("ID_FS_LABEL")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handle non-numeric or malformed `size` attributes more defensively.
If `size_attr` is present but not a clean integer, `int(size_attr)` will raise `ValueError` and we’ll hit the generic `Exception` handler, causing the whole function to return `None` instead of just treating the size as unknown. Wrap the cast in a narrow `try/except` for `TypeError`/`ValueError`, log a warning, and fall back to `usb_size = 0` so other USB info can still be returned.
```suggestion
# Size in bytes: udev attributes 'size' is in 512-byte sectors
size_attr = device.attributes.get("size")
usb_size = 0
if size_attr:
try:
usb_size = int(size_attr) * 512
except (TypeError, ValueError):
log.warning(
"Could not parse device size attribute %r; treating size as unknown",
size_attr,
)
label = device.get("ID_FS_LABEL")
```
</issue_to_address>
### Comment 2
<location path="tests/test_get_usb_info.py" line_range="97-100" />
<code_context>
-def test_get_usb_info_returns_empty_when_lsblk_fails(monkeypatch) -> None:
+def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None:
mount_path = "/media/testuser/USB"
device_node = "/dev/sdb1"
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for PermissionError and malformed pyudev size to fully cover get_usb_info error handling.
This currently only exercises the generic `Exception` path via `os.stat` failure. To fully cover `get_usb_info`’s error handling, please also add tests that:
1. Simulate `PermissionError` from `os.stat` or `pyudev.Devices.from_device_number` and assert `get_usb_info` returns `None`.
2. Simulate a non-numeric `device.attributes.get("size")` that causes `int(size_attr)` to raise `ValueError`, and assert it also returns `None`.
These will ensure the dedicated `PermissionError` branch and malformed size handling remain covered and regressions are caught.
</issue_to_address>
### Comment 3
<location path="tests/test_find_usb.py" line_range="66-62" />
<code_context>
-def test_find_usb_falls_back_to_dir_name_when_lsblk_fails(monkeypatch) -> None:
+def test_find_usb_falls_back_to_dir_name_when_pyudev_fails(monkeypatch) -> None:
user = "testuser"
mount_path = f"/media/{user}/NO_LABEL"
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for pyudev returning an empty/None label (non-exceptional) to exercise the fallback path.
The new test covers the exception path from `pyudev.Devices.from_device_number`. Please also add a test where `from_device_number` succeeds but `device.get("ID_FS_LABEL")` returns `None` or `""`, so we verify the fallback to the directory basename for non-exceptional pyudev failures as well.
</issue_to_address>
### Comment 4
<location path="tests/test_get_usb_info_and_detect_windows_fixes.py" line_range="24-33" />
<code_context>
- if "SIZE" in cmd:
- return size + "\n"
- return label + "\n"
+def _mock_os_stat_and_pyudev(monkeypatch, size="1000000000", label="MY_USB"):
+ # Mock os.stat safely
+ os_stat_orig = gui_module.os.stat
- return impl
+ def mock_os_stat(p):
+ if str(p).startswith("/dev/"):
+ m = MagicMock()
+ m.st_rdev = 1234
+ return m
+ return os_stat_orig(p)
+
+ monkeypatch.setattr(gui_module.os, "stat", mock_os_stat)
+
+ # Mock pyudev
+ mock_context = MagicMock()
+ mock_device = MagicMock()
+ # size in 512-byte sectors
+ mock_device.attributes = {"size": str(int(size) // 512)}
+ mock_device.get.return_value = label
+ monkeypatch.setattr(gui_module.pyudev, "Context", lambda: mock_context)
+ monkeypatch.setattr(gui_module.pyudev.Devices, "from_device_number", lambda ctx, type, num: mock_device)
</code_context>
<issue_to_address>
**suggestion:** Consider reusing _mock_os_stat_and_pyudev in other tests to reduce duplication and keep behaviour aligned.
Similar pyudev mocking appears in `tests/test_get_usb_info.py` and `tests/test_find_usb.py`. Reusing this helper there (or moving it to a shared test helper/fixture) would keep all pyudev-dependent tests aligned on how `os.stat.st_rdev` and `device.attributes['size']` are interpreted, and make future changes to the pyudev integration easier to apply consistently. This is a non-blocking suggestion for test maintainability.
Suggested implementation:
```python
def _fake_partitions(mount, device):
return lambda all=False: [SimpleNamespace(mountpoint=mount, device=device)]
def _mock_os_stat_and_pyudev(monkeypatch, size="1000000000", label="MY_USB"):
# Mock os.stat safely
os_stat_orig = gui_module.os.stat
def mock_os_stat(p):
if str(p).startswith("/dev/"):
m = MagicMock()
m.st_rdev = 1234
return m
return os_stat_orig(p)
monkeypatch.setattr(gui_module.os, "stat", mock_os_stat)
# Mock pyudev
mock_context = MagicMock()
mock_device = MagicMock()
# size in 512-byte sectors
mock_device.attributes = {"size": str(int(size) // 512)}
mock_device.get.return_value = label
monkeypatch.setattr(gui_module.pyudev, "Context", lambda: mock_context)
monkeypatch.setattr(gui_module.pyudev.Devices, "from_device_number", lambda ctx, type, num: mock_device)
```
To fully implement the reuse suggestion across the test suite, you should also:
1. Ensure `gui_module` and `MagicMock` are imported at the top of `tests/test_get_usb_info_and_detect_windows_fixes.py` (if not already present):
- `from unittest.mock import MagicMock`
- `import gui_module` (or whatever module contains the GUI code under test).
2. In `tests/test_get_usb_info.py` and `tests/test_find_usb.py`, remove any duplicated pyudev/os.stat mocking logic and call `_mock_os_stat_and_pyudev(monkeypatch, ...)` from this module instead, e.g.:
- `from tests.test_get_usb_info_and_detect_windows_fixes import _mock_os_stat_and_pyudev`
- Then use `_mock_os_stat_and_pyudev(monkeypatch, size="...", label="...")` in those tests.
3. If you prefer not to import from a test module, you can instead move `_mock_os_stat_and_pyudev` into a shared helper (e.g. `tests/usb_test_helpers.py` or `tests/conftest.py`) and update:
- This file to import and use that shared helper.
- `tests/test_get_usb_info.py` and `tests/test_find_usb.py` to import and use the shared helper as well.
</issue_to_address>Hi @Radiump123! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+97
to
100
| def test_get_usb_info_returns_empty_when_pyudev_fails(monkeypatch) -> None: | ||
| mount_path = "/media/testuser/USB" | ||
| device_node = "/dev/sdb1" | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Add tests for PermissionError and malformed pyudev size to fully cover get_usb_info error handling.
This currently only exercises the generic Exception path via os.stat failure. To fully cover get_usb_info’s error handling, please also add tests that:
- Simulate
PermissionErrorfromos.statorpyudev.Devices.from_device_numberand assertget_usb_inforeturnsNone. - Simulate a non-numeric
device.attributes.get("size")that causesint(size_attr)to raiseValueError, and assert it also returnsNone.
These will ensure the dedicated PermissionError branch and malformed size handling remain covered and regressions are caught.
| monkeypatch.setattr(find_usb_module.pyudev, "Context", lambda: mock_context) | ||
| monkeypatch.setattr(find_usb_module.pyudev.Devices, "from_device_number", lambda ctx, type, num: mock_device) | ||
|
|
||
| result = find_usb_module.find_usb() |
There was a problem hiding this comment.
suggestion (testing): Add a test for pyudev returning an empty/None label (non-exceptional) to exercise the fallback path.
The new test covers the exception path from pyudev.Devices.from_device_number. Please also add a test where from_device_number succeeds but device.get("ID_FS_LABEL") returns None or "", so we verify the fallback to the directory basename for non-exceptional pyudev failures as well.
Comment on lines
+24
to
+33
| def _mock_os_stat_and_pyudev(monkeypatch, size="1000000000", label="MY_USB"): | ||
| # Mock os.stat safely | ||
| os_stat_orig = gui_module.os.stat | ||
|
|
||
| return impl | ||
| def mock_os_stat(p): | ||
| if str(p).startswith("/dev/"): | ||
| m = MagicMock() | ||
| m.st_rdev = 1234 | ||
| return m | ||
| return os_stat_orig(p) |
There was a problem hiding this comment.
suggestion: Consider reusing _mock_os_stat_and_pyudev in other tests to reduce duplication and keep behaviour aligned.
Similar pyudev mocking appears in tests/test_get_usb_info.py and tests/test_find_usb.py. Reusing this helper there (or moving it to a shared test helper/fixture) would keep all pyudev-dependent tests aligned on how os.stat.st_rdev and device.attributes['size'] are interpreted, and make future changes to the pyudev integration easier to apply consistently. This is a non-blocking suggestion for test maintainability.
Suggested implementation:
def _fake_partitions(mount, device):
return lambda all=False: [SimpleNamespace(mountpoint=mount, device=device)]
def _mock_os_stat_and_pyudev(monkeypatch, size="1000000000", label="MY_USB"):
# Mock os.stat safely
os_stat_orig = gui_module.os.stat
def mock_os_stat(p):
if str(p).startswith("/dev/"):
m = MagicMock()
m.st_rdev = 1234
return m
return os_stat_orig(p)
monkeypatch.setattr(gui_module.os, "stat", mock_os_stat)
# Mock pyudev
mock_context = MagicMock()
mock_device = MagicMock()
# size in 512-byte sectors
mock_device.attributes = {"size": str(int(size) // 512)}
mock_device.get.return_value = label
monkeypatch.setattr(gui_module.pyudev, "Context", lambda: mock_context)
monkeypatch.setattr(gui_module.pyudev.Devices, "from_device_number", lambda ctx, type, num: mock_device)To fully implement the reuse suggestion across the test suite, you should also:
- Ensure
gui_moduleandMagicMockare imported at the top oftests/test_get_usb_info_and_detect_windows_fixes.py(if not already present):from unittest.mock import MagicMockimport gui_module(or whatever module contains the GUI code under test).
- In
tests/test_get_usb_info.pyandtests/test_find_usb.py, remove any duplicated pyudev/os.stat mocking logic and call_mock_os_stat_and_pyudev(monkeypatch, ...)from this module instead, e.g.:from tests.test_get_usb_info_and_detect_windows_fixes import _mock_os_stat_and_pyudev- Then use
_mock_os_stat_and_pyudev(monkeypatch, size="...", label="...")in those tests.
- If you prefer not to import from a test module, you can instead move
_mock_os_stat_and_pyudevinto a shared helper (e.g.tests/usb_test_helpers.pyortests/conftest.py) and update:- This file to import and use that shared helper.
tests/test_get_usb_info.pyandtests/test_find_usb.pyto import and use the shared helper as well.
SourceryAI
reviewed
May 7, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The os.stat/pyudev mocking pattern is repeated across several tests; consider extracting a small helper or fixture (similar to
_mock_os_stat_and_pyudev) to avoid duplication and make the tests easier to maintain. - In
get_usb_infoandfind_usb, the blanketexcept Exceptionaround the pyudev logic makes it harder to reason about failures; narrowing this to expected error types (e.g.OSError/pyudev errors) would keep logging and behaviour more predictable while still handling failure cases. - When reading the
sizeattribute from the udev device, it may be worth defensively validating that the value is a non-negative integer beforeint()conversion to avoid unexpectedValueErroror weird values from unusual udev setups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The os.stat/pyudev mocking pattern is repeated across several tests; consider extracting a small helper or fixture (similar to `_mock_os_stat_and_pyudev`) to avoid duplication and make the tests easier to maintain.
- In `get_usb_info` and `find_usb`, the blanket `except Exception` around the pyudev logic makes it harder to reason about failures; narrowing this to expected error types (e.g. `OSError`/pyudev errors) would keep logging and behaviour more predictable while still handling failure cases.
- When reading the `size` attribute from the udev device, it may be worth defensively validating that the value is a non-negative integer before `int()` conversion to avoid unexpected `ValueError` or weird values from unusual udev setups.
## Individual Comments
### Comment 1
<location path=".github/workflows/python-compatibility.yml" line_range="11-12" />
<code_context>
jobs:
test:
- name: "Test — Python ${{ matrix.python-version }}"
+ name: "Run Tests (Python 3.10+)"
runs-on: ubuntu-latest
strategy:
</code_context>
<issue_to_address>
**suggestion:** Job name no longer differentiates between Python versions in the matrix
The static name makes all matrix jobs look identical in the Actions UI, which makes it harder to see which Python version failed. Please reintroduce the version in the job name (e.g., `Run Tests (Python ${{ matrix.python-version }})`) so each matrix entry is distinguishable.
```suggestion
test:
name: "Run Tests (Python ${{ matrix.python-version }})"
```
</issue_to_address>Hi @Radiump123! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
11
to
-12
| test: | ||
| name: "Test — Python ${{ matrix.python-version }}" |
There was a problem hiding this comment.
suggestion: Job name no longer differentiates between Python versions in the matrix
The static name makes all matrix jobs look identical in the Actions UI, which makes it harder to see which Python version failed. Please reintroduce the version in the job name (e.g., Run Tests (Python ${{ matrix.python-version }})) so each matrix entry is distinguishable.
Suggested change
| test: | |
| name: "Test — Python ${{ matrix.python-version }}" | |
| test: | |
| name: "Run Tests (Python ${{ matrix.python-version }})" |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
get_usb_info, parsingdevice.attributes.get("size")viaint(size_attr)assumes a non-empty numeric string; consider mirroring the previouslsblkpath by validating the value (e.g.isdigit()or catchingValueError) and logging a warning before defaulting to 0 to avoid unexpected exceptions on malformed udev attributes. - In
find_usb, the broadexcept Exceptionaround the pyudev lookup silently swallows all failures; narrowing this to expected error types or at least logging at debug level would make diagnosing USB detection issues easier without changing behavior for callers. - The test suite now repeats similar
os.statand pyudev mocking patterns in multiple files; consider extracting a small helper or fixture (e.g._mock_os_stat_and_pyudev) to reduce duplication and keep the mocking behavior consistent, and clean up stray/unneeded imports or informal comments in tests (like theIdk why I don't like thatremark).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `get_usb_info`, parsing `device.attributes.get("size")` via `int(size_attr)` assumes a non-empty numeric string; consider mirroring the previous `lsblk` path by validating the value (e.g. `isdigit()` or catching `ValueError`) and logging a warning before defaulting to 0 to avoid unexpected exceptions on malformed udev attributes.
- In `find_usb`, the broad `except Exception` around the pyudev lookup silently swallows all failures; narrowing this to expected error types or at least logging at debug level would make diagnosing USB detection issues easier without changing behavior for callers.
- The test suite now repeats similar `os.stat` and pyudev mocking patterns in multiple files; consider extracting a small helper or fixture (e.g. `_mock_os_stat_and_pyudev`) to reduce duplication and keep the mocking behavior consistent, and clean up stray/unneeded imports or informal comments in tests (like the `Idk why I don't like that` remark).
## Individual Comments
### Comment 1
<location path="src/lufus/drives/get_usb_info.py" line_range="33-37" />
<code_context>
- if not size_output.isdigit():
- log.warning("Could not parse device size: %r", size_output)
+ # Size in bytes: udev attributes 'size' is in 512-byte sectors
+ size_attr = device.attributes.get("size")
+ usb_size = int(size_attr) * 512 if size_attr else 0
+
+ label = device.get("ID_FS_LABEL")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handle non-numeric or malformed `size` attributes more defensively.
With this change, a non-numeric or malformed `size` attribute will cause `int(size_attr)` to raise `ValueError`, be caught by the generic `Exception` handler, and make the function return `None`, whereas previously we just logged a warning and used size 0. To preserve the prior resilience, consider checking `size_attr.isdigit()` (after decoding if needed) or catching `ValueError` around `int(size_attr)`, logging a warning, and defaulting the size to 0 instead of failing the whole call.
```suggestion
# Size in bytes: udev attributes 'size' is in 512-byte sectors
size_attr = device.attributes.get("size")
usb_size = 0
if size_attr:
try:
usb_size = int(size_attr) * 512
except (ValueError, TypeError):
log.warning("Could not parse device size attribute: %r", size_attr)
label = device.get("ID_FS_LABEL")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
get_usb_infoandfind_usb, the new pyudev-based paths now rely on a broadexcept Exceptionwhere previouslyTimeoutExpired/CalledProcessErrorwere handled explicitly; consider restoring more targeted exception handling (and logging) around the pyudev lookup so that timeouts or permission issues are not silently swallowed. - The pyudev/os.stat mocking logic is duplicated across multiple tests (e.g., in
test_get_usb_info,test_get_usb_info_and_detect_windows_fixes, andtest_find_usb); you could extract a shared helper to keep the tests shorter and reduce the risk of inconsistencies between them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `get_usb_info` and `find_usb`, the new pyudev-based paths now rely on a broad `except Exception` where previously `TimeoutExpired`/`CalledProcessError` were handled explicitly; consider restoring more targeted exception handling (and logging) around the pyudev lookup so that timeouts or permission issues are not silently swallowed.
- The pyudev/os.stat mocking logic is duplicated across multiple tests (e.g., in `test_get_usb_info`, `test_get_usb_info_and_detect_windows_fixes`, and `test_find_usb`); you could extract a shared helper to keep the tests shorter and reduce the risk of inconsistencies between them.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is up to date with 'origin/less-deps-feature-win-tweaks'. Changes to be committed: modified: .github/workflows/ci.yml modified: .github/workflows/python-compatibility.yml modified: pyproject.toml modified: scripts/build-appimage.sh modified: src/lufus/drives/find_usb.py modified: src/lufus/drives/formatting.py modified: src/lufus/drives/get_usb_info.py modified: src/lufus/gui/dialogs.py modified: src/lufus/gui/gui.py modified: src/lufus/gui/i18n.py modified: src/lufus/gui/languages/Deutsch.csv modified: src/lufus/gui/languages/English.csv modified: "src/lufus/gui/languages/Espa\303\261ol.csv" modified: "src/lufus/gui/languages/Fran\303\247ais.csv" modified: "src/lufus/gui/languages/Portugue\314\202s Brasileiro.csv" modified: src/lufus/gui/languages/Svenska.csv modified: "src/lufus/gui/languages/\320\240\321\203\321\201\321\201\320\272\320\270\320\271.csv" modified: "src/lufus/gui/languages/\321\203\320\272\321\200\320\260\321\227\320\275\321\201\321\214\320\272\320\260.csv" modified: "src/lufus/gui/languages/\330\271\330\261\330\250\331\212.csv" modified: "src/lufus/gui/languages/\340\246\254\340\246\276\340\246\202\340\246\262\340\246\276.csv" modified: src/lufus/gui/workers.py modified: src/lufus/state.py modified: src/lufus/utils.py modified: src/lufus/writing/flash_usb.py modified: src/lufus/writing/install_ventoy.py modified: src/lufus/writing/windows/flash.py modified: src/lufus/writing/windows/tweaks.py modified: tests/test_find_usb.py modified: tests/test_flash_usb_and_find_usb_fixes.py modified: tests/test_get_usb_info.py modified: tests/test_get_usb_info_and_detect_windows_fixes.py Untracked files: Archive.tar.gz TODO root-log.txt src/lufus/gui/languages/U0420U0443U0441U0441U043aU0438U0439.csv src/lufus/gui/languages/U0443U043aU0440U0430U0457U043dU0441U044cU043aU0430.csv src/lufus/gui/languages/U0639U0631U0628U064a.csv src/lufus/gui/languages/U09acU09beU0982U09b2U09be.csv src/lufus/gui/languages/EspaU00f1ol.csv src/lufus/gui/languages/FranU00e7ais.csv src/lufus/gui/languages/PortugueU0302s Brasileiro.csv user-log.txt
with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is ahead of 'origin/less-deps-feature-win-tweaks' by 1 commit. (use "git push" to publish your local commits) Changes to be committed: modified: src/lufus/gui/gui.py new file: src/lufus/gui/languages/U0420U0443U0441U0441U043aU0438U0439.csv new file: src/lufus/gui/languages/U0443U043aU0440U0430U0457U043dU0441U044cU043aU0430.csv new file: src/lufus/gui/languages/U0639U0631U0628U064a.csv new file: src/lufus/gui/languages/U09acU09beU0982U09b2U09be.csv new file: src/lufus/gui/languages/EspaU00f1ol.csv new file: src/lufus/gui/languages/FranU00e7ais.csv new file: src/lufus/gui/languages/PortugueU0302s Brasileiro.csv modified: src/lufus/writing/flash_usb.py
Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is ahead of 'origin/less-deps-feature-win-tweaks' by 2 commits. (use "git push" to publish your local commits) Changes to be committed: deleted: src/lufus/gui/languages/U0420U0443U0441U0441U043aU0438U0439.csv deleted: src/lufus/gui/languages/U0443U043aU0440U0430U0457U043dU0441U044cU043aU0430.csv deleted: src/lufus/gui/languages/U0639U0631U0628U064a.csv deleted: src/lufus/gui/languages/U09acU09beU0982U09b2U09be.csv deleted: src/lufus/gui/languages/EspaU00f1ol.csv deleted: src/lufus/gui/languages/FranU00e7ais.csv deleted: src/lufus/gui/languages/PortugueU0302s Brasileiro.csv
with '' will be ignored, and an empty message aborts the commit. On branch less-deps-feature-win-tweaks Your branch is ahead of 'origin/less-deps-feature-win-tweaks' by 3 commits. (use "git push" to publish your local commits) Changes to be committed: modified: src/lufus/gui/gui.py
| # injection risk here. | ||
| try: | ||
| process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL) | ||
| process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL, env=env) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 security issues, 5 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
drives/formatting.remount, when adriveargument is passed themountvariable is never set anymore (the lookup was removed), so the subsequentif not drive or not mountwill always hit themountbranch and prevent remounting; consider restoring the mount-point lookup for the supplied drive. - The change in
utils.elevate_privilegesto drop validation ofstate.themebefore exportingLUFUS_THEMEreopens the possibility of injecting unexpected values into the elevated environment; it would be safer to keep the filename/path validation or similar sanitisation. - The new
load_translationsimplementation ingui/i18n.pyalways usesutf-8instead ofutf-8-sigand assumeskey/valuecolumns are present, which means CSVs with a BOM or malformed/missing columns will now raise instead of being skipped; consider reintroducing BOM-tolerant reading and defensive checks around row keys.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `drives/formatting.remount`, when a `drive` argument is passed the `mount` variable is never set anymore (the lookup was removed), so the subsequent `if not drive or not mount` will always hit the `mount` branch and prevent remounting; consider restoring the mount-point lookup for the supplied drive.
- The change in `utils.elevate_privileges` to drop validation of `state.theme` before exporting `LUFUS_THEME` reopens the possibility of injecting unexpected values into the elevated environment; it would be safer to keep the filename/path validation or similar sanitisation.
- The new `load_translations` implementation in `gui/i18n.py` always uses `utf-8` instead of `utf-8-sig` and assumes `key`/`value` columns are present, which means CSVs with a BOM or malformed/missing columns will now raise instead of being skipped; consider reintroducing BOM-tolerant reading and defensive checks around row keys.
## Individual Comments
### Comment 1
<location path="src/lufus/gui/i18n.py" line_range="16-15" />
<code_context>
if lang_file.exists():
- # utf-8-sig strips BOM that some editors insert before the first byte.
- # Graceful row handling: skip malformed rows (no key column, empty key).
- with open(lang_file, encoding="utf-8-sig", newline="") as f:
+ # read translations from csv :3
+ with open(lang_file, encoding="utf-8", newline="") as f:
for row in csv.DictReader(f):
- key = row.get("key", "").strip()
</code_context>
<issue_to_address>
**issue:** CSV parsing no longer tolerates BOMs or malformed rows and can now raise KeyError
The previous version used `utf-8-sig` and `row.get()` to tolerate BOMs and malformed rows; this change removes both safeguards. With plain `utf-8` and direct `row["key"]`/`row["value"]` indexing:
1. CSVs with a UTF-8 BOM will expose `"\ufeffkey"` instead of `"key"`, causing `KeyError` and failing translation loading.
2. Rows missing `key`/`value` now raise `KeyError` instead of being skipped.
If this robustness is still required, please either restore `utf-8-sig` and `get()`-based access, or add explicit checks around `row.get("key")`/`row.get("value")` before use.
</issue_to_address>
### Comment 2
<location path="src/lufus/utils.py" line_range="15" />
<code_context>
# if the app was able to find them before elevation.
env = os.environ.copy()
if state.theme:
- # Validate theme is a safe filename/path: no path separators, no shell metacharacters,
- # and must resolve inside the config directory to prevent path traversal.
</code_context>
<issue_to_address>
**🚨 issue (security):** Removing validation of LUFUS_THEME reintroduces potential path / injection issues in the elevated environment
`state.theme` used to be constrained to a safe filename (no separators/`..`/restricted charset) before being exported as `LUFUS_THEME`, protecting the elevated process from hostile or malformed values. Now it’s exported as-is, so any user- or config-controlled value could be interpreted as a path, enabling traversal or pointing the elevated process at arbitrary locations. Even if you’re simplifying, please reintroduce a minimal check (e.g., `os.path.basename(theme) == theme` and reject `..`) to keep the elevated environment safe.
</issue_to_address>
### Comment 3
<location path="src/lufus/writing/flash_usb.py" line_range="123-132" />
<code_context>
+ buf = b""
</code_context>
<issue_to_address>
**suggestion:** dd progress parsing may spin on EOF and can be noisy for non-progress stderr lines
Two small behaviors to consider tightening:
1. If `dd` emits a final stderr fragment without a trailing newline/carriage return, it will remain in `buf` and never be logged. It would be safer to flush any remaining `buf` after the read loop.
2. All non-progress stderr is logged at `warning` level. Some `dd` variants print benign informational lines, which could now spam logs and alarm users. Consider logging those at `info` or filtering obviously harmless messages.
These aren’t critical, but they would make progress reporting and logging more predictable.
Suggested implementation:
```python
buf = b""
while True:
chunk = process.stderr.read1(65536)
if not chunk:
# Flush any remaining partial stderr fragment so it is not lost
if buf:
# Decode conservatively and avoid logging a purely empty line
decoded = buf.decode(errors="replace").rstrip("\r\n")
if decoded:
# Log at info to avoid alarming users with benign dd output
logger.info("dd stderr (final fragment): %s", decoded)
break
buf += chunk
```
1. Ensure there is a `logger` available in this module:
- If the file already defines or imports a logger (for example, `logger = logging.getLogger(__name__)`), reuse it instead of introducing a new one.
- If not, add at the top of the file:
```python
import logging
logger = logging.getLogger(__name__)
```
2. In the part of this file where non-progress `dd` stderr lines are currently logged (likely something like `logger.warning("dd stderr: %s", line)`), adjust the behavior:
- Either:
- Change the level to `info`: `logger.info("dd stderr: %s", line)`, **or**
- Add simple filtering, e.g.:
```python
harmless_phrases = ("records in", "records out", "bytes copied")
if any(p in line for p in harmless_phrases):
logger.info("dd stderr: %s", line)
else:
logger.warning("dd stderr: %s", line)
```
- This will keep benign informational `dd` output from spamming warnings while preserving higher severity for unexpected stderr content.
</issue_to_address>
### Comment 4
<location path="scripts/build-appimage.sh" line_range="7-12" />
<code_context>
-# echo "requirements-system.txt not found!"
-# exit 1
-# fi
+echo "------------ Installing system libraries ------------"
+apt-get update && apt-get upgrade -y
+INSTALLER="apt-get install -y"
+if [[ -f requirements-system.txt ]]; then
+ $INSTALLER $(cat requirements-system.txt) >> appimage-setup.log
+ echo "System libraries installed."
+else
+ echo "requirements-system.txt not found!"
</code_context>
<issue_to_address>
**suggestion (performance):** Running `apt-get update && apt-get upgrade -y` in the AppImage build script can have unintended side effects in CI/build environments
Running `apt-get update && apt-get upgrade -y` in the build script makes the build environment mutable and less predictable:
- Increases build time and network usage.
- May update unrelated system packages on CI runners, leading to nondeterministic builds.
- Can conflict with CI provider recommendations, where full upgrades are discouraged or blocked.
Prefer installing only the required packages via `apt-get install -y` (assuming the base image is maintained) or managing system packages in the CI/YAML configuration instead of inside the build script.
Suggested implementation:
```
echo "------------ Installing system libraries ------------"
INSTALLER="apt-get install -y"
if [[ -f requirements-system.txt ]]; then
$INSTALLER $(cat requirements-system.txt) >> appimage-setup.log
echo "System libraries installed."
else
echo "requirements-system.txt not found!"
exit 1
fi
```
Depending on your CI setup, you may want to:
1. Move `apt-get update` (and any controlled upgrade policy) into the CI/YAML configuration so the base image is explicitly managed there.
2. Ensure the base image used in CI is regularly updated so `apt-get install` can resolve the packages listed in `requirements-system.txt` without needing an inline `apt-get update` here.
</issue_to_address>
### Comment 5
<location path="tests/test_flash_usb_and_find_usb_fixes.py" line_range="384-393" />
<code_context>
+ def test_progress_cb_receives_milestones_and_scaled_dd_progress(self, monkeypatch):
</code_context>
<issue_to_address>
**issue (testing):** Progress callback tests still assume a two-argument progress_cb and conflate progress and status, but flash_usb now calls progress_cb(progress) and uses status_cb for messages.
In the current tests, `progress_cb(progress, status)` will raise a `TypeError` because `flash_usb.flash_usb` now calls `progress_cb(progress)` and sends messages via a separate `status_cb`. As a result, `status_messages` is never populated.
Please update the tests to:
- Use a single-argument `progress_cb(progress)` and capture messages with a separate `status_cb(message)`.
- Pass both callbacks to `flash_usb_module.flash_usb(..., progress_cb=progress_cb, status_cb=status_cb)`.
- Update assertions so they no longer expect status text from `progress_cb`.
This will align the tests with the new callback API and avoid spurious failures from the wrong signature.
</issue_to_address>
### Comment 6
<location path="src/lufus/drives/formatting.py" line_range="358-361" />
<code_context>
subprocess.run(
[_find_tool("parted"), "-s", raw_device, "mkpart", "primary", "1MiB", "100%"],
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location path="src/lufus/drives/formatting.py" line_range="365-368" />
<code_context>
subprocess.run(
[_find_tool("parted"), "-s", raw_device, "mkpart", "primary", "1MiB", "100%"],
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location path="src/lufus/writing/flash_usb.py" line_range="105" />
<code_context>
process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL, env=env)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # injection risk here. | ||
| try: | ||
| process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL) | ||
| process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL, env=env) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
… lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche less-deps-feature-win-tweaks Votre branche est à jour avec 'origin/less-deps-feature-win-tweaks'. Modifications qui seront validées : modifié : .github/workflows/ci.yml
Owner
Author
|
Gotta fix these |
… lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche less-deps-feature-win-tweaks Votre branche est à jour avec 'origin/less-deps-feature-win-tweaks'. Modifications qui seront validées : modifié : scripts/build-appimage.sh modifié : src/lufus/drives/formatting.py modifié : src/lufus/gui/i18n.py modifié : src/lufus/utils.py modifié : src/lufus/writing/flash_usb.py modifié : tests/test_flash_usb_and_find_usb_fixes.py
Veuillez saisir le message de validation pour vos modifications. Les lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche less-deps-feature-win-tweaks Votre branche est en avance sur 'origin/less-deps-feature-win-tweaks' de 1 commit. (utilisez "git push" pour publier vos commits locaux) Modifications qui seront validées : modifié : src/lufus/gui/gui.py modifié : src/lufus/writing/windows/detect.py
… lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche less-deps-feature-win-tweaks Votre branche est à jour avec 'origin/less-deps-feature-win-tweaks'. Modifications qui seront validées : modifié : src/lufus/drives/formatting.py modifié : tests/test_flash_usb_and_find_usb_fixes.py
… lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche less-deps-feature-win-tweaks Votre branche est à jour avec 'origin/less-deps-feature-win-tweaks'. Modifications qui seront validées : modifié : tests/test_flash_usb_and_find_usb_fixes.py
… lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche less-deps-feature-win-tweaks Votre branche est à jour avec 'origin/less-deps-feature-win-tweaks'. Modifications qui seront validées : modifié : tests/test_linux_iso_detection.py
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 security issues, 5 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
writing/windows/flash.py, replacingshutil.whichwithsubprocess.run(['which', ...])introduces a runtime dependency on the externalwhichbinary and repeated process spawning; consider reverting toshutil.whichto keep tool detection lightweight and more portable. - In
tests/test_find_usb.py,_ORIG_STATis assigned from_real_os.stat, but_real_osis never defined or imported, so the helper will fail before tests run; captureos.statdirectly (e.g._ORIG_STAT = os.stat) or import the intended alias. - The Windows tweaks helpers in
writing/windows/tweaks.pynow call baremkdir/rmviasubprocess.runwithout sudo or the existingrun_cmdabstraction, which may fail depending on privileges and diverges from the new centralised privileged-command handling; consider routing these through the same sudo/run_cmd path for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `writing/windows/flash.py`, replacing `shutil.which` with `subprocess.run(['which', ...])` introduces a runtime dependency on the external `which` binary and repeated process spawning; consider reverting to `shutil.which` to keep tool detection lightweight and more portable.
- In `tests/test_find_usb.py`, `_ORIG_STAT` is assigned from `_real_os.stat`, but `_real_os` is never defined or imported, so the helper will fail before tests run; capture `os.stat` directly (e.g. `_ORIG_STAT = os.stat`) or import the intended alias.
- The Windows tweaks helpers in `writing/windows/tweaks.py` now call bare `mkdir`/`rm` via `subprocess.run` without sudo or the existing `run_cmd` abstraction, which may fail depending on privileges and diverges from the new centralised privileged-command handling; consider routing these through the same sudo/run_cmd path for consistency.
## Individual Comments
### Comment 1
<location path="src/lufus/drives/formatting.py" line_range="104-111" />
<code_context>
# mountain
def remount(drive: str = None) -> bool:
- mount = None
if not drive:
mount, drive, _ = _get_mount_and_drive()
else:
- # drive was supplied by caller; resolve mount point from current state
- _, _, mount_dict = _get_mount_and_drive()
- # find the mount point whose device node matches the given drive
- mount = next((mp for mp, _label in mount_dict.items()), None)
+ # Look up mount point for the specified drive
+ from lufus.drives.find_usb import find_usb
+
+ mount_dict = find_usb()
+ mount = mount_dict.get(drive)
if not drive:
</code_context>
<issue_to_address>
**issue (bug_risk):** The remount logic misuses `find_usb()` and will not find the mount point for a given device node.
`_get_mount_and_drive()` used to give you both the selected `(mount, drive)` and a mapping keyed appropriately. Now `find_usb()` returns `{mount_path: label}`, but you’re calling `mount = mount_dict.get(drive)` where `drive` is `'/dev/sdX...'`, so this lookup will almost always be `None`. As a result, `remount(drive='/dev/...')` will incorrectly hit the "No mount point found" path even when the device is mounted. You should either invert the mapping from `find_usb()` so you can look up by device node, or restore a helper that returns `(mount, drive, mount_dict)` with keys that match `drive` values.
</issue_to_address>
### Comment 2
<location path="src/lufus/gui/gui.py" line_range="1378" />
<code_context>
+ self.log_message(f"Failed to restore terminal: {e}")
# reset ui state :D
- self.progress_bar.setRange(0, 100) # exit indeterminate mode
self.progress_bar.setValue(0)
self.progress_bar.setFormat("")
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing the progress-bar range reset can leave the bar stuck in indeterminate mode after cancel.
Since `cancel_process` no longer calls `self.progress_bar.setRange(0, 100)`, a bar that was set to an indeterminate range `(0, 0)` will stay visually “busy” even after cancel, because updating only `value` does not exit indeterminate mode. Please reintroduce the range reset or add a conditional reset when leaving indeterminate mode.
</issue_to_address>
### Comment 3
<location path="src/lufus/writing/windows/flash.py" line_range="274" />
<code_context>
if efi_src:
efi_items = os.listdir(efi_src)
_status(f"Found EFI/ with {len(efi_items)} items: {efi_items}")
- for item in efi_items:
- src_path = os.path.join(efi_src, item)
- dst_path = os.path.join(mount_efi, "EFI", item)
- if os.path.isdir(src_path):
- shutil.copytree(src_path, dst_path, dirs_exist_ok=True)
- else:
- os.makedirs(os.path.dirname(dst_path), exist_ok=True)
- shutil.copy2(src_path, dst_path)
+ run_cmd(["sudo", "cp", "-r"] + [os.path.join(efi_src, i) for i in efi_items] + [mount_efi])
_status("Copied EFI/ tree to EFI partition")
else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Copying EFI and boot trees directly into `mount_efi` changes the on-disk layout compared to the previous `mount_efi/EFI` and `mount_efi/boot` locations.
The previous logic explicitly copied into `mount_efi/EFI` and `mount_efi/boot`, matching firmware’s expected layout. The new `cp -r <EFI/*> mount_efi` and `cp -r <boot/*> mount_efi` calls instead flatten the directory structure so entries like `EFI/Microsoft/...` become `<mount_efi>/Microsoft/...`. This can break boot and violate `_fix_efi_bootloader`’s assumption that `mount_efi/EFI` exists. Please update the copy commands to preserve the original `EFI/` and `boot/` subdirectories (e.g., by using `mount_efi/EFI` and `mount_efi/boot` as destinations).
</issue_to_address>
### Comment 4
<location path="src/lufus/writing/windows/tweaks.py" line_range="59-62" />
<code_context>
cmd_string = "\n".join(commands) + "\n"
log.info("win_hardware_bypass: injecting registry keys into boot.wim at %s...", mount)
try:
- os.makedirs("/media/tempwinmnt", exist_ok=True)
+ subprocess.run(["mkdir", "/media/tempwinmnt"], check=True)
subprocess.run(["wimmountrw", f"{mount}/sources/boot.wim", "2", "/media/tempwinmnt"], check=True)
</code_context>
<issue_to_address>
**issue (bug_risk):** Switching to `mkdir`/`rm -rf` subprocess calls changes robustness and error handling for temporary mount directories.
Replacing `os.makedirs(..., exist_ok=True)` / `shutil.rmtree(..., ignore_errors=True)` with `subprocess.run([
</issue_to_address>
### Comment 5
<location path="src/lufus/writing/windows/flash.py" line_range="157-158" />
<code_context>
def _find_ntfs_tool(status_cb=None) -> str | None:
"""Find mkfs.ntfs/mkntfs, installing ntfs-3g if needed. Returns command name or None."""
for candidate in ["mkfs.ntfs", "mkntfs"]:
- if shutil.which(candidate):
+ if subprocess.run(["which", candidate], capture_output=True).returncode == 0:
return candidate
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Replacing `shutil.which` with an external `which` command introduces a new dependency and potential portability issues.
Both `_find_ntfs_tool` and `_ensure_wimlib` now rely on an external `which` via `subprocess.run([...])` instead of `shutil.which`, assuming:
- A `which` binary exists on `PATH` (not guaranteed everywhere).
- Its behavior matches expectations, whereas `shutil.which` is already cross‑platform and handles PATH resolution.
Unless there is a strong reason to avoid `shutil.which`, please revert to it, or at least use it as a fallback when the external `which` call fails to avoid portability/regression issues.
</issue_to_address>
### Comment 6
<location path="src/lufus/drives/formatting.py" line_range="364-367" />
<code_context>
subprocess.run(
[_find_tool("parted"), "-s", raw_device, "mkpart", "primary", "1MiB", "100%"],
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location path="src/lufus/drives/formatting.py" line_range="371-374" />
<code_context>
subprocess.run(
[_find_tool("parted"), "-s", raw_device, "mkpart", "primary", "1MiB", "100%"],
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location path="src/lufus/writing/flash_usb.py" line_range="105" />
<code_context>
process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL, env=env)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
-59
to
62
| os.makedirs("/media/tempwinmnt", exist_ok=True) | ||
| subprocess.run(["mkdir", "/media/tempwinmnt"], check=True) | ||
| subprocess.run(["wimmountrw", f"{mount}/sources/boot.wim", "2", "/media/tempwinmnt"], check=True) | ||
| subprocess.run( | ||
| ["chntpw", "e", "/media/tempwinmnt/Windows/System32/config/SYSTEM"], |
There was a problem hiding this comment.
issue (bug_risk): Switching to mkdir/rm -rf subprocess calls changes robustness and error handling for temporary mount directories.
Replacing os.makedirs(..., exist_ok=True) / shutil.rmtree(..., ignore_errors=True) with `subprocess.run([
| # injection risk here. | ||
| try: | ||
| process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL) | ||
| process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL, env=env) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
… lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche less-deps-feature-win-tweaks Votre branche est à jour avec 'origin/less-deps-feature-win-tweaks'. Modifications qui seront validées : modifié : src/lufus/drives/formatting.py modifié : src/lufus/gui/gui.py modifié : src/lufus/utils.py modifié : src/lufus/writing/windows/flash.py modifié : src/lufus/writing/windows/tweaks.py
Veuillez saisir le message de validation pour vos modifications. Les lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche less-deps-feature-win-tweaks Votre branche est à jour avec 'origin/less-deps-feature-win-tweaks'. Modifications qui seront validées : modifié : src/lufus/writing/windows/flash.py
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 security issues, 1 other issue, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
cancel_process,_procs_with_device_mountedwill unconditionally sendSIGKILLto any PID with the device mounted; it might be safer to sendSIGTERMfirst, optionally skip your own process, and make it clear in the log that other user processes may be force-killed. - The Windows flashing helpers now use
run_cmdwith hard-codedsudofor operations likemkdir,cp, andsync; if Lufus is already running underpkexecor another elevation mechanism this can fail—consider centralising privilege handling so these calls run with the correct effective user instead of assumingsudo. - In the Windows path of
FlashWorker.run, any non-ISOflash_modeis now treated assuccess = Falsewithout a clear user-facing explanation; it may be clearer to either disable unsupported Windows/DD combinations in the UI or emit a specific status/error message about unsupported mode selection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `cancel_process`, `_procs_with_device_mounted` will unconditionally send `SIGKILL` to any PID with the device mounted; it might be safer to send `SIGTERM` first, optionally skip your own process, and make it clear in the log that other user processes may be force-killed.
- The Windows flashing helpers now use `run_cmd` with hard-coded `sudo` for operations like `mkdir`, `cp`, and `sync`; if Lufus is already running under `pkexec` or another elevation mechanism this can fail—consider centralising privilege handling so these calls run with the correct effective user instead of assuming `sudo`.
- In the Windows path of `FlashWorker.run`, any non-ISO `flash_mode` is now treated as `success = False` without a clear user-facing explanation; it may be clearer to either disable unsupported Windows/DD combinations in the UI or emit a specific status/error message about unsupported mode selection.
## Individual Comments
### Comment 1
<location path="scripts/build-appimage.sh" line_range="7-15" />
<code_context>
-# echo "requirements-system.txt not found!"
-# exit 1
-# fi
+echo "------------ Installing system libraries ------------"
+INSTALLER="apt-get install -y"
+if [[ -f requirements-system.txt ]]; then
+ $INSTALLER $(cat requirements-system.txt) >> appimage-setup.log
+ echo "System libraries installed."
+else
+ echo "requirements-system.txt not found!"
+ exit 1
+fi
</code_context>
<issue_to_address>
**suggestion (bug_risk):** System dependency install runs `apt-get install` without a preceding `apt-get update`, which can fail on fresh runners.
On fresh CI runners or minimal images, `apt-get install` can fail with “package not found” or stale metadata if `apt-get update` hasn’t been run. Please ensure we run `apt-get update` (and optionally set `DEBIAN_FRONTEND=noninteractive`) before invoking `$INSTALLER` so the AppImage build remains reliable.
```suggestion
echo "------------ Installing system libraries ------------"
export DEBIAN_FRONTEND=noninteractive
INSTALLER="apt-get install -y"
if [[ -f requirements-system.txt ]]; then
echo "Updating apt package index..."
apt-get update >> appimage-setup.log
echo "Installing system libraries from requirements-system.txt..."
$INSTALLER $(cat requirements-system.txt) >> appimage-setup.log
echo "System libraries installed."
else
echo "requirements-system.txt not found!"
exit 1
fi
```
</issue_to_address>
### Comment 2
<location path="src/lufus/drives/formatting.py" line_range="368-371" />
<code_context>
subprocess.run(
[_find_tool("parted"), "-s", raw_device, "mkpart", "primary", "1MiB", "100%"],
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 3
<location path="src/lufus/drives/formatting.py" line_range="375-378" />
<code_context>
subprocess.run(
[_find_tool("parted"), "-s", raw_device, "mkpart", "primary", "1MiB", "100%"],
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="src/lufus/writing/flash_usb.py" line_range="105" />
<code_context>
process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL, env=env)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # injection risk here. | ||
| try: | ||
| process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL) | ||
| process = subprocess.Popen(dd_args, stderr=subprocess.PIPE, stdout=subprocess.DEVNULL, env=env) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
R-Larocque
force-pushed
the
less-deps-feature-win-tweaks
branch
from
426985f to
9f94364
Compare
Owner
Author
|
If it ain't broke, don't fix it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Replace external system utilities with Python and /proc-/sysfs-based logic, improve flashing progress reporting, and modernize USB/ISO handling while dropping the Windows tweaks GUI workflow.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests: