Shell Review

Table of Contents

• Quick Start
• When to Use
• Required TodoWrite Items
• Workflow
• Output Format

Shell Script Review

Audit shell scripts for correctness, safety, and portability.

Verification

After review, run shellcheck <script> to verify fixes address identified issues.

Testing

Run pytest plugins/pensive/tests/skills/test_shell_review.py -v to validate review patterns.

Quick Start

/shell-review path/to/script.sh

When To Use

• CI/CD pipeline scripts
• Git hook scripts
• Wrapper scripts (run-*.sh)
• Build automation scripts
• Pre-commit hook implementations

When NOT To Use

• Non-shell scripts (Python, JS, etc.)
• One-liner commands that don't need review

Required TodoWrite Items

1. shell-review:context-mapped 2. shell-review:exit-codes-checked 3. shell-review:portability-checked 4. shell-review:safety-patterns-verified 5. shell-review:structure-checked 6. shell-review:evidence-logged 7. shell-review:findings-verified

Workflow

Step 1: Map Context (shell-review:context-mapped)

Identify shell scripts:

# Find shell scripts
find . -not -path "*/.venv/*" -not -path "*/__pycache__/*" \
  -not -path "*/node_modules/*" -not -path "*/.git/*" \
  -name "*.sh" -type f | head -20
# Check shebangs
rg -l "^#!/" scripts/ hooks/ 2>/dev/null | head -10
# fallback: grep -l "^#!/" scripts/ hooks/ 2>/dev/null | head -10

Document:

• Script purpose and trigger context
• Integration points (make, pre-commit, CI)
• Expected inputs and outputs

Step 2: Exit Code Audit (shell-review:exit-codes-checked)

@include modules/exit-codes.md

Step 3: Portability Check (shell-review:portability-checked)

@include modules/portability.md

Step 4: Safety Patterns (shell-review:safety-patterns-verified)

@include modules/safety-patterns.md

Step 5: Structure Patterns (shell-review:structure-checked)

@include modules/structure-patterns.md

Step 6: Evidence Log (shell-review:evidence-logged)

Use imbue:proof-of-work to record findings with file:line references.

Summarize:

• Critical issues (failures masked, security risks)
• Major issues (portability, maintainability)
• Minor issues (style, documentation)

Output Format

## Summary
Shell script review findings

## Scripts Reviewed
- [list with line counts]

## Exit Code Issues
### [E1] Pipeline masks failure
- Location: script.sh:42
- Anchor: `verbatim source text at file:line`
- Pattern: `cmd | grep` loses exit code
- Fix: Use pipefail or capture separately

## Portability Issues
[cross-platform concerns]

## Safety Issues
[unquoted variables, missing set flags]

## Recommendation
Approve / Approve with actions / Block

Verify Findings Are Grounded (shell-review:findings-verified)

Every finding must cite a real location and a verbatim anchor. Write findings to .review/findings.json and confirm each citation resolves:

python plugins/imbue/scripts/citation_verifier.py \
  --findings .review/findings.json --repo-root .

Drop or label UNVERIFIED any finding the verifier fails (exit 1); only verified findings enter the report. See Skill(imbue:review-core) Step 5 and Skill(imbue:structured-output) for the schema.

Exit Criteria

• [ ] Exit code propagation verified (pipelines checked for pipefail or

capture-and-check)

• [ ] Portability issues documented (Bash-isms in #!/bin/sh scripts flagged)
• [ ] Safety patterns verified (no echo, braced vars, :? expansion, cd in

subshells, no basename/dirname)

• [ ] Structure patterns verified (library/executable distinction, main call,

preamble, depcheck, shfmt formatting)

• [ ] Evidence logged with file:line references via imbue:proof-of-work
• [ ] Every reported finding carries a Location + verbatim Anchor

confirmed by citation_verifier.py (exit 0), or unverified findings were dropped or labeled UNVERIFIED

Exit Code Patterns

Critical: Pipeline Exit Codes

The default bash behavior is that a pipeline's exit code equals the last command's exit code. This masks failures:

# BAD - grep always succeeds if it finds lines, hiding make failure
if (make typecheck 2>&1 | grep -v "^make\["); then
    echo "Passed"  # WRONG - runs even when make fails!
fi

Fix 1: Use pipefail

set -o pipefail

# Now pipeline fails if ANY command fails
if make typecheck 2>&1 | grep -v "^make\["; then
    echo "Passed"
fi

Fix 2: Capture Output and Exit Code Separately

# Capture output, preserve exit code
local output
local exit_code=0
output=$(make typecheck 2>&1) || exit_code=$?

# Filter output for display
echo "$output" | grep -v "^make\[" || true

# Check actual exit code
if [ "$exit_code" -eq 0 ]; then
    echo "Passed"
else
    echo "Failed"
    return 1
fi

Fix 3: Use PIPESTATUS (Bash-specific)

make typecheck 2>&1 | grep -v "^make\["
if [ "${PIPESTATUS[0]}" -ne 0 ]; then
    echo "Make failed"
    exit 1
fi

Detection Commands

Find pipeline patterns that may mask failures:

# Commands piped to grep/head/tail (common culprits)
grep -n "| grep" scripts/*.sh
grep -n "| head" scripts/*.sh
grep -n "| tail" scripts/*.sh

# Pipelines in if conditions
grep -n "if.*|" scripts/*.sh

# Subshells with pipelines
grep -n "\$(.*|" scripts/*.sh

set -e Pitfalls

set -e (exit on error) has exceptions that can surprise:

set -e

# These do NOT trigger exit:
cmd || true           # Explicit fallback
if cmd; then ...      # Part of condition
cmd && other          # Part of AND/OR list
while cmd; do ...     # Loop condition

# This DOES trigger exit:
cmd                   # Standalone command that fails

Subshell Exit Codes

# BAD - subshell exit code lost
(cd /tmp && failing_command)
echo "This runs even if failing_command failed"

# GOOD - check subshell result
if ! (cd /tmp && failing_command); then
    echo "Failed"
    exit 1
fi

# GOOD - use || to handle failure
(cd /tmp && failing_command) || { echo "Failed"; exit 1; }

Common Patterns to Flag

Shell Portability

Shebang Lines

#!/bin/sh          # POSIX shell (most portable)
#!/bin/bash        # Bash (most features)
#!/usr/bin/env bash  # Bash via env (handles non-standard paths)

If using Bash features, use #!/usr/bin/env bash for portability across systems where bash may not be at /bin/bash.

Bash-Only Features

These require #!/bin/bash or #!/usr/bin/env bash:

Detection Commands

# Find Bash-isms in #!/bin/sh scripts
grep -l "^#!/bin/sh" scripts/*.sh | while read f; do
    # Check for [[ ]]
    grep -n "\[\[" "$f" && echo "  ^ $f uses [[ ]]"
    # Check for arrays
    grep -n "=(" "$f" && echo "  ^ $f uses arrays"
done

# Find all shebang types
grep -h "^#!" scripts/*.sh | sort -u

Common Portability Fixes

Test Brackets

# BAD - Bash only
if [[ -f "$file" && "$var" == "value" ]]; then

# GOOD - POSIX
if [ -f "$file" ] && [ "$var" = "value" ]; then

String Comparison

# BAD - Bash only (== works but not standard)
if [ "$a" == "$b" ]; then

# GOOD - POSIX
if [ "$a" = "$b" ]; then

Arithmetic

# BAD - Bash only
((count++))
if (( count > 10 )); then

# GOOD - POSIX
count=$((count + 1))
if [ "$count" -gt 10 ]; then

Local Variables

# BAD - 'local' is not POSIX (but widely supported)
local var="value"

# GOOD - explicitly use in functions only, document assumption
# Most modern shells support 'local', acceptable if documented

macOS vs Linux

# sed -i differs
# Linux: sed -i 's/a/b/' file
# macOS: sed -i '' 's/a/b/' file

# Portable approach
sed 's/a/b/' file > file.tmp && mv file.tmp file

# Or detect platform
case "$(uname -s)" in
    Darwin*) SED_INPLACE="sed -i ''" ;;
    *)       SED_INPLACE="sed -i" ;;
esac

Recommendation

1. Use #!/usr/bin/env bash and document Bash requirement 2. Or use #!/bin/sh and avoid ALL Bash-isms 3. Don't mix - pick one and be consistent

Shell Safety Patterns

No echo: use log() or printf

All output must go through log() from scripts/logging.sh or via printf(1). The only exception is usage() body lines (after the first), where printf is used directly.

Detection:

# Bare echo calls in non-comment lines
rg -n '^\s*echo\s' scripts/ .githooks/ plugins/*/hooks/
# fallback: grep -rn '^\s*echo\s' scripts/ .githooks/

Fix: replace echo "msg" with log "msg" or printf '%s\n' "msg".

Braced variable references

Every variable reference must use the braced form ${VAR}, not bare $VAR. This avoids surprises with adjacent text and is required for consistent ShellCheck compliance.

Detection:

rg -n '\$[A-Za-z_][A-Za-z_0-9]*[^}]' scripts/

Fix: $VAR → ${VAR}, $1 → ${1}, $@ → "${@}".

:? expansion instead of branching on unset

Never branch on an unset variable before triggering an exit-path. Use ${VAR:?message} so the shell emits the message and exits immediately when the variable is unset or empty.

# Bad: branches on unset, then exits
if [ -z "${DIR}" ]; then
  log 4 "DIR is unset"
  exit 1
fi

# Good: parameter expansion handles it
process_dir "${DIR:?DIR must be set}"

Detection:

rg -n '\[ -z.*\$\{?\w' scripts/   # [ -z "$VAR" ] before exit

cd inside a subshell

Every cd must be wrapped in a subshell so that the change of directory does not persist and a failed cd cannot leave the script in the wrong directory.

# Bad: cd leaks to caller scope; fails silently without set -e
cd "${build_dir}"
make clean

# Good: scoped and guarded
(cd "${build_dir:?No build dir}" && make clean)

Detection:

rg -n '^\s*cd\s+[^(]' scripts/     # cd not wrapped in (

Source relative to script location

External files must be sourced relative to the script's own location, not the caller's working directory.

# Bad: breaks when invoked from any other directory
. ./logging.sh

# Good: always resolves from the script's directory
MYDIR="${0%/*}"
. "${MYDIR%/}/logging.sh"

Use ${0%/*} (POSIX parameter expansion) instead of dirname "$0".

No basename or dirname

Use POSIX parameter expansion instead of the external commands basename and dirname.

Detection:

rg -n '\bbasename\b|\bdirname\b' scripts/

Library loading check form

When a script must verify a library was sourced, use the canonical case form, not [ -z … ] or [ -n … ]:

# Required form: distinguishes unset/empty/loaded
case "${__logging_loaded:-NULL}" in
  1) : ;;    # loaded
  *) printf 'logging.sh not loaded\n' >&2; exit 1 ;;
esac

Detection for non-canonical form:

rg -n '\[ -[zn].*__\w+_loaded' scripts/

set -e / set -u in libraries

Files meant to be sourced must not enable set -e or set -u because the flags leak to the caller and can exit the caller's session on unrelated commands.

Detection:

rg -n '^set -[eu]' scripts/logging.sh

printf over echo

For data output and multi-line messages, prefer printf with a fixed format string. Never build the format string from untrusted text.

# Bad: echo interprets escape sequences inconsistently
echo "Processing ${file}"

# Good: fixed format, no interpretation surprises
printf 'Processing %s\n' "${file}"

For logging through log(), pass the message as an argument. log() uses printf internally.

Checklist

• [ ] No raw echo calls (use log() or printf)
• [ ] All variables in braced form ${VAR}
• [ ] Unset required variables caught with :? expansion
• [ ] Every cd is wrapped in a subshell
• [ ] External files sourced via ${0%/*} relative path
• [ ] No basename/dirname; use param expansion
• [ ] Library guard uses case "${__lib_loaded:-NULL}" in
• [ ] Library scripts have no set -e or set -u

Shell Script Structure Patterns

Library vs executable

A script is a library if it has no main() function (e.g. scripts/logging.sh). A script is an executable if it defines main() and ends with main "${@}".

Libraries signal their presence by setting a __-prefixed guard variable (e.g. __logging_loaded=1). Callers verify it was sourced with the canonical form:

case "${__logging_loaded:-NULL}" in
  1) : ;;
  *) printf 'logging.sh not loaded\n' >&2; exit 1 ;;
esac

See also: safety-patterns.md → "Library loading check form".

Detection: library with execute bit

find scripts/ -name "*.sh" -perm /u+x | while IFS= read -r f; do
  rg -q 'main\(\)' "${f}" || printf 'Library with +x: %s\n' "${f}"
done

Detection: executable missing main "${@}" as last line

find scripts/ -name "*.sh" -perm /u+x | while IFS= read -r f; do
  last="$(tail -1 "${f}")"
  case "${last}" in
    'main "${@}"') : ;;
    *) printf 'Missing main call: %s\n' "${f}" ;;
  esac
done

Preamble for executable scripts

Every executable script must start with this preamble (using scripts/shellcheck.sh as the canonical example):

#!/bin/sh
set -eu

MYDIR="${0%/*}"
readonly MYDIR

# shellcheck source=scripts/logging.sh
. "${MYDIR%/}/logging.sh"

• #!/bin/sh: POSIX dialect; no Bash extensions
• set -eu: exit on error (-e), error on unset (-u)
• MYDIR="${0%/*}": script directory without dirname
• readonly MYDIR: marks the variable immutable
• # shellcheck source=…: lets shellcheck follow the source
• . "${MYDIR%/}/logging.sh": loads log() and banner()

All functionality in functions; no top-down execution

Scripts must never execute logic at top level. Every statement belongs inside a named function. The only top-level calls are:

1. set -eu (preamble) 2. Variable declarations (readonly, assignments) 3. Source statements (. lib.sh) 4. main "${@}" on the last line

Detection:

# Top-level commands outside function definitions
# (rough heuristic — awk parses function body depth)
awk '/^[a-z_][a-z_0-9]*\(\)/{depth++} /^\}/{depth--}
     depth==0 && /^\s*[a-z]/ && !/^(readonly|MYDIR|LOG|\.|\s*#)/{print NR": "$0}' script.sh

depcheck() for external dependencies

Any script relying on tools beyond POSIX must define depcheck(). Required tools use log 5 (critical); optional tools use log 3 (notice). Dependency lists allow the check logic to stay unchanged when tools are added.

REQUIRED_DEPENDENCIES="shellcheck shfmt"

depcheck() {
  _dc_missing=""
  for _dc_util in ${REQUIRED_DEPENDENCIES}; do
    command -v "${_dc_util}" >/dev/null 2>&1 ||
      _dc_missing="${_dc_missing:+"${_dc_missing} "}${_dc_util}"
  done
  case "${#_dc_missing}" in
    0) return 0 ;;
  esac
  log 5 "Required utilities not found: ${_dc_missing}"
  return 1
}

Building _dc_missing with ${_dc_missing:+"${_dc_missing} "}${_dc_util} is the POSIX way to append a space-separated word without leaving a leading space. It avoids arrays, which are a Bash extension.

usage() function

Scripts that accept flags must define usage(). The first output line must use log "Usage: …". Subsequent lines use printf.

usage() {
  log "Usage: scripts/myscript.sh [-h] [-x|-t] [ARGS]"
  printf '  -h       Show this help and exit (exit 0)\n'
  printf '  -x, -t   Enable xtrace for debugging\n'
  printf '  ARGS     Files or patterns to process\n'
}

The usage and help case patterns must accept both spellings and any case:

case "${1}" in
  *[uU][sS][aA][gG][eE] | *[hH][eE][lL][pP] | -h)
    usage
    exit 0
    ;;
esac

xtrace support (-x / -t flags)

Every executable script must support a flag to enable xtrace for debugging. The preferred flags are -x and -t.

XTRACE=0

# …inside main() after arg parsing:
case "${XTRACE}" in
  1) set -x ;;
esac

readonly for non-modified globals

Global variables that do not change during execution must be marked readonly. Group these declarations near the top of the script, after the preamble.

readonly MYDIR
readonly VERSION="1.0.0"
readonly CONFIG_FILE="${MYDIR%/}/../.config"

Platform detection

When commands differ by OS, assign the command and its arguments to separate variables using uname -s and distribution files.

KERNEL="$(uname -s)"
case "${KERNEL}" in
  *BSD | [Ll]inux)
    . /etc/os-release
    case "${ID}" in
      freebsd) INSTALLER="pkg"; INSTALLER_ARG="install" ;;
      ubuntu | debian) INSTALLER="apt-get"; INSTALLER_ARG="install -yqq" ;;
      alpine) INSTALLER="apk"; INSTALLER_ARG="add" ;;
      fedora | centos | rhel)
        for _pm_cmd in dnf yum; do
          command -v "${_pm_cmd}" >/dev/null 2>&1 && INSTALLER="${_pm_cmd}"
        done
        INSTALLER_ARG="install"
        ;;
    esac
    ;;
  Darwin)
    INSTALLER="brew"
    INSTALLER_ARG="install"
    ;;
esac

"${INSTALLER:?No known installer selected}" ${INSTALLER_ARG} "${PACKAGES}"

Note: ${INSTALLER_ARG} is intentionally unquoted here so its space-separated arguments expand into multiple words.

case over test / [ ]

Prefer case statements over test/[ ] for branching. case is faster (no subprocess), cleaner, and handles patterns natively.

# Preferred
case "${answer}" in
  [yY] | [yY][eE][sS]) confirm ;;
  *) abort ;;
esac

# Avoid
if [ "${answer}" = "y" ] || [ "${answer}" = "Y" ]; then
  confirm
fi

Formatting: shfmt -p -i 2 -ci

All scripts must be formatted with:

shfmt -p -i 2 -ci -w script.sh

• -p POSIX mode (no Bash extensions)
• -i 2 two-space indent
• -ci indent case label bodies

Run from the repository root:

# Check all scripts
shfmt -p -i 2 -ci -d scripts/

# Apply formatting in-place
shfmt -p -i 2 -ci -w scripts/*.sh

Checklist

• [ ] Library: no execute bit, no main(), __-guard present
• [ ] Executable: starts with preamble, ends with main "${@}"
• [ ] No top-level logic (only declarations, source, main "${@}")
• [ ] depcheck() present when external tools are required
• [ ] usage() present and accepts -h / usage/help variants
• [ ] -x/-t flags supported and enable xtrace
• [ ] Non-modified globals are readonly
• [ ] Platform branching uses uname -s and INSTALLER pattern
• [ ] case used instead of [ ] for branching
• [ ] shfmt -p -i 2 -ci -d reports no diff