From c5ed2d6f90b5d0a1ddc7979a2c7e860118332bba Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 26 Apr 2026 14:37:45 -0700 Subject: [PATCH] fix: pre-landing review auto-fixes (5 correctness + observability) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /ship Step 9 review surfaced 9 INFORMATIONAL findings on the new helper + migration. Five auto-fixed with no behavior regression (26/26 tests pass): bin/gstack-gbrain-source-wireup: - Version compare: put floor "0.18.0" first in `sort -V` stdin so equal-or- greater $v always sorts to position 2. Stable across sort implementations. - _worktree_add_detached: drop `2>/dev/null` on the `worktree add`, surface git's stderr through `prefix` so users see WHY adds fail (disk, perms). - ensure_worktree: same observability fix on the `git checkout --detach` path during HEAD-advance, so users see the actual git error before recovery. - do_probe: replace `[ -d X ] || [ -f X ] && set=present` (precedence trap — the `&&` short-circuits when the dir branch fails) with explicit if-block. - do_probe: capture `check_source_state`'s return code explicitly via `set +e; ...; rc=$?; set -e`. `$?` after an `if`/`elif` chain is fragile under set -e and may not reach the elif under some shell versions. - do_wireup: same explicit return-code capture for `ensure_worktree`. The prior `ensure_worktree || { if [ $? = 2 ]; ...` pattern relied on `$?` reflecting the function's return after `||`, which is implementation-defined. gstack-upgrade/migrations/v1.15.1.0.sh: - Trim whitespace from `gstack-config get gbrain_sync_mode` output via `tr -d '[:space:]'`. Trailing newlines would mis-classify "off\n" as a non-empty non-off mode and incorrectly invoke the helper. Skipped findings (cosmetic / out of scope): - `python3 -c` reads `~/.gbrain/config.json` via `expanduser` instead of the helper's `$GBRAIN_CONFIG` variable (cosmetic; HONORS HOME override). - Long sync-failure error message could truncate to last N lines (cosmetic log readability). Co-Authored-By: Claude Opus 4.7 (1M context) --- bin/gstack-gbrain-source-wireup | 52 ++++++++++++++++++-------- gstack-upgrade/migrations/v1.15.1.0.sh | 4 +- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/bin/gstack-gbrain-source-wireup b/bin/gstack-gbrain-source-wireup index 702384e5..a70d5e22 100755 --- a/bin/gstack-gbrain-source-wireup +++ b/bin/gstack-gbrain-source-wireup @@ -127,8 +127,9 @@ gbrain_version_ok() { local v v=$(gbrain --version 2>/dev/null | awk '{print $2}') [ -z "$v" ] && return 1 - # 0.18.0 minimum (gbrain sources shipped here) - [ "$(printf '%s\n0.18.0\n' "$v" | sort -V | head -1)" = "0.18.0" ] + # 0.18.0 minimum (gbrain sources shipped here). Put the floor first in stdin + # so equal or greater $v sorts to position 2 — head -1 == "0.18.0" iff $v >= floor. + [ "$(printf '0.18.0\n%s\n' "$v" | sort -V | head -1)" = "0.18.0" ] } # ---- worktree management ---- @@ -140,7 +141,9 @@ _worktree_add_detached() { local sha sha=$(git -C "$GSTACK_HOME" rev-parse HEAD 2>/dev/null) || return 1 git -C "$GSTACK_HOME" worktree prune 2>/dev/null || true - git -C "$GSTACK_HOME" worktree add --detach "$WORKTREE" "$sha" 2>/dev/null + # Surface git errors via prefix so users see WHY the add failed (disk, perms, etc). + git -C "$GSTACK_HOME" worktree add --detach "$WORKTREE" "$sha" 2>&1 | prefix + return "${PIPESTATUS[0]}" } ensure_worktree() { @@ -152,7 +155,9 @@ ensure_worktree() { if [ "$NO_PULL" = "0" ]; then local sha sha=$(git -C "$GSTACK_HOME" rev-parse HEAD 2>/dev/null) || return 1 - ( cd "$WORKTREE" && git checkout --detach "$sha" 2>/dev/null ) || { + # Surface checkout errors via prefix so users see WHY the advance failed + # (uncommitted changes in the detached worktree, ref ambiguity, etc). + ( cd "$WORKTREE" && git checkout --detach "$sha" 2>&1 | prefix; exit "${PIPESTATUS[0]}" ) || { warn "worktree at $WORKTREE could not advance to $sha; resetting via remove + re-add" git -C "$GSTACK_HOME" worktree remove --force "$WORKTREE" 2>/dev/null || rm -rf "$WORKTREE" _worktree_add_detached || return 1 @@ -185,14 +190,23 @@ check_source_state() { do_probe() { local id worktree_status="absent" gbrain_status="missing" source_status="absent" id=$(derive_source_id 2>/dev/null) || id="(unknown)" - [ -d "$WORKTREE/.git" ] || [ -f "$WORKTREE/.git" ] && worktree_status="present" + # Use explicit if-block so [ -d ] || [ -f ] doesn't get short-circuited by && + # precedence (the `||` and `&&` chain has trap behavior in bash test syntax). + if [ -d "$WORKTREE/.git" ] || [ -f "$WORKTREE/.git" ]; then + worktree_status="present" + fi if gbrain_version_ok; then gbrain_status="ok ($(gbrain --version 2>/dev/null | awk '{print $2}'))" - if check_source_state "$id"; then - source_status="registered ($WORKTREE)" - elif [ $? = 1 ]; then - source_status="registered (different path)" - fi + # Capture check_source_state's return code explicitly. Relying on $? after + # an `if`-elif chain is fragile under set -e and undefined under some shells. + set +e + check_source_state "$id" + local css_rc=$? + set -e + case "$css_rc" in + 0) source_status="registered ($WORKTREE)" ;; + 1) source_status="registered (different path)" ;; + esac fi echo "source_id=$id" echo "worktree=$WORKTREE" @@ -213,14 +227,22 @@ do_wireup() { exit 0 fi - ensure_worktree || { - if [ $? = 2 ]; then + # Capture ensure_worktree's return code explicitly. `$?` after `||` reflects + # the LAST command in the function under set -e, which is unreliable when the + # function has multiple internal exit paths. + set +e + ensure_worktree + ew_rc=$? + set -e + case "$ew_rc" in + 0) : ;; # success + 2) [ "$STRICT" = "1" ] && die "no $GSTACK_HOME/.git; run /setup-gbrain Step 7 (gstack-brain-init) first" 2 warn "no $GSTACK_HOME/.git; skipping (benign skip)" exit 0 - fi - die "git worktree creation failed at $WORKTREE" 1 - } + ;; + *) die "git worktree creation failed at $WORKTREE" 1 ;; + esac # Source registration: probe state, then act. set +e diff --git a/gstack-upgrade/migrations/v1.15.1.0.sh b/gstack-upgrade/migrations/v1.15.1.0.sh index cde74339..6ac6ec88 100755 --- a/gstack-upgrade/migrations/v1.15.1.0.sh +++ b/gstack-upgrade/migrations/v1.15.1.0.sh @@ -29,7 +29,9 @@ WIREUP_BIN="${BIN_DIR}/gstack-gbrain-source-wireup" # Skip if user never opted into brain-sync. SYNC_MODE="" if [ -x "$CONFIG_BIN" ]; then - SYNC_MODE=$("$CONFIG_BIN" get gbrain_sync_mode 2>/dev/null || echo "") + # Trim whitespace defensively: gstack-config can emit trailing newlines, + # which would mis-classify "off\n" as a non-empty non-off mode. + SYNC_MODE=$("$CONFIG_BIN" get gbrain_sync_mode 2>/dev/null | tr -d '[:space:]' || echo "") fi if [ "$SYNC_MODE" = "off" ] || [ -z "$SYNC_MODE" ]; then exit 0