Lab: Use ShellCheck to clean up an existing script

This lab takes a poorly-written script and uses ShellCheck to fix it. By the end you will have the discipline for applying static analysis to legacy code - and, just as importantly, a clear sense of where static analysis stops.

Objective

Produce a ShellCheck-clean version of a legacy script, prove it behaves correctly against a fixture directory, and be able to name the class of defect ShellCheck did not report.

Architecture

One host. Everything happens under ~/shellcheck-lab and a disposable fixture directory. No system paths are modified.

Requirements

• shellcheck 0.9 or later (sudo apt install shellcheck, or sudo dnf install ShellCheck). Check with shellcheck --version.
• bash 4.4 or later.
• No root privileges. Do not run any part of this lab as root.

Scenario

You have inherited a maintenance script from a departing colleague. It runs nightly on a fleet. Nobody has read it in two years. Your job is to make it safe to keep running.

Tasks

Task 1: Create a script with issues

mkdir -p ~/shellcheck-lab
cd ~/shellcheck-lab

cat > bad-script.sh <<'EOF'
#!/bin/bash
FILES=$(ls /tmp)
for f in $FILES
do
    if [ -f $f ]
    then
        echo "File: $f"
        cp $f $f.bak
    fi
done
cd /tmp
rm -f *.tmp
EOF

chmod +x bad-script.sh

Task 2: Run ShellCheck

shellcheck bad-script.sh

Real output from ShellCheck 0.11.0. The machine-readable form is easier to count, so both are shown:

$ shellcheck -f gcc bad-script.sh
bad-script.sh:5:13: note: Double quote to prevent globbing and word splitting. [SC2086]
bad-script.sh:8:12: note: Double quote to prevent globbing and word splitting. [SC2086]
bad-script.sh:8:15: note: Double quote to prevent globbing and word splitting. [SC2086]
bad-script.sh:11:1: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164]
bad-script.sh:12:7: note: Use ./*glob* or -- *glob* so names with dashes won't become options. [SC2035]

Five findings, four codes:

Note the exit status: shellcheck returns 1 when it reports anything, which is what makes it usable in CI.

Task 3: Fix each warning

Fixing the four codes is mechanical. Fixing the script is not - see Task 4.

cat > good-script.sh <<'EOF'
#!/usr/bin/env bash
#
# Back up every regular file in ONE directory, then delete that
# directory's *.tmp files. The directory is an argument, never a
# hardcoded path. Safe to run twice.
set -euo pipefail
shopt -s nullglob

target="${1:?usage: good-script.sh DIRECTORY}"
[[ -d "$target" ]] || { printf 'not a directory: %s\n' "$target" >&2; exit 2; }

for f in "$target"/*; do
    [[ -f "$f" ]] || continue
    if [[ "$f" == *.bak || -e "$f.bak" ]]; then
        continue                      # already a backup, or already backed up
    fi
    printf 'backing up: %s\n' "$f"
    cp -- "$f" "$f.bak"
done

find "$target" -maxdepth 1 -type f -name '*.tmp' -delete
EOF

chmod +x good-script.sh

What each change buys:

• for f in "$target"/* with shopt -s nullglob - a glob instead of parsed ls output, and an empty directory yields no iterations rather than the literal string *.
• Every expansion quoted - clears SC2086 and makes filenames with spaces work.
• cp -- "$f" - the -- ends option parsing, so a file named -i is treated as a filename. This is the same defect SC2035 flagged on the rm line.
• "${1:?usage: ...}" - the script refuses to run with no argument instead of defaulting to something dangerous.
• [[ "$f" == *.bak || -e "$f.bak" ]] - idempotence. Run it twice and you get file.log.bak, not file.log.bak.bak.
• No cd - clearing SC2164 by deleting the cd is better than clearing it with || exit. A script that never changes directory cannot act on the wrong one.

Task 4: The bug ShellCheck did not find

Compare the two scripts on behaviour, not on lint codes.

Task 5: Verify clean

shellcheck good-script.sh
echo "exit status: $?"

Expect no output and exit status 0.

Task 6: Test against a fixture directory

The script takes the directory as an argument, so the test acts only on the fixture:

rm -rf /tmp/test-shellcheck
mkdir -p /tmp/test-shellcheck
echo "test" > /tmp/test-shellcheck/file1.log
echo "test" > /tmp/test-shellcheck/file2.tmp

bash ~/shellcheck-lab/good-script.sh /tmp/test-shellcheck

ls -A /tmp/test-shellcheck
# file1.log  file1.log.bak  file2.tmp.bak

file2.tmp was backed up first, then deleted - so file2.tmp.bak remains and file2.tmp does not.

Now prove idempotence and the guard rails:

# Second run: no new .bak.bak files
bash ~/shellcheck-lab/good-script.sh /tmp/test-shellcheck
ls -A /tmp/test-shellcheck        # unchanged

# Refuses a missing directory
bash ~/shellcheck-lab/good-script.sh /nope
echo "exit: $?"                   # not a directory: /nope  /  exit: 2

# Refuses to run with no target at all
bash ~/shellcheck-lab/good-script.sh
echo "exit: $?"                   # usage message / exit: 1

Task 7: Document

SHELLCHECK LAB
==============
Original: bad-script.sh - 5 findings across 4 codes
  SC2086 x3 (lines 5, 8, 8), SC2164 (line 11), SC2035 (line 12)
Fixed: good-script.sh - 0 findings

Fixes applied:
- Replaced $(ls) with a glob, plus nullglob
- Quoted every expansion (SC2086)
- Added -- before filename operands (SC2035)
- Removed the cd entirely rather than adding || exit (SC2164)
- Added set -euo pipefail

Fixes ShellCheck did NOT ask for, found by reading the script:
- Directory is now an argument, not the hardcoded /tmp
- Refuses to run without a target directory
- Skips .bak files, so repeat runs do not chain .bak.bak

Verification: ShellCheck clean (exit 0); tested against
/tmp/test-shellcheck; second run produced no changes.

Validation

• shellcheck good-script.sh produces no output and exits 0.
• Running the script against the fixture leaves file1.log, file1.log.bak and file2.tmp.bak.
• A second run changes nothing.
• Running it with no argument, or with a path that is not a directory, exits non-zero without touching any files.

Expected Outcome

Two scripts in ~/shellcheck-lab: the unmodified specimen and a clean, parameterised, idempotent replacement - plus a written record that separates the defects the linter found from the defects you found.

Troubleshooting

Cleanup

rm -rf /tmp/test-shellcheck
rm -rf ~/shellcheck-lab

Nothing outside these two directories was modified, which is the point.

What You Learned

• ShellCheck codes map to concrete failure modes: SC2086 to filenames with spaces, SC2035 to filenames that look like options, SC2164 to acting in the wrong directory.
• Quote the real output. Invented counts and codes make a report unreproducible.
• Deleting a risky construct beats annotating it: no cd is safer than cd ... || exit.
• A ShellCheck-clean script can still hardcode a shared path, duplicate a filesystem, and be non-idempotent. The linter checks syntax and expansion, never intent.