Skip to content

fix(installer): keep incomplete-cleanup summary reachable under errexit - #1303

Merged
tw93 merged 1 commit into
tw93:mainfrom
iTofu:fix/installer-errexit-summary
Jul 31, 2026
Merged

fix(installer): keep incomplete-cleanup summary reachable under errexit#1303
tw93 merged 1 commit into
tw93:mainfrom
iTofu:fix/installer-errexit-summary

Conversation

@iTofu

@iTofu iTofu commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • bin/installer.sh runs under set -euo pipefail, and main() called perform_installers bare before capturing $?. Any nonzero return — most commonly INSTALLER_EXIT_INCOMPLETE (3), when the selection includes files that path validation refuses to delete — aborted the script at that line with raw exit 3.
  • The case $exit_code handler below was therefore unreachable for every nonzero status: the "Installer cleanup incomplete / Failed to remove N installers" summary added in Fix installer deletion reporting #990 never rendered, and the terminal showed nothing after the confirmation — just an unexplained failed command. The cancel path was affected too: quitting the menu exited 1 instead of the designed 0.
  • Capture the status with perform_installers || exit_code=$?, the same idiom delete_selected_installers already uses, so incomplete cleanup prints its report and exits 1 as designed. This follows the Pipefail Safety guidance in CONTRIBUTING.md.
  • Add a regression test that invokes main() with errexit active, matching production invocation.

Observed on a real run: 46 installers selected, 41 removed, 5 rejected by app protection — the deletion audit log recorded everything, but the terminal output stopped after the file list and the command exited 3.

Reproduction

The failure branch only triggers when a selected file is refused at delete time. Deletion normally succeeds for every selected file, which is why this path is rarely exercised. The easiest organic trigger is an installer whose filename matches the proxy-tool protection patterns in lib/core/app_protection_data.sh: the scan lists it as a candidate, then validate_path_for_deletion refuses it at delete time.

dd if=/dev/zero of=~/Downloads/ClashX-repro.dmg bs=1k count=4
mo installer --dry-run   # select it, Enter, then Enter to confirm

Without this fix: output stops after the "Files to be removed:" list, no summary, exit code 3. With this fix: "Dry run complete / Failed to remove 1 installer (delete failed)", exit code 1.

The new bats test reproduces the same shape hermetically by planning /System as a protected entry, so CI does not depend on any local files.

Safety Review

  • Does this change affect cleanup, uninstall, optimize, installer, remove, analyze delete, update, or install behavior?
    • Installer reporting only. Deletion routing, plan validation, and protection checks are unchanged; the fix only captures the status of perform_installers without tripping errexit so the existing summary and exit-code handler can run.
  • Does this change affect path validation, protected directories, symlink handling, sudo boundaries, or release/install integrity?
    • No.

Tests

  • MOLE_TEST_NO_AUTH=1 bats tests/installer.bats tests/installer_fd.bats tests/installer_zip.bats — 42/42, including the new regression test, which fails against main without this fix
  • ./scripts/check.sh — shfmt, shellcheck, and syntax checks pass
  • Manual: scripted-PTY run of ./mole installer --dry-run with a selection containing protected installers. Before: exit 3, no output after the file list. After: full summary ("Failed to remove 10 installers" with per-file reasons), exit 1. Cancelling the menu now exits 0 instead of 1.

Safety-related changes

  • None.

@iTofu
iTofu requested a review from tw93 as a code owner July 27, 2026 11:59
@tw93
tw93 merged commit 6614451 into tw93:main Jul 31, 2026
@tw93

tw93 commented Jul 31, 2026

Copy link
Copy Markdown
Owner

@iTofu, the real 46-installer example made this one easy to trust. Thank you for tracing the silent exit back to one small errexit edge. The cleanup report can now explain protected files instead of leaving users with a raw status 3.

You can test the merged patch today with mo update --nightly if you use the script install, then rerun the installer case that used to stop after the file list. Stable will pick it up next.

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.

2 participants