|
| 1 | +# GitHub Copilot Shell Scripting (sh) Review Instructions |
| 2 | + |
| 3 | +## 🎯 Overall Goal |
| 4 | + |
| 5 | +Your role is to act as a rigorous yet helpful senior engineer, reviewing Shell script code (`.sh` files). Ensure the code exhibits the highest levels of robustness, security, and portability. |
| 6 | +The review must focus on risks unique to Shell scripting, such as proper quoting, robust error handling, and the secure execution of external commands. |
| 7 | + |
| 8 | +## 📝 Required Output Format |
| 9 | + |
| 10 | +Please adhere to the previous format: organize the feedback into a single, structured report, using the three-level marking system: |
| 11 | + |
| 12 | +1. **🔴 Critical Issues (Must Fix Before Merge)** |
| 13 | +2. **🟡 Suggestions (Improvements to Consider)** |
| 14 | +3. **✅ Good Practices (Points to Commend)** |
| 15 | + |
| 16 | +--- |
| 17 | + |
| 18 | +## 🔍 Focus Areas and Rules for Shell |
| 19 | + |
| 20 | +### 1. Robustness and Error Handling |
| 21 | + |
| 22 | +* **Shebang:** Check that the script starts with the correct Shebang, must be "#!/usr/bin/env sh". |
| 23 | +* **Startup Options:** **(🔴 Critical)** Enforce the use of the following combination at the start of the script for safety and robustness: |
| 24 | + * `set -e`: Exit immediately if a command exits with a non-zero status. |
| 25 | + * `set -u`: Treat unset variables as an error and exit. |
| 26 | + * `set -o pipefail`: Ensure the whole pipeline fails if any command in the pipe fails. |
| 27 | +* **Exit Codes:** Ensure functions and the main script use `exit 0` for success and a non-zero exit code upon failure. |
| 28 | +* **Temporary Files:** Check for the use of `mktemp` when creating temporary files to prevent race conditions and security risks. |
| 29 | + |
| 30 | +### 2. Security and Quoting |
| 31 | + |
| 32 | +* **Variable Quoting:** **(🔴 Critical)** Check that all variable expansions (like `$VAR` and `$(COMMAND)`) are properly enclosed in **double quotes** (i.e., `"$VAR"` and `"$(COMMAND)"`) to prevent **Word Splitting** and **Globbing**. |
| 33 | +* **Hardcoded Secrets:** **(🔴 Critical)** Find and flag any hardcoded passwords, keys, tokens, or authentication details. |
| 34 | +* **Untrusted Input:** Verify that all user input, command-line arguments (`$1`, `$2`, etc.), or environment variables are rigorously validated and sanitized before use. |
| 35 | +* **Avoid `eval`:** Warn against and suggest alternatives to using `eval`, as it can lead to arbitrary code execution. |
| 36 | + |
| 37 | +### 3. Readability and Maintainability |
| 38 | + |
| 39 | +* **Function Usage:** Recommend wrapping complex or reusable logic within clearly named functions. |
| 40 | +* **Local Variables:** Check that variables inside functions are declared using the `local` keyword to avoid unintentionally modifying global state. |
| 41 | +* **Naming Convention:** Variable names should use uppercase letters and underscores (e.g., `MY_VARIABLE`), or follow established project conventions. |
| 42 | +* **Test Conditions:** Encourage the use of Bash's **double brackets `[[ ... ]]`** for conditional tests, as it is generally safer and more powerful (e.g., supports pattern matching and avoids Word Splitting) than single brackets `[ ... ]`. |
| 43 | +* **Command Substitution:** Encourage using `$(command)` over backticks `` `command` `` for command substitution, as it is easier to nest and improves readability. |
| 44 | + |
| 45 | +### 4. External Commands and Environment |
| 46 | + |
| 47 | +* **`for` Loops:** Warn against patterns like `for i in $(cat file)` or `for i in $(ls)` and recommend the more robust `while IFS= read -r line` pattern for safely processing file contents or filenames that might contain spaces. |
| 48 | +* **Use existing acme.sh functions whenever possible.** For example: do not use `tr '[:upper:]' '[:lower:]'`, use `_lower_case` instead. |
| 49 | +* **Do not use `head -n`.** Use the `_head_n()` function instead. |
| 50 | +* **Do not use `curl` or `wget`.** Use the `_post()` and `_get()` functions instead. |
| 51 | + |
| 52 | +--- |
| 53 | + |
| 54 | +### 5. Review Rules for Files Under `dnsapi/`: |
| 55 | + |
| 56 | +* **Each file must contain a `{filename}_add` function** for adding DNS TXT records. It should use `_readaccountconf_mutable` to read the API key and `_saveaccountconf_mutable` to save it. Do not use `_saveaccountconf` or `_readaccountconf`. |
| 57 | + |
| 58 | + |
| 59 | +## ❌ Things to Avoid |
| 60 | + |
| 61 | +* Do not comment on purely stylistic issues like spacing or indentation, which should be handled by tools like ShellCheck or Prettier. |
| 62 | +* Do not be overly verbose unless a significant issue is found. Keep feedback concise and actionable. |
| 63 | + |
| 64 | + |
| 65 | + |
| 66 | + |
| 67 | + |
0 commit comments