Repository navigation
Migrate the platform to Alpine for root volume immutability - #1
Draft
bgshacklett wants to merge 54 commits into
Draft
bgshacklett wants to merge 54 commits into
bgshacklett wants to merge 54 commits into
Conversation
Owner
Author
|
@cubic-dev-ai review this pull request |
@bgshacklett I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
12 issues found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="etc/unattended.sh">
<violation number="1" location="etc/unattended.sh:22">
P2: The `*.disabled` case is unreachable dead code because `find` already filters to only `*.sh` files. If the intent is to skip disabled scripts (e.g., `01-setup.sh.disabled`), the `find` pattern needs to be broadened so those files enter the pipeline in the first place.</violation>
<violation number="2" location="etc/unattended.sh:48">
P2: No guard for an empty `$BOOT`. If the boot media isn't found, `$BOOT` will be empty and `. "$BOOT/unattended.lib.sh"` will attempt to source `/unattended.lib.sh`, failing with a misleading "No such file" error. Add a check after the discovery block.</violation>
</file>
<file name="etc/pre-network.d/10-setup-wifi.qemu.sh">
<violation number="1" location="etc/pre-network.d/10-setup-wifi.qemu.sh:107">
P2: Use `$IP` instead of bare `ip` here for consistency with the rest of the script. The `IP=/sbin/ip` variable was defined to ensure the correct binary is used; this line bypasses that.</violation>
</file>
<file name="extras/wpa_supplicant.conf.example">
<violation number="1" location="extras/wpa_supplicant.conf.example:1">
P2: Country code should be uppercase `US` per ISO 3166-1 alpha-2 convention. Lowercase may not be recognized correctly by the kernel's wireless regulatory domain subsystem on some platforms.</violation>
</file>
<file name="extras/answers.txt.example">
<violation number="1" location="extras/answers.txt.example:23">
P2: Comment says "Set timezone to local time" but the value is UTC. Either update the comment to reflect that UTC is intentional, or change the value to an actual local timezone placeholder (e.g., `${TIMEZONE}`).</violation>
</file>
<file name="etc/unattended.exec.d/90-cleanup.any.sh">
<violation number="1" location="etc/unattended.exec.d/90-cleanup.any.sh:7">
P1: `shred` is not available in BusyBox/Alpine by default (requires `coreutils` package). Because the error is suppressed with `2>/dev/null || true`, the WiFi credentials file will silently remain on disk undeleted. Add a fallback `rm` to ensure the file is at least removed when `shred` isn't available.</violation>
</file>
<file name="etc/unattended.exec.d/99-commit.any.sh">
<violation number="1" location="etc/unattended.exec.d/99-commit.any.sh:16">
P2: Reset `OPTIND=1` before the `getopts` loop. Without this, if `lbu_commit` is invoked more than once in the same shell process (or the script is sourced after another `getopts` user), argument parsing silently skips all options.</violation>
</file>
<file name=".gitignore">
<violation number="1" location=".gitignore:11">
P2: `!**/README.md` will not un-ignore `etc/unattended.conf.d/README.md` because its parent directory is excluded by `etc/*`. Git does not traverse into ignored directories to apply negation patterns. You need to explicitly un-ignore the parent directory first, then re-ignore its contents.</violation>
</file>
<file name="scripts/alpine-rpi.sh">
<violation number="1" location="scripts/alpine-rpi.sh:49">
P2: When `EXTRA_FILES` is empty (the common case), `${EXTRA_FILES[@]:-()}` evaluates to the literal string `"()"`, making `EXTRA_FILES` a one-element array containing `"()"` rather than remaining empty. Use the `${arr[@]+...}` pattern to safely handle unset/empty arrays under `set -u`.</violation>
<violation number="2" location="scripts/alpine-rpi.sh:108">
P2: The `umount` in the trap is missing `sudo`. Since `mount` is called with `sudo` on line 102, the cleanup trap will fail to unmount unless the script is already running as root.</violation>
<violation number="3" location="scripts/alpine-rpi.sh:315">
P1: Bug: `extra_files=("$@")` captures all positional parameters including the first 8 named ones. This causes spurious "not a file" warnings and means the `${#extra_files[@]} -gt 0` check is always true. Use `("${@:9}")` to capture only arguments beyond the 8th.</violation>
</file>
<file name="etc/unattended.exec.d/70-setup-diskless.any.sh">
<violation number="1" location="etc/unattended.exec.d/70-setup-diskless.any.sh:107">
P1: `INTERFACESOPTS_SSID` and `INTERFACESOPTS_PSK` are never exported, so `envsubst` in `setup_alpine()` will substitute them with empty strings. Unlike `LBUOPTS` (which is explicitly exported), these remain local shell variables invisible to `envsubst`.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
I've fixed all 12 review issues:
✅ All shell scripts pass syntax checking ( Pushed commits to |
- Use $IP variable instead of bare `ip` in setup-wifi script - Export INTERFACESOPTS_SSID/PSK for envsubst visibility - Add rm fallback when shred is unavailable (BusyBox/Alpine) - Reset OPTIND=1 before getopts loop in lbu_commit - Remove unreachable *.disabled dead code from run_steps - Add guard for empty $BOOT before sourcing lib - Fix misleading timezone comment (value is UTC) - Fix country code to uppercase US per ISO 3166-1 - Fix .gitignore negation for README.md in ignored directories - Fix empty EXTRA_FILES array handling under set -u - Add sudo to umount in cleanup trap - Fix extra_files to capture only args beyond the 8 named params
- Restore etc/unattended.lib.sh (sourced in 3 places but missing on disk) with a real _logger function and a config_sim_ap stub. POSIX sh; the alias form in unattended.sh doesn't survive the exec.d subshell boundary. - setup_wifi_config: fail loudly when no tty is present instead of hanging on `read -rp` from a script/CI context. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces etc/alpine.lock, sourced by alpine-rpi.sh at startup, to make image builds deterministic across runs: - ALPINE_BRANCH / ALPINE_TARBALL_NAME / ALPINE_TARBALL_SHA256 pin which alpine-rpi-*.tar.gz is fetched and verified. - RPI_FW_COMMIT pins the raspberrypi/firmware DTB to a specific commit (raw.githubusercontent.com is byte-deterministic per commit sha, so the commit itself is the integrity guarantee — no separate DTB sha needed). `fetch_atomically` gains an optional EXPECTED_SHA256 third arg and a shared `_verify_sha256` helper; verification runs on both cache-hit and fresh-download paths, with cache-poisoning fall-through to re-download. `resolve_tarball_url` short-circuits to the locked name when set; otherwise it falls back to the existing index scrape, so behavior with no lock file is unchanged. New `refresh-lock` subcommand (and `make refresh-lock`) regenerates the lock from latest-stable. Implemented with single-tool awk passes over buffered curl responses to avoid SIGPIPE-under-pipefail issues that bit several iterations of this code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an idempotent `init-config` subcommand (and matching make target)
that copies the per-deployment templates from extras/ into their
gitignored etc/ destinations on first run. Existing files are reported
as "keep" and never overwritten, so it's safe to re-run after editing.
- etc/answers.txt and etc/answers-qemu.txt are populated from the same
template; the two stay separate so QEMU-specific tuning can land later
without touching the SD-card path.
- etc/unattended.conf.d/alpine-setup.any.conf gets the example config.
- extras/answers.txt.example: USERSSHKEY now points at the boot-media
authorized_keys file via envsubst (${BOOT}/authorized_keys) instead of
a literal "<Insert SSH Key Here>" placeholder that would have been
passed verbatim to setup-alpine.
- _populate_config_common fails fast with a "run: make init-config"
hint when answers_src is missing, instead of letting `install` emit a
cryptic stat error.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds .github/workflows/lint.yaml that runs shellcheck across scripts/ and etc/ on push/PR, plus a `make lint` target that does the same locally. shfmt is deliberately deferred — the codebase has mixed 2/4 space indentation and that's a separate churn. Fixes the 8 existing findings so the gate passes clean today: - alpine-rpi.sh: rename _copy_launch_files locals (TMP/DTB_FILE) to avoid shadowing outer scope (SC2030/SC2031). - unattended.sh: add `# shellcheck source=` directive for the dynamic $BOOT/unattended.lib.sh source line (SC1091). - 10-setup-wifi.qemu.sh: replace `ls | grep` with a glob loop (SC2010); rewrite `A && B || C` as explicit if/exit so failure paths are unambiguous (SC2015). - 99-commit.any.sh: disable SC2086 with notes at the three sites where $_fwd_opts is intentionally word-split (POSIX sh, no arrays). Removes the empty sdcard() stub and its dispatch entry. The `make sdcard` target stays — it chains populate-boot-sd + populate-config-sd and never needed the shell stub. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
6 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="etc/unattended.sh">
<violation number="1" location="etc/unattended.sh:20">
P1: The `find | sort | while read` pipeline runs the loop in a subshell where `set -e` error propagation is unreliable across shell implementations. If a step script fails, the script may silently continue and reboot with incomplete setup. Redirecting from a temporary file avoids the subshell issue and ensures `set -e` catches failures.</violation>
</file>
<file name="etc/unattended.exec.d/99-commit.any.sh">
<violation number="1" location="etc/unattended.exec.d/99-commit.any.sh:62">
P1: Password is leaked to stderr in the hint message. `$_fwd_opts` contains `-p PASSWORD` and is printed verbatim in this error output, potentially exposing the encryption password in logs.</violation>
</file>
<file name="etc/pre-network.d/10-setup-wifi.qemu.sh">
<violation number="1" location="etc/pre-network.d/10-setup-wifi.qemu.sh:32">
P2: APK repository is switched to HTTP to bootstrap CA certs but never reverted to HTTPS. Subsequent `apk add` calls (lines 72-73) download and install packages over plain HTTP, which is vulnerable to man-in-the-middle attacks. After `ca-certificates-bundle` is installed, switch back to HTTPS.</violation>
</file>
<file name="etc/unattended.exec.d/70-setup-diskless.any.sh">
<violation number="1" location="etc/unattended.exec.d/70-setup-diskless.any.sh:77">
P2: If no `.boot_repository` file is found, `xargs dirname` receives empty input and invokes `dirname` with no arguments, which fails under `set -e`. Add a guard or use `xargs -r` (BusyBox supports it) to avoid running `dirname` on empty input, and validate that `LBUOPTS` is non-empty before proceeding.</violation>
</file>
<file name="scripts/alpine-rpi.sh">
<violation number="1" location="scripts/alpine-rpi.sh:322">
P1: DTB path mismatch: `populate_boot_qemu` saves the DTB to `boot/qemu-rpi4.dtb`, but `_verify` and `_copy_launch_files` (called by `launch`) use the global `DTB_FILE` (e.g., `bcm2710-rpi-3-b-plus.dtb`). These commands will fail because the file doesn't exist at the expected path.
Consider using a consistent variable (e.g., `QEMU_DTB_PATH`) that `populate_boot_qemu` sets and that `verify`/`launch` reference, or change `populate_boot_qemu` to save the DTB at the board-derived `DTB_FILE` path.</violation>
<violation number="2" location="scripts/alpine-rpi.sh:540">
P1: `with_p1_qemu` relies on an EXIT trap for cleanup but never unmounts explicitly on return. In `launch()`, this causes compounding failures: `verify()` leaves the image mounted, then `with_p1_qemu` is called again (stacked mount on same path), and finally `exec` prevents the last trap from ever firing — leaving loop devices leaked and the image host-mounted while QEMU accesses it via `-drive`.
Consider adding explicit cleanup at the end of `with_p1_qemu` (unmount + loop_unmap after `"$@"` returns) and using the trap only as a safety net for abnormal exits.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
1. run_steps no longer swallows failures (etc/unattended.sh) The `find | sort | while read` pipeline ran the loop in a subshell, so `set -e` inside a failing step never bubbled up — the pipe exited 0 and the script rebooted into a half-set-up persistent state. Rewritten to read from a temp file in the current shell; the caller now aborts (and does NOT reboot) when any step fails, leaving the live installer up for SSH/serial diagnosis. 2. with_p1_qemu cleans up explicitly (scripts/alpine-rpi.sh) The previous code only set an EXIT trap. EXIT traps don't compose — each `trap` replaces the prior — and `exec` skips them entirely. In launch(), this meant verify()'s mount/loop leaked, the second with_p1_qemu replaced the (already-replaced) trap, and then `exec qemu-system-aarch64` ran with the image still mounted on the host — concurrent access through the loop device and QEMU's -drive risked corruption. Now explicit umount + loop_unmap run after the wrapped command returns; the trap is reduced to a safety net for abnormal exits. 3. DTB path/name split (scripts/alpine-rpi.sh) DTB_FILE conflated two concepts: a bare filename used as a URL path component, and a relative on-disk path. populate_boot_qemu's `DTB_FILE="boot/qemu-rpi4.dtb"` override only lived for that one invocation; subsequent `verify`/`launch` runs re-read DTB_FILE as the bare board name and looked for it at the wrong location. Now DTB_NAME (URL) and DTB_PATH (`boot/$DTB_NAME`, on-disk) are separate; populate_boot_qemu no longer overrides anything. 4. Empty xargs no longer aborts setup-alpine (70-setup-diskless.any.sh) `find ... | head -1 | xargs dirname` crashed under `set -e` when find matched nothing (xargs invokes dirname with no args, dirname errors "missing operand"). Also left LBUOPTS empty, which silently produced "/cache" as APKCACHEOPTS. Replaced with an explicit `dirname` call guarded by an empty-check that uses _die for a clear failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1. Revert apk repos to HTTPS after CA bootstrap (10-setup-wifi.qemu.sh) Switching to HTTP was needed to install ca-certificates-bundle, but subsequent `apk add` calls (iproute2, hostapd, wpa_supplicant, etc.) continued over plain HTTP. APK still verifies package signatures against /etc/apk/keys, but an MITM could spoof the index to deliver older vulnerable versions. Flip back to HTTPS as soon as the trust store is in place. 2. Redact -p PASSWORD from lbu_commit error hint (99-commit.any.sh) The "Alt:" hint reproduced $_fwd_opts verbatim to stderr, which would leak the encryption password to logs if -p was ever passed. Today nothing in the codebase passes -p, but the wrapper accepts it, so this is defense in depth — sed-scrub `-p VALUE` to `-p ***`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous README only covered installing the swimctl app on a running Pi — none of the image-build pipeline that this branch added was documented. Now leads with the build flow (requirements, init-config, QEMU loop, SD card, lock-file, lint, suffix convention) and preserves the existing app-install content verbatim under a clearly labeled subsection. Adds a callout that the cgroups instructions in the app-install section were written against Raspberry Pi OS; the Alpine image produced here puts cmdline.txt at the FAT partition root rather than under /boot/firmware. Wiring cgroups into the Alpine cmdline is left for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
No description provided.