This change allows to have a nice output like:

Update details:  {
  stdout: '☑️  No updates found\n' +
    '☑️  The existing battery visudo file is what it should be for version v1.3.2\n',
  stderr: ''
}

not only in terminal but in gui.log as well. Version 1.3.2 logs stdout only. Btw, stdout and stderr are logged for errors as well, unless we decide to ignore resolved/rejected value.

Makes the new app session more visible in the gui.log

This check is not needed anymore. The smc command just reads smc changing nothing

Old GUI version actually works if the new battery.sh is already installed, but it cannot be used to update from current version. It will install the new battery script using current battery executable, but the next time Old GUI starts user will get an error alert. I'm gonna need one more commit to prevent it.

Done

Not needed here, charging is restored below, don't do it twice.

install is just a system utility which does the same job as cp/chown/chmod commands we deleted above and below.

Ah this is dope, I didn't know that util. I see the default for -m is already 755 but I like it explicitly in here for clarity indeed

Only executables and folders require 755. Others are good with just 644 which is read/write for owner and read for everyone else.

A user once had a root ownership on ~/Library/LaunchAgents folder (#420 (comment)). Not sure how that could happen, but lets make installer capable of fixing it.

visudo does not need $calling_user argument anymore. Will remove.

done

mktemp is a standard system utility for temporary directory allocation, used extensively on macOS. The system owns the directory and will remove it at some point on startup if we don’t remove it ourselves.

True that, nice add

sudo chown root:wheel "$binfolder/smc" will cause the update to fail (because set -e is enabled) on installations where /usr/local/bin/smc is missing or temporarily unavailable. Consider guarding this with an existence check and/or installing/updating smc as part of the update flow so battery update remains reliable.

When $binfolder/smc is missing the battery setup is broken. It would be misleading for update script to succeed like everything is fine. When Terminal users see the error, they are expected to run battery reinstall. GUI app will trigger reinstall on its startup. We can adopt suggested change, but I don't see a reason for that.

the line changing smc ownership is not needed anymore and removed

tempfolder=/Users/$calling_user/.battery-tmp is a predictable, user-writable location, but later the script runs sudo install ... "$batteryfolder/..." from inside that tree. This creates a local privilege-escalation opportunity via symlink/TOCTOU replacement of the downloaded artifacts before the sudo install steps. Use a mktemp -d temp directory with restrictive permissions (and ideally root-owned for the portion used as the sudo install source) and clean it up via trap.

An unlikely scenario, but I agree. Will add mktemp -d to setup.sh

done

sudo install -d -m 755 -o root -g wheel "$binfolder" will modify the ownership of the existing /usr/local/bin directory. On Intel/Homebrew setups that rely on user-owned /usr/local, this can break package installs/updates and other tooling. If the goal is to prevent replacement of battery executables, consider installing into a dedicated root-owned subdirectory (or only enforcing root ownership on the specific battery files + a dedicated parent dir) rather than taking ownership of the entire /usr/local/bin.

Homebrew lives somewhere in /opt on Apple silicon macs and this app supports silicon macs only. But regarding some other 3pp software we don't even know exists, the Copilot's comment is true.
That means we are homeless now. Additionally, the change would require a modification of PATH variable for any possible shell used by users so the battery script can be launched without a full path.

Battery executables were relocated to /usr/local/co.palokaj.battery in order to avoid conflicts with other apps. /usr/local is SIP protected by system which makes it the best choice (see this Discussion with Copilot for details).
The name for co.palokaj.battery folder was chosen the same as bundle id of the battery.app. It's purpose is to be unique to avoid conflicts with other software. Also, it has to indicate that it's a battery.app's folder, so when a user sees it in their PATH variable they don't freak out.
The executables are installed right inside /usr/local/co.palokaj.battery. There are no 'bin' or 'sbin' subfolders because they are unlikely to ever be needed.
Also, /usr/local/bin/battery and /usr/local/bin/smc are now symlinks, mostly for compatibility with current version of GUI app. The names are already claimed and having them does no harm.

Please, let me know if you prefer a different name for co.palokaj.battery folder.

Name is fine for now. It is also an identity that links to my apple certificate that I use for signing. It's not user facing so it's fine

The GUI still runs battery via PATH (const battery = ${ path_fix } battery``) while path_fix includes user-writable locations like `/opt/homebrew/bin`. This reintroduces PATH-based spoofing risk (a malicious `battery` earlier in PATH could be executed and then prompt for admin credentials / run privileged subcommands). Prefer invoking the absolute path (`/usr/local/bin/battery`) everywhere, and avoid inheriting `process.env.PATH` in `shell_options` for these calls.

Makes sense.

Now we are using a full executable path and minimal PATH. path_fix is not needed and removed.

Nice approach

The comment/doc for the shell helpers says rejected results are Error objects with extra cmd and code properties, but the implementation only attaches output (and the timeout error sets code but not cmd). Either populate err.cmd consistently (and ensure err.code is always set for exec errors) or adjust the documented contract so downstream callers/logging can rely on it.

Good catch. cmd and code are included in error objects by other means, that's what I see in logs. But I don't add them to timeout errors. Also, if the process is terminated by a signal there will be no code.

done

The curl -sS "$github_url_setup_sh" | bash pipeline downloads and executes a remote shell script from GitHub without any integrity or authenticity verification. If the GitHub repository or the network path is compromised, an attacker can serve a modified setup.sh that runs arbitrary code (including invoking sudo) under the installing user's session. Replace this pattern by downloading to a file and verifying a pinned commit/tag or checksum before execution, or by executing only locally distributed installer scripts instead of live curl|bash.

The described problem is not related to this PR and deserves a new one. But only if we decide to consider compromise of the GitHub repository or the HTTPS channel a threat in our security model.
If the actuallymentor's GitHub account doesn't change owner and users don't have state actors targeting them, everybody should be fine.

The update_silent action executes curl -sS "$github_url_update_sh" | bash as root via a passwordless sudoers rule, which downloads and runs an unpinned script from GitHub with full root privileges and no integrity verification. Compromise of the upstream repository or the HTTPS channel would immediately yield arbitrary root code execution on every client that invokes battery update_silent (including the GUI’s automatic background updates). Mitigate by avoiding curl|bash, pinning to immutable versions (e.g. commit SHA) and verifying checksums/signatures before executing any downloaded update logic, or by shipping and updating from locally stored, signed artifacts instead of live scripts.

Signatures would resolve all listed threats (even though those threats are unlikely), I like the idea, but again, not in this PR.

exec_async() implements timeouts via Promise.race, but it does not terminate the underlying subprocess when the timeout wins. For commands like the online checks (curl), this can leave long-running/hung processes behind even though the caller has moved on. Prefer using child_process.exec’s built-in timeout/killSignal options or enforce timeouts at the command level (e.g. curl --max-time/--connect-timeout) so the process is actually stopped.

The behavior we have is exactly the one we need for 'battery maintain'.
Regarding hanging network requests, I consider it not a big deal. Also, the PR does not make the situation here worse comparing to what we already have in the latest version 1.3.2.

In the update action, version_before is computed by executing $battery_binary version before check_installation_integrity runs. If the installation is partially broken (e.g. missing or non-executable battery binary), this call can fail/noise before you’ve decided to force reinstall. Consider moving the version_before call to after the integrity check (and only when $battery_binary is known to be runnable), or guard it so the reinstall path remains clean and reliable.

will fix.

done

The update flow doesn’t check the result of the sudo install ... steps. If install fails (e.g. permissions, disk full), the script will still print the success message and exit 0, leaving a partial update. Consider checking the exit status of the install commands (or enabling set -e / explicit || exit with a clear error message) so failures are surfaced.

It would make sense to check for this kind of errors if we had a way to recover from them. So, the only problem we have is we can tell user that everything is fine when it's actually not. It's a consistent behavior throughout the whole bash code base.

The download/unzip/copy steps aren’t checked for failure (no set -e and no explicit exit-status checks). If curl/unzip fails, the script can continue and still print a successful install message, potentially leaving a broken partial installation. Consider enabling strict error handling (e.g. set -euo pipefail) or adding explicit checks with a clear failure message before proceeding to install steps.

Already answered regarding this kind of issues above. In practice, setup.sh is the most reliable piece of code here. A single pill for all the problems.