From 948e84fcd27c557cced52d4baef388a387eb9075 Mon Sep 17 00:00:00 2001
From: Mahyar McDonald <mac@anysphere.co>
Date: Fri, 31 Oct 2025 12:27:30 -0700
Subject: [PATCH] add shellcheck linting

- Updated variable expansions to use double quotes for better safety and to prevent word splitting.
- Replaced `ls` with `find` in cache operations to handle non-alphanumeric filenames more robustly.
- Enhanced array handling in scripts by using `${*:N}` syntax for concatenation.
- Improved readability and consistency in conditional checks by using `[[ ... ]]` instead of `[ ... ]`.
- Added comments for clarity on specific operations and shellcheck directives.
---
 .github/workflows/shellcheck.yml | 28 +++++++++++
 .shellcheckrc                    | 31 ++++++++++++
 README.md                        | 81 ++++++++++++++++++++++++++++++++
 install_and_cache_pkgs.sh        | 19 ++++----
 lib.sh                           | 53 +++++++++++++--------
 post_cache_action.sh             |  6 +--
 pre_cache_action.sh              | 19 ++++----
 restore_pkgs.sh                  | 23 +++++----
 8 files changed, 211 insertions(+), 49 deletions(-)
 create mode 100644 .github/workflows/shellcheck.yml
 create mode 100644 .shellcheckrc

diff --git a/.github/workflows/shellcheck.yml b/.github/workflows/shellcheck.yml
new file mode 100644
index 0000000..9072853
--- /dev/null
+++ b/.github/workflows/shellcheck.yml
@@ -0,0 +1,28 @@
+name: ShellCheck
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+  push:
+    branches:
+      - master
+      - dev
+      - staging
+
+permissions:
+  contents: read
+
+jobs:
+  shellcheck:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - name: Run ShellCheck
+        uses: ludeeus/action-shellcheck@master
+        with:
+          scandir: '.'
+          format: gcc
+          severity: style
+
diff --git a/.shellcheckrc b/.shellcheckrc
new file mode 100644
index 0000000..fb23e08
--- /dev/null
+++ b/.shellcheckrc
@@ -0,0 +1,31 @@
+# ShellCheck configuration file
+# See https://github.com/koalaman/shellcheck/wiki/Directives for more information
+
+# Enable all optional checks
+enable=all
+
+# Disable specific warnings that may be acceptable in this project
+# SC1090: Can't follow non-constant source. Use a path to a file that can be checked
+#         (disabled because we use dynamic sourcing with script_dir)
+disable=SC1090
+
+# SC1091: Not following: <file> was not specified as input
+#         (disabled because lib.sh is sourced dynamically)
+disable=SC1091
+
+# SC2310: Function invoked in && condition (set -e disabled)
+#         (acceptable pattern for conditional execution)
+disable=SC2310
+
+# SC2311: Bash implicitly disabled set -e in command substitution
+#         (acceptable pattern - we want to capture output even if function fails)
+disable=SC2311
+
+# SC2312: Consider invoking command separately to avoid masking return value
+#         (many of these are acceptable patterns in command substitutions)
+disable=SC2312
+
+# Exclude external files that we don't control
+exclude-dir=.git
+exclude-dir=testlogs
+
diff --git a/README.md b/README.md
index c0d1c3b..e9a0807 100644
--- a/README.md
+++ b/README.md
@@ -154,3 +154,84 @@ For more context and information see [issue #57](https://github.com/awalsh128/ca
 ### Cache Limits
 
 A repository can have up to 5GB of caches. Once the 5GB limit is reached, older caches will be evicted based on when the cache was last accessed. Caches that are not accessed within the last week will also be evicted. To get more information on how to access and manage your actions's caches, see [GitHub Actions / Using workflows / Cache dependencies](https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#viewing-cache-entries).
+
+## Development
+
+### Prerequisites
+
+- **Go 1.20+** (for building the `apt_query` binary) - version specified in `go.mod`
+  - Install from [golang.org](https://golang.org/dl/) or via package manager
+  - Verify installation: `go version`
+- **ShellCheck** (for linting shell scripts) - install via:
+  - macOS: `brew install shellcheck`
+  - Linux: `sudo apt-get install shellcheck` or see [shellcheck installation guide](https://github.com/koalaman/shellcheck#installing)
+  - Windows: Available via [scoop](https://scoop.sh/) or [chocolatey](https://chocolatey.org/)
+
+### Building
+
+The project includes Go binaries (`apt_query-arm64` and `apt_query-x86`) that are used by the shell scripts to query APT package information.
+
+**Build all packages:**
+```bash
+go build -v ./...
+```
+
+**Build for specific architecture:**
+```bash
+# For ARM64 (Apple Silicon, ARM servers)
+GOARCH=arm64 go build -o apt_query-arm64 ./src/cmd/apt_query
+
+# For x86_64 (Intel/AMD)
+GOARCH=amd64 go build -o apt_query-x86 ./src/cmd/apt_query
+```
+
+**Run tests:**
+```bash
+go test -v ./...
+```
+
+### Linting
+
+This project uses [ShellCheck](https://github.com/koalaman/shellcheck) to ensure shell script quality and catch common errors. The configuration is stored in `.shellcheckrc`.
+
+**Run ShellCheck locally:**
+```bash
+shellcheck *.sh
+```
+
+**IDE Integration:**
+
+Many IDEs and editors can automatically run ShellCheck:
+
+- **VS Code**: Install the [ShellCheck extension](https://marketplace.visualstudio.com/items?itemName=timonwong.shellcheck)
+- **Vim/Neovim**: Use [ALE](https://github.com/dense-analysis/ale) or [coc-shellcheck](https://github.com/josa42/coc-shellcheck)
+- **IntelliJ/CLion**: ShellCheck is integrated in recent versions
+- **Sublime Text**: Install [SublimeLinter-shellcheck](https://github.com/SublimeLinter/SublimeLinter-shellcheck)
+
+**Go Linting:**
+
+This project uses [golangci-lint](https://golangci-lint.run/) for Go code quality checks.
+
+**Run golangci-lint locally:**
+```bash
+# Install golangci-lint (if not already installed)
+# macOS: brew install golangci-lint
+# Linux: See https://golangci-lint.run/usage/install/
+
+golangci-lint run
+```
+
+**IDE Integration:**
+
+- **VS Code**: Install the [Go extension](https://marketplace.visualstudio.com/items?itemName=golang.go) for syntax highlighting, auto-completion, and built-in linting
+- **IntelliJ/GoLand**: Built-in Go support with linting and formatting
+- **Vim/Neovim**: Use [vim-go](https://github.com/fatih/vim-go) for Go development
+
+### CI/CD
+
+The GitHub Actions workflows will automatically:
+- **Build and test** Go code on pull requests
+- **Run ShellCheck** on shell scripts (blocks PRs on failures)
+- **Run golangci-lint** on Go code (blocks PRs on failures)
+
+All checks run on pull requests and pushes to `master`, `dev`, and `staging` branches.
diff --git a/install_and_cache_pkgs.sh b/install_and_cache_pkgs.sh
index 1a544ad..26e088b 100755
--- a/install_and_cache_pkgs.sh
+++ b/install_and_cache_pkgs.sh
@@ -19,7 +19,7 @@ cache_dir="${1}"
 add_repository="${3}"
 
 # List of the packages to use.
-input_packages="${@:4}"
+input_packages="${*:4}"
 
 if ! apt-fast --version > /dev/null 2>&1; then
   log "Installing apt-fast for optimized installs..."
@@ -31,7 +31,7 @@ if ! apt-fast --version > /dev/null 2>&1; then
 fi
 
 # Add custom repositories if specified
-if [ -n "${add_repository}" ]; then
+if [[ -n "${add_repository}" ]]; then
   log "Adding custom repositories..."
   for repository in ${add_repository}; do
     log "- Adding repository: ${repository}"
@@ -76,7 +76,10 @@ install_log_filepath="${cache_dir}/install.log"
 
 log "Clean installing ${package_count} packages..."
 # Zero interaction while installing or upgrading the system via apt.
-sudo DEBIAN_FRONTEND=noninteractive apt-fast --yes install ${packages} > "${install_log_filepath}"
+# Note: sudo doesn't affect redirects, but we want the output in the file anyway
+# shellcheck disable=SC2024
+# We intentionally redirect output here; the redirect happens as the current user which is fine
+sudo DEBIAN_FRONTEND=noninteractive apt-fast --yes install "${packages}" > "${install_log_filepath}"
 log "done"
 log "Installation log written to ${install_log_filepath}"
 
@@ -86,7 +89,7 @@ installed_packages=$(get_installed_packages "${install_log_filepath}")
 log "Installed package list:"
 for installed_package in ${installed_packages}; do
   # Reformat for human friendly reading.  
-  log "- $(echo ${installed_package} | awk -F\= '{print $1" ("$2")"}')"
+  log "- $(echo "${installed_package}" | awk -F= '{print $1" ("$2")"}')"
 done
 
 log_empty_line
@@ -98,7 +101,7 @@ for installed_package in ${installed_packages}; do
 
   # Sanity test in case APT enumerates duplicates.
   if test ! -f "${cache_filepath}"; then
-    read package_name package_ver < <(get_package_name_ver "${installed_package}")
+    read -r package_name package_ver < <(get_package_name_ver "${installed_package}")
     log "  * Caching ${package_name} to ${cache_filepath}..."
 
     # Pipe all package files (no folders), including symlinks, their targets, and installation control data to Tar.
@@ -109,9 +112,9 @@ for installed_package in ${installed_packages}; do
       while IFS= read -r f; do
         if test -f "${f}" -o -L "${f}"; then
           get_tar_relpath "${f}"
-          if [ -L "${f}" ]; then
+          if [[ -L "${f}" ]]; then
             target="$(readlink -f "${f}")"
-            if [ -f "${target}" ]; then
+            if [[ -f "${target}" ]]; then
               get_tar_relpath "${target}"
             fi
           fi
@@ -125,7 +128,7 @@ for installed_package in ${installed_packages}; do
   # Comma delimited name:ver pairs in the all packages manifest.
   manifest_all="${manifest_all}${package_name}=${package_ver},"
 done
-log "done (total cache size $(du -h ${cache_dir} | tail -1 | awk '{print $1}'))"
+log "done (total cache size $(du -h "${cache_dir}" | tail -1 | awk '{print $1}'))"
 
 log_empty_line
 
diff --git a/lib.sh b/lib.sh
index 755d939..1d4eac8 100755
--- a/lib.sh
+++ b/lib.sh
@@ -17,14 +17,16 @@ set +e
 #   Filepath of the install script, otherwise an empty string.
 ###############################################################################
 function execute_install_script {
-  local package_name=$(basename ${2} | awk -F\= '{print $1}')  
-  local install_script_filepath=$(\
+  local package_name
+  package_name=$(basename "${2}" | awk -F= '{print $1}')  
+  local install_script_filepath
+  install_script_filepath=$(\
     get_install_script_filepath "${1}" "${package_name}" "${3}")
   if test ! -z "${install_script_filepath}"; then
     log "- Executing ${install_script_filepath}..."
     # Don't abort on errors; dpkg-trigger will error normally since it is
     # outside its run environment.
-    sudo sh -x ${install_script_filepath} ${4} || true
+    sudo sh -x "${install_script_filepath}" "${4}" || true
     log "  done"
   fi
 }
@@ -40,9 +42,17 @@ function execute_install_script {
 ###############################################################################
 function get_install_script_filepath {
   # Filename includes arch (e.g. amd64).
-  local filepath="$(\
-    ls -1 ${1}var/lib/dpkg/info/${2}*.${3} 2> /dev/null \
-    | grep -E ${2}'(:.*)?.'${3} | head -1 || true)"
+  local filepath
+  # Use glob expansion instead of ls|grep for better handling of non-alphanumeric filenames
+  # Use nullglob to prevent literal match when no files found
+  shopt -s nullglob
+  for f in "${1}"var/lib/dpkg/info/"${2}"*."${3}"; do
+    if [[ -f "${f}" ]] && [[ "${f}" =~ ${2}'(:.*)?.'"${3}" ]]; then
+      filepath="${f}"
+      break
+    fi
+  done
+  shopt -u nullglob
   test "${filepath}" && echo "${filepath}"
 }
 
@@ -66,7 +76,7 @@ function get_installed_packages {
       log_err "Unable to parse package name and version from \"${line}\""
       exit 2
     fi
-  done < <(grep "^Unpacking " ${install_log_filepath})
+  done < <(grep "^Unpacking " "${install_log_filepath}")
   if test -n "${dep_packages}"; then
     echo "${dep_packages:0:-1}"  # Removing trailing space.
   else
@@ -83,13 +93,13 @@ function get_installed_packages {
 ###############################################################################
 function get_package_name_ver {
   local ORIG_IFS="${IFS}"
-  IFS=\= read name ver <<< "${1}"
+  IFS='=' read -r name ver <<< "${1}"
   IFS="${ORIG_IFS}"
   # If version not found in the fully qualified package value.
   if test -z "${ver}"; then
     # This is a fallback and should not be used any more as its slow.
     log_err "Unexpected version resolution for package '${name}'"
-    ver="$(apt-cache show ${name} | grep '^Version:' | awk '{print $2}')"
+    ver="$(apt-cache show "${name}" | grep '^Version:' | awk '{print $2}')"
   fi
   echo "${name}" "${ver}"  
 }
@@ -105,16 +115,19 @@ function get_normalized_package_list {
   # Remove commas, and block scalar folded backslashes,
   # extraneous spaces at the middle, beginning and end
   # then sort.
-  local packages=$(echo "${1}" \
+  local packages
+  packages=$(echo "${1}" \
     | sed 's/[,\]/ /g; s/\s\+/ /g; s/^\s\+//g; s/\s\+$//g' \
     | sort -t' ')
-  local script_dir="$(dirname -- "$(realpath -- "${0}")")"
+  local script_dir
+  script_dir="$(dirname -- "$(realpath -- "${0}")")"
 
-  local architecture=$(dpkg --print-architecture)
-  if [ "${architecture}" == "arm64" ]; then
-    ${script_dir}/apt_query-arm64 normalized-list ${packages}
+  local architecture
+  architecture=$(dpkg --print-architecture)
+  if [[ "${architecture}" == "arm64" ]]; then
+    "${script_dir}"/apt_query-arm64 normalized-list "${packages}"
   else
-    ${script_dir}/apt_query-x86 normalized-list ${packages}
+    "${script_dir}"/apt_query-x86 normalized-list "${packages}"
   fi
 }
 
@@ -127,8 +140,8 @@ function get_normalized_package_list {
 #   The relative filepath to archive.
 ###############################################################################
 function get_tar_relpath {
-  local filepath=${1}
-  if test ${filepath:0:1} = "/"; then
+  local filepath="${1}"
+  if test "${filepath:0:1}" = "/"; then
     echo "${filepath:1}"
   else
     echo "${filepath}"
@@ -153,7 +166,7 @@ function validate_bool {
   if test "${1}" != "true" -a "${1}" != "false"; then
     log "aborted"
     log "${2} value '${1}' must be either true or false (case sensitive)."
-    exit ${3}
+    exit "${3}"
   fi
 }
 
@@ -167,12 +180,12 @@ function validate_bool {
 #   Log lines from write.
 ###############################################################################
 function write_manifest {  
-  if [ ${#2} -eq 0 ]; then 
+  if [[ ${#2} -eq 0 ]]; then 
     log "Skipped ${1} manifest write. No packages to install."
   else
     log "Writing ${1} packages manifest to ${3}..."
     # 0:-1 to remove trailing comma, delimit by newline and sort.
-    echo "${2:0:-1}" | tr ',' '\n' | sort > ${3}
+    echo "${2:0:-1}" | tr ',' '\n' | sort > "${3}"
     log "done"
   fi
 }
diff --git a/post_cache_action.sh b/post_cache_action.sh
index a6a5689..96c59d7 100755
--- a/post_cache_action.sh
+++ b/post_cache_action.sh
@@ -29,12 +29,12 @@ test "${debug}" = "true" && set -x
 add_repository="${6}"
 
 # List of the packages to use.
-packages="${@:7}"
+packages="${*:7}"
 
 if test "${cache_hit}" = "true"; then
-  ${script_dir}/restore_pkgs.sh "${cache_dir}" "${cache_restore_root}" "${execute_install_scripts}" "${debug}"
+  "${script_dir}"/restore_pkgs.sh "${cache_dir}" "${cache_restore_root}" "${execute_install_scripts}" "${debug}"
 else
-  ${script_dir}/install_and_cache_pkgs.sh "${cache_dir}" "${debug}" "${add_repository}" ${packages}
+  "${script_dir}"/install_and_cache_pkgs.sh "${cache_dir}" "${debug}" "${add_repository}" "${packages}"
 fi
 
 log_empty_line
diff --git a/pre_cache_action.sh b/pre_cache_action.sh
index 5cb64cb..6986d9b 100755
--- a/pre_cache_action.sh
+++ b/pre_cache_action.sh
@@ -10,7 +10,7 @@ source "${script_dir}/lib.sh"
 # Setup first before other operations.
 debug="${4}"
 validate_bool "${debug}" debug 1
-test ${debug} == "true" && set -x
+test "${debug}" == "true" && set -x
 
 # Directory that holds the cached packages.
 cache_dir="${1}"
@@ -28,7 +28,8 @@ debug="${4}"
 add_repository="${5}"
 
 # List of the packages to use.
-input_packages="${@:6}"
+# Use * instead of @ to concatenate array elements into a single string
+input_packages="${*:6}"
 
 # Trim commas, excess spaces, and sort.
 log "Normalizing package list..."
@@ -36,7 +37,7 @@ packages="$(get_normalized_package_list "${input_packages}")"
 log "done"
 
 # Create cache directory so artifacts can be saved.
-mkdir -p ${cache_dir}
+mkdir -p "${cache_dir}"
 
 log "Validating action arguments (version='${version}', packages='${packages}')...";
 if grep -q " " <<< "${version}"; then
@@ -47,7 +48,9 @@ fi
 
 # Is length of string zero?
 if test -z "${packages}"; then
-  case "$EMPTY_PACKAGES_BEHAVIOR" in
+  # shellcheck disable=SC2154
+  # EMPTY_PACKAGES_BEHAVIOR is an environment variable passed from GitHub Actions
+  case "${EMPTY_PACKAGES_BEHAVIOR}" in
     ignore)
       exit 0
       ;;
@@ -66,7 +69,7 @@ fi
 validate_bool "${execute_install_scripts}" execute_install_scripts 4
 
 # Basic validation for repository parameter
-if [ -n "${add_repository}" ]; then
+if [[ -n "${add_repository}" ]]; then
   log "Validating repository parameter..."
   for repository in ${add_repository}; do
     # Check if repository format looks valid (basic check)
@@ -100,13 +103,13 @@ log "- CPU architecture is '${cpu_arch}'."
 value="${packages} @ ${version} ${force_update_inc}"
 
 # Include repositories in cache key to ensure different repos get different caches
-if [ -n "${add_repository}" ]; then
+if [[ -n "${add_repository}" ]]; then
   value="${value} ${add_repository}"
   log "- Repositories '${add_repository}' added to value."
 fi
 
 # Don't invalidate existing caches for the standard Ubuntu runners
-if [ "${cpu_arch}" != "x86_64" ]; then
+if [[ "${cpu_arch}" != "x86_64" ]]; then
   value="${value} ${cpu_arch}"
   log "- Architecture '${cpu_arch}' added to value."
 fi
@@ -119,5 +122,5 @@ log "- Value hashed as '${key}'."
 log "done"
 
 key_filepath="${cache_dir}/cache_key.md5"
-echo ${key} > ${key_filepath}
+echo "${key}" > "${key_filepath}"
 log "Hash value written to ${key_filepath}"
diff --git a/restore_pkgs.sh b/restore_pkgs.sh
index 4556265..79ac2db 100755
--- a/restore_pkgs.sh
+++ b/restore_pkgs.sh
@@ -6,7 +6,7 @@ set -e
 # Debug mode for diagnosing issues.
 # Setup first before other operations.
 debug="${4}"
-test ${debug} == "true" && set -x
+test "${debug}" == "true" && set -x
 
 # Include library.
 script_dir="$(dirname -- "$(realpath -- "${0}")")"
@@ -18,30 +18,33 @@ cache_dir="${1}"
 # Root directory to untar the cached packages to.
 # Typically filesystem root '/' but can be changed for testing.
 cache_restore_root="${2}"
-test -d ${cache_restore_root} || mkdir ${cache_restore_root}
+test -d "${cache_restore_root}" || mkdir "${cache_restore_root}"
 
 # Cache and execute post install scripts on restore.
 execute_install_scripts="${3}"
 
-cache_filepaths="$(ls -1 "${cache_dir}" | sort)"
-log "Found $(echo ${cache_filepaths} | wc -w) files in the cache."
+# Use find instead of ls to better handle non-alphanumeric filenames
+cache_filepaths="$(find "${cache_dir}" -mindepth 1 -maxdepth 1 -type f -o -type d | sort)"
+file_count=$(echo "${cache_filepaths}" | wc -w)
+log "Found ${file_count} files in the cache."
 for cache_filepath in ${cache_filepaths}; do
-  log "- "$(basename ${cache_filepath})""
+  log "- $(basename "${cache_filepath}")"
 done
 
 log_empty_line
 
 log "Reading from main requested packages manifest..."
-for logline in $(cat "${cache_dir}/manifest_main.log" | tr ',' '\n' ); do
+while IFS= read -r logline; do
   log "- $(echo "${logline}" | tr ':' ' ')"
-done
+done < <(tr ',' '\n' < "${cache_dir}/manifest_main.log")
 log "done"
 
 log_empty_line
 
 # Only search for archived results. Manifest and cache key also live here.
-cached_filepaths=$(ls -1 "${cache_dir}"/*.tar 2>/dev/null | sort)
-cached_filecount=$(echo ${cached_filepaths} | wc -w)
+# Use find instead of ls to better handle non-alphanumeric filenames
+cached_filepaths=$(find "${cache_dir}" -maxdepth 1 -name "*.tar" -type f 2>/dev/null | sort)
+cached_filecount=$(echo "${cached_filepaths}" | wc -w)
 
 log "Restoring ${cached_filecount} packages from cache..."
 for cached_filepath in ${cached_filepaths}; do
@@ -51,7 +54,7 @@ for cached_filepath in ${cached_filepaths}; do
   log "  done"
 
   # Execute install scripts if available.    
-  if test ${execute_install_scripts} == "true"; then
+  if test "${execute_install_scripts}" == "true"; then
     # May have to add more handling for extracting pre-install script before extracting all files.
     # Keeping it simple for now.
     execute_install_script "${cache_restore_root}" "${cached_filepath}" preinst install