From 46cfc4ada5d9efe849d0f789178e492d98801011 Mon Sep 17 00:00:00 2001 From: Spencer Bull Date: Wed, 2 Sep 2026 01:01:13 -0500 Subject: [PATCH] Judge the CLI teardown by what is left, and match flags as definitions Two review follow-ups. The probe counted any mention of --tui/--query in the help as support -- Hermes already writes "With --tui:" into --dev's description, so a release that dropped the option while keeping the prose would still read as ready. A flag now counts only when followed by a shape argparse prints after a definition: the usage bracket, the gap before same-line help text, an uppercase metavar, or the line end. Not probed by parsing a real invocation on purpose -- a release that ignores unknown arguments would turn the probe into a live session. And the teardown trusted its commands: a stub rm that failed aborted Remove Hermes under set -e before any ~/.hermes handling, while mise failures vanished into || true. --remove now attempts every step, then judges by what is left -- the marked stub still present, or mise still resolving the tool -- and Remove Hermes tolerates the failure until the runtime is handled, then carries it in its exit code. Findings from the same codex review at xhigh, verified and proven by mutation before landing. Co-Authored-By: Codex Co-Authored-By: Claude --- bin/omarchy-install-hermes-cli | 32 ++++++++++++++++++++------- bin/omarchy-remove-ai-hermes | 11 +++++++++- test/shell.d/hermes-cli-test.sh | 35 +++++++++++++++++++++++++++++- test/shell.d/hermes-remove-test.sh | 13 +++++++++++ 4 files changed, 81 insertions(+), 10 deletions(-) diff --git a/bin/omarchy-install-hermes-cli b/bin/omarchy-install-hermes-cli index 4a6923a3..35405269 100755 --- a/bin/omarchy-install-hermes-cli +++ b/bin/omarchy-install-hermes-cli @@ -88,21 +88,27 @@ hermes_runs() { timeout 15 "$HOME/.local/bin/hermes" --version >/dev/null 2>&1 } +# A flag counts only when the help defines it, not whenever it is mentioned: +# what follows must be a shape argparse prints after a definition -- the usage +# line's closing bracket, the gap before same-line help text, an uppercase +# metavar, or the end of the line. Prose like "With --tui: run ..." stays +# prose, and --tui-theme or --tui_mode never answers for --tui. Not probed by +# parsing an actual invocation on purpose: a release that ignores unknown +# arguments would turn the probe into a live session. +help_defines_flag() { + grep -qE -- "$1(]|[[:space:]][[:upper:]]|[[:space:]]{2}|$)" <<<"$2" +} + # Probed for the flags omarchy-agent actually passes -- --query to seed the # session and --tui to keep it interactive -- rather than a flag that merely # shipped alongside them: the previous marker, --oneshot, was dropped in Hermes # v0.20 and left working installs stranded as "not ready". -# -# Matched at a flag boundary, not as a substring: a bare grep for --tui also -# accepts a release that lists only --tui-theme while having dropped --tui -# itself, which would call an install ready that omarchy-agent cannot drive -- -# the same false verdict this check exists to prevent, merely inverted. hermes_prompt_ready() { local help hermes_runs && help=$(timeout 15 "$HOME/.local/bin/hermes" chat --help 2>/dev/null) && - grep -qE -- '--tui([^[:alnum:]_-]|$)' <<<"$help" && - grep -qE -- '--query([^[:alnum:]_-]|$)' <<<"$help" + help_defines_flag '--tui' "$help" && + help_defines_flag '--query' "$help" } # --owns answers whether the wrapper on PATH is the one this command wrote, so @@ -127,7 +133,17 @@ if [[ $mode == "--remove" ]]; then if ours; then mise rm -g "$tool" >/dev/null 2>&1 || true mise uninstall --all "$tool" >/dev/null 2>&1 || true - rm -f "$HOME/.local/bin/hermes" + rm -f "$HOME/.local/bin/hermes" 2>/dev/null || true + + # Every step is attempted before any is judged, and judged by what is left + # rather than by what the commands claimed: the marked stub still answering + # hermes, or mise still resolving the tool, is a CLI still installed no + # matter how the removal exited. + if ours || mise where "$tool" >/dev/null 2>&1; then + echo "Could not remove the Hermes CLI Omarchy installed." >&2 + echo "Remove ~/.local/bin/hermes and the mise tool '$tool', then run omarchy-install-hermes-cli --remove again." >&2 + exit 1 + fi fi exit 0 diff --git a/bin/omarchy-remove-ai-hermes b/bin/omarchy-remove-ai-hermes index f332dc4b..1145fbd1 100755 --- a/bin/omarchy-remove-ai-hermes +++ b/bin/omarchy-remove-ai-hermes @@ -13,7 +13,10 @@ omarchy-pkg-drop hermes-desktop # the app never superseded -- an interrupted install, or the terminal CLI from # before the app existed. Remove Hermes clears that too, scoped by the installer # to what Omarchy owns so a hermes the user set up themselves is left alone. -omarchy-install-hermes-cli --remove +# Tolerated here rather than fatal, so the ~/.hermes handling below still runs; +# the failure is answered for at the end instead of being swallowed. +cli_removed=true +omarchy-install-hermes-cli --remove || cli_removed=false # The app writes this when the runtime it provisions under ~/.hermes has landed, # and it is the only thing that tells that runtime apart from one the user @@ -64,3 +67,9 @@ else echo "Hermes Desktop has been removed." echo "It never finished installing its own Hermes, so nothing in ~/.hermes was touched." fi + +# The messages above still hold -- the app and its runtime are gone -- but a CLI +# teardown that failed already said so on stderr, and that stands. +if [[ $cli_removed == "false" ]]; then + exit 1 +fi diff --git a/test/shell.d/hermes-cli-test.sh b/test/shell.d/hermes-cli-test.sh index 69a52f61..24f8e8bc 100755 --- a/test/shell.d/hermes-cli-test.sh +++ b/test/shell.d/hermes-cli-test.sh @@ -430,12 +430,19 @@ pass "--remove is idempotent when no Hermes CLI is present" printf '%s\n' "#!/bin/bash" "$stub_marker" >"$remove_home/.local/bin/hermes" chmod +x "$remove_home/.local/bin/hermes" : >"$mise_log" -OMARCHY_TEST_MISE_WHERE_OK=1 run_remove || fail "--remove succeeds tearing down an owned CLI" +run_remove || fail "--remove succeeds tearing down an owned CLI" tr '\0' '\n' <"$mise_log" | grep -q '^rm$' || fail "--remove drops the mise tool from config" tr '\0' '\n' <"$mise_log" | grep -q '^uninstall$' || fail "--remove uninstalls the mise tool" [[ ! -e $remove_home/.local/bin/hermes ]] || fail "--remove takes the stub it owns" pass "--remove tears down the mise CLI and the stub this installer owns" +# When mise still resolves the tool after the teardown, the environment +# survived whatever uninstall claimed, and --remove has to say so. +printf '%s\n' "#!/bin/bash" "$stub_marker" >"$remove_home/.local/bin/hermes" +chmod +x "$remove_home/.local/bin/hermes" +OMARCHY_TEST_MISE_WHERE_OK=1 run_remove && fail "--remove claims success while mise still resolves the tool" +pass "--remove fails when the mise environment survives the teardown" + foreign_remove_body="#!/bin/bash exec /usr/local/bin/my-own-hermes \"\$@\"" printf '%s\n' "$foreign_remove_body" >"$remove_home/.local/bin/hermes" @@ -450,6 +457,16 @@ tr '\0' '\n' <"$mise_log" | grep -Eq '^(rm|uninstall)$' && fail "--remove never removes a mise environment it cannot prove is Omarchy's" pass "--remove leaves a Hermes the user installed themselves" +# Judged by what is left, not by what rm claimed: a stub that survives the +# teardown is a CLI still installed, and --remove has to say so. +printf '%s\n' "#!/bin/bash" "$stub_marker" >"$remove_home/.local/bin/hermes" +chmod +x "$remove_home/.local/bin/hermes" +chmod 555 "$remove_home/.local/bin" +run_remove && fail "--remove claims success while the stub survives" +chmod 755 "$remove_home/.local/bin" +rm -f "$remove_home/.local/bin/hermes" +pass "--remove fails when the stub cannot be removed" + # The app's marker says its install once landed, not that it is still there. A # wrapper whose runtime has since gone answers for nothing, so readiness runs # the command, exactly as it does for a hermes the user installed themselves. @@ -541,3 +558,19 @@ SH chmod +x "$ready_home/.hermes/hermes-agent/venv/bin/hermes" run_ready_check && fail "--check accepts flags that extend --tui/--query with an underscore" pass "an underscore continuation is not the bare flag" + +# A flag mentioned in another option's help text is not that option. Hermes +# already writes "With --tui:" into --dev's description, so prose has to stay +# prose even when both names appear in it. +cat >"$ready_home/.hermes/hermes-agent/venv/bin/hermes" <<'SH' +#!/bin/bash +if [[ ${1:-} == "chat" && ${2:-} == "--help" ]]; then + echo " --dev With --tui: run sources via tsx" + echo " --log FILE Where --query output lands" +else + echo "hermes-agent 0.0.0-test" +fi +SH +chmod +x "$ready_home/.hermes/hermes-agent/venv/bin/hermes" +run_ready_check && fail "--check accepts flags that appear only in option descriptions" +pass "a flag mentioned in prose is not a defined option" diff --git a/test/shell.d/hermes-remove-test.sh b/test/shell.d/hermes-remove-test.sh index 6045c9a1..d3389087 100755 --- a/test/shell.d/hermes-remove-test.sh +++ b/test/shell.d/hermes-remove-test.sh @@ -22,6 +22,7 @@ SH cat >"$mock_bin/omarchy-install-hermes-cli" <<'SH' #!/bin/bash printf '%s\0' "$@" >>"$OMARCHY_TEST_INSTALLER_LOG" +exit "${OMARCHY_TEST_INSTALLER_STATUS:-0}" SH chmod +x "$mock_bin"/* @@ -46,6 +47,7 @@ remove() { : >"$test_tmp/installer-log" OMARCHY_TEST_DROP_LOG="$test_tmp/drop-log" \ OMARCHY_TEST_INSTALLER_LOG="$test_tmp/installer-log" \ + OMARCHY_TEST_INSTALLER_STATUS="${OMARCHY_TEST_INSTALLER_STATUS:-0}" \ HOME="$test_home" PATH="$mock_bin:$PATH" \ bash "$ROOT/bin/omarchy-remove-ai-hermes" >/dev/null 2>&1 } @@ -131,3 +133,14 @@ remove || fail "remove succeeds with a wrapper pointing at a sibling directory" [[ -f $test_home/.local/bin/hermes && $(cat "$test_home/.local/bin/hermes") == "$sibling_body" ]] || fail "a wrapper pointing at ~/xhermes is not mistaken for one pointing into ~/.hermes" pass "removal matches the runtime path as a plain string" + +# A CLI teardown that fails must not stop the runtime handling, and must not be +# papered over either: the data work still happens, and the failure reaches the +# caller's exit code. +seed_install +printf '%s\n' "#!/bin/bash" "exec $test_home/.hermes/hermes-agent/venv/bin/hermes \"\$@\"" \ + >"$test_home/.local/bin/hermes" +OMARCHY_TEST_INSTALLER_STATUS=1 remove && fail "a failed CLI teardown surfaces in the exit code" +[[ ! -d $test_home/.hermes/hermes-agent ]] || + fail "a failed CLI teardown does not stop the runtime removal" +pass "a failed CLI teardown is reported after the runtime is handled"