Keep first-run retryable and tolerate a missing DMI probe

[scottjones]

First-run discarded a failed user finalization and still marked the run complete, so a later retry never ran. Finalization is now logged and flagged like the other steps.

Hardware leaves that read DMI abort the whole hardware pass under set -e when the file is missing. Guard those probes (same || true form on SPI and Surface), and skip bin/__pycache__ in the locate test so a Python import cannot fail a later scan.

[llstrk]

Automated AI review

Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review.

Verified: the first-run change works as described, and the DMI guard in apple/fix-spi-keyboard.sh fixes a real abort of the hardware pass.

The locate-test commit now conflicts with quattro, which removed that scan. #11125 and #13362 carry the same first-run fix. The Surface and Intel edits do not change behaviour on a machine without DMI, and docs/file-layout.md still describes the old first-run.

Verified: first-run, with finalization stubbed to fail and the real omarchy-done:

Login	Base	Head
1, finalization fails (exit 42)	later steps run, first-run marked done	later steps run, Failed: finalize user (exit code: 42) logged, not marked
2, finalization succeeds	exits at the done check, no retry	retries, marks done
3	exits at the done check	exits at the done check

Finalization now runs after mkdir -p "$state_dir" and the log setup, and finalize_user_status=$? is the first command in the else branch, so it holds the finalizer's status (42, 3 and 1 were logged as injected). --force is still passed through. If finalization keeps failing, the whole first-run sequence, including the welcome and Wi-Fi toasts, repeats at every login. That matches the retry contract docs/file-layout.md already states for the other steps.

The locate-test commit conflicts with quattro

quattro commit 273c340 ("Remove obsolete repository scan from locate test") deleted the repository scan in test/shell.d/locate-test.sh that commit 400dad0 edits. That file is the only conflicted path, and GitHub reports the PR as conflicting. The bug it fixed was real at the merge base: the agent-usage tests import bin/ scripts with SourceFileLoader, which writes bin/__pycache__/*.pyc, and the old scan then failed with UnicodeDecodeError.

Impact: the branch needs a conflict resolution, and on the current base this change has nothing left to fix.

Suggested change: drop commit 400dad0 and rebase the other two onto quattro. With the tip's version of the locate test, first-run-test.sh, install-leaf-guards-test.sh and locate-test.sh all pass on a simulated merge. #11847 is another fix for the same removed scan.

Overlap with #11125 and #13362

• Retry first-run when user finalization fails #11125 (opened 2026-09-10) removes the same || true line and runs finalization through run_first_run_step. It also updates docs/file-layout.md and tests both a failed and a successful finalization. Merged with this PR, only test/shell.d/first-run-test.sh conflicts. bin/omarchy-provision-first-run merges cleanly into a script that calls omarchy-provision-user twice, so resolving only the test conflict would ship the double call.
• Converge omarchy-mac and omarchy-mx-mac into upstream Omarchy #13362 contains this PR's first-run block and test almost unchanged (its description credits this PR's author for the fix), and conflicts with it in first-run-test.sh.
• In install/hardware/apple/fix-spi-keyboard.sh, Run Omarchy on Apple Silicon under Asahi Linux #9835 guards the same DMI line differently and [Intel Mac P07] Preserve the MacBook8,1 SPI PIO workaround #12685 rewrites the line next to it; both conflict with this PR there.

Impact: the three PRs overlap, and a combined merge that resolves only the test conflict would call omarchy-provision-user twice.

Suggested change: settle on one first-run fix, and keep exactly one finalization call in whichever combination lands.

Only the SPI guard prevents an abort on a machine without DMI

Hardware leaves run through run_logged (install/helpers/logging.sh) as bash -eE -c 'source "$1"', without pipefail, and a failing leaf stops the rest of the hardware pass. The unchanged leaves were run through the real run_logged, with an empty /sys for missing DMI and a sentinel leaf after each one:

Leaf	Base	Head
apple/fix-spi-keyboard.sh	exit 1, later leaves skipped	exit 0
fix-surface-keyboard.sh, real omarchy-hw-surface	exit 0 (branch not entered)	exit 0
fix-surface-keyboard.sh, predicate forced true	exit 1	exit 0
intel/lpmd.sh, intel/thermald.sh, grep forced to fail	exit 0	exit 0

• Surface: the read sits inside if omarchy-hw-surface, which itself needs DMI sys_vendor to be "Microsoft Corporation", so on a machine without DMI the branch never runs. The guard only matters if the predicate matches but product_name is unreadable.
• Intel: without pipefail, the status of grep | cut | tr is tr's, so a failing grep cannot make the assignment fail. The || true only matters if pipefail is enabled. These two edits are needed for the new guard test to pass, not to prevent an abort.

No other unguarded /sys or /proc read in install/ could abort a leaf under errexit.

Impact: the PR description overstates the fix: in the tested conditions, only the Apple SPI read aborted the hardware pass.

Suggested change: describe the Surface and Intel edits as consistency changes for the new test, since the PR description says leaves (plural) abort the pass.

install-leaf-guards-test.sh is a line-based check

The test greps for $(... /sys|/proc ...), then skips any line containing || and any line starting with if or elif. Run over synthetic leaves and compared with what each form does under bash -eE with the file missing:

Form	Aborts?	Test result
x="$(cat /sys/...)", x=$(</sys/...)	yes	flagged
backticks; path in a variable; substitution split across lines	yes	passes
if [[ ... ]]; then x=$(cat /sys/...); fi on one line	yes	passes (skipped as an if line)
another || elsewhere on the line	yes	passes
local x=$(...), echo "$(...)", [[ $(...) == ... ]] &&, pipelines without pipefail	no	flagged

Impact: the test name "install leaves tolerate a missing sysfs probe" claims more than the check proves. Open PRs that add the same unguarded DMI read (for example #8285, #10331 and #12286) would start failing it once this lands, which is partly the point, but it also flags harmless lines.

Suggested change: either restrict the match to plain assignment lines, or say in the test comment that it is a heuristic for the common form.

Documentation: docs/file-layout.md (line 270 at this head) still says first-run "first runs omarchy-provision-user || true". The same section already states that on failure the marker is not written and the sequence retries next login.

Optional test improvement: the new first-run test only covers a failing finalizer. A driver that always set first_run_failed=1, so first-run never completes, still passes all of first-run-test.sh. A success case that asserts the first-run-user mark would catch that.

---

Review information

Test scope: pinned head 400dad0d, merge base 2fbac0c8 and quattro at d3fb0284, plus a simulated merge onto that tip. The PR's tests, first-run (finalizer stubbed with fixed exit codes) and the four hardware leaves (system-changing helpers stubbed; the Intel leaves read an Intel host CPU's /proc/cpuinfo, with the grep failure injected) ran in an isolated sandbox. Missing DMI was reproduced with an empty /sys. No Apple Silicon or Surface hardware was used, so the premise that Apple Silicon has no DMI comes from the PR. The full hardware pass and the real omarchy-provision-user were not run. Overlap with other open PRs was checked by merging their heads with this one.

AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.

Opt out: To stop receiving these reviews, reply to this comment saying so.

[omarchybot]

Reviewed at 400dad0 by Claude Opus 5.5, with Codex Medium as a second opinion. I read the diff against quattro and compared it with #11125, which fixes the same first-run bug. Nothing was run on a worker, because the first-run half has a better competing fix (below), and testing a fix that will not be merged would spend a worker for nothing.

First-run: #11125 is the better fix for this half. Both changes stop discarding omarchy-provision-user's status and leave first-run-user unmarked on failure, and both capture the exit code correctly. #11125 does it by calling the existing run_first_run_step helper, where this PR repeats that helper's start/complete/fail logging inline. #11125 also updates docs/file-layout.md, which here still says first-run runs omarchy-provision-user || true (line 270 at this head). Its test covers a successful finalization as well as a failed one. The test here covers only the failure, so a driver that never marked first-run complete would still pass it. #13362 carries the same first-run block as this PR.

DMI guard: a real fix, and the part only this PR has. run_logged sources each hardware leaf under bash -eE. When /sys/class/dmi/id/product_name is missing, the unguarded assignment in install/hardware/apple/fix-spi-keyboard.sh takes cat's failure, aborts the leaf, and stops the hardware pass. The || true form matches what apple/fix-suspend-nvme.sh and apple/fix-brcmfmac-supplicant.sh already do. The Surface guard only matters when omarchy-hw-surface matches but product_name cannot be read. The two Intel edits change nothing without pipefail, because each pipeline's status is tr's. So those three edits are for consistency, plus the new guard test. That test is a line-based heuristic: it skips any line containing || or starting with if, so a pass does not prove every probe is guarded.

Locate test: nothing left to fix. quattro commit 273c340 removed the repository scan that 400dad0 edits. That commit is why GitHub reports this PR as conflicting.

Codex Medium also preferred #11125 for the first-run fix and agreed about the DMI and locate commits. It saw the question after I had reasoned about it, and it can read this session, so its agreement is not guaranteed to be independent. It added the Surface case above, where detection succeeds through product_family while product_name is missing.

This waits on the maintainer to choose between this PR and #11125 for the first-run fix. If #11125 lands, the DMI guard (ca166b0) is worth keeping as a PR of its own, without the first-run and locate-test commits.