From f91d2e5453eb0075ea3b91441639d8d47eae6fdc Mon Sep 17 00:00:00 2001 From: acrogenesis Date: Sat, 29 Aug 2026 22:42:33 -0600 Subject: [PATCH] Restore strict migration ordering --- agents/skills/migrations.md | 2 +- bin/omarchy-migrate | 34 ++------ migrations/1787946619.sh | 28 ++----- migrations/1788025225.sh | 37 ++++----- .../shutdown-unit-home-execstop.sh | 2 +- .../legacy-power-udev-rules-migration-test.sh | 61 ++++++++++---- test/shell.d/migrate-scope-test.sh | 82 +++++-------------- test/shell.d/privileged-heredoc-test.sh | 6 +- ...ired-installer-artifacts-migration-test.sh | 14 ++-- 9 files changed, 110 insertions(+), 156 deletions(-) diff --git a/agents/skills/migrations.md b/agents/skills/migrations.md index 4cae6668..0ae0b7aa 100644 --- a/agents/skills/migrations.md +++ b/agents/skills/migrations.md @@ -126,7 +126,7 @@ New migration format: - Start with an `echo` describing what the migration does. - Use `$OMARCHY_PATH` to reference the Omarchy directory. - Be idempotent. Check existing state before changing it. -- When a temporary condition must leave the migration pending without blocking later migrations, write `$OMARCHY_MIGRATION_DEFER_TOKEN` to `$OMARCHY_MIGRATION_DEFER_FILE`, then exit 75. `omarchy-migrate` requires both signals before it continues the queue without writing that migration's completion marker. An unmarked exit 75 is an ordinary failure and aborts the run, so a child command returning `EX_TEMPFAIL` cannot accidentally defer a migration forever. +- Migrations are strictly ordered and synchronous. A migration that cannot finish must exit non-zero, remain pending, and stop the queue; never mark later migrations complete against state an earlier migration has not established. - Use helper commands such as `omarchy-cmd-present`, `omarchy-cmd-missing`, `omarchy-pkg-add`, `omarchy-pkg-drop`, `omarchy-pkg-present`, and `omarchy-pkg-missing` when appropriate. diff --git a/bin/omarchy-migrate b/bin/omarchy-migrate index 59791789..e64aae32 100755 --- a/bin/omarchy-migrate +++ b/bin/omarchy-migrate @@ -85,40 +85,18 @@ wait_for_pacman_transaction mkdir -p "$STATE_DIR" [[ -d $MIGRATIONS_DIR ]] || exit 0 -deferred=() -while IFS=$'\t' read -r name file marker; do +while IFS=$'\t' read -r name file marker <&3; do [[ -n $name ]] || continue if [[ ! -f $marker ]]; then echo -e "\e[32m\nRunning migration (${name%.sh})\e[0m" - defer_file=$(mktemp "$STATE_DIR/.defer.XXXXXX") - defer_token="$BASHPID-$RANDOM-$RANDOM" - migration_status=0 - - if OMARCHY_PATH="$OMARCHY_PATH" \ - OMARCHY_MIGRATION_DEFER_FILE="$defer_file" \ - OMARCHY_MIGRATION_DEFER_TOKEN="$defer_token" \ - bash -euo pipefail "$file"; then - mkdir -p "$(dirname "$marker")" - touch "$marker" - else - migration_status=$? - if (( migration_status == 75 )) && [[ $(<"$defer_file") == "$defer_token" ]]; then - deferred+=("$name") - echo "Migration ${name%.sh} was deferred and will be retried later." - else - rm -f "$defer_file" - exit "$migration_status" - fi - fi - - rm -f "$defer_file" + OMARCHY_PATH="$OMARCHY_PATH" bash -euo pipefail "$file" 3<&- + mkdir -p "$(dirname "$marker")" + touch "$marker" fi -done < <(migration_entries) +done 3< <(migration_entries) # Clear a login-time notification the user left sitting there and then resolved # by running migrations some other way. The substring matches both the current # and legacy notification titles. -if ((${#deferred[@]} == 0)); then - omarchy-notification-dismiss "Omarchy Migrations" >/dev/null 2>&1 || true -fi +omarchy-notification-dismiss "Omarchy Migrations" >/dev/null 2>&1 || true diff --git a/migrations/1787946619.sh b/migrations/1787946619.sh index be89acfe..d6a0705a 100644 --- a/migrations/1787946619.sh +++ b/migrations/1787946619.sh @@ -10,14 +10,6 @@ as_root() { fi } -defer_privileged_repair() { - echo "Cannot remove the legacy privileged udev rule; omarchy-migrate will retry it later." >&2 - if [[ -n ${OMARCHY_MIGRATION_DEFER_FILE:-} && -n ${OMARCHY_MIGRATION_DEFER_TOKEN:-} ]]; then - printf '%s\n' "$OMARCHY_MIGRATION_DEFER_TOKEN" >"$OMARCHY_MIGRATION_DEFER_FILE" - fi - exit 75 -} - # Omarchy 3 generated these two rules with an unquoted heredoc, so the installing # user's $HOME was expanded and the file on disk names that absolute home path. # udev runs RUN+= as root, and @@ -86,23 +78,19 @@ rule_runs_from_home() { return 1 } -removed=0 - for legacy_rule in "99-power-profile.rules:omarchy-powerprofiles-set" "99-wifi-powersave.rules:omarchy-wifi-powersave"; do rule_file="$rules_dir/${legacy_rule%%:*}" if [[ -f $rule_file ]] && rule_runs_from_home "$rule_file" "${legacy_rule##*:}"; then if ! as_root rm -f "$rule_file"; then - defer_privileged_repair + echo "Administrator privileges are required to remove the vulnerable legacy udev rule. Ask an administrator to run omarchy-migrate." >&2 + exit 1 fi - removed=1 + + # Reload after each removal, not after the whole loop. If removing a later + # rule fails, udevd must not keep running one this migration already deleted. + # Best effort the way install/post-install/udev.sh is: a machine with no + # udevd to talk to has had the file removed, and the next boot reads fresh. + as_root udevadm control --reload 2>/dev/null || true fi done - -if (( removed )); then - # Drop the rule from the running udevd too; until it reloads, the rule that was - # just deleted still fires on the next power_supply event. Best effort the way - # install/post-install/udev.sh is: a machine with no udevd to talk to has - # already had the file removed, and the next boot reads the directory fresh. - as_root udevadm control --reload 2>/dev/null || true -fi diff --git a/migrations/1788025225.sh b/migrations/1788025225.sh index d7dbb785..31656ddc 100644 --- a/migrations/1788025225.sh +++ b/migrations/1788025225.sh @@ -253,21 +253,18 @@ plymouth_unit_runs_from_home() { first_run_sudoers="$sudoers_dir/first-run" tsui_sudoers="$sudoers_dir/tsui" -defer_privileged_repair() { - echo "Cannot complete the privileged installer-artifact repair; omarchy-migrate will retry it later." >&2 - if [[ -n ${OMARCHY_MIGRATION_DEFER_FILE:-} && -n ${OMARCHY_MIGRATION_DEFER_TOKEN:-} ]]; then - printf '%s\n' "$OMARCHY_MIGRATION_DEFER_TOKEN" >"$OMARCHY_MIGRATION_DEFER_FILE" - fi - exit 75 +fail_privileged_repair() { + echo "Cannot complete the privileged installer-artifact repair. An administrator must run omarchy-migrate to repair this machine." >&2 + exit 1 } # This is a machine-wide repair with per-user migration markers. A root-owned, # readable marker lets later non-sudo users finish their own migration run after # one privileged account has inspected and repaired the machine. Until then, -# exit 75 asks omarchy-migrate to leave this migration pending while continuing -# with every later migration instead of wedging the whole queue. +# the migration fails loudly and remains pending. After an administrator repairs +# the machine, this marker lets every other account complete without using sudo. if ! as_root true 2>/dev/null; then - defer_privileged_repair + fail_privileged_repair fi # Removing a unit and reloading systemd are one repair. Persist the second half @@ -275,10 +272,10 @@ fi # retry that now sees no unit on disk. if [[ -e $reload_needed_marker ]]; then if ! as_root systemctl daemon-reload >/dev/null 2>&1; then - defer_privileged_repair + fail_privileged_repair fi if ! as_root rm -f "$reload_needed_marker"; then - defer_privileged_repair + fail_privileged_repair fi fi @@ -289,17 +286,17 @@ inspect_sudoers_file() { # an if-condition makes "file missing" indistinguishable from "sudo failed", # which could mark a live grant repaired without ever reading it. if ! kind=$(as_root bash -c 'if [[ -f $1 ]]; then printf file; elif [[ -e $1 ]]; then printf other; else printf missing; fi' bash "$file"); then - defer_privileged_repair + fail_privileged_repair fi [[ $kind == "file" ]] || return 0 if ! content=$(as_root cat "$file"); then - defer_privileged_repair + fail_privileged_repair fi if "$predicate" <<<"$content"; then if ! as_root rm -f "$file"; then - defer_privileged_repair + fail_privileged_repair fi fi } @@ -314,24 +311,24 @@ if [[ -f $plymouth_unit ]] && plymouth_unit_runs_from_home <"$plymouth_unit"; th # ExecStop is the path this migration exists to keep root away from; disabling # only drops the multi-user.target symlink. if ! as_root install -Dm644 /dev/null "$reload_needed_marker"; then - defer_privileged_repair + fail_privileged_repair fi if ! as_root systemctl disable omarchy-plymouth-shutdown.service >/dev/null 2>&1; then - defer_privileged_repair + fail_privileged_repair fi if ! as_root rm -f "$plymouth_unit"; then - defer_privileged_repair + fail_privileged_repair fi # systemd keeps serving the copy it already loaded until it rereads the # directory, so without this the unit is still there to run at shutdown. if ! as_root systemctl daemon-reload >/dev/null 2>&1; then - defer_privileged_repair + fail_privileged_repair fi if ! as_root rm -f "$reload_needed_marker"; then - defer_privileged_repair + fail_privileged_repair fi fi if ! as_root install -Dm644 /dev/null "$machine_marker"; then - defer_privileged_repair + fail_privileged_repair fi diff --git a/test/shell.d/fixtures/privileged-heredoc/shutdown-unit-home-execstop.sh b/test/shell.d/fixtures/privileged-heredoc/shutdown-unit-home-execstop.sh index 13b91cdd..54b21414 100644 --- a/test/shell.d/fixtures/privileged-heredoc/shutdown-unit-home-execstop.sh +++ b/test/shell.d/fixtures/privileged-heredoc/shutdown-unit-home-execstop.sh @@ -1,4 +1,4 @@ -#!/usr/bin/env bash +#!/bin/bash # Install Plymouth package echo "Installing Plymouth..." diff --git a/test/shell.d/legacy-power-udev-rules-migration-test.sh b/test/shell.d/legacy-power-udev-rules-migration-test.sh index 0f38fce8..969d182f 100755 --- a/test/shell.d/legacy-power-udev-rules-migration-test.sh +++ b/test/shell.d/legacy-power-udev-rules-migration-test.sh @@ -105,7 +105,8 @@ pass "migration removes a Wi-Fi power save rule that runs out of a user home" fail "migration reloads udev after removing a rule" "$(cat "$CALLS")" pass "migration reloads udev after removing a rule" -# Both files gone is still one machine-wide reload, not one per file. +# Reload each removed rule immediately, so a later failure cannot leave an +# already-deleted rule active in udevd. reset_machine write_vulnerable_power_rule write_vulnerable_wifi_rule @@ -113,9 +114,9 @@ run_migration [[ ! -e $power_rule && ! -e $wifi_rule ]] || fail "migration removes both legacy rules in one pass" -(( $(reload_count) == 1 )) || - fail "migration reloads udev once for both removals" "$(cat "$CALLS")" -pass "migration removes both legacy rules and reloads udev once" +(( $(reload_count) == 2 )) || + fail "migration reloads udev after each removal" "$(cat "$CALLS")" +pass "migration removes both legacy rules and reloads after each one" # The second run is what every other account on the machine does, and what a # user gets from running omarchy-migrate again. @@ -210,29 +211,55 @@ run_migration fail "migration removes another user's rule rooted outside /home" "$(cat "$wifi_rule")" pass "migration removes another user's rule rooted outside /home" -# A user who cannot elevate must leave this repair pending without preventing -# later migrations from running. Once another account removes the machine-wide -# file, the next retry can complete without sudo. +# A user who cannot elevate leaves this migration pending and stops the ordered +# queue. Once an administrator removes the machine-wide file, a retry completes. reset_machine write_vulnerable_wifi_rule -defer_file="$test_dir/defer-signal" -defer_token="legacy-udev-repair" -: >"$defer_file" set +e HOME="$home_dir" \ OMARCHY_UDEV_RULES_DIR="$rules_dir" \ - OMARCHY_MIGRATION_DEFER_FILE="$defer_file" \ - OMARCHY_MIGRATION_DEFER_TOKEN="$defer_token" \ PATH="$test_dir/failing-bin:$PATH" \ - bash -euo pipefail "$migration" >"$test_dir/defer.out" 2>&1 -defer_status=$? + bash -euo pipefail "$migration" >"$test_dir/elevation-failure.out" 2>&1 +failure_status=$? set -e -(( defer_status == 75 )) || fail "migration defers when sudo cannot remove a vulnerable rule" "status=$defer_status" +(( failure_status != 0 )) || fail "migration fails when sudo cannot remove a vulnerable rule" [[ -e $wifi_rule ]] || fail "migration keeps the vulnerable rule when its elevated removal fails" -[[ $(<"$defer_file") == "$defer_token" ]] || fail "migration authenticates its deferral to the runner" -pass "migration defers instead of blocking the queue when removal cannot elevate" +grep -q 'Ask an administrator to run omarchy-migrate' "$test_dir/elevation-failure.out" || + fail "migration explains how a non-sudo user can complete the repair" "$(cat "$test_dir/elevation-failure.out")" +pass "migration fails loudly with administrator guidance when removal cannot elevate" + +# If the first removal succeeds but the second one cannot elevate, the first +# rule must already have been dropped from the running udevd. +reset_machine +write_vulnerable_power_rule +write_vulnerable_wifi_rule +cat >"$test_dir/failing-bin/sudo" <<'STUB' +#!/bin/bash + +printf 'sudo %s\n' "$*" >>"$CALLS" +if [[ $* == *99-wifi-powersave.rules ]]; then + exit 1 +fi +exec "$@" +STUB +chmod +x "$test_dir/failing-bin/sudo" +: >"$CALLS" + +set +e +HOME="$home_dir" \ + OMARCHY_UDEV_RULES_DIR="$rules_dir" \ + PATH="$test_dir/failing-bin:$test_dir/bin:$PATH" \ + bash -euo pipefail "$migration" >"$test_dir/partial-failure.out" 2>&1 +partial_status=$? +set -e + +(( partial_status != 0 )) || fail "migration fails when the second rule cannot be removed" +[[ ! -e $power_rule && -e $wifi_rule ]] || fail "migration preserves the expected partial-removal state" +(( $(reload_count) == 1 )) || + fail "migration reloads udev before a later removal failure" "$(cat "$CALLS")" +pass "a later removal failure cannot leave an already-deleted rule loaded" # Nothing named the wrong binary is ours: the same path with a different command # is a rule this migration cannot claim to know anything about. diff --git a/test/shell.d/migrate-scope-test.sh b/test/shell.d/migrate-scope-test.sh index d31240b9..ddb7f59e 100644 --- a/test/shell.d/migrate-scope-test.sh +++ b/test/shell.d/migrate-scope-test.sh @@ -76,68 +76,30 @@ grep -q '^before-fail$' "$calls" || fail "migration runner started failing migra ! grep -q '^after-fail$' "$calls" || fail "migration runner stops failing migration under strict mode" pass "migration runner does not mark failed migrations complete" -deferred_root="$test_tmp/deferred-omarchy" -deferred_home="$test_tmp/deferred-home" -deferred_calls="$test_tmp/deferred-calls" -mkdir -p "$deferred_root/migrations" "$deferred_home" +stdin_root="$test_tmp/stdin-omarchy" +stdin_home="$test_tmp/stdin-home" +stdin_calls="$test_tmp/stdin-calls" +mkdir -p "$stdin_root/migrations" "$stdin_home" -cat >"$deferred_root/migrations/100-deferred.sh" <<'SH' -echo deferred >>"$TEST_CALLS" -printf '%s\n' "$OMARCHY_MIGRATION_DEFER_TOKEN" >"$OMARCHY_MIGRATION_DEFER_FILE" -exit 75 +cat >"$stdin_root/migrations/100-reader.sh" <<'SH' +IFS= read -r value +printf 'reader:%s\n' "$value" >>"$TEST_CALLS" SH -cat >"$deferred_root/migrations/200-after.sh" <<'SH' -echo after >>"$TEST_CALLS" +cat >"$stdin_root/migrations/200-after.sh" <<'SH' +echo after-reader >>"$TEST_CALLS" SH -HOME="$deferred_home" \ -OMARCHY_PATH="$deferred_root" \ -TEST_CALLS="$deferred_calls" \ - "$ROOT/bin/omarchy-migrate" >"$test_tmp/deferred.out" +printf 'migration input\n' | \ + HOME="$stdin_home" \ + OMARCHY_PATH="$stdin_root" \ + TEST_CALLS="$stdin_calls" \ + "$ROOT/bin/omarchy-migrate" >"$test_tmp/stdin.out" -grep -q '^deferred$' "$deferred_calls" || fail "migration runner starts a deferred migration" -grep -q '^after$' "$deferred_calls" || fail "migration runner continues after a deferred migration" -[[ ! -f $deferred_home/.local/state/omarchy/migrations/100-deferred.sh ]] || - fail "migration runner leaves a deferred migration pending" -[[ -f $deferred_home/.local/state/omarchy/migrations/200-after.sh ]] || - fail "migration runner records a later successful migration" -grep -q 'was deferred and will be retried later' "$test_tmp/deferred.out" || - fail "migration runner reports a deferred migration" -pass "migration runner leaves exit-75 migrations pending and continues the queue" - -HOME="$deferred_home" OMARCHY_PATH="$deferred_root" \ - "$ROOT/bin/omarchy-migrate" --pending >"$test_tmp/deferred-pending.out" -grep -q '^100-deferred\.sh$' "$test_tmp/deferred-pending.out" || - fail "migration runner still reports a deferred migration as pending" -! grep -q '^200-after\.sh$' "$test_tmp/deferred-pending.out" || - fail "migration runner does not report the completed later migration as pending" -pass "migration runner reports only the deferred migration as pending" - -raw_75_root="$test_tmp/raw-75-omarchy" -raw_75_home="$test_tmp/raw-75-home" -raw_75_calls="$test_tmp/raw-75-calls" -mkdir -p "$raw_75_root/migrations" "$raw_75_home" - -cat >"$raw_75_root/migrations/100-child-tempfail.sh" <<'SH' -echo child-tempfail >>"$TEST_CALLS" -bash -c 'exit 75' -SH -cat >"$raw_75_root/migrations/200-after.sh" <<'SH' -echo after-tempfail >>"$TEST_CALLS" -SH - -set +e -HOME="$raw_75_home" \ -OMARCHY_PATH="$raw_75_root" \ -TEST_CALLS="$raw_75_calls" \ - "$ROOT/bin/omarchy-migrate" >"$test_tmp/raw-75.out" 2>"$test_tmp/raw-75.err" -raw_75_status=$? -set -e - -(( raw_75_status == 75 )) || - fail "migration runner preserves an unmarked child exit 75" "status=$raw_75_status" -grep -q '^child-tempfail$' "$raw_75_calls" || fail "migration runner starts the exit-75 child" -! grep -q '^after-tempfail$' "$raw_75_calls" || fail "migration runner stops after an unmarked exit 75" -[[ ! -f $raw_75_home/.local/state/omarchy/migrations/100-child-tempfail.sh ]] || - fail "migration runner leaves an unmarked exit-75 migration incomplete" -pass "migration runner does not mistake a child EX_TEMPFAIL for intentional deferral" +grep -q '^reader:migration input$' "$stdin_calls" || + fail "migration runner preserves the caller's stdin for a migration" "$(cat "$stdin_calls")" +grep -q '^after-reader$' "$stdin_calls" || + fail "a migration reading stdin does not swallow later queue entries" "$(cat "$stdin_calls")" +[[ -f $stdin_home/.local/state/omarchy/migrations/100-reader.sh && + -f $stdin_home/.local/state/omarchy/migrations/200-after.sh ]] || + fail "migration runner marks both stdin-isolated migrations complete" +pass "migration queue uses a private file descriptor instead of migration stdin" diff --git a/test/shell.d/privileged-heredoc-test.sh b/test/shell.d/privileged-heredoc-test.sh index 0bbe1abf..9501113b 100755 --- a/test/shell.d/privileged-heredoc-test.sh +++ b/test/shell.d/privileged-heredoc-test.sh @@ -475,8 +475,8 @@ privileged_destination() { while ((follow < ${#scan_lines[@]})); do hop=${scan_lines[follow]} follow=$((follow + 1)) - line_carries_destination "$hop" "$dest" || continue [[ $hop =~ (^|[[:space:]])(install|cp|mv)([[:space:]]|$) ]] || continue + line_carries_destination "$hop" "$dest" || continue while IFS= read -r hop_dest; do [[ $hop_dest == $'\002elevated' ]] && continue [[ $hop_dest == "$dest" ]] && continue @@ -761,7 +761,9 @@ shell_sources() { local file first while IFS= read -r -d '' file; do - grep -Iq . "$file" 2>/dev/null || continue + # Binary files and sources with no heredoc operator have nothing this check + # can classify. Filter them before collect_vars and the line-by-line scan. + grep -Iq '<<' "$file" 2>/dev/null || continue case $file in *.sh | *.hook) diff --git a/test/shell.d/retired-installer-artifacts-migration-test.sh b/test/shell.d/retired-installer-artifacts-migration-test.sh index 1769649e..0d5d6703 100755 --- a/test/shell.d/retired-installer-artifacts-migration-test.sh +++ b/test/shell.d/retired-installer-artifacts-migration-test.sh @@ -571,8 +571,8 @@ FAIL_DAEMON_RELOAD_ONCE_MARKER="$reload_failure_seen" run_migration reload_status=$? set -e -(( reload_status == 75 )) || - fail "migration defers after a failed daemon-reload" "status=$reload_status" +(( reload_status != 0 )) || + fail "migration fails after a failed daemon-reload" "status=$reload_status" [[ ! -e $plymouth_unit && -e $reload_needed_marker && ! -e $machine_marker ]] || fail "migration records the pending reload without marking the repair complete" @@ -606,15 +606,15 @@ HOME="$home_dir" \ gate_status=$? set -e -(( gate_status == 75 )) || - fail "migration defers when it cannot elevate to inspect the sudoers directory" "status=$gate_status$(printf '\n%s' "$(cat "$test_dir/gate.out")")" +(( gate_status != 0 )) || + fail "migration fails when it cannot elevate to inspect the sudoers directory" "status=$gate_status$(printf '\n%s' "$(cat "$test_dir/gate.out")")" [[ -e $readable/first-run ]] || fail "migration keeps a live grant when elevation fails" [[ ! -e $machine_marker ]] || fail "migration leaves the machine repair unmarked when elevation fails" -grep -q 'will retry it later' "$test_dir/gate.out" || - fail "migration says why it could not inspect the directory" "$(cat "$test_dir/gate.out")" -pass "migration defers without marking a readable sudoers directory repaired when elevation fails" +grep -q 'An administrator must run omarchy-migrate' "$test_dir/gate.out" || + fail "migration explains how the machine-wide repair can complete" "$(cat "$test_dir/gate.out")" +pass "migration fails without marking the machine repaired and names the administrator action" # After one privileged account completes the machine repair, a non-sudo user # can finish their per-user migration without probing sudo again.