From c9061b27a8b8f2ca3e7226ed44b422948e8405aa Mon Sep 17 00:00:00 2001
From: Krzysztof kuhy Rudnicki <krzysztofrudnicki0@gmail.com>
Date: Sat, 29 Aug 2026 13:07:28 +0200
Subject: [PATCH] fix: make shellcheck agree between local and CI, and fix the
 group picker

CI failed on shellcheck where the local hook passed: the runner's shellcheck
reports SC2015 (`A && B || C` is not if-then-else) and mine did not. Both now
run with --severity=style so a version skew cannot pass locally and fail in
CI. The cleanup traps are plain ifs, which is what the finding was about --
`kill` on an already-dead pid makes B fail and C run while A is true.

live_smoke.sh choose_group had two bugs that only surface when it is actually
run, neither reachable from the offline suite:
- it fetched /v1/groups twice, once to display and once to resolve the picked
  index, so an unstable order would hand back a different group than the one
  shown. Fetched once into a variable now.
- `read -r -p` wrote its prompt to stdout, which the caller captures with
  $(...), so the prompt text would have been swallowed into the group id.
  Prompt and listing both go to stderr; stdout carries only the id.
Also validates that the pick is a number and in range.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yaBT45KhHqxTKVCTA87iy
---
 .github/workflows/ci.yml |  4 +++-
 .pre-commit-config.yaml  |  4 +++-
 scripts/e2e.sh           | 16 ++++++++++++----
 scripts/live_smoke.sh    | 32 ++++++++++++++++++++++++--------
 4 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 6981006..2371f37 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -36,7 +36,9 @@ jobs:
         run: .venv/bin/mypy signal_bot fake_signal
 
       - name: shellcheck
-        run: shellcheck run.sh scripts/*.sh
+        # --severity=style is the floor CI enforces; the local pre-commit hook
+        # uses the same flag so a version skew cannot pass locally and fail here.
+        run: shellcheck --severity=style run.sh scripts/*.sh
 
       - name: pytest (100% branch coverage)
         run: .venv/bin/python -m pytest -q
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index c93176d..c8aea1a 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -77,7 +77,9 @@ repos:
 
       - id: shellcheck
         name: shellcheck
-        entry: shellcheck
+        # Same severity floor as CI: a stricter runner version must not be
+        # the thing that discovers a finding this hook let through.
+        entry: shellcheck --severity=style
         language: system
         types: [shell]
 
diff --git a/scripts/e2e.sh b/scripts/e2e.sh
index 9e6e9e2..9a9b5e7 100755
--- a/scripts/e2e.sh
+++ b/scripts/e2e.sh
@@ -28,11 +28,19 @@ BOT_PID=""
 TEMP_DIR=""
 
 cleanup() {
-    [[ -n "$BOT_PID" ]] && kill "$BOT_PID" 2>/dev/null || true
-    [[ -n "$FAKE_PID" ]] && kill "$FAKE_PID" 2>/dev/null || true
+    # Plain ifs, not `A && B || C`: that is not if-then-else, and C runs when
+    # A is true but B fails -- which is exactly the case here (kill on an
+    # already-dead pid).
+    if [[ -n "$BOT_PID" ]]; then
+        kill "$BOT_PID" 2>/dev/null || true
+    fi
+    if [[ -n "$FAKE_PID" ]]; then
+        kill "$FAKE_PID" 2>/dev/null || true
+    fi
     wait 2>/dev/null || true
-    [[ -n "$TEMP_DIR" && -d "$TEMP_DIR" ]] && rm -rf "$TEMP_DIR"
-    return 0
+    if [[ -n "$TEMP_DIR" && -d "$TEMP_DIR" ]]; then
+        rm -rf "$TEMP_DIR"
+    fi
 }
 trap cleanup EXIT
 
diff --git a/scripts/live_smoke.sh b/scripts/live_smoke.sh
index 264188d..34c75d3 100755
--- a/scripts/live_smoke.sh
+++ b/scripts/live_smoke.sh
@@ -30,8 +30,9 @@ readonly FALLBACK_IMAGE="${FALLBACK_IMAGE:-bbernhard/signal-cli-rest-api:1787779
 TEMP_DIR=""
 
 cleanup() {
-    [[ -n "$TEMP_DIR" && -d "$TEMP_DIR" ]] && rm -rf "$TEMP_DIR"
-    return 0
+    if [[ -n "$TEMP_DIR" && -d "$TEMP_DIR" ]]; then
+        rm -rf "$TEMP_DIR"
+    fi
 }
 trap cleanup EXIT
 
@@ -106,15 +107,30 @@ wait_for_link() {
 }
 
 choose_group() {
-    local number="$1"
+    local number="$1" groups choice id
+    # Fetched ONCE: two separate calls could return a different order, so the
+    # index the user picks would resolve against a different list.
+    groups="$(curl -sf "${API}/v1/groups/${number}")"
+
+    # Every prompt goes to stderr: this function's stdout is captured by $(...)
+    # and must contain nothing but the chosen group id.
     echo "== groups on this account:" >&2
-    curl -sf "${API}/v1/groups/${number}" \
-        | jq -r '.[] | "\(.id)\t\(.name // "(unnamed)")"' \
+    printf '%s' "$groups" | jq -r '.[] | "\(.id)\t\(.name // "(unnamed)")"' \
         | nl -w3 -s'  ' >&2
     echo >&2
-    read -r -p "Pick the number of the PRIVATE group to use: " choice
-    curl -sf "${API}/v1/groups/${number}" \
-        | jq -r --argjson n "$choice" '.[$n - 1].id'
+
+    read -r -p "Pick the number of the PRIVATE group to use: " choice </dev/tty >&2
+    if ! [[ "$choice" =~ ^[0-9]+$ ]]; then
+        echo "Error: '$choice' is not a number" >&2
+        return 1
+    fi
+
+    id="$(printf '%s' "$groups" | jq -r --argjson n "$choice" '.[$n - 1].id // empty')"
+    if [[ -z "$id" ]]; then
+        echo "Error: no group at position $choice" >&2
+        return 1
+    fi
+    printf '%s' "$id"
 }
 
 write_env() {