Skip to content

install: fix alias/function parse collision and PS profile discovery#54

Merged
BrettKinny merged 2 commits into
mainfrom
claude/debug-aliases-photo-sync-LH8CV
Apr 10, 2026
Merged

install: fix alias/function parse collision and PS profile discovery#54
BrettKinny merged 2 commits into
mainfrom
claude/debug-aliases-photo-sync-LH8CV

Conversation

@BrettKinny
Copy link
Copy Markdown
Collaborator

Two regressions from the past ~2 days caused shell integration to break:

Bug 1 — Linux: bash syntax error at source time
Commit 646a589 switched from writing alias sqrbx='...' to writing shell
functions (sqrbx() { ... }), but _add_shell_func only guarded against
re-adding functions. Legacy alias lines from pre-646a589 installs stayed
in .bashrc, and when bash sourced the file interactively (expand_aliases
on), alias expansion happened at parse time — turning sqrbx() into
docker start -ai squarebox() and producing:

syntax error near unexpected token `('

Bug 2 — Windows: PowerShell profile never updated

  • $LOCALAPPDATA/Microsoft/PowerShell/pwsh.exe was missing the /7/
    component, so the per-user install never got found.
  • Microsoft Store install path was not checked.
  • Commit 55f9183 gated every diagnostic behind --verbose, so a silent
    skip gave users zero feedback.
  • The PS sqrbx-rebuild function invoked bare bash, which can resolve
    to WSL bash instead of Git Bash when launched from PowerShell.

Fix: managed block + dedicated sourced file

Mirror the Dockerfile pattern (~/.squarebox-*-aliases sourced from
~/.bashrc). install.sh now:

  • Writes the four function bodies to ~/.squarebox-shell-init, overwritten
    unconditionally on every run (plus an inline unalias as belt-and-
    suspenders against any alias shadowing).
  • Scrubs legacy content from $SHELL_RC via portable awk + mktemp + mv:
    removes any prior # >>> squarebox >>> block, legacy standalone
    alias sqrbx=... lines, and legacy one-liner function defs from the
    646a589-era writer.
  • Appends a stable one-line sentinel block to $SHELL_RC that sources
    the init file.
  • Runs bash -n $SHELL_RC after editing as a self-check so future
    regressions that break the rc file are caught at install time.
  • Forces $SHELL_RC=$HOME/.bashrc when MSYSTEM is set so rebuilds
    launched from a parent PowerShell target the right file.

For PowerShell:

  • Discovery order: command -v → where.exe pwsh → file probes covering
    Program Files/PowerShell/7, LOCALAPPDATA/Microsoft/PowerShell/7, and
    LOCALAPPDATA/Microsoft/WindowsApps.
  • $PROFILE capture uses [Console]::Out.Write plus BOM + CR stripping
    to avoid trailing-newline and UTF-8 BOM contamination.
  • Sanity-checks the returned profile path contains a Documents/PowerShell
    or Documents/WindowsPowerShell component before using it.
  • Applies the same managed-block pattern (awk scrub + sentinel append)
    to the PS profile.
  • PowerShell sqrbx-rebuild now explicitly resolves Git Bash at runtime
    (Program Files/Git/bin/bash.exe, with PATH fallback) so it never runs
    install.sh under WSL.
  • Un-gates the "pwsh not found, skipping" message so Windows users
    always see feedback.

Also updates .github/workflows/e2e.yml 1.4 to exercise the real
managed-block logic from install.sh against a fixture containing the
exact legacy cruft that triggered Bug 1 — a permanent regression guard.

Two regressions from the past ~2 days caused shell integration to break:

Bug 1 — Linux: bash syntax error at source time
Commit 646a589 switched from writing `alias sqrbx='...'` to writing shell
functions (`sqrbx() { ... }`), but `_add_shell_func` only guarded against
re-adding functions. Legacy alias lines from pre-646a589 installs stayed
in .bashrc, and when bash sourced the file interactively (expand_aliases
on), alias expansion happened at parse time — turning `sqrbx()` into
`docker start -ai squarebox()` and producing:

  syntax error near unexpected token `('

Bug 2 — Windows: PowerShell profile never updated
- `$LOCALAPPDATA/Microsoft/PowerShell/pwsh.exe` was missing the `/7/`
  component, so the per-user install never got found.
- Microsoft Store install path was not checked.
- Commit 55f9183 gated every diagnostic behind --verbose, so a silent
  skip gave users zero feedback.
- The PS `sqrbx-rebuild` function invoked bare `bash`, which can resolve
  to WSL bash instead of Git Bash when launched from PowerShell.

Fix: managed block + dedicated sourced file

Mirror the Dockerfile pattern (~/.squarebox-*-aliases sourced from
~/.bashrc). install.sh now:

- Writes the four function bodies to ~/.squarebox-shell-init, overwritten
  unconditionally on every run (plus an inline `unalias` as belt-and-
  suspenders against any alias shadowing).
- Scrubs legacy content from $SHELL_RC via portable awk + mktemp + mv:
  removes any prior `# >>> squarebox >>>` block, legacy standalone
  `alias sqrbx=...` lines, and legacy one-liner function defs from the
  646a589-era writer.
- Appends a stable one-line sentinel block to $SHELL_RC that sources
  the init file.
- Runs `bash -n $SHELL_RC` after editing as a self-check so future
  regressions that break the rc file are caught at install time.
- Forces `$SHELL_RC=$HOME/.bashrc` when MSYSTEM is set so rebuilds
  launched from a parent PowerShell target the right file.

For PowerShell:
- Discovery order: command -v → where.exe pwsh → file probes covering
  Program Files/PowerShell/7, LOCALAPPDATA/Microsoft/PowerShell/7, and
  LOCALAPPDATA/Microsoft/WindowsApps.
- $PROFILE capture uses `[Console]::Out.Write` plus BOM + CR stripping
  to avoid trailing-newline and UTF-8 BOM contamination.
- Sanity-checks the returned profile path contains a Documents/PowerShell
  or Documents/WindowsPowerShell component before using it.
- Applies the same managed-block pattern (awk scrub + sentinel append)
  to the PS profile.
- PowerShell `sqrbx-rebuild` now explicitly resolves Git Bash at runtime
  (Program Files/Git/bin/bash.exe, with PATH fallback) so it never runs
  install.sh under WSL.
- Un-gates the "pwsh not found, skipping" message so Windows users
  always see feedback.

Also updates .github/workflows/e2e.yml 1.4 to exercise the real
managed-block logic from install.sh against a fixture containing the
exact legacy cruft that triggered Bug 1 — a permanent regression guard.
Copilot AI review requested due to automatic review settings April 10, 2026 01:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes recent shell-integration regressions on Linux (bash alias/function parse collision) and Windows (PowerShell profile discovery/update not happening) by switching to a managed-block + dedicated init file approach and improving PowerShell detection/profile handling.

Changes:

  • Writes shell functions into ~/.squarebox-shell-init and updates .bashrc/.zshrc via a scrubbed managed sentinel block, with a post-edit syntax check.
  • Improves PowerShell (pwsh) discovery and profile update logic, and makes sqrbx-rebuild explicitly use Git Bash.
  • Updates E2E workflow to validate the managed-block scrub/append logic against a legacy-cruft fixture.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
install.sh Reworks bash/zsh integration into a sourced init file + managed sentinel; improves PowerShell discovery and profile managed-block updates.
.github/workflows/e2e.yml Extends E2E to run the real shell-integration block against a legacy .bashrc fixture and assert idempotency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread install.sh
Comment on lines +168 to +170
cat >> "$SHELL_RC" <<'SQRBXRCEOF'
# >>> squarebox >>>
[ -f "$HOME/.squarebox-shell-init" ] && . "$HOME/.squarebox-shell-init"
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQRBX_INIT is written under USER_HOME, but the sentinel added to $SHELL_RC sources "$HOME/.squarebox-shell-init". On Git Bash/MSYS2 where USER_HOME (derived from USERPROFILE) differs from HOME (/home/user), this will leave the init file unsourced and shell integration will not work. Make the init file path consistent (e.g., write it under $HOME, or have the sentinel source the USER_HOME location).

Suggested change
cat >> "$SHELL_RC" <<'SQRBXRCEOF'
# >>> squarebox >>>
[ -f "$HOME/.squarebox-shell-init" ] && . "$HOME/.squarebox-shell-init"
cat >> "$SHELL_RC" <<SQRBXRCEOF
# >>> squarebox >>>
[ -f "$SQRBX_INIT" ] && . "$SQRBX_INIT"

Copilot uses AI. Check for mistakes.
Comment thread install.sh Outdated
Comment on lines +143 to +150
cat > "$SQRBX_INIT" <<'SQRBXEOF'
# Managed by squarebox install.sh — overwritten on every install.
# Drop any stale aliases with these names so they don't shadow the functions.
unalias sqrbx squarebox sqrbx-rebuild squarebox-rebuild 2>/dev/null || true
sqrbx() { if command -v winpty &>/dev/null && [[ -n "${MSYSTEM:-}" ]]; then winpty docker start -ai squarebox; else docker start -ai squarebox; fi; }
squarebox() { sqrbx "$@"; }
sqrbx-rebuild() { "$HOME/squarebox/install.sh" "$@"; }
squarebox-rebuild() { sqrbx-rebuild "$@"; }
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated sqrbx-rebuild function runs "$HOME/squarebox/install.sh", but this script installs into "$INSTALL_DIR" which is based on USER_HOME (USERPROFILE on Windows). When HOME != USER_HOME (common in Git Bash), sqrbx-rebuild will point at a non-existent path. Consider emitting the resolved install.sh path (INSTALL_DIR/install.sh) into the init file, or computing it at runtime using USERPROFILE+cygpath.

Suggested change
cat > "$SQRBX_INIT" <<'SQRBXEOF'
# Managed by squarebox install.sh — overwritten on every install.
# Drop any stale aliases with these names so they don't shadow the functions.
unalias sqrbx squarebox sqrbx-rebuild squarebox-rebuild 2>/dev/null || true
sqrbx() { if command -v winpty &>/dev/null && [[ -n "${MSYSTEM:-}" ]]; then winpty docker start -ai squarebox; else docker start -ai squarebox; fi; }
squarebox() { sqrbx "$@"; }
sqrbx-rebuild() { "$HOME/squarebox/install.sh" "$@"; }
squarebox-rebuild() { sqrbx-rebuild "$@"; }
cat > "$SQRBX_INIT" <<SQRBXEOF
# Managed by squarebox install.sh — overwritten on every install.
# Drop any stale aliases with these names so they don't shadow the functions.
unalias sqrbx squarebox sqrbx-rebuild squarebox-rebuild 2>/dev/null || true
sqrbx() { if command -v winpty &>/dev/null && [[ -n "\${MSYSTEM:-}" ]]; then winpty docker start -ai squarebox; else docker start -ai squarebox; fi; }
squarebox() { sqrbx "\$@"; }
sqrbx-rebuild() { "${INSTALL_DIR}/install.sh" "\$@"; }
squarebox-rebuild() { sqrbx-rebuild "\$@"; }

Copilot uses AI. Check for mistakes.
Comment thread install.sh Outdated
Comment on lines 177 to 179
if ! bash -n "$SHELL_RC" 2>/dev/null; then
echo "Warning: $SHELL_RC fails bash syntax check after edit — please inspect." >&2
fi
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The self-check always runs bash -n "$SHELL_RC", but SHELL_RC can be ~/.zshrc when the user’s shell is zsh. Valid zshrc content may fail a bash syntax check and cause a misleading warning on every install. Consider gating this to bash rc files, or using the appropriate shell syntax check (e.g., zsh -n) when SHELL_RC is a zsh file.

Suggested change
if ! bash -n "$SHELL_RC" 2>/dev/null; then
echo "Warning: $SHELL_RC fails bash syntax check after edit — please inspect." >&2
fi
case "$SHELL_RC" in
*.bashrc|*.bash_profile|*/.bashrc|*/.bash_profile)
if ! bash -n "$SHELL_RC" 2>/dev/null; then
echo "Warning: $SHELL_RC fails bash syntax check after edit — please inspect." >&2
fi
;;
*.zshrc|*.zprofile|*/.zshrc|*/.zprofile)
if command -v zsh >/dev/null 2>&1 && ! zsh -n "$SHELL_RC" 2>/dev/null; then
echo "Warning: $SHELL_RC fails zsh syntax check after edit — please inspect." >&2
fi
;;
esac

Copilot uses AI. Check for mistakes.
Comment thread install.sh Outdated
# Sanity check: a real profile path lives under Documents/PowerShell or
# Documents/WindowsPowerShell. Guards against stray bytes producing bogus paths.
case "$_ps_profile" in
*/Documents/PowerShell/*|*/Documents/WindowsPowerShell/*|*/PowerShell/*) ;;
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says the PowerShell profile sanity check should only accept paths under Documents/PowerShell or Documents/WindowsPowerShell, but the case pattern also allows */PowerShell/*, which is much broader and weakens the guard against bogus paths. Either tighten the allowed patterns to match the stated intent, or update the comment to reflect the broader acceptance criteria.

Suggested change
*/Documents/PowerShell/*|*/Documents/WindowsPowerShell/*|*/PowerShell/*) ;;
*/Documents/PowerShell/*|*/Documents/WindowsPowerShell/*) ;;

Copilot uses AI. Check for mistakes.
Comment thread .github/workflows/e2e.yml
Comment on lines +73 to +76
export HOME="$(mktemp -d)"
export USER_HOME="$HOME"
SHELL_RC="$HOME/.bashrc"
cat > "$SHELL_RC" <<'LEGACY'
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow fixture sets USER_HOME="$HOME", so it won’t catch regressions where USER_HOME and HOME differ (the exact scenario called out in install.sh for MSYS2/Git Bash). Consider setting USER_HOME to a different temp dir than HOME and asserting that the init file is written/sourced from the correct location, so the test would fail if these paths diverge again.

Copilot uses AI. Check for mistakes.
Five review comments, all valid:

1. Init file was written under $USER_HOME but the sentinel in $SHELL_RC
   sources "$HOME/.squarebox-shell-init" — on Git Bash these diverge
   (USER_HOME=C:/Users/..., HOME=/home/user) and the init file would be
   silently unsourced. Write it under $HOME to match the running shell.

2. sqrbx-rebuild body used "$HOME/squarebox/install.sh" but the clone
   lives at "$INSTALL_DIR" (USER_HOME-based). On Git Bash that resolves
   to /home/user/squarebox/install.sh — which doesn't exist. Switch the
   heredoc to unquoted and bake $INSTALL_DIR into the function body at
   install time (escaping $@ and ${MSYSTEM:-} so they stay literal and
   get expanded at runtime by the user's shell).

3. Post-edit self-check always ran `bash -n`, but SHELL_RC can be a
   .zshrc that uses zsh-only syntax (setopt, glob qualifiers) which
   would trip bash -n and emit misleading warnings for zsh users on
   every install. Gate by filename: bash -n for .bashrc/.bash_profile,
   zsh -n for .zshrc/.zprofile (when zsh is available).

4. PowerShell profile sanity-check case pattern included `*/PowerShell/*`
   which is much broader than the "Documents/PowerShell" stated intent
   and weakens the guard against garbage bytes. Tighten to the two
   Documents patterns.

5. The e2e fixture set USER_HOME=$HOME, so it couldn't catch regressions
   where the two diverge — which is exactly the scenario that produced
   comments 1 and 2. Use two distinct mktemp dirs and add explicit
   assertions that the init file lives under HOME (not USER_HOME), that
   sqrbx-rebuild references $INSTALL_DIR (not $HOME/squarebox), and that
   $@ / ${MSYSTEM:-} are preserved as literals in the init file.

Also document the HOME/USER_HOME distinction in CLAUDE.md so future
changes don't reintroduce the confusion.
@BrettKinny BrettKinny merged commit 379cec4 into main Apr 10, 2026
1 check passed
@BrettKinny BrettKinny deleted the claude/debug-aliases-photo-sync-LH8CV branch April 10, 2026 02:29
BrettKinny pushed a commit that referenced this pull request Apr 10, 2026
The PowerShell alias bug has been "fixed" 5+ times (#42, #44, #45, #53,
#54, #55) because install.sh tried to detect pwsh from Git Bash — a
fundamentally fragile approach (MSYS2 PATH isolation, Store reparse
points, CRLF/BOM contamination, etc.).

New approach: let PowerShell handle its own profile natively.
install.ps1 writes the profile using $PROFILE directly (trivially
reliable), then delegates Docker/bash setup to install.sh --no-pwsh.
install.sh no longer attempts cross-shell detection — it just prints
instructions pointing to install.ps1.

Net: 96 lines of brittle detection code removed from install.sh,
replaced by a clean 95-line install.ps1 and a 7-line pointer message.

https://claude.ai/code/session_01PaJQ2YfgmEg4ZXiCsav24B
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants