Skip to content

[master] Make host file helpers report failure instead of silently patching - #4320

Merged
patrickelectric merged 6 commits into
bluerobotics:masterfrom
joaoantoniocardoso:ports/master/host-file-helpers
Aug 27, 2026
Merged

[master] Make host file helpers report failure instead of silently patching#4320
patrickelectric merged 6 commits into
bluerobotics:masterfrom
joaoantoniocardoso:ports/master/host-file-helpers

Conversation

@joaoantoniocardoso

@joaoantoniocardoso joaoantoniocardoso commented Aug 26, 2026

Copy link
Copy Markdown
Member

Port of #4242 into master.

Fix #4235.

load_file treated a failed cat as an empty file, locate_file never
returned None, and save_file ignored a failed upload. A read-only boot
partition or a missing file then looked like a successful patch.
…missing

revert_update_dwc2 and clean_config_pi3 walked cmdline.txt / config.txt
without the None guard the other boot patches already use.
load_file now raises, so a down SSH used to crash wifi can_work instead
of selecting NetworkManager the way an empty os-release used to.
On Bullseye, find errors on /boot/firmware/config.txt then still prints
/boot/config.txt. A returncode check would drop that path with green tests.
…r patch fails

A HostFileError from cgroups aborted the listcomp, so a startup.json
rewrite that already returned True never got its reboot.
…eye on read failure

load_file now raises when /etc/os-release cannot be read over SSH. Treat
that like the old empty string so serial enumeration keeps working.
@joaoantoniocardoso
joaoantoniocardoso marked this pull request as ready for review August 27, 2026 02:46
@joaoantoniocardoso
joaoantoniocardoso requested a review from a team August 27, 2026 02:46
@github-actions

Copy link
Copy Markdown

Automated PR Review

0. Summary

  • Verdict: MINOR SUGGESTIONS ✏️

Introduces HostFileError, makes load_file/save_file raise on failure instead of silently returning empty strings, fixes locate_file to actually return None (matching its Optional[str] annotation), updates the three callers (get_host_os, NavigatorPi4.get_serials, blueos_startup_update) to handle the new exception, and wraps the patch-application loop in a per-patch try/except so one bad patch no longer aborts the startup. Adds pytest coverage for the new behavior.

1. Correctness & Implementation Bugs

  • 1.1 [minor] core/tools/blueos_startup_update/blueos_startup_update.py:822 — the new per-patch guard except Exception as patch_error: logger.error(f"Patch {name} failed: {patch_error}") swallows the traceback. Because the caller is now the last line of defense against a broken patch (previously the process would die and be visible in logs), the stack trace is exactly what an operator will want when triaging a boot failure. Prefer logger.exception(f"Patch {name} failed") (loguru supports it) so the traceback is captured.

6. Code Quality & Style

  • 6.1 [nit] core/libs/commonwealth/src/commonwealth/utils/commands.py:151save_file now silently ignores a failed backup (logger.warning only) but raises on a failed write. That is likely intentional (missing source file → nothing to back up), but the asymmetry is worth a one-line comment so future readers do not "fix" it by promoting the warning to a raise.
  • 6.2 [nit] core/tools/blueos_startup_update/blueos_startup_update.py:295-297,320-322 — the new guards use logging.warning(...) (stdlib) while the rest of these functions use logger (loguru). This matches the pre-existing update_cgroups/update_dwc2/update_navigator_overlays guards, so the inconsistency is not introduced here — but it would be a good moment to unify on logger for the file. Optional.

7. Tests

  • 7.1 [nit] core/libs/commonwealth/src/commonwealth/utils/tests/test_commands.py:60test_save_file_raises_when_upload_fails monkeypatches run_command to always return returncode=0, which means the backup step "succeeds" but upload_file is mocked separately. Consider one more case where the backup cp fails (returncode!=0) but upload_file succeeds, to lock in the "log warning, do not raise" behavior added in save_file.
  • 7.2 [nit] No test covers the new try/except wrapper around the patch loop in blueos_startup_update.main. A tiny test that stubs two patches (one raising, one returning True) and asserts the good one still ends up in patches_requiring_restart would guard the isolation property that motivates this PR.

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

@patrickelectric
patrickelectric merged commit 82b1d73 into bluerobotics:master Aug 27, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Boot configuration patches can silently do nothing, or wipe config.txt, and still ask for a reboot

2 participants