Every server has them: the backup script from 2017, the deploy wrapper nobody wants to touch, the cron job that "just works". They were written by humans under time pressure, never reviewed, and they run as root. Our guide to reviewing AI-generated scripts covers code a model wrote. This one turns it around: the model is the reviewer, and the code is yours.
The order matters. ShellCheck first, because it is deterministic, fast, and right about what it reports. Then an AI pass aimed only at what static analysis cannot see — and then tests that prove each fix, because a model's review is a list of hypotheses, not a verdict.
ShellCheck First: The Codes That Matter
Here is a typical legacy script, and what ShellCheck 0.11 actually says about it in the compact gcc format:
#!/bin/bash
SRC=$1
DEST=/var/backups/app
export STAMP=$(date +%F)
cd $DEST
for f in $(ls $SRC/*.conf); do
cp $f $DEST/$STAMP/
done
rm -rf $DEST/old/
tar czf /tmp/app-$STAMP.tgz $(find $SRC -name '*.log')
run() { $@; }
$ shellcheck -f gcc backup.sh
backup.sh:4:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
backup.sh:5:1: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164]
backup.sh:6:10: error: Iterating over ls output is fragile. Use globs. [SC2045]
backup.sh:6:15: note: Double quote to prevent globbing and word splitting. [SC2086]
# ...four more SC2086 notes on lines 7 and 10 trimmed
backup.sh:10:29: warning: Quote this to prevent word splitting. [SC2046]
backup.sh:11:9: error: Double quote array expansions to avoid re-splitting elements. [SC2068]
| Code | What it means | Typical fix |
|---|---|---|
| SC2086 | Unquoted variable is word-split and glob-expanded | "$var" |
| SC2046 | Unquoted $(...) result splits on whitespace | Quote it, or use find -print0 with read -d '' |
| SC2164 | cd can fail and later commands run in the wrong directory | cd "$dir" || exit 1 |
| SC2155 | export/local x=$(cmd) hides the exit status of cmd | Declare on one line, assign on the next |
| SC2068 | Unquoted $@ re-splits every argument | "$@" |
| SC2045 | Looping over ls output breaks on unusual names | for f in "$SRC"/*.conf |
| SC2115 | rm -rf "$var/" becomes rm -rf / when var is empty | rm -rf "${var:?}"/ |
Gate on warnings, not notes — shellcheck --severity=warning ignores info and style findings. On a large legacy codebase, start there or the noise buries the real problems.
Follow sourced files — shellcheck -x lets it read files pulled in with source, so variables defined there stop showing up as unassigned.
Let it write the mechanical fixes — shellcheck -f diff backup.sh | git apply applies the quoting fixes it is certain about. Review the diff. Do not hand-edit forty quotes yourself.
Suppress deliberately — when a finding is intentional, add # shellcheck disable=SC2086 on the line above with a comment saying why. Never disable globally.
What ShellCheck Cannot See
ShellCheck reasons about syntax and data flow inside one file. It does not know what the script is for, who else touches the same paths, or how bash's error handling behaves at runtime. This short script passes ShellCheck with no findings at all:
#!/bin/bash
set -e
i=0
((i++)) # exits the script: ((0)) returns status 1 under set -e
TMP=/tmp/report.$$ # predictable name in a world-writable directory
date > "$TMP"
TARGET="$BASE/cache" # BASE unset? TARGET is "/cache"
rm -rf "$TARGET" # SC2115 only fires when the variable is used directly
Temp file races (TOCTOU) — a name built from $$ can be guessed. Another user can create a symlink at that path first, and a root script then writes through it. The fix is tmp=$(mktemp), which creates a unique file with mode 0600 in one step, plus trap 'rm -f -- "$tmp"' EXIT.
set -e is not what people think — it is ignored inside any function called from if, && or ||; ((i++)) kills the script when i is 0; and with pipefail, yes | head -1 exits 141 because the writer gets SIGPIPE. Adding set -euo pipefail to an old script changes its behaviour, so it needs testing like any other change.
Destructive paths built indirectly — rm -rf on a variable assembled three lines earlier from another variable that might be empty. ShellCheck catches the direct case; the model is better at following the chain.
eval and command construction — eval "$CMD" is quoted correctly, so ShellCheck is satisfied. Whether CMD can contain text from a filename, a webhook, or a config file is a question about intent.
IFS leaks — a script that sets IFS=, to parse one CSV line and never restores it changes how every later unquoted expansion splits. Prefer IFS=, read -r a b c, which scopes it to the one command.
Concurrency — two cron runs overlapping, or a check-then-create lock file. exec 9>/run/lock/backup.lock; flock -n 9 || exit 0 is atomic; [ -f lock ] || touch lock is not.
A Review Prompt That Produces Usable Findings
A vague "review this script" gets a vague list that mixes real bugs with style opinions and repeats what ShellCheck already found. Give the model line numbers, the ShellCheck output, how the script runs, and a fixed format:
$ shellcheck -f gcc backup.sh > /tmp/sc.txt
$ ollama run llama3.2 "You are reviewing a production bash script written by a
human years ago. It runs nightly from root's crontab on Ubuntu 22.04.
SCRIPT (with line numbers):
$(nl -ba backup.sh)
SHELLCHECK ALREADY REPORTED:
$(cat /tmp/sc.txt)
Do not repeat the ShellCheck findings. Look only for what static analysis misses:
logic errors, race conditions, unsafe temp files, destructive commands whose
target could be empty or wrong, failures that set -e does not catch, eval or
commands built from external input, and missing cleanup on exit.
Report each finding as:
LINE: <number> SEVERITY: critical|high|medium|low
ISSUE: <one sentence>
TRIGGER: <a concrete input or condition that makes it fail>
FIX: <the minimal change>
critical = data loss or privilege escalation; high = silently wrong result;
medium = fails loudly in a plausible case; low = robustness.
Prefix any finding you are not certain of with UNSURE. If a category has no
issues, say so instead of inventing one."
TRIGGER is the key field — a finding without a concrete failing input is an opinion. A finding with one can be reproduced in thirty seconds, which is how you separate real bugs from plausible-sounding ones.
Context changes severity — the same predictable temp file is low severity in a user's personal script and critical in a root cron job. Say who runs it and how.
Line numbers drift — models miscount, especially in long files. Check each cited line with sed -n '42p' backup.sh before you act on the finding.
Keep proprietary scripts local — internal scripts contain hostnames, paths, and sometimes credentials. A local model keeps them on your machine.
Triage: Treat Findings as Hypotheses
Expect some findings to be wrong. Models flag things that cannot happen in context, miss a guard ten lines up, and sometimes suggest a "fix" that changes behaviour other scripts depend on. Triage each one before you edit anything:
Reproduce the trigger — run the script in a container or scratch directory with the exact failing input: a filename with a space, an empty argument, a missing directory. If it does not fail, mark the finding rejected and move on.
You set the severity — the model's severity is a suggestion. You know whether the script handles customer data or rotates logs in /tmp.
Watch for behaviour changes — replacing $(ls $SRC/*.conf) with a glob is correct, but when nothing matches, the glob stays as the literal pattern unless you set shopt -s nullglob. That is a different failure mode, not no failure.
Legacy bugs can be load-bearing — a downstream job may depend on the script exiting 0 even when it partly fails. Find the callers before you make failures fatal.
Ask why, not just what — when a finding is unclear, ask the model to walk through the execution step by step with the trigger input. Bad reasoning usually shows up within two steps.
Prove Each Fix With bats
A fix without a test is another unreviewed change. bats-core runs plain bash assertions, and every TRIGGER from the review can become a test case. These assume the fixed script takes the source and destination as arguments:
# test/backup.bats
setup() {
SRC=$(mktemp -d)
DEST=$(mktemp -d)
touch "$SRC/my app.conf"
}
teardown() {
rm -rf -- "$SRC" "$DEST"
}
@test "copies configs whose names contain spaces" {
run ./backup.sh "$SRC" "$DEST"
[ "$status" -eq 0 ]
[ -f "$DEST/my app.conf" ]
}
@test "refuses an empty source argument" {
run ./backup.sh "" "$DEST"
[ "$status" -ne 0 ]
}
@test "fails when the destination is missing instead of writing elsewhere" {
run ./backup.sh "$SRC" /nonexistent/dest
[ "$status" -ne 0 ]
}
$ bats test/
Write the test before the fix — run it against the original script and watch it fail. That proves the bug was real and the test can detect it.
run captures everything — bats' run stores the exit code in $status and combined output in $output, so a failing command does not abort the test file.
Ask the model for test cases — it is good at listing hostile inputs: leading dashes, newlines in names, empty directories, read-only targets. You still decide which ones matter.
Wire It Into CI
ShellCheck and bats are deterministic, so they can block a merge. AI review is not: the same prompt can return different findings on each run. Keep it as an advisory step that a human reads, not as a gate.
# .pre-commit-config.yaml
repos:
- repo: https://github.com/koalaman/shellcheck-precommit
rev: v0.10.0
hooks:
- id: shellcheck
args: ["--severity=warning"]
# .github/workflows/shell.yml
name: shell
on: [push, pull_request]
jobs:
check:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: ShellCheck
run: git ls-files -z '*.sh' | xargs -0 -r shellcheck --severity=warning
- name: bats
run: |
sudo apt-get update && sudo apt-get install -y bats
bats test/
ShellCheck ships on GitHub's Ubuntu runners — no install step needed. Pin the version in pre-commit so local results match CI.
Ratchet legacy repos — if the whole repo fails today, check only changed files at first, and tighten the severity as the backlog shrinks.
Review on change, not on schedule — run the AI review prompt on the diff of a pull request, together with the full file for context. A scheduled sweep of unchanged scripts produces findings nobody acts on. For jobs that run from cron, our cron guide covers the environment differences that cause most surprises.
Frequently Asked Questions
Can AI replace ShellCheck for reviewing bash scripts?
No. ShellCheck is deterministic and precise about quoting, word splitting and syntax, and it never invents findings. Run it first and fix what it reports. Then use AI for what static analysis cannot judge: intent, race conditions, unsafe temp files, and destructive commands whose target can be empty.
Why should I give the model the ShellCheck output?
So it does not spend its answer repeating findings you already have. Telling it what ShellCheck reported, and asking only for issues outside that list, points the model at logic and runtime behaviour, which is where it adds value.
Is adding set -euo pipefail to an old script always safe?
No. It changes behaviour: arithmetic like ((i++)) can exit the script when the value is zero, pipelines such as yes | head exit 141 under pipefail, and unset variables that used to expand to nothing now abort. It is the right default, but in a legacy script treat it as a change that needs testing.
How do I know whether an AI review finding is real?
Require a concrete trigger for every finding, an input or condition that makes the script fail, then reproduce it in a scratch directory or container. Check the cited line number yourself, since models miscount. If the trigger does not reproduce, reject the finding.
What is the safe way to create temp files in bash?
Use mktemp or mktemp -d, which create a unique file or directory with restrictive permissions in one atomic step, and remove it with a trap on EXIT. Names built from $$ or the date are predictable and open the script to symlink attacks in shared directories like /tmp.