fix(installer): keep incomplete-cleanup summary reachable under errexit - #1303
Merged
Conversation
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 |
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.
Summary
bin/installer.shruns underset -euo pipefail, andmain()calledperform_installersbare before capturing$?. Any nonzero return — most commonlyINSTALLER_EXIT_INCOMPLETE(3), when the selection includes files that path validation refuses to delete — aborted the script at that line with raw exit 3.case $exit_codehandler 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.perform_installers || exit_code=$?, the same idiomdelete_selected_installersalready uses, so incomplete cleanup prints its report and exits 1 as designed. This follows the Pipefail Safety guidance in CONTRIBUTING.md.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, thenvalidate_path_for_deletionrefuses it at delete time.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
/Systemas a protected entry, so CI does not depend on any local files.Safety Review
perform_installerswithout tripping errexit so the existing summary and exit-code handler can run.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 againstmainwithout this fix./scripts/check.sh— shfmt, shellcheck, and syntax checks pass./mole installer --dry-runwith 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