[1.4] Make host file helpers report failure instead of silently patching

[joaoantoniocardoso]

Follow-up to #4232. Addresses #4235.

The shared host-file helpers used by blueos_startup_update could not report failure. A failed cat looked like an empty file (so the next successful write could replace config.txt / cmdline.txt with only the patch), locate_file never returned None (so the existing is None guards never fired), and save_file ignored a failed upload then returned to a caller that asked for a reboot.

What changed

• load_file raises HostFileError when cat fails. A real empty file (returncode == 0) is still "".
• locate_file returns None when find prints nothing. It still accepts a match when find exits 1 because an earlier candidate was missing (Bullseye: /boot/firmware/config.txt does not exist, /boot/config.txt does).
• save_file raises HostFileError when the scp or the sudo mv onto the destination fails. A failed backup is logged and is not treated as success of the write.
• revert_update_dwc2 and clean_config_pi3 skip when the boot file was not found, matching the other boot patches.
• A failed patch is logged and skipped; patches that already returned True still request a reboot. The previous list comprehension aborted on the first HostFileError, so a startup.json rewrite could lose its restart.
• get_host_os() returns HostOs.Other if the host /etc/os-release cannot be read, which is what an empty read used to do. load_file stays strict. That keeps wifi.can_work from taking down the wifi API when SSH is not up yet.

A patch that raises no longer returns True, so BOOT_LOOP_DETECTOR is not created for work that did not happen.

Test plan

Local: tests in core/libs/commonwealth/commonwealth/utils/tests/test_commands.py and test_general.py. Reverting each of the three helper fixes turns the matching test red. locate_file is also pinned for find exit 1 with a printed path (Bullseye). get_host_os is pinned to Other when load_file raises.

On hardware, the patched commands.py was copied into blueos-core on three boards, exercised, then restored. Boot partitions were remounted read-only for the save test and remounted read-write afterwards. config.txt / cmdline.txt hashes were unchanged.

Board	Image	Locate (real)	Missing locate	Missing load	RO save
Pi 4B Rev 1.5 (192.168.0.177)	Bookworm, /boot/firmware	/boot/firmware/config.txt	shipped '' → patched None	shipped swallowed → HostFileError	HostFileError on mv, file unchanged
Pi 5B Rev 1.0 (192.168.0.124)	Bookworm, /boot/firmware	/boot/firmware/config.txt	shipped '' → patched None	shipped swallowed → HostFileError	HostFileError on mv, file unchanged
Pi 4B Rev 1.2 (192.168.0.88)	Bullseye, /boot	/boot/config.txt	shipped '' → patched None	shipped swallowed → HostFileError	HostFileError on mv, file unchanged

On the read-only boot partition, scp to /tmp still succeeds and sudo mv onto the FAT is what fails. That matches the investigation of #4235: the silent every-other-boot reboot is the ignored-scp path; a remounted-RO boot raises (previously CalledProcessError, now HostFileError) and does not reboot. Either way the patches no longer claim success.

• Pi4 Bookworm
• Pi5 Bookworm
• Pi4 Bullseye

[github-actions]

Automated PR Review

0. Summary

• Verdict: MINOR SUGGESTIONS ✏️

Makes the shared host-file helpers in core/libs/commonwealth/commonwealth/utils/commands.py report failure: load_file raises HostFileError when cat fails, locate_file returns None on an empty find, save_file raises when the scp or sudo mv fails, and upload_file no longer silently ignores a failed mv. get_host_os() swallows the new exception (returning HostOs.Other), revert_update_dwc2 / clean_config_pi3 gain the same is None guard the other boot patches already had, and the patch loop in blueos_startup_update.main() catches per-patch exceptions so one failure doesn't strand subsequent patches. Solid rationale, matching tests, and hardware verification documented in the PR body.

1. Correctness & Implementation Bugs

• 1.1 [minor] core/services/ardupilot_manager/flight_controller_detector/linux/navigator.py:84 — NavigatorPi5.get_serials() still calls load_file("/etc/os-release") directly and does not catch HostFileError. The PR body explicitly notes "load_file stays strict" and updates get_host_os() accordingly, but this second direct caller was overlooked. Under the new behavior, a failed read (e.g. SSH not yet up, RO boot partition edge case) will raise where the pre-PR code silently defaulted to the Bullseye serial mapping, and the exception will propagate out through AutopilotManager.get_serials(). Consider either wrapping this call in try/except HostFileError (fallback to Bullseye or Bookworm as appropriate) or routing it through get_host_os() from commonwealth.utils.general so the safe-default behavior is uniform.

7. Tests

• 7.1 [nit] core/libs/commonwealth/commonwealth/utils/tests/test_commands.py:56 — test_upload_file_returns_mv_failure monkeypatches run_command unconditionally, so the scp path and the mv path both use the same stub. The test relies on the fact that upload_file_with_ssh_key is patched to succeed and the only subsequent run_command call is the sudo mv, which is correct today but fragile if the function ever grows another run_command invocation. A tiny call counter (return _result(0) for the first call, failure for the second) would pin the intent more explicitly.
• 7.2 [nit] There is no test covering the new save_file behavior where the backup cp fails but the upload succeeds (the "log-a-warning, keep going" path). Cheap to add alongside test_save_file_succeeds_when_upload_succeeds and would lock in the choice not to raise for a missing backup.

8. Documentation

• 8.1 [nit] core/tools/blueos_startup_update/blueos_startup_update.py:813 — logger.error(f"Patch {name} failed: {patch_error}") records only the exception message, which is fine for HostFileError but drops the traceback for anything unexpected caught by the broad except Exception. logger.opt(exception=True).error(...) (or logger.exception(...)) would keep the traceback for post-mortem without changing the flow. Matches the existing pattern in this file — take or leave.

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.

[patrickelectric]

Let's wait for 1.4.4 release.